Closed JDGrimes closed 9 years ago
With the creation of hook descriptions, we run into the language barrier. We have to deal with it, either when the hook settings are being edited in the UI, or later when the hook is being fired, or both. There are two approaches that we could take. One is to attempt to have the code generate the description from scratch. However, the problem with that is that it isn't localizable. The second option would be to let the user specify the description themselves. And we do that already (though those descriptions are not for the logs but the HTGP shortcode). But in order for it to offer the user the ability to use things like the post title in a description, we'd need to introduce some kind of place- holders. The problem there is again that placeholders aren't very translatable. We could work around that using JS to prettify the placeholders, though.
One possible solution to all of this is to continue to put the responsibility of writing descriptions on the developer. The new API would just make the hook code more composable. However, this could potentially be a severe limit to the user.
Generating an transaction-specific description is a problem confined mainly to the points component. Ranks and badges wouldn't need that. So perhaps we don't need to make it a part of the API. However, we should consider the possibility that other future uses of the API may also desire this ability (though we could just wait and burn that bridge when we come to it). Note that we don't need to worry about the existing hooks, we can probably generate a description template when we update.
The general instance-specific description is something that it required by the points component, and is currently not needed by the ranks component. It probably would be used by badges. As noted above, the general description may be supplied by the user. This is a feature we'll have to keep for the points component, and perhaps we really should consider making it the user's responsibility to write a description. We could still provide defaults, though they might not be very useful.
The only way that I can think of to escape the i18n issues is to have either the user or the developer write descriptions for every possible combination of placeholders. If we don't use placeholders, the developer has to do it. But if we do use placeholders, the user still has to do it, because of log text regeneration.
Possible solution: stop regenerating logs. Sounds kind of scary, but logs are logs after all, and aren't really meant to be regenerated. One reason that we did it was to avoid broken links in the logs. But we could fix that by never adding links to start with. Also, that is really and edge case; most of the time the broken links will only be in really old logs no-one will ever see. Another reason we regenerate is in case a post gets deleted because it had a bad word in the title. That's an edge case though. It could also be solved by letting admins delete log entries, although they arguably shouldn't have to do that. (But again, logs are logs.)
Another alternative to regeneration is to delete the logs instead of regenerating them, or to just have a single, generic fallback rather than a complex series of progressive degradations.
Note: log regeneration isn't a problem if we go the no-API route, because there will be no placeholders, and thus nothing to regenerate.
This is going beyond the scope of this API, but one thing to keep in mind is multilingual sites. Because the logs are static, they aren't really compatible with multilingual plugins, because each log entry would have to be translated separately. If we're going to have to change the way logs work at all, we should do it in a way that will allow them to be translated on-the-fly. Which would mean that we stop storing log text, and the problem of regeneration goes away.
On the other hand though, this makes network-wide logs more difficult. Though really, I'm not sure if network-wide logs have many real uses. And because the logs are all stored in one table, it is not impossible to display them from across the network.
But if we stop storing the logs, what do we have to store to be able to generate them? We can store the ID of the main object, and that goes a long way, but it isn't necessarily sufficient. The current query, for example, doesn't necessarily have an ID. But that is kind of a special case.
Note, however, that none of this actually solves the log regeneration conundrum, because now the logs are "re-"generated every single time they are displayed. However, in this case there are only two possible scenarios: either the stored ID matches an existing thing or it doesn't (with the exception of the current query). In the first case, we can use the template. In the second case, we can use a generic fallback.
But the question then becomes, "Where do we get the template from?". And the answer is... I have no idea. If we do the no-API or placeholders API, we must store the instance ID. But then if an instance gets deleted, the logs for it would no longer display (maybe not a bad thing?).
And a more general issue with the dynamic logs idea: what happens if the code that generates those logs gets removed? Two thoughts in regard to that: 1., maybe we don't care, 2., maybe we shouldn't display them anyway, because the code that hides them from users with insufficient caps won't be there either (I assume; that API hasn't been explored yet).
Getting the template from the instance ID with the no-API or placeholders API, we would need to make it possible to switch the template for a translation before it was used.
Another issue with dynamic logs, is that the log text is no longer searchable.
In the end though, affecting log storage is beyond the scope of this API. Also, I've just realized that even with regeneration and static logs, we still only need two versions of the logs text, the one with placeholders and the basic one.
Another aspect that we need to explore is what would happen when the settings of a hook instance change. This is only a problem with the placeholders and developer APIs, because there is no regeneration with the no-API.
With static logs, we only need to get the template when the logs are regenerated. If the instance was removed completely, we just wouldn't be able to regenerate the log.
But what if the instance still exists, but the settings have changed? If a Content Publish hook changes from awarding for just Pages to awarding for all content types, there is no problem, because the latter is more generic. But if it happens the other way around, the log might specifically mention the word "page", when in fact the content being awarded could have been of any type.
A possible solution would be to store the old version of the generic log. At first it might seem that this would cause lots of bloat in the database. But we wouldn't need to store the new version each time the hook instance was edited, only when the generic text was changed.
The alternative would be to stop regenerating points logs.
With dynamic logs the problem would be worse, because the logs are constantly being generated. If the instance had been deleted, we couldn't display the log at all. But there is no reason why we shouldn't keep displaying the log (as long as we can still determine whether the user has the necessary caps).
So in that case, we would need to store a copy of the generic template anyway.
And not generating the logs at all wouldn't really be an option.
The developer API can gracefully degrade in 2 steps as the placeholder API can (or use more steps, if desired). The problem isn't handling, e.g., the post title, being in the template. The problem that would have to bee solved is how to handle all of the possible target users (for example, in the comment hook: comment author, comment replied-to author, post author, site admin). Also, to handle all possible periods and rates, etc. So the problem is not incorporating the attributes of the main object the action affected, but the other settings of the hook.
The problem with the targets could conceivably be solved by requiring hooks to be target-specific. This would require each hook to still specify the action and the target, like it currently is. This would cramp the API for users, and require more work by developers (though it would be pretty easy). However, it would simplify the logs problems, and possibly the UI as well.
The problem with periods might not be a problem at all. The question is really whether the period belongs in the log text or not. The Periodic points hook does include the period: "Daily points", "Weekly points", etc. However, it could just be "Visited the site." It could also be "Visited the site :rate times per-:period_name." But I think that really the period wouldn't usually be transparent. So maybe special text for periods should be optional, and only added by developers where they need it.
The problem with rates is that the log message would be "Commented on :rate posts" instead of just "Commented on a post". However, handling of plurals is already made very simple by the i18n API provided by WordPress. The problem though, is that the rate can be based on one or more arguments. Because this is so variable, I'm not sure it is feasible to have text for each.
Without period texts, the Content Publish hook text might look like this:
Published content. | Published :rate pieces of content. Published a :post_type. | Published :rate :post_type_plural. Published :post_title. | ditto.
For the Leave Comment hook:
Left a comment. | Left :rate comments. Left a comment on a :post_type. | Left :rate comments on :post_type_plural. Left a comment on :post_title. | Left :rate comments on :post_title.
And for other rates:
Left 5 comments on posts by the same author. Left 5 comments that were replies. etc.
I'm wondering if a possible solution would be to split the description of the action that awarded the points from the description of the hook instance that decided to award those points. We already do this for admins, I know, but I'm talking about with the logs themselves. So you'd see in the logs that you received x points and that it was related to a certain comment, and then you also see the details about the hook that awarded the points.
10 points | :comment_link on :post_title | Awarded every time you comment 3 times. 5 points | :comment_link on :post_title | Awarded every time you comment. 3 points | :comment_link on :post_title | Awarded every time you comment on 4 Media by the same author.
The main issue I see with this is that in the first and last examples we might not want to be so specific about a particular comment.
The problem is that we'd still need to make the log text customizable by the user. Either that or we need to make the API so that the developer can further customize the log text.
Consider using the Leave Comment hook to award points for a reply to a forum topic. While the text might usually be something like "Left a comment on a Topic", that might not be what the admin would want (though honestly I thought the problem was worse than that).
Anyway, this could be achieved through filters.
In short, the UI APIs for the logs are beyond the scope of this project, and should be avoided unless absolutely necessary. That is, we should only go there if crafting a sane developer API proves impossible.
Should rates be a part of the points hook API? Within the points hooks API, an action has always affected a single object. Rates would make this no longer so. It makes more sense not to expose the rates part of the API to the points hook UI. We will need it for badges, but for that we can probably wait and build the rates API later when we actually need it.
I think we also need to remember that points transactions can come from things other than hooks. So hooks can't dictate the points logs API. They are only one thing that is used to award points. Right now they are the main thing, but points can be modified directly too. There is room for the logs API to mature, but we can't make that a part of this project. It can follow later, but if doesn't have to be done now, it shouldn't be.
I'm leaning back towards the developer API. We just need to:
First the latter. Limits and expirations aren't a problem, because they don't need to be exposed in the logs anyway. The main things to consider are reversals and constraints.
Constraints are really the killers. The problem is that we can't possibly imagine every possible constraint and hook combination. So we have to decide whether constraints should be a part of the log text. If it isn't needed, we're OK. If it is, we need to decide if it is possible to do through the developer API without breaking translatability.
It seems like most of the time constraints should be part of the log text. So let's consider whether it is possible to achieve that without the UI APIs.
There are basically two ways to approach it: we have to have the developer write every combination, or build the text in some way.
The developer can't write every combination, because new hooks and constraints can be added. If we do force the developer to write the log text for every combination, we'll also be forcing the translators to do lots of work. But I think it might be possible to do it, if each constraint had to be registered for each hook. This is limiting the API again, but it makes things easier when it comes to the UI.
If possible, it would be better to build the text somehow. But we have to find a way that is translatable. In that case, we can't string different pieces of sentences together. We'd have to instead have separate sentences for each constraint. So instead of saying, "You left a comment on a Topic in the fish category," we'd have to say something like: "Left a comment on some content. The content was a Topic. The content was in the fish category." Possibly there would be a way to do it where this information would be conveyed to the user, but not in so many words. But again, we'd rather not modify the logs API if we don't have to.
Reversals occur when the action is undone. I don't think this represents any challenges beyond those above, although it does duplicate them under the current reversal and logs APIs.
Something related to the logs API and user caps, is that once an object that a log relates to is deleted (e.g., a post), it may no longer be possible to determine whether a user has the caps to view it. Therefore, the logs would have to be hidden/deleted if they are not regenerated. Possibly there are also cases where they should be removed even though we do regenerate the logs.
I'm really not sure there is a sane solution to the dev API conundrum. So let's see if the UI API problems are simpler to solve.
- The user has to learn what contexts the logs will be used in and write the logs appropriately. For example, the logs are displayed to the current user, but may also be displayed to all users. The logs may be displayed at the time the action occurs, but may also be viewed much later.
This is just a matter of user education. It is also possible that users may not need to provide for all of these contexts, depending on how they are using the plugin. Also, we can steer them in the right direction with the basic defaults (see below).
- The two (or three!) different description templates could be confusing.
If done properly, this issue should be minimized. It already exists anyway, because the current points hooks UI has the "description" field, with no explanation of what it is used for. So the new UI would certainly be an improvement, if anything.
- The user may be bored having to write these from scratch every time.
But they won't have to if we offer simple defaults. They'd only need to customize the text when they wanted to use more complex constraint settings (and even then it would still be optional).
I might have mentioned this before, but it might be possible to introduce a developer API for the logs later, so the UI API only needs to be a starting point. The question now becomes though, how do we implement it? With or without placeholders? Actually, this too could be a progressive enhancement that doesn't have to be a part of the initial version.
The main concern with not implementing placeholders is that the settings of the hook would be more likely to get out of sync with the log text. For example, if a Content Publish hook changes from awarding for just Pages to awarding for all content types, we run into the problem of "pages" possibly having been hard-coded into the log. But perhaps the UI could help to minimize this issue. Introducing placeholders only makes this less of an issue; it can still happen just as easily if the user doesn't use the placeholders. Placeholders actually double this problem, because we also have to worry about regeneration, and so we have two log templates.
Without placeholders is by definition simpler. We should go that direction for now. We can always add placeholders later.
The main question we need to answer right now, then, is whether this is how these settings should be stored. Should they be a part of the generic hook settings, or do they need a special place. The hook settings are sort of for the target-specific settings. So if we put it there we're kind of limiting it to the points target. But we will want descriptions in the badges target as well. We need to define the purposes of each of these descriptions more generically though, and decide if the badges component will really be using the same type of description.
The purpose of the first description is to describe to the user how they can trigger this event to happen. It's predecessor is the How to Get Points shortcode, which displays a list of ways that you can earn points. For badges, we might indeed want to list all of the available badges.
The purpose of the second description is to describe to the user the action that triggered the event. This is used in the points logs. I said above that we wouldn't need it for badges, but though it might not be used in logs, it would be displayed to a user with the list of their badges.
The main difference is the tense of the messages. I have been calling them the instance-specific and transaction-specific messages, respectively. And I think that instance_description
and transaction_description
would be good identifiers for them.
Because they are generic, cross-target settings, they should be stored accordingly.
For the points target we also need the reverse-transaction description. We don't currently need it for the badges target, but if we add logs, we'd possibly use it.
We may still have to tie hooks to specific user targets. It is hard to create default descriptions of an action so generic they apply to any possible target. At that point they aren't really much help to the admin either. However, that is really a separate issue.
After some discussion in https://github.com/WordPoints/hooks-api/issues/2#issuecomment-142934630, I've decided to stop logging reversals for now.
Because they are generic, cross-target settings
Despite the fact that these same sorts of things might be used across different reactors, I think we should have each reactor handle them separately for now. There are probably all kinds of niche use-cases here that we've never thought of, where storing a description wouldn't be needed. Let's make the API as flexible as possible, we can always tie things down/provide more bootstrap later.
As a part of implementing this, both the Action and Event objects were given a get_description()
method. I think that maybe the idea was that the Event description would be the instance-specific, and the Action description would be the transaction-specific. However, looking at them now, I'm just wondering whether we really need both Actions and Events to have descriptions. And in a broader sense, whether we need both Action and Event objects at all.
Currently the Event descriptions are being used as the default instance-specific descriptions in the UI. The Action descriptions aren't being used yet, but the plan was to put them to use as the transaction-specific descriptions (aka log text) in the UI.
I was beginning to think that maybe we should just move both descriptions to the Event objects. However, as I began to think about spam and reversals (#2), it seemed better to keep the descriptions on the Action objects, since that would make it possible to add a spam/reverse/other action to an event and provide a description, without having to modify the event object. Otherwise the event class would also have to be replaced to make the added fire type describable.
Of course, our current implementation of the points reactor doesn't actually use any descriptions when an event is spammed. But it is possible that we might want to use them in the future.
This leads us to ask whether we should be storing descriptions on the event at all—perhaps we should just supply the per-instance description to the Action objects as well. Or, do we really need both types of description?
The difference between the two descriptions (instance-wide and per-transaction) is mostly the tense: the instance-wide should be passive tense ("Publishing a post"), whereas the log text has traditionally been past-tense ("Published a post"). However, the log text can really have either tense without appearing too strange. We may want to keep in mind that log text may not be the only way that a transaction-specific description would be used.
I think we could honestly ask whether we really need to supply defaults for these at all. We decided to do so above as a means of prompting the user as to which tense to use, and to make the process of adding a reaction easier, since it could save a step for the user. However, as noted above:
It is hard to create default descriptions of an action so generic they apply to any possible target. At that point they aren't really much help to the admin either.
"Comment", for example is the broadest description we can give of a comment being left on a post, and yet is isn't very useful. It could be used, but would it be? The defaults might actually be more annoying than having to learn to write your own descriptions.
To summarize the problem, the traditional descriptions relate the action to the user in some way, but we can't do that with these defaults. And a description of the event itself isn't always that helpful to the user in crafting a sentence that relates the action to the user, because there can be so many different possible targets of the action, and we don't even know what they all are beforehand since relationships can be added by modules.
There is also a sense in which we are trying to make a square peg fit a round hole. The event description isn't being used in the UI to describe the event, it is being used to describe a particular reaction to that event. And the action description isn't being used to describe the action, but a particular reactor transaction in reaction to that action.
Here are the Event descriptions that we currently have:
Publishing content. Leaving a comment. Registering. Visiting the site.
And the Action descriptions:
Published content. Visiting the site. Account deleted. Registration. Content deleted. Comment removed. New comment. Comment approved.
Notes:
Visiting the site
isn't past tense like the others.New comment
isn't really a description of the action but of the result. Commented
wouldn't actually cut it either though, it wouldn't be broad enough (assumes the user awarded points left the comment).{entity} {acted upon}
. So Published content
is too broad, and should be Content published
instead, to not link it to a particular target. And Registration
could probably be changed to User registered
, although that does sound a bit odd when the user views it in their own logs.Considering this, we'd need to modify all of the Event descriptions if we keep them. To help make that decision, let's consider what we might modify them to, and whether it will be significantly different from the Action descriptions.
For starters, lets just take one Event description, Publishing content
. We need to change this to be broader, while maintaining the correct tense. Let's define the exact tense that we expect here.
The descriptions descend from the How To Get Points shortcode, which is basically supposed to answer the question, "How do I get points?" So each of the descriptions is meant to tell you one way to get points. I've thought of each of them as an ending for the sentence, "You can get points by..."
You can get points by... Publishing content
, for example.
The format we've being using is {doing this to} {entity}
.
Unfortunately, this brings us to a dilemma, because that format by nature assumes a particular user is doing the acting. That a particular user is doing.
We could change the format slightly to alleviate this, but back-compat does need to be kept in mind, because users will be introducing the shortcode with some text (either part of a page or the page title, etc.) on their site.
Frankly, without tying the Event descriptions to particular targets, they are useless, because they don't actually tell you how to get points. Ceasing to offer defaults will be a change though, because we currently do. Of course, we'll have to import this anyway so we can import the defaults current users are using. It just might be a change for some of them in the future if there is no default anymore, especially since it is a required field (or could that change?).
However, supplying default descriptions for every possible target is literally impossible, because it is impossible to anticipate all of the relationships that entities will have, etc. That doesn't mean we can't provide defaults for some of the possible targets, however, I'm not sure we want to start traveling down a road that we know has no end.
…it is a required field (or could that change?)
I really don't think we can keep from making this a required field, since it is currently the thing used to differentiate the different reactions to an event in the UI. That could change in future, but I think it is a bit drastic for this release.
Providing default reaction descriptions in the UI is, in short, to much trouble to go through at the moment relative to the value of what is gained. I'm not saying that we'll never want to consider it again in the future, but right now it isn't worth it.
Now let's consider the action descriptions again. They should all pretty much follow the format {entity} {acted upon}
. If we followed that rule strictly, we'd end up with something like this:
Content published. Content deleted. Site visited. Account registered. Account deleted. Comment added. Comment approved. Comment removed.
Notes:
Site visited
seems a bit strange. It works well for the site owner, but almost seems to exclude the site visitor (who also happens to be the most likely target).Account
instead of User
seems somewhat more natural, however, I'm not sure that it follows WordPress lingo closely. It could also potentially conflict with other uses/definitions of the word "account".Comment added
matches the pattern, but I actually think that I like New comment
better. Comment left
is a bit closer to the pattern, but I don't like it either.In short, while these are, in theory at least, broad defaults that would be usable as such in the UI, they don't really fit my definition of "useful". They are very formal/impersonal. And, by definition they have to be that way to be broad enough to be usable. They have to just tell you what action took place, not how that action related to you. If users did leave all of these at the defaults, things could definitely become confusing.
Keep in mind though, that these aren't just to be used as defaults. When it comes to the spam/reversal of events, we aren't tied to a particular reaction at all, so the "default" is all we have. Comment removed
for example. Interestingly, I just realized that when it comes to reversal, we don't know how the target was related to the entity anyway (we don't currently save that information, though we could). But reversals aren't even being used in the API right now, so retaining this just for them, when we don't even know how they will be implemented, might be a bit of a stretch. If we decide not to use them in the UI, I'm not sure it would make sense to keep them around for theoretical uses in the API. We could always introduce them again later.
For some reason, ripping out the Action descriptions still seems less straightforward than ripping out the Event descriptions. Yet they really suffer from the same problems. It is odd that we would try to provide the default for the log text, when we've already admitted that we can't possibly provide the default for the description. I think that no matter how hard I try, I still get hung up on the reversal log text (which we don't even use right now).
So let's just focus on the other descriptions for a moment:
Content published. Site visited. Account registered. Comment added. Comment approved.
Content published
is lame, and can't assume what type of content it is. Probably it would get changed 85% of the time.Site visited
doesn't match the primary use very well either, probably it would be changed 99% of the time.Account registered
does sound a little bit strange when you think about it, and might not match the terminology a particular site uses. Probably would get changed 60% of the time.Comment added
andComment approved
. Ah, here's a nice conundrum. Only one of these will be shown as the default in the API. Yet we have no way of really knowing which one will likely be the most accurate on a given site (or, OK, we might just not be checking the settings to find out but still, the point remains that using one or the other assumes that one of two actions will happen, when the comments can actually get added either way). Anyway, more on that in a moment.Comment approved
is the one currently shown in the UI. Probably it would be changed 95% of the time.
Now, let's be clear here. Things may look bleak in terms of percent usage, but that isn't necessarily our only goal. We can say for certain that these defaults aren't particularly useful though, and that there is really know way they can be made more useful without major new API changes that won't be happening for version 1.0.
The other goal of the defaults, as stated above, was to help steer the user in the right direction in terms of tense and things when they are writing the log text:
The user has to learn what contexts the logs will be used in and write the logs appropriately. For example, the logs are displayed to the current user, but may also be displayed to all users. The logs may be displayed at the time the action occurs, but may also be viewed much later.
But do these logs really provide that guidance, or do they just tell the user "logs should be bland and unexciting and not actually provide much info to the user." :-)
As I replied to myself above:
This is just a matter of user education. It is also possible that users may not need to provide for all of these contexts, depending on how they are using the plugin.
Users will have to learn this anyway, and offering these as defaults won't really help them there. A more full-fledged API could provide more verbose, finely-tailored defaults, which might actually help the user. But that's beyond current scope. Again, we could revisit it.
So, I propose that we allow the log text to be finely handcrafted, just like the description. The current state of the API means that we gain relatively little by offering defaults. If we find that users would find it helpful in the future, we can introduce a better API then.
We need an API to generate a description of a specific action, like for the points logs.