-
-
Notifications
You must be signed in to change notification settings - Fork 573
Resolves #4857 #4872
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
Resolves #4857 #4872
Conversation
|
@gabeparra01 can you add a spec for this behavior? Preferably for both create and update? |
@dorner I'm glad you asked! I noticed another issue that needed to be fixed while writing the tests: Only schedule reminder email if box is checked I added the specs in this commit: Added specs for distribution reminder emails |
dorner
left a comment
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.
Looks good, thanks!
|
@gabeparra01: Your PR |

Resolves #4857
Description
Local testing showed that the schedule_reminder_email method works as expected. The issue seems to be that the method is only called in the controller's update method when it should also be called in the create method.
There was an edge case concern discussed here. I debugged the workflow for updating an existing distribution and confirmed that the updated

issued_atdate is being used to reschedule the reminder email. Please see the screenshot attached below. Although, I do not know if the previous reminder email would be canceled. I think I could add a method here to check for old reminders? But that is another edge case.Type of change
How Has This Been Tested?
This was tested locally and I took the following steps:
Screenshots