Skip to content

Use form object pattern for Idv::AddressController submission#10388

Merged
jmhooper merged 3 commits intomainfrom
jmhooper-use-form-object-pattern-in-address-controller
Apr 9, 2024
Merged

Use form object pattern for Idv::AddressController submission#10388
jmhooper merged 3 commits intomainfrom
jmhooper-use-form-object-pattern-in-address-controller

Conversation

@jmhooper
Copy link
Contributor

@jmhooper jmhooper commented Apr 9, 2024

While I was working on #10385 I noticed that the Idv::AddressController includes a form object but does not implement the Form Object Pattern the way I expected. Specifically the form was only initialized and used to validate input params. As a result a few of the expected features were missing. Namely error messages did not appear on the rendered view. This commit adjusts Idv::AddressController to use the pattern as expected.

This commit also adds specs for Idv::AddressForm which I noticed were missing despite the form object having some complexity to account for the address_edited value in the FormResponse.

Now the form looks like this if you submit an error:
image

Previously this was not the case. The form would not render errors at all. This was not a huge issue since there was client side validation.

While I was working on #10385 I noticed that the `Idv::AddressController` includes a form object but does not implement the Form Object Pattern the way I expected. Specifically the form was only initialized and used to validate input params. As a result a few of the expected features were missing. Namely error messages did not appear on the rendered view. This commit adjusts `Idv::AddressController` to use the pattern as expected.

This commit also adds specs for `Idv::AddressForm` which I noticed were missing despite the form object having some complexity to account for the `address_edited` value in the `FormResponse`.

[skip changelog]
@jmhooper jmhooper requested a review from a team April 9, 2024 15:50
Comment on lines +46 to 51
ATTRIBUTES.each do |attribute_name|
if send(attribute_name).to_s != params[attribute_name].to_s
@address_edited = true
end
send(:"#{attribute_name}=", params[attribute_name].to_s)
end
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like this is very similar to set_ivars_with_pii except this sets @address_edited = true, do you think it would be worth trying to combine? or nah

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 made an effort to combine and struggled with a good way to do it well. I am going to be working on this again in #10385 and I'm hoping that it will be easier when we have a struct to represent an address because then we can just compare them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I'll be working on this in a follow-up to #10385 since #10385 is just for reads.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@zachmargolis: I just opened #10390 which includes these changes to the AddressForm: https://github.com/18F/identity-idp/pull/10390/files#diff-1b0c80e533f13f34a865f61b08ff023a3efbd21ba6bfa03aa84e0c3d0c108128

I got it out of the business of tracking address edits. My thinking is that once we stop overwriting the address in pii_from_doc we will get a fix for this bug: https://github.com/18F/identity-idp/pull/10385/files#diff-ee8ce27b8fbb65556458f217dd0017a6418194fd31087457ac79c219f750d337R130

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome!

@jmhooper jmhooper merged commit f495687 into main Apr 9, 2024
@jmhooper jmhooper deleted the jmhooper-use-form-object-pattern-in-address-controller branch April 9, 2024 17:34
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.

3 participants