WordPoints / buddypress

WordPoints module integrating it with BuddyPress
GNU General Public License v2.0
1 stars 0 forks source link

Favorite Activity Update hook event #19

Closed JDGrimes closed 7 years ago

JDGrimes commented 7 years ago

To fire when a user favorites an activity update.

All types of activity can be favorited, including comments. So we'll have to decide how to handle this.

JDGrimes commented 7 years ago

I suppose that we could either limit this to just status updates and comments on status updates, or else we would need to introduce a generic entity as suggested before. Because we were kind of waiting until later to introduce entities for every activity type.

However, on further thought, I think that it is going to be rather confusing that all comments and favorites aren't counted. I mean, it currently makes sense for all comments and favorites to be counted the same, since to the user they pretty much are the same thing. There isn't any difference in the type of activity item when it comes commenting on it or favoriting it. So perhaps we should make these events generic to all types (which means a generic entity), or else go ahead and introduce entities for every type.

JDGrimes commented 7 years ago

I think partly our issue is that we were conflating introducing entities for every type with introducing a "new activity" event for every type. Which is not the same thing at all. We can add entities for different types without adding events for every type. However, when it comes to the comments and favorites, we do have an issue, because we would have our events for those multiplying like crazy.

Which is where the generic entity comes in. With a generic entity, we can still let the user choose between activity types in the future (or even now) via conditions on the activity type as an attribute, rather than having separate events. And I think for these events that really makes the most sense. So let's go ahead and do that. It's uncharted territory, but experimentation has to start somewhere. I don't think that anything that we do couldn't be changed in the future with a little work. So let's do it.

JDGrimes commented 7 years ago

I was considering what attributes we should register for the generic entity. Instead of the "author", that relationship should be just the generic "user" or something of the sort. I also don't think that we need to register the content attribute. For new posts it is the post's content, and the same for comments on posts, and new forum posts and topics for the groups component (which uses just an excerpt). All of the other activity types just have empty content. So, I wouldn't say that the content is a truly generic attribute, and thus we should probably omit it for now.

JDGrimes commented 7 years ago

Something to consider is how we will create human ID for this generically. Right now we are creating an excerpt from the content, but that' obviously won't work when not all activity types have any content. I suppose when the content is empty, we should just fall back to the action property.

JDGrimes commented 7 years ago

In regard to the event itself, it is really an event in which a relationship forms, and so it has two "primary" args. The events testcase doesn't support this, so it is something that we'd need to update. But really, the reason that it doesn't support it is because I'm not sure we foresaw this as being a valid possibility. Yet here it is. So I'm going to have to go back and do some research as to exactly what we ended up deciding in regard to that, and what implications there are.

JDGrimes commented 7 years ago

https://github.com/WordPoints/hooks-api/issues/78 concluded thus:

Thus we have formally proven... [that] events have at most only a single "primary" arg.

But I'm not sure if we considered events like this or not. Perhaps we did and concluded that neither arg is primary, they are both stateful. Which is what I'm researching now.

JDGrimes commented 7 years ago

I think this is relevant (https://github.com/WordPoints/hooks-api/issues/78#issue-126274774):

It is already clear that an event can have an unlimited number of stateful args. Stateful args are args, like the current:user, that don't really "participate" in the event. They are sort of bystanders that just happen to be present when the event happens. In other words, they're only there because of the current state of things—they just happen to be the current/previous/next when the event occurs. It is important to understand though, that by "participation" we mean more than just having a part in the event. For example, the current user has a part in the (hypothetical) Edit User event: they are the editor who is editing the user. However, because that event can happen repeatedly with any number of other users acting as the editor, and without this event being reversed, both the editor and the edited user are considered to be stateful args; neither of them is irreversibly modified so that the event cannot happen again without them being changed back. In other words, an arg is stateful unless it participates in the event in such a way that its status is toggled; it must be modified in some way that must be undone before the event could occur again.

So I think that we basically decided that in this case we are dealing with two stateful args, rather than two signature args. Because the event can occur any number of times with other values for those args, without a reversal of this particular fire occurring.

However, I think this case is different than the one we considered there, because editing a user does not forge a relationship, and is generally not reversible. In this case a relationship is forged, and the event is not repeatable unless it is first reversed by that relationship being broken. So it seems to me now that both args come together to give the event a unique signature.

JDGrimes commented 7 years ago

Currently, if we don't incorporate both args into the signature of the event, the reversals extension will not be able to detect reversals correctly (the legacy points reversals and repeat blocker extensions would also suffer, but they aren't really relevant here). What would happen, if we made these both stateful args, would be that when one hit was reversed, all hits would be, for all users. Which is why reversible events need to have signature args. If we just register one of the args, it will still cause problems: if we register only the user, hits for all favorites for that user will be reversed when one favorite is removed; if we register only the activity, hits for all users for that activity we be reversed when one user un-favorites it. So we really do have to have two signature args here for this to work as we want. We'll need to open a ticket upstream for that.

JDGrimes commented 7 years ago

I've just confirmed the above behavior by favoriting two items and then removing just one of them from being a favorite. All of the transactions were reversed.

points_ _adminide_ _buddypress-develop_-_2016-12-20_17 23 07

JDGrimes commented 7 years ago

I've opened a ticket upstream, but it's going to be too much to put in a point release. So we won't be able to have this event until WordPoints 2.3.0. But we don't have to wait until it is released, we can go ahead and create it now but only register it based on the WordPoints version.

JDGrimes commented 7 years ago

Should we reverse the event when an activity is marked as spam or deleted? It doesn't really represent a reversal of the event itself, but the activity will no longer be among the user's favorites (unless it was just marked as spam and then ham again).

I think that this is probably not a very common occurrence, so let's leave it for now, and we can always add these actions to the event later.