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.

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

Merged
merged 18 commits into from
May 7, 2024
Merged

Conversation

ShanaLMoore
Copy link
Collaborator

@ShanaLMoore ShanaLMoore commented May 1, 2024

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

image

Notes

kirkkwang and others added 4 commits May 1, 2024 06:39
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'.
@ShanaLMoore ShanaLMoore changed the title I1019 batch email notifications 🚧 WIP: I1019 batch email notifications May 1, 2024
@ShanaLMoore ShanaLMoore added the patch-ver for release notes label May 1, 2024
db/schema.rb Outdated Show resolved Hide resolved
The prior default value of `off` appears to be a reserved term
and did not work for i18n translations.
@laritakr laritakr force-pushed the i1019-batch-email-notifications branch from 41e4780 to ce55983 Compare May 1, 2024 19:52
- Show email frequency on user profile
- Add selection list to edit profile menu to update frequency
- Add appropriate translations to en.yml

Ref: scientist-softserv/palni-palci#1022
@laritakr laritakr force-pushed the i1019-batch-email-notifications branch from ce55983 to 0861258 Compare May 1, 2024 20:00
@laritakr
Copy link
Collaborator

laritakr commented May 1, 2024

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.
@kirkkwang kirkkwang force-pushed the i1019-batch-email-notifications branch from fa8a90b to 7c0c652 Compare May 2, 2024 23:57
@kirkkwang kirkkwang marked this pull request as ready for review May 3, 2024 16:18
@kirkkwang kirkkwang changed the title 🚧 WIP: I1019 batch email notifications 🎁 1019 batch email notifications May 3, 2024
laritakr
laritakr previously approved these changes May 6, 2024
Copy link
Collaborator

@laritakr laritakr left a 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>
Copy link
Collaborator

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.

@ShanaLMoore
Copy link
Collaborator Author

ShanaLMoore commented May 6, 2024

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.
@kirkkwang kirkkwang merged commit 57108c2 into main May 7, 2024
4 checks passed
@kirkkwang kirkkwang deleted the i1019-batch-email-notifications branch May 7, 2024 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch-ver for release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants