[buddypress-trac] [BuddyPress Trac] #6210: Create New Invitations API
noreply at wordpress.org
Mon Mar 11 16:10:35 UTC 2019
#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
> - 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?
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
Ticket URL: <https://buddypress.trac.wordpress.org/ticket/6210#comment:26>
BuddyPress Trac <http://buddypress.org/>
More information about the buddypress-trac