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

4473 expand reminder date possibilities #4606

Open
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

jlandiseigsti
Copy link

@jlandiseigsti jlandiseigsti commented Aug 22, 2024

Resolves #4473

Description

Replaces Reminder Day with Reminder Schedule. This is a recurring schedule that can occur on alternating months, or send reminders based on days of the week. Anywhere that a Organization or Partner Group could have a reminder day now has a reminder schedule.

Type of change

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

How Has This Been Tested?

System tests to walk through the process.
Model tests to ensure the translation from the form to the ical schedule happens correctly.
Validation tests to ensure that bad data doesn't get submitted.

jlandiseigsti and others added 9 commits July 22, 2024 08:34
Via db migrations, replaces the simple integer `reminder_day` with the more complex
`reminder_schedule` which is an ical string that can be parsed to a repeating Schedule class.
Adjusts the logic in the `fetch_partners_to_reminder_now_service` to use the repeating schedule.
Updates related tests.
Builds an ActiveModel, ReminderSchedule, which takes the necessary information
and turns it into an IceCubeSchedule, which is what ultimate get saved in the db.

Builds the form for this reminder schedule. Still needs to conditionally show or hide
sections depending on if the user wants date or day of the week.
Now either while creating new Organization/Partner Groups or editing them,
all the transfers from Reminder Date to Reminder Day have been made.
This uses css for ease and accessibility.
 Adds a number of refactors, including moving all the create_schedule logic
 to the deadlinable helper, and creates unit and system tests for the new functionality.
Reintroduces the functionality of having warnings for having the reminder
date the same as the deadline date.
We don't want to create a new schedule if nothing has changed,
as it will reset the start date and potentially mess with those
who have every 2/3/etc months reminders.
@jlandiseigsti jlandiseigsti force-pushed the 4473-expand-reminder-date-possibilities branch 2 times, most recently from acba202 to aa3e2f5 Compare September 16, 2024 20:11
…into 4473-expand-reminder-date-possibilities
@jlandiseigsti jlandiseigsti force-pushed the 4473-expand-reminder-date-possibilities branch from aa3e2f5 to 044e6bf Compare September 16, 2024 20:14
@jlandiseigsti jlandiseigsti marked this pull request as ready for review September 16, 2024 20:19
@cielf cielf self-requested a review September 19, 2024 16:20
Copy link
Collaborator

@cielf cielf left a comment

Choose a reason for hiding this comment

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

Hey @jlandiseigsti. Thank you! I took this out for a manual test spin, and noticed some things that need attention:
1/ I don't think we need the "Send reminders every X months". If they set a reminder date, then it's monthly.
2/ The reminder date doesn't seem to be persisting.
3/ It's not giving an error if I enter a negative date.

Also a question -- would your method allow "last" in the reminder day of the week area?

@jlandiseigsti
Copy link
Author

Hi, @cielf , I'll get to work addressing those issues.

I believe the ice cube gem allows for "Last"...I'll implement that.

@jlandiseigsti
Copy link
Author

One product question: if an organization wants to indicate that they want to stop sending reminders, how should they indicate on the edit page? Unlike the Partner Group page, there's not a Send Reminder Checkbox.

@jlandiseigsti
Copy link
Author

jlandiseigsti commented Sep 19, 2024

One product question: if an organization wants to indicate that they want to stop sending reminders, how should they indicate on the edit page? Unlike the Partner Group page, there's not a Send Reminder Checkbox.

My assumption is that they either delete the date, if set to "Date", or choose the Blank Option in every Nth Day, if set to "Day of the Wee". But I wanted to confirm that.

@jlandiseigsti
Copy link
Author

Also, regarding these two corrections:

2/ The reminder date doesn't seem to be persisting.
3/ It's not giving an error if I enter a negative date.

Was the value for Every N Months empty when these happened? That's the only way I was able to recreate the errors (as I had written it that Every N Months blank meant no reminder, and thus no validation/saving.) If so, that's great news, as I'm removing the Every N Months. If there was a value for Every N Months when those bugs occurred, there's something else going on I have to figure out.

All reminders should be Monthly, so every N Months is removed.

Also allows users to select "Last" as an option.
@cielf
Copy link
Collaborator

cielf commented Sep 19, 2024

One product question: if an organization wants to indicate that they want to stop sending reminders, how should they indicate on the edit page? Unlike the Partner Group page, there's not a Send Reminder Checkbox.

My assumption is that they either delete the date, if set to "Date", or choose the Blank Option in every Nth Day, if set to "Day of the Wee". But I wanted to confirm that.

That sounds good as a mechanic to me. It matches up to what they would do now.

@cielf
Copy link
Collaborator

cielf commented Sep 19, 2024

Also, regarding these two corrections:

2/ The reminder date doesn't seem to be persisting.
3/ It's not giving an error if I enter a negative date.

Was the value for Every N Months empty when these happened? That's the only way I was able to recreate the errors (as I had written it that Every N Months blank meant no reminder, and thus no validation/saving.) If so, that's great news, as I'm removing the Every N Months. If there was a value for Every N Months when those bugs occurred, there's something else going on I have to figure out.

Might have been -- there's a placeholder of 1 and I might have just left it like that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expand possible dates for reminder emails
2 participants