Skip to:
Content

BuddyPress.org

Opened 6 months ago

Closed 5 months ago

Last modified 6 weeks ago

#8540 closed enhancement (fixed)

Add caching and other enhancements to BP_Signup class.

Reported by: dcavins Owned by: dcavins
Milestone: 10.0.0 Priority: normal
Severity: normal Version: 9.0.0
Component: Members Keywords: has-patch dev-reviewed commit
Cc: dcavins

Description

While working with the BP_Signup class for network membership requests, I've found a few ways it can be improved to behave more consistently.

  • Add caching for Signup SQL queries and BP_Signup objects.
  • Synchronize signup result format provided by get() and construct() methods.
  • Add action points so that the cache items can be cleaned up.
  • Use cached functions rather than making direct database calls.
  • Improve behavior of meta-updating method.

Also, I believe that I can resolve #7938 while I'm familiar with the mechanics of this class.

Attachments (6)

8540.1.patch (22.1 KB) - added by dcavins 6 months ago.
Improve behavior of BP_Signup class.
8540-change-active-handling.patch (3.7 KB) - added by dcavins 6 months ago.
Improve handling of active status in signups class.
user-already-active.png (21.5 KB) - added by dcavins 6 months ago.
The second patch changes the query behavior so that already activated signups are recognized on the activation screen.
8540.1.suggestions.patch (5.0 KB) - added by imath 5 months ago.
8540-change-active-handling.suggestions.patch (1.1 KB) - added by imath 5 months ago.
8540.2.patch (27.6 KB) - added by dcavins 5 months ago.
Improvements on original patch (including imath's suggestions, corrections to tests, and improved documentation in the BP_Signup class)

Download all attachments as: .zip

Change History (13)

@dcavins
6 months ago

Improve behavior of BP_Signup class.

@dcavins
6 months ago

Improve handling of active status in signups class.

@dcavins
6 months ago

The second patch changes the query behavior so that already activated signups are recognized on the activation screen.

#1 @dcavins
6 months ago

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

#2 @imath
6 months ago

Really nice! This looks very promising. Thanks a lot for your work on it. I'll test the patch & give it a deeper look asap.

#3 @imath
5 months ago

  • Keywords dev-reviewed added; dev-feedback removed

Hi @dcavins

Thanks again for your work on these improvements and sorry about my late review about them.

I've tested 8540.1.patch. I believe there's an issue with the way the sent_date is set.
See the screenshot below:

https://cldup.com/LDBQmonqWv.png

I believe when the sent_date is not set or equals to '0000-00-00 00:00:00', it should be set with the registered date of the signup. I've included this suggestion and some WP PHP Code standards ones into 8540.1.suggestions.patch.

After applying 8540-change-active-handling.patch, I get a failing test:

1) BP_Tests_BP_Signup::test_objects_should_be_cached
Failed asserting that 48 matches expected 0.

/buddypress/tests/phpunit/testcases/members/class-bp-signup.php:550

In 8540-change-active-handling.suggestions.patch you'll find some other formatting suggestions 😉.

Finally I confirm I can register, activate, and that the admin can see, resend, activate and delete the new signup into the Manage Signups screen. I also confirm an already activated account trying to use the activation key again into the activation screen get the error message you shared into your screenshot 👍

#4 @dcavins
5 months ago

Thanks for the review @imath!

The reason for my change to the calculation of the sent_date when it is not set is that in the current situation it makes sense to assume the activation was sent at registration time (though we don't record it). However, in the case of the new signup being a *site membership request*, it's important to know that the activation mail was not yet sent. I'm thinking that a data update task when BP10 is installed will be to explicitly set the sent_date and count_sent for existing signups so that we can discern between old signups where we assume the date and number of times sent, and new requests that have never been sent. (A problem with that will be that that data is stored in a serialized array, so I believe we'll have to loop over each BP_Signup in php, ugh.)

In any case, we can apply your logic now, and I'll change it as needed for requests to work. Thanks!

@dcavins
5 months ago

Improvements on original patch (including imath's suggestions, corrections to tests, and improved documentation in the BP_Signup class)

#5 @imath
5 months ago

  • Keywords commit added

Thanks for updating the patch @dcavins it looks good!

#6 @dcavins
5 months ago

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

In 13098:

Improve BP_Signup class.

  • Add caching for BP_Signup SQL queries and BP_Signup objects.
  • Synchronize signup result format provided by get() and construct() methods.
  • Add action points so that the cache items can be cleaned up.
  • Use cached functions rather than making direct database calls.
  • Improve behavior of meta updating method.

Fixes #8540.

#7 @dcavins
6 weeks ago

In 13165:

BP_Signup improvements.

On multisite, when resending actvation emails, send the
right kind (user or user + site). Also ensure that default
meta is set via wpmu meta filters.

introduce bp_members_get_signup_by() and
bp_members_site_requests_enabled().

Correct default value calculation in BP_Signup
to allow backcompat.

See #8540.

Note: See TracTickets for help on using tickets.