buddypress-group-email-subscription
buddypress-group-email-subscription copied to clipboard
boonebgorges
Proposed improvements for actual initialization of default settings for ...
Thanks for this Yann. I'm trying it out now. A huge thanks if this works as this has been bugging us for quite some time.
@vfowler, please be aware this only fixes initialization of the 'ass_replies_to_my_topic' and 'ass_replies_after_me_topic' settings for existing and new users site-wide. I's only for forum posts follow-ups default settings as defined out-of-the-box by the plugin. If there is any other default settings that you have defined for specific groups and have troubles with, this is another issue.
@yannbug - That sounds about right; thanks for the pull request. I think we did this intentionally because users might not want to be subscribed automatically to their posts. Currently, you'll have to educate your users to go to their notification settings; it's a user educaton issue.
Your code will overwrite any existing settings for current users if a site admin deactivates GES and reactivates it. It would probably be better to use a WP_User_Query and checking if those meta entries exist.
It might be interesting to use your code as a basis for an admin option, but since this code only affects the legacy forums, I'm hesitant to add support for it. @boonebgorges - what do you think?
However, what you stated in the wp.org forums sounds like the real bug:
People expect to be alerted when people reply after them, because that's the default setting they see when they go to the settings page. They don't press the "save" button because the setting is already indicated as such. And so they are never initialized, this setting is never applied to them and they won't receive any alert.
We'll need to address this.
@vfowler - This pull request only fixes issues for those using the BP legacy forums, not when using the bbPress plugin.
@r-a-y yes, it is a bug to present default settings which are not activated. What I did is activate the settings according to what is actually displayed. Which fixes it.
You're actually wrong about my changes overwriting any current user settings when deactivating/reactivating the plugin. I took care of that, you can check. Please just test it in real life, like I did. What you suggest about using a WP_user_query is wrong, it's not needed at all (please check the way add_user_meta() works!) -> Any existing setting will not be overwritten.
So I think you can safely pull this change in your plugin: it will NOT have any other side effect. It will only make it work like anyone should think it works: what is showed in the interface of each user will actually be enabled.
...and many people will stop complaining that your plugin does not work, because it will now do something out of the box for all existing users, as expected. I spent hours finding where the bug was when my client asked me to fix your plugin because some users randomly did not get the email alerts. I do not agree at all this has anything to do with user education: there is no way a normal user or community manager can figure out the displayed settings are not enabled. This is just plainly a bug, and in my opinion it has probably hurt your plugin's adoption by community managers more than you think.
If you decide to change the default setting for having users not automatically subscribed to their own posts, well, just change that and comment 2 lines in my code. We still need this fix for the other setting which is eagerly needed by many users of your plugin!
As you can see it's just a tiny few safe llnes of code to make so many of your users' life so much better ;) ( The code will not even be loaded for users that don't have the legacy forums enabled. )
I'll be happy to answer any further question about my code, but please don't resist fixing what's broken. As experienced WP plugin developers, we're on the same side :)
PS: of course it only fixes the issue for BP legacy forums, because the bug is only present for those forums... which are still widely/mostly used with BuddyPress ;)
@r-a-y: just to be clear: add_meta_data( '', '', '', $unique=true ) does not duplicate or change a usermeta value if the usermeta key is already present for that user. This is the purpose of the 4th argument to that WordPress built-in function, when set to true.
@see http://codex.wordpress.org/Function_Reference/add_user_meta @see http://codex.wordpress.org/Function_Reference/add_metadata @see http://core.trac.wordpress.org/browser/tags/3.6/wp-includes/meta.php#L0 lines 54~57
...if you have any doubt about this ;) - so there is no way existing users settings can be overwritten even when re-installing the plugin. You can just check it out for yourself anyway.
@yannbug - Thanks for pointing out my mistake with add_user_meta(). I glanced over your code quickly and missed the fourth parameter.
I do appreciate you taking a look into the plugin and for your passion into this issue. Believe me, we are on the same side here!
However, I do have reservations with your activation code because you do not run your code if there are more than 1000 users on an install because as you stated in your code, it doesn't scale well. There are many sites that have more than 1000 users.
An alternative might be to check if the logged-in user has those meta entries set, if the logged-in user doesn't, then we add it for them. Of course, the problem with this is a user has to be logged-in to the site, but if the user is an active member, then this will work fine.
A part of me still believes this is a user education issue because if you add an important plugin like GES to your site, there will have to be some form of announcement post on the site, right? This gives the site admin the opportunity to talk about GES and tell users about the notification settings, etc.
It seems to me that there is a bug here, and we have to make a decision about how to solve it in the way that'll be least disruptive.
In ass_group_subscription_notification_settings(), we default to 'yes' for 'ass_replies_to_my_topic' and 'ass_replies_after_me_topic'. But the way that these values are used (see around https://github.com/boonebgorges/buddypress-group-email-subscription/blob/master/bp-activity-subscription-functions.php#L160), it's assumed that users with no value do not have 'yes' for these two settings - in other words, it defaults to 'no'.
The decision is which way the fix should go.
a. We can just change the default value of the notification setting to 'no'. This would be minimally disruptive: users who'd previously changed their setting to 'yes' would continue to receive notifications, while users who hadn't would continue not to. So, there'd be no change from the current behavior.
b. We can do something closer to what @yannbug suggests and make it default to 'yes'. However, as @r-a-y notes, there are some problems with the migration process that'd be required if we took the tack suggested in the pull request. So here's another idea.
ass_user_settings_array() was designed so that you could do a single db lookup for the whole loop. But what if we did this differently, introducing a new function that looks like this:
function ass_get_user_setting( $user_id, $setting ) {
$value = bp_get_user_meta( $user_id, $setting, true );
if ( '' === $value ) {
$value = apply_filters( 'ass_default_user_setting', 'yes', $user_id, $setting );
}
return $value;
}
Then, instead of trying to save cycles by slurping up all user settings with ass_user_settings_array(), we do ass_get_user_setting( $user_id, 'ass_replies_to_my_topic' ) checks inside the loop. We lose a bit of efficiency (though, realistically, probably not that much - we're generally not talking about huge numbers of users in a given group). But it'd solve the problem at hand by allowing new users to have their default settings set to 'yes'.
It's worth noting that this suggestion (as well as @yannbug's suggestion) would be a potentially disruptive change. Many people who are currently not receiving emails in certain cases, would now be receiving them. This would be good for some communities, but might be bad for others. In any case, we could (and probably should) also add the proposed 'ass_default_user_setting' filter to the notification settings page, so that whether the plugin default is 'yes' or 'no', site admins can write a quick filter to switch it on or off.
You can do it whichever way you want, but my code suggestion was meant to be the quickest, most simple fix, with as much code isolation as possible to prevent any possible side-effect. I did not want to touch any of your core functions.
I believe the fix for new users is the most important one. You could probably pull that part in right away: it has no disruptive effect on existing user base, while fixing the issue for all new registered users to date.
@r-a-y: there is no way a community manager can "educate" all of its community members to disregard what the screen tells them. This is not education, this is the contrary of it. That cannot be a serious solution to this problem. Especially since most community managers are not aware of the issue. There is no way for them to guess where the problem comes from: for them it's just a random bug where some users get the alerts, and some don't. How would you expect them to be warned? One could not seriously intend on stating in the plugin documentation that contrary to what is displayed on their settings screens, users should not expect that default settings are enabled.
You're both right there could be an additional fix for sites with many users. I modestly paved the way, you can take it from here. Should it prevent you from making the present fix available to all in the meantime? It fixes the issue for all new users! That's what is mostly needed. And I do not believe it is disruptive for existing user-base because it won't override any existing registered setting, in any way. It will just make the actual plugin behavior consistent whith what the settings screens say. You could argue that fixing an old bug is disruptive but hey, that kind of disruption is called "progress" in my dictionary :)
@boonebgorges: in my experience, community managers love the fact that the "alert for replies after me" setting is enabled by default out of the box: this is a great way to encourage community fidelity, without too much spam issues. I think it would be a pity to step back on that default. I really think it would be far better to make it work as expected (and obviously as originally as designed/planned).
PS : don't bother crediting me for anything, I don't care: just fix the issue any way you want, but please fix it now that's in plain sight ;)
Thanks @yannbug. This is all reasonable, but again you are operating on the assumption that all community owners would want the "replies to my own posts" etc settings "on" by default. It's not clear that this is the case. On balance, I think that it's better to default to on in this particular case, but we definitely should make it possible to change this behavior.
On 08/21/2013 03:23 PM, yannbug wrote:
You can do it whichever way you want, but my code suggestion was meant to be the quickest, most simple fix, with as much code isolation as possible to prevent any possible side-effect. I did not want to touch any of your core functions.
I believe the fix for new users is the most important one. You could probably pull that part in right away: it has no disruptive effect on existing user base, while fixing the issue for all new registered users to date.
@r-a-y https://github.com/r-a-y: there is no way a community manager can "educate" all of its community members to disregard what the screen tells them. This is not education, this is the contrary of it. That cannot be a serious solution to this problem. Especially since most community managers are not aware of the issue. There is no way for them to guess where the problem comes from: for them it's just a random bug where some users get the alerts, and some don't. How would you expect them to be warned? One could not seriously intend on stating in the plugin documentation that contrary to what is displayed on their settings screens, users should not expect that default settings are enabled.
You're both right there could be an additional fix for sites with many users. I modestly paved the way, you can take it from here. Should it prevent you from making the present fix available to all in the meantime? It fixes the issue for all new users! That's what is mostly needed. And I do not believe it is disruptive for existing user-base because it won't override any existing registered setting, in any way. It will just make the actual plugin behavior consistent whith what the settings screens say. You could argue that fixing an old bug is disruptive but hey, that kind of disruption is called "progress" in my dictionary :)
@boonebgorges https://github.com/boonebgorges: in my experience, community managers love the fact that the "alert for replies after me" setting is enabled by default out of the box: this is a great way to encourage community fidelity, without too much spam issues. I think it would be a pity to step back on that default. I really think it would be far better to make it work as expected (and obviously as originally as designed/planned).
PS : don't bother crediting me for anything, I don't care: just fix the issue any way you want, but please fix it now that's in plain sight ;)
— Reply to this email directly or view it on GitHub https://github.com/boonebgorges/buddypress-group-email-subscription/pull/48#issuecomment-23042943.
@boonebgorges: when fixing the issue for my client, I did not operate on any assumption other than make it work like it was designed: it's not me that chosed to have this setting "on" by default. It's what your code very clearly states, that this was intended to be on by default. But when presenting my client with the choice between setting it "off" by default, or making it work as designed by the plugins' authors, they wanted it to stay on. Right now it is broken for everyone because the screen says it is on, while in reality it is not. So again I'm not assuming anything else than making the plugin behavior consistent with what the settings screen says. That's just fixing things. My mission was just to remove the bug, not build a new feature. (And oh! they specifically asked me to fix the code, not the users! ;) )
Now you can decide to implement a new feature instead, or change the design and requirements, thats completely your right as the plugins' author, but that's another issue altogether. In my opinion, fixing things by making them work the way they were intended in the first place is not disruptive. It's just a fix.
Introducing a new feature, on the other hand, might be disruptive. The important point in my opinion, is just not to delay the bug fix by wanting to add features that were not requested by people that reported the bug in the first place. If you can make it consistant and introduce an improved feature at the same time, that's fine with me, as long as my client does not have to suffer a regression on this bug in the next plugin release. There's no hurry as far as I'm concerned becaused this fix is now publicly available. But please just make sure that the next plugin release does not make this bug re-appear, wether you decide to introduce a variation on my proposed code or any other way. Ohterwise there is a risk that the project might fork, and we all want to avoid that at any cost.
Thanks for considering my proposed improvements anyway. And keep on the good work on this useful plugin.
The bug is that there is a mismatch between the default noted on the notification settings screen and the default assumed in the code itself. Fixing that mismatch means choosing to make it 'yes' throughout or 'no' throughout. I agree that 'yes' is probably the best choice.
