Skip to:
Content

BuddyPress.org

Opened 7 weeks ago

Closed 3 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 7 weeks ago.
Improve behavior of BP_Signup class.
8540-change-active-handling.patch (3.7 KB) - added by dcavins 7 weeks ago.
Improve handling of active status in signups class.
user-already-active.png (21.5 KB) - added by dcavins 7 weeks 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 6 weeks ago.
8540-change-active-handling.suggestions.patch (1.1 KB) - added by imath 6 weeks ago.
8540.2.patch (27.6 KB) - added by dcavins 5 weeks 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 (12)

@dcavins
7 weeks ago

Improve behavior of BP_Signup class.

@dcavins
7 weeks ago

Improve handling of active status in signups class.

@dcavins
7 weeks ago

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

#1 @dcavins
7 weeks ago

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

#2 @imath
7 weeks 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
6 weeks 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 weeks 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 weeks ago

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

#5 @imath
5 weeks ago

  • Keywords commit added

Thanks for updating the patch @dcavins it looks good!

#6 @dcavins
3 weeks 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.

Note: See TracTickets for help on using tickets.