Skip to content

Replace static page document capture with mounted React application#3986

Merged
aduth merged 5 commits intomasterfrom
aduth-replace-static-doc-auth
Jul 29, 2020
Merged

Replace static page document capture with mounted React application#3986
aduth merged 5 commits intomasterfrom
aduth-replace-static-doc-auth

Conversation

@aduth
Copy link
Contributor

@aduth aduth commented Jul 28, 2020

Why: Static content is intended as a fallback in case e.g. JavaScript is disabled.

Up until now, the document capture screen has largely serves as a testing ground implementing both the new React-based application, alongside the static fallback form. This works well for incremental updates, but it poses some challenge in testing and demonstration of the intended flow. The changes introduced in this pull request seek to update the screen to render only the React application when available. It also removes some demonstration code (accordion) and moves some content into more appropriate steps of the flow (capture on selfie step).

In order to test the fallback flow, it will now be required to disable JavaScript while viewing the page. Alternatively, we could also expose either a configuration flag or query parameter that could be appended to control whether the React application is rendered.

For review, it may help to use "Hide whitespace changes" in the "Files changed" tab settings, since the bulk of the changes to document_capture.html.erb are indenting the contents within the <% if Figaro.env.acuant_sdk_document_capture_enabled == 'true' %> condition.

hide whitespace changes

Screen recording:

react-only-flow mov

@aduth aduth requested a review from solipet July 28, 2020 16:02
@zachmargolis
Copy link
Contributor

Looks like the acceptance specs that used the form fields are very cranky:

        Unable to find file field "doc_auth_front_image" that is not disabled

Do we need to progressively enhance the page rather than render a different one?

@aduth
Copy link
Contributor Author

aduth commented Jul 28, 2020

Looks like the acceptance specs that used the form fields are very cranky:

        Unable to find file field "doc_auth_front_image" that is not disabled

Do we need to progressively enhance the page rather than render a different one?

Could you show me where to find this, or how to recreate the error?

It's intended that this enhance the current combined server-rendered document capture form. In an environment where JavaScript is disabled, the form should still be shown. The React application will clear out the page contents and mount the application in its place, only if JavaScript is enabled.

For automated testing, I was under the impression that since the default behavior is to run without JavaScript, the server-rendered form fields should still be available for the tests to use.

Since I did move the form rendering within the <% if Figaro.env.acuant_sdk_document_capture_enabled == 'true' %> condition, maybe this could be the cause of the regression?

@aduth
Copy link
Contributor Author

aduth commented Jul 29, 2020

Could you show me where to find this, or how to recreate the error?

I chatted with @zachmargolis over Slack, and was able to discover the error details through CircleCI.

I'll plan to dig into it shortly, but my initial line of thinking for why this is an issue is one of:

  1. JS is enabled and the test should either not be running with JS enabled, or be updated to account for the new UI
  2. Environment variables which control whether the server-rendered form are displayed are different in the test environment

@aduth
Copy link
Contributor Author

aduth commented Jul 29, 2020

Environment variables which control whether the server-rendered form are displayed are different in the test environment

See changes in 086f464. I'm not actually sure that this environment variable even serves a purpose any longer, since as best I can tell, it's not used to alter the flow of any controller logic, but instead whether view contents are displayed. In the case of this test spec, since the purpose of the spec is to verify document capture, we can assume the environment variable should be set to 'true', as otherwise the fallback static form will not be displayed.

@zachmargolis
Copy link
Contributor

I'm not actually sure that this environment variable even serves a purpose any longer

I don't remember how we have the various fallbacks configured, but if you think it's time to remove the environment variable, let's go for it

From a quick check it's on in dev, int and staging, but off in prod

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

@aduth
Copy link
Contributor Author

aduth commented Jul 29, 2020

I don't remember how we have the various fallbacks configured, but if you think it's time to remove the environment variable, let's go for it

Its existence precedes me, and I'm not very clear if removing it would be detrimental in ways that aren't obvious to me. It appears that it could be removed, from what I can tell. I'll plan to manage this separately (LG-3249).

aduth added 5 commits July 29, 2020 17:16
**Why**: Static content is intended as a fallback in case e.g. JavaScript is disabled
**Why**: To create a more sensible flow where the second step involves selfie capture
**Why**: These should always be visible, and don't need to be controlled by React
**Why**: The purpose of the spec is to verify document capture flow, which is rendered only under the condition that the environment variable is assigned as `'true'`
@aduth aduth force-pushed the aduth-replace-static-doc-auth branch from 086f464 to aba2719 Compare July 29, 2020 21:19
@aduth aduth merged commit 44f509a into master Jul 29, 2020
@aduth aduth deleted the aduth-replace-static-doc-auth branch July 29, 2020 21:48
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.

2 participants