[buddypress-trac] [BuddyPress Trac] #6210: Create New Invitations API

[buddypress-trac]

#6210: Create New Invitations API
-----------------------------------------+-----------------------
 Reporter:  dcavins                      |       Owner:  dcavins
     Type:  enhancement                  |      Status:  reopened
 Priority:  low                          |   Milestone:
Component:  Core                         |     Version:
 Severity:  normal                       |  Resolution:
 Keywords:  dev-feedback trac-tidy-2018  |
-----------------------------------------+-----------------------

Comment (by dcavins):

 Thanks Boone, for your review. That's exactly the kind of feedback I was
 hoping for. Comments below.

 Replying to [comment:25 boonebgorges]:
 > Thanks for bringing this one back to life, @dcavins !
 >
 > I've done a casual review and I have some questions and feedback. In no
 particular order:
 >
 > - Can you explain how `component_name` and `component_action` work?
 Specifically, it feels like `component_name` is enough to differentiate
 between, eg, group invitations and site invitations.

 I'm not using `component_action` at the moment for group invitations, but
 added it for flexibility, because I can imagine that a single component
 could need multiple invitation types. For example, if some "sites"
 invitation setup was needed to be able to invite people to subscribe to
 update emails or to join the site, then you could identify your different
 invitations as `sites/subscribe` or `sites/join`. Of course, two new
 extension classes could be used to arrive at the same end result.

 > - Regarding naming: I'm not a huge fan of `BP_Invitations_Invitation`.
 IMO it's better to go with `BP_Invitation` or `BP_Core_Invitation`. And
 `BP_Invitations` is not very clear to me. Maybe something like
 `BP_Invitation_Manager`, `BP_Group_Invitation_Manager`, etc?
 https://stackoverflow.com/questions/1866794/naming-classes-how-to-avoid-
 calling-everything-a-whatevermanager

 This is one of the biggest doubts I had. I named
 `BP_Invitations_Invitation` in a sort-of-BP pattern
 (`BP_Notifications_Notification`), but think it's awkward. I'll happily
 rename the classes.

 > - Otherwise, the general pattern of having a base manager class and then
 extending it per component seems OK.
 > - Because `run_send_action()` and `run_acceptance_action()` seem to be
 the critical integration points for specific components, perhaps it'd make
 sense (from the point of view of self-documentation) to make the base
 class `abstract` and to make these two methods `abstract`.

 Yes, I started out with the base class being abstract, but thought there
 might be some high-level use in being able to invoke it directly to do
 mass invitation clean-ups without having to work through the extension
 classes. This seems like an edge-case use to me, though, since you could
 work directly with the invitations object class to do mass deletions, for
 instance. If you think the base class should be abstract, with certain key
 methods being marked abstract, I think it's a good idea.

 > - I see that, in various places, you're deprecating the use of
 `membership_id` in the case of acceptance/rejection. Obviously, I
 understand why you have to do this, and some level of compatibility
 breakage is unavoidable in this kind of change. But I'm concerned that
 it's going to cause backward compatibility concerns of an awkward type.
 For example, in `bp_group_request_accept_link()` you are swapping
 `membership_id` with `user_id` in the existing link format. If a third-
 party plugin is manually building a link of this sort, and accidentally
 puts in a "membership id" that matches up with a valid user_id, it could
 result in a link that accepts the wrong invitation. This is bad. In cases
 like this, perhaps it's better to break those old links altogether, and
 change the format: `admin/membership-requests/accept-request/` or
 something like that. More generally, it'll be necessary to have a high-
 level summary of compatibility breaks, so we can share with BP devs.

 Yes, the intertwining of group memberships and invitations causes some
 problems, because group invitations and requests are cached and referred
 to by their ID in the `group_membership` table. So any time the streams
 get crossed with the new table IDs, it causes an issue, like in
 `bp_get_user_groups()`. I was able to support backwards compatibility in
 many cases, but, for example, if a dev is issuing invitations by using the
 `BP_Groups_Member` class directly (we were doing that in our tests), I'm
 not sure how to prevent a problem. I agree with you that making a clean
 break where we have to is probably better than creating a situation where
 it fails occasionally, and frustratingly.

 I was hoping for very much like 100% backwards compatibility, but that
 isn't possible, so having a plan on how to break compatibility in sensible
 ways will be needed.

 Thanks again for taking a look! This is quite a big change to be making,
 so I was happy that there were many tests to cover various aspects of
 group membership and find problems, but having your input is much more
 valuable.

-- 
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/6210#comment:26>
BuddyPress Trac <http://buddypress.org/>
BuddyPress Trac