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

AdditionalExpenses and ContactTopicAnswer controllers & policies #6060

Conversation

thejonroberts
Copy link
Contributor

@thejonroberts thejonroberts commented Sep 25, 2024

What github issue is this PR for, if any?

Preparation for Case Contact Form re-design - #6048

Creates policy and controllers for AdditionalExpenses and ContactTopicAnswers. These resources will be used by frontend to persist/delete nested form items as they are added & removed - which is why only create/destroy actions exist in the controllers. This is necessary so that autosave of the form during topic answer (note) input does not re-create a new note (or try to delete a previously deleted note) every time it runs.

Previously, a blank ContactTopicAnswer would be created in the first details step per selected topic... so each answer during notes step would already have a persisted CTA to update during autosave. Expenses were not affected since autosave only happened during notes step.

Separated this stuff out to make for easier review.

@github-actions github-actions bot added ruby Pull requests that update Ruby code Tests! 🎉💖👏 labels Sep 25, 2024
@thejonroberts thejonroberts force-pushed the contact-form-refactor-prep-expenses-topics branch from fea791a to bbaf3dd Compare September 25, 2024 04:55
@@ -191,6 +194,9 @@
end
resources :case_court_orders, only: %i[destroy]

resources :additional_expenses, only: %i[create destroy],
constraints: lambda { |req| req.format == :json }

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did these as root-level resources. They could be case_contact member resources (nested), as they can't exist without a contact parent. I tend to error on the side of not nesting resources... and it was easier. Can move if need be.

The constraints for :json was new to me. Made sense for the no-view / api style endpoint.

@thejonroberts thejonroberts force-pushed the contact-form-refactor-prep-expenses-topics branch from bbaf3dd to cf46fac Compare September 25, 2024 16:04
@thejonroberts thejonroberts marked this pull request as ready for review September 25, 2024 16:21
@thejonroberts thejonroberts mentioned this pull request Sep 25, 2024
2 tasks
@thejonroberts thejonroberts force-pushed the contact-form-refactor-prep-expenses-topics branch from cf46fac to f09a6b4 Compare September 28, 2024 18:22
@elasticspoon
Copy link
Collaborator

Hey! @thejonroberts

Trying to review this PR, mainly trying to get my head about the autosave json stuff. Could you explain this part a bit more (from the big PR):

Maintaining the autosave feature was complicated! Before, notes were the only thing that could change during an autosave, but now everything is in the form that gets sent during the autosave update. Therefore, nested_attributes_for contact_topic_answers and additional_expenses required a stimulus NestedForm controller that could create and destroy records -- otherwise, autosave would make new records for every save, or try to delete records that had already been removed in the prior update. This also required creating controllers & routes for those json requests.

If I understand you correctly you are saying on every form update(?) we create new contact topic answers and new additional expenses.

Doesn't the existing autosave just submit the form in the background with a debounce? It works with both notes and contact topic answers. What is breaking that behavior here?

Did you explore any other ways of retaining the autosave without the additional controllers? What were the issues?

@thejonroberts
Copy link
Contributor Author

thejonroberts commented Sep 29, 2024

@elasticspoon the relevant bits of the old flow were:

  1. Select applicable ContactTopics during details step
  2. Enter Answers for those 'selected' in 1 (& autosave on text value inputs)
  3. Enter Expenses

Step 2 is the only step with autosave in effect. Since topics were already selected in step 1, you aren't adding new (non-persisted) ContactTopicAnswers, only changing the value of existing answers. (I don't think removing was an option, you could just sort of ignore the topics you didn't want to elaborate on). Therefore autosave can submit & re-submit the entire form, everything is already in the database, and it's update only.

Expenses/Step 3 doesn't autosave, so you can add/remove as much as you want without hitting the database until submission.

In the new, all-in-one form, you run into issues autosaving the form:

  1. Adding an entry (topic answer or expense)
    • Add Note -> enter value -> autosave creates an Answer record...
    • Enter more info in same field -> autosave creates another record because form doesn't have an ID with the field, it was added after page load.
    • Submit or reload the page, you will have multiple records, one for each time autosave happened.
  2. Deleting an entry
    • When editing an existing contact that has answers/expenses with IDs
    • Delete an item - form hides in display but marks hidden _destroy input true
    • Autosave should work fine once
    • Autosave again or submit will fail RecordNotFound because you've deleted the record already, but we tried to destroy it again.

Put simply, the form needs to be kept in sync with the database when associated records are created or destroyed. The form needs to know IDs of created records and when a record has been deleted, so that autosave can work.

The delete could probably be worked around / rescued somehow in the controller. But the Add problem requires the form to know IDs before it tries to autosave any database records.

The only other approach I thought of was something like:

# in update action method:
@case_contact.contact_topic_answers.destroy_all
@case_contact.additional_expenses.destroy_all
@case_contact.update(case_contact_params)

Deleting all the records just seems wrong... especially before an update... seemed too risky to me, if update fails, what then?

So I figured make the Add and Delete buttons a database-persisted action via these controllers, plus javascript (other PR) to update the form with the new IDs or removing deleted entries entirely (so they do not make a second destroy request).

@thejonroberts
Copy link
Contributor Author

thejonroberts commented Sep 29, 2024

@elasticspoon it seems like a good use of a turboframe - just replace the entire form on autosave.... but I worried about the response replacing an input as the user is interacting with it!
like user input, pauses -> (autosave request), user resumes typing -> that is replaced with autosave response (first input)

@thejonroberts thejonroberts force-pushed the contact-form-refactor-prep-expenses-topics branch from f09a6b4 to d3418d2 Compare September 30, 2024 16:13
@elasticspoon
Copy link
Collaborator

What you are saying makes sense. Thanks for the detailed explanation.

Deleting all the records just seems wrong... especially before an update... seemed too risky to me, if update fails, what then?

I guess you could wrap it in a transaction, but with it happening as often as it does, that might be an issue.

@thejonroberts thejonroberts force-pushed the contact-form-refactor-prep-expenses-topics branch from d3418d2 to ba5069a Compare October 6, 2024 16:13
@thejonroberts
Copy link
Contributor Author

Closing as #6048 has gotten some feedback and is almost ready for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ruby Pull requests that update Ruby code Tests! 🎉💖👏
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants