-
Notifications
You must be signed in to change notification settings - Fork 49
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
🎁 1019 batch email notifications #2200
Conversation
This commit fleshes out the `BatchEmailNotificationJob` and adds a preview for the email that will be sent.
This commit adds the new job to the queue whenever a new tenant is created. Also adding a method to the `User` model to mark all undelivered emails as delivered when their batch email frequency is changed to 'off'.
The prior default value of `off` appears to be a reserved term and did not work for i18n translations.
41e4780
to
ce55983
Compare
- Show email frequency on user profile - Add selection list to edit profile menu to update frequency - Add appropriate translations to en.yml Ref: notch8/palni-palci#1022
ce55983
to
0861258
Compare
Requires translations to run. |
This commit will ensure the correct bootstrap class is on the batch email frequency select box. It will also use the font awesome calendar icon for the label.
Since it seems 'off' was protected, we switched to 'never' instead, this commit will clean up any other places where it was set to 'off'.
This commit will add a callback for `Mailboxer::Receipt` to mark the receipt as delivered if the user set their batch_email_frequency to 'never'.
This commit will add a few specs and also redo the logic for the batch email notification send frequency. We needed a migration to the users model to track when the last email was sent to check if it's time to send another one based on their batch_email_frequency setting.
fa8a90b
to
7c0c652
Compare
This commit runs the Google translations for i18n files.
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.
Just a comment to consider, but definitely not a stopper at this point. Looks good to me.
<body> | ||
<h2>Hello, <%= @user.name %></h2> | ||
<p>You have the following notifications:</p> | ||
<p>Click <a href="<%= @url %>">here</a> to view them in Hyku.</p> |
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'm wondering if we should use the application &/or tenant name rather than "Hyku"? Not a big deal, but many of the users may not know what "Hyku" is. If the email is too generic, wouldn't they possibly just assume it's spam? Including the actual site name, a possible site logo, etc would seem more clear.
Additionally, I can see a future request might be to use i18n for the email text... just something to consider.
We discussed we will replace HYKU with the account settings, in the email. Maybe institution name? Email language support will be out of scope for this round. Have the client create another ticket for this request. |
This commit will add the Site's application name into the email subject as well as the body.
Story
Users can get the notifications in the form of an email digest set to daily, weekly, or monthly frequencies.
Refs
Expected Behavior Before Changes
Users only get notifications that they would check in the admin dashboard (top right area).
Expected Behavior After Changes
Users can choose a daily, weekly, or monthly email digest of notifications sent to their registered email.
Screenshots / Video
Notes