Skip to:
Content

BuddyPress.org

Opened 10 months ago

Last modified 3 months ago

#8144 new defect (bug)

Use of bp_core_signup_send_activation_key not checked while querying for signups

Reported by: zishanj Owned by:
Milestone: 7.0.0 Priority: normal
Severity: normal Version:
Component: Members Keywords: has-patch
Cc:

Description

Use of filter bp_core_signup_send_activation_key is not checked while querying for signups which result in display of wrong values for date_sent and count_sent.

Attachments (9)

signup-activationkey.diff (1.0 KB) - added by zishanj 10 months ago.
signup-activationkey.2.diff (1.0 KB) - added by zishanj 10 months ago.
added braces
signup-activationkey.3.diff (1.0 KB) - added by zishanj 10 months ago.
added spaces
8144.patch (5.3 KB) - added by imath 10 months ago.
8144.2.patch (5.3 KB) - added by zishanj 10 months ago.
fixed patch 8144
8144.3.patch (5.3 KB) - added by zishanj 10 months ago.
fixed naming convention sent_date in bp-members-functions.php
8144.4.patch (5.5 KB) - added by zishanj 10 months ago.
fixed 2 more naming issues with sent_date
8144.5.patch (5.6 KB) - added by zishanj 10 months ago.
fixed issue with not displaying 'Unsent' in Last Sent column in class-bp-signup.php
8144.6.patch (5.7 KB) - added by zishanj 10 months ago.
fixed undefined_index

Download all attachments as: .zip

Change History (28)

#1 @imath
10 months ago

  • Keywords has-patch needs-refresh added
  • Milestone set to 6.0.0

Good catch!

Thanks for this new patch.

Let's improve it making sure to respect some WordPress coding standards adding some spaces and braces :)
-> https://developer.wordpress.org/coding-standards/wordpress-coding-standards/php/#brace-style

Last edited 10 months ago by imath (previous) (diff)

@zishanj
10 months ago

added braces

@zishanj
10 months ago

added spaces

#2 @espellcaste
10 months ago

Can I reuse that hook so that we don't duplicate it?

@imath
10 months ago

#3 @imath
10 months ago

  • Keywords reporter-feedback added; needs-refresh removed

Thanks a lot for adding spaces and braces to your patch @zishanj. I can see a situation where using the filter to get the sent date of the activation email can be wrong. If signups were made before or after the filter was added by the site owner.

That's why I think it's safest to trace the "Unsent" activation email information into the Signups meta like I did in 8144.patch

What do you think ?

(btw @espellcaste thanks for your feedback)

#4 @zishanj
10 months ago

That previous patch shows the empty column for Last Sent and Email Sent is displayed 0. I couldn't found any issue so far with the patch or may be I missed it. I have applied your patch 8144 and it should not show the values in columns Last Sent and Email Sent should be 0 as there was no email sent yet. Here is the screenshot.

Last edited 10 months ago by zishanj (previous) (diff)

#5 @zishanj
10 months ago

Patch 8144 works with new signup

@zishanj
10 months ago

fixed patch 8144

#6 @zishanj
10 months ago

There is issue with naming convention of var date_sent and sent_date in patch 8144 and 2 fixes in class-bp-signup.php. Its working fine now with both old and new signups. New patch uploaded.

Last edited 10 months ago by zishanj (previous) (diff)

@zishanj
10 months ago

fixed naming convention sent_date in bp-members-functions.php

#7 @zishanj
10 months ago

@imath Need to further review these naming convention date_sent and sent_date in patch 8144.3

@zishanj
10 months ago

fixed 2 more naming issues with sent_date

@zishanj
10 months ago

fixed issue with not displaying 'Unsent' in Last Sent column in class-bp-signup.php

#8 @zishanj
10 months ago

seems working perfect now. screenshot

#9 @imath
10 months ago

@zishanj Thanks for all your patches, but I believe you're too focus on your specific case :) The right way to go with the issue is 8144.patch because it covers all cases.

I've tested your 8144.5.patch and I get a notice error, an empty cell, and a wrong information about one of the signup:
https://cldup.com/hNHOCxg1wq.png

  1. The regular case is: the signup confirmation is sent (in other words the bp_core_signup_send_activation_key is not used` ) 8144.patch is displaying the date this confirmation was sent.
  2. bp_core_signup_send_activation_key is used: 8144.patch is making sure all new signups will have the right count to 0 and Unsent as the displayed date.
  3. bp_core_signup_send_activation_key was used before the patch was applied: signups have a wrong count + a wrong date, but that's logic - the patch wasn't there when these signups happened.

I advise you to first test the regular case (remove the filter).

So to me 8144.patch is the MVP to fix this issue :)

@zishanj
10 months ago

fixed undefined_index

#10 @zishanj
10 months ago

Original 8144 do had some issues with naming convention of sent_date and date_sent and with setting count_sent, sent_date values. Patch 8144.6 will fix that undefined_index issue.

#11 @imath
10 months ago

8144.6 is wrong again :)

Please start by testing the regular case (remove the filter).

Changing the naming might be problematic for other people extending the signup object. I believe it's safer to keep it the way it is.

#12 @zishanj
10 months ago

please see comment #5. Its also important to use sent_date instead of date_sent otherwise its value will be empty in other file which result in wrong value.

#13 @imath
10 months ago

its value will be empty in other file which result in wrong value.

Could you be more specific about "the other file" ? Which one is it ?

#14 @zishanj
10 months ago

see these files:
src/bp-members/classes/class-bp-members-admin.php
src/bp-members/classes/class-bp-members-list-table.php
src/bp-members/classes/class-bp-members-ms-list-table.php

and then in file src/bp-members/classes/class-bp-signup.php there should be additional check in case sent_date meta is not set.

#15 @zishanj
10 months ago

Sorry about the confusion. Just review it again in depth, here are some suggested changes in file src/bp-members/classes/class-bp-signup.php from patch 8144:

On line 243:

$signup->date_sent = $signup->registered;

Should be changed to:

$signup->date_sent = '0000-00-00 00:00:00';

I think it should be default to 0 otherwise showing registered date confuse the admin if he has disabled the send registration key.

On line 262:

$signup->count_sent = 1;

should be changed to

$signup->count_sent = 0;

It should be default to 0 to avoid confusion in case admin has disabled the send registration key.

#16 @imath
10 months ago

  • Keywords reporter-feedback removed

I disagree.

The default behavior is to send the activation key. A wide majority of users do so.

Changing as you suggested in your last comment, will fix your specific configuration and will confuse the wide majority of BuddyPress users.

8144.patch takes care of solving the issue from now on & for both behaviours. I've rechecked adding/removing the filter.

#17 @zishanj
10 months ago

It should not effect the user who have enabled the send registration key as there is now sent_date meta. But it will effect the user who have disabled the send registration key because it will show the non-zero value by default when it has not sent the email.

#18 @imath
5 months ago

  • Milestone changed from 6.0.0 to Up Next

As we haven't come to a compromise about it. Let's carry on discussing during next dev cycle.

#19 @imath
3 months ago

  • Milestone changed from Up Next to 7.0.0
Note: See TracTickets for help on using tickets.