Skip to content

Upload document capture images to S3 as PUT request#4401

Merged
aduth merged 2 commits intomasterfrom
aduth-s3-upload-put
Nov 13, 2020
Merged

Upload document capture images to S3 as PUT request#4401
aduth merged 2 commits intomasterfrom
aduth-s3-upload-put

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Nov 12, 2020

Why: Corrresponding to how the presigned URLs are configured, since it's expected that the user could need to change (replace) their selected image.

See: https://github.com/18F/identity-idp/blob/e1e8983/app/helpers/aws_s3_helper.rb#L8
Discussion: https://gsa-tts.slack.com/archives/CNCGEHG1G/p1605187899258500?thread_ts=1605046986.258200&cid=CNCGEHG1G

**Why**: Corrresponding to how the presigned URLs are configured, since it's expected that the user could need to change (replace) their selected image.

See: https://github.com/18F/identity-idp/blob/e1e8983/app/helpers/aws_s3_helper.rb#L8
@aduth aduth requested a review from solipet November 12, 2020 14:10
Copy link
Contributor

@solipet solipet left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM, one add-on request:

@zachmargolis
Copy link
Contributor

Can we also update the fake local version too? (Make it PUT and change to update as the action name)

Maybe we can update some acceptance specs to exercise this fake controller too? Unsure how big that is

@aduth
Copy link
Contributor Author

aduth commented Nov 12, 2020

Can we also update the fake local version too? (Make it PUT and change to update as the action name)

Yeah, good call. I'll update those.

Maybe we can update some acceptance specs to exercise this fake controller too? Unsure how big that is

Unsure off the top of my head, but I'll give it a shot, as it seems like a good thing to have.

@aduth
Copy link
Contributor Author

aduth commented Nov 13, 2020

Can we also update the fake local version too? (Make it PUT and change to update as the action name)

Updated in cd03568.

Maybe we can update some acceptance specs to exercise this fake controller too? Unsure how big that is

I've got some tests written in my local branch, though it turned out to be a big more of a rabbit hole than I expected, since we don't have much precedent for testing the document capture JavaScript in feature tests, and there's some basic setup required to resolve an issue with fetch not wanting to communicate with the test environment's domain_name (i.e. not actually the test environment's host name). While I've got it working, since it could do for some extra scrutiny, I'll plan to merge this as-is and create a quick follow-up pull request.

@aduth aduth merged commit 2617812 into master Nov 13, 2020
@aduth aduth deleted the aduth-s3-upload-put branch November 13, 2020 16:44
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