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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rename email reminders task to include first #246

Merged
merged 1 commit into from
Jun 28, 2019

Conversation

cintamani
Copy link
Contributor

This renames the Email reminders to include first so that we can distinguish between the first and second email reminders later on.

Open to suggestion. I am not super super happy about this naming 馃

This rename the Email reminders to include `first` so that we can distinguish between the first and second email reminders later on.
@cintamani cintamani self-assigned this Jun 28, 2019
@cintamani cintamani added the housekeeping Refactoring, tidying up or other work which supports the code label Jun 28, 2019
Copy link
Member

@Cruikshanks Cruikshanks left a comment

Choose a reason for hiding this comment

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

I've spoken to the team in the office today. Currently it looks like the content for the first and second email is the same.

However they feel there is scope for having different content in the second email.

So the 2 service objects will have 2 different params

  • the period of time to filter expiring registrations by
  • the email to actually send

So to me that leaves 2 options

  • go with what you have done (and I cannot think of a better name at this time 馃榿 )
  • pass in these values to a generic RenewalReminderService from the rake task

I like to keep rake tasks as simple as possible, so to avoid looking at env vars and the like my preference would be we end up with a FirstRenewalReminderService, SecondRenewalReminderService, and a BaseRenewalReminderService possibly in a app/services/reminder namespace.

On that basis I'm happy with the changes here.

@cintamani
Copy link
Contributor Author

Thanks @Cruikshanks :D Me and Iris discussed this already #245 (comment) :) we agree with the 2 separate services approach but I have left this change out of the other main PRs :)

@cintamani cintamani merged commit 612d1e8 into master Jun 28, 2019
@cintamani cintamani deleted the rename-email-reminder-task-name branch June 28, 2019 13:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
housekeeping Refactoring, tidying up or other work which supports the code
Projects
None yet
2 participants