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.

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

[NEW] Add admin and user setting for notifications #4339

Closed
wants to merge 22 commits into from
Closed

[NEW] Add admin and user setting for notifications #4339

wants to merge 22 commits into from

Conversation

alexbrazier
Copy link
Contributor

@alexbrazier alexbrazier commented Sep 17, 2016 •
edited by engelgabriel
Loading

@RocketChat/core

Closes #3862
Closes #5867

Add admin and user setting to control the default room preference for when to receive notifications.

image

image

image

@alexbrazier alexbrazier changed the title Feature/default notifications Add admin & user setting for notifications Sep 17, 2016
@alexbrazier alexbrazier changed the title Add admin & user setting for notifications Add admin and user setting for notifications Sep 17, 2016
@rodrigok
Copy link
Member

@alexbrazier Your ideia is great, but we need to evaluate this carefully cuz these changes can add a huge impact on performance for large rooms. I'll try to do some tests ASAP.

@alexbrazier
Copy link
Contributor Author

@rodrigok I made an update to add a setting where notifications for all messages is disabled for large channels. Default currently set to 100 members or more. I have added a screenshot.

@geekgonecrazy
Copy link
Contributor

@alexbrazier any thoughts on from a UI/UX perspective disabling the radio buttons to choose all messages if over user limit?

@alexbrazier
Copy link
Contributor Author

@geekgonecrazy Do you mean in the per channel notification settings for the user (3rd screenshot)? A user is still allowed to individually set All messages as a preference if they want on a per channel basis, even if the number of users is over the limit. The limit only disables all message notifications on the admin and user global settings for ALL channels (the first 2 screenshots).

@alexbrazier
Copy link
Contributor Author

@geekgonecrazy Any news on whether this PR is going to be merged? Do you want me to change anything?

@engelgabriel engelgabriel added this to the 0.44.0 milestone Oct 17, 2016
@rodrigok rodrigok modified the milestones: 0.44.0, 0.45.0 Oct 25, 2016
@engelgabriel engelgabriel modified the milestones: 0.45.0, 0.46.0 Nov 1, 2016
@rodrigok
Copy link
Member

rodrigok commented Nov 1, 2016

IMHO, this PR will add a huge load to large rooms, but we can use it after the merge of branch experimental that includes cache for some records and relations, then we can update this PR to just iterate over subscriptions to get user data rather than a new find.

@alexbrazier
Copy link
Contributor Author

@rodrigok I added a setting to set the number of users in a room to disable this feature for. Currently the default is 100, so if a room has 100 users or more the notifications will go back to the original, so hopefully shouldn't have much of an impact.

@engelgabriel engelgabriel modified the milestones: 0.55.0, 0.54.0 Mar 22, 2017
@engelgabriel engelgabriel modified the milestones: 0.55.0, 0.56.0 Apr 13, 2017
@KirschbaumP
Copy link

any news?

@engelgabriel engelgabriel modified the milestones: 0.56.0, 0.57.0 May 9, 2017
@marceloschmidt marceloschmidt force-pushed the feature/default-notifications branch from dc88948 to 9cd748b Compare May 10, 2017 20:09
@marceloschmidt
Merge branch 'develop' into pr/4339-alexbrazier-feature/default-notif…
42f668d 
…ications

# Conflicts:
#	packages/rocketchat-i18n/i18n/en.i18n.json
#	packages/rocketchat-lib/server/lib/sendNotificationsOnMessage.js
#	packages/rocketchat-lib/server/startup/settings.coffee
#	packages/rocketchat-push-notifications/client/views/pushNotificationsFlexTab.html
#	packages/rocketchat-push-notifications/server/models/Subscriptions.js
@marceloschmidt
Copy link
Member

Just fixed the conflicts! @RocketChat/core can we merge it please?

@rodrigok
Copy link
Member

@alexbrazier can you fix the conflicts? I'll review it after that

@sampaiodiego sampaiodiego changed the title Add admin and user setting for notifications [NEW] Add admin and user setting for notifications May 24, 2017
@fuzunspm
Copy link

fuzunspm commented Jun 5, 2017

any news?

@engelgabriel engelgabriel modified the milestones: 0.57.0, 0.58.0 Jun 19, 2017
@stalley stalley mentioned this pull request Jul 12, 2017
Merged
@zipper2110
Copy link

This feature is to selebrate a 1 year birthday soon :)

@megamaced
Copy link

Commiserate 1 year birthday :-(. Should be merged

@geekgonecrazy
Copy link
Contributor

Not exactly a year. ;-) Still have 2 months.

@alexbrazier do you have a chance to merge conflicts?

rodrigok added a commit that referenced this pull request Jul 28, 2017
@rodrigok
Merge pull request #7479 from SKA-T/develop
78b3901 
[NEW] Add admin and user setting for notifications #4339
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marceloschmidt marceloschmidt marceloschmidt approved these changes

Assignees

@rodrigok rodrigok

Labels
None yet
Projects
None yet
Milestone
0.58.0
Development

Successfully merging this pull request may close these issues.

[Q] How to change the default settings for channel notifications? Set default setting for notifications