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

Patch autosave associations to support conditional validations #2381

Merged
merged 2 commits into from
May 29, 2018

Conversation

lostie
Copy link
Contributor

@lostie lostie commented May 25, 2018

What

For more details please read NOTE in config/initializers/01_autosave_association_patch.rb

Claim associations are autovalidated by default as long as they're defined. With this change, they're only validated if they're required for a specific step/stage or if they're being submitted through the API.

Ticket

Check your claim page fixes

Why

Rails models that accept nested attributes for some of its associations autosave and validate those associations by default.

What this means in effect is that, even though the application has its own stage/step validation setup for the claims model, as long as an association was previously set in another stage, when submitting data for another stage, that association will still be autosaved and validated. (This is noted more so when one of the stages is submitted as a draft with invalid data for one of the associations and other stage is then submitted with valid data but fails due to the previously submitted association being invalid, even though we're not at the step of validating that association).

How

  • The claim model no longer has a default form_step, as that information is specifically related with the claim submission process only, and the form_step is now set only during that flow.
  • Autosave associations in Rails was patched to evaluate the value set for validate: <value> as by default it just checks if is true or not. This way it allows us to control when these associations are actually validated, letting the validations classes behave as expected during the submission flow and only validate the data related with that particular stage. (NOTE: Obviously this patching will potentially have an impact on any future Rails updates)

Also, worth of note:

  • Some of the fee factories, now detect if the associated fee type already exists instead of creating a brand new one. This was done mainly to avoid flakey tests and unexpected behaviours and should also helps making use of the seed data more reliably going forward.
  • There's a known issue with the factories definition that needs to be addressed. The resolution is not part of this PR as this would take quite a lot of refactoring. In this PR, the previous behaviour (which was to create a claim with a default form_step was kept).

@ghost ghost assigned lostie May 25, 2018
@ghost ghost added the in progress label May 25, 2018
@lostie lostie added the WIP label May 25, 2018
@lostie lostie force-pushed the check-your-claim-page-further-fixes branch 7 times, most recently from 5ebc9d4 to e83dae8 Compare May 25, 2018 15:05
For more details please read NOTE in
config/initializers/01_autosave_association_patch.rb

claim associations are autovalidated by default as long as they're
defined. With this change, they're only validated if they're required
for a specific step/stage or if they're being submitted through the API.
@lostie lostie force-pushed the check-your-claim-page-further-fixes branch from e83dae8 to 4ef04d3 Compare May 25, 2018 15:54
@lostie lostie requested a review from colinbruce May 29, 2018 09:20
Copy link
Contributor

@colinbruce colinbruce left a comment

Choose a reason for hiding this comment

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

👍 LGTM

Rationale behind it was the first step should be mandatory, but
apparently people still WANT to save as draft no matter what.
@ghost ghost added the in progress label May 29, 2018
@lostie lostie merged commit be98056 into master May 29, 2018
@ghost ghost removed the in progress label May 29, 2018
@lostie lostie deleted the check-your-claim-page-further-fixes branch May 29, 2018 14:29
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.

2 participants