CUNY Academic Commons

Thanks, Ray, but I don't think it does. What I'm noting is that I'm suddenly seeing a rash of individuals who seem to have the "No Email" BP email setting selected as a default. Maybe it's always been that way, but it seems like a new change to me (and an unwelcome one). Unfortunately, the areas you've pointed to don't seem to relate to the sitewide default subscription settings for new members. . . .

Hi Matt,

I just tried reproducing this on cdev, but couldn't replicate this.

I created a new group, then I logged in as a regular member and joined the group. The regular member's group subscription level was set to "All Mail".

One difference I noticed while testing on cdev is you can't select the Group Email Subscription Defaults during the group creation process or in the group admin settings area. On a fresh BP install, you are able to do this.

Not sure if this is intentional or not based on any older Redmine issues. Though, I'm guessing it is. Boone, can you elaborate on this?

I created a new group, then I logged in as a regular member and joined the group. The regular member's group subscription level was set to "All Mail".

Same here. See #353

I've dug through the database to see if anything strange is happening. It appears that, in some cases, users in the group are not being given any email subscription status at all, which translates to No Email.

I can't reproduce this in any of my tests, so I'm not quite sure what to say about the root issue. It's hard to debug something that's not happening (users being given a default subscription level). Ray, maybe you have some insight, as you know this plugin fairly well. It looks like the subscription level should be set at 'groups_member_after_save', and I don't see how this step could be saved, no matter how you are joined to a group (which is the reason why this hook is used in the first place).

I don't have a sense of how widespread the problem is. In the couple groups I looked at (which were recent groups created by Matt, since those are the groups that were reported as problematic), there were one or two members each who didn't have a subscription level set. Ray or I could write a script that will crawl over the database to see how many group members overall do not have subscription levels set. Such a script could also correct the problem in those specific cases, by setting a default subscription level for problematic members. But this will take time, which neither of us really have at the moment. Dom is also working on some other time-sensitive issues.

Matt, can you give a sense of how you want this to fit into priority lists? I would rather not have anyone drop what they're doing at the moment, but if it's high priority for you, then we'll figure something else. Thanks.

One note on this:

In the couple groups I looked at (which were recent groups created by Matt, since those are the groups that were reported as problematic), there were one or two members each who didn't have a subscription level set.

The two groups in which I noticed this issue are here:

http://commons.gc.cuny.edu/groups/gc-digital-fellows/

http://commons.gc.cuny.edu/groups/dhdebates-towards-a-networked-academy/

In both cases, I noticed that most people in the group had "No Email" chosen as a setting, so I explicitly asked them to alter their settings. So, the fact that they have an email setting now is not necessarily indicative of the fact that they had one when they joined.

As far as priorities, I need to see this in context of other dev priorities. I think it's relatively important, because it impacts group communications significantly. But everything is important, so it would be easier for me to prioritize in the context of other dev tasks. If I had to assign this a target version, I'd hope for 1.4.6.

Thanks, Boone. Users joined the group by requesting membership on the group home page and having membership approved by the group admin. Both groups were and are set up as private groups. Please let me know whether I can provide any additional detail.

Gotta do some homework before class now, but I will start on trying to reproduce this issue when I get home.

Thanks, Dom. This issue is important.

Dom: If you're busy with coursework, I can take a look at this.

If you've got some time go ahead and take a look. I won't be able to until at least ~11pm tonight anyways. Plus, there's some parts of this issue I haven't had to deal with yet. Specifically how a group communicates with the DB to determine the specific setting we're talking about in this issue. Also, I don't know which plugin Boone in referencing in comment 4. So the beginning of my testing and attempts to recreate the problem are going to be spent just finding out where all this info is.

If you wanna start and then update me here to give me head start that works for me as well. The faster we can get to a solution is what really matters.

Thanks so much to you both for prioritizing this.

Hi Ray -- it's definitely happening with new members to the site who join groups. If you guys would like, I can give you specific accounts/groups where this has happened. It's definitely a default setting and not something that is happening via user action/choice.

Yeah sure, pass the group links. Let me know if the group is also available on cdev, so we can try and replicate this there.

It's possible that this is only an issue for new accounts (though I don't have any good guesses why this would be the case). So it might be worth testing whether the problem cam be reproduced for a newly created WP user.

Yes, some of the relevant cases involved new users and new groups

Boone, Ray,

I've re-read all comments in this issue thrice now to make sure I haven't missed anything. I've also been pouring over the code in the Group Email Subscription plugin. From what I can tell, the file bp-activity-subscription-functions.php is the only place that a default subscription issue could be occurring. All the other files are responsible for loading and initializing the plugin correctly, or supporting it in some other unrelated way.

I've also tried to recreate this issue in my local env by following various steps outlined in this thread. Matt, I'm sorry but I just cannot reproduce. I've created a group, created a new user, joined as that new user and then verified that the notification settings are set to All Email for both myself (super admin) and my unprivileged user. I've followed the advice from Ray and setup my group's settings to allow modification of individual member settings and everything matches his screenshots. The one place I am not 100% sure of proper configuration is the DB. I'm hoping Boone and Ray can point me in a better direction here.

I've learned that the default subscription level for the plugin is stored in the wp_bp_groups_groupmeta table. That was easy enough to figure out from the plugin's code. I use Sequel when poking around in DB's [ yes Boone, I switched back to a Mac :-) ] and when I search through the wp_bp_groups_groupmeta table for the ID 284 (ID of my test group) I see various entries. Then when I search for a meta_key of "ass_default_subscription" I see various other entries for groups in my local dev DB. What I don't see is an entry for "ass_default_subscription" with a group_id of 284. Very suspect if you ask me. After my next paragraph I will be asking a series of questions about specific sections of the plugin's code which may or may not shed some light on this missing DB value.

What I'm not sure of is where to verify the subscription settings for individual users in the DB. I've looked through wp_usermeta but I'm just not seeing the kinds of meta_value entires that would correspond to individual group settings for this specific plugin. I'm sure I'm just not searching correctly or for the right terms. Could one of you please point me to the corner of the DB in which these settings are hiding?

Before continuing I'd just like to point out that I still struggle with the Plugin API. What I mean is, actions and filters still confuse the hell outa me! As you can imagine, combing through a BP plugin can cause the still uninitiated to suffer from code blindness. So please forgive me if I ask a question that doesn't make any sense because of the way that function is attached to a particular action or filter. What I need to do is write up a post about the Plugin API the same way I did for WP initialization. I will get to that as soon as I can. OK, now for the questions (all the following functions are in bp-activity-subscription-functions.php):

1.) Line 722 - ass_set_default_subscription()
It appears to me that this function will return; if the member passed as an argument has yet to be confirmed after requesting to join a private group - thereby short-circuiting the creation of a default subscription status. Could this be contributing to the missing subscription status you mentioned in comment 4 Boone? In comment 8 Matt clearly states that the groups in question are private. I'm just sayin'.

2.) Line 743 - ass_join_group_message()
If the function above runs first and is short-circuited, and this function is executed later, wouldn't the value of $status be assigned 'no' on line 752? I realize that this function is not touching the DB, and therefore probably isn't responsible for the issue, but I bring it up because more than a few times in this thread All Email has been mentioned as the preferred default. Shouldn't line 752 read: $status = 'supersub';?

3.) Line 1132 - ass_change_all_email_sub()
This function outputs the message at the bottom of Ray's second screenshot. Again this may not be directly causing the intermittent No Email issue but it does fly right in the face of the previously established affinity with All Email being the preferred default. If no default subscription level is returned from the DB on line 1140, then $default_email_sub will be assigned 'no'. This variable is used in the link which clearly indicates to the admin that upon clicking it all users will receive the default of No Email.

4.) Line 1148 - ass_manage_all_members_email_update()
The only reason I am mentioning this function is because it appears to be responsible for applying the changes initiated by the link that is being rendered in the function above. Actually, now that i've typed it, I realize that this really isn't a question. Sorry about that.

I hope these ramblings haven't confused more than they've clarified. All in all it seems to me that this plugin may be suffering from BP_Schizophrenia. There are places in the code that clearly lean towards a default of No Email while prevailing thought here and in #353 would indicate otherwise.

I'm going to continue testing to see if I can cause a problem when the group is private, but I wanted to get an update out now - before it got too late.

Boone,

I missed one question:

5.) Line 637 of bp-custom.php - cac_core_screen_notification_settings()
Could this function be interfering with the DB and somehow overriding the desired default subscription setting? I'm leaning toward no, but I wanted to point it out because it calls ass_group_subscription() which is intimately linked with storing subscription values in the DB.

Dom,

Thanks for all the digging you've done so far.

In short, you're correct that the plugin does default to 'No Email'. That's because, generally speaking, defaulting to 'All Email' makes for a spammy plugin. However, for the purposes of the Commons, the default setting is 'All Email' (aka 'supersub'). This is accomplished by means of the following function, found around 956 in bp-custom.php:

function cac_default_group_email_setting( $setting ) {
    return 'supersub';
}
add_filter( 'ass_default_subscription_level', 'cac_default_group_email_setting' );

The idea is this: Every time a default subscription level for a group is pulled from the db in the plugin, it should then be fed through the filter 'ass_default_subscription_level'; our bp-custom.php function then filters this value and changes it to 'supersub'.

It's possible that there is a place in the plugin where the default value is not, in fact, being put through the filter in question, which would explain this behavior. However, I'm unable to determine any such place in the codebase.

As for your specific questions:

1) This function runs every time a group_member object is saved (thus the 'group_member_after_save' hook). BP_Groups_Member objects are saved at lots of different occasions. A few off the top of my head:
a) a user joins a public group
b) a user requests membership to a private group
c) a user's membership request is granted
d) a user is invited to a group
e) a user accepts a group invitation
f) a user leaves a group
The is_confirmed check ensures that users are not subscribed to the group in cases like (b), (d), and (f). So this shouldn't be causing the problem.

2) See comment above about how the default_subscription value is filtered. In any case, ass_join_group_message() only displays a message to new users - it does not set the default itself. (As a side note, it's architecturally ugly that we fetch the default value here - we should be getting the subscription status from the function where the status is actually set, rather than recalculating it. But that shouldn't have anything to do with the problem at hand.)

3) and 4) are both only run from the group admin screens, which is not the workflow described by Matt.

5) This function is the screen function for one of the Settings tabs, and thus is only fired when viewing /members/foo/settings/notifications. See cac_core_add_settings_nav(). So it should be irrelevant to the current issue.

Looking through this plugin, there is a lot about it that is pretty ugly, and in particular there are a lot of places where the ugliness (inconsistency, unnecessary reduplication, etc) is probably causing various bugs. In the medium term, we should think about doing some major cleanup work, to make the plugin code a bit more robust. (I want to be careful about doing too much refactoring-for-refactoring's sake, because this plugin is used by many, many people and it mostly works, but there is some low-hanging fruit).

Anyway, for the purposes of the current ticket, I'm a little bit at a loss. I've done another round of testing, and I can't reproduce the issue. And I've combed over the plugin itself, to see if I can spot an obvious hole in its logic, but I can't find anything there either. The nuclear option is to add a function that steps in after the entire group-joining process is over, and forcing the subscription status to 'supersub'. This is what ass_set_default_subscription() is already supposed to do, but it is theoretically possible to step into the group join process even later and override it.

Ray, Dom, any more ideas for how to debug, before I deploy this nuclear option and cross my fingers that it helps?

Hi All -- since I don't want to make this ticket private, I'll follow up via email with some specific cases in which I've seen this happen.

Discovered another weird instance of this, this time of a different nature because it didn't involve new members. In the MALS Administration group, which has four members, I discovered yesterday that three of the members's subscription settings were "No Email" and "New Topics," whereas I'm pretty sure that all had been "All Email" -- though, of course, I could be wrong about that, since it's possible that this is user error and that the users originally signed up with those settings. I know that the one person who had "No Email" was surprised to find that setting on her account.

Boone, Ray,

What do think about injecting a little temporary code into this plugin for the 1.4.6 release that will log group subscriptions to a file? We could output any relevant information to this issue and see if we can catch the error when it happens. At the very least we should be able to verify the email settings assigned when a user joined the group and compare them to the user's global settings in the DB.

In the issues that I monitored for RBE, the logs played an instrumental role in tracking down very pesky bugs. Since we cannot "see" this issue happening maybe a log would help record the problem so we could trace it back to the source? The logs would also allow us to "test on production," changing our own settings and joining different groups to see how it affects our email subscription settings.

Boone and Ray can respond on technical matters, but I'll just say that this bug worries me a great deal and I'd love us to try to figure it out. Logs sound like a good first step to me.

Dom - Sure, it'd be fine with me if you wanted to rig up a log. Things you'll probably want to log:
- group id
- user id
- email subscription status
- time
- logged in user (there are certain instances where the logged in user is not the user joining the group, like when membership requests are being accepted by an admin)
- the current URL (use wp_guess_url())
- the value of the constant DOING_AJAX

Ray may have more items that could be logged. But this should give a pretty clear picture of what's happening.

Thanks Boone.

I'll be spending most of my time today on this and #2146 & #2142 because they are the highest priority 1.4.6 issues and my other 1.4.6 issues are waiting for reporter feedback and will probably get punted.

As this is my first attempt at WP logging, I will have something later today for you and Ray to review so we can get some feedback on this issue into tomorrow's release. That way we have a better chance of resolving it in 1.4.7 or 1.4.8.

• group status (public, private, hidden) - AFAIK, this issue has only happened to private and hidden groups, so it'd be nice to verify that.

Dom: If you want to know how RBE does logging, check out the bp_rbe_log() function. It uses error_log(), which saves you a ton of time needed to setup the log, etc. Make sure that if you copy and paste the function, to change the constant BP_RBE_DEBUG_LOG_PATH to a different constant and filepath!

• groups_member_after_save with a priority of 10
• groups_member_after_save with a priority of 30

This is so we can see what the subscription level looks like before GES fires and after it fires.

Ray,

Thanks for the info. I did start by looking at RBE logging and once I saw how simply it was implemented (by leaning on PHP's logger) I decided that would be perfect in GES as well. I took the debug log path constant and pasted it at the top of bp-activity-subscription-main.php so I could be sure that it would exist before the plugin's functions file was required (and of course I did rename the debug log file :-) ). Then I copied your bp_rbe_log() function and pasted it at the bottom of bp-activity-subscription-functions.php, so it could be called whenever, wherever, and repeatedly if needed.

But now I'm at a loss. There are two things I don't understand: when and where do I hook the logging function into the groups_member_after_save action and once I do that, how do I build the message that will be passed to the logging function? Argument passing between actions and filters has always confused the hell out of me. My main problem is that I am very "visual" when I am coding/debugging - I am always dumping objects to the screen so I can inspect them to "see" what I'm working with. I never know when to use '$object->value' or '$object[$value]' until I var_dump() it right before I'm going to access it.

I have a feeling that I need to be working towards a solution similar to the following:

On line 823 of bp-groups-classes.php, do_action_ref_array( 'groups_member_after_save', array( &$this ) ); is executed - which is inside the save() method of the BP_Groups_Member class. I think this means that when I hook into this action my function will receive a BP_Groups_Member object. Is that correct? If so then the GES version of the logging function can just be slightly rewritten to parse the info we need from that object and dump it to the debug log (as you suggest) right before and right after (priorities 10 and 30).

If it sounds like I'm on the right track please let me know, any help or advice would be greatly appreciated.

I think this means that when I hook into this action my function will receive a BP_Groups_Member object. Is that correct?

Correct. So:

function cac_ges_log( $member ) {
    var_dump( $member ); // this will show the BP_Group_Member object
}
add_action( 'groups_member_after_save', 'cac_ges_log', 10 );

You may need some information beyond what's stored in the member object as well. I suggest the following:

$loggedin_user_id = get_current_user_id();
$current_url = wp_guess_url();
$group = groups_get_group( 'group_id=' . $member->group_id ); // pull up the BP_Groups_Group object
$group_status = $group->status; // 'private', 'public', or 'hidden'

Yup, you're on the right track, Dom!

I would write the hooks like this:

// log stuff before GES fires
function cac_before_ges_logger( $member ) {
    $info = array();

    // pull up the BP_Groups_Group object
    $group = groups_get_group( 'group_id=' . $member->group_id );

    $info['loggedin_user_id'] = get_current_user_id();
    $info['current_url']      = wp_guess_url();
    $info['group_status']     = $group->status;

    // if you want to add more variables, add them to the $info array

    // now log the item using your copy of bp_rbe_log() or whatever you renamed it to! :)
    bp_ges_log( '-- before GES: ' . print_r( $info, true ) );
}
add_action( 'groups_member_after_save', 'cac_before_ges_logger', 10 );

// log stuff after GES fires
function cac_after_ges_logger( $member ) {
    $info = array();

    // pull up the BP_Groups_Group object
    $group = groups_get_group( 'group_id=' . $member->group_id );

    $info['loggedin_user_id'] = get_current_user_id();
    $info['current_url']      = wp_guess_url();
    $info['group_status']     = $group->status;

    // if you want to add more variables, add them to the $info array

    // now log the item using your copy of bp_rbe_log() or whatever you renamed it to! :)
    bp_ges_log( '-- after GES: ' . print_r( $info, true ) );
}
add_action( 'groups_member_after_save', 'cac_after_ges_logger', 30 );

The code above isn't complete yet because it doesn't add in the subscription levels and whatnot, but it should be a good jumping-off point!

This looks really great, Dom. Thanks.

Nicely formatted, Dom! :)

I think since we have the timestamps in the log, we don't really need to differentiate between 'before' and 'after'.
To be a little clearer, if $gsub_status is empty, you could set the string to "No subscription yet."

Hopefully, this will give us some clues!

By the way, thanks for reminding me that 1.4.6 goes out tomorrow! Got to attend to some of my own tickets :)

Thanks, Dom!

I'm glad everyone's happy with it. :-)

I hope this gives some insight into this frustrating issue!

Should I go ahead and add in Ray's suggestion:

if $gsub_status is empty, you could set the string to "No subscription yet."

I went ahead and added the default email subscription text suggested by Ray:

https://github.com/castiron/cac/commit/0cb85118df5867cda0f1a87d4a785ea70971171b

Boone,

Are we punting this or closing it? I know you like to close out the issues that have code going into a release to keep the Redmine history somewhat intelligible, but I don't think the code added for this issue counts as resolvable code. It may be more important to keep this issue going in 1.4.7, and maybe even 1.4.8, until there is actually a real fix or resolution?

Done.

Just witnessed another instance of this. New Commons member Mike F - http://commons.gc.cuny.edu/members/mikefabricant/

joined the ARC seminar group - http://commons.gc.cuny.edu/groups/arc-inequalities-seminar/

Email setting: no email

I don't know for a fact that he didn't choose that himself, but I'm willing to bet that he just hit "join group"

Dom - FYI, I made a few changes to the debug logger at deployment time. See https://github.com/castiron/cac/commit/18d85cdff2473aae164ed1bd367afd2cbb3c985c. I moved the log into a directory that is writeable by apache, and I also recorded the referer URL (so that we'd have a meaningful URL in the case of AJAX requests).

Otherwise, looks like it's looking great. Matt, please let us know when another instance arises, so we can check the logs.

• All of the failed subscriptions so far have come from group -- "The Group for Group Admins" -- which is a public group.
• All other groups were successful in subscribing the user to "All Mail"
• Several instances of the subscription not saving properly came from a user's group invites page.
• AJAX is on for every single occurrence (successful subscriptions and failed subscriptions).
• The current URL is not accurate, which might be related to the AJAX request.

Some tiny clues so far, but every bit helps!

Okay -- I've got a great use case for you: the History APO just set up a new group for the History Ph.D. program. She wants to use it as a listserv instead of a typical email listserv. Almost all new members, who were invited in using Invite Anyone, look like they entered the group with a "No Email" setting by default: http://commons.gc.cuny.edu/groups/history-program/members/

This is starting to look like an urgent issue to me.

I've compiled the logs to date into a .csv file (20121004-ges.csv, attached). I don't have time to investigate at the moment, but maybe Dom could have a look for any patterns.

I think I tracked down the problem. Ray and Dom, I'm going to explain it here, as it's a bit of a doozy, and I want to get a sanity check. Reference commit: https://github.com/castiron/cac/commit/9b9a4a64927c1051155060a333fb654a35f98326

After staring at the log data for a while, I realized that a bunch of the group name column read Group For Group Admins. That didn't seem right, especially because the current/referring URLs for some of those events were different groups (like the history-program group that Matt mentions above). So I looked at the plugin that populates GFGA, in particular the function that is hooked to the groups_member_after_save hook. After I messed with it a little, I saw that the check near the end of the function (38-41 of https://github.com/castiron/cac/blob/098d6da4d051fc77166b11a932117c618bfc9a2e/wp-content/plugins/cac-group-admin-auto-add/bp-group-adminmod.php#L5) was using the variable name $member to pull up potential GFGA group memberships. The problem is that $member is also the name of the function variable containing the object passed from the groups_member_after_save hook in BP_Groups_Member::save(). Since PHP 5.0 (I think), objects passed to functions in this way (as in do_action_ref_array()) are passed by reference. Thus, when $member@ is repopulated in this function, the change is reflected in the upstream object. In turn, that new object (with the incorrect group_id property) is passed to the other functions hooked to 'groups_member_after_save'. That includes the GES function ass_set_default_subscription(). At the end of this latter function, as a result, the user gets subscribed to the GFGA instead of the correct group. (They probably never receive notifications, because they're not actually members of GFGA, even though they have a subscription setting there.)

So I believe that changing the variable name as in https://github.com/castiron/cac/commit/9b9a4a64927c1051155060a333fb654a35f98326 will fix the problem. I'm having a hard time reproducing the error reliably, so I can't know for sure - I think it will have to be tested in the live environment.

A doozy, indeed.

Inspector Boone and the Case of the Masquerading Variable!

That sounds like the culprit because the Group Admin Auto-add snippet runs at "groups_group_before_save", which is before the GES hook at "groups_group_after_save" and both reference the same object, which gets modified first by the Auto-add snippet and referenced again in later hooks.

This might also solve a bunch of other unknown issues we previously didn't know about!

I agree with Ray...

A doozy, indeed.

Pass by reference, while not impossible to understand, is still very hazy for me. I understand the basic principle, but it's incredibly powerful. And, as evidenced by this issue, very dangerous if not taken into proper consideration.

If this in fact is causing the root issue here, I'm wondering how it's affecting email sub status specifically. Is ass_set_default_subscription() in effect picking up the sub status from GFGA? Or maybe it's missing a sub status all together because the info it's expecting does not exist where it's expected to?

Also, do you think this means that the prod db is full of "garbage" group subscriptions now?

Thanks for the feedback, guys. I've applied my change as a hotfix on the live site. I'll check the logs in a few days.

Also, do you think this means that the prod db is full of "garbage" group subscriptions now?

It means that lots of people are incorrectly subscribed to the GFGA (though they probably never receive notifications, because they're still not members of the group). More importantly, it means that lots of people do not have subscription levels set for certain groups. I'm going to leave the ticket open so that a fix can be worked out - probably a script that walks through group memberships from the last few months, identifies those with no email status, and forces them to supersub.

Huge thanks to all of you for your time and careful attention to this ticket. And Boone -- big thanks and congrats to you on discovering what is hopefully the underlying issue.

Huge -- many thanks, Boone.

Can/should we open a separate ticket up for what you noted above?

I'm going to leave the ticket open so that a fix can be worked out - probably a script that walks through group memberships from the last few months, identifies those with no email status, and forces them to supersub.

#2208