Ignore reverses before hits

[JDGrimes]

From https://github.com/WordPoints/hooks-api/issues/66#issuecomment-167881161:

We could then do very generic stuff like prevent a reverse occurring when a regular fire never occurred. Though I'm not sure that that would be the desired behavior across all reactors. Also, what about when new reactions are added? they still might get a reverse directed at them first before a fire even with such handling in place. So that would still need to be handled on a per-reaction basis. And a properly constructed reactor should already take all of that into account.

[JDGrimes]

The main question is whether there will ever be a reason that we'd want to have a reactor that needed to listen for reverses even when no fire/hit had originally occurred. But that doesn't really make sense. The design of the API is such that the reverse fire is intended to take place only after a fire has actually occurred (so for the spam fire). If we're specifically listening for the reverses, what we really want is an inverted event. The reverse and spam fires don't tie the event to particular reactions, so it isn't possible to build a reactor that just works inversely. We can't award points for the reverse fire and subtract them on the regular fire, because we wouldn't know how many points to award. We'd have to pull up the reactions and simulate the handling that happens in the regular fire object within the reactor. Also, the event object would have the wrong title and description. So what is needed is really a different event.

This does lead me to question whether we should be handling the reversals the way we are though, not tying them to particular reactions. If we were to put a hit log in place, we could know exactly which hits were being reversed, and pass that information on to the reactor.

The main point is, that by definition, as our reversals should only occur when a prior hit is being reversed. Note that this is about a prior hit being reversed, not just any fire. There is no reason for us to notify the reactor of a reverse when no hit actually occurred on prior fires, because it won't even have been notified that those fires occurred at all.

In short, a reversal should only be fired after a hit, and this must be determined per reaction (since hits occur per reaction).

This means that it would be fine for us to abort a reverse fire at the router/firer level if no hits had occurred. Though, as noted above, special handling will be needed when there are some hits but not for all reactions. I suppose this wouldn't really need to be special handling though. We could just leave this to the reverse firer. It would pull up a list of hits being reversed and notify the reactor of each one. When there are no hits for a reaction, no reversal hit will be sent.

[JDGrimes]

What happens when a new reaction gets created midstream? So there is a hit, and then a new reaction gets created, and then there is a reverse. This works properly since only the hits will be reversed, and a new reaction won't have a hit so it won't be reversed.

[JDGrimes]

There is one thing here that still needs to be addressed, and that is that currently we aren't performing reversals for deleted reactions, which is in conflict with the conclusion of #70.

[JDGrimes]

Reversals and Nonexistent Reaction Groups

We also skip the reversal if the reactor or reaction group isn't found. As far as the reactor itself goes, this is a necessity, since it is the reactor object that performs the reversal. So we can't possibly perform the reversal without the reactor object anyway.

But what about when the reaction group is no longer registered? Should we continue to perform reversals then? I suppose that perhaps we could, in theory at least. But I'm apprehensive that it might lead to issues with context, etc. In fact, the reason that a reaction group isn't available could indeed be because of context and not because it is no longer available at all.

Actually, I guess we've never really addressed the issue of context and reversals much at all. What happens when a hit occurs in one context but is reversed in another? Is that even possible, due to the contexts of the entities involved? Currently, what happens during a fire if the context changes or if some of the args for the event come from a different context is undefined (#63). So we don't really need to worry about a reversal occurring out of context at the moment, meaning that this point is moot, at least for now.

So at present the only reason that a reaction group wouldn't be available at the time of the reversal would be if it was not being registered anymore.

In any case, what this means is that we cannot get the reaction group object. As a result, we can not construct a reaction object as we might otherwise do, because the reaction constructor requires a reaction storage object. And why do we need a reaction object, do you ask? Because currently a WordPoints_Hook_Fire must be constructed with a reaction object. So to support reversals for nonexistent reaction groups, we'd have to create some sort of special reaction group stub object. That just seems like the wrong way to go. Also, I'm unconvinced that it would really ever be expected behavior to run reversals for nonexistent reaction groups.

So let's ignore that possibility for now.

[JDGrimes]

But we still have to decide about reversals and deleted reactions. I suppose that we could just construct the reaction object with the ID we have from the hit log and the reaction group object. But then I'm really not sure how the reactor would identify it as a nonexistent reaction, since it would have an ID and everything. I suppose they'd have to go to the trouble of retrieving the reaction storage object and calling the reaction_exists() method with the reaction ID, which seems like a lot to go through.

I suppose that we should really just construct the reaction object with an empty ID. But then when it comes time to log the reverse hit that empty ID will be used for the value of the reaction_id field. That just doesn't seem right to me, though perhaps it really is since the reaction doesn't exist anymore.

Right now I am leaning against this, and just leaving it like it is. Really it is sort of strange continuing to reverse hits even when the reactions no longer exist. If we really want to do that we could introduce a custom firer for it. Right now this just doesn't "feel" right. Let's wait a bit before deciding.

[JDGrimes]

In https://github.com/WordPoints/hooks-api/issues/108#issuecomment-192738803 we've decided that the conclusion from #70 no longer makes sense after #98.

Instead, what should happen is that the reaction should be partly deactivated/disabled in some way if part of its operation is to continue. Exactly what that looks like in the UI is up to a particular implementation to decide.

I should also note that if we really want to do something like #70, we can still do it, but not with the firer API. In other words, it wouldn't be part of the hooks API proper.

[JDGrimes]

Leaving this open since the work in this issue itself may be overturned by #108.

[JDGrimes]

We do want to ignore reverses before fires in the points reactor, but this has been taken care of as a side-effect of #99. So we can close this as fixed.