-
Notifications
You must be signed in to change notification settings - Fork 500
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
New feature for muting email and/or in-app notifications #8530
Conversation
… given types of notifications
Hi all, Eryk and I have been looking at the option to customize your notifications and emails as this is something we would want in our (KU Leuven RDR) installation and it’s something that has been mentioned before by others in the community. Above, in Eryk’s screenshot, you can get a first idea of how notifications can be turned on or off by users in our current testing set-up. Scenario 1: Leave it all up to the users. They are able to mute each notification and email separately. Scenario 2: Only allow users to choose between ‘all notifications’ and ‘essential notifications’ (e.g. access request notifications and API token expiration notifications), with the admin then being able to decide what ‘essential notifications’ are for the dataverse via the admin dashboard or via API. Scenario 3: The feature is only manageable from the admin level and not up to the users individually. Users don’t have any choice and the admin decides for the entire dataverse what the notifications are that a user receives for datasets in that dataverse. Please provide us with some input and maybe further suggestions as we continue to work on this feature to make it a bit more user friendly. Dieuwertje |
This code change is between scenario 1 and 2: the admin can choose which notifications are essential and are not selectable by users, and the user can mute the remaining (not essential) notifications all at once, or one by one by picking them from the list. |
… position in bundle.properties
…e users cannot unmuted these muted types
I have added the support for the third scenario: you can set |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @ErykKul and @DieuwertjeBloemen for your work on this.
To make it more likely this code will be accepted upstream, you should try to discuss your configuration approach with the other devs.
src/main/java/edu/harvard/iq/dataverse/authorization/providers/builtin/DataverseUserPage.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ErykKul I left a few comments... 😉
What looks like isn't covered currently: what about concurring admin settings? What happens if a notification is in both never and always muted? I think it would be fine to define a "wining" type, but that should be clearly formulated/documented in code and IMHO also be logged, as this might cause confusion.
src/main/java/edu/harvard/iq/dataverse/settings/SettingsServiceBean.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/harvard/iq/dataverse/UserNotificationServiceBean.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/harvard/iq/dataverse/authorization/users/AuthenticatedUser.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/harvard/iq/dataverse/authorization/users/AuthenticatedUser.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/harvard/iq/dataverse/authorization/providers/builtin/DataverseUserPage.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/harvard/iq/dataverse/authorization/providers/builtin/DataverseUserPage.java
Show resolved
Hide resolved
@poikilotherm, I have implemented the requested changes. Thank you for your detailed review, I appreciate it since I can learn some things early on. As for the user interface change, I think it should be first discussed by people responsible for functionality, e.g., @DieuwertjeBloemen. I have also documented that the always muting has priority over never muting, if both are set for a certain type, a warning is printed in the log. |
Hi, I'm currently just collecting as much input as possible for the user interface and it's functionality. So, thanks for the input and I'll note it down. Then, I'll collect all input and work out a couple options for Eryk to look at to see what's technically feasable as a first version of the feature. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've run the code and played with it but haven't done extensive testing. I did try to improve docs and tests here and there. Also, please note than in addition to the original issue (#7492), thanks to @ErykKul this pull request now also addresses #8487.
Also, as discussed in Slack, we renamed some SQL scripts in ea342ab so developers will need to fix up their flyway history (or drop their database and get set up again): https://guides.dataverse.org/en/5.10.1/developers/sql-upgrade-scripts.html#renaming-sql-upgrade-scripts
@ErykKul I'm having trouble deploying this pr for testing as it is colliding on the flyway script number: 5.10.1.0.1 | 8533-semantic-updates. Would you mind updating your script number to avoid this? Thanks |
duplicate_table is thrown when the constraint is unique
In the API Guide, some of the notifications versus email endpoints have the same title, which makes it somewhat confusing: |
@ErykKul I've finished my testing, thanks for all the work. I did find one thing that could use a look: when putting or deleting an end user notification type, eg. CREATEDS of either notification or email, that is an invalid string, eg. xCREATEDS, the return is {} and 500 error and the server log shows an error: [2022-05-23T18:36:32.514+0000] [Payara 5.2021.5] [WARNING] [AS-EJB-00056] [javax.enterprise.ejb.container] [tid: _ThreadID=75 _ThreadName=http-thread-pool::http-listener-1(1)] [timeMillis: 1653330992514] [levelValue: 900] [[ |
@kcondon I have improved the error handling. Thank you for testing and feedback! |
@ErykKul Thanks, that looks good. One other thing I noticed and it might be a limitation related to sessions versus API, is that when you update the notification state using the end user api, it does not affect the current user session (does not mute/unmute or appear in the notifications checklist) until the user logs out, then back in. Is this a technical limitation as I mentioned? We have seen such in the past so I did not report it, assuming it was the same. |
@ErykKul I've discussed the session issue with the dev team and it was viewed as minor impact and likely not easy to fix so we'll merge it. Thanks! |
What this PR does / why we need it:
This new feature can be turned off by setting notification.showMuteOptions to "false" in Bundle.properties. By default it is enabled now. This feature allows turning off notifications (email or in-app) for user specified types. The types of notifications that can be turned off by users for themselves can be limited by using empty notification.typeDescription property, e.g., notification.typeDescription.CONFIRMEMAIL is turned off by default (also in Bundle.properties).
Which issue(s) this PR closes:
Special notes for your reviewer:
I have used flags system (bitwise comparisons) in order to avoid a join on db. Retrieving user query is called frequently, so this is done in order to keep performance acceptable.
Suggestions on how to test this:
Remember to set the Bundle properties. Login as a user and go to notifications, you should be able to mute certain notifications only for yourself.
Does this PR introduce a user interface change? If mockups are available, please link/include them here:
The notifications tab has a new feature. This could be turned off in order to keep the user interface unchanged.
Is there a release notes update needed for this change?:
Yes.
Alternative UI: