Skip to:
Content

Opened 17 months ago

Closed 15 months ago

Last modified 12 months ago

#5374 closed enhancement (fixed)

Administation screen to manage signups

Reported by: imath Owned by: imath
Milestone: 2.0 Priority: high
Severity: normal Version:
Component: Component - Members Keywords: 2nd-opinion has-patch needs-testing
Cc:

Description

As discussed in dev chats, it can be interesting to "build" a new administration screen to manage/moderate the registrations. I've been playing in this area lately, and i think i'll be able to suggest a first patch in a few days. I've benchmarked Boone's unconfirmed plugin and a plugin i built a while ago to get some inspiration.

My idea is to add a new view in the WP List Table of the users.php page at the end of the list of roles.
We could have actions like "Activate" or "Delete" the pending account, i've also noticed, there were a site option available in multisite configs called banned_email_domains, these domains are checked during the registration process. So we could also add an option to ban email domains just before deleting the registration.

Here's a screenshot of my idea of the Signups Administration screen :

http://farm3.staticflickr.com/2873/12328044663_cb8fb9d933_o.png

Here are the main difficulties i'm seeing :

  • A unique class (eg "BP_Members_Signups") to get the signups whatever WordPress config is
  • bulk activate and notification emails, as i think a user whose account has been activated needs to be informed
  • On non multisite config, "Pending accounts" are already users with a specific usermeta and a user_status set to 2 and as such, they are listed in blog's users page, and took in account in the count_users() WordPress function, so we will surely need to "filter" the WP_User_Query used in the WP List Table

Found a relative ticket : #4651

Attachments (18)

5374.diff (70.2 KB) - added by imath 17 months ago.
5374.02.diff (73.1 KB) - added by imath 17 months ago.
5374.03.diff (76.2 KB) - added by imath 17 months ago.
5374.04.diff (75.4 KB) - added by imath 17 months ago.
5374.05.patch (84.5 KB) - added by boonebgorges 16 months ago.
5374.06.patch (84.6 KB) - added by imath 16 months ago.
5374.07.patch (84.7 KB) - added by imath 16 months ago.
5374.08.patch (84.9 KB) - added by boonebgorges 16 months ago.
5374.adminbar-bubble.diff (6.9 KB) - added by imath 16 months ago.
5374.submenu.diff (4.4 KB) - added by imath 16 months ago.
5374.unviewed-signups.diff (42.3 KB) - added by imath 16 months ago.
5374.not-network-activated.diff (2.2 KB) - added by imath 16 months ago.
5374.signupid.diff (1.6 KB) - added by imath 15 months ago.
5374.sign-up-disabled.patch (3.1 KB) - added by johnjamesjacoby 15 months ago.
5374.sign-up-disabled.02.diff (5.3 KB) - added by imath 15 months ago.
5374.sign-up-disabled.03.cleanup.diff (2.5 KB) - added by imath 15 months ago.
5374.sign-up-disabled.03.nocheck.diff (2.0 KB) - added by imath 15 months ago.
5374.sign-up-disabled.03.link.diff (1.7 KB) - added by imath 15 months ago.

Download all attachments as: .zip

Change History (88)

comment:1 @ircbot17 months ago

This ticket was mentioned in IRC in #buddypress-dev by imathfromparis. View the logs.

comment:2 follow-up: @boonebgorges17 months ago

Excellent!

I've gotten really good feedback on the Unconfirmed plugin, so I would recommend mirroring its feature set: the ability to resend activation emails, to activate manually, and to delete registrations.

Really looking forward to seeing a patch.

comment:3 in reply to: ↑ 2 @imath17 months ago

  • Keywords has-patch needs-testing added

Replying to boonebgorges:

Really looking forward to seeing a patch.

Here it is :)

Thanks a lot for your last comment Boone. I've followed your advice and i've added the ability to resend activation emails as it was a feature my work in progress didn't include. It also completely changed the way i've build this first patch.

You can see a screen capture of the result of the 5374.diff patch.

First, this patch is changing the registration process for non multisite configs. At the beginning i wanted not to change too much things in this area, but i've change my mind progressively.

The main change is that i think we should really use the $wpdb->signups table even for non multisite configs, because :

  1. Sign-ups are not 'yet' users, and today they are in the users list and in the amount of users that WordPress count_users() function returns
  2. In case a non multisite config becomes a multisite one, the sign-ups will already be in the right place
  3. Having one way of getting datas is really helpful for the different actions that the administration will add, especially to inform the community administrator on how many times he resent an activation link and on what was the last time he sent it.

So, first part of the patch takes care of creating the table and "migrating" eventual "old" sign-ups in it for non multisite config (only if sign-ups are allowed). Then it introduces a new class to dialog with this table and take care of the 3 actions (and corresponding bulk actions) that are available in the sign-ups administration screen :

  • Resend an activation link (once per day max)
  • Delete a sign-up
  • Activate a sign-up

The Sign-ups Administration screen will be available as illustrated on the screen capture from one of the links of the WP_Users_List_Table views. it includes a search, reordering sign-ups by id (date of registration) or username and each action must be confirmed in a specific screen.
Finally, the patch as i've said earlier changes the sign up process to use the $wpdb->signups table instead of usermeta/user_status for non multisite configs.

I've tested it in multisite, BuddyPress network activated and not, and in non multisite config. It seems to work fine.
I think it can be improved by maybe creating a set of functions to call the sign-up class, and for multisite configs we could add a "moderation tool" in order to include an option to add email domains to the WordPress blacklist before deleting a signup.

What do you think?

@imath17 months ago

comment:4 @imath17 months ago

I forgot that plugins might use the user_status + activation_key mechanism. It's problematic. I'm afraid this patch is a bit too "direct". We could keep on creating users but removing their capacities till the signup is activated so that the count_user() WordPress function don't take them in account.

@imath17 months ago

comment:5 @imath17 months ago

5374.02.diff keeps on creating users for plugins compatibility unless a constant is defined.

@imath17 months ago

comment:6 @imath17 months ago

5374.03.diff checks $wpdb->users user_status field before running the signups management actions (delete/resend/activate) in case this field has been modified without using BuddyPress functions (for instance by directly using an update query).

comment:7 @ircbot17 months ago

This ticket was mentioned in IRC in #buddypress-dev by imathfromparis. View the logs.

comment:8 @henry.wright17 months ago

Hi imath

I took a look at your patch and have some feedback on what you're starting to put in place.

Even though the ticket is specifically designed to improve the management of signups from a site admin back-end point of view, I think functions that you've introduced such as check_user_status pave the way for a far more friendly front-end registration process.

For instance, if a user with user_status == 2 were able to sign in before activating their account - check_user_status could be used to disable important functionality for these users such as compose messages, update profile, activity comment etc whilst still allowing them to browse around as a logged in member. I've found the requirement to visit their email account and click the activation link before they can log in is off-putting for a subset of users - almost like closing the door in their face. Giving them a 'locked-down glimpse' of what they can do on the website might motivate more users to visit their email and hit the activation link.

Apologies if this went slightly off-topic but reading through your patch gave me the idea.

Last edited 17 months ago by henry.wright (previous) (diff)

comment:9 @imath17 months ago

hi henry.wright

Thanks for your comment, let's first check with other core developers that the patch can be applied before modifying the meaning of user_status = 2 ;)

@imath17 months ago

comment:10 @imath17 months ago

5374.04.diff is the last version of the patch updated to trunk, so that it's easier to test

comment:11 @ircbot17 months ago

This ticket was mentioned in IRC in #buddypress-dev by imathfromparis. View the logs.

@boonebgorges16 months ago

comment:12 follow-up: @boonebgorges16 months ago

imath - I've had a chance to review this patch in some detail. Very nice work - I think it's going to be a huge improvement.

5374.05.patch is a refresh, with the following improvements:

  • Change the name of the new class to BP_Signup and moved to a new file bp-members-classes.php. This seems more consistent to me.
  • Eliminated some overly-specific methods in BP_Signup, and added some parameters to the main get() method so that it'd do all the necessary work.
  • Added unit tests for BP_Signup
  • Improved inline docs, codestyling, escaping
  • Changed name of "Mail Count" column to "# Times Resent". I don't love either of these phrases, but I think that mine is a little clearer.
  • Added an Activate link to the row-actions. I know that clicking the username would lead to the Activate screen, but IMO it was not clear enough. Now you have both options :)
  • Cleaned up the text on the Help tab
  • Changed BP_Signup so that it handles serializing/unserializing meta values. Arrays in and arrays out - let the database method do the dirty work.
  • BP_Signup::add() returns the ID of the new item on success, rather than true. Mainly makes unit testing easier.
  • Changed the name of the backward compatibility constant to BP_SIGNUPS_SKIP_USER_CREATION, and added some additional inline explanatory text. The constant is a very good idea, but the idea is sorta confusing, and it's very important that plugin devs etc understand it, so I wanted to make it as clear as possible.

Thoughts for improvements:

  • It's confusing to have the Mail link simply disappear when within 24 hours of the last resend. Maybe we could change the color or have some hover-text or disable it *or something*.

Aside from that, everything seemed to work well. I tested MS vs non-MS, as well as upgrades from 1.9, and I couldn't find any glaring bugs :) See what you think of my changes. Once you and I have agreed on a patch that's technically viable, let's try to get a couple people to test the patch before committing - there are a lot of possible configs to test against, and I'd like a rough sense that it's working before putting it into trunk.

Thanks for your great work on this!

@imath16 months ago

comment:13 in reply to: ↑ 12 @imath16 months ago

Replying to boonebgorges:

I've had a chance to review this patch in some detail. Very nice work - I think it's going to be a huge improvement.

Thanks a lot boonebgorges & for all this great improvements :)

I've tested it on multisite with BuddyPress not network activated and on a non multisite config. I've found 3 details, explaining 5374.06.patch :)

1- Bulk Actions are "$_POSTed" instead of "$_GETed", so i've modified
2- Concerning signups username check when the constant BP_SIGNUPS_SKIP_USER_CREATION is set to true, a letter was missing in a var. so it wasn't checking
3- in the resend method, the old meta was lost as soon as a mail was sent, so i've added a line ;)

Other than this little details, i confirm it's working great !!

It's confusing to have the Mail link simply disappear when within 24 hours of the last resend. Maybe we could change the color or have some hover-text or disable it *or something*.

i understand, i think that we could simply print the text with no link instead of not showing it at all.

Finally, i agree, it would be great to have some tests on various configs to see if every case is covered. I think after the commit, it can also be interesting to write a note on bpdevel to inform of the changes about the way signups will be managed. And maybe progessively stop creating users to 'BP_SIGNUPS_SKIP_USER_CREATION' by default ;)

comment:14 follow-up: @boonebgorges16 months ago

1- Bulk Actions are "$_POSTed" instead of "$_GETed", so i've modified
2- Concerning signups username check when the constant BP_SIGNUPS_SKIP_USER_CREATION is set to true, a letter was missing in a var. so it wasn't checking
3- in the resend method, the old meta was lost as soon as a mail was sent, so i've added a line ;)

Ha ha, I'm a bit sloppy. Thanks!

i understand, i think that we could simply print the text with no link instead of not showing it at all.

I'm tempted to be less heavy-handed than this. If a site admin wants to resend an activation email sooner than this, I don't think we should prevent it. Maybe we want to *discourage* it. How about this: Leave the Email link there all the time. Then, on the confirmation screen, show a message that says when the user was last notified. If it's within the last 24 hours, change the message to a warning, maybe with bold styling or red letters, that lets them know that it's been less than a day, and maybe they should consider waiting. But continue to let the email be sent. Does that sound OK to you? (I'm not sure what this will mean for bulk actions - can you let me know?)

comment:15 in reply to: ↑ 14 @imath16 months ago

Replying to boonebgorges:

(I'm not sure what this will mean for bulk actions - can you let me know?)

i think we can display a "red" message after the name on the confirmation screen. It will need to change the way the resend bulk action works to stop not sending the message. I think it can easily be done.
I agree with your plan. I'll update the patch asap ;)

@imath16 months ago

comment:16 @imath16 months ago

5374.07.patch allows to send an activation email even if one was sent less than 24 hours ago.
In the confirmation screen (bulk or regular actions) the list of users to confirm in case of a resend action will include an information about the last date an activation email was sent and if that date is less than 24 hours ago a message will be displayed after that date. The goal is to *discourage* admin to send more than one per day.
In the signups list, the email row actions is now always shown.

I've found a fatal error today with latest WP 3.9 if the db_raw_version is already set to 7892 and the admin allow the registrations, then on a non multisite we create the signups table. So we need to make sure upgrade.php is loaded. That's what i've included in this patch.

comment:17 @ircbot16 months ago

This ticket was mentioned in IRC in #buddypress-dev by boonebgorges. View the logs.

comment:18 follow-up: @DJPaul16 months ago

I have reviewed some of the patch:

  • In the changes tobp_update_to_2_0(), why do we need to strip whitespace from $user_login? Is this something WPMU does when adding to the signups table?
  • delete_user_option( $signup->ID, 'capabilities' ); -- this looks dangerous. Is this really the best way just to hide this user from the regular user list until the account is activated? What happens if an admin edits a user's WP profile before the user activates their account? I think would WP re-build these options.

comment:19 in reply to: ↑ 18 @imath16 months ago

Replying to DJPaul:

I have reviewed some of the patch:

Thanks a lot.

  • In the changes tobp_update_to_2_0(), why do we need to strip whitespace from $user_login? Is this something WPMU does when adding to the signups table?

Yes see function wpmu_validate_user_signup() : https://core.trac.wordpress.org/browser/trunk/src/wp-includes/ms-functions.php#L472

  • delete_user_option( $signup->ID, 'capabilities' ); -- this looks dangerous. Is this really the best way just to hide this user from the regular user list until the account is activated? What happens if an admin edits a user's WP profile before the user activates their account? I think would WP re-build these options.

Actually, each BuddyPress members on a multisite config is by default created this way, see wpmu_create_user() : https://core.trac.wordpress.org/browser/trunk/src/wp-includes/ms-functions.php#L1096
I think it's on the contrary a good way to avoid some "troubles". As today, when a user registers on a non multisite config, he's a user so he can easily be edited (changing password, email and all) by the admin.
Using this, the user won't be listed, the users count will be consistent and the user won't be in pending accounts and in users list at the same time.
To edit the user, the admin must go into his MySql software, get the id and open it in the user-edit.php page. What happens in this case: the role select box displays, just as it's the case in multisite config : "No role for this site".
I've tested updating the user: it doesn't change his user_status (remains to 2) and when the user activate his account, it works fine.

comment:20 @ircbot16 months ago

This ticket was mentioned in IRC in #buddypress-dev by imathfromparis. View the logs.

comment:21 follow-up: @boonebgorges16 months ago

  • Keywords commit added

I've had one more look through this patch, and I think it's ready to go.

I've attached 5374.08.patch, which makes the following changes:

  • Changed column names: 'Last Sent' instead of 'Last email'; '# Times Emailed' instead of '# Times Resent'. The former seems clearer to me, and the latter is more correct (because the counter goes to 1 when the user first registers, so it hasn't yet been *re*sent)
  • Put the "Last notified" string on its own line, which I think makes things clearer.
  • Some localization cleanup

imath, I think you should go ahead and commit this if you feel good about it. The more time we have for users to test, the better.

@boonebgorges16 months ago

comment:22 in reply to: ↑ 21 @imath16 months ago

Replying to boonebgorges:

imath, I think you should go ahead and commit this if you feel good about it. The more time we have for users to test, the better.

Great!! Thanks for the new patch and improvements i'll run some more tests on single, multisite BuddyPress network activated and not, to be sure, and i'll commit it ;)

comment:23 @imath16 months ago

In 8119:

Introduce Sign Ups Management

In Users Administration Screen, a new view is now available to manage the pending accounts of a site or of the network of sites. The following actions are supported:

  • Resend the activation email
  • Delete the pending account
  • Activate the pending account

The corresponding bulk actions are also supported. A search box is available in order to let the administrator easily find some specific pending accounts.

The registration process have also been modified so that multisite and regular configs handles it in a similar way. A mechnanism is in place to ensure plugin backward compatibility concerning the regular configs.

See #5374

props boonebgorges, imath

Fixes #4651

comment:24 @imath16 months ago

  • Keywords commit removed

I've re-run the tests on the different configs before committing the patch. I leave the ticket open in case a specific config is having troubles.

comment:25 follow-up: @johnjamesjacoby16 months ago

After updating to latest trunk on my local, I see no indication that this new feature exists. I purposely looked around the admin UI without looking into exactly how the code reveals itself, and couldn't find it. Any help? :)

comment:26 @boonebgorges16 months ago

In 8121:

Don't use mysql_real_escape_string() in BP_Signup

This causes problems on PHP 5.5+, where the mysql_* functions have been
deprecated.

See #5374

comment:27 @boonebgorges16 months ago

In 8122:

Better verbiage for "Pending Accounts". See #5374

comment:28 @boonebgorges16 months ago

johnjamesjacoby - Dashboard > Users > Pending

comment:29 follow-up: @boonebgorges16 months ago

In 8123:

Register the signup table in the members global even if registration is disabled

Conditionally registering the table causes problems for our unit tests. It also
means that we have to worry about table creation when enabling registration
later on.

See #5374

comment:30 in reply to: ↑ 29 @imath16 months ago

Replying to boonebgorges:

It also means that we have to worry about table creation when enabling registration
later on.

Thanks a lot boonebgorges for your latest commit. I'll need to double check but i think it's ok as during the table creation process, i don't use the members global table but $wpdb->signups or bp_core_get_table_prefix() . 'signups'

comment:31 in reply to: ↑ 25 @imath16 months ago

Replying to johnjamesjacoby:

After updating to latest trunk on my local, I see no indication that this new feature exists. I purposely looked around the admin UI without looking into exactly how the code reveals itself, and couldn't find it. Any help? :)

Thanks a lot for your feedback johnjamesjacoby. As boonebgorges replied, the Sign-ups Management screens are located in "Dashboard > Users > Pending".

Once on the users page, you'll see the "Pending accounts" view. Above the WP_List_Table :
All | Administrator | ... | Pending accounts

But, this is a really interesting feedback. As i'm completely in it, i know where to manage signups, users might have the same difficulties you've experienced finding it.

I've been thinking about it for a few hours. I'm not sure we should display the users submenu, or create a new menu for it.

We could use the WP Pointer after the upgrade to tell the Admin "Signups can be managed here". Then i thought, it could be interesting to help the Admin find it when needed: "when new signups he's not aware of were created". So a bit like the comment bubble in the Admin bar, i suggest that in case of new signups, we display a specific bubble in the Admin bar with a link to directly access to the screen. See the image below (i'm not sure about the color, but we can't miss the bubble with this one ;) ).

http://farm4.staticflickr.com/3822/13127201734_6e4dc9f119_o.png

I've built a quick patch (5374.adminbar-bubble.diff) that checks every 6 hours for signups that were created after the last time the Admin went on the signup Administration screen.

What do you think of this suggestion ?

comment:32 follow-up: @boonebgorges16 months ago

Cool idea, imath! I like it.

I'm thinking it may also be worthwhile to add a Pending submenu to the Users top-level menu. What do you think of that?

comment:33 in reply to: ↑ 32 @imath16 months ago

Replying to boonebgorges:

Cool idea, imath! I like it.

I'm thinking it may also be worthwhile to add a Pending submenu to the Users top-level menu. What do you think of that?

Thanks boonebgorges :)

Actually adding a Pending submenu was my first idea, but i thought it wasn't a good idea. I may be wrong. I explain why :
The Pending accounts is simulating a view after the list of roles in users.php. I thought why having the submenu in this case as we don't have submenus for "All", "Administrator", etc.. That's why i've chosen the admin bar.

What's happening in this part is that i actually create a submenu, i hide it in favor of the view in users.php.

comment:34 @boonebgorges16 months ago

I thought why having the submenu in this case as we don't have submenus for "All", "Administrator", etc.

True. But think about this from the point of view of the user. Site admins sometimes visit the Users menu to manage existing users - say, to change their roles. For this purpose, they're probably not thinking "I'd better find the Administrators menu, so I can demote an administrator", they're thinking "I need to modify users". So the Users menu makes sense. But *pending* users somehow seem like a different category - certainly, the actions that you take with respect to pending accounts (resend email, activate, etc) are much different from the actions you take with existing users. So, I think that a Users > Pending submenu in addition to the 'Pending' filter on the Users page, might be justified (if redundant).

comment:35 @imath16 months ago

I've just tested see 5374.submenu.diff and edited some verbiage in the signup WP_List_Tables to match with r8122

You surely right :)

@imath16 months ago

comment:36 follow-up: @boonebgorges16 months ago

Cool, I say go with submenu.diff.

I took a closer look at the "bubble" patch. I like the idea, but I think you've overthought it. It's like a "notifications" icon. If I understand correctly, what we're trying to do is show the admin how many unviewed or "new" signups are pending. So why not do this: when a new signup is created, increment some value ('_bp_unviewed_signups'); use that value to populate the bubble; then set it back to 0 when an admin views the Signups page. The 6-hour transient, and the count-since-last-registered query, seems unnecessary to me.

comment:37 @imath16 months ago

In 8136:

Display a Manage Signups submenu to Users menu in wp-admin

Signups management is simulating a new view in the wp-admin users page. A Pending link is available after the different WordPress views to filter the users by roles. It appears the Signups screen can be a bit difficult to find for site or network administrators. As a result, it is best to display the Manage Signups submenu instead of removing it.

See #5374

props johnjamesjacoby, boonebgorges

comment:38 in reply to: ↑ 36 @imath16 months ago

Replying to boonebgorges:

I like the idea, but I think you've overthought it.

Yes you're definitely right, thanks for your feedback!! Actually, now we have the submenu, i think it's best to add the "bubble" there instead of the WP Admin Bar (that would avoid to think to configs where the BP Admin bar is used).
So i've built another patch in the way you suggested : the use of a '_bp_unviewed_signups' option. I hope i haven't overthought it again :) But i think it could be interesting to save an array of unviewed signup ids instead of incrementing an int and to count this array for the bubble, then we could use it in the WP_List_Table to add a left border on the rows of the unviewed signups like WordPress does for comments. This way the admin would easily see who are the unviewed ones.

http://farm4.staticflickr.com/3746/13168837113_75023164f4_z.jpg

Original Size

The other benefit i see is that the unviewed signups can be updated if the Admin goes on the signups management page, but also if a user activates his account, so when an account is activated, i check for his signup id and eventually remove it from the '_bp_unviewed_signups' option.

You'll find this new suggestion in 5374.unviewed-signups.diff, what do you think ?

comment:39 @imath16 months ago

Note to self: there's a problem with signups administration screens when BuddyPress is not network activated and the user is a regular Admin of the blog where BuddyPress is activated.

comment:40 follow-up: @boonebgorges16 months ago

Cool idea, imath. I like this better than the toolbar menu item.

comment:41 in reply to: ↑ 40 @imath16 months ago

Replying to boonebgorges:

Cool idea, imath. I like this better than the toolbar menu item.

Thanks for your feedback :)

comment:42 @imath16 months ago

On config where BuddyPress is not network activated, i've noticed that the fact that a regular admin can access to the manage signups sceen was problematic as we are bailing if !is_super_admin() when actions are posted. The patch 5374.not-network-activated.diff removes the access to signups management to regular admins.

comment:43 @ircbot16 months ago

This ticket was mentioned in IRC in #buddypress-dev by imathfromparis. View the logs.

comment:44 @imath16 months ago

In 8148:

Make sure Signups Management is restricted to Super Admin when BuddyPress is not network activated

On a multisite config, when BuddyPress is not network activated, we need to prevent a regular admin to access to the signups management screens as the different actions requires Super Admin privileges.

See #5374

comment:45 @imath16 months ago

In 8152:

Add admin menus to manage signups only if they are allowed

Before adding the menu to manage signups we must be sure the site allows people to register.

See #5374

comment:46 @imath16 months ago

About 5374.unviewed-signups.diff i'm hesitating...

When sites have a lot of signups, an admin can be "annoyed" to have the counting unviewed signups bubble in Users admin menu. Maybe we should add a constant or a new setting to allow the admin deactivate the bubble.

What do you think ?

comment:47 follow-up: @boonebgorges15 months ago

Maybe we should add a constant or a new setting to allow the admin deactivate the bubble.

This seems like overkill. If we think it's going to be annoying to users (as the admin of a few large sites, I can imagine how it might be), let's not include it at all. This could be something provided by a plugin instead.

comment:48 in reply to: ↑ 47 @imath15 months ago

Replying to boonebgorges:

This could be something provided by a plugin instead.

Yeah i agree. I think we simply need to help plugin developers to create this feature by passing the signup_id into the bp_core_signup_user action (5374.signupid.diff).

@imath15 months ago

comment:49 follow-up: @boonebgorges15 months ago

  • Owner set to boonebgorges
  • Status changed from new to assigned

Yes, and/or the entire BP_Signup object.

comment:50 @boonebgorges15 months ago

  • Owner changed from boonebgorges to imath

comment:51 in reply to: ↑ 49 @imath15 months ago

Replying to boonebgorges:

Yes, and/or the entire BP_Signup object.

In this part i think we're using static methods so i'm not sure we have it, i'll check to be sure.

comment:52 @johnjamesjacoby15 months ago

In 8184:

Clean up BP_Signup class.

  • Fix issue with sign-up meta not being populated correctly.
  • Remove stray semi-colons.
  • Code formatting clean-up.
  • See #5374.

comment:53 follow-up: @johnjamesjacoby15 months ago

I'd like to propose we remove the bp_get_signup_allowed() checks here. It's currently not possible to moderate sign-ups while they are disabled, because the UI isn't available.

comment:54 in reply to: ↑ 53 ; follow-up: @imath15 months ago

Replying to johnjamesjacoby:

I'd like to propose we remove the bp_get_signup_allowed() checks here. It's currently not possible to moderate sign-ups while they are disabled, because the UI isn't available.

Well, lately (r8152) i've added the bp_get_signup_allowed() check before the $this->signups_page = add_users_page(...) precisely because the UI was loaded on a non multisite blog causing a SQL error as the table $wpdb->signups wasn't created.
I'm hesitating, or we should check the signups table is available.

comment:55 in reply to: ↑ 54 @johnjamesjacoby15 months ago

Replying to imath:

I'm hesitating, or we should check the signups table is available.

What happens when sign-ups are turned on without the update routine being processed? I'm weary of not converting the sign-ups always, and the state that leaves single site installations with existing pending sign-ups in.

comment:56 @imath15 months ago

Sorry johnjamesjacoby i think i replied too quick! you're creating the table even if signups are not allowed during upgrade so my case wouldn't exist.

comment:57 follow-up: @boonebgorges15 months ago

IMO we should always be creating the table. Remove this check too: https://buddypress.trac.wordpress.org/browser/trunk/bp-core/admin/bp-core-schema.php#L54

comment:58 in reply to: ↑ 57 @imath15 months ago

Replying to boonebgorges:

IMO we should always be creating the table. Remove this check too: https://buddypress.trac.wordpress.org/browser/trunk/bp-core/admin/bp-core-schema.php#L54

Yes, maybe in case we have no items in the WP_List_Table and if signups are not allowed we should then display a link to the setting to allow signup

comment:59 @boonebgorges15 months ago

Yes, maybe in case we have no items in the WP_List_Table and if signups are not allowed we should then display a link to the setting to allow signup

Good idea.

comment:60 @imath15 months ago

5374.sign-up-disabled.02.diff is johnjamesjacoby's patch + https://buddypress.trac.wordpress.org/browser/trunk/bp-core/admin/bp-core-schema.php#L54 and a link to edit settings if no items and ! bp_get_signup_allowed()

comment:61 follow-up: @boonebgorges15 months ago

This looks fine to me (though maybe you could isolate the relevant changes into a single commit, separate from some of the other miscellaneous things in that patch)

comment:62 in reply to: ↑ 61 @imath15 months ago

Replying to boonebgorges:

This looks fine to me (though maybe you could isolate the relevant changes into a single commit, separate from some of the other miscellaneous things in that patch)

Thanks for your feedback, i'll do it tonight.

comment:63 @imath15 months ago

To prepare the commits, just split 5374.sign-up-disabled.02.diff in 3 parts :

  • 5374.sign-up-disabled.03.cleanup.diff
  • 5374.sign-up-disabled.03.nocheck.diff
  • 5374.sign-up-disabled.03.link.diff

comment:64 @johnjamesjacoby15 months ago

Looks good to me. Thanks for separating everything out.

Commit at your leisure.

comment:65 @imath15 months ago

In 8195:

Clean up bp_update_to_2_0() and BP_Members_Admin class

Improve code formatting

props johnjamesjacoby

See #5374

comment:66 @imath15 months ago

In 8196:

Remove the bp_get_signup_allowed() checks in Sign-ups management feature

  • Create $wpdb->signups table even if sign-ups are not allowed on single blog config
  • Display the UI to manage sign-ups even if signups are not allowed

This way, administrator who temporarly deactivated registrations will still be able to moderate the pending signups.

props johnjamesjacoby, boonebgorges

See #5374

comment:67 @imath15 months ago

In 8198:

Add a link to Registration settings in Sign-ups Administration screen

In case registration is disabled and no pending sign-ups were found display a link to the blog or network settings.

props imath, boonebgorges

See #5374

comment:68 follow-up: @boonebgorges15 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

I'm calling this one :) Let's open separate tickets for further issues.

Thanks for your work imath! Great new feature.

comment:69 in reply to: ↑ 68 @imath15 months ago

Replying to boonebgorges:

I'm calling this one :) Let's open separate tickets for further issues.

Thanks for your work imath! Great new feature.

you're welcome & thanks a lot to you and johnjamesjacoby :)

comment:70 @sooskriszta12 months ago

Paves the way for #4132

Note: See TracTickets for help on using tickets.