Only reverse hits from the previous matching fire

[JDGrimes]

In https://github.com/WordPoints/hooks-api/issues/98#issuecomment-183686044 it was brought up that we're reversing all matching hits that have occurred in the reverse firer, instead of only those that were triggered by the last toggle of the entity.

An explanation of why this is now a problem is given in https://github.com/WordPoints/hooks-api/issues/98#issuecomment-183764400:

The toggle events are events that occur when the status of an entity is toggled. The event cannot have an intermediate state, it either has one or another. Actually, there can be many different status, but it can only have one at a time. We might call these quantum events because they can only occur in discrete quantum leap-like changes occur in an entity.

Anyway, the point is that the events flip back and forth between an "on" state and one or more "off" states. We don't really worry about which "off" state the entity is in, we just define the reverse as the transition away from the "on" state to something else (anything else, really).

So when a reverse occurs, it is reversing a particular toggle "on" event, not all "on" events that have ever occurred.

Normally we wouldn't have to worry about this, because any "on" events would always be reversed. Each time a reversal occurred. But if we allow for extensions to prevent a reverse fire from hitting (that is, from reversing the hit), that no longer holds true, and requires special handling from the reactor, to maintain the correct behavior. And so the reversal of only the latest hits (the hits from the last toggle "on") becomes necessary.

The biggest hurdle that we have to overcome to achieve this is figuring out how to identify which hits were from the previous matching fire, and which are not. We can't rely on the hit logs, because it is always possible that the previous fire was a miss for that reaction, and not a hit.

Of course, because every fire will be followed by a reverse fire before another fire occurs, it would be possible to set some sort of flag in the database when the reverse fire occurs, for hits that weren't actually reversed by it.

[JDGrimes]

Of course, because every fire will be followed by a reverse fire before another fire occurs, it would be possible to set some sort of flag in the database when the reverse fire occurs, for hits that weren't actually reversed by it.

So what I'm thinking is that we'd pull out all of the non-superseded hits, and as we looped through them, any that weren't hit would have the superseded_by flag set to 0, rather than just being null.

However, I'm wondering whether it would be better to add a column to the table specially for that purpose, or—and this is more extensible—introduce a hit log meta table. I have a feeling that we'll have other uses for a meta table, at least in the future. Possibly even right now with the Periods component.

The main issue is that a meta table would be a performance hit that would make the queries more complex. On the other hand, reversals generally won't be happening nearly so often as regular fires will, so performance isn't really a huge issue.

The biggest thing to consider is whether the superseded_by column should be used this way, and whether indeed it should really be in the hit log table at all. The hit log table should really be a somewhat firer-agnostic thing. Any firer should be able to use it. But the superseded_by column is sort of something that was added just for the reverse firer. There doesn't seem to be much need for it beside that. So perhaps we should be considering moving that column out of the table completely. Or, if we decide it has more general usefulness and should stay, that would indicate that we probably shouldn't be doing funny stuff like this with it.

[JDGrimes]

This was fixed in 617bc1f, which also moved this information into the new hit meta table.

[JDGrimes]

617bc1f has basically been reverted by 41f8be5, although we'll probably ultimately want to maintain this behavior in the points reactor. Exactly where the logic should go though hasn't been decided yet. I'll reopen this as a reminder until it is.

[JDGrimes]

This is something that we'll be using across reactors, so I think it should go into an extension.

However, extensions don't currently listen for misses. I'm not sure when I removed that or why, but the after_miss() method wasn't a part of the initial commit. :-(

I did find this reference in https://github.com/WordPoints/hooks-api/issues/108#issuecomment-192728869:

But though the particular logic in question does not raise an issue in and of itself, it does bring to our attention the fact that we'd have to now reintroduce the ability for extensions to listen for misses, or else such an extension as is needed could not exist.

The only alternative would be to change the logic in some way to work around this if possible. I suppose actually that this would be possible, because we could mark all of those past hits as misses in the should_hit() method, and then mark those that hit as such only later in the after_hit() method. This is only a hack, of course, and it would probably lead to unintended side-effects for any other extensions or reactors that queried the hit logs in some way. In fact, this problem already exists with the current arrangement, especially if extensions run queries that span across reactions, because the metadata is only updated by the reverse firer after all of the reactions are looped through.

Anyway, if we go this route, we'd need to implement an after_miss() method for extensions, in addition to finding solutions to the two bullet points above.

[JDGrimes]

I was wondering just now whether we really need to introduce this yet, since we currently aren't using any reverse-blocking extensions. But I suppose that it makes sense to go ahead and begin using this, because if we ever do implement a reverse-blocker, it would have to log a bunch of meta for each prior hit when we started using this otherwise. And not every dev will necessarily realize the implications of of blocking hits for these reactors, and see that they need an extension like this. So we'll go ahead with this I guess.

[JDGrimes]

Legacy points logs

There's a problem with this, and that is the points reactor's back compat with past logs from the old points hooks. These won't be reflected in the hit logs of course, but we do intend to reverse them in the points reactor. However, since they aren't in the hit logs, the reversals extension will block the reverse fires for them from ever hitting, resulting in them never being reversed. We'd have to also include back-compat of some sort in the reversals extension to allow for them to hit.

Or maybe a better idea would be to simply listen for these via a separate reactor. Then we could move all of the back-compat code to that from the points reactor, and register it only on legacy sites. I guess the problem there though is that only a single reactor can listen for each reaction. Unless we add a second, hidden reaction of some sort when we import the legacy hooks, that used this legacy reactor. But then we'd also have to take care to delete that reaction when its accompanying reaction was deleted by the user, etc.

So maybe it would be better just to allow for the legacy code to be separated but to hook into the extension and reactor somehow. Either that, or use a separate reactor that is a child of the points reactor and also includes the legacy code. And the same for the extension. Now that we can store reactions from multiple different reactors in a single reaction store, we could conceivably do this, though we might need to tweak a little bit of the JS.

[JDGrimes]

Before we embark on that though, I suppose it would be better to get the points reactor's reversal method more close to what it really should be first. So let's tackle #80 et al.

[JDGrimes]

Now that the legacy code has been moved to its own reactor, I'm wondering whether this is really handling that we should have for it at all. In the old points hooks, the reversals could never be blocked, so they always reversed all matching logs. Of course, all matching logs would always have been from the last fire, since no fires could be blocked. I guess it's best to go ahead and handle this anyway then, for consistency, if for no other reason.

[JDGrimes]

I think we're done here.