API for reversals

[JDGrimes]

Before we can build an API for this, we need to map out exactly what reversals are.

Reversal History

First, some history. From the release of WordPoints 1.0 there has always been some sort of reversal feature. A good history of this is in https://github.com/WordPoints/wordpoints/issues/221. The reversal feature was originally introduced to solve a potential problem: spammers/users gaming the system. It is natural for an admin to expect, for example, that a comment which is deleted shouldn't give its author points.

Reversals Today

There are actually quite a few caveats here, and there have been many quirks in the past, but at the moment, this issue is solved by doing reversals automatically, and entirely behind-the-scenes. There are no settings the administrator has to fill out for each hook, it just happens. So when a user's comment is deleted, for example, any points that they earned for it are removed.

Reversals' Future

The current trajectory of reversals in WordPoints is that they are an automatic background task that helps stop users who try to game the system. On the principle of "decisions, not options", and that spam just shouldn't matter, this seems like a good path to take. It does raise a question though: Might reversals have a broader application in the future, beyond just stopping cheaters?

The answer to this question is no. This is what the reversal API is all about. That is its sole purpose. Any other use for reversals will either have to fit within the API, or use its own.

---

The API

Now that we understand what reversals are, we can talk about what the API should look like.

Essence

Let's start by examining the essence of what it is now:

• An action is called (like delete_post).
• A function that handles reversals is hooked to this action (::reverse_hook()).
• The function retrieves the ID of the affected object ($post_id).
• The function searches the points logs for all transactions of that type associated with that object ID that haven't already been auto-reversed.
• The function then performs points transactions to reverse each of those logs, and then marks them as auto-reversed.

Comment Complexities

The comment points hooks are slightly more complex. First, they check that the comment status is actually being transitioned from approved to something else before firing (because the transition_comment_status hook fires for all kinds of transitions). I think what we really need to do here is find a different action to hook to, one that doesn't require this extra stuff. However, we may want to keep this issue in mind as there might be other actions that have to be filtered before reversals happen. This also has repercussions if we decide to change this from a "comment approved" hook to a more general "comment status transition" hook.

The second added complexity of the comment points hooks is that they save the last status of the comment that was awarded points as comment metadata. I believe that this is a legacy thing that is no longer necessary, and its only current purpose is to keep multiple hooks from firing for a single comment (https://github.com/WordPoints/wordpoints/issues/221#issuecomment-69475705). Therefore this is really a non-issue and we don't need to worry about it in our API.

[JDGrimes]

Comment Transitions

This also has repercussions if we decide to change this from a "comment approved" hook to a more general "comment status transition" hook.

I've been toying with this idea for a bit while working on this. It could cause issues with reversals though, because if the hook transitioned for all statuses, we'd end up reversing the transactions for all previous status transitions for a comment, and that probably would lead to strange behavior when applied to other hooks. Another hook that is in a similar situation is the Post hook.

The real issue here is what a hook should relate to. Should the hook relate to any status transition, with the type of transition being a Constraint? Or, should the hook relate to one specific status transition, like "to approved"?

Hooks relate to actions, so it comes down to deciding what an action is. Is just any status transition of a comment an action, or must an action be more specific than that? Technically speaking, transitioning the comment to approved and transitioning it to spam are two different actions. They might even have different workflows. In a broader sense, status transitions are things that happen to lots of objects in WordPress, and we have to consider that in other cases the flows may contrast even more.

In light of this, I think each action needs to be as fine-grained as possible. "Transitioning a comment's status" isn't an action; "approving a comment" is. It has specific intention and a specific flow.

That said, we also need to consider custom post statuses, and custom post status transitions. Users may want to award points for these, but each one would have to be registered as a separate hook. This is really the main argument for turning the status into a constraint. It does have merit, so we need to decide how best to handle custom post status transitions. Admittedly, WordPress itself doesn't handle custom post statuses very well yet (WP#12706). So I'm comfortable with pushing that decision off into the future.

Basic API Outline

In our API, we have Hooks, Constraints, Args, and Targets. It might make sense to model the reversal API partly on the firing API. The main business logic of performing the reversal would be handled by the Target, which would be reverse fired at by the Hook. The Args, Constraints, and hook Instances themselves wouldn't need to be involved. This would keep the API simple.

I think it is also worthy of note here that not all Hooks are reverse-able. So only those that are will be need to hook to reversing actions.

[JDGrimes]

Universal Reversability

I think it is also worthy of note here that not all Hooks are reverse-able. So only those that are will be need to hook to reversing actions.

I'm actually thinking that this might not be true. I was thinking, when I wrote that, of the User Register and User Visit hooks. It seems like there is no reason to reverse the User Register hook when the user gets deleted, because they'll be loosing all their points anyway. However, we need to consider the case where the hook is used to instead award the site owner on multisite instead of just the user registering. In that case, it would make sense to reverse the transaction if that user got removed from the site later.

The same is true for the User Visit hook. If the site admin is being awarded points for visits and a user gets removed for being a spammer, they should have their points from that user removed.

So I think that in fact, (almost?) all hooks are reversable.

[JDGrimes]

Reversal Descriptions

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. — https://github.com/WordPoints/hooks-api/issues/8#issuecomment-110517923

This presents a problem, because a reversal is tied to a specific entity across any number of reactions, and not to a specific reaction. In other words, we don't know the reaction(s) when a reversal takes place, so we can't access their metadata. This leads us to question exactly how we should handle reversals in regard to points logs, etc.

Traditionally reversals have been logged (they actually started out as separate hooks, after all). Because their job is mainly to deter spammers and folks trying to game the system, logging reversals has been something that I've questioned in the past. The problem is that we don't know whether the affected target user will actually be the one who was spamming/gaming—for example, a reaction might award the post author for a comment by a spammer, and we don't want to confuse that post author when that comment gets removed by vanishing away his points without a trace.

However, I'm beginning to question again whether this is really a valid argument. If we want to stick closely by the purpose of reversals (and we do), then their job is to automatically clean up after spammers. Logging the transaction just for the spammer may not be necessary. In fact, it brings them attention. Any use of reversals that needs the reversal to be logged should probably come under suspicion as a misuse.

So I think there is an argument to be made that logging reversals is just asking for them to be misused for other purposes that they really aren't meant for.

The main concern is that users will see points disappear, and not know why, at times when there is really no spamming involved. However, if that happens, isn't it an indication that our spammer/gamer detection is faulty?

I think it comes down to this. We originally did this with spam in mind. And that is the road we've continued to go down, in theory. However, at the same time, we've fuzzed the lines between a spammer/gamer/guy-who-wrote-a-post-for-a-specific-purpose-and-later-deleted-it. In other words, we've made points ephemeral things that only exist as long as the entity. But that net is much broader than is necessary to catch the spammers.

In one sense, I think we've made the reversal API into a retroactive API of sorts, and turned points into badges. You get a badge that says, "I have 100 posts currently published on the site." If enough get removed that the number goes below 100, the badge goes away (don't confuse this with the badge that says "I have published 100 posts"). As you can see, not even all badges should behave that way, and it isn't a good design for a points system. Because badges can't be "spent". Points can. And once you've used them, you've used them. When a comment get's deleted and the user has 0 points because they just bought something with them, we can't go back and undo that transaction so we can pull our points back. It just doesn't work that way.

I think in the future we should travel down the path of allowing points transactions themselves to be marked as spam/reversed by the moderators. (We could even have a moderation queue, though probably that would be overkill for most sites.) The point is, we can't identify a "spam" post. We can identify a "spam" comment, and possibly some other entities that actually have spam actions that relate to them. So I think the path forward is to limit ourselves to only reversing the things that we can positively identify as spam, and offering a different means of handling stuff that we can't (if needed).

(We're also introducing another way to combat spam, via the periods extension.)

That said, we need to decide how to do this in a backward-compatible manner (or should we?). We also still need to decide whether to log reversals (and I think really these two things probably go hand in hand).

[JDGrimes]

Backward Compatibility

So, let's first answer the question, do we need to log reversals for hooks we import from the old Points Hooks API, to maintain backward compatibility for the users? An alternative would be to not import them at all, if we thought we needed to continue logging the reversals.

Basically I have envisioned all of the core points hooks being imported to the new hooks API, while any custom hooks would have to be left behind. We would continue to display the old points hooks screen only to users which still had some old points hooks using that API. Perhaps we'd even go so far as to stop providing the old forms of the hooks there. I think it would make sense that we'd stop registering the core points hooks by default once they are no longer used.

However, we have to consider the back-compat that has been in the comment and post points hooks since WordPoints/wordpoints#221 and WordPoints/wordpoints#256, respectively. They allow auto-reversal to be disabled on legacy installs. So we need to decide whether to still import those instances where auto-reversal is disabled. Because it isn't backward-compatible, it will cause problems. We need to either not import those, or make it possible to disable auto-reversal per-instance in the new API as well (at lest for legacy installs).

Because this is really a points-only piece of API, we probably could actually just import the auto-reversal disabling for legacy sites, and handle it using the points reactor. We'd need to decide whether or not to expose the option in the UI still, though.

I say it is points-only, but we need to separate the spam API from the reversal API, because the badges might need to have reversal actions, though maybe not spam actions. That is, other reactors, like badges, may want to remove a badge if an event is reversed, regardless of the reason.

However, one reason that we might not want to import the non–auto-reversing instances at all is that some of them may be paired with comment delete hooks. We can't import the comment delete hooks, so if we imported the matching comment hooks we'd end up with the comment delete hook on the old screen and the matching comment hook on the new screen, which would probably be a pain for users.

For such installs, our best way forward is probably to just not import those problematic points hooks for now. Down the road, when we get ready to remove support for the old screen entirely, we can do an emergency import on any points hooks that are left there, and just do our best to maintain back-compat.

Now, to the unpaired auto-reversing comment and post hooks. There is no reason that we shouldn't import those, unless it would have to do with the reversal logs. Users would be used to reversals from these hooks being logged. But since we're not really talking about legacy installs, there would never have been any public facing manifestation of that in the UI; it would have just been something that happened behind-the-scenes. Does respect for backward-compatibility mandate that we continue to log reversals for these instances? Will the user miss this feature? More specifically, will it break the intended use of their hooks?

Because this hasn't been manifested to them in the UI, the users may not even be aware of reversals at all. So I don't think we need to worry about logging reversals for legacy reasons.

[JDGrimes]

So we need to:

• [x] Separate the reversal and spam APIs.
• [x] Maybe stop logging spam reversals.

[JDGrimes]

Actually, I was forgetting that if we want to continue logging the post reversals, we'd have to make special provision for that once the spam and reversal APIs are split, since spam doesn't apply to the posts by default.

[JDGrimes]

Lets stop logging spam reversals for now. If users complain, we can bring it back in a future version. Instead, we'll just delete the original log.

[JDGrimes]

Note that in terms of back-compat, this will change the reversal flow for existing users as well. Now the comment will need to be marked as spam, not just deleted, in order to have the spam API pick it up. But it would be easy hook the event back to the comment disapprove action, if desired, for legacy users.

[JDGrimes]

News of this change was posted on the WordPoints.org blog, and received positive feedback.

I think that we a basically done here for the initial version. What is still needed is to:

• [x] Decide whether to include the reversal code in the initial version (as opposed to only including the spam code).

I think if there is anything else here, it needs to wait until it is actually needed ;)

[JDGrimes]

Decide whether to include the reversal code in the initial version (as opposed to only including the spam code).

Note that we also need to decide whether to continue registering the reverse actions for events. I think that it makes sense to continue doing that even if we don't include the reverse-related code, because there are many potential uses for reversals down the road. I'm not sure what we'll do with the badges reactor, for example, but it might use reversals and not just spam actions.

I just happened to think that I wonder whether we might have issues if some reverse and spam actions overlap. (I think that the comment_deapprove and comment_spam actions overlap, for example.) But I don't think this would end up being an issue, because we'd probably need to keep track of comment IDs anyway and make sure we didn't double-reverse for some reason.

[JDGrimes]

I see no downside to including the reversal code in the initial version, so I'm going to go ahead and close this. If we come across a reason not to include it before release, we can change our minds.