Define steps in ConsentForm #482
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tvararu
commented
Sep 5, 2023
Replace the `on:` context based validation with conditional validation that relies on specifying a `form_step`. This also makes the validations fire off for previous steps, and at the end for the final `record` step. Unifies the `*_params` methods in the EditController into one `update_params` method. `required_for_step?` takes an optional `exact` parameter, for transient steps like the `school` one that ask a question but don't actually save a field to the database (and shouldn't be re-validated on other steps). Overall approach based on: https://www.joshmcarthur.com/2014/12/23/rails-multistep-forms.html
misaka
reviewed
Sep 5, 2023
misaka
reviewed
Sep 5, 2023
|
Sorry, I should say that those comments aside, I really like this change ... stuff like sticking the form steps into the model is good, even if at first glance the form step s are a controller concern ... the fact that they are here with the step validations is good. |
Allows us to use .create! as before.
tvararu
force-pushed
the
refactor-edit
branch
from
72e8092 to
99a774d
Compare
Allows us to use a more terse syntax for testing validations.
misaka
reviewed
Sep 6, 2023
tvararu
commented
Sep 6, 2023
spec/models/consent_form_spec.rb
Outdated
Comment on lines
32
to
36
| # expect(cf).to validate_comparison_of(:date_of_birth) | ||
| # .is_less_than(Time.zone.today) | ||
| # .is_greater_than_or_equal_to(22.years.ago.to_date) | ||
| # .is_less_than_or_equal_to(3.years.ago.to_date) | ||
| # .on(:update) |
There was a problem hiding this comment.
I'm not sure why this doesn't work
NoMethodError:
undefined method `validate_comparison_of' for #<RSpec::ExampleGroups::ConsentForm::Validations::WhenFormStepIsDateOfBirth "example at ./spec/models/consent_form_spec.rb:30">
There was a problem hiding this comment.
Looked into this ... looks like the most recent version of should-matchers, 5.3.0 from Dec 2022, doesn't include this matcher. This matcher was added earlier this year and hasn't been released yet. 🤦
tvararu
force-pushed
the
refactor-edit
branch
from
3c04d2b to
cacaf89
Compare
tvararu
commented
Sep 6, 2023
Comment on lines
+52
to
+56
| it do | ||
| should validate_inclusion_of(:is_this_their_school).in_array( | ||
| %w[yes no] | ||
| ).on(:update) | ||
| end |
There was a problem hiding this comment.
This is the linter's idea, not mine
misaka
approved these changes
Sep 6, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace the
on:context based validation with conditional validation that relies on specifying aform_step. This also makes the validations fire off for previous steps, and at the end for the finalrecordstep.Unifies the
*_paramsmethods in the EditController into oneupdate_paramsmethod.required_for_step?takes an optionalexactparameter, for transient steps like theschoolone that ask a question but don't actually save a field to the database (and shouldn't be re-validated on other steps).Overall approach based on:
https://www.joshmcarthur.com/2014/12/23/rails-multistep-forms.html