Skip to:
Content

Opened 4 years ago

Closed 4 years ago

#1268 closed enhancement (fixed)

E-mail subscription improvements

Reported by: dimadin Owned by:
Milestone: 1.1 Priority: high
Severity: normal Version: 1.1-alpha
Component: Back-end Keywords: has-patch
Cc: tracs@…, nightgunner5@…

Description

I already expressed my opinion about this feature in forum topic and took time to actually make some of the changes I suggested.

In this patch:

  • from function bb_user_subscribe_link checking of subscription status is moved to new function bb_is_user_subscribed which is then used
  • from bb-includes/action.subscribe.php code for writing subscription status in database is moved to a new function bb_subscripton_management which is then used
  • added functions for showing of checkbox below post form
  • added hooks for bb_user_subscribe_link and bb_user_subscribe_checkbox
  • added documentation for mentioned functions, plus for bb_notify_subscribers
  • copied functions checked, selected, disabled and checked_selected_helper from WordPress for easier handling of input forms

To do:

  • add hooks in bb_notify_subscribers (this should be done in conjunction with fixing #1267 )
  • add checkboxes on Discussion Settings page for subscribe/unsubscribe link and checkox, and if functions for showing this on front-end
  • set default options for above on installation

Attachments (7)

subscribtion_improvements.patch (11.6 KB) - added by dimadin 4 years ago.
1268.diff (20.3 KB) - added by GautamGupta 4 years ago.
Better Subscriptions
1268.2.diff (26.4 KB) - added by GautamGupta 4 years ago.
subscribtion_improvements_2.patch (19.9 KB) - added by dimadin 4 years ago.
1268.3.diff (27.9 KB) - added by GautamGupta 4 years ago.
action.subscribe.php.rej (2.2 KB) - added by chrishajer 4 years ago.
bb-includes/action.subscribe.php.rej
subscribtion_improvements_3.patch (847 bytes) - added by dimadin 4 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 GautamGupta4 years ago

  • Milestone changed from 1.0.3 to 1.1
  • Version changed from 1.0.2 to 1.1-alpha

comment:2 dimadin4 years ago

I think that this should be done in 1.0.3 if subscriptions will be introduced in it. Not all features must come into core but at least functions for checking of subscription status and writing subscription status in database with filters so that we could make plugins on top of it. This patch is tested and is not that much big so it could be easily modified and committed.

Same applies to #1267 since it is obviously that it is a bug.

We will wait too long for 1.1 and this shouldn't wait tha much.

comment:3 GautamGupta4 years ago

  • Priority changed from normal to high

This is actually a 1.1 feature, so should be on 1.1 milestone only.

comment:4 dimadin4 years ago

But subscription will get in 1.0.3 when it is released?

comment:5 GautamGupta4 years ago

If 1.0.3 gets out, it will only go with bug fixes and minor enhancements.

comment:6 GautamGupta4 years ago

bb_is_user_subscribed makes use of 3 queries and the patch runs it 4 times! It should be reduced!

GautamGupta4 years ago

Better Subscriptions

comment:7 GautamGupta4 years ago

Attached another patch which is based on dimadin's patch. Here is what the patch does:

  • from function bb_user_subscribe_link checking of subscription status is moved to new function bb_is_user_subscribed which is then used
  • from bb-includes/action.subscribe.php code for writing subscription status in database is moved to a new function bb_subscription_management which is then used
  • added a checkbox below textbox to allow the user to subscribe to topic from there (within the template)
  • added hooks for subscription functions
  • added documentation for subscription functions
  • copied functions checked, selected, disabled and checked_selected_helper from WordPress for easier handling of input forms
  • Instead of 3 queries to check if the user is subscribed, the patch uses only a single query.

I have tested it and it is working, but more testing would be appreciated.

comment:8 follow-up: dimadin4 years ago

Gautam, thank you for your review and updates! Here are my thoughts about your patch (note that I didn't test it, just looked at a code).

It's great that you added action and filters, especialy most important one at bb_notify_subscribers, but I think that this one should be changed so that before bb_mail is used we check if there is $message returned after filters are applied. This is because we can choose not to send e-mail in some cases (eg. we could have a plugin which would check if user visited topic after last e-mail: if he did, send e-mail, if he didn't, don't send).

There is one thing where I don't support in your changes. I strongly dissagre with hard-coding of subscription checkbox inside of a theme and instead propose not only what I already made in my patch, but also removing hard-coded bb_user_subscribe_link and adding it via action to postmeta. This way we will have compatibility with all (or most themes) while with your and Matt's approach of hard-coding, all themes need to be edited in order that subscriptions work.

I noticed this problem after redesign of bbpress.org and posted two posts about it and we still don't have subscriptions links.

Also, I will repeat my proposal that subscriptions should be optional. We should have checkboxes on Settings > Writing (or other admin page) for both subscription links and subscriptions checkboxes so that users could easily turn on/off this feature. Only reason why I didn't make this in my patch was because I don't know how to dill with default values of this: should it be left unchecked or should we have functions to check it on installation/upgrade (btw, I didn't find some place in installation where default options are set).

If I have more time today, I'll test your patch and review it in more detail.

comment:9 in reply to: ↑ 8 ; follow-up: GautamGupta4 years ago

Replying to dimadin:

There is one thing where I don't support in your changes. I strongly dissagre with hard-coding of subscription checkbox inside of a theme and instead propose not only what I already made in my patch, but also removing hard-coded bb_user_subscribe_link and adding it via action to postmeta. This way we will have compatibility with all (or most themes) while with your and Matt's approach of hard-coding, all themes need to be edited in order that subscriptions work.

While I agree with you on this (check #1244 - 3rd point), it has more problems as theme's can't customize the text, some would like to have the checkbox at the beginning of text, some at end of text etc. Kevin suggested to have another file for the anonymous posting form (as if the same file isn't present in your theme, it is taken from Kakumei), but how many files should we make? Another file for each addition?

Also, I will repeat my proposal that subscriptions should be optional. We should have checkboxes on Settings > Writing (or other admin page) for both subscription links and subscriptions checkboxes so that users could easily turn on/off this feature. Only reason why I didn't make this in my patch was because I don't know how to deal with default values of this: should it be left unchecked or should we have functions to check it on installation/upgrade (btw, I didn't find some place in installation where default options are set).

Agree.

comment:10 in reply to: ↑ 9 ; follow-up: dimadin4 years ago

Replying to GautamGupta:

While I agree with you on this (check #1244 - 3rd point), it has more problems as theme's can't customize the text, some would like to have the checkbox at the beginning of text, some at end of text etc. Kevin suggested to have another file for the anonymous posting form (as if the same file isn't present in your theme, it is taken from Kakumei), but how many files should we make? Another file for each addition?

Anyone can easily customize any string in *Press. If theme author doesn't want default checkbox/subscribtion link, he can simply remove action and add his action, or hard-code it in a theme.

comment:11 in reply to: ↑ 10 Nightgunner54 years ago

  • Cc nightgunner5@… added

Replying to dimadin:

Anyone can easily customize any string in *Press. If theme author doesn't want default checkbox/subscribtion link, he can simply remove action and add his action, or hard-code it in a theme.

Is the average user going to want to change text in a file marked post form or would they even know how to change the string using gettext?

When WP released Widgets as a core feature, they didn't introduce a new action to inject them into old themes. Simply put, people are smart enough to be able to insert a block of text into a file when told where to put it.

I don't think that it's a good idea to add a single input to a file and consider it a template. Just put the checkbox with the other inputs in post-form.php and tell people to copy-pasta if they ask why the feature isn't available on their forum.

comment:12 GautamGupta4 years ago

I agree with Nightgunner5. I'm attaching another patch which does the following in addition to the previous points:

  • Adds an option for subscriptions (when you upload the new files, and go to admin panel you would be asked to upgrade your database - that is actually to add the new option, the value set there is true). To check if subscriptions are active or not, one can use bb_is_subscriptions_active function.
  • If a plugin filters the message in the mail and no message is returned, then no mail is sent to that user. (as per the request by dimadin)
  • Some coding standard fixes here and there.
  • Added tabindexes.

More testing would be appreciated.

GautamGupta4 years ago

comment:13 dimadin4 years ago

Ha, I was just testing changes to your previous patch that I will propose here (it seems that we are both inspired by Matt's post on wpdevel).

I again strongly disagree with hardcoding of subscriptions elements in a template files instead of using built-in hooks. Update on wordpress.org/support once again proved I'm right.

Will post later with my patch.

comment:14 dimadin4 years ago

OK, here is my new patch. It includes fixes from Gautam's latest patch, except for template. (note that I'm not sure about bb and db versions which are included)

What it does:

  • Adds option in bb-admin/options-discussion.php for turning on/off email subscription. Option will be turned on by default.
  • Adds subscription link for subscribing/unsubscribing in topicmeta action.
  • Adds checkbox for subscribing/unsubscribing in edit_form and post_form actions.
  • Adds hooks in all subscription related functions: you can change default texts, remove some actions, make plugin for cases when email is sent etc.

There is discussion here about should subscription link and checkbox be hardcoded in template or added via hook. One argument is that it will be easier for user to manually edit his template to add this function than to have it automatically enabled because he can't easily filter display of link/form?

Don't want default subscription link? Simply add this line in your functions.php and default link will not be shown so you can hardcode your own:

remove_action( 'topicmeta', 'bb_user_subscribe_link' );

Don't want default subscription checkbox? Its easy too:

remove_action( 'edit_form', 'bb_user_subscribe_checkbox' ); 
remove_action( 'post_form', 'bb_user_subscribe_checkbox' );

Want to change label text in checkbox?

function filter_checkbox_label($default_label) {
        $new_label = 'New text';
        return $new_label;
}
add_filter ( 'bb_user_subscribe_checkbox_label', 'filter_checkbox_label');

You can't tell that EVERY user should manually edit his template to enable this functionality because SOME users want different style or text. This claim is false.

If user want to change something, he already needs to edit template so it won't be hard for him to add one, two, or four lines of code do disable default behavior.

Why I know I'm right?

Look at bbpress.org/forums or wordpress.org/support. Both forums are running on trunk version of bbPress (which has email subscription for more than half a year), but both don't have subscription available to users because someone needs to edit template. And nobody is editing it because people hate to do this kind of stuff.

GautamGupta4 years ago

comment:15 GautamGupta4 years ago

Attached another patch which adds checkbox and link via actions as suggested by jjj on IRC. It is based on dimadin's patch, in addition to those points here are some more:

  • Adds tabindex to the checkbox
  • Kakumei theme removes the default checkbox actions and adds the checkbox in the left side of send post button - this way it would also be an example for people and the space in the left of the button would also be utilized.
  • Subscription email is also sent on anonymous users' post.
  • Minor fixes here and there.

comment:16 chrishajer4 years ago

I applied 1268.3.diff and got the following error:

Hunk #1 FAILED at 3.
1 out of 1 hunk FAILED -- saving rejects to file bb-includes/action.subscribe.php.rej

I will attach that file next

chrishajer4 years ago

bb-includes/action.subscribe.php.rej

comment:17 chrishajer4 years ago

  • Resolution set to fixed
  • Status changed from new to closed

(In [2471]) Multiple fixes for email subscriptions. Note the template changes in kakumei. Probably fixes #1268. Big props to dimadin and GautamGupta

comment:18 chrishajer4 years ago

(In [2472]) Forgot to add in new functions.php files for kakumei and kakumei-blue. Fixes #1268. Props GautamGupta

comment:19 dimadin4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

I added a new patch which adds new optional parameter $user_id to function bb_subscription_management. This would make easier subscriptions by other plugins etc.

Tested and wont break anything.

comment:20 chrishajer4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

(In [2517]) Add optional $user_id for subscriptions (useful for plugins). Further fixes #1268. Props dimadin

Note: See TracTickets for help on using tickets.