Import old Points Hooks to new Points Reactor

[JDGrimes]

Related: #45

[JDGrimes]

Related: #31

[JDGrimes]

We'll be retaining the old screen in case there are any hooks that we cannot import, for back-compat. However, when we do that, I think that we should disable the ability to create and edit/delete points types on that screen, just so that users don't get confused and think that the points types on each screen are independent. This will also help to make it clear that the new screen is the one to use in the future, and the old one is just kept around out of courtesy.

[JDGrimes]

A transition like this can really throw users off. When the new interface/API contains bugs or inadvertently removes a feature that some users depend on, it is very frustrating to them. I don't know how many times I've seen WordPress or a plugin introduce a revamp of a UI or something like that only to have a bunch of angry users on their hands. 'The new version is completely broken. Why couldn't they just stick with what worked. I'm going to downgrade to the previous version, and won't be updating again, ever.'

Obviously, that's never what is intended. And I think that with WordPoints we take special caution and go above and beyond in our testing. But with a revamp this big, something is bound to go wrong. When it does, we shouldn't leave users with the choice of giving up on WordPoints or downgrading to the prior version. I think there is a better way to do upgrades like this.

Beta testing is nice but it doesn't offer the same scope of usage that the plugin will actually get once released. So it can never catch everything. With a change this big, relying on beta testing isn't enough.

What I propose we do is a sort of "soft launch". Since we're not going to be completely ripping out the old version of the UI yet, just hiding it, let's provide users with a button that can take them back to the old version in a single click. That way if there are some bugs, the user won't have to downgrade or wait for us to fix them. They can just temporarily go back to using the old UI.

We'd provide a button for them to switch back to the new UI too, of course.

Perhaps we could also let them shoot me an email when they switch back to the old UI so that I know why and can work on fixing things.

In short, we should look at this as the beginning of a transition, and not demand that users make that transition, but instead keep working on improving the new UI and API until it gives them everything that they want. There should be no reason for them not to transition. They should want to transition.

[JDGrimes]

Something to keep in mind https://github.com/WordPoints/hooks-api/issues/2#issuecomment-142965329:

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.

[JDGrimes]

An issue with switching back and forth between the two UIs is that there isn't total compatibility between the new and old hooks, as noted in https://github.com/WordPoints/hooks-api/issues/96#issuecomment-221567365:

This will also likely be affected by whether we decide to let users freely switch back and forth between the two UIs as suggested in #23 (comment).

This is something to keep in mind, although we still fill out the auto_reversed meta key, so for the most part the two should work when switched back and forth. The only issue would be that the log types for the legacy hooks would be different than for the new ones. That would mean that if a user switches from the new hooks back to the old hooks, double-hits could occur. If a new hook had a hanging hit, the legacy hook wouldn't reverse it because the logs wouldn't be recognized by the reversal query, since it would be querying by the legacy log type. (A shim to fix this could be added though.) Also, we might have difficulty with the fact that the legacy hooks set comment/post metadata to keep track of whether they had already awarded points, and we no longer do that.

[JDGrimes]

We also have to consider that we might not be able to deport (for want of a better word) all of our hooks back to the legacy version. If the user has used new features that aren't supported by the legacy hooks, things will break. So maybe we should disable this feature after they create new hooks. Or else we'd have to keep the new screen around for those hooks, or maybe just tell the user to delete them or warn them when they downgrade?

[JDGrimes]

Another thing to keep in mind is that we have to split the post/comment related hooks by post type when importing if they apply to all post types, and then recombine them if we deport them. Which is only possible if they all still exist and none of them has changed.

But there are other complications as well, such as when a new post type has been added to the site, but a new reaction hasn't been created for it. I guess that that is actually a change in behavior compared to what we did previously, but I'm not sure that we can really provide back-compat for that in a sane manner. Anyway, the point is that just because a reaction doesn't exist for a points type doesn't mean that a reaction was deleted, it might just be that a new post type has been added along the way. I guess that really it still wouldn't be safe to deport though. Because we'd be suddenly begin to award points for a particular post type when we never did before. That is, it wouldn't be safe to deport to a single points hook as we had originally imported from, we'd have to deport it split for a points hook for each post type.

[JDGrimes]

Related thought: maybe we could come up with a way to notify a user when new events become available. So that when a new post type was registered, for example, we could show an admin notice saying that "the Publish {Post Type} event was added", or something to that effect. We'd include a link to the screen so that the user could begin adding new reactions to that event, of course. (And probably we'd want to highlight the event and scroll it into view. Maybe even automatically add a new empty reaction for the user.)

That'd be for a separate ticket though.

[JDGrimes]

We don't currently display anything to indicate that reversals of a reaction are blocked. We should at least display a message for the legacy reactions. Probably this should be a checkbox so that the user can re-enable this, as they can now. I suppose we'll only do that for legacy sites at present though.

[JDGrimes]

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.

I just happened to think of this again. Although as I consider it, I'm thinking that it really isn't likely to be a significant issue.

Then again, I just happened to think of the issues this would cause if the user re-enabled reversals on the imported hook, but didn't delete the old comment delete hook. The points would be subtracted twice upon reversal then.

We could at least import the hooks that aren't paired, though of course it takes work to figure that out.

In fact, we could probably import the reversal hooks, as we're going to have to do anyway eventually:

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.

We'd just have to add a filter that would change the reversal amount in our legacy reactor.

This would still only work for paired reversal hooks however. If there was a comment delete hook used without a comment approve hook, it would no longer work, because our reactor would never be called into play

Then again, maybe the thing to do is to provide "comment unapprove" and "post delete" events on legacy sites where there are unpaired hooks of this sort.

It would be nice if we knew whether this feature was even used on any legacy sites or not.

[JDGrimes]

I wonder if we could port unknown custom points hooks over to the new API as well, using some sort of adapter. We'd still have to use the old handlers for such hooks, but we could still handle their settings through the new screen.

It's an interesting thought, but I'm not sure that it would be worth it. We also have to remember that any JS/CSS associated with those hooks might not end up getting loaded on our new screen (or might be incompatible even if it is), so it is possible that the hooks won't work.

In that case, we'd really be forcing the user to switch back to the old screen entirely more quickly than they'd otherwise need to, rather than providing them the best of both worlds as we're hoping to now.

[JDGrimes]

Then again, maybe the thing to do is to provide "comment unapprove" and "post delete" events on legacy sites where there are unpaired hooks of this sort.

This is a good idea, and likely what we'll do when we eventually remove that screen. (or possibly even before.) However, now is not the time to do that. Let's go ahead and get the initial version of this out. We can come back to this part later, maybe when we actually have some cold data about whether it is even an issue.

[JDGrimes]

Likewise, I don't think that it is worth it for us to import any hooks that have auto-reversing disabled right now. It is best to wait until the old screen is removed to do that, because until then it will still be possible for users to end up creating hooks on each screen that only work one way, and so are not self-contained. And in that case, strange things could happen, as pointed out above.

[JDGrimes]

I've just realized that we don't actually register the comment removed and post deleted points hook anymore, even on legacy sites. It seems that when we moved the class code in https://github.com/WordPoints/wordpoints/commit/8f3135e41394b4371b2bfe54bb66c6ba5c25b937, somehow we forgot to continue registering the hooks. So they are no longer registered since WordPoints 2.0.0. However, I think that this is a bug that we should fix in the next release.

[JDGrimes]

Note that we need to continue displaying the legacy hooks screen even on non-legacy sites, if they are using a module that registers other points hooks which haven't been imported.

[JDGrimes]

The main thing left for this ticket is to decide whether to support switching back to the old screen after it has been disabled. (Maybe that should really be for another ticket though?) I still like the idea, but it is much more complex under the hood than it seems to be on the surface.

[JDGrimes]

Maybe a middle-ground would be to not import the hooks automatically but offer import on demand for legacy sites. That still doesn't help the user if they decide that importing was a mistake and they want to go back though.

[JDGrimes]

The main reason that I'd expect people to want to revert back would be because of some change in hook behavior, or because they were unable to use the new interface for accessibility reasons (which I actually doubt).

The main behavioral changes are that they are likely to miss is the fact that they can no longer order the hooks (although they will remain in somewhat the same order actually, not necessarily, because we import based on the instance array which is ordered by creation date, not by the order they are displayed in the UI), and the fact that the points log messages are no longer auto-generated, and thus are not dynamic and contain less information.

Aside from this, there is also the possibility that they will dislike the fact that the post points hook now reverses transactions on depublish rather than on delete. Speaking of that, I wonder what happens when a post has been depublished but was awarded points. Since the points wouldn't have been removed, will we end up firing again under the new API when the post is republished if we import such a hook?

Anyway, this just makes me wonder whether we would do better to wait to import until we have complete parity.

[JDGrimes]

Since the points wouldn't have been removed, will we end up firing again under the new API when the post is republished if we import such a hook?

Yes, it turns out that we will. I guess that should be addressed in another ticket though.

[JDGrimes]

The main behavioral changes are that they are likely to miss is the fact that they can no longer order the hooks… and the fact that the points log messages are no longer auto-generated, and thus are not dynamic and contain less information.

These are both things that we could fix in the future, and we're definitely planning to do the latter (#8). Which is what has me wondering whether we should just wait until we do until we import these.

The main objection to that which I can think of is that some users may want to see their hooks imported in the mean time, and if we don't offer them that option, they'll either request it (and probably think it a bit strange that we don't offer it), or else they will switch them over manually. Mostly I don't want people to waste time doing that work when it can be done automatically. But really I guess it makes some sense for them to do this manually, as that makes it more obvious exactly what is changing in regard to the hook settings. Then again, there are things that they can't see/won't realize, which could lead to unexpected behavior of their new reactions. I suppose that that is something that we can't entirely avoid anyway though, as long as the old hooks screen is still available to them because of hooks we couldn't import. Still, importing most (or in some cases all) of the hooks for them does make things simpler for them and prevents them from accidentally doing something that they didn't intend to do.

[JDGrimes]

I guess really before we get bogged down in this discussion, it would be a good idea to consider why a user would actually want to import the hooks. As cool as our new UI/API is, there is really no reason for the legacy hooks to be imported at this time, other than for the purpose of being able to see all of their hooks and reactions in a single location. We might have reasons to import the hooks now (or at some point), like promoting the new screen (or because we're going to drop the old one—but we're not yet). But as far as the user is concerned, simplicity is the only reason to import them.

[JDGrimes]

All in all, for the sake of back-compat, I'm actually leaning toward not importing the legacy hooks yet. Not that there aren't good arguments for it, but I don't think we should waste time making those arguments right now. Our time would be better spent getting the initial version of the UI and moving forward with the modules. It's great that we've provided a good foundation here though.

[JDGrimes]

Here is our failing post delete test, for reference:

    /**
     * Test that it imports legacy points hooks on install.
     *
     * @since 1.0.0
     */
    public function test_imported_post_points_hook_does_not_refire() {

        $legacy_slug = 'post';
        $settings = array( 
            'points' => 10,
            'post_type' => 'post',
            'auto_reverse' => 1,
        );

        $this->create_points_type();

        $hook_type = "wordpoints_{$legacy_slug}_points_hook";
        wordpointstests_add_points_hook( $hook_type, $settings );

        $user_id = $this->factory->user->create();

        $post_id = $this->factory->post->create(
            array(
                'post_author' => $user_id,
                'post_type'   => $settings['post_type'],
            )
        );

        $this->assertEquals(
            $settings['points']
            , wordpoints_get_points( $user_id, 'points' )
        );

        $this->factory->post->update_object(
            $post_id
            , array( 'post_status' => 'draft' )
        );

        $this->assertEquals(
            $settings['points']
            , wordpoints_get_points( $user_id, 'points' )
        );

        $this->install();

        $this->factory->post->update_object(
            $post_id
            , array( 'post_status' => 'publish' )
        );

        $this->assertEquals(
            $settings['points']
            , wordpoints_get_points( $user_id, 'points' )
        );
    }

[JDGrimes]

I'm going to close this in favor the the issues I've opened up on the core WordPoints repo. There's still lots of useful info here for reference though.