Prevent hitting when points value can't be calculated

[JDGrimes]

In #5 and #6 we've discussed the fact that the extension has to implement the should_hit() method, even though it doesn't really need to. However, after further thought, I think that we should perform some checks in the should_hit() method, and abort the fire if we can't calculate the number of points. Because returning 0 does effectively terminate the hit, since a transaction involving 0 points is not performed or logged.

This has consequences in regard to repeatability, limits, rates, etc., which we need to consider carefully to make sure this is the right thing to do.

[JDGrimes]

As far as when the value wouldn't be able to be calculated, this would happen whenever the value that we were supposed to use happened not to be usable, either because it happened to be an invalid type (edge-case), or because it just wasn't set at all (which is more likely, think of optional attributes, like custom fields).

[JDGrimes]

Along these same lines, the maximum and minimum would provide another thing that would circumvent firing. Basically, they are a condition. Which makes me wonder, why do we need them? Couldn't they just as well be conditions? The only reason not to would be when they were combined with ratios, etc. Then again, maybe we should just have more complex condition types, that can more easily handle ratios. But having all of the settings in a single location, where the dynamic points ratio and condition ratio cannot get out of sync, makes much more sense.

[JDGrimes]

Actually, the min and max are different than conditions anyway, because they are caps, not requirements. In other words, the goal is to award a user no more than 50 points, for example, not to prevent them from being awarded at all.

[JDGrimes]

Which basically means that the min and max are irrelevant here, since they never prevent the reaction from firing.

[JDGrimes]

This has consequences in regard to repeatability, limits, rates, etc., which we need to consider carefully to make sure this is the right thing to do.

First, rates and limits. Both of these are based on hits. Without our intervention here, a hit is technically taking place without the user actually being awarded any points. So the question here relates to the intentions of rates and limits. Is the intention to limit the number of times that the user is awarded points, or the number of times that they could have been awarded points, that is, that a matching fire occurs (a "hit"). Normally there is no distinction here, a hit means that the user is awarded points.

I think really that this partly comes down to the intentions of the individual user in a particular scenario. Do they want to place a limit so that each user is only awarded for 5 posts, or so that each user is only potentially awarded for 5 posts? However, as things currently work, if a condition wasn't met, it wouldn't count toward the limit. So at present users wouldn't really have the concept of potential hits in regard to limits, it wouldn't be possible for them to do (though we haven't introduced limits or rates yet, so this is all hypothetical). A consistent experience thus calls for us to block this from being counted as a hit, I think. It wouldn't count toward rates and limits by default, but we could introduce a setting to override this behavior if desired.

[JDGrimes]

In some ways I suppose that really this is a broader question that will have to be answered in regard to optional attributes/relationships. When the attribute isn't present, then how should a condition respond, in general? I guess for many conditions, the equals condition for example, the condition simply will not be met. So in general the condition will fail, except when it is a negative condition, that requires something not to be the case. There could be a simple not empty condition, but then there could also be a string does not contain condition. This is where there could be multiple intentions from the user, and on that type of condition we might want to give them an option as to what to do when the value is empty (it won't contain the value obviously, but do we want to hit anyway?). However, I think that the default will still be that the condition is considered met in that case, and thus a hit will be made.

I guess that kind of contradicts what we just said above, then. Because the default is that a missing attribute would mean that the condition would be met in that case. But on the other hand, we did say that for positive conditions, the hid would naturally be blocked, and there would be no two ways about it (though we could offer the user an option there too if we wanted, I suppose). And I guess that is a more fitting analog for what we are talking about in this ticket, a positive thing that is expected but isn't there, even though it isn't exactly a condition.

[JDGrimes]

Along the same lines of what we should do for missing attributes generally, what about when the target is missing? We currently count that as a hit, I think. This already has real-world consequences in regard to the comment author, for example. Comments can be left by a guest, and the hook will fire, and hit, but no points will be awarded because the value of the target arg (the user ID) will be 0/empty. (This also goes for other events where similar things can occur.) So hits are already being logged (the real-world consequences) without points actually getting awarded.

In other words, the system was designed so that a hit was determined entirely independently of the reactor. I've wondered at times whether this was proper, or whether the reactor should be able to abort the hit. Specifically I've thought several times about having the reactor validate the target. But I guess reactors can have other settings (like here!) that might also involve args that could be validated.

This would change a reactor from being totally reactionary to also having a say in whether the fire was a hit, just like the extensions do. It could open the door to less separation between reactors and extensions. Everything could be lumped into a reactor, rather than more cleanly meted out into extensions.

(I guess one means of fulfilling the separation of reactors and extensions would be to have an extension that validates the target.)

Of course, in this case, we are actually dealing with an extension that is modifying the behavior of a reaction.

The real underlying question though is what the expectations of the user would be in regard to repeatability and rates and limits. If a user only wants to award points for five matching comments, they probably don't want to have guests end up leaving some of the matching comments and thus registered users not end up getting those points at all. The idea of a hit is that it will have the intended affect—the reactor will react. Which in this case it can't.

....

[JDGrimes]

This is a decision that needs to be made, but it is apparent that it is beyond the scope of this module. There are similar issues in core that need to be resolved. So this is a decision that we need to first make in core, and then we will follow suit here. For now, we'll leave things as they stand, which is basically analogous to what core does.

---

Note to the future, we might also want to consider how reversals relate to some of this.

[JDGrimes]

Actually, let's leave it open as a reminder.

[JDGrimes]

In #8 (comment) it was suggested to use the regular points field as a fallback when the arg is not set. I guess that kind of relates to this.