Comment/Comment Removed points hooks don't work together as intended #221
Comments
|
Not logging the reversal is new territory, and may have unintended side affects. Although, a hook should have only one function. Logging the reversal might encourage the reversal feature to be used differently than intended. When we go through with this, the Comment Removed hook should probably be renamed to the Comment Spam hook, and only run when a user leaves a spam comment, since reversals will already be removing points awarded if a comment is just deleted. |
|
As far as just deleting the logs goes, on the plus side, it means we don't have to mess with new log messages, and also, that will keep us from giving the user two messages when their comment is marked as spam if the Comment Removed hook is also in use. The con is that it could be confusing to a user to suddenly have some points revoked. However, if the user is spamming, what does it really matter? The only thing that makes me second guess it is for the new Comment Received points hook. The post author isn't a spammer, and yet they might get points revoked if a spam comment gets removed. So they could still be confused, and wouldn't be a guilty party. Of course, if they are themselves marking the comment as spam, they'll be able to figure out what happened pretty quickly. Also, that could discourage post authors from flagging spam comments on their posts. It's beyond the scope of this issue, and more in line with #111, but we could handle that in two ways:
Again, beyond the scope of this, but something that still requires thought. I kind of like 1, but it's probably way beyond the scope of 1.8.0, unfortunately. |
|
Oh, and option 3 here is to do reversals differently for the Comment Received hook, and instead of doing things behind-the-scenes, logging what is going on, at least for now. Probably the best course for 1.8.0. |
|
We shouldn't attempt this for 1.8.0. We'll do only what is needed for #111. |
|
Another possibility would be to integrate the two hooks so that there aren't two log entries when one is reversed. If the other one is present they would just hook into each other. That could be complicated though, and might not work exactly as intended when the hooks are configured for different post types (or one is configured for all post types). |
|
Honestly, why can't we just remove the Comment Removed hook entirely. It won't really be needed any more, and it would make things simpler, I think. |
|
We could even do something like this: When we update the points hooks, check if the Comment Removed hook is being used, and if it has more points than the Comment points hook, we could let it stay on those sites. We'd hide it on the rest of the sites. |
|
I'm thinking that we already punish spammers enough by having them waste their time—they wont end up with any extra points for leaving spam comments. Usually if a spammer is obnoxious I'd expect that the site's moderators will delete their account. Anyway, I'm thinking it is time to end the extra punishment thing. We'll maintain backwards compatibility for those that are using the Comment Removed points hook to punish. |
|
What about back-compat for users who don't use a corresponding Comment Removed hook for each Comment hook? We'd suddenly be removing points for removed comments. I think the best backward compatible solution is this:
Then, on update:
|
|
New troubles: When we perform the auto-reversal (as it presently works), we just perform a logs query to pull out all of the logs matching that comment ID. We then loop through these and reverse the transactions. The only metadata associated with a transaction is the comment ID. This means that there is no direct way to determine what instance it was that fired the original transaction. However, the auto reverse setting would be saved as an instance setting. In other words, it is a per-instance thing. So we need to determine which instance logged the transaction before we can determine whether it should be reversed. There are several possible approaches to solve this:
Of these three options, I think that perhaps no. 1 is most promising. The issue with it, that of potential conflicts between different instances, is not vital. It can also be partly solved by saving a |
|
For the solution to no. 1, here is my proposal for how we determine the reversal setting to use: We get the type of post the comment is on. If there is an instance specifically for the post type, we use the auto-reversal setting from it. Otherwise, if there is an instance with the All post type setting, we use the auto-reversal setting from it. Conflict resolved. |
|
In regard to conflicts from multiple hooks affecting the same post type, this is moot: only one hook will fire per comment per points type no matter how many instances apply to it, because the comment meta key that holds the last status that the comment was awarded/reversed for is based only on the points type. So even in the past, when there were multiple instances affecting the same post type, only one would award points. |
|
We might want to consider recombining the Post and Post Delete points hooks and implementing auto-reversal as well. We don't currently have the same issues there, because the Post Delete hook only fires when a post is deleted, but there could be benefits. |
|
Oh, and something interesting that I noticed. The Comment points hook awards the post author as well as everyone else. We decided not to do this in the Comment Received points hook. I don't know that it is something we should change in either case now, due to backward incompatibility. It would be nice to be consistent though. Part of my rationale for keeping the comment hook the way it is, is that it's nice to reward a post author for interacting with users who comment on their post, especially if they are answering questions. |
|
I'm thinking about how far we should go in terms of limiting or maintaining the legacy aspects of this. I'm thinking we'll do it like this:
This could leave a user confused if there is a left-over Comment Removed hook but no un-paired Comment hooks. However, that is really an edge-case that isn't significant enough to consider. I had thought about possibly restricting these legacy features as much as possible, only enabling them for the current hook instances that required them. However, I think this could easily confuse and frustrate the user. So the new plan is to enable these legacy features across the board when they are in use at all, and then we'll remove them in 2.0. That will give time that might allow many users to discover the change. |
We already know the comment ID, so we are just wasting resources pulling it out of the points log meta table. Working on #221
Previously the Comment points hook did not support auto reversal. However, auto-reversal has been automatically handled by it’s parent class since 1.8.0. This has been disabled for the Comment points hook up to now by overriding the `reverse_hook()` method. This removes that method, enabling auto-reversal. The parent class is also updated to make it possible for child classes to allow auto-reversal to be disabled per-instance by the user, and the Comment hook class has been configured enable this feature. See #221
We introduce the `$wordpoints_component` and `$previous_version` properties, and use them in the `update_component()` method, making it optional to supply its arguments. See #221
On update to 1.9.0, we combine any Comment/Comment Removed hook pairs that have the same number of points. For the rest, we turn off auto-reversal, for backward compatibility. See #221
Only on sites where disabling auto-reversal of the Comment hook is necessary because of existing hooks do we allow it to be disabled as a legacy feature. We also introduce a new method to the base hook class, `set_option()`, and include unit tests for it and related methods. See #221
We now only register it when it is required for legacy sites where it is still used. See #221
The tests involving the Comment and Comment Removed hooks need to be updated now that the Comment Removed hook is disabled by default. We now need to manually register the Comment Removed hook in any test that requires it. However, the only way to do that is by calling `WordPoints_Points_Hooks::initialize_hooks()` again after calling the `register()` method to register the hook’s class. This results in a second set of hook objects being instantiated. This effectively causes the hooks to be fired twice in the tests that do this. This state doesn’t leak into other tests, because the actions are reset after each test. It does cause an issue with the Comment hook in regard to auto-reversal. The auto-reversal feature can be disabled on legacy installs by setting a certain hook type “option”. This is object instance-specific, and is saved in a property. This causes problems when we need to reinitialize the hooks, because then the objects returned by the `get_handler_by_id_base()` method are different from the objects which are hooked to the actions. This state of things will continue to affect each test after a re-initialization occurs. Because of this, I’ve found it necessary to isolate the Comment Removed-related tests that re-initialize the hooks, and add them to a new excluded `legacy` test group. We don’t currently run this group on Travis, and I’m going to leave it like that for now. See #221
|
I guess that's it. |
Now that the Comment Removed hook may be disabled on legacy installs, we need to provide a separate function to generate the log text, in case the logs get regenerated for some reason. This adds a new function to do that, and it is now used by the Comment Removed hook class’s `logs()` method. Includes unit tests. See #221
The Comment points hook has been present since the plugin's first release. The Comment Removed points hook was split off from it in #46. They do not, and have never, behaved intuitively.
The original intent was to create a hook that awarded a user points when they received a comment. That's how the Comment points hook was born. Foresight dictated that there would need to be some way of handling spam. Users would need to be punished when they tried to game the system. This desire to mitigate spam originally resulted in the addition of a second points field to the Comment points hook. This was for the number of points to remove from the user if their comment was marked as spam or otherwise removed. This allowed the user to just remove the awarded points when a comment was marked as spam, or by increasing the amount, to actually remove more points than the user originally received thus penalizing them.
Eventually I decided that the best way to handle these two functions of a single points hook was to split the hook in two. Thus, the Comment Removed points hook was born.
The flow of events that the hook(s) were designed to cover would go something like this:
And they work great in that scenario. But there are other possible scenarios, like these:
.
While these types of scenarios where taken into account when the hooks were designed, they aren't handled well. For example, in both of the above cases the user has left a valid comment, but they haven't actually received an increase in points from it. It would be even worse if, e.g., 25 points were removed for spam comments. Then the user could leave a valid comment and end up losing 5 points. Ouch!
In both of the above scenarios the user should have ended up with +20 points. The question this ticket is here to explore it how to best make that happen.
The idea first conceived in #111 is to introduce automatic reversal of certain hooks. The Comment hook would reverse it's award when a comment is removed. In other words, it would be completely severed from the Comment Removed points hook, which would itself implement the same reversal feature.
The flow for the comments points hook would then look like this:
The problematic flows would look like this:
.
Now in both of these cases the user would have +20 points. The Comment Removed hook would operate separately, and is not included in the flows outlined above. With that hook configured as well the flows would look like this:
.
The main question at this point is whether these "reversals" should be logged or be done behind-the-scenes, deleting the related log information.
The text was updated successfully, but these errors were encountered: