Use of bp_core_signup_send_activation_key not checked while querying for signups

[zishanj]

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.

[imath]

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

[zishanj]

added braces

[zishanj]

added spaces

[espellcaste]

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

[imath]

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)

[zishanj]

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.

[zishanj]

Patch ​8144 works with new ​signup

[zishanj]

fixed patch 8144

[zishanj]

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.

[zishanj]

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

[zishanj]

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

[zishanj]

fixed 2 more naming issues with sent_date

[zishanj]

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

[zishanj]

seems working perfect now. ​screenshot

[imath]

@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:

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]

fixed undefined_index

[zishanj]

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.

[imath]

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.

[zishanj]

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.

[imath]

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 ?

[zishanj]

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.

[zishanj]

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.

[imath]

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.

[zishanj]

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.

[imath]

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