Network Invitations and Membership Requests

[dcavins]

I’ve been thinking about how to make network invitations work smoothly, and also what opportunities network invitations and requests offer for sites generally. One of the big improvements we could offer is helping to cut down on spam signups. I’m thinking that if the registration order is changed somewhat, we could prevent non-verified users from adding themselves to the site to have to be removed later.

Possible change to the registration process

1. Enter your email address and we'll send you a verification code. (This will essentially create a membership request or self-invitation in the invitations table, depending on the registration settings.)
2. Follow email link (use email address and verification code) to access registration form.

• Will need to add verification_code field to invitations table. Maybe add a verified column as well (that would be marked true when user completes verification step)?
• Also maybe need to add a meta column or similar, for follow-on actions, like site or group membership invitations that would be extended upon successful network invitation acceptance.
• Registration form email address will be readonly so that it is locked to the verified address.

Site invites & requests options for site admin to choose from:

• Anyone can sign up. Email form will create a “self-invitation” that needs no approval, just verification.
• Potential users may request membership and be approved by site admins. Email form will create a membership request that requires verification then approval (maybe approval converts the request into an invitation?).
• Network members can invite people to join. Invitation will be created that requires verification, but is already “approved”

These modes can be mixed and matched, for example:

• Create a public site - Allow anyone to sign up and also allow invitations to be sent by network members.
• Create a somewhat private site - New members may request membership and also allow invitations to be sent by network members.
• Create a pretty private site - Only way to join is by being invited by a current member.

Please share any thoughts you have about other ways we can improve the registration process or other things to think about while designing network requests and invitations.

Thanks!

[imath]

Hi @dcavins

Thanks for starting working on this. Here are my first thoughts.

I would suggest to first list :

• how joining a site of a network is happening today in WordPress (The network admin can add network members to a site, I don't remember about site admins, new members can create their own site during signup)
• what are the available types of site: public, hidden from search results (if i remember well)

Create a public site - Allow anyone to sign up and also allow invitations to be sent by network members.

I'm unsure it's possible to sign up for a blog that isn't the root one today. When you say network members do you mean the ones that have a role on the sub site (eg: read at the very least) or any member ?

About private site it doesn't really exist today, we have sites that are not indexed by search engines, but you don't need to be a member of a site to view its content. Are you suggesting to extend WordPress using the blogs component for instance ?

NB: I remember Signups used to create members with no roles on any site, and I think BuddyPress now forces a signed up member to have a role on the root blog.

I also think the BP Invitations API could be used to improve the way members are added to a site by Network admin or regular admins for non MultiSites config. I have the GDPR in mind. Somehow today when an admin is adding a new user to his site there are no proofs the added user gave their consent...

[dcavins]

Hi @imath-

Thanks for your comments. Let me clarify a few of my less-than-clear wordings.

• In this ticket, I'm only trying to accomplish invitations to a BuddyPress instance, I guess that is the "network" or the "base site of a network" though the terminology is pretty sloppy around these concepts. If successful, the user would be added to the wp_users table.

• "Public" or "private" sites as I'm describing here is not a technical concept. I'm describing how a BP admin could choose to allow registration on her site. At the moment, the options are "anyone can register" and "no one can register." A side goal of this ticket is to allow the BP admin to have more control over new membership, which is a big problem (mostly spam) for the niche networks that I admin. "Anyone can join" works for Facebook, but BP seems to work best when it's more targeted, and less overrun by spammers. So, I see a real opportunity here to offer new modes of membership that fall between "Anyone can join" and "No one can join."

I agree wholeheartedly with your GDPR thoughts, too. That seems like another nice opportunity here.

Thanks again!

[boonebgorges]

Thank you for beginning the discussion about this, @dcavins ! I agree that the account registration process is a pain point for many BP installations. Spam is sometimes a problem, though there are others: limited customizability (such as multi-step registration), the difficulty of integrating with SSO (those tools often auto-provision accounts in a way that bypasses BP's profile fields), the lack of a built-in invitation system (see Invite Anyone), etc.

For a bit of context, the current registration flow is:

1. User visits the BP register page
2. User enters WP account info (username, password, email) and BP profile data, as well as blog info if enabled
3. A new entry is created in the signups table, which includes user-provided data as well as a confirmation code
4. An email is sent to the user to confirm their registration. This is the main anti-spam and anti-impersonation prevention measure. The email contains a confirmation link, built using the confirmation code.
5. User clicks through and confirms registration
6. The entry in the signups table is converted to a WP user, a site is created if relevant. These steps are handled by WP.
7. BP saves any xprofile data stored with the signup

Your proposal suggests taking some of the existing flow that's handled by and/or closely mirrors WPMS (such as the signups table for DB storage and the WP-native user creation in f) and putting it into BP.

I have a couple broad questions and concerns about this proposal.

First, by doing ourselves what WP currently does (and has always done), we risk compatibility breaks of various sorts. For example, plugins that modify the WP user registration process, and as such automatically work with WP, may not work in the same way. The same can be said for site-specific customizations. Related, it seems bad for us to add a bunch of infrastructure to BP - like confirmation codes and meta - that duplicate what already exists in wp_signups, if for no other reason than that we don't need the maintenance overhead.

Second, I don't really see how the proposed changes help to mitigate spam registrations. The current system means that the majority of successful spam accounts are generated by real people (automated turk, etc) than by bots. Moving the confirmation email step earlier in the process won't affect that.

I love the idea of having an account invitation system, but it seems to me we could simplify it by using the wp_signups tools that already exist. So, a BP invite could trigger the creation of a signup, but we wouldn't allow WP/BP to send a confirmation email. Instead, we'd send our own invitation email, and the "accept" link would be something like: /register/?key=12345. When we detect key=12345 on the register page, we check for a matching signup, and perhaps pre-fill some of the info, like email. Then, we allow the user to walk through the existing registration system, including the confirmation step. (We might need some mechanism to allow someone from manually entering the confirmation code into /activate/ without going through registration, but this seems fairly minor.)

If we really want to help prevent spam signups, I think there are other things we could focus on, which are independent of the invitation system. BP could ship with things like honeypots; the option for admins to confirm pending registrations; improved integration with third-party services (Akismet?) for pre-screening of invitations; etc.

But I do really like the idea that having out-of-the-box site invitations would allow admins to configure their site in order to limit registration in various ways. As we build, we should think about how and whether this system can be leveraged by people trying to customize registration in yet other ways - I'm thinking especially about institutional SSO (ldap, shibboleth, etc).

[dcavins]

Thanks for your feedback, Boone. Yes, it would be much simpler to not change the actual registration process, so let's start there.

One question I have about your flow (which mostly matches what I was imagining, probably because I've used Invite Anyone for a long time), is that you say:

---

When we detect key=12345 on the register page, we check for a matching signup, and perhaps pre-fill some of the info, like email.

---

This makes it sound like when a user invites a new member, a bare record is created in the wp_signups table. Is this what you were intending? I was imagining that an invitation would be created, and the wp_signup would be created when the user responded (and the various hooks around signup would be fired then, rather than at invitation). In your scenario, when a user "registered" they'd be updating an existing signup, but as far as most plugins were concerned, it would be a registration.

It's an interesting approach, and I'll think more about how we can interact with the wp_signups table as part of an invitations/request process and still keep the existing important hooks around registration intact.

Thanks!

[boonebgorges]

This makes it sound like when a user invites a new member, a bare record is created in the wp_signups table. Is this what you were intending?

Yes, that was my idea. My main thought was that we could then avoid duplicating features that already exist in wp_signups - particularly activation_key but perhaps also meta (not 100% sure the latter would be needed). There's a trade-off here: by avoiding duplication, we also require a bit of extra work to avoid some of the default behavior of WP when putting entries into the wp_signups table. So it might not be worth it. I guess the overarching principle is to design for simplicity rather than complexity :-D

[imath]

As we've discussed about during our latest dev-chat. We're too short for 6.0.0 for this feature. But we'll be back on it asap.

[dcavins]

Basic logic for sending/accepting invitations and Legacy template pack interfaces

[dcavins]

Site admins can enable network invitations, even if “anyone can register” is disabled. (Register and Activate screens are still created if invitations are allowed.)

[dcavins]

Users can send invitations by email address to outside users.

[dcavins]

Users can view a list of their sent invitations (and can resend the email or cancel the invite)

[dcavins]

If “anyone can register” is disabled, visiting the register screen results in “not allowed” message.

[dcavins]

If visitor has a valid invitation, access to registration screen is allowed.

[dcavins]

Upon completion of registration form, users who are registering as a result of an invitation are not sent the activation email and are activated (they’ve already verified their email address by responding to the invitation).

[dcavins]

Hello friends,

I’ve attached a first patch that includes the core logic for network invitations and the interface pieces for using network invitations in the Legacy template pack. I would appreciate your feedback about the location of the new code. I’ve placed it in the members component, which make some kind of sense, but do worry that it is kind of dispersed and maybe it would be better to gather it up. I’ve not created a BP component for it, but probably will have to (adding links to the admin bar, for instance), if that changes your opinion. Here’s a summary of what is in the current patch:

• Site admins can enable network invitations, even if “anyone can register” is disabled. (Register and Activate screens are still created if invitations are allowed.)

• Users can send invitations by email address to outside users.

• Users can view a list of their sent invitations (and can resend the email or cancel the invite)

• If “anyone can register” is disabled, visiting the register screen results in “not allowed” message.

• If visitor has a valid invitation, access to registration screen is allowed.

• Upon completion of registration form, users who are registering as a result of an invitation are not sent the activation email and are activated (they’ve already verified their email address by responding to the invitation).

Tasks yet to be completed (invitations):

• Allow non-members to opt out from emails from this site
• BP REST
• Nouveau template pack support
• Central list of invitations in WP Admin. I started this but there is a lot of work in those tables, geez.
• Add any “must have” capabilities from Invite Anyone plugin—limits on number of invitations a user can send, ability to add group invitations to network invite???

Thanks for your feedback!

[imath]

Hi @dcavins,

Thanks again for your work on this first patch. I haven't tested it yet, but here's a first review of the code:

• instead of _e() let's use esc_html_e()
• missing docblock over do_action( 'bp_admin_settings_after_network_invitations' );
• about the 'network' term, I'd suggest to simply use invitations or relationship_invitations instead (to avoid confusion with WordPress use for sites network).
• Invitations are not a component, so I'd probably rename this function bp_is_network_invitations_component() in favor of bp_is_network_invitations_screen()
• the @since keyword of the bp_members_user_can_filter() function should be 7.0.0 😅
• Still in bp_members_user_can_filter() function what to expect about the // @TODO: comment ?
• Looks like the maybe_prevent_activation_emails() function is not finished, was it used for your tests, maybe you should remove it?
• the maybe_make_registration_email_input_readonly() misses a BP prefix eg: bp_members_...
• About bp_network_invitations_add_legacy_welcome_message(), bp_network_invitations_add_legacy_registration_disabled_message(), bp_network_invitations_filter_nouveau_registration_messages( $messages ) , I believe these hooks/functions should be placed into the buddypress-functions.php of the corresponding template packs.
• About bp_network_invitations_add_legacy_registration_disabled_message() the variable $message should be escaped before output.
• In bp_network_invite_user(), bp_network_invitation_delete_invites() you don't need to prefix the hook name when using bp_parse_args() as this function adds it. You can directly use network_invite_user for instance.
• In bp_network_invitation_resend_by_id() I believe you should remove error_log( 'send by id '.print_r( $success, true ) );
• In bp_network_invitation_delete_invites() you should remove extra lines before the closing } of the function.
• I'm curious In bp_network_invitation_get_hash() why using hash_hmac() or wp_generate_password()? wp_hash() is problematic ? As it's possible to filter the hash value, I'd make things easier using wp_hash().
• In bp_invitations_setup_nav() could you align the array parameters like WP Code standards advise it ? For instance

  bp_core_new_nav_item(
      array(
          'name'                    => __( 'Invitations', 'buddypress' ),
          'slug'                    => bp_get_members_invitations_slug(),
          'position'                => 80,
          'screen_function'         => 'members_screen_send_invites',
          'default_subnav_slug'     => 'list-invites',
          'show_for_displayed_user' => $user_has_access
      )
  );
  

• I'd probably move/rename this file src/bp-members/bp-members-notifications.php into the BP Groups component to avoid fatals if the Groups component is not active.
• Ah maybe that was your idea when you wrote the two comments // add_action( 'groups_membership_accepted', 'groups_notification_membership_request_completed', 10, 3 ); and // add_action( 'groups_membership_rejected', 'groups_notification_membership_request_completed', 10, 3 );
• In bp_members_format_notifications() you'll need to review the @since docblock keywords and the name of the hooks.
• What about these commented lines // add_action( 'groups_membership_accepted', 'bp_groups_accept_request_mark_notifications', 10, 2 ); and // add_action( 'groups_membership_rejected', 'bp_groups_accept_request_mark_notifications', 10, 2 );
• In bp_members_screen_notification_settings() you'll need to review the @since docblock keywords and use esc_html_e() instead of _e() and echo esc_html_x() instead of _ex()`.
• In bp_members_invitations_slug(), bp_get_members_invitations_slug(), bp_has_network_invitations(), bp_the_network_invitations(), bp_network_invitations_pagination_count (the filter), bp_get_network_invitations_pagination_links(), bp_get_the_network_invitation_action_links (the filter), you'll need to review the @since docblock keyword.
• Don't forget you still have things // @todo in public function network_invitations_admin_load(), public function invitations_admin() and in public function run_send_action() 😉
• In public function run_send_action() Let's make sure multiline functions are written like this (WPCS) :

  $invite_url = esc_url(
      add_query_arg(
          array(
              'inv' => $invitation->id,
              'ih'  => bp_network_invitation_get_hash( $invitation ),
          ),
          bp_get_signup_page()
      )
  );
  

btw, I'd probably do the esc_url into the 'invites.url' token value.

• In public function run_acceptance_action() you should review the @since docblock keywords in hooks + avoid extra empty lines at the end of the function.
• In src/bp-members/classes/class-bp-network-invitations-list-table.php, you should review all @since docblock keywords of the file and make sure to align variables, and array parameters according to WPCS.
• In src/bp-members/classes/class-bp-network-invitations-template.php, you should review all @since docblock keywords of the file and make sure to align array parameters according to WPCS.
• In src/bp-members/screens/list-invites.php, src/bp-members/screens/send-invites.php & src/bp-members/screens/send-invites.php, you should review all @since docblock keywords and avoid extra spaces into the 3 code lines under Get the action. (both functions) and where you use bp_core_add_message() without the error` type.
• In src/bp-templates/bp-legacy/buddypress/members/single/invitations.php & src/bp-templates/bp-legacy/buddypress/members/single/invitations/invitations-loop.php the @version docblock keyword should be 7.0.0,
• In src/bp-templates/bp-legacy/buddypress/members/single/invitations/invitations-loop.php, please use esc_html_e() instead of _e() and be careful to not forget the @TODO`
• About bp_the_network_invitation_property() I believe I would remove this function and create specific template tags to be able to sanitize/format them when needed, for instance:

  function bp_network_invitation_content() {
      $content = bp_get_network_invitation_property( 'content' );
  
      echo wptexturize( $content );
  }
  

• In src/bp-templates/bp-legacy/buddypress/members/single/invitations/list-invites.php, src/bp-templates/bp-legacy/buddypress/members/single/invitations/send-invites.php & src/bp-templates/bp-nouveau/buddypress/members/single/invitations.php the @version docblock keyword should be 7.0.0 + please use esc_html_e() instead of _e()`
• In src/bp-templates/bp-nouveau/buddypress/members/single/invitations.php you should remove eh?

I'll test the patch to see how it works asap 😉

[dcavins]

Hi @imath, I greatly appreciate your review, but I'm sorry to have wasted your time with formatting and debug statements (and todos and such). I was really hoping for a more limited functionality review, for concerns like your questions about the hashing function. (I chose hash_hmac() because we use it in email functions, but wp_hash() will work just fine.)

There's a lot yet to do, so this patch is an in-progress snapshot, but you've highlighted lots of things I can fix now. Thanks again!

[dcavins]

@imath, a very specific question: What should we call these invitations in cases like the setting name bp-enable-network-invitations? I don't think we can just call that bp-enable-invitations--it's a bit misleading. How about bp-enable-community-invitations as you suggested at last week's dev meeting? Thanks!

[imath]

@dcavins You’re welcome 👌 I’m always happy to help! I’ll test the patch asap.

About my concern on the « network » term and more globally about the network invitations feature : I think the approach @r-a-y chose for the « star » feature of the BP Messages component, could also be used in this case : I like the idea of checking bp_is_active( 'members', 'invitations' ). The more I think of it the more I believe function names should be prefixed bp_members_invitations ( BP / component / feature). So for the option it could be named bp_allow_members_invitations. But if I’m the only one having this concern about the « network » term use, then I’m totally fine to get used to it 😉

[dcavins]

Replying to imath:

I think the approach @r-a-y chose for the « star » feature of the BP Messages component, could also be used in this case : I like the idea of checking bp_is_active( 'members', 'invitations' ).

This is interesting, and I'll add that, but can you help me understand the the goal of the is_active() check? Is it primarily to avoid situations where the plugin files/template files are from different/incompatible versions? It looks in the case of the cover-image functionality that the is_active check is used in combination with a site option check, so I'm guessing that the is_active check would only be false if the component setup class doesn't include the features item? Or is it meant to be filterable by plugins/themes/etc?

Thanks!

[imath]

Sure, the « star » feature is active by default and you can disable it using a dynamic filter available in bp_is_active(). So it can avoid the use of an option. What I like about r-a-y’s approach is that just like component the files for the star feature are only loaded if the feature is active.

I saw in the patch that it was possible to « bypass » the fact registration was disabled by the administrator activating an option to allow members invitations. I’m unsure we should do that. I feel the members invitations should be active by default if user accounts can be created and then use the bp_is_active filter to eventually deactivate it.

I may be wrong but I believe if an admin disallows registration he would also disallow members invitations to be the only one to control who can join.

[dcavins]

Replying to imath:

I saw in the patch that it was possible to « bypass » the fact registration was disabled by the administrator activating an option to allow members invitations. I’m unsure we should do that. I feel the members invitations should be active by default if user accounts can be created and then use the bp_is_active filter to eventually deactivate it.

I may be wrong but I believe if an admin disallows registration he would also disallow members invitations to be the only one to control who can join.

Hi @imath, thanks for your comments. I feel pretty strongly that allowing registration via invitations when public site registration is disabled is a key part of this change, because it allows site owners to use a referral system to build a community of trusted users (and would go a long way toward keeping spam users to a minimum, if that is critical for your community). Because it is a potential opening, I've made the invitations feature an opt-in for the site administrator, so that she must choose to open that vector, rather than be surprised by it. I have added a message to the opt-in section when site registration is disabled, too, to make the "danger" clear (see ​https://buddypress.trac.wordpress.org/attachment/ticket/8139/1-options.png above). Maybe @boonebgorges could weigh in, because it seems to me that a limited registration scheme is important to his plugin Invite Anyone.

Thanks again!

[imath]

Fine with me, you seem pretty sure of yourself 😅, then I would advise you 2 things:

1. Test multisite config asap: as we use WordPress functions there, it may need some more filters eg: on the registration option.
2. I haven't checked it, but could an email be sent to the administrator to inform him about who invited who?

[imath]

I’ve tested the patch on BP Legacy but invitation emails were not sent or resent.

My test config was VVV, single site, registration disabled. I’ve tested on VVV as it comes with mailhog.

When I mention a user, an email is sent to the mentioned user.

[dcavins]

Thanks, @imath. It's necessary to use the "reinstall emails" BP Tool to make sure the needed emails are available. I'm assuming that we can do a delta update emails during the BP update routine. Thanks again!

[imath]

Thanks @dcavins I didn't think about doing so. Well now @espellcaste added a filter to the bp_email_get_schema() function in #8173 : yes ;)

The invitations process looks good to me. The only improvement I'd suggest make sure to have a subject into the invitation email. Thanks for your work on this.

[dcavins]

Since I encountered that email problem earlier, I've wondered if it wouldn't be friendly to write an error to the log when the email logic can't find the email to use. Otherwise it fails silently. Or maybe it doesn't? ​https://github.com/buddypress/BuddyPress/blob/master/src/bp-core/classes/class-bp-email.php#L988

I didn't see any logs when the template didn't exist, did you?

[imath]

Yes, bp_send_emails() returns an error when it fails. Here's what I do in a plugin to send a feedback in this case: ​https://github.com/imath/reception/blob/master/inc/classes/class-reception-verified-email-rest-controller.php#L394

[dcavins]

Same functionality as 8139.01, but incorporates naming suggestions and other improvements suggested by imath.

[dcavins]

I've just attached an .02 patch above, that provides the same basic functionality as .01 (invitations can be sent from legacy template pack, invitations can be accepted). This is early times on this ticket though, and I still need to:

• Add REST API support.
• Add Nouveau template pack support.
• Add a WP Admin management interface.
• Add the "opt-out from invitations from this site" workflow.
• Write tests for all interfaces.

I don't think I'll manage those tasks and have fully tested code in the next week, though, so I think this update should slide to v8, but be committed sooner rather than later.

[imath]

Thanks a lot for your progress about it. I think it's a wise decision you took 👍. Let's take the time to make it great!

[dcavins]

Replying to imath:

Thanks a lot for your progress about it. I think it's a wise decision you took 👍. Let's take the time to make it great!

Thanks @imath. I'm looking forward to doing some development work in Nouveau and the REST API. Can you recommend a Nouveau component that is a good "ideal" to use as inspiration? Same for the REST API: do you or @espellcaste have a suggestion about a nice clean endpoint to model?

[imath]

The Group's members management UI is interesting because it uses the BP REST API and can be used in Admin and on front-end :)

​https://buddypress.trac.wordpress.org/browser/trunk/src/bp-groups/js/manage-members.js

[dcavins]

Member invitations are functional in legacy theme; admin management screen added at /wp-admin/users.php?page=bp-members-invitations

[dcavins]

I am attaching my current in-process patch. There's plenty still left to do, but there's also a lot of work that has been done.

What is currently working:
Site admins can enable site invitations at BuddyPress Settings > options.
In BP Legacy, members can visit their profile at /members/me/invitations/ to invite new members to join the site and view sent invitations (or act on them by resending or canceling).
In both Legacy and Nouveau, Invited members receive a link like ​http://bpdev.local/register/?inv=19&ih=ebcdb3681ddc215e79f46d2fe17b56e0 where the ih parameter is a hashed key of sorts using some info from the inviting user (see bp_members_invitations_get_hash()). This key allows invited users to join the site even if "anyone can join" is not enabled (and site invitations are enabled).
A list table of all site invitations is available to the admin at /wp-admin/users.php?page=bp-members-invitations.

What is not yet built:
Member's invitation profile pane is not appearing in the WP Admin Bar nav.
BP REST needs a new members invitations endpoint to allow for the fetching/creation/resending/deletion of members invitations.

• Fetching can be accomplished via bp_members_invitations_get_invites(). Should be restricted to sending user and admins.
• Creation via bp_members_invitations_invite_user()
• Resend via bp_members_invitation_resend_by_id()
• Delete via bp_members_invitations_delete_by_id() or bp_members_invitations_delete_invites() to bulk delete.

Nouveau needs the user's pane to send new invitations and also to resend/delete existing invitations.
Clean up invitations when a user is deleted.
Invitation and email logic will need to rely on opt-out work happening in #8448.

Thanks for any help and/or comments you can give!

[dcavins]

Member invitations are functional in legacy theme; admin management screen added at /wp-admin/users.php?page=bp-members-invitations. Fix many incorrect version notes.

[imath]

Thanks a lot for your patch and directions. I’ll look at it in details asap 👌

[imath]

Hi @dcavins,

I've just tested the patch. It works pretty well, thanks a lot for your work on it. I'm now going to work on the Nouveau integration. Here are some possible improvements I'm suggesting about current patch :)

• Activate and register page could be automatically created if registration are disabled and the admin chose to activate network invites.
• "Pending Invites" instead of "Sent invites" for the user's front-end profile
• "Send Invite" instead of "Invite new members" for the user's front-end profile
• Some information about what will happen once the form has been submitted.
• Form to send invites as default screen for the user's front-end profile
• "was invited by user and became a registered member" for the new member activity
• Screen notification/email notification to inform the user was successfully invited.
• New Email setting to disable email notification
• Install email on upgrade (see example in #8428)
• #8428 The welcome email, should any specific message be included in it?
• Let's move Invitations Management into BuddyPress tools. We should also to the same progressively for Signup Management I believe.
• ​8139.04.fixInvitationsManagementNotices.patch is fixing some notices. FYI it's possible to reach the resend confirmation screen using the Resend Bulk action even for accepted invites.
• in bp_get_members_invitations_pagination_count() make sure to use strict comparison eg: 1 === (int) $query_loop->total_invitation_count
• BP_Members_Admin::members_invitations_admin_load() this part looks weird ( -1 == $doaction ) + some strict comparison to make eg: 'do_resend' === $doaction, 'do_delete' === $doaction
• I missed the missing strict comparison about 'resend' === $action in 8139.04.fixInvitationsManagementNotices.patch
• Another strict comparison missing in BP_Members_Invitations_Template::invitations() -> $this->current_invitation + 1 === $this->current_invitation_count.

[imath]

@dcavins, in ​8139.04.improveOutputSanitizationAndDoNouveau.patch, you'll find:

• ✅ Nouveau integration.
• Output sanitization recommandations.

Above is a screenshot in Nouveau + some direction about how I think the member invitations front-end display should look (it's not in the patch).

This looks really promising, looking forward to see it available in 8.0.0 ;)
I wish you "courage" for the remaining work to make it happen!

[dcavins]

Progress combo patch, changes many behaviors since .04

[dcavins]

I've just uploaded a new patch that changes some behaviors and adds some new functionality beyond the .04 patch. Changes are as follows:

• Add the "invitations" menu item to the user menu in the WP admin bar.
• Move management of invitations into BP Tools tabbed interface.
• Store the accepted invitation ID as user meta at signup completion for use later.
• Change the activity item text to "{Invitee} accepted an invitation from {inviter} and became a registered member" when the registration happened as the result of an invitation.
• Create register and activate pages if invitations are allowed and "anyone can register" isn't checked.
• Switch up default screen order per imath's request.
• Add instructions to top of "send invite" form.
• Imath's patches are integrated, so Nouveau is supported. :)

Other comments:
Yes, current users cannot be invited.
From the WP Admin management screen, if you attempt to resend accepted invites, those invites will be filtered out and not present in the list of emails to be sent.

Thanks again for your testing and feedback!

[imath]

Wow! These are great improvements 💪👏 I Started a review, and I'll test it asap.

[dcavins]

Yes, a lot of work has been done, but there're still a few things to do! Thanks again for your testing and feedback!

[imath]

Hi @dcavins, thanks a lot for your progress about the feature: we're getting close!

• ​8139.05.refreshed.patch is a refreshed version of your patch to take in account the latest commit I did recently.
• ​8139.05.review.patch contains my recommandations and more integration with BP Nouveau

Here's some explanations about my recommandations (the 8139.05.review.patch suggests fixes):

• There's a PHP 8.0 deprecated usage of a required parameter after an optional one in BP_Members_Invitation_Manager->run_acceptance_action().
• The BuddyPress tools sub menu is not highlighted when viewing the Invitations Management screen.
• The BP_Members_Invitations_List_Table views and action links were still using the users.php base url (instead of tools.php).
• I hope we'll fix this ticket before BP 78.0.0! There was some inline comment typos like this.
• I also saw some WP code formatting issues and I've added some escaping functions at various places.
• wp_doing_ajax() was introduced in WP 4.7, let's use it!
• I've added the JavaScript & CSS parts for the member's Pending invites front-end screen: the PHP part still needs to be done.

About the tests I did:

• The email subject is empty a BP Email invite is sent.
• The email invitation unsubscribe link doesn't seem to be handled yet.
• I believe it would be better to say "Hi Name" instead of "Hi email" into the email salutation.
• A failing test appeared in BP_Tests_Invitations::test_bp_invitations_add_invitation_avoid_duplicates since .05.patch.
• The super admin cannot view another member's Invitations front-end pages, is this on purpose? If so, maybe the navigations shouldn't be displayed for him.

I'm confident you'll be able to have it ready soon. Keep up the good work!

[imath]

Oh, and I forgot to point you to this ticket #8459 It looks like some changes were made upstream about checking user_can with a regular visitor (ID = 0, or exist cap)

[dcavins]

Thanks for your thorough review, @imath! Here are my questions and comments:

I've added the JavaScript & CSS parts for the member's Pending invites front-end screen: the PHP part still needs to be done.

Can you explain what you mean here? Is this for Nouveau? The invite actions (resend/delete) are working for me in Nouveau, so I'm not sure what is missing.

Adding a name to the invitation email.

I'm not opposed to this idea, but think we should move it to a future enhancement, because I think it would require adding a meta table for invitations to store that info. (We could pass it through from the form for the initial email, but then resends would miss that info.)

The email subject is empty a BP Email invite is sent.

The email subject is filled in on my installation. Maybe you could use the "reinstall emails" tool on your dev environment to test? My example emails say "Adminer has invited you to join bpdev" for the subject, where "adminer" is the display name of the user who created the invite.

The email invitation unsubscribe link doesn't seem to be handled yet.

Right, that was relying on the new opt-outs code to be added, so I should be able to do it easily now.

The super admin cannot view another member's Invitations front-end pages, is this on purpose? If so, maybe the navigations shouldn't be displayed for him.

The super admin should be able to see the list, at least. Showing the "send invites" form seems a little weird, since the admin should be doing that elsewhere. Do you agree?

Thanks again for your review and your patches!

[imath]

Hi!

You're welcome!

Sorry I wasn't very clear about the first point, I agree. I was referring to the JavaScript I've added to add the dynamic part for the bulk actions (check all checkoxes, only make the bulk submit button enabled if a bulk action is selected. But I haven't done the backend part to actually handle these bulk actions.

About the second point, I agree, but we shouldn't display the email it seems weird to do so, Let's just say "Hi" not "Hi email". I'll look into it, because I had to deal with it in a plugin I've built. So I'll work on a separate ticket for this specific part.

I did reinstall the emails 😔, but no changes. It must be my config. If it's working for you, then great!

Awesome for the unsubscribe link.

About the super admin, I'm fine with the invites screen only, but as he can manage this from the tool screen, I believe it's easier to completely disable the full Nav.

Let's carry on!

[dcavins]

Hi @imath,

The empty subject issue is strange. I had a problem like that early on when the required tokens weren't being supplied to the email function. But it's working for me in both legacy and nouveau, so I'm concerned about your issue. Could {{inviter.name}} not be set on your setup somehow? It's also used in the email content, so...

The super admin not having access to a user's invitations screen was related to the switch of the default screen from "list" to "send" and the admin not having access to the "send" screen (I'm using bp_is_my_profile() to limit access to that screen). I've resolved the issue by switching the nav parameters to
'default_subnav_slug' => ( $user_can_send && bp_is_my_profile() ) ? 'send-invites' : 'list-invites',
I think we should leave admin access there, because some people just prefer to do work on the front end.

Thanks again for the review!

[dcavins]

Gathers previous patches; adds notifications and opt-out handling.

[dcavins]

The newest patch merges the previous work with imath's patches and adds some new functionality:

• New emails are installed on upgrade.
• Screen notification is set up to let inviter know of invitation acceptance. (I haven't worked with notifications in a while, so I'm going to have to remind myself how to also send an email as part of the notification routine.)
• Unsubscribe links in membership invitation emails now result in opt-out being recorded. However, the success feedback is recorded via bp_core_add_message(), but the messages aren't displayed on the home page, where the user ends up. Opinions welcome on how better to handle that.
• Site admin can manage a user's invitations from their profile.
• BP tools menu item stays highlighted when working on invitations management screen.

Thanks for your feedback!

[imath]

Thanks a lot for the updated patch 💪, I'll test it tomorrow morning!

[imath]

Hi @dcavins !

We're even closer 😉 Here's the result of my tests on a fresh WP trunk install:

• I confirm the Invitation Email has a subject ✅
• I really don't like the salutation "
• I confirm the un-subscriptions are handled ✅
• I confirm it's not possible to invite a user who has opted out, or an existing user ✅
• I had a notice error about a missing 'member_id' argument into bp_email_set_default_headers() 😬.
• The screen notification is cool 👍
• *But* The invite screen notification is not "sent" to all inviters if the user was invited by more than one member. (I haven't checked the invitations table to see if all invites are updated as accepted for the email, but if not I believe it should).
• *But* When you click on WP Admin Bar bubble link, it's not cleared from the active notifications. User needs to go to the member's notifications screen to mark it as read.
• Watch out: BP_Tests_Invitations::test_bp_invitations_add_invitation_avoid_duplicates is still failing since .05.patch 😉.

About the success feedback when a user unsubscribed. Let's start with a wp_die() There's only one thing we need to know in this case: are we out of it!

Possible improvements:

1. I believe the error message when sending an invite fails should be improved: "There was a problem sending that invitation. Possible reasons are: this email is the one of an existing member, or the user has opted out from being invited to this site, you have no luck!")
2. I clicked on an invite link, registered and then clicked again on the invite link and I can reach the registration form again. I believe we should at least check the email when an invitation link has been clicked and at the very least add an error, eg: "you are already a member of the site, please login. If you forgot your password, go this way {link to wp-login/lostpassword action}".
3. When I click again on an unsubscribe link, the feedback message should be different. I can imagine some anxious people clicking more than once on this link, so telling them "Hey we got you the first time, you don't need to unsubscribe again, you already are!" will make them happy.

Very important improvement, I even think it's a blocker:
When users opt out we can't keep usable data about them, that's why we are hashing their emails. So as soon as a user opt outs, the corresponding invitation data must be erased. I was able to resend an invitation from the regular member's pending invite screen as well as being an admin from the Invitation Management screen. Not even the admin can do that.

PS: I haven't reviewed the code this time.

8139.06.companion.patch is fixing the PHP notice and suggests to use the wp_die() way to confirm the user unsubscribed.

[dcavins]

Thanks again for your feedback.

The email salutation is set in bp_email_get_salutation(). If we don't want to show the email address, it looks like we'll have to change the logic there or add a filter on that hook to replace the salutation with just "Hi".

Thank you for solving the opt-out message display. Your answer is perfect, and I didn't realize that wp_die() could do that, though it seems obvious now.

Thanks also for fixing the warning in bp_email_set_default_headers(). I had to rejigger some things to make nonmember unsubscribe links work better yesterday and missed that piece.

Regarding who should be notified when a user accepts an invitation. I am of two minds about this. If two users invite the same person, the new user will have had to accept the invitation via the link in one of the emails. So I decided that the followed link should determine which invitation was primarily accepted, and that inviter is used in the activity item and receives a notification. The other invitations are marked as accepted, but do not receive notifications. I can see the notification part both ways. One possibility is that we could send a notification to the other inviters that the person is now a member of the site, so they can go friend them or introduce themselves.

Yep, the notification needs to be marked "read" when they follow the link. That's on my to-do list.

I'll see what the test is failing about.

I agree with all of your improvement suggestions, though I thought the "you are already a member" when attempting to follow an accepted invite was already in there at one point.

Your last point about not keeping the invitations after opt-out is also true and will be simple to implement.

Thanks!

[dcavins]

I've attached another patch. In this patch:

• Apply imath's patch.
• Mark notification as read on visit to new user profile.
• Add notification to users who invited new member (but weren't sender of "accepted" invite). They get "username is now a member of the site" while the accepted user gets "username accepted your membership invitation"
• Delete invites when someone opts out.
• Add "you're already a member!" message for accepted invitations.
• What is the correct way to escape the html that might contain a url in bp_members_invitations_get_registration_welcome_message() and bp_members_invitations_add_legacy_welcome_message()?

Opt-outs

• Add a check in BP_Email::validate() to make sure user hasn't opted out before sending them an email. 
• Add a new message for repeat opt-out link clickers: "You have already unsubscribed from all communication from this site."

[dcavins]

This is just the code that is new in .07 and not in .06.

[dcavins]

Fix tests, and new warnings in BP_Email::validate change.

[dcavins]

In 12916:

BP_Invitation_Manager: Verify passed email addresses.

Ensure that incoming email addresses pass
an is_email() check.
Also fix a documentation error in BP_Invitation.

See #8139.

[dcavins]

In 12917:

Member Invites: Introduce BP_Members_Invitation_Manager.

Introduce new BP_Members_Invitation_Manager class, bp_user_can filter.

See #8139.

[dcavins]

In 12918:

Member Invites: Add creation, deletion functions.

Introduce functions that apply the logic in
BP_Members_Invitation_Manager, including:

• bp_members_invitations_get_invites
• bp_members_invitations_user_has_sent_invites
• bp_members_invitations_invite_user
• bp_members_invitation_resend_by_id
• bp_members_invitations_delete_by_id
• bp_members_invitations_delete_invites
• bp_members_invitations_get_hash
• bp_get_members_invitation_from_request

See #8139.

[dcavins]

In 12919:

Member Invites: Add core template functions.

Add class BP_Members_Invitations_Template and
template functions in BP_Members_Template.

See #8139.

[dcavins]

In 12920:

Member Invites: Add screens.

Introduce new logic on registration screens
and list-invites and send-invites screens.

See #8139.

[dcavins]

In 12921:

Member Invites: Add Nouveau template pack.

Add templates and logic for outputting the
member invitations screens.

Super props imath.

See #8139.

[dcavins]

In 12922:

Member Invites: Add Legacy template pack.

See #8139.

[dcavins]

In 12923:

Member Invites: Change behavior of registration form.

Add custom messages to the registration form when the user
is accepting an invitation.

See #8139.

[dcavins]

In 12924:

Member Invites: Add items to WP Admin bar account menu.

Add links to a user's invitations screens to the WP Admin Bar menu.

See #8139.

[dcavins]

In 12925:

Member Invites: Add notifications.

Add screen notifications for letting users know when their invitee
accepts an invitation.

See #8139.

[dcavins]

In 12926:

Member Invites: Filter "new member" activity item.

If a new user is accepting an invite, filter the
message to reflect that.

See #8139.

[dcavins]

In 12927:

Member Invites: Add WP Admin management screen.

Add WP Admin Screen to manage member invites
as tab on the BuddyPress > Tools screen.

See #8139.

[dcavins]

In 12928:

Member Invites: Add setting to enable member invites.

Member invites are enabled on two levels:

• Invites can be disabled in code by filtering

bp_is_active( 'members', 'invitations' ).

• Invitations can be enabled for a BuddyPress site

by visiting the BP settings > options and checking
"Allow registered members to invite people to join this network"

See #8139.

[dcavins]

In 12929:

Member Invites: Install emails on upgrade to 8.0.

Include the new emails and descriptions and install them
during the upgrade to 8.0.

See #8139.

[imath]

In 12930:

Member Invites: add missing /* translators: */ inline comments

This commit also:

• adds some tiny improvements to get along with WP Coding Standards,
• removes a forgotten PHP notice using the right $args variable instead of the undefined $all_args one in bp_members_invitations_get_registration_welcome_message()
• removes an extra <hr class="wp-header-end"> into the Manage Invitations Admin screen that was causing admin notices to appear twice.
• Uses the wpautop() function instead of an inexisting wpautop filter into bp_members_invitations_add_legacy_welcome_message() and bp_members_invitations_add_legacy_registration_disabled_message().

Huge Props dcavins for this awesome new feature!

See #8139

[imath]

Hi @dcavins

I've just retested everything, it works great! The above commit is fixing some tiny details. Once you had a good rest, the last step will be to implement bulk actions for Member's front end pending invites screen. Thanks again for your great work on this feature 💪.

[imath]

In 12932:

BP Email: introduce "unnamed" salutation

The member invites feature's goal is to invite external users to join a community. The BP Email feature uses a named salutation by default. As a result, the salutation of invitation emails was "Hi email@…,". The user's name is not yet known and using this kind of salutation is odd. The member invites feature's need is to simply use an "unnamed" salutation eg: "Hi,". To satisfy it we are updating the BP Email type schema with a new property $named_salutation which defaults to true. If this property is false, then the sent email will from now on use an unnamed salutation.

NB: to prevent potential back compatibiliy issues, each kind of salutation has their own filters

• Use bp_email_get_unnamed_salutation to edit the unnamed one.
• Use bp_email_get_salutation to edit the default one (named).

Props dcavins for coloring outside the lines a bit 😉

See #8139

[dcavins]

In 12933:

Member Invites: Update a few action hooks.

Use members_invitations as the prefix when possible.

See #8139.

[dcavins]

In 12934:

BP Email: Variable name change.

Props imath.

See #8139.

[dcavins]

In 12935:

Member Invites: Add BP Legacy bulk management template parts.

Add the template function
bp_members_invitations_bulk_management_dropdown()
and the JavaScript required to check/uncheck all invites.
Also remove an unneeded empty column from the
legacy pending invites view.

See #8139.

[dcavins]

In 12936:

Member Invites: Add bulk action handler.

Handle front end bulk management requests
from Legacy and Nouveau Templates.

See #8139.

[dcavins]

In 12938:

Member Invites: Change permissions check in bulk management.

In bp_members_invitations_action_bulk_manage(),
only continue if the current user is managing his own
invitations or is a site admin.

Props imath.

Fixes #8139.

[dcavins]

Centralize some "user can" logic to make it filterable and consistent.

[dcavins]

I've just added a patch that centralizes some of the invitations screen access logic to make it more consistent between the bp_members_invitations_setup_nav() and bp_members_admin_bar_add_invitations_menu() implementations.

I've also added a couple of new user_can capabilities to allow plugins to modify the access. For instance, I'm building a plugin that allows users to choose characteristics of who can send invites, like maybe only site admins can, or only members who have belonged to the site for more than a week. This change allows access to the "send" screen even if the user can't actually send, so they can be shown an error message if desired. The core BP behavior is not changed: users who can't send invites can't reach the send invites screen. But a plugin could allow access to the send screen and still set "can send invites" to false.

Thanks for your feedback!

[dcavins]

An example usage of the "can reach send screen but can't send" combo.

[imath]

Hi @dcavins

I'm fine with the suggested improvements. In ​8139.09.02-capabilities.patch, you'll find some suggestions mainly about the Nouveau integration :

• Instead of inserting the Feedback HTML output, we should use the template function bp_nouveau_user_feedback() and add the feedback messages into the $feedback_messages array inside the bp_nouveau_get_user_feedback() function.
• Still in this function I've edited the message ID for the case no invitations are pending as it had previously a conflicting name with the groups no pending invites one (my bad).

I've also edited the 'bp_members_invitations_view_send_screen' capability check as when I'm logged in as a regular user I wasn't able to view the restricted message you used into your screenshot. Maybe you should give an extra check about this one 😉.

Once you're positive about this last point, don't hesitate to commit.

[dcavins]

In 12945:

Member Invites: Centralize access logic.

• User must be logged in to pass bp_members_send_invitation cap.
• Add new capabilities bp_members_invitations_view_screens

and bp_members_invitations_view_send_screen.

• Use new capabilities in bp_members_admin_bar_add_invitations_menu()

and bp_members_invitations_setup_nav() so that the logic is
consistent in both places.

See #8139.

[dcavins]

In 12946:

Member Invites: Add "not allowed" message to invite send form.

BuddyPress Core will not allow you to access the send form
unless you have the bp_members_send_invitation
capability, so this message will generally not be accessed.
However, if a plugin wishes to allow access to the "send"
screen but prevent sending in order to display an error
message, this message will be used.

Props imath.

See #8139.

[dcavins]

Handle dynamic default screen handling.

[imath]

Hi @dcavins as it's not really possible to edit the default subnav item I believe your patch is fine. But it should be possible to edit the default subnav: see #8471

Looking deeply at it, I have some other questions:

1. Why all actions are not into the src/bp-members/actions directory (bp_network_invitations_catch_send_action() and bp_members_invitations_action_handling()) ?
2. Why aren't you hooking to bp_members_setup_nav instead of bp_setup_nav ?

More globally (it's not specific to the Invitations feature, it revealed it) I must say I'm a bit concerned by the late_includes hook as it happens before our navigation system is built so we can't be positive about what's screen function is being run when opening the user's primary nav item.

[dcavins]

Replying to imath:

I've also edited the 'bp_members_invitations_view_send_screen' capability check as when I'm logged in as a regular user I wasn't able to view the restricted message you used into your screenshot. Maybe you should give an extra check about this one 😉.

Hi imath, you won't get that custom message unless a plugin is providing it. You can see my proof-of-concept invite limiter code here ​https://github.com/dcavins/bp-invitation-limiter

Regarding your other questions:

• Sure I can put all the actions-hooked items into the actions file. I sort of don't like that filing system (it's a little like putting all of your spreadsheets in one directory on your desktop), because I would prefer functionally related items to be stored together--like all the invitations code together rather than filed by whether it's a filter or action-hooked function. But I don't have strong feelings about it.

Thanks again for your review!

[imath]

@dcavins I’ve been thinking about the current_action potential issue. I think I’d feel safer if there was only one file to use for screens : this way all functions would be available no matter the order of subnav items.

[imath]

I'd feel safer with ​8139-load-one-inviations-screen.patch. This way no matter the order of the subnav is, actions will be completed. The patch is also replacing some network_invitations terms in nonce or variables.

[dcavins]

In 12951:

Member Invites: Change screen file loading logic.

• Move send and list invitations load actions to single screen file.

This avoids an issue where the current action isn't known
as of late_includes and could, in unusual cases, cause the
wrong screen file to be loaded.

• Replace several vestigial 'network_invitations' mentions.

Props imath.

See #8139.