CAPT-1633 Form objects for reminders | CAPT-1634 Encapsulate pre/post form rendering/submission logic per-journey (Part 1)

[your]

This PR is the first iteration of CAPT-1634 applied to an isolated controller that handles a mini-journey for reminders (ECP/LUPP) and converts the reminders slugs to form objects as per CAPT-1633. In a follow-up PR, I will be adding the new concern to the claims controller and iterate with any improvements or changes required.

How to review / my notes

1. Review the FormSubmittable concern and associated request spec; I won't repeat the rationale here as it's explained extensively in various comments inside the concern; worth noting that:

• It's a first iteration, designed with speed in mind; each GET/POST/PATCH request on a <path>/:slug will dynamically define all the pre-post rendering/submission callback methods for that slug only. I have considered defining them as singleton methods so we could tap into them in a more declarative way (i.e. passing a block or a method name and options), but I didn't feel like pursuing it yet (it would probably mean storing all the callbacks and options in a variable for all slugs in a single req-res cycle):

   # potential future iteration...

   after_form_save_success :retrieve_student_loan_details, on: :"personal-details"

   # ...vs current iteration

   def personal_details_after_form_save_success
     retrieve_student_loan_details
   end

• I have changed the concern name so many times, but I really like FormSubmittable now as it helps define a specific trait of controllers that handle form-based page sequences
• This approach can allow for cleaner code as long as developers keep the callback implementations readable and slim
• Callbacks can be difficult to debug; so are the rails controller filters (*_action), and are necessary for complex request cycles. I have added logging when filters are triggered; see an example here:

tail -f log/development.log| grep callback
2024-05-13 20:34:15.434222 I [79219:puma srv tp 005] {request_id: d0d27584-edb4-428d-92fb-cf40c0472e2f} Journeys::AdditionalPaymentsForTeaching::RemindersController -- Executing callback #handle_form_submission
2024-05-13 20:34:15.434457 I [79219:puma srv tp 005] {request_id: d0d27584-edb4-428d-92fb-cf40c0472e2f} Journeys::AdditionalPaymentsForTeaching::RemindersController -- Executing callback #email_verification_after_form_save_failure
2024-05-13 20:34:42.736920 I [79219:puma srv tp 001] {request_id: eb06c4f0-40b3-44f4-90a1-d5ee838d25d8} Journeys::AdditionalPaymentsForTeaching::RemindersController -- Executing callback #email_verification_before_update
2024-05-13 20:34:42.739362 I [79219:puma srv tp 001] {request_id: eb06c4f0-40b3-44f4-90a1-d5ee838d25d8} Journeys::AdditionalPaymentsForTeaching::RemindersController -- Executing callback #handle_form_submission
2024-05-13 20:34:42.752033 I [79219:puma srv tp 001] {request_id: eb06c4f0-40b3-44f4-90a1-d5ee838d25d8} Journeys::AdditionalPaymentsForTeaching::RemindersController -- Executing callback #email_verification_after_form_save_success

• The concern is not tight to the PageSequence logic (which should probably be revised anyway), and only expects slugs to be defined along with current_slug and next_slug, so it can be used with any sequence

2. Look at the new form objects for the reminders

• I have realised that our Form#model_name should probably be generic and not linked to any particular AR model. I'm using a generic "form" name for the newly converted reminders slugs, but it could be extended to all the other forms
• I have tried to re-use existing form objects for the reminders slugs, but:
  • personal-details is quite different from our "shared" one, as in this case it only focuses on full_name and email_address. I don't think it's worth extracting validations and sharing, not in this PR
  • email-confirmation is similar to the "shared" one, but in the shared one the OTP generation timestamp is an attribute of the current_claim... I didn't want that to be an attribute of current_reminder, but rather a transient attribute for the form object, hence I have injected it into the session and proxied to the form object. A similar approach could be used for the shared one
• I'm using a current_data_object wrapper as the claim: argument to initialising the form object; I think it's a good abstraction
• Forms needed to be re-organised per controller and not just journey
• I have realised that our reminders journey is always "open", even if a window is closed... addressed that as well.

https://dfedigital.atlassian.net/browse/CAPT-1633
https://dfedigital.atlassian.net/browse/CAPT-1634

[github-actions]

Review app deployed to https://s118d02-app-pr-2742-as.azurewebsites.net/additional-payments/claim

[rjlynch]

Could we store the sent_one_time_password_at on the Reminder? Then we can access it in the form object which might make things a bit simpler

[your]

I understand your question, and I remember seeing you explain why that was a good idea for the Claim model.

My thoughts are that storing sent_one_time_password_at, a transient attribute, directly on the Reminder model might introduce unnecessary coupling between the model and the form object. While with the existing approach there is coupling between the controller and the form object, this coupling is more acceptable and expected within the context of MVC architecture. Controllers (and the "state", e.g. in the session) are meant to interact with form objects to handle form submissions and orchestrate the flow of data within the application. Either way I don't feel too strong, and it might make sense to have a consistent approach, but I wouldn't consider it a blocker. Will have a think

[rjlynch]

I'm not sure sent_one_time_password_at is really transient 🤔 but I don't think it's too big a deal to keep it in the session 👍

[slorek]

👏 I think this is awesome work, thank you.

I do understand there will initially be a bit of extra cognitive load during debugging, but once you understand the callback structure (which you have documented well) it might actually make it a lot easier since the concerns are more clearly separated.

This is not the same problem as callbacks which are inserted in non-obvious ways since there is a fixed structure which will be commonly used as it unlocks the ability to support multiple journeys.