LG-3207: incorporate selfie in document capture step by solipet · Pull Request #3929 · 18F/identity-idp

solipet · 2020-07-16T02:29:21Z

As a user, I want to complete all of my doc_auth image uploads in one form so I can proof with a vendor that doesn't allow staggered uploads.

This PR adds the selfie upload to the document_capture_step. It's now one big long form with input fields to allow uploading the front/back of the DL and the selfie image.

anniehirshman-gsa · 2020-07-16T18:05:46Z

@solipet Including two mockups here with design suggestions: 1 with just my two biggest priority changes, and 1 with two more revisions if you have time:

At minimum:

Add 1px primary-light horizontal border between the three sections, with 2 rem (or spacing token: 4) space above and below
Update image tips in body text font style

If possible (if you don't have time to do this now, no worries - we can revisit later):

Add an h1 to the top of the page ("Upload your state-issued ID and a photo of you"), and update each section heading to h2
Remove illustrations to streamline the form

Let me know if you have any questions or if any of these are a pain. Thanks so much!

Minimum changes example:

Additional changes example:

app/forms/idv/document_capture_form.rb

app/services/acuant/acuant_client.rb

jmhooper · 2020-07-16T18:57:07Z

app/services/acuant/acuant_client.rb

+        selfie_response = post_selfie(image: selfie_image, instance_id: instance_id)
+        [selfie_response, pii]
+      else
+        results


Looks like this line is uncovered. I think it makes sense for us to drop in a unit test for what happens if posting the selfie fails.

solipet · 2020-07-16T20:59:10Z

@solipet Including two mockups here with design suggestions: 1 with just my two biggest priority changes, and 1 with two more revisions if you have time:

@anniehirshman-gsa Implemented all - here's a new screenshot:

anniehirshman-gsa · 2020-07-16T21:09:22Z

It looks perfect! Thank you @solipet!

app/forms/idv/document_capture_form.rb

…e step

zachmargolis · 2020-07-20T17:55:32Z

app/views/idv/doc_auth/document_capture.html.erb

this ===== looks like a merge error?

jmhooper · 2020-07-21T16:50:58Z

spec/services/acuant/acuant_client_spec.rb

+    let(:facial_match_url) { URI.join(Figaro.env.acuant_facial_match_url, '/api/v1/facematch') }
+    let(:liveness_url) { URI.join(Figaro.env.acuant_passlive_url, '/api/v1/liveness') }
+
+    it 'sends an upload image request for the front, back, and selfie images' do


Do we need to enable liveness for this spec? It looks like otherwise it ignores the selfie.

I noticed the gap in the coverage above. Maybe a test case for this w/ selfie enabled and selfie disabled?

jmhooper · 2020-07-21T16:51:11Z

spec/services/acuant/acuant_client_spec.rb

      )

-      result = subject.post_images(
+      result, _pii = subject.post_images(


This doesn't return PII anymore, right?

jmhooper · 2020-07-22T14:39:45Z

app/services/acuant/acuant_client.rb

+    end
+
+    # rubocop:disable Metrics/AbcSize
+    def post_images(front_image:, back_image:, selfie_image:,


I think we can drop the instance ID here, right?

jmhooper · 2020-07-22T14:40:53Z

config/locales/errors/es.yml

      token_missing: falta el token
    confirm_password_incorrect: La contraseña es incorrecta.
    doc_auth:
+      document_capture_info_html: |-


We need to remember to translate these before we merge

jmhooper · 2020-07-22T14:51:13Z

app/services/idv/steps/doc_auth_base_step.rb


+      # making a new method to avoid breaking the existing flow
+      def save_pii_in_session(pii)
+        flow_session[:pii_from_doc] = pii.merge(


Looks like this is uncovered, but I also can't find where it is used. I think if we pulled this out we'd fix our coverage woes.

aduth · 2020-07-23T20:04:25Z

app/views/idv/doc_auth/document_capture.html.erb

 <%= render 'idv/doc_auth/start_over_or_cancel' %>
 <%= javascript_pack_tag 'image-preview' %>
 </div>
+<%= javascript_pack_tag 'document-capture' %>


I think this may have been the artifact of a rebase after #3922 was merged. I don't think this change should be here. In #3922, it was moved higher in the file, and now there's two script tags that unfortunately conflict with one another 😬

Sneaking in a fix at #3964 (comment)

**Why**: So the React component renders in the page See: #3929 (comment)

* Disable prop type validation for test files **Why**: These restrictions are useful for runtime components, but unnecessary overhead for written tests. * Create stub React submission upload component flow **Why**: While the API endpoints for submitting the document upload form values does not yet exist, it can be abstracted as a function which receives a payload object and returns a Promise resolving or rejecting by the result of an attempt to upload. This implements Suspense-based asynchronous component handling, and introduces an optional upload context used in tests to replace the submission handler with a mock value. * Remove duplicate script pack tag **Why**: So the React component renders in the page See: #3929 (comment)

aduth · 2020-07-29T13:42:21Z

app/views/idv/doc_auth/document_capture.html.erb

+
+      <%= f.input :selfie_image_data_url, as: :hidden %>
+      <%= f.input :selfie_image, label: false, as: :file, required: true, wrapper_class: 'mt3 sm-col-8' %>
+      <div class='my2' id='selfie_target'>


Just noting that this tag is unclosed.

Suggested change

<div class='my2' id='selfie_target'>

<div class='my2' id='selfie_target'></div>

I encountered it in #3986. Not sure it's the cause of the errors there, but it was the source of some confusion in trying to keep the static rendered "Start over" and "Cancel" buttons, and them not showing 😅 I'll plan to include a fix for it with those changes.

solipet requested review from aduth, amathews-fs, anniehirshman-gsa and jmhooper July 16, 2020 02:29

solipet force-pushed the dprice-lg-3207-incorporate-selfie-in-document-capture-step branch from 97d20cf to 3a26dfe Compare July 16, 2020 02:53

jmhooper reviewed Jul 16, 2020

View reviewed changes

solipet commented Jul 17, 2020

View reviewed changes

app/forms/idv/document_capture_form.rb Outdated Show resolved Hide resolved

solipet force-pushed the dprice-lg-3207-incorporate-selfie-in-document-capture-step branch from 1d1e20e to 11479f4 Compare July 20, 2020 15:01

solipet added 6 commits July 20, 2020 13:42

LG-3207: Incorporate the selfie upload to the desktop document captur…

2377849

…e step

specs and tweaks

5076079

updating the design of the form

5a293c8

final layout tweaks, response wrapper for pii, conditionally run selfie

477229c

filter selfie unless required

1f331d7

English placeholders for es and fr locales

393c156

solipet force-pushed the dprice-lg-3207-incorporate-selfie-in-document-capture-step branch from d7afb03 to c7a78bb Compare July 20, 2020 17:45

zachmargolis reviewed Jul 20, 2020

View reviewed changes

rubocop

753d2fa

solipet force-pushed the dprice-lg-3207-incorporate-selfie-in-document-capture-step branch from c7a78bb to 753d2fa Compare July 20, 2020 18:00

jmhooper reviewed Jul 21, 2020

View reviewed changes

expanded specs for better coverage

4bc7b00

solipet force-pushed the dprice-lg-3207-incorporate-selfie-in-document-capture-step branch from 95c2c51 to 4bc7b00 Compare July 21, 2020 20:32

more spec coverage

3d5b1d4

solipet force-pushed the dprice-lg-3207-incorporate-selfie-in-document-capture-step branch from f1cc396 to 3d5b1d4 Compare July 22, 2020 03:19

more coverage

7116c4e

jmhooper approved these changes Jul 22, 2020

View reviewed changes

jmhooper reviewed Jul 22, 2020

View reviewed changes

solipet added 2 commits July 22, 2020 11:37

translations

d8c5194

remove method no longer needed

4a5f38b

jmhooper merged commit 2783655 into master Jul 22, 2020

jmhooper deleted the dprice-lg-3207-incorporate-selfie-in-document-capture-step branch July 22, 2020 15:52

aduth reviewed Jul 23, 2020

View reviewed changes

aduth added a commit that referenced this pull request Jul 23, 2020

Remove duplicate script pack tag

e370137

**Why**: So the React component renders in the page See: #3929 (comment)

aduth mentioned this pull request Jul 23, 2020

Create stub React submission upload component flow #3964

Merged

aduth reviewed Jul 29, 2020

View reviewed changes

aduth mentioned this pull request Nov 2, 2021

Remove unused document capture notice text #5574

Merged

	<div class='my2' id='selfie_target'>
	<div class='my2' id='selfie_target'></div>

Conversation

solipet commented Jul 16, 2020

Uh oh!

anniehirshman-gsa commented Jul 16, 2020

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

solipet commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

anniehirshman-gsa commented Jul 16, 2020

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

solipet commented Jul 16, 2020 •

edited

Loading