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

[IMPROVE] Change default user's preference for notifications to 'All messages' #15420

Merged
merged 4 commits into from
Sep 21, 2019

Conversation

sampaiodiego
Copy link
Member

@sampaiodiego sampaiodiego commented Sep 20, 2019

More information on this blog post

@sampaiodiego sampaiodiego added this to the 2.1.0 milestone Sep 20, 2019
if (mobileNotifications && mobileNotifications.value === 'mentions') {
Settings.update({ _id: 'Accounts_Default_User_Preferences_mobileNotifications' }, { value: 'all' });
}
},
Copy link
Contributor

@geekgonecrazy geekgonecrazy Sep 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is still going to be such a shocker for existing deployments. :( Even with the blog post

Copy link
Member Author

@sampaiodiego sampaiodiego Sep 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hopefully they'll be shocked in a good way 😬

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same 😬

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use the new helper to alert admins about this change (if the setting was updated to all)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get a good enough looking banner to show to admins :/

geekgonecrazy
geekgonecrazy previously approved these changes Sep 20, 2019
@@ -243,7 +243,7 @@ settings.addGroup('Accounts', function() {
],
public: true,
});
this.add('Accounts_Default_User_Preferences_desktopNotifications', 'mentions', {
this.add('Accounts_Default_User_Preferences_desktopNotifications', 'all', {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we really do this for desktop too? TBH the mobile one didn't annoy me when we first tried this change out. But the desktop one very quickly made me trigger busy mode to silence them

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMHO if we change one we should change both

@sampaiodiego sampaiodiego changed the title [IMPROVE] Change default value for notifications to All [IMPROVE] Change default value for notifications to 'All messages' Sep 21, 2019
@sampaiodiego sampaiodiego changed the title [IMPROVE] Change default value for notifications to 'All messages' [IMPROVE] Change default user's preference for notifications to 'All messages' Sep 21, 2019
@sampaiodiego sampaiodiego merged commit 97856c3 into develop Sep 21, 2019
@sampaiodiego sampaiodiego deleted the change-notification-pref branch September 21, 2019 03:02
@sampaiodiego sampaiodiego mentioned this pull request Sep 27, 2019
@antgel
Copy link
Contributor

antgel commented Sep 28, 2019

Just for reference, will this affect existing deployments upon upgrade i.e. change users' preferences?

@rodrigok
Copy link
Member

@antgel you can find more information on this blog post.

It will change the server setting for notifications and not the user's preferences, but all users that are using the default value that inherits the server's value will have the behavior changed for the existent installations.

@antgel
Copy link
Contributor

antgel commented Sep 28, 2019

@rodrigok Thanks for the answer. I'm completely mystified by this, as part of how we sell RC internally is the low noise compared to email. I don't understand why people would want to get notifications for everything. What is the point of mentions in that case? (Note, I'm reasonably assuming that most users aren't experts and don't tweak their preferences.)

@reetp
Copy link

reetp commented Sep 29, 2019

When updating to version 2.1.0 or later the servers that were still using the previous default value Mentions, will have the value updated to All Messages.

Just seen this (because we don't all read every github issue)

This is completely and utterly insane.

Admin set defaults should never be changed like this unless it is actually a bug.

Now going to have as many mystified admins running round wondering what the hell has happened as their users phones suddenly explode with notification messages, and probably overwhelm the notification gateways as well.

And I thought Rocket.Chat was looking at limiting the number of notifications, not increasing them?

@sampaiodiego
Copy link
Member Author

@antgel mostly because people coming from other platforms are used to get notifications for everything. Since we're changing the default value at server level, you can change it back to the value you think is better without asking to your users.

@reetp this is in fact considered a bug to many users that expect to receive a notification when a message is sent to a group (channel/private channel) they participate. that's why we are changing the default value.

also this is why we're being very verbose about this change, because we want to let everybody know about it before hand, so when they upgrade and they don't even want to try the new value, they can change it back right away.

we'll not limit notifications during this transition period, it is stated on the blog post as well:

We will also be re-evaluating the push notification limits on our pricing plans, taking into account the increased usage.

@antgel
Copy link
Contributor

antgel commented Sep 30, 2019

@sampaiodiego I write in good faith, I don't intend to flame. :)

@antgel mostly because people coming from other platforms are used to get notifications for everything. Since we're changing the default value at server level, you can change it back to the value you think is better without asking to your users.

Okay, but why not just leave it for existing installs? It's a significant change in behaviour. If admins were hearing so much about this "bug", they would either 1) change the server default or 2) educate their users.

@reetp this is in fact considered a bug to many users that expect to receive a notification when a message is sent to a group (channel/private channel) they participate. that's why we are changing the default value.

Then these users need to understand more about ChatOps and noise. Again, what is the point of @-mentions if every "Thanks" or "Great" is going to disturb everyone? I respectfully disagree that it's "considered a bug" if it's by design. If users want noise, let them use email / WhatsApp, or configure RC to be noisy. If something was especially relevant for them, they should have been @-tagged.

also this is why we're being very verbose about this change, because we want to let everybody know about it before hand, so when they upgrade and they don't even want to try the new value, they can change it back right away.

Well, to be honest, I saw this by chance because I follow the releases in GitHub.

@sampaiodiego
Copy link
Member Author

@antgel I'm sure you write in good faith, as so do I :)

based on the feedback we received we really think this will benefit more users than harm. Even internally we had this concern of being hammered by a huge amount of notifications.. Turned out even some team members are feeling notifications more reliable.

we decided to change the default preference even for existing servers because this way we know this change will reach more people.. and we're being very verbose about it (with the big notice on release notes, the blog post and even on the next AMA we'll talk about this) so admins will know what to do if their users start complaining..

just to make it clear, we're not removing anything you already have, we're just changing the server's default configuration.. you can change it back right after you upgrade your rocket.chat, and you'll have the exactly same behavior as before. this is the first time we are doing this and we're doing it because we think it will benefit most of the users.

@antgel
Copy link
Contributor

antgel commented Oct 26, 2019

@sampaiodiego Um, where is this in the configuration? Have upgraded to 2.1.2 and can't find it.

@antgel
Copy link
Contributor

antgel commented Oct 26, 2019

@sampaiodiego Found it. For anyone else wondering:
image

@flam22
Copy link

flam22 commented Nov 7, 2019

Strangely, I am missing these two settings in the 'Default User Preferences'. All I see are 'Audio Notifications Default Alert' immediately followed by the 'Unread Tray Icon Alert' boolean setting. Currently on version 2.2.0.

Mongo listing:

rs0:PRIMARY> db.rocketchat_settings.find({'_id': 'Accounts_Default_User_Preferences_desktopNotifications'}).pretty()
{
	"_id" : "Accounts_Default_User_Preferences_desktopNotifications",
	"value" : "all",
	"_updatedAt" : ISODate("2019-11-06T15:23:35.508Z"),
}

Later... A restart seems to have fixed it!

@sampaiodiego
Copy link
Member Author

@flam22 yes, just found this bug as well.. restarting fixes it, but I'll create a proper fix.. thanks for letting us know

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

Successfully merging this pull request may close these issues.

6 participants