Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
rodrigok merged 2 commits into from
Dec 11, 2017
Merged

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

rodrigok merged 2 commits into from
Dec 11, 2017

Conversation

@goiaba
Copy link
Contributor

@goiaba goiaba commented Jun 19, 2017

@RocketChat/core

Closes #3887

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
Copy link
Contributor Author

goiaba commented Jul 24, 2017 •
edited
Loading

@RocketChat/core

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

@joshuamorse
Copy link

joshuamorse commented Aug 4, 2017

I would love to see this merged!

@danieljhochman
Copy link

danieljhochman commented Aug 15, 2017

+1 Would also love to see this merged.

@engelgabriel engelgabriel modified the milestone: 0.59.0 Aug 23, 2017
@rodrigok
Copy link
Member

rodrigok commented Aug 23, 2017

@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
Copy link
Contributor Author

goiaba commented Aug 23, 2017

@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
Copy link
Member

rodrigok commented Aug 23, 2017

@goiaba Exactly that :) thanks

@engelgabriel engelgabriel modified the milestones: 0.59.0, 0.60.0 Aug 25, 2017
@goiaba goiaba force-pushed the default-user-settings branch from 3d21e95 to 810d8a8 Compare November 10, 2017 16:39
@goiaba goiaba force-pushed the default-user-settings branch from 810d8a8 to ce3ff2e Compare November 23, 2017 01:07
goiaba
goiaba commented Nov 27, 2017
View reviewed changes
tests/end-to-end/ui/11-admin.js Outdated
Copy link
Contributor Author

@goiaba goiaba Nov 27, 2017

Choose a reason for hiding this comment

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

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?

Copy link
Member

@rodrigok rodrigok Dec 7, 2017

Choose a reason for hiding this comment

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

I have no idea 😄

@goiaba
Copy link
Contributor Author

goiaba commented Nov 27, 2017

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

@goiaba goiaba force-pushed the default-user-settings branch 2 times, most recently from f201ba9 to 6952ba5 Compare December 6, 2017 17:48
rodrigok
rodrigok reviewed Dec 7, 2017
View reviewed changes
packages/rocketchat-lib/server/lib/sendNotificationsOnMessage.js Outdated
Copy link
Member

@rodrigok rodrigok Dec 7, 2017

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@goiaba goiaba Dec 7, 2017

Choose a reason for hiding this comment

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

@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?

Copy link
Member

@rodrigok rodrigok Dec 7, 2017

Choose a reason for hiding this comment

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

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

rodrigok
rodrigok reviewed Dec 7, 2017
View reviewed changes
packages/rocketchat-ui-account/client/accountPreferences.js Outdated
Copy link
Member

@rodrigok rodrigok Dec 7, 2017

Choose a reason for hiding this comment

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

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

Copy link
Member

@rodrigok rodrigok Dec 7, 2017

Choose a reason for hiding this comment

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

Same for Desktop_Notifications_Default_Alert and Mobile_Notifications_Default_Alert

Copy link
Contributor Author

@goiaba goiaba Dec 7, 2017

Choose a reason for hiding this comment

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

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

@rodrigok
Copy link
Member

rodrigok commented Dec 7, 2017 •
edited
Loading

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

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

@goiaba goiaba force-pushed the default-user-settings branch from 070173a to d78aec8 Compare December 11, 2017 16:35
@goiaba
[NEW] Implements user's preferences helper function
c7dcc51 
- 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
@goiaba goiaba force-pushed the default-user-settings branch from d78aec8 to c7dcc51 Compare December 11, 2017 20:02
@rodrigok rodrigok merged commit 1bcba85 into RocketChat:develop Dec 11, 2017
@goiaba goiaba deleted the default-user-settings branch December 11, 2017 23:14
@sampaiodiego sampaiodiego mentioned this pull request May 5, 2018
Merged
@sampaiodiego sampaiodiego mentioned this pull request Jul 17, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@rodrigok rodrigok rodrigok approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

0.60.0

Development

Successfully merging this pull request may close these issues.

5 participants

@goiaba @joshuamorse @danieljhochman @rodrigok @engelgabriel