[NEW] Allow user's default preferences configuration #7285

goiaba · 2017-06-19T15:59:18Z

@RocketChat/core

This improvement allows to set default values to be used in user's preferences. It creates a new section named "Default User Settings" into "Accounts" section where default configurations can be set.

goiaba · 2017-07-24T17:55:25Z

@RocketChat/core

It would be nice if someone could review this PR. :-)

joshuamorse · 2017-08-04T17:39:49Z

I would love to see this merged!

danieljhochman · 2017-08-15T10:34:22Z

+1 Would also love to see this merged.

rodrigok · 2017-08-23T13:37:20Z

@goiaba This will change the values to the default ones just when the user save his profile.

Using that solution you will update the values and lost the state undefined, so if we change one setting on admin from, lets say, false to true there is no way to update the users to the new value cuz we can't know if the user has no value defined or if he set the value to false and don't want to change it to true.

The correct implementation for this is create a helper function to return the user's setting instead of accessing the user.preferences.settings..., the helper should receive the user object of id and the preference name/path to return, check the preference and return it if exists or return the setting from admin area if it does not exists.

What do you think @goiaba ?

goiaba · 2017-08-23T19:14:19Z

@rodrigok You mean we should replace every users.settings.preferences.<a-key> in the code by something like getUserPreference(userId, preferenceKey [,fallbackValue])?

The getUserPreference method would be responsible by choosing between the default value (when the user value is undefined) and the value the user set. If so, I like this approach.

rodrigok · 2017-08-23T20:24:30Z

@goiaba Exactly that :) thanks

goiaba · 2017-11-27T21:55:52Z

tests/end-to-end/ui/11-admin.js

+
+			describe('default user preferences', () => {
+				before(() => {
+					if (admin.accountsButtonCollapseDefaultUserPreferences.isVisible()) {


I'm wondering why this if is necessary here. Why are all the groups already expanded (and consequently the expand class does not exists in the html) when the test reach the Accounts section?

I have no idea 😄

goiaba · 2017-11-27T21:58:40Z

@rodrigok I did some stuff here but need help to test accordingly. Too many changes...

rodrigok · 2017-12-07T13:22:56Z

packages/rocketchat-lib/server/lib/sendNotificationsOnMessage.js

-	RocketChat.models.Users.findUsersByIds(userIds, { fields: { 'settings.preferences.audioNotifications': 1, 'settings.preferences.desktopNotifications': 1, 'settings.preferences.mobileNotifications': 1 } }).forEach((user) => {
-		userSettings[user._id] = user.settings;
+	const users = {};
+	RocketChat.models.Users.findUsersByIds(userIds, { fields: { 'settings.preferences': 1 } }).forEach((user) => {


This part is not good, we was getting only users that did change their configurations, not all users as this code does now.

@rodrigok Actually, as I didn't touch the userIds list and we are using a findByIds function, I think we are getting the same users, but now returning all their preferences. Or the fields argument generates something like a where clause?

🤦‍♂️ You're right, was too much code to read 😄

rodrigok · 2017-12-07T13:34:54Z

packages/rocketchat-ui-account/client/accountPreferences.js

 	},
 	defaultDesktopNotificationDuration() {
-		return RocketChat.settings.get('Desktop_Notifications_Duration');
+		return RocketChat.settings.get('Accounts_Default_User_Preferences_desktopNotificationDuration');


Did you change Desktop_Notifications_Duration to Accounts_Default_User_Preferences_desktopNotificationDuration right? Is there a migration for that?

Same for Desktop_Notifications_Default_Alert and Mobile_Notifications_Default_Alert

Right, I forgot the migration script. :-( I Will work on it.

rodrigok · 2017-12-07T14:03:24Z

@goiaba It looks awesome, reviewing the code I had 2 main concerns:

Migration for certain values you changed
~~The sendNotificationsOnMessage can increase the process load cuz we are returning all users to iterate over now~~

- The function returns one of three values: 1 - The user's preference (user.settings.preferences.<preference>) 2 - The default system setting value (Default_User_Preferences_<preference>) 3 - A default value passed as argument to the function - Besides the introduction of the helper function: - Removes unused preferences (user.settings.preferences.unreadRoomsMode, user.settings.preferences.audioNotificationValue) and creates migration script (v105) - Implements some simple tests

engelgabriel modified the milestone: 0.59.0 Aug 23, 2017

rodrigok added the stat: waiting response label Aug 23, 2017

engelgabriel modified the milestones: 0.59.0, 0.60.0 Aug 25, 2017

goiaba force-pushed the default-user-settings branch from 3d21e95 to 810d8a8 Compare November 10, 2017 16:39

goiaba force-pushed the default-user-settings branch from 810d8a8 to ce3ff2e Compare November 23, 2017 01:07

goiaba commented Nov 27, 2017

View reviewed changes

goiaba force-pushed the default-user-settings branch 2 times, most recently from f201ba9 to 6952ba5 Compare December 6, 2017 17:48

rodrigok reviewed Dec 7, 2017

View reviewed changes

goiaba force-pushed the default-user-settings branch from 070173a to d78aec8 Compare December 11, 2017 16:35

goiaba force-pushed the default-user-settings branch from d78aec8 to c7dcc51 Compare December 11, 2017 20:02

Merge branch 'develop' into default-user-settings

ba09d67

rodrigok approved these changes Dec 11, 2017

View reviewed changes

rodrigok removed the stat: waiting response label Dec 11, 2017

rodrigok merged commit 1bcba85 into RocketChat:develop Dec 11, 2017

goiaba deleted the default-user-settings branch December 11, 2017 23:14

sampaiodiego mentioned this pull request May 5, 2018

[NEW] Improvements to notifications logic #10686

Merged

sampaiodiego mentioned this pull request Jul 17, 2018

Users stuck online (green) since upgrade 0.65.1 -> 0.660, not being sent notifications #11378

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NEW] Allow user's default preferences configuration #7285

[NEW] Allow user's default preferences configuration #7285

goiaba commented Jun 19, 2017

goiaba commented Jul 24, 2017 •

edited

Loading

joshuamorse commented Aug 4, 2017

danieljhochman commented Aug 15, 2017

rodrigok commented Aug 23, 2017

goiaba commented Aug 23, 2017

rodrigok commented Aug 23, 2017

goiaba Nov 27, 2017

rodrigok Dec 7, 2017

goiaba commented Nov 27, 2017

rodrigok Dec 7, 2017

goiaba Dec 7, 2017

rodrigok Dec 7, 2017

rodrigok Dec 7, 2017

rodrigok Dec 7, 2017

goiaba Dec 7, 2017

rodrigok commented Dec 7, 2017 •

edited

Loading

[NEW] Allow user's default preferences configuration #7285

[NEW] Allow user's default preferences configuration #7285

Conversation

goiaba commented Jun 19, 2017

goiaba commented Jul 24, 2017 • edited Loading

joshuamorse commented Aug 4, 2017

danieljhochman commented Aug 15, 2017

rodrigok commented Aug 23, 2017

goiaba commented Aug 23, 2017

rodrigok commented Aug 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goiaba commented Nov 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodrigok commented Dec 7, 2017 • edited Loading

goiaba commented Jul 24, 2017 •

edited

Loading

rodrigok commented Dec 7, 2017 •

edited

Loading