rubyforgood
4473 expand reminder date possibilities
Resolves #4473
Description
This PR is built off of the work done by jlandiseigsti.
This PR expands the _deadline_day_fields.html.erb partial to include options to configure:
- The monthly frequency of reminders
- The date of the month the reminder will be sent
- The nth weekday of the month the reminder will be sent
This definition is then saved as an ICAL format string to the new reminder_schedule field of the Organization or PartnerGroup models which replaces the old reminder_day field. The ice_cube gem is used to convert to and from the ICAL format schedule.
The form is reactive, and will show users a preview of when the next reminder/deadline will be based on their selection as well as warns them if their choices violates a constraint. This is implemented using a Stimulus controller reworked from the prior deadline_day_pickers.js.
This PR expands the Deadlinable ActiveSupport::Concern, to contain the logic for validating the fields of the _deadline_day_fields.html.erb, converting the values to ICAL format, etc.
This PR modifies the FetchPartnersToRemindNowService to consider the Organization and PartnerGroup reminder_schedule when determining if a partner should be emailed.
The documentation in the user guide has been updated and expanded to more thoroughly explain these features. Included is the excellent flowchart which clief created.
This PR adds the following dependencies:
In my investigation of the rrule library, I found the similar rschedule library. While I went with rrule because it was older and had more widespread usage, if some issue with rrule comes up rschedule seems like a good first place to look next.
It was considered changing how the Partner.send_reminders field worked, as it's strange that it matters for Organization level schedules, but not for PartnerGroup level schedules. I am still of the opinion this is something that should be fixed, but when discussed with @cielf it was determined it could be saved for another PR.
Similarly, it was considered reworking deadline_day to be similar to reminder_schedule and permit deadlines to be defined as "The Nth Weekday of the month". Since this wasn't specifically requested, it was determined it could be saved for another PR.
The deadline_day field is constrained to be between 1 and 28. For some reason, this constraint is enforced in the db/schema.rb for PartnerGroup.deadline_day but not Organization.deadline_day. I refrained from modifying either definition in the schema because the intention was unclear to me, and I think it possible deadline_day will be reworked as described above anyway.
Type of change
- New feature (non-breaking change which adds functionality)
- Documentation update
How Has This Been Tested?
I manually tested that the form and it's dynamic elements worked as expected.
Multiple tests were added to spec/models/concerns/deadlinable_spec.rb to verify the various new functions added.
Tests validating the constraints on reminder_schedule and deadline_day fields in spec/models/organization_spec.rb and spec/models/partner_group_spec.rb were removed, as they were redundant with the similar tests added to deadlinable_spec.rb and Deadlinable is the place where those constraints are validated.
A test was added to spec/services/deadline_service_spec.rb to verify that PartnerGroup.reminder_schedule is used in place of Organization.reminder_schedule when available.
spec/services/partners/fetch_partners_to_remind_now_service_spec.rb was updated to use the new reminder_schedule format, to test different types of schedules, and to ensure Partner.send_reminders worked as it currently does.
Multiple tests were added in spec/support/deadline_day_fields_shared_example.rb which verify the use of the _deadline_day_fields.html.erb form in the admin, organization, and partner group contexts. Included was a test to verify that the next reminder and deadline dates calculated by the frontend match the calculations on the backend.
Included in jlandiseigsti's work were tests added to spec/system/organization_system_spec.rb verifying assorted viewing and editing workflows that were not directly related to the issue. They seemed like fine tests and it didn't seem worth the effor to make a separate PR for them so they were kept around.
Screenshots
The new deadline day form, showing the calculated reminder and deadline dates calculated for a schedule by a day of the month
The new deadline day form, showing the calculated reminder and deadline dates calculated for a schedule by a weekday
The new deadline day form, showing the deadline date calculated to be next month since otherwise it would be prior to the reminder
The new deadline day form, showing a warning when the entered values are outside bounds
The new deadline day form, showing a warning when reminder and deadline day are the same
The organization's details, updated with a new schedule
@Benjamin-Couey I ran into an issue when I was trying to test the migration.
start with production branch bin / setup bin / start change the organization reminder/deadline to 15 and 20 change the first group's reminder/deadline to 16 and 21
change the branch to this one.
run migration (while the server is still up)
log shows:
20:02:01 web.1 |
20:02:34 worker.1 | bin/rails aborted!
20:02:34 worker.1 | NameError: uninitialized constant IceCube (NameError)
20:02:34 worker.1 |
20:02:34 worker.1 | recurrence_schedule = IceCube::Schedule.new
20:02:34 worker.1 | ^^^^^^^
20:02:34 worker.1 | /Users/clfisher/projects/human-essentials/spec/factories/organizations.rb:46:in 'block (2 levels) in
IS that expected? Usually I can run the migratiion on a separate thread and the process keeps going.
Ok -- this is back in my court following discussions during office hours. It's ok.
1/ On testing the migration -- I set the reminder and deadline dates on both the organization and a partner group. After migration, the reminder dates are not defined. The deadline dates were correct, though (which I would expect because they aren't being touched)
2/ The calculation of the next date -- if it's more than monthly, it's starting n months in the future.
So they can't set up a quarterly that begins in the next month.
Bleah.
Would it be workable to add a way for them to set the start month if they are not doing monthly?
[Note: I also was pretty sure I saw a difference between how it was calculating the first date for the two different modes, but I haven't been able to recreate it yet. Keep an eye out for weirdness there?]
3/ I doubt we need periods of more than 6 months -- really no more than 4 -- but I can ask the NDBN folk if they know of anyone who is working on longer than a quarterly basis -- there's an outside chance that some folk would like annual. So this is a heads up rather than a "change this now".
@Benjamin-Couey Please see the above 3 points.
1/ There was supposed to be a migration that converted old reminder_day into an equivalent reminder_schedule but clearly something is broken. I will look into it.
2/ It would be workable to set a start date; the recurrence schedules already need a start date and now they just use whatever the current date is.
I might suggest the start date always being available, not only if a monthly frequency greater than one is selected. It would let users, for example, have a monthly reminder start next month even if the reminder day hasn't happened yet.
3/ Alright, I will stay tuned.
@Benjamin-Couey Please see the above 3 points.
@cielf Alright, the first two points should have been addressed in the most recent commits.
1/ Fixed a bug with the migration that prevented it from converting from remidner_day to reminder_schedule.
2/ I added the start date field to the _deadline_day_fields.html.erb form. It defaults to the current day. By setting the start date to before or after the reminder day, you should be able to control which month the next reminder is scheduled for.
Once we're happy with the new field, I'll update the user guide to explain it.
@Benjamin-Couey
Getting closer! -- but two steps forward, one sideways.
1/ The date the reminders are started should show in the views as well, for both the organization and for the partner groups
2/ If I enter June 18 as the reminder start date, then hit save, when I come back in, the "when should the reminders start being sent?" is set to today's date. This date needs to be persistent (in the UI, not just the db), as they may edit the organization before the start date, and hijinks would ensue.)
3/ If I enter a date in the past that would result in a "Your next reminder" that is in the past, I should get some sort of error. I think it's valid (and easier) to not allow "when should the reminders start getting sent?" that are before today. 3a / Followup question: If the date first date calculated is today, will reminders get set out immediately, or is the mechanism such that it's a job run sometime early in the morning? The question behind that question is: if the users reasonably think they are sending out reminders today, are they right? [I think they are, but we should double-check that, and if they are not, don't allow today, and adjust the default to tomorrow ]
FWIW, I haven't tested the actual sending as yet (I only want to go through that process once).
@cielf while updating stuff so an entered start date remains persistent across page loads, some weird behavior I hadn't anticipated came up that I wanted your opinion on.
Because the start date field auto fills with today in the form, the first time anyone saves the org details form that start date will be saved. While technically consistent with what you described, it seems unintuitive that making no changes in a form but clicking the save button will create persistent data for the object.
How do you want to handle this situation?
Well, I suppose we could save it only if the other date info is filled in.
That would mean that for the people who don't currently have reminder dates, if they ever decide to, they'd be working with a default of 'today' when they do set somehing up. The people who already have reminder dates, it shouldn't be a big issue for.
Does that make sense to you?
Well, I suppose we could save it only if the other date info is filled in.
That would mean that for the people who don't currently have reminder dates, if they ever decide to, they'd be working with a default of 'today' when they do set somehing up. The people who already have reminder dates, it shouldn't be a big issue for.
Does that make sense to you?
That make sense, thank you.
FWIW, Probably should put a comment to explain why in the code -- this is sufficiently unusual that it won't be intuitively obvious to future coders.
@cielf Alright, pushed some commits addressing things that came up in the last office hours. Namely:
- The start date field now comes after the monthly frequency field and is labeled "Reminder start being sent after:"
- The messages that appear as the user fills out the form now read "Your next reminder date is [reminder date]."
- The dates calculated on the front end now take into account both the start date of the reminder schedule, as well as the current date. For example, if today is the 22nd, the reminder is on the 21st, and the start date is the 20th, the interface will tell the user their next reminder date is June 21st.
- As a consequence of this, the interface won't tell the user that their next reminder is today. Given the inconsistency of reminders firing on the day they were set up that we discussed in the office hours, this seems desirable. Once we're happy with the interface, I'll make a special note about this in the user guide).
@Benjamin-Couey Tiny change #1: The label "Reminder start being sent after:" Should be "Reminders start being sent after"
Otherwise the bank-facing functionality looks good. Manually testing the sending would be difficult.
I think we can send this to @dorner for all the techical advice. (And I'll rely on him to determine that the testing of the outbound is sufficient).
@dorner regarding your comments.
I think I get the idea and I'll start working on reworking what I have. There is one point I'd like some clarification.
Then all you need to do is make sure that the organization's
reminder_schedulemethod returns aSchedulewith the right data, and you're good.
reminder_schedule is currently the name of the field that stores the schedule information as an ICal string. Am I correct in assuming that's still how we want to store the data in the database, we'd just like there to also be a method in the service class that spits out a service object from an ActiveRecord?
Yeah. Probably the cleanest API would be something like:
# Model
def reminder_schedule
Schedule.new(self.reminder_schedule_definition) # maybe rename the column to this
end
Then we don't have to assume anything about the underlying database schema.
@dorner Alright, the most recent commit contains my first pass on the new service object you described. I still need to hook it up everywhere besides the Organization page and update the tests, but I wanted to get your opinion on the overall architecture before I went to far.
As an overview, ReminderScheduleService is a class that contains all the IceCube logic and all the fields and validation needed to make a reminder schedule, and methods to spit out the schedule description and Ical representation.
This service is associated to models with the ReminderScheduleable concern which adds getters and setters so the ReminderScheduleService fields can be set through the model and callbacks to update/read the reminder_schedule_definition.
@dorner
I have some questions about your overall vision for the ReminderScheduleService, namely how data flows from fields in the form, to parameters in a controller action, to the fields of a ReminderScheduleService object when we remove the getters and setters that make the fields of ReminderScheduleService virtual fields of the associated ActiveRecord object.
If we look at the form, we see that it's currently using the simple form gem. The form is reused in multiple views where a simple form object (f) is initialized for a specific active record model (for example, this partner group view or this organization view).
If the associated ActiveRecord object (Organization or PartnerGroup), doesn't have the attributes associated with these fields, this whole form would need to be reworked. At the moment, I can't see a super straightforward way to do that, though perhaps I'm missing something.
Once the parameters have been passed to the controller, we then need to either create a ReminderScheduleService object with them or update an existing one. Again, giving the ActiveRecord object the virtual fields helps here, letting the existing assignment of parameters to the object also update the ReminderScheduleService. The alternative I can think of is to manually build the ReminderScheduleService from the params, which is admittedly not horrible but still not as clean.
How do you envision handling the data flow I described?
You should be able to do a fields_for :schedule_reminder inside the form. You might need to do a bit of twiddling, but I don't think the form requires an ActiveRecord necessarily, so the accessors should work well enough to print the correct HTML for the form. This would give you a nested part of the form that can be taken out in the controller to pass into the ReminderScheduleService.
@dorner Alright, third time's the charm. The recent commit contains my attempt removing the concern as you described. As before, it still needs to be hooked up to the PartnerGroup and the tests need updating, but I wanted to get your opinion on the architecture.
As on overview:
The Organization is now responsible for all validations relating to the deadline_day field. The ReminderScheduleService no longer makes any assumptions nor holds a reference to it's parent object.
The _deadline_day_fields.html.erb form now creates a nested form for the parent object's ReminderScheduleService object's fields. deadline_day is still associated with the parent object.
A separate reminder_schedule_params was added to the OrganizationsController controller. Prior to an Organization being updated, these params will be used to update the org's ReminderScheduleService object.
One minor suggestion, but otherwise I like the approach!
Good to hear. I'll make the tweak you suggested and then get started hooking everything up and updating the tests.
Alright, I finished fully implementing the new ReminderScheduleService, updating the existing tests to make use of the service, and adding new tests for the new service.
@cielf it's now ready for you to do another functional pass.
The "Reminders start after" field seems to be missing from the organization view.
We still have some tweaks to make to the next reminder date calculation. Here's the case: On organizaiton: every 3 months reminder start being sent after 07/09/2025 Day of the month 1 This should result in a next reminder date of August 1, not October 1
I know they could backdate the reminders start being sent after, but that is not at all intuitive. If this is behaviour I approved earlier, I was wrong.
Is this going to cause problems?
@cielf Regarding your comments in order.
Good catch, it will be fixed in the next set of commits.
I wasn't aware there was supposed to be a "Reminders start after" field in the Organization, and I assume Partner Group, view. It would be relatively straightforward to add. Regarding that, do you think it makes sense to only show the "Reminders start after" field when reminders haven't started yet? Feels like it's a little unintuitive to always see "Reminder start after 10/10/2022" or whatever.
So, it looks like the next occurrence calculation working that way is just the way the IceCube gem does things. There is the workaround of recalculating the start date the user entered based on the monthly frequency they selected. For example, if they enter the values you did and the start date is recalculated to 05/09/2025, then the next occurrence will be October 1.
Of course, this comes with the downside of the start date they entered not being the actual start date. One other idea would be to rework the start date field to have the user select the first month they'd like the reminder to be sent, instead of when the schedule starts?
@Benjamin-Couey -- I think I'm missing something about how this works (I haven't looked at the code). Is the "reminders start after" not saved, then?
@Benjamin-Couey -- I think I'm missing something about how this works (I haven't looked at the code). Is the "reminders start after" not saved, then?
The value entered into the "reminders start after" field is saved. I just wasn't aware we wanted to show that value on the org details page.