FEATURE: add more granular user option levels for email notifications (PR #7143)

This PR adds a new email_private_messages_always option for user email preferences, which defaults to TRUE.

This PR migrates email user options to a new data structure:

  • email_messages_level, with options: always, only_when_away and never (defaults to always)
  • email_level, with options: always, only_when_away and never (defaults to only_when_away)

The respective default site settings have also been migrated.

The UI now looks like this: image


You’ve signed the CLA, pmusaraj. Thank you! This pull request is ready for review.

Converting the options to a JSON string and back seems a bit cumbersome here.

My suggestion would be to use a single string value could be any of the three values. In your emailOptionsChanged callback use a switch to set the appropriate values based on the string.

I am also curious if you considered using a single value for this setting on the server side rather than adding a second option, which seems simpler in many ways?

I have been going back and forth between all three options (JSON conversion, setting a string value and updating the options server-side) and ended up with the JSON conversion because it seemed cleaner (i.e. less code).

I just noticed that we have a “checkbox + dropdown” in the UI for Activity Summary (in the same screen) so I will try using that pattern for the email preferences, and do any conversion, if necessary, in emailOptionsChanged.

Small one. Indentation is off here.

Same as https://github.com/discourse/discourse/pull/7143/files#r264484239

There is some code duplication between here and line 103. Perhaps we can extract the logic into a method?

@pmusaraj I made some changes to how we test Jobs::UserEmail so you’ll need to rebase. Sorry! Aside from the minor comments, the PR looks good to me.

Thanks for raising this, there was a bug just above line 103 too, where email_always would affect all types of posts, including private messages. Fixed and added a test.

(Will need to update tests to avoid stubs and fix merge conflict.)

@tgxworld not a problem! Will rebase shortly.

I love this feature but the data model bugs me and the UX also should be simplified a bit:

Send me an email when someone messages me 
only when I am away

Send me an email when someone quotes me, replies to my post, mentions my @username, or invites me to a topic
only when I am away

Notice how there is no checkbox before Send me an email when some messages me and the second one.

We have 2 fields on user options table that are in one of the 3 states. always, only when away, never

So we got to move away from booleans here, to simplify everything. I know the migration is a tiny bit painful but the data model will be a lot cleaner

if user_option.email_messages_level == UserOption.email_levels[:away]

# AND 

if user_option.email_level == UserOption.email_levels[:always]

Seems like a cleaner data model to me that is easier to reason about.

Yes exactly what @SamSaffron said - that was what I wanted too.

Where are the trooleans? :wink:

Where are the trooleans?

Sadly I can only find one


Oh … I said trooleans not trolleans.

1 Warning
:warning: This pull request is big! We prefer smaller PRs whenever possible, as they are easier to review. Can this be split into a few smaller PRs?

Generated by :no_entry_sign: Danger

Ok, I have updated the PR with server-side changes. It’s considerably bigger than the first PR, so will require re-reviewing (mostly server-side, client-side it’s a lot simpler).

I initially wrote reversible migrations, but I don’t think the changes here can be reversed because in the new data model, email_messages_level = :always is decoupled from email_level = always.

The title of this pull request changed from “FEATURE: add separate user email_always preference for private messages” to "FEATURE: add more granular user option levels for email notifications

I’d prefer if you could use UserOption.email_level_types as much as possible instead of using magic numbers. I see a lot of 0 and 2 in the code and have no idea what they mean unless I go looking.

This should be a const in javascript rather than 1.

Also it’s a good candidate for Ember.computed.equal. I’d suggest replacing both computed properties with that macro.

Not sure why our es6 linter didn’t flag this but we should use braces for if statements.