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

Combined email expiry notifications with more information #413

Merged

Conversation

aestoltm
Copy link
Collaborator

@aestoltm aestoltm commented Jun 20, 2022

Fixes #270 and Fixes #297 : Combined email expiry notifications for resources that are expiring soon. Also included more information given to the user for email notifications for resources expiring soon and expired resources.

Tested using task tests for #270 and included more statuses to test such as 'Payment Pending', 'Payment Requested', 'Unpaid' . Also tested different allocation attribute values for 'EXPIRE NOTIFICATION'.

Update: Fixes #289 : Edited original commit to send emails to center staff for allocations that have expired. This is configurable by setting 'ADMIN EXPIRE NOTIFICATION' allocation attribute to 'No' if receiving emails for a specific allocation is not desired. Need to run 'coldfront add_allocation_defualts' to update allocation choices.

@dsajdak dsajdak added the needs review Waiting for review label Jul 21, 2022
@aebruno aebruno requested a review from dsajdak August 5, 2022 13:49
coldfront/core/allocation/tasks.py Outdated Show resolved Hide resolved
docs/pages/config.md Outdated Show resolved Hide resolved
Copy link
Contributor

@dsajdak dsajdak left a comment

Choose a reason for hiding this comment

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

Changes have been tested and look really good. Just a few other changes requested:

  1. When an allocation is set to expire today, we're not seeing that in any emails. It's not a separate email and the combined email is showing a breakdown of allocations expiring in 7, 14, and 30 days.
  2. For the admin email listing the allocations that have expired, let's remove the /renew in the URL. This is a remnant of the emails going to users and not necessary for the admins.
  3. For the admin email, no need to separate that list out by project as it doesn't matter to the admins. You can just list out all the allocations that expired that day.

Copy link
Contributor

@dsajdak dsajdak left a comment

Choose a reason for hiding this comment

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

Testing on this looks good. I think we just want to update this wording here and then we're good to go: https://github.com/ubccr/coldfront/pull/413/files/fa465b580b9a2ad6d30b06abe4b4730b2bb7d7de

'email/admin_allocation_expired.txt',
admin_template_context,
EMAIL_SENDER,
[EMAIL_TICKET_SYSTEM_ADDRESS,]
Copy link
Member

Choose a reason for hiding this comment

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

Let's send the admin email to the EMAIL_ADMIN_LIST instead of the ticket system address.

@dsajdak dsajdak self-requested a review August 9, 2022 15:49
Copy link
Contributor

@dsajdak dsajdak left a comment

Choose a reason for hiding this comment

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

Minor change requested on email subject & first line:
Email subject: Your access to resource(s) are expiring soon - change to 'Your access to [center name]'s resources is expiring soon'
First line: Your access to CCR resources are expiring soon. - change to 'Your access to [center name]' resources is expiring soon'

@dsajdak dsajdak self-requested a review August 9, 2022 15:51
Copy link
Contributor

@dsajdak dsajdak left a comment

Choose a reason for hiding this comment

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

Sorry meant for this to be a 'request changes' review. See comment: #413 (review)

Copy link
Contributor

@dsajdak dsajdak left a comment

Choose a reason for hiding this comment

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

Looks great!

@aebruno aebruno merged commit 9bc9066 into ubccr:master Aug 9, 2022
@dsajdak dsajdak removed the needs review Waiting for review label Aug 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants