Skip to:
Content

BuddyPress.org

Opened 2 years ago

Last modified 12 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: Under Consideration 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 2 years ago.
signup-activationkey.2.diff (1.0 KB) - added by zishanj 2 years ago.
added braces
signup-activationkey.3.diff (1.0 KB) - added by zishanj 2 years ago.
added spaces
8144.patch (5.3 KB) - added by imath 2 years ago.
8144.2.patch (5.3 KB) - added by zishanj 2 years ago.
fixed patch 8144
8144.3.patch (5.3 KB) - added by zishanj 2 years ago.
fixed naming convention sent_date in bp-members-functions.php
8144.4.patch (5.5 KB) - added by zishanj 2 years ago.
fixed 2 more naming issues with sent_date
8144.5.patch (5.6 KB) - added by zishanj 2 years 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 2 years ago.
fixed undefined_index

Download all attachments as: .zip

Change History (29)

#1 @imath
2 years 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 2 years ago by imath (previous) (diff)

@zishanj
2 years ago

added braces

@zishanj
2 years ago

added spaces

#2 @espellcaste
2 years ago

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

@imath
2 years ago

#3 @imath
2 years 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
2 years 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 2 years ago by zishanj (previous) (diff)

#5 @zishanj
2 years ago

Patch 8144 works with new signup

@zishanj
2 years ago

fixed patch 8144

#6 @zishanj
2 years 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 2 years ago by zishanj (previous) (diff)

@zishanj
2 years ago

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

#7 @zishanj
2 years ago

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

@zishanj
2 years ago

fixed 2 more naming issues with sent_date

@zishanj
2 years ago

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

#8 @zishanj
2 years ago

seems working perfect now. screenshot

#9 @imath
2 years 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
2 years ago

fixed undefined_index

#10 @zishanj
2 years 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
2 years 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
2 years 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
2 years 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
2 years 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
2 years 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
2 years 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
2 years 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
19 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
17 months ago

  • Milestone changed from Up Next to 7.0.0

#20 @imath
12 months ago

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