Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make "All Email" the default setting for group email subscription #30

Closed
boonebgorges opened this issue Nov 20, 2012 · 22 comments
Closed
Milestone

Comments

@boonebgorges
Copy link
Member

No description provided.

@boonebgorges
Copy link
Member Author

@r-a-y At the moment, Commons In A Box is not doing anything like this (modifying the behavior of a plugin, either through setting options or filter hooks). My own opinion is that it's generally a bad idea to do so. However, this particular issues is important to the people paying the bills, and there may be others in the future, so I would like to settle on a strategy for handling these kinds of mods. A couple of questions:

  1. Do we do it in the Commons In A Box plugin, or in cbox-theme? This issue doesn't really seem theme related, but on the other hand the theme is where we've been doing most of this kind of miscellaneous stuff that's not directly related to plugin installation/upgrading.
  2. Do we add Dashboard settings for these sorts of things? On the one hand, boo on Dashboard settings. But on the other hand, I'm not keen on the idea of making these sorts of potentially obtrusive decisions on behalf of site admins, and then not providing easy ways of reversing Commons In A Box's decisions.
  3. In this particular case, we could take the altertate route of patching GES itself to add this option. There's already a filter in place. We could abstract it into a saved setting, exposed in the UI at wp-admin/network/admin.php?page=ass_admin_options. Then, Commons In A Box (or cbox-theme) could be responsible merely for setting the default value of this setting on installation, and not touching it afterward. That way we wouldn't have to deal with any UI on the Commons In A Box end.

I'm thinking that the GES patch is the way to go with this ticket, though we still have to come up with an answer to question 1. If we're going to include it in Commons In A Box, maybe we introduce a new class for these sorts of setup mods, with separate methods for each mod/plugin?

Curious to get your thoughts before we start anything. Thanks, @r-a-y

@r-a-y
Copy link
Member

r-a-y commented Nov 26, 2012

I've written a little bit on this here: #35 (comment) (read the second point)

I think for straightforward hotfixes we can just add the code into a bp-custom.php-like file.

For code that needs a decision from the user, we'll need to do a dashboard settings page of some kind (yeah I know). Or we can do it like hotfixes, but add a filter to override it for each code snippet.

The filter method is not user-friendly and I think the whole point of CBOX is to make it easy to do things.

For this specific issue, we can do point 3. For the general problem, I'm afraid we might have to go the dashboard route.

Do we have any current issues that will need user intervention?

r-a-y added a commit that referenced this issue Nov 28, 2012
* Introduces 'CBox_Settings' class to register settings and output settings admin page.
* Introduces 'CBox_Frontend' class to run conditional code needed on the frontend.

Some initial, available settings include:
* BuddyPress: Setting the default member page tab to 'Profile'. Fixes #33.
* GES: Make 'All Mail' the default group subscription option (not working yet). See #30.
@boonebgorges
Copy link
Member Author

Fixed in 4199d9d and 72b8d0e

boonebgorges added a commit that referenced this issue Dec 4, 2012


Also refactors the settings API a bit so that multiple settings can be
registered for each plugin. See #30
@mkgold
Copy link
Member

mkgold commented Dec 31, 2012

If I'm reading the commits correctly ( 4199d9d ) this issue has been closed without being fixed, since the commit includes the following note: "* GES: Make 'All Mail' the default group subscription option (not working yet)."

@mkgold mkgold reopened this Dec 31, 2012
@boonebgorges
Copy link
Member Author

The issue was fixed in 2d8ed1d.

@boonebgorges
Copy link
Member Author

Oops, I mean 72b8d0e

@mkgold
Copy link
Member

mkgold commented Dec 31, 2012

Oops, I mean 72b8d0e

Is that right? I'm not seeing where that code references default GES notification settings, though it is entirely possible that I'm missing something.

@boonebgorges
Copy link
Member Author

In 4199d9d, @r-a-y introduced the filter necessary to change GES's default setting, but it didn't work yet. In 72b8d0e, I made a few fixes to the way Commons In A Box initializes so that his filter would kick in correctly.

@mkgold
Copy link
Member

mkgold commented Dec 31, 2012

Ah, cool -- thanks. So, can you explain how this will affect new and existing instances of CBOX?

@boonebgorges
Copy link
Member Author

All installations of Commons In A Box will have All Email as the default
setting of GES.

On 12/31/12 10:22, mkgold wrote:

Ah, cool -- thanks. So, can you explain how this will affect new and
existing instances of CBOX?


Reply to this email directly or view it on GitHub
#30 (comment).

@mkgold
Copy link
Member

mkgold commented Dec 31, 2012

So it overwrites existing subscription settings on existing versions of CBOX when CBOX is upgraded to the latest version? If so, does it overwrite all subscription settings or just those with "no email"? Or does it simply change the default subscription setting for groups created from the point of the upgrade moving forward and leave existing subscriptions untouched?

@boonebgorges
Copy link
Member Author

does it simply change the default subscription setting for groups
created from the point of the upgrade moving forward and leave existing
subscriptions untouched?

This. It's the default setting.

On 12/31/12 10:31, mkgold wrote:

does it simply change the default subscription setting for groups
created from the point of the upgrade moving forward and leave existing
subscriptions untouched?

@boonebgorges
Copy link
Member Author

Also, it affects all groups, even those that have already been
created, assuming that they had not yet set a subscription level.

On 12/31/12 10:31, mkgold wrote:

does it simply change the default subscription setting for groups
created from the point of the upgrade moving forward and leave existing
subscriptions untouched?

@mkgold
Copy link
Member

mkgold commented Dec 31, 2012

okay -- thanks.

@r-a-y
Copy link
Member

r-a-y commented Dec 31, 2012

All installations of Commons In A Box will have All Email as the default setting of GES.

This is partially true. The site admin has to check off the respective option in the CBOX settings menu so "All Mail" will become the default setting in GES.

@mkgold
Copy link
Member

mkgold commented Dec 31, 2012

Hi Ray -- interesting, as that doesn't sound like a default setting! What will the setting be if "All Mail" is not checked?

@r-a-y
Copy link
Member

r-a-y commented Dec 31, 2012

Hi Ray -- interesting, as that doesn't sound like a default setting! What will the setting be if "All Mail" is not checked?

GES also has an admin setting for this, but if that isn't set, then I believe it's set to "No Mail" by default.

If the CBOX admin setting is set, then that overrides GES.

@mkgold
Copy link
Member

mkgold commented Dec 31, 2012

then I believe it's set to "No Mail" by default.

Thanks, Ray. It appears, then, that we haven't really made "All Email" the default setting and thus haven't really accomplished the task described in the ticket.

Is that because the dev team thinks we shouldn't make "all email" the default? If so, I've yet to see a justification for that. Subscription settings need a default; why is "no email" to be preferred over "all email"? I've explained why I think "all email" should be the default for community sites here: http://redmine.gc.cuny.edu/issues/2270 . If you guys object, I'd love to hear why. Thanks in advance for your thoughts.

@boonebgorges
Copy link
Member Author

The discussion above makes it fairly clear that this is an oversight. No need for debate.

@r-a-y, please take whatever steps are necessary to initialize the All Email override when installing Commons In A Box.

@boonebgorges boonebgorges reopened this Dec 31, 2012
@mkgold
Copy link
Member

mkgold commented Dec 31, 2012

Okay -- thanks. Should you decide you want to discuss this in more depth at some point, please let me know.

r-a-y added a commit that referenced this issue Jan 3, 2013
Because we want our 'CBox_GES_All_Mail' class to be autoloaded (see #30),
we need to add some logic to allow this to happen.

Create new setup_globals() method - grabs existing settings from the CBOX
admin settings page and merge it with our autoloaded classes.
r-a-y added a commit that referenced this issue Jan 3, 2013
This option is now autoloaded by CBOX.  See commit e9dd0e6.
@r-a-y
Copy link
Member

r-a-y commented Jan 3, 2013

Okay, "All Mail" should now be the default option in GES without user intervention.

@r-a-y r-a-y closed this as completed Jan 3, 2013
@mkgold
Copy link
Member

mkgold commented Jan 14, 2013

Fantastic - thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants