#8139 closed enhancement (fixed)
Network Invitations and Membership Requests
Reported by: | dcavins | Owned by: | dcavins |
---|---|---|---|
Milestone: | 8.0.0 | Priority: | normal |
Severity: | normal | Version: | 5.0.0 |
Component: | Registration | Keywords: | has-patch |
Cc: | dcavins |
Description
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
- 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.)
- 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 averified
column as well (that would be markedtrue
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!
Attachments (25)
Change History (109)
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
5 years ago
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
5 years ago
#4
@
5 years ago
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!
#5
@
5 years ago
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:
- User visits the BP register page
- User enters WP account info (username, password, email) and BP profile data, as well as blog info if enabled
- A new entry is created in the
signups
table, which includes user-provided data as well as a confirmation code - 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.
- User clicks through and confirms registration
- The entry in the
signups
table is converted to a WP user, a site is created if relevant. These steps are handled by WP. - 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).
#6
@
5 years ago
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!
#7
@
5 years ago
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
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
5 years ago
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
5 years ago
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
5 years ago
#11
@
5 years ago
- Milestone changed from 6.0.0 to Up Next
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.
@
4 years ago
Site admins can enable network invitations, even if “anyone can register” is disabled. (Register and Activate screens are still created if invitations are allowed.)
@
4 years ago
Users can view a list of their sent invitations (and can resend the email or cancel the invite)
@
4 years ago
If “anyone can register” is disabled, visiting the register screen results in “not allowed” message.
@
4 years ago
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).
#13
@
4 years ago
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!
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
4 years ago
#15
@
4 years ago
- Keywords needs-refresh added
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 useesc_html_e()
- missing docblock over
do_action( 'bp_admin_settings_after_network_invitations' );
- about the 'network' term, I'd suggest to simply use
invitations
orrelationship_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 ofbp_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 thebuddypress-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 usingbp_parse_args()
as this function adds it. You can directly usenetwork_invite_user
for instance. - In
bp_network_invitation_resend_by_id()
I believe you should removeerror_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 usinghash_hmac()
orwp_generate_password()
?wp_hash()
is problematic ? As it's possible to filter the hash value, I'd make things easier usingwp_hash()
. - In
bp_invitations_setup_nav()
could you align the array parameters like WP Code standards advise it ? For instancebp_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 useesc_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
inpublic function network_invitations_admin_load()
,public function invitations_admin()
and inpublic 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
@sincedocblock 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 be7.0.0
, - In
src/bp-templates/bp-legacy/buddypress/members/single/invitations/invitations-loop.php
, please useesc_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 be7.0.0
+ please useesc_html_e() instead of
_e()` - In
src/bp-templates/bp-nouveau/buddypress/members/single/invitations.php
you should removeeh?
I'll test the patch to see how it works asap 😉
#16
@
4 years ago
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!
#17
@
4 years ago
@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!
#18
follow-up:
↓ 19
@
4 years ago
@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 😉
#19
in reply to:
↑ 18
@
4 years ago
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!
#20
follow-up:
↓ 21
@
4 years ago
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.
#21
in reply to:
↑ 20
@
4 years ago
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!
#22
@
4 years ago
Fine with me, you seem pretty sure of yourself 😅, then I would advise you 2 things:
- Test multisite config asap: as we use WordPress functions there, it may need some more filters eg: on the registration option.
- I haven't checked it, but could an email be sent to the administrator to inform him about who invited who?
#23
@
4 years ago
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.
#24
@
4 years ago
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!
#25
@
4 years ago
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.
#26
@
4 years ago
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?
#27
@
4 years ago
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
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
4 years ago
@
4 years ago
Same functionality as 8139.01, but incorporates naming suggestions and other improvements suggested by imath.
#29
@
4 years ago
- Milestone changed from 7.0.0 to Up Next
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.
#30
follow-up:
↓ 31
@
4 years ago
- Keywords has-patch added; needs-refresh removed
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!
#31
in reply to:
↑ 30
@
4 years ago
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?
#32
@
4 years ago
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
This ticket was mentioned in Slack in #buddypress by imath. View the logs.
4 years ago
@
4 years ago
Member invitations are functional in legacy theme; admin management screen added at /wp-admin/users.php?page=bp-members-invitations
#35
@
4 years ago
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()
orbp_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!
@
4 years ago
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.
#37
@
4 years ago
- Keywords needs-refresh added
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
.
#38
@
4 years ago
@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 makes me think, do you check the user is not already a member before sending the invite ? If not, I think you should add this check 😬
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!
#39
@
4 years ago
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!
#40
@
4 years ago
- Keywords needs-refresh removed
Wow! These are great improvements 💪👏 I Started a review, and I'll test it asap.
#41
@
4 years ago
Yes, a lot of work has been done, but there're still a few things to do! Thanks again for your testing and feedback!
#42
follow-up:
↓ 44
@
4 years ago
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 oftools.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!
#43
@
4 years ago
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)
#44
in reply to:
↑ 42
@
4 years ago
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!
#45
@
4 years ago
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!
#46
@
4 years ago
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!
#47
@
4 years ago
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!
#49
@
4 years ago
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:
- 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!")
- 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}".
- 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.
#50
@
4 years ago
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!
#51
@
4 years ago
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()
andbp_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."
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
4 years ago
#68
@
4 years ago
- Keywords needs-patch added; has-patch removed
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 💪.
#74
@
4 years ago
- Owner set to dcavins
- Resolution set to fixed
- Status changed from assigned to closed
In 12938:
#75
@
4 years ago
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!
This ticket was mentioned in Slack in #buddypress by dcavins. View the logs.
4 years ago
#77
follow-up:
↓ 81
@
4 years ago
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 thebp_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.
#80
@
3 years ago
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:
- Why all actions are not into the
src/bp-members/actions
directory (bp_network_invitations_catch_send_action()
andbp_members_invitations_action_handling()
) ? - Why aren't you hooking to
bp_members_setup_nav
instead ofbp_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.
#81
in reply to:
↑ 77
@
3 years ago
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!
#82
@
3 years ago
@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.
#83
@
3 years ago
- Keywords has-patch added; needs-patch removed
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.
Hi @dcavins
Thanks for starting working on this. Here are my first thoughts.
I would suggest to first list :
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...