LG-3204: Create Image Upload API by ironman5366 · Pull Request #3985 · 18F/identity-idp

ironman5366 · 2020-07-28T15:58:41Z

This creates an API endpoint that images can be asynchronously uploaded to, to fulfill the Document Capture step.

The API endpoint is at /api/verify/upload, and it accepts a POST containing the JSON attributes front, back, and selfie. In this first version, it returns just a status of either success or error, and a string message which is an explanation of that status.

# Conflicts: # app/services/doc_auth_mock/doc_auth_mock_client.rb # app/services/idv/steps/doc_auth_base_step.rb

.eslintrc

…ntity-idp into will-3204

app/javascript/app/document-capture/image_upload.js

app/javascript/app/document-capture/hooks/use-csrf.js

aduth · 2020-07-30T17:24:54Z

app/controllers/idv/image_upload_controller.rb

+    def error_json(reason)
+      {
+        status: 'error',
+        message: reason,
+      }
+    end


What status code are we returning in the error case? Is it still going to be 200 OK ?

Asking because it's something where we should probably want the status code to match the response status here, and in doing so, we could perform success / error handling on the basis of status code alone. i.e. we wouldn't need status at all, if the HTTP status code is enough of a signal.

app/controllers/idv/image_upload_controller.rb

aduth · 2020-08-06T13:04:42Z

I merged in master in 2bb4deb to bring the branch back up to date, resolving conflicts with some of the new logging changes (#4001, #4007), and to preemptively address an issue I'd encountered in starting to wire up the front-end to the new endpoint (#4013). I see some test failures related to the logging changes, which I'm unclear yet if are due to an incompatibility of the changes, or an error on my part in resolving the conflicts.

I pushed a few changes in ad271a0 and d6fda5f based on my prior feedback, incorporating the CSRF token management into the existing upload context, in a way that allows the upload implementation to receive the token as an argument automatically. d6fda5f replaces the stubbed upload implementation with a real one, using this new endpoint:

The form now sends and receives real data 🎉

app/javascript/app/document-capture/components/acuant-capture.jsx

@ironman5366

**Why**: @ironman5366 introduced this pattern in that PR. It is very helpful for moving to the API. It can stands on its own and cleans things up. I moved the changes to this PR to help decompose that one a bit.

@ironman5366

**Why**: @ironman5366 introduced this pattern in that PR. It is very helpful for moving to the API. It can stands on its own and cleans things up. I moved the changes to this PR to help decompose that one a bit.

**Why**: PropTypes were removed in favor of TypeScript-validated JSDoc See: #4043

aduth · 2020-08-12T14:13:00Z

app/controllers/idv/image_upload_controller.rb

+      nil
+    end
+
+    def check_content_type(request)


I marked my original comment as resolved, but flagging @zachmargolis's comment at #3985 (comment) for follow-up.

Also should we 415 for this? https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/415

**Why**: Type-checking for free

zachmargolis · 2020-08-12T16:20:24Z

app/services/idv/steps/document_capture_step.rb

+        api_upload = flow_session['api_upload']
+        response = (api_upload && api_upload['documents']) || post_form_images


We can take advantage of Hash#dig here I think:

Suggested change

api_upload = flow_session['api_upload']

response = (api_upload && api_upload['documents']) || post_form_images

response = flow_session.dig('api_upload', 'documents') || post_form_images

zachmargolis · 2020-08-12T16:21:29Z

app/services/idv/steps/document_capture_step.rb

+        selfie = selfie_image ? selfie_image.read : nil
+        post_images(front_image.read, back_image.read, selfie)


We can do the nil-safe operator (or .try but I think rubocop prefers nil-safe these days)

Suggested change

selfie = selfie_image ? selfie_image.read : nil

post_images(front_image.read, back_image.read, selfie)

post_images(front_image.read, back_image.read, selfie_image&.read)

zachmargolis · 2020-08-12T16:23:01Z

app/services/idv/steps/document_capture_step.rb

+      end
+
+      def handle_response(response)
+        if response.to_h[:success]


usually our response objects have .success?

Suggested change

if response.to_h[:success]

if response.success?

zachmargolis · 2020-08-12T16:25:55Z

spec/controllers/idv/image_upload_controller_spec.rb

+          'front': 'front_image',
+          'back': 'back_image',
+          'selfie': 'selfie_image',


you can drop the quotes on these keys, they're still symbols (and same for the others in this file)

Suggested change

'front': 'front_image',

'back': 'back_image',

'selfie': 'selfie_image',

front: 'front_image',

back: 'back_image',

selfie: 'selfie_image',

zachmargolis · 2020-08-12T16:27:41Z

app/controllers/idv/image_upload_controller.rb

+    def check_image_fields(request)
+      data = request.body.read
+      @front_image = data['front']
+      @back_image = data['back']
+      @selfie_image = data['selfie']
+      'Missing image keys' unless [@front_image, @back_image, @selfie_image].all?
+    end


This has a side effect of setting the instance variables, so I think the name check_ is a little misleading.... honestly, the keys are straightforward I would consider not even setting them as instance variables at all and just checking for their presence

zachmargolis · 2020-08-12T16:31:43Z

app/controllers/idv/image_upload_controller.rb

+      steps = %i[check_content_type check_image_fields]
+      steps.each do |step|
+        err = method(step).call(request)
+        return error_json(err) if err
+      end
+      nil


With only two steps.... I don't think this is a great use for this level of dynamic behavior, there are a few alternatives I would prefer (in order of most to least preferred)

Let's make a Form object -- we do this is many other controllers, make a form object that performs the validations and returns useful error structures the controllers know how to render (LMK if you need a more detailed example)

Move these into before actions, so that way they can just render and stop the chain (instead of having a loop to check for errors and stop)

before_action :check_content_type, only: [:upload] before_action :check_image_fields, only[:upload]

Just call the methods directly:

error = check_content_type(request) || check_image_fields(request) error_json(err) ir err

zachmargolis · 2020-08-12T16:33:42Z

config/routes.rb

  post '/api/sms/receive' => 'sms#receive'

+  # Image upload API for doc auth
+  post '/api/verify/upload' => 'idv/image_upload#upload'


Sometimes we try to stick to built-in rails method names/actions to be "RESTful"¹ as much as we can, WDYT of renaming this to #create?

I think "RESTful" is kind of a garbage term and can mean anything... in this case I'm just saying let's use the default names in rails for CRUD verbs

zachmargolis · 2020-08-13T19:25:11Z

@aduth this is starting to diverge from master branch a bunch... I think it's time to close this branch out and I was planning on opening a new PR with just the backend changes for image uploading, is that OK with you?

aduth · 2020-08-13T19:26:20Z

@aduth this is starting to diverge from master branch a bunch... I think it's time to close this branch out and I was planning on opening a new PR with just the backend changes for image uploading, is that OK with you?

Sure, that's fine with me 👍

ironman5366 and others added 9 commits July 20, 2020 16:50

WIP image upload API work

324af20

🔧 WIP fixes and testing code

ed40895

🔧 WIP additions and changes to image upload API

f9bf1c9

WIP improvements on image API structure and tie ins

1870b3a

Merge branch 'master' into will-3204

1e1ef4e

Merge branch 'master' into will-3204

a9a58e0

# Conflicts: # app/services/doc_auth_mock/doc_auth_mock_client.rb # app/services/idv/steps/doc_auth_base_step.rb

🔧 WIP integrations with new image API

ec7a44f

Start working on image specs and move liveness checking

519a058

Small fixes & corrections

0870a2d

ironman5366 added the status - work in progress label Jul 28, 2020

ironman5366 and others added 2 commits July 28, 2020 12:06

Linter fixes

f4abfbe

Retry tests

7fd72f6

aduth reviewed Jul 28, 2020

View reviewed changes

.eslintrc Outdated Show resolved Hide resolved

ironman5366 and others added 3 commits July 29, 2020 09:47

Merge branches 'master' and 'will-3204' of https://github.com/18F/ide…

80b6467

…ntity-idp into will-3204

Store the response better in the session

e5f3c9c

Spec & lint fixes

190ba85

ironman5366 added status - ready for review and removed status - work in progress labels Jul 29, 2020

ironman5366 requested a review from jmhooper July 29, 2020 16:23

Small 🐛 fix

fc6654c

ironman5366 marked this pull request as ready for review July 29, 2020 17:25

aduth reviewed Jul 30, 2020

View reviewed changes

aduth reviewed Aug 3, 2020

View reviewed changes

app/controllers/idv/image_upload_controller.rb Show resolved Hide resolved

aduth mentioned this pull request Aug 5, 2020

Represent document capture file state as data URL #4012

Merged

aduth added 3 commits August 6, 2020 08:23

Merge branch 'master' into will-3204

2bb4deb

Make CSRF available as value of UploadContext

ad271a0

Wire upload to send form values payload

d6fda5f

aduth reviewed Aug 6, 2020

View reviewed changes

app/javascript/app/document-capture/components/acuant-capture.jsx Outdated Show resolved Hide resolved

jmhooper removed the status - ready for review label Aug 7, 2020

jmhooper added the status - work in progress label Aug 7, 2020

Merge branch 'master' into will-3204

b220a13

aduth added 3 commits August 12, 2020 09:58

Merge branch 'master' into will-3204

55e1fcc

Refactor UploadContextProvider to remove propTypes

f8d26ed

**Why**: PropTypes were removed in favor of TypeScript-validated JSDoc See: #4043

Fix broken tests related to input key change

a3acc63

aduth reviewed Aug 12, 2020

View reviewed changes

Add type detail to upload implementation

de321a2

**Why**: Type-checking for free

zachmargolis reviewed Aug 12, 2020

View reviewed changes

zachmargolis closed this Aug 13, 2020

zachmargolis mentioned this pull request Aug 13, 2020

Add image upload API (LG-3204) #4069

Merged

aduth deleted the will-3204 branch August 18, 2020 12:04

		api_upload = flow_session['api_upload']
		response = (api_upload && api_upload['documents']) \|\| post_form_images

	api_upload = flow_session['api_upload']
	response = (api_upload && api_upload['documents']) \|\| post_form_images
	response = flow_session.dig('api_upload', 'documents') \|\| post_form_images

		selfie = selfie_image ? selfie_image.read : nil
		post_images(front_image.read, back_image.read, selfie)

Conversation

ironman5366 commented Jul 28, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aduth commented Aug 6, 2020

Uh oh!

Uh oh!

aduth Aug 12, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zachmargolis commented Aug 13, 2020

Uh oh!

aduth commented Aug 13, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aduth Aug 12, 2020 •

edited

Loading