LG-13346 | FormObject best practices for how_to_verify by n1zyy · Pull Request #10682 · 18F/identity-idp

n1zyy · 2024-05-22T19:47:38Z

🎫 Ticket

Link to the relevant ticket:
LG-13346

🛠 Summary of changes

Applies some of the best practices described in this Google doc to bring this into line with how FormObject is used elsewhere.

There are two changes to behavior here:

Rather than just enforcing presence of the selection parameter, we validate that it's an accepted value. It should be impossible for this to happen without someone manipulating the form, though.
The error message is now actually displayed if the form is submitted without making a selection. Screenshot below.

👀 Screenshots

The red error condition is now shown if an invalid submission (somehow) occurs. It was previously not showing up.

Normalizes the way we're using FormObjects. changelog: Internal, FormObject normalization, Updates the HowToVerify flow

n1zyy · 2024-05-22T19:50:48Z

app/controllers/idv/how_to_verify_controller.rb

-
      analytics.idv_doc_auth_how_to_verify_visited(**analytics_arguments)
-      @idv_how_to_verify_form = Idv::HowToVerifyForm.new(selection: @selection)
+      @idv_how_to_verify_form = Idv::HowToVerifyForm.new


AFAICT this code was a no-op. It was just determining what was passed into the form's constructor, but I don't believe it influenced the form's display.

Ah so I think this is old code from when the form was a pair radio buttons rather than a set of buttons that submit the form itself. These values would set the radio button to the previous selection if the user went back in the flow. At least that's a guess.

I think that is accurate. We had a button that would submit for radio button selection. The view was refactored and I think think got missed.

n1zyy · 2024-05-22T19:53:26Z

app/controllers/idv/how_to_verify_controller.rb

      else
-        flash[:error] = result.first_error_message
-        redirect_to idv_how_to_verify_url
+        render :show, locals: { error: result.first_error_message }


By instantiating the form directly, we can just re-render the form.

This seems like a good idea from an analytics perspective as well, instead of sending folks back and getting a new event, we just get the single event from the first time they arrive. Which I think is closer to what we want to track 🤔

n1zyy · 2024-05-22T19:55:44Z

app/forms/idv/how_to_verify_form.rb

+    validates :selection, inclusion: {
+      in: [REMOTE, IPP],
+      message: proc { I18n.t('errors.doc_auth.how_to_verify_form') },
+    }


So it appears that inclusion does not imply presence: with only lines 15-18 here, a nil selection was apparently accepted. So I have both.

But we had presence already, did it still accept a nil selection?

presence worked as expected. I wanted to ensure we only accepted known values, so I had initially changed it from presence to inclusion, under the arguably-reasonable expectation that, since [REMOTE, IPP] does not include nil, it wouldn't be accepted. Turns out both are needed.

n1zyy · 2024-05-22T19:56:23Z

app/views/idv/how_to_verify/show.html.erb

+  <%= error %>
+  <% end %>
+<% end %>
+


I am not a UX whiz, but this allows us to conditionally render the error message which was previously not showing up.

I think this is fine. The semantics are handled by the AlertComponent itself, and you're doing something that looks like what other alert components are already doing. Is there a design to reference, though?

Oh, you're making me realize I should probably run this by UX explicitly. We were previously setting a flash message to this string, but it wasn't displaying. So I ✨ fixed the glitch ✨, but I should get UX buy-in on this change.

n1zyy · 2024-05-22T19:59:04Z

spec/controllers/idv/how_to_verify_controller_spec.rb

    end

+    context 'an invalid selection is submitted' do
+      # (This should only be possible if someone alters the form)


Do you object to me calling this out? The buttons just select "ipp" or "remote" as options, but a mischievous user could submit something weird. Or is it evident enough why we would care about invalid selections even if the user doesn't have the selections?

How is this different from the form submission error case that's handled in the spec starting on line 266? Could those two specs be merged into one? I think that might be cleaner?

Good catch.

My thinking at the time was that 266 tests when no selection is made, while this test is when an invalid selection is made, which wasn't previously handled.

But "invalid" is ambiguous wording used in both tests, and they should result in the same behavior.

Now the question is, do I make this a mini shared example or just duplicate it? 🤔

OK yeah, we have specific behavior expectations for no selection vs. some selection so now that you say I think we should maybe keep them separate. Mini shared example is my preference, since it's what we'd end up doing if this spec gets larger anyway, right?

n1zyy · 2024-05-22T19:59:56Z

spec/controllers/idv/how_to_verify_controller_spec.rb

+          step: 'how_to_verify',
+          analytics_id: 'Doc Auth',
+          skip_hybrid_handoff: nil,
+          'selection' => selection,


This key comes through as a string vs. symbol, which is gross. However, the other tests in here are doing the same thing, so it's... precedented grossness.

n1zyy · 2024-05-22T20:02:14Z

spec/controllers/idv/how_to_verify_controller_spec.rb

      end
    end
-
-    context 'undo/back' do


It seems like this is duplicative in intent of https://github.com/18F/identity-idp/blob/main/spec/controllers/idv/how_to_verify_controller_spec.rb#L171

n1zyy · 2024-05-22T20:06:18Z

spec/controllers/idv/how_to_verify_controller_spec.rb

-
-    context 'undo/back' do
-      it 'sets skip_doc_auth to nil and does not redirect' do
-        put :update, params: { undo_step: true }


This is doing an HTTP PUT with undo_step: true as a form parameter, which does not make sense to me. There is no apparent way for the user to do this, and no code in the controller to act on this—it appears that this is equivalent to what happens when you submit a blank form, which, again, we already test. So I removed this whole case.

Please let me know if I'm misinterpreting.

I am not sure why put vs calling undo_step.call with args. Aside from that, this test was to show that when you move backwards in the flow that some of the values on idv_session get set back to nil (the values before making any selection on this page.)

Goal was to testing the highlighted pieces below:

Ah-ha, this is helpful context; thanks!

I'm inclined to think that this provides equivalent coverage: https://github.com/18F/identity-idp/blob/main/spec/controllers/idv/how_to_verify_controller_spec.rb#L259-L266

I think the only way to get undo_step to fire are to fail the validation. Are you on board with this?

Ummm, so I think the form submission error that you link to is checking that those items are nil because they are not set yet, not because they are set back to nil on undo. In other words, that test is viewing it as if it is someone's first time hitting this page, not that they are moving backwards. The undo step only fires on a form submission, not when folks hit the back button in the browser. So it's only called when an update action happens on a controller.

I am not sure it captures it but think you can delete this test. I am not seeing tests for each step in our flow (address, ssn, etc.) that test undo_step but there is a spec testing undo_step so it is covered.

The bit I linked to is testing the update action. However, I went ahead and pulled all this out into a shared context which I think will make this all easier to follow.

Just waiting on ruby 3.3.1 to build because I pulled in main. 🤦‍♂️

That change is 58fa3b8 if that makes it easier to read.

n1zyy · 2024-05-22T20:07:19Z

spec/controllers/idv/how_to_verify_controller_spec.rb

      expect(subject.idv_session.skip_doc_auth).to be_nil
      expect(subject.idv_session.opted_in_to_in_person_proofing).to be_nil
-      expect(response).to redirect_to(idv_how_to_verify_url)
+      expect(response).to render_template :show


We now let the form handle this for us, so we no longer need to set a flash message, and we just re-render the form (show template) vs. redirecting.

Same comment as above: does it make sense to combine this test case with the 'an invalid selection is submitted' spec?

JackRyan1989

Looks good! Tested and works as expected. Just have some qs!

JackRyan1989 · 2024-05-22T20:51:27Z

app/controllers/idv/how_to_verify_controller.rb

-
      analytics.idv_doc_auth_how_to_verify_visited(**analytics_arguments)
-      @idv_how_to_verify_form = Idv::HowToVerifyForm.new(selection: @selection)
+      @idv_how_to_verify_form = Idv::HowToVerifyForm.new


Ah so I think this is old code from when the form was a pair radio buttons rather than a set of buttons that submit the form itself. These values would set the radio button to the previous selection if the user went back in the flow. At least that's a guess.

JackRyan1989 · 2024-05-22T20:53:48Z

app/controllers/idv/how_to_verify_controller.rb

      else
-        flash[:error] = result.first_error_message
-        redirect_to idv_how_to_verify_url
+        render :show, locals: { error: result.first_error_message }


This seems like a good idea from an analytics perspective as well, instead of sending folks back and getting a new event, we just get the single event from the first time they arrive. Which I think is closer to what we want to track 🤔

JackRyan1989 · 2024-05-22T20:56:51Z

app/forms/idv/how_to_verify_form.rb

+    validates :selection, inclusion: {
+      in: [REMOTE, IPP],
+      message: proc { I18n.t('errors.doc_auth.how_to_verify_form') },
+    }


But we had presence already, did it still accept a nil selection?

JackRyan1989 · 2024-05-23T12:57:59Z

app/views/idv/how_to_verify/show.html.erb

+  <%= error %>
+  <% end %>
+<% end %>
+


I think this is fine. The semantics are handled by the AlertComponent itself, and you're doing something that looks like what other alert components are already doing. Is there a design to reference, though?

JackRyan1989 · 2024-05-23T13:02:03Z

spec/controllers/idv/how_to_verify_controller_spec.rb

    end

+    context 'an invalid selection is submitted' do
+      # (This should only be possible if someone alters the form)


How is this different from the form submission error case that's handled in the spec starting on line 266? Could those two specs be merged into one? I think that might be cleaner?

JackRyan1989 · 2024-05-23T13:02:59Z

spec/controllers/idv/how_to_verify_controller_spec.rb

-
-    context 'undo/back' do
-      it 'sets skip_doc_auth to nil and does not redirect' do
-        put :update, params: { undo_step: true }


JackRyan1989 · 2024-05-23T13:03:50Z

spec/controllers/idv/how_to_verify_controller_spec.rb

      expect(subject.idv_session.skip_doc_auth).to be_nil
      expect(subject.idv_session.opted_in_to_in_person_proofing).to be_nil
-      expect(response).to redirect_to(idv_how_to_verify_url)
+      expect(response).to render_template :show


Same comment as above: does it make sense to combine this test case with the 'an invalid selection is submitted' spec?

JackRyan1989 · 2024-05-23T20:58:40Z

spec/controllers/idv/how_to_verify_controller_spec.rb

    end
    let(:analytics_name) { :idv_doc_auth_how_to_verify_submitted }

+    shared_examples_for 'invalid form submissions' do


👨🏻‍🍳 💋

JackRyan1989

I think you still need UX to check the alert but the code LGTM.

n1zyy added 2 commits May 22, 2024 15:34

LG-13346 | FormObject fix on HowToVerify

207d6b9

Normalizes the way we're using FormObjects. changelog: Internal, FormObject normalization, Updates the HowToVerify flow

Merge branch 'main' into mattw/LG-13346_formobject_how_to_verify

0530c66

n1zyy commented May 22, 2024

View reviewed changes

n1zyy marked this pull request as ready for review May 22, 2024 20:08

n1zyy requested review from a team, JackRyan1989 and gina-yamada May 22, 2024 20:08

lintfix

6730480

JackRyan1989 reviewed May 23, 2024

View reviewed changes

n1zyy added 2 commits May 23, 2024 15:51

Test cleanup

58fa3b8

Merge branch 'main' into mattw/LG-13346_formobject_how_to_verify

f7a7d8a

matthinz approved these changes May 23, 2024

View reviewed changes

JackRyan1989 reviewed May 23, 2024

View reviewed changes

JackRyan1989 approved these changes May 23, 2024

View reviewed changes

n1zyy merged commit da81567 into main May 24, 2024

n1zyy deleted the mattw/LG-13346_formobject_how_to_verify branch May 24, 2024 17:37

This was referenced May 28, 2024

Deploy RC 383 to Production #10702

Merged

Deploy RC 384 to Production #10714

Merged

Conversation

n1zyy commented May 22, 2024

🎫 Ticket

🛠 Summary of changes

👀 Screenshots

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1zyy May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

n1zyy May 22, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gina-yamada May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gina-yamada May 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JackRyan1989 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

n1zyy May 22, 2024 •

edited

Loading

n1zyy May 22, 2024 •

edited

Loading

gina-yamada May 23, 2024 •

edited

Loading

gina-yamada May 23, 2024 •

edited

Loading