From 7650a8a08ff3dada8137921c62b5a0b898d57bca Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 26 Oct 2021 13:09:15 -0400 Subject: [PATCH 01/30] Fail JavaScript feature specs if any console messages are logged (#5492) * Configure global feature spec console log monitoring **Why**: So that it's less of a mystery when things break * Refactor message customization as custom class * Temporarily allow document-capture errors * Allow skipping expected browser console errors e.g. 404 page, or specs asserting against an error resposne * Only assert console logging on supported browser drivers NoMethodError: undefined method `manage' for # * Remove redundant visit Because (a) it's not necessary and (b) it makes console log catching angry because the page will prompt before navigating. * Fix path for email delete confirmation accessibility delete_email_path is for the POST submission confirming the deletion, so not something the user would "visit" directly * Increment reload times for JS-disabled doc polling scenario It was relying on the page refresh. Better to handle it in the test though, since it matches the test description * Rename allow_js_error to allow_browser_log Because it may not be originating from JS, and may not be an error * Always get logs Suspected that messages are being held between specs --- spec/features/accessibility/static_pages_spec.rb | 2 +- spec/features/accessibility/user_pages_spec.rb | 4 ++-- spec/features/idv/doc_auth/link_sent_step_spec.rb | 10 +--------- .../features/idv/doc_auth/test_credentials_spec.rb | 2 +- spec/rails_helper.rb | 14 ++++++++++++++ spec/support/browser_console_log_error.rb | 9 +++++++++ 6 files changed, 28 insertions(+), 13 deletions(-) create mode 100644 spec/support/browser_console_log_error.rb diff --git a/spec/features/accessibility/static_pages_spec.rb b/spec/features/accessibility/static_pages_spec.rb index 1245505e5b4..173f16502ed 100644 --- a/spec/features/accessibility/static_pages_spec.rb +++ b/spec/features/accessibility/static_pages_spec.rb @@ -2,7 +2,7 @@ require 'axe-rspec' feature 'Accessibility on static pages', :js do - scenario 'not found page' do + scenario 'not found page', allow_browser_log: true do visit '/non_existent_page' expect(page).to be_axe_clean.according_to :section508, :"best-practice", :wcag21aa diff --git a/spec/features/accessibility/user_pages_spec.rb b/spec/features/accessibility/user_pages_spec.rb index 89dfe1da276..d96d5120b57 100644 --- a/spec/features/accessibility/user_pages_spec.rb +++ b/spec/features/accessibility/user_pages_spec.rb @@ -110,11 +110,11 @@ expect(page).to be_uniquely_titled end - scenario 'edit email page' do + scenario 'delete email page' do user = create(:user) sign_in_and_2fa_user(user) - visit delete_email_path(id: user.email_addresses.take.id) + visit manage_email_confirm_delete_path(id: user.email_addresses.take.id) expect(page).to be_axe_clean.according_to :section508, :"best-practice", :wcag21aa expect(page).to label_required_fields diff --git a/spec/features/idv/doc_auth/link_sent_step_spec.rb b/spec/features/idv/doc_auth/link_sent_step_spec.rb index 43d79265e2c..49ee8eaed84 100644 --- a/spec/features/idv/doc_auth/link_sent_step_spec.rb +++ b/spec/features/idv/doc_auth/link_sent_step_spec.rb @@ -80,10 +80,6 @@ metadata[:js] = true let(:doc_capture_polling_enabled) { true } - before do - visit current_path - end - it 'automatically advances when the mobile flow is complete' do expect(page).to_not have_css 'meta[http-equiv="refresh"]', visible: false expect(page).to_not have_button(t('forms.buttons.continue')) @@ -100,10 +96,6 @@ shared_examples 'with doc capture polling disabled' do let(:doc_capture_polling_enabled) { false } - before do - visit current_path - end - context 'clicks back link' do before do click_doc_auth_back_link @@ -117,7 +109,7 @@ end it 'refreshes page 4x with meta refresh extending timeout by 40 min and can start over' do - 3.times do + 4.times do expect(page).to have_css 'meta[http-equiv="refresh"]', visible: false visit idv_doc_auth_link_sent_step end diff --git a/spec/features/idv/doc_auth/test_credentials_spec.rb b/spec/features/idv/doc_auth/test_credentials_spec.rb index 0158f409df6..a7c57d8818b 100644 --- a/spec/features/idv/doc_auth/test_credentials_spec.rb +++ b/spec/features/idv/doc_auth/test_credentials_spec.rb @@ -31,7 +31,7 @@ expect(page).to have_content('Jane') end - it 'triggers an error if the test credentials have a friendly error' do + it 'triggers an error if the test credentials have a friendly error', allow_browser_log: true do complete_doc_auth_steps_before_document_capture_step attach_file( diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index a7c2bd0ebfe..57158ef004e 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -105,4 +105,18 @@ class Analytics example.run Bullet.enable = false end + + config.after(:each, type: :feature, js: true) do |spec| + next unless page.driver.browser.respond_to?(:manage) + + # Always get the logs, even if logs are allowed for the spec, since otherwise unexpected + # messages bleed over between specs. + javascript_errors = page.driver.browser.manage.logs.get(:browser).map(&:message) + next if spec.metadata[:allow_browser_log] + + # Temporarily allow for document-capture bundle, since it uses React error boundaries to poll. + javascript_errors.reject! { |e| e.include? 'js/document-capture-' } + # Consider any browser console logging as a failure. + raise BrowserConsoleLogError.new(javascript_errors) if javascript_errors.present? + end end diff --git a/spec/support/browser_console_log_error.rb b/spec/support/browser_console_log_error.rb new file mode 100644 index 00000000000..66ac644cf52 --- /dev/null +++ b/spec/support/browser_console_log_error.rb @@ -0,0 +1,9 @@ +class BrowserConsoleLogError < StandardError + def initialize(messages) + @messages = messages + end + + def to_s + "Unexpected browser console logging:\n\n#{@messages.join("\n\n")}" + end +end From aa3b6624d6c7569765425c55785bed8203cf37d6 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 26 Oct 2021 13:09:55 -0400 Subject: [PATCH 02/30] Refactor document capture provider hierarchy via component composition (#5535) * Refactor document capture provider hierarchy via component composition **Why**: Flattening results in diffs which aren't as unyieldy when adding or removing from the component hierarchy. * Cast data attributes as string **Why**: Avoid verbose equivalent TypeScript casting * Use variadic arguments for composeComponents See: https://github.com/18F/identity-idp/pull/5535#discussion_r734803962 * Use implicit undefined for component pair props **Why**: Destructures the same, avoids explicit reference to undefined (which by definition is more on the implicit end "absence of" a defined value, vs. explicit empty null), smaller bundled size. --- .../packages/components/package.json | 2 +- .../packages/compose-components/README.md | 17 +++++ .../packages/compose-components/index.js | 51 +++++++++++++ .../compose-components/index.spec.jsx | 27 +++++++ .../packages/compose-components/package.json | 8 +++ .../packages/document-capture/package.json | 4 +- .../packages/react-i18n/package.json | 2 +- app/javascript/packs/document-capture.jsx | 71 +++++++++---------- package.json | 6 +- yarn.lock | 46 ++++++------ 10 files changed, 168 insertions(+), 66 deletions(-) create mode 100644 app/javascript/packages/compose-components/README.md create mode 100644 app/javascript/packages/compose-components/index.js create mode 100644 app/javascript/packages/compose-components/index.spec.jsx create mode 100644 app/javascript/packages/compose-components/package.json diff --git a/app/javascript/packages/components/package.json b/app/javascript/packages/components/package.json index c3d276925b1..400ac567672 100644 --- a/app/javascript/packages/components/package.json +++ b/app/javascript/packages/components/package.json @@ -3,6 +3,6 @@ "private": true, "version": "1.0.0", "dependencies": { - "react": "^17.0.1" + "react": "^17.0.2" } } diff --git a/app/javascript/packages/compose-components/README.md b/app/javascript/packages/compose-components/README.md new file mode 100644 index 00000000000..fec7f76731e --- /dev/null +++ b/app/javascript/packages/compose-components/README.md @@ -0,0 +1,17 @@ +# `@18f/identity-compose-components` + +A utility function to compose a set of React components and their props to a single component. + +Convenient for flattening a deeply-nested arrangement of context providers, for example. + +## Example + +```jsx +const App = composeComponents( + [FirstContext.Provider, { value: 1 }], + [SecondContext.Provider, { value: 2 }], + AppRoot, +); + +render(App, document.getElementById('app-root')); +``` diff --git a/app/javascript/packages/compose-components/index.js b/app/javascript/packages/compose-components/index.js new file mode 100644 index 00000000000..d152e389d7f --- /dev/null +++ b/app/javascript/packages/compose-components/index.js @@ -0,0 +1,51 @@ +import { createElement } from 'react'; + +/** @typedef {import('react').ComponentType

} ComponentType @template P */ + +/** + * @typedef {[ComponentType

, P]} NormalizedComponentPair + * + * @template P + */ + +/** + * @typedef {[ComponentType

, P]|[ComponentType

]|ComponentType

} ComponentPair + * + * @template P + */ + +/** + * A utility function to compose a set of React components and their props to a single component. + * + * Convenient for flattening a deeply-nested arrangement of context providers, for example. + * + * @example + * ```jsx + * const App = composeComponents( + * [FirstContext.Provider, { value: 1 }], + * [SecondContext.Provider, { value: 2 }], + * AppRoot, + * ); + * + * render(App, document.getElementById('app-root')); + * ``` + * + * @param {...ComponentPair<*>} components + * + * @return {ComponentType<*>} + */ +export function composeComponents(...components) { + return function ComposedComponent() { + /** @type {JSX.Element?} */ + let element = null; + for (let i = components.length - 1; i >= 0; i--) { + const componentPair = /** @type {NormalizedComponentPair<*>} */ (Array.isArray(components[i]) + ? components[i] + : [components[i]]); + const [ComponentType, props] = componentPair; + element = createElement(ComponentType, props, element); + } + + return element; + }; +} diff --git a/app/javascript/packages/compose-components/index.spec.jsx b/app/javascript/packages/compose-components/index.spec.jsx new file mode 100644 index 00000000000..7e1e296e7c0 --- /dev/null +++ b/app/javascript/packages/compose-components/index.spec.jsx @@ -0,0 +1,27 @@ +import { createContext, useContext } from 'react'; +import { render } from '@testing-library/react'; +import { composeComponents } from './index.js'; + +describe('composeComponents', () => { + it('composes components', () => { + const FirstContext = createContext(null); + const SecondContext = createContext(null); + const AppRoot = () => ( + <> + {useContext(FirstContext)} + {useContext(SecondContext)} + + ); + + const ComposedComponent = composeComponents( + [FirstContext.Provider, { value: 1 }], + [SecondContext.Provider, { value: 2 }], + [({ children }) => <>{children}3], + AppRoot, + ); + + const { getByText } = render(); + + expect(getByText('123')).to.be.ok(); + }); +}); diff --git a/app/javascript/packages/compose-components/package.json b/app/javascript/packages/compose-components/package.json new file mode 100644 index 00000000000..8ec82d11652 --- /dev/null +++ b/app/javascript/packages/compose-components/package.json @@ -0,0 +1,8 @@ +{ + "name": "@18f/identity-compose-components", + "private": true, + "version": "1.0.0", + "dependencies": { + "react": "^17.0.2" + } +} diff --git a/app/javascript/packages/document-capture/package.json b/app/javascript/packages/document-capture/package.json index 7c522b3f53a..393b64ed7ed 100644 --- a/app/javascript/packages/document-capture/package.json +++ b/app/javascript/packages/document-capture/package.json @@ -4,7 +4,7 @@ "version": "1.0.0", "dependencies": { "focus-trap": "^6.2.3", - "react": "^17.0.1", - "react-dom": "^17.0.1" + "react": "^17.0.2", + "react-dom": "^17.0.2" } } diff --git a/app/javascript/packages/react-i18n/package.json b/app/javascript/packages/react-i18n/package.json index 0906ab47a9f..75371b668c6 100644 --- a/app/javascript/packages/react-i18n/package.json +++ b/app/javascript/packages/react-i18n/package.json @@ -3,6 +3,6 @@ "private": true, "version": "1.0.0", "dependencies": { - "react": "^17.0.1" + "react": "^17.0.2" } } diff --git a/app/javascript/packs/document-capture.jsx b/app/javascript/packs/document-capture.jsx index d2d16028432..31fb9617b91 100644 --- a/app/javascript/packs/document-capture.jsx +++ b/app/javascript/packs/document-capture.jsx @@ -1,4 +1,5 @@ import { render } from 'react-dom'; +import { composeComponents } from '@18f/identity-compose-components'; import { AppContext, DocumentCapture, @@ -138,41 +139,39 @@ loadPolyfills(['fetch', 'crypto', 'url']).then(async () => { appName: /** @type string */ (appRoot.dataset.appName), }; - render( - - - - - - - - - - - - - - - - - , - appRoot, + const App = composeComponents( + [AppContext.Provider, { value: appContext }], + [DeviceContext.Provider, { value: device }], + [AnalyticsContext.Provider, { value: { addPageAction, noticeError } }], + [ + AcuantContextProvider, + { + credentials: getMetaContent('acuant-sdk-initialization-creds'), + endpoint: getMetaContent('acuant-sdk-initialization-endpoint'), + glareThreshold, + sharpnessThreshold, + }, + ], + [ + UploadContextProvider, + { + endpoint: String(appRoot.getAttribute('data-endpoint')), + statusEndpoint: String(appRoot.getAttribute('data-status-endpoint')), + statusPollInterval: + Number(appRoot.getAttribute('data-status-poll-interval-ms')) || undefined, + method: isAsyncForm ? 'PUT' : 'POST', + csrf, + isMockClient, + backgroundUploadURLs, + backgroundUploadEncryptKey, + formData, + }, + ], + [I18nContext.Provider, { value: i18n.strings }], + [ServiceProviderContextProvider, { value: getServiceProvider() }], + [AssetContext.Provider, { value: assets }], + [DocumentCapture, { isAsyncForm, onStepChange: keepAlive }], ); + + render(, appRoot); }); diff --git a/package.json b/package.json index 09a647e654e..29095cb14e8 100644 --- a/package.json +++ b/package.json @@ -32,8 +32,8 @@ "identity-style-guide": "^6.2.0", "intl-tel-input": "^17.0.8", "postcss-clean": "^1.1.0", - "react": "^17.0.1", - "react-dom": "^17.0.1", + "react": "^17.0.2", + "react-dom": "^17.0.2", "source-map-loader": "^1.1.3", "zxcvbn": "4.4.2" }, @@ -57,7 +57,7 @@ "mocha": "^8.2.1", "mq-polyfill": "^1.1.8", "prettier": "^2.2.1", - "react-test-renderer": "^17.0.1", + "react-test-renderer": "^17.0.2", "sinon": "^9.2.2", "sinon-chai": "^3.5.0", "svgo": "^1.3.2", diff --git a/yarn.lock b/yarn.lock index e2424b4ee88..80049cda8ee 100644 --- a/yarn.lock +++ b/yarn.lock @@ -7642,19 +7642,19 @@ raw-body@2.4.0: iconv-lite "0.4.24" unpipe "1.0.0" -react-dom@^17.0.1: - version "17.0.1" - resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-17.0.1.tgz#1de2560474ec9f0e334285662ede52dbc5426fc6" - integrity sha512-6eV150oJZ9U2t9svnsspTMrWNyHc6chX0KzDeAOXftRa8bNeOKTTfCJ7KorIwenkHd2xqVTBTCZd79yk/lx/Ug== +react-dom@^17.0.2: + version "17.0.2" + resolved "https://registry.yarnpkg.com/react-dom/-/react-dom-17.0.2.tgz#ecffb6845e3ad8dbfcdc498f0d0a939736502c23" + integrity sha512-s4h96KtLDUQlsENhMn1ar8t2bEa+q/YAtj8pPPdIjPDGBDIVNsrD9aXNWqspUe6AzKCIG0C1HZZLqLV7qpOBGA== dependencies: loose-envify "^1.1.0" object-assign "^4.1.1" - scheduler "^0.20.1" + scheduler "^0.20.2" -"react-is@^16.12.0 || ^17.0.0", react-is@^17.0.1: - version "17.0.1" - resolved "https://registry.yarnpkg.com/react-is/-/react-is-17.0.1.tgz#5b3531bd76a645a4c9fb6e693ed36419e3301339" - integrity sha512-NAnt2iGDXohE5LI7uBnLnqvLQMtzhkiAOLXTmv+qnF9Ky7xAPcX8Up/xWIhxvLVGJvuLiNc4xQLtuqDRzb4fSA== +"react-is@^16.12.0 || ^17.0.0", react-is@^17.0.1, react-is@^17.0.2: + version "17.0.2" + resolved "https://registry.yarnpkg.com/react-is/-/react-is-17.0.2.tgz#e691d4a8e9c789365655539ab372762b0efb54f0" + integrity sha512-w2GsyukL62IJnlaff/nRegPQR94C/XXamvMWmSHRJ4y7Ts/4ocGRmTHvOs8PSE6pB3dWOrD/nueuU5sduBsQ4w== react-is@^16.8.1: version "16.13.1" @@ -7669,20 +7669,20 @@ react-shallow-renderer@^16.13.1: object-assign "^4.1.1" react-is "^16.12.0 || ^17.0.0" -react-test-renderer@^17.0.1: - version "17.0.1" - resolved "https://registry.yarnpkg.com/react-test-renderer/-/react-test-renderer-17.0.1.tgz#3187e636c3063e6ae498aedf21ecf972721574c7" - integrity sha512-/dRae3mj6aObwkjCcxZPlxDFh73XZLgvwhhyON2haZGUEhiaY5EjfAdw+d/rQmlcFwdTpMXCSGVk374QbCTlrA== +react-test-renderer@^17.0.2: + version "17.0.2" + resolved "https://registry.yarnpkg.com/react-test-renderer/-/react-test-renderer-17.0.2.tgz#4cd4ae5ef1ad5670fc0ef776e8cc7e1231d9866c" + integrity sha512-yaQ9cB89c17PUb0x6UfWRs7kQCorVdHlutU1boVPEsB8IDZH6n9tHxMacc3y0JoXOJUsZb/t/Mb8FUWMKaM7iQ== dependencies: object-assign "^4.1.1" - react-is "^17.0.1" + react-is "^17.0.2" react-shallow-renderer "^16.13.1" - scheduler "^0.20.1" + scheduler "^0.20.2" -react@^17.0.1: - version "17.0.1" - resolved "https://registry.yarnpkg.com/react/-/react-17.0.1.tgz#6e0600416bd57574e3f86d92edba3d9008726127" - integrity sha512-lG9c9UuMHdcAexXtigOZLX8exLWkW0Ku29qPRU8uhF2R9BN96dLCt0psvzPLlHc5OWkgymP3qwTRgbnw5BKx3w== +react@^17.0.2: + version "17.0.2" + resolved "https://registry.yarnpkg.com/react/-/react-17.0.2.tgz#d0b5cc516d29eb3eee383f75b62864cfb6800037" + integrity sha512-gnhPt75i/dq/z3/6q/0asP78D0u592D5L1pd7M8P+dck6Fu/jJeL6iVVK23fptSUZj8Vjf++7wXA8UNclGQcbA== dependencies: loose-envify "^1.1.0" object-assign "^4.1.1" @@ -8067,10 +8067,10 @@ saxes@^5.0.0: dependencies: xmlchars "^2.2.0" -scheduler@^0.20.1: - version "0.20.1" - resolved "https://registry.yarnpkg.com/scheduler/-/scheduler-0.20.1.tgz#da0b907e24026b01181ecbc75efdc7f27b5a000c" - integrity sha512-LKTe+2xNJBNxu/QhHvDR14wUXHRQbVY5ZOYpOGWRzhydZUqrLb2JBvLPY7cAqFmqrWuDED0Mjk7013SZiOz6Bw== +scheduler@^0.20.2: + version "0.20.2" + resolved "https://registry.yarnpkg.com/scheduler/-/scheduler-0.20.2.tgz#4baee39436e34aa93b4874bddcbf0fe8b8b50e91" + integrity sha512-2eWfGgAqqWFGqtdMmcL5zCMK1U8KlXv8SQFGglL3CEtd0aDVDWgeF/YoCmvln55m5zSk3J/20hTaSBeSObsQDQ== dependencies: loose-envify "^1.1.0" object-assign "^4.1.1" From a8133c7b8f9acbbbc7b7d208ec7ee7936a79841c Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 26 Oct 2021 15:08:02 -0400 Subject: [PATCH 03/30] Resolve lint errors on MFA setup view (#5548) **Why**: Incidentally supporting related work in LG-5185, isolated to limit size of pull request of that effort. --- .erb-lint.yml | 1 - .../two_factor_authentication_setup/index.html.erb | 12 +++++++----- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/.erb-lint.yml b/.erb-lint.yml index efe20705430..dfc4220a9da 100644 --- a/.erb-lint.yml +++ b/.erb-lint.yml @@ -57,7 +57,6 @@ linters: - '*/app/views/users/shared/_otp_delivery_preference_selection.html.erb' - '*/app/views/users/shared/_otp_make_default_number.html.erb' - '*/app/views/users/totp_setup/*' - - '*/app/views/users/two_factor_authentication_setup/*' - '*/app/views/users/verify_password/*' - '*/app/views/users/verify_personal_key/*' - '*/app/views/users/webauthn_setup/*' diff --git a/app/views/users/two_factor_authentication_setup/index.html.erb b/app/views/users/two_factor_authentication_setup/index.html.erb index 8228694aa1d..f4569619c81 100644 --- a/app/views/users/two_factor_authentication_setup/index.html.erb +++ b/app/views/users/two_factor_authentication_setup/index.html.erb @@ -1,9 +1,11 @@ <% title t('titles.two_factor_setup') %> <% if @presenter.icon %> - <%= image_tag(asset_url(@presenter.icon), - class: 'margin-bottom-3', - alt: @presenter.icon_alt_text) %> + <%= image_tag( + asset_url(@presenter.icon), + class: 'margin-bottom-3', + alt: @presenter.icon_alt_text, + ) %> <% end %>

<%= @presenter.heading %>

@@ -29,7 +31,7 @@ ) %> <%= label_tag( "two_factor_options_form_selection_#{option.type}", - class: "usa-radio__label", + class: 'usa-radio__label', ) do %> <%= option.label %> @@ -53,4 +55,4 @@ <%= render 'shared/cancel', link: destroy_user_session_path %> -<%= javascript_packs_tag_once("webauthn-unhide-signup") %> +<%= javascript_packs_tag_once('webauthn-unhide-signup') %> From c1f61537e8a9892dee5590067979e2dbbae3cc6b Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 26 Oct 2021 15:09:59 -0400 Subject: [PATCH 04/30] LG-5245: Track drag-and-drop file input as click interaction (#5545) **Why**: To be able to accurately track interaction with document capture fields, we want to be able to assume that a "click" precedes image selection. Includes a new property on the event to allow for differentiation between whether event occurs by drag/traditional click. --- .../components/acuant-capture.jsx | 11 ++++++-- .../components/file-input.jsx | 6 ++++- .../components/acuant-capture-spec.jsx | 25 +++++++++++++++++++ .../components/file-input-spec.jsx | 20 +++++++++++++++ 4 files changed, 59 insertions(+), 3 deletions(-) diff --git a/app/javascript/packages/document-capture/components/acuant-capture.jsx b/app/javascript/packages/document-capture/components/acuant-capture.jsx index 54dddca2561..43f6a51dbf4 100644 --- a/app/javascript/packages/document-capture/components/acuant-capture.jsx +++ b/app/javascript/packages/document-capture/components/acuant-capture.jsx @@ -102,6 +102,11 @@ import './acuant-capture.scss'; * @prop {string} name Prefix to prepend to user action analytics labels. */ +/** + * A noop function. + */ +const noop = () => {}; + /** * Returns true if the given Acuant capture failure was caused by the user declining access to the * camera, or false otherwise. @@ -339,15 +344,16 @@ function AcuantCapture( * @template {(...args: any[]) => any} T * * @param {string} source Click source. + * @param {{isDrop: boolean}=} metadata Additional payload metadata to log. * * @return {(fn: T) => (...args: Parameters) => ReturnType} */ - function withLoggedClick(source) { + function withLoggedClick(source, metadata = { isDrop: false }) { return (fn) => (...args) => { if (!isSuppressingClickLogging.current) { addPageAction({ label: `IdV: ${name} image clicked`, - payload: { source }, + payload: { source, ...metadata }, }); } @@ -521,6 +527,7 @@ function AcuantCapture( value={value} errorMessage={ownErrorMessage ?? errorMessage} onClick={withLoggedClick('placeholder')(startCaptureOrTriggerUpload)} + onDrop={withLoggedClick('placeholder', { isDrop: true })(noop)} onChange={onUpload} onError={() => setOwnErrorMessage(null)} /> diff --git a/app/javascript/packages/document-capture/components/file-input.jsx b/app/javascript/packages/document-capture/components/file-input.jsx index 00a13acdd5d..97f7388998a 100644 --- a/app/javascript/packages/document-capture/components/file-input.jsx +++ b/app/javascript/packages/document-capture/components/file-input.jsx @@ -14,6 +14,7 @@ import useInstanceId from '../hooks/use-instance-id'; import usePrevious from '../hooks/use-previous'; /** @typedef {import('react').MouseEvent} ReactMouseEvent */ +/** @typedef {import('react').DragEvent} ReactDragEvent */ /** @typedef {import('react').ChangeEvent} ReactChangeEvent */ /** @typedef {import('react').RefAttributes} ReactRefAttributes */ /** @typedef {import('react').ReactNode} ReactNode */ @@ -31,6 +32,7 @@ import usePrevious from '../hooks/use-previous'; * @prop {Blob|string|null|undefined} value Current value. * @prop {ReactNode=} errorMessage Error to show. * @prop {(event:ReactMouseEvent)=>void=} onClick Input click handler. + * @prop {(event:ReactDragEvent)=>void=} onDrop Input drop handler. * @prop {(nextValue:File?)=>void=} onChange Input change handler. * @prop {(message:ReactNode)=>void=} onError Callback to trigger if upload error occurs. */ @@ -108,7 +110,8 @@ function FileInput(props, ref) { capture, value, errorMessage, - onClick = () => {}, + onClick, + onDrop, onChange = () => {}, onError = () => {}, } = props; @@ -278,6 +281,7 @@ function FileInput(props, ref) { onChange={onChangeIfValid} capture={capture} onClick={onClick} + onDrop={onDrop} accept={accept ? accept.join() : undefined} aria-describedby={hint ? hintId : undefined} /> diff --git a/spec/javascripts/packages/document-capture/components/acuant-capture-spec.jsx b/spec/javascripts/packages/document-capture/components/acuant-capture-spec.jsx index 743e1843402..1248f3b3d4b 100644 --- a/spec/javascripts/packages/document-capture/components/acuant-capture-spec.jsx +++ b/spec/javascripts/packages/document-capture/components/acuant-capture-spec.jsx @@ -867,18 +867,43 @@ describe('document-capture/components/acuant-capture', () => { label: 'IdV: test image clicked', payload: { source: 'placeholder', + isDrop: false, }, }); expect(addPageAction.getCall(1)).to.have.been.calledWith({ label: 'IdV: test image clicked', payload: { source: 'button', + isDrop: false, }, }); expect(addPageAction.getCall(2)).to.have.been.calledWith({ label: 'IdV: test image clicked', payload: { source: 'upload', + isDrop: false, + }, + }); + }); + + it('logs drag-and-drop as click interaction', () => { + const addPageAction = sinon.stub(); + const { getByLabelText } = render( + + + + + , + ); + + const input = getByLabelText('Image'); + fireEvent.drop(input); + + expect(addPageAction.getCall(0)).to.have.been.calledWith({ + label: 'IdV: test image clicked', + payload: { + source: 'placeholder', + isDrop: true, }, }); }); diff --git a/spec/javascripts/packages/document-capture/components/file-input-spec.jsx b/spec/javascripts/packages/document-capture/components/file-input-spec.jsx index 0df7062fd26..61fa91b988e 100644 --- a/spec/javascripts/packages/document-capture/components/file-input-spec.jsx +++ b/spec/javascripts/packages/document-capture/components/file-input-spec.jsx @@ -232,6 +232,26 @@ describe('document-capture/components/file-input', () => { expect(onChange.getCall(0).args[0]).to.equal(file); }); + it('calls onClick when clicked', () => { + const onClick = sinon.stub(); + const { getByLabelText } = render(); + + const input = getByLabelText('File'); + userEvent.click(input); + + expect(onClick).to.have.been.calledOnce(); + }); + + it('calls onDrop when receiving drop event', () => { + const onDrop = sinon.stub(); + const { getByLabelText } = render(); + + const input = getByLabelText('File'); + fireEvent.drop(input); + + expect(onDrop).to.have.been.calledOnce(); + }); + it('allows changing the selected value', () => { const file2 = new window.File([file], 'file2.jpg'); const onChange = sinon.stub(); From 6adb02a8248fba7069f96930dc99c1ed6605ba45 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 26 Oct 2021 15:52:11 -0400 Subject: [PATCH 05/30] LG-5079: Create separate throttle for address verification (#5546) * LG-5079: Create separate throttle for address verification **Why**: As a user, I expect that if it takes me several attempts to complete the "Verify your information" verification during IAL2, my remaining attempts do not carry over subsequent into the phone/address verification, so that I have adequate opportunity to try to complete this step independent of my previous difficulty. * Restore and improve spec for proof_ssn It wasn't working as expected, because the created throttle wasn't actually throttled. The controller will always assign a time for expires_at, even if that time is 'now' --- app/controllers/idv/gpo_controller.rb | 4 ++-- app/controllers/idv/phone_controller.rb | 8 ++++++-- app/controllers/idv/phone_errors_controller.rb | 2 +- app/models/throttle.rb | 5 +++++ app/services/idv/phone_step.rb | 2 +- config/application.yml.default | 2 ++ lib/identity_config.rb | 2 ++ spec/controllers/idv/phone_controller_spec.rb | 10 +++++----- spec/controllers/idv/phone_errors_controller_spec.rb | 8 ++++---- .../controllers/idv/session_errors_controller_spec.rb | 11 ++++++++--- spec/features/idv/doc_auth/verify_step_spec.rb | 2 +- spec/services/idv/phone_step_spec.rb | 6 +++--- spec/support/features/idv_helper.rb | 8 -------- spec/support/idv_examples/max_attempts.rb | 4 ++-- 14 files changed, 42 insertions(+), 32 deletions(-) diff --git a/app/controllers/idv/gpo_controller.rb b/app/controllers/idv/gpo_controller.rb index 94bf7bdf992..4ba153f68cd 100644 --- a/app/controllers/idv/gpo_controller.rb +++ b/app/controllers/idv/gpo_controller.rb @@ -145,7 +145,7 @@ def form_response(result, success) def idv_throttle_params { user: idv_session.current_user, - throttle_type: :idv_resolution, + throttle_type: :proof_address, } end @@ -171,7 +171,7 @@ def max_attempts_reached if idv_attempter_throttled? analytics.track_event( Analytics::THROTTLER_RATE_LIMIT_TRIGGERED, - throttle_type: :idv_resolution, + throttle_type: :proof_address, step_name: :gpo, ) flash_error diff --git a/app/controllers/idv/phone_controller.rb b/app/controllers/idv/phone_controller.rb index 2b88aed7e02..e53ad15e7b2 100644 --- a/app/controllers/idv/phone_controller.rb +++ b/app/controllers/idv/phone_controller.rb @@ -8,7 +8,7 @@ class PhoneController < ApplicationController before_action :set_idv_form def new - redirect_to failure_url(:fail) and return if idv_attempter_throttled? + redirect_to failure_url(:fail) and return if throttle.throttled? async_state = step.async_state if async_state.none? @@ -35,10 +35,14 @@ def create private + def throttle + @throttle ||= Throttle.for(user: current_user, throttle_type: :proof_address) + end + def max_attempts_reached analytics.track_event( Analytics::THROTTLER_RATE_LIMIT_TRIGGERED, - throttle_type: :idv_resolution, + throttle_type: :proof_address, step_name: step_name, ) end diff --git a/app/controllers/idv/phone_errors_controller.rb b/app/controllers/idv/phone_errors_controller.rb index bfa8ec84ba2..0551ab57e7e 100644 --- a/app/controllers/idv/phone_errors_controller.rb +++ b/app/controllers/idv/phone_errors_controller.rb @@ -24,7 +24,7 @@ def failure private def throttle - Throttle.for(user: idv_session.current_user, throttle_type: :idv_resolution) + Throttle.for(user: idv_session.current_user, throttle_type: :proof_address) end def confirm_idv_phone_step_needed diff --git a/app/models/throttle.rb b/app/models/throttle.rb index ca773d0c7b1..bdfd4c65a02 100644 --- a/app/models/throttle.rb +++ b/app/models/throttle.rb @@ -13,6 +13,7 @@ class Throttle < ApplicationRecord verify_personal_key: 7, verify_gpo_key: 8, proof_ssn: 9, + proof_address: 10, } THROTTLE_CONFIG = { @@ -52,6 +53,10 @@ class Throttle < ApplicationRecord max_attempts: IdentityConfig.store.proof_ssn_max_attempts, attempt_window: IdentityConfig.store.proof_ssn_max_attempt_window_in_minutes, }, + proof_address: { + max_attempts: IdentityConfig.store.proof_address_max_attempts, + attempt_window: IdentityConfig.store.proof_address_max_attempt_window_in_minutes, + }, }.with_indifferent_access.freeze # Either target or user must be supplied diff --git a/app/services/idv/phone_step.rb b/app/services/idv/phone_step.rb index b6007ccd99c..eeaf9122c2d 100644 --- a/app/services/idv/phone_step.rb +++ b/app/services/idv/phone_step.rb @@ -95,7 +95,7 @@ def phone_param end def throttle - @throttle ||= Throttle.for(user: idv_session.current_user, throttle_type: :idv_resolution) + @throttle ||= Throttle.for(user: idv_session.current_user, throttle_type: :proof_address) end def failed_due_to_timeout_or_exception? diff --git a/config/application.yml.default b/config/application.yml.default index b9d3db625a9..647e6e9448c 100644 --- a/config/application.yml.default +++ b/config/application.yml.default @@ -158,6 +158,8 @@ proofing_allow_expired_license: 'false' proofing_expired_license_after: '2020-03-01' proofing_expired_license_reproof_at: '2023-03-01' proofing_send_partial_dob: 'false' +proof_address_max_attempts: '5' +proof_address_max_attempt_window_in_minutes: '360' proof_ssn_max_attempts: '10' proof_ssn_max_attempt_window_in_minutes: '60' push_notifications_enabled: 'false' diff --git a/lib/identity_config.rb b/lib/identity_config.rb index 1de4e3ec2bd..58cffad3120 100644 --- a/lib/identity_config.rb +++ b/lib/identity_config.rb @@ -227,6 +227,8 @@ def self.build_store(config_map) config.add(:proofing_expired_license_after, type: :date) config.add(:proofing_expired_license_reproof_at, type: :date) config.add(:proofing_send_partial_dob, type: :boolean) + config.add(:proof_address_max_attempts, type: :integer) + config.add(:proof_address_max_attempt_window_in_minutes, type: :integer) config.add(:proof_ssn_max_attempts, type: :integer) config.add(:proof_ssn_max_attempt_window_in_minutes, type: :integer) config.add(:push_notifications_enabled, type: :boolean) diff --git a/spec/controllers/idv/phone_controller_spec.rb b/spec/controllers/idv/phone_controller_spec.rb index 447cd00cb32..832b8d88f97 100644 --- a/spec/controllers/idv/phone_controller_spec.rb +++ b/spec/controllers/idv/phone_controller_spec.rb @@ -3,7 +3,7 @@ describe Idv::PhoneController do include IdvHelper - let(:max_attempts) { idv_max_attempts } + let(:max_attempts) { Throttle.max_attempts(:proof_address) } let(:good_phone) { '+1 (703) 555-0000' } let(:bad_phone) do Proofing::Mock::AddressMockClient::UNVERIFIABLE_PHONE_NUMBER @@ -68,7 +68,7 @@ context 'when the user is throttled' do before do - create(:throttle, :with_throttled, user: user, throttle_type: :idv_resolution) + create(:throttle, :with_throttled, user: user, throttle_type: :proof_address) end it 'redirects to fail' do @@ -327,8 +327,8 @@ create( :throttle, user: user, - throttle_type: :idv_resolution, - attempts: max_attempts_less_one, + throttle_type: :proof_address, + attempts: max_attempts - 1, ) end @@ -340,7 +340,7 @@ expect(@analytics).to receive(:track_event).with( Analytics::THROTTLER_RATE_LIMIT_TRIGGERED, - throttle_type: :idv_resolution, + throttle_type: :proof_address, step_name: a_kind_of(Symbol), ) diff --git a/spec/controllers/idv/phone_errors_controller_spec.rb b/spec/controllers/idv/phone_errors_controller_spec.rb index b3c57bc4455..308af2abf34 100644 --- a/spec/controllers/idv/phone_errors_controller_spec.rb +++ b/spec/controllers/idv/phone_errors_controller_spec.rb @@ -61,7 +61,7 @@ let(:user) { create(:user) } before do - create(:throttle, user: user, throttle_type: :idv_resolution, attempts: 1) + create(:throttle, user: user, throttle_type: :proof_address, attempts: 1) end it 'assigns remaining count' do @@ -82,7 +82,7 @@ let(:user) { create(:user) } before do - create(:throttle, user: user, throttle_type: :idv_resolution, attempts: 1) + create(:throttle, user: user, throttle_type: :proof_address, attempts: 1) end it 'assigns remaining count' do @@ -103,7 +103,7 @@ let(:user) { create(:user) } before do - create(:throttle, user: user, throttle_type: :idv_resolution, attempts: 1) + create(:throttle, user: user, throttle_type: :proof_address, attempts: 1) end it 'assigns remaining count' do @@ -124,7 +124,7 @@ let(:user) { create(:user) } before do - create(:throttle, :with_throttled, user: user, throttle_type: :idv_resolution) + create(:throttle, :with_throttled, user: user, throttle_type: :proof_address) end it 'assigns expiration time' do diff --git a/spec/controllers/idv/session_errors_controller_spec.rb b/spec/controllers/idv/session_errors_controller_spec.rb index a2bd42082f3..af15f55899a 100644 --- a/spec/controllers/idv/session_errors_controller_spec.rb +++ b/spec/controllers/idv/session_errors_controller_spec.rb @@ -69,7 +69,7 @@ before do user = create(:user) stub_sign_in(user) - create(:throttle, user: user, throttle_type: :idv_resolution, attempts: 1) + create(:throttle, user: user, throttle_type: :proof_address, attempts: 1) end it 'assigns remaining count' do @@ -90,7 +90,7 @@ before do user = create(:user) stub_sign_in(user) - create(:throttle, :with_throttled, user: user, throttle_type: :idv_resolution) + create(:throttle, :with_throttled, user: user, throttle_type: :proof_address) end it 'assigns expiration time' do @@ -110,10 +110,15 @@ context 'while throttled' do let(:ssn) { '666666666' } + around do |ex| + freeze_time { ex.run } + end + before do stub_sign_in create( :throttle, + :with_throttled, target: Pii::Fingerprinter.fingerprint(ssn), throttle_type: :proof_ssn, ) @@ -123,7 +128,7 @@ it 'assigns expiration time' do get action - expect(assigns(:expires_at)).to be_kind_of(Time) + expect(assigns(:expires_at)).not_to eq(Time.zone.now) end end end diff --git a/spec/features/idv/doc_auth/verify_step_spec.rb b/spec/features/idv/doc_auth/verify_step_spec.rb index 7cdad3de0d0..d39a0da6300 100644 --- a/spec/features/idv/doc_auth/verify_step_spec.rb +++ b/spec/features/idv/doc_auth/verify_step_spec.rb @@ -5,7 +5,7 @@ include DocAuthHelper let(:skip_step_completion) { false } - let(:max_attempts) { idv_max_attempts } + let(:max_attempts) { Throttle.max_attempts(:idv_resolution) } let(:fake_analytics) { FakeAnalytics.new } let(:user) { create(:user, :signed_up) } before do diff --git a/spec/services/idv/phone_step_spec.rb b/spec/services/idv/phone_step_spec.rb index a0596767fc9..6d27fb519c9 100644 --- a/spec/services/idv/phone_step_spec.rb +++ b/spec/services/idv/phone_step_spec.rb @@ -40,7 +40,7 @@ } describe '#submit' do - let(:throttle) { create(:throttle, user: user, throttle_type: :idv_resolution) } + let(:throttle) { create(:throttle, user: user, throttle_type: :proof_address) } it 'succeeds with good params' do context = { stages: [{ address: 'AddressMock' }] } @@ -172,8 +172,8 @@ create( :throttle, user: user, - throttle_type: :idv_resolution, - attempts: max_attempts_less_one, + throttle_type: :proof_address, + attempts: Throttle.max_attempts(:proof_address) - 1, ) subject.submit(phone: bad_phone) diff --git a/spec/support/features/idv_helper.rb b/spec/support/features/idv_helper.rb index a4493dcb82b..c9e486737eb 100644 --- a/spec/support/features/idv_helper.rb +++ b/spec/support/features/idv_helper.rb @@ -5,14 +5,6 @@ def self.included(base) base.class_eval { include JavascriptDriverHelper } end - def max_attempts_less_one - idv_max_attempts - 1 - end - - def idv_max_attempts - Throttle::THROTTLE_CONFIG[:idv_resolution][:max_attempts] - end - def user_password Features::SessionHelper::VALID_PASSWORD end diff --git a/spec/support/idv_examples/max_attempts.rb b/spec/support/idv_examples/max_attempts.rb index fbf7f6a73f9..80d9176fc79 100644 --- a/spec/support/idv_examples/max_attempts.rb +++ b/spec/support/idv_examples/max_attempts.rb @@ -71,7 +71,7 @@ context 'after completing one less than the max attempts' do it 'allows the user to continue if their last attempt is successful' do - max_attempts_less_one.times do + (Throttle.max_attempts(:proof_address) - 1).times do fill_out_phone_form_fail click_continue click_on t('idv.failure.button.warning') @@ -86,7 +86,7 @@ end def perfom_maximum_allowed_idv_step_attempts - max_attempts_less_one.times do + (Throttle.max_attempts(:proof_address) - 1).times do yield click_idv_continue click_on t('idv.failure.button.warning') From 1c4cadb1533beb89194aaf215b2731b43319c58c Mon Sep 17 00:00:00 2001 From: Mitchell Henke Date: Wed, 27 Oct 2021 10:05:46 -0500 Subject: [PATCH 06/30] Add link to continue to SP after recovering with personal key (#5541) * simple version of continuing to SP after recovering with personal key * Update app/views/accounts/_personal_key.html.erb Co-authored-by: Zach Margolis * fix specs * fix erblint * add spec * move sp continue into own partial * add translations Co-authored-by: Zach Margolis --- .erb-lint.yml | 4 --- .../accounts/connected_accounts_controller.rb | 2 ++ .../accounts/history_controller.rb | 2 ++ .../two_factor_authentication_controller.rb | 2 ++ app/controllers/accounts_controller.rb | 2 ++ app/controllers/events_controller.rb | 2 ++ app/view_models/account_show.rb | 22 +++++++++++-- app/views/accounts/_password_reset.html.erb | 6 +--- .../_pending_profile_bounced_gpo.html.erb | 6 +--- .../accounts/_pending_profile_gpo.html.erb | 6 +--- app/views/accounts/_personal_key.html.erb | 6 +--- .../_service_provider_continue.html.erb | 3 ++ app/views/accounts/show.html.erb | 33 ++++++++++++------- config/locales/account/en.yml | 1 + config/locales/account/es.yml | 1 + config/locales/account/fr.yml | 1 + spec/controllers/accounts_controller_spec.rb | 4 +++ spec/view_models/account_show_spec.rb | 10 ++++++ .../connected_accounts/show.html.erb_spec.rb | 1 + .../accounts/history/show.html.erb_spec.rb | 1 + spec/views/accounts/show.html.erb_spec.rb | 24 ++++++++++++++ .../show.html.erb_spec.rb | 2 ++ 22 files changed, 103 insertions(+), 38 deletions(-) create mode 100644 app/views/accounts/_service_provider_continue.html.erb diff --git a/.erb-lint.yml b/.erb-lint.yml index dfc4220a9da..03811f95088 100644 --- a/.erb-lint.yml +++ b/.erb-lint.yml @@ -11,10 +11,6 @@ linters: - '*/app/views/accounts/_connected_app.html.erb' - '*/app/views/accounts/_emails.html.erb' - '*/app/views/accounts/_identity_item.html.erb' - - '*/app/views/accounts/_password_reset.html.erb' - - '*/app/views/accounts/_pending_profile_bounced_gpo.html.erb' - - '*/app/views/accounts/_pending_profile_gpo.html.erb' - - '*/app/views/accounts/_personal_key.html.erb' - '*/app/views/accounts/_phone.html.erb' - '*/app/views/accounts/_piv_cac.html.erb' - '*/app/views/accounts/_webauthn.html.erb' diff --git a/app/controllers/accounts/connected_accounts_controller.rb b/app/controllers/accounts/connected_accounts_controller.rb index 6c2a5db033f..4a60e70ccbd 100644 --- a/app/controllers/accounts/connected_accounts_controller.rb +++ b/app/controllers/accounts/connected_accounts_controller.rb @@ -9,6 +9,8 @@ def show @view_model = AccountShow.new( decrypted_pii: nil, personal_key: flash[:personal_key], + sp_session_request_url: sp_session_request_url_without_prompt_login, + sp_name: decorated_session.sp_name, decorated_user: current_user.decorate, locked_for_session: pii_locked_for_session?(current_user), ) diff --git a/app/controllers/accounts/history_controller.rb b/app/controllers/accounts/history_controller.rb index ff60ea139bf..5919aadb0d1 100644 --- a/app/controllers/accounts/history_controller.rb +++ b/app/controllers/accounts/history_controller.rb @@ -9,6 +9,8 @@ def show @view_model = AccountShow.new( decrypted_pii: nil, personal_key: flash[:personal_key], + sp_session_request_url: sp_session_request_url_without_prompt_login, + sp_name: decorated_session.sp_name, decorated_user: current_user.decorate, locked_for_session: pii_locked_for_session?(current_user), ) diff --git a/app/controllers/accounts/two_factor_authentication_controller.rb b/app/controllers/accounts/two_factor_authentication_controller.rb index ba93183a373..94119e693ef 100644 --- a/app/controllers/accounts/two_factor_authentication_controller.rb +++ b/app/controllers/accounts/two_factor_authentication_controller.rb @@ -10,6 +10,8 @@ def show @view_model = AccountShow.new( decrypted_pii: nil, personal_key: flash[:personal_key], + sp_session_request_url: sp_session_request_url_without_prompt_login, + sp_name: decorated_session.sp_name, decorated_user: current_user.decorate, locked_for_session: pii_locked_for_session?(current_user), ) diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index e327e8dbf43..7d5394d58c3 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -11,6 +11,8 @@ def show @view_model = AccountShow.new( decrypted_pii: cacher.fetch, personal_key: flash[:personal_key], + sp_session_request_url: sp_session_request_url_without_prompt_login, + sp_name: decorated_session.sp_name, decorated_user: current_user.decorate, locked_for_session: pii_locked_for_session?(current_user), ) diff --git a/app/controllers/events_controller.rb b/app/controllers/events_controller.rb index c7b6e6f885f..8686b1e62ec 100644 --- a/app/controllers/events_controller.rb +++ b/app/controllers/events_controller.rb @@ -10,6 +10,8 @@ def show @view_model = AccountShow.new( decrypted_pii: nil, personal_key: nil, + sp_session_request_url: sp_session_request_url_without_prompt_login, + sp_name: decorated_session.sp_name, decorated_user: current_user.decorate, locked_for_session: pii_locked_for_session?(current_user), ) diff --git a/app/view_models/account_show.rb b/app/view_models/account_show.rb index da0bfdd61bf..855df2b7414 100644 --- a/app/view_models/account_show.rb +++ b/app/view_models/account_show.rb @@ -1,10 +1,14 @@ class AccountShow - attr_reader :decorated_user, :decrypted_pii, :personal_key, :locked_for_session, :pii + attr_reader :decorated_user, :decrypted_pii, :personal_key, :locked_for_session, :pii, + :sp_session_request_url, :sp_name - def initialize(decrypted_pii:, personal_key:, decorated_user:, locked_for_session:) + def initialize(decrypted_pii:, personal_key:, sp_session_request_url:, sp_name:, decorated_user:, + locked_for_session:) @decrypted_pii = decrypted_pii @personal_key = personal_key @decorated_user = decorated_user + @sp_name = sp_name + @sp_session_request_url = sp_session_request_url @locked_for_session = locked_for_session @pii = determine_pii end @@ -30,6 +34,20 @@ def show_manage_personal_key_partial? end end + def show_service_provider_continue_partial? + sp_name.present? && sp_session_request_url.present? + end + + def show_gpo_partial? + decorated_user.pending_profile_requires_verification? + end + + def showing_any_partials? + show_service_provider_continue_partial? || show_manage_personal_key_partial? || + show_pii_partial? || show_password_reset_partial? || show_personal_key_partial? || + show_gpo_partial? + end + def backup_codes_generated_at decorated_user.user.backup_code_configurations.order(created_at: :asc).first&.created_at end diff --git a/app/views/accounts/_password_reset.html.erb b/app/views/accounts/_password_reset.html.erb index 5c248d45449..219ee48c5bf 100644 --- a/app/views/accounts/_password_reset.html.erb +++ b/app/views/accounts/_password_reset.html.erb @@ -1,8 +1,4 @@ -<%= render 'shared/alert', { - type: 'warning', - class: 'margin-bottom-8', - text_tag: 'div', -} do %> +<%= render 'shared/alert', { type: 'warning', text_tag: 'div' } do %>

<%= t('account.index.reactivation.instructions') %>

diff --git a/app/views/accounts/_pending_profile_bounced_gpo.html.erb b/app/views/accounts/_pending_profile_bounced_gpo.html.erb index 2bb4f0f6ce6..6291582f886 100644 --- a/app/views/accounts/_pending_profile_bounced_gpo.html.erb +++ b/app/views/accounts/_pending_profile_bounced_gpo.html.erb @@ -1,8 +1,4 @@ -<%= render 'shared/alert', { - type: 'warning', - class: 'margin-bottom-8', - text_tag: 'div', -} do %> +<%= render 'shared/alert', { type: 'warning', text_tag: 'div' } do %>

<%= t('account.index.verification.bounced') %>

diff --git a/app/views/accounts/_pending_profile_gpo.html.erb b/app/views/accounts/_pending_profile_gpo.html.erb index 38ad9b713ad..413e069eda1 100644 --- a/app/views/accounts/_pending_profile_gpo.html.erb +++ b/app/views/accounts/_pending_profile_gpo.html.erb @@ -1,8 +1,4 @@ -<%= render 'shared/alert', { - type: 'warning', - class: 'margin-bottom-8', - text_tag: 'div', -} do %> +<%= render 'shared/alert', { type: 'warning', text_tag: 'div' } do %>

<%= t('account.index.verification.instructions') %>

diff --git a/app/views/accounts/_personal_key.html.erb b/app/views/accounts/_personal_key.html.erb index a757ac96739..09c6adb4de2 100644 --- a/app/views/accounts/_personal_key.html.erb +++ b/app/views/accounts/_personal_key.html.erb @@ -1,8 +1,4 @@ -<%= render 'shared/alert', { - type: 'warning', - class: 'margin-bottom-8', - text_tag: 'div', -} do %> +<%= render 'shared/alert', { type: 'warning', class: 'margin-bottom-2', text_tag: 'div' } do %>

<%= t('idv.messages.personal_key') %>

diff --git a/app/views/accounts/_service_provider_continue.html.erb b/app/views/accounts/_service_provider_continue.html.erb new file mode 100644 index 00000000000..2b0e9d8f120 --- /dev/null +++ b/app/views/accounts/_service_provider_continue.html.erb @@ -0,0 +1,3 @@ +<%= render 'shared/alert', { type: 'info', text_tag: 'div' } do %> + <%= link_to(t('account.index.continue_to_service_provider', service_provider: view_model.sp_name), view_model.sp_session_request_url) %> +<% end %> diff --git a/app/views/accounts/show.html.erb b/app/views/accounts/show.html.erb index 4c4837219ee..a0e8707b8a9 100644 --- a/app/views/accounts/show.html.erb +++ b/app/views/accounts/show.html.erb @@ -1,18 +1,27 @@ <% title t('titles.account') %> -<% if @view_model.show_personal_key_partial? %> - <%= render 'accounts/personal_key', view_model: @view_model %> -<% end %> -<% if @view_model.show_password_reset_partial? %> - <%= render 'accounts/password_reset', view_model: @view_model %> -<% end %> +<% if @view_model.showing_any_partials? %> +
+ <% if @view_model.show_personal_key_partial? %> + <%= render 'accounts/personal_key', view_model: @view_model %> + <% end %> -<% if @view_model.decorated_user.pending_profile_requires_verification? %> - <% if @view_model.decorated_user.gpo_mail_bounced? %> - <%= render 'accounts/pending_profile_bounced_gpo' %> - <% else %> - <%= render 'accounts/pending_profile_gpo' %> - <% end %> + <% if @view_model.show_password_reset_partial? %> + <%= render 'accounts/password_reset', view_model: @view_model %> + <% end %> + + <% if @view_model.show_gpo_partial? %> + <% if @view_model.decorated_user.gpo_mail_bounced? %> + <%= render 'accounts/pending_profile_bounced_gpo' %> + <% else %> + <%= render 'accounts/pending_profile_gpo' %> + <% end %> + <% end %> + + <% if @view_model.show_service_provider_continue_partial? %> + <%= render 'accounts/service_provider_continue', view_model: @view_model %> + <% end %> +
<% end %> <%= render 'accounts/header', view_model: @view_model %> diff --git a/config/locales/account/en.yml b/config/locales/account/en.yml index 3880f507710..e8f9193574a 100644 --- a/config/locales/account/en.yml +++ b/config/locales/account/en.yml @@ -32,6 +32,7 @@ en: backup_code_confirm_regenerate: Yes, regenerate codes backup_codes_exist: Generated backup_codes_no_exist: Not generated + continue_to_service_provider: Continue to %{service_provider} default: default device: '%{browser} on %{os}' dob: Date of birth diff --git a/config/locales/account/es.yml b/config/locales/account/es.yml index 065aa4e4af2..cd8e05f037a 100644 --- a/config/locales/account/es.yml +++ b/config/locales/account/es.yml @@ -33,6 +33,7 @@ es: backup_code_confirm_regenerate: Sí, regenerar códigos. backup_codes_exist: Generado backup_codes_no_exist: No generado + continue_to_service_provider: Continuar con %{service_provider} default: defecto device: '%{browser} en %{os}' dob: Fecha de nacimiento diff --git a/config/locales/account/fr.yml b/config/locales/account/fr.yml index 56e33d63eb0..4692b6634a8 100644 --- a/config/locales/account/fr.yml +++ b/config/locales/account/fr.yml @@ -34,6 +34,7 @@ fr: backup_code_confirm_regenerate: Oui, régénérer les codes backup_codes_exist: Généré backup_codes_no_exist: Non généré + continue_to_service_provider: Continuer à %{service_provider} default: défaut device: '%{browser} sur %{os}' dob: Date de naissance diff --git a/spec/controllers/accounts_controller_spec.rb b/spec/controllers/accounts_controller_spec.rb index aba60a92fd3..da9c64691a7 100644 --- a/spec/controllers/accounts_controller_spec.rb +++ b/spec/controllers/accounts_controller_spec.rb @@ -44,6 +44,8 @@ view_model = AccountShow.new( decrypted_pii: nil, personal_key: nil, + sp_session_request_url: nil, + sp_name: nil, decorated_user: user.decorate, locked_for_session: false, ) @@ -86,6 +88,8 @@ view_model = AccountShow.new( decrypted_pii: nil, personal_key: nil, + sp_session_request_url: nil, + sp_name: nil, decorated_user: user.decorate, locked_for_session: false, ) diff --git a/spec/view_models/account_show_spec.rb b/spec/view_models/account_show_spec.rb index d940bbbf356..8551ba73c99 100644 --- a/spec/view_models/account_show_spec.rb +++ b/spec/view_models/account_show_spec.rb @@ -14,6 +14,7 @@ ) profile_index = AccountShow.new( decrypted_pii: decrypted_pii, personal_key: '', decorated_user: user.decorate, + sp_session_request_url: nil, sp_name: nil, locked_for_session: false ) @@ -28,6 +29,7 @@ email_address.update!(last_sign_in_at: 1.minute.from_now) profile_index = AccountShow.new( decrypted_pii: {}, personal_key: '', decorated_user: decorated_user, + sp_session_request_url: nil, sp_name: nil, locked_for_session: false ) @@ -46,6 +48,7 @@ profile_index = AccountShow.new( decrypted_pii: {}, personal_key: '', decorated_user: user.decorate, + sp_session_request_url: nil, sp_name: nil, locked_for_session: false ) @@ -61,6 +64,7 @@ ).to receive(:enabled?).and_return(false) profile_index = AccountShow.new( decrypted_pii: {}, personal_key: '', decorated_user: user, + sp_session_request_url: nil, sp_name: nil, locked_for_session: false ) @@ -78,6 +82,8 @@ account_show = AccountShow.new( decrypted_pii: {}, personal_key: '', + sp_session_request_url: nil, + sp_name: nil, decorated_user: user.reload.decorate, locked_for_session: false, ) @@ -95,6 +101,8 @@ account_show = AccountShow.new( decrypted_pii: {}, personal_key: '', + sp_session_request_url: nil, + sp_name: nil, decorated_user: user.reload.decorate, locked_for_session: false, ) @@ -113,6 +121,8 @@ AccountShow.new( decrypted_pii: decrypted_pii, personal_key: '', + sp_session_request_url: nil, + sp_name: nil, decorated_user: user.decorate, locked_for_session: false, ) diff --git a/spec/views/accounts/connected_accounts/show.html.erb_spec.rb b/spec/views/accounts/connected_accounts/show.html.erb_spec.rb index 601117eceb9..123de533c63 100644 --- a/spec/views/accounts/connected_accounts/show.html.erb_spec.rb +++ b/spec/views/accounts/connected_accounts/show.html.erb_spec.rb @@ -10,6 +10,7 @@ :view_model, AccountShow.new( decrypted_pii: nil, personal_key: nil, decorated_user: decorated_user, + sp_session_request_url: nil, sp_name: nil, locked_for_session: false ), ) diff --git a/spec/views/accounts/history/show.html.erb_spec.rb b/spec/views/accounts/history/show.html.erb_spec.rb index a0ae03f63b8..b537e95feee 100644 --- a/spec/views/accounts/history/show.html.erb_spec.rb +++ b/spec/views/accounts/history/show.html.erb_spec.rb @@ -11,6 +11,7 @@ :view_model, AccountShow.new( decrypted_pii: nil, personal_key: nil, decorated_user: decorated_user, + sp_session_request_url: nil, sp_name: nil, locked_for_session: false ), ) diff --git a/spec/views/accounts/show.html.erb_spec.rb b/spec/views/accounts/show.html.erb_spec.rb index 4010a697d83..d9cb2be9302 100644 --- a/spec/views/accounts/show.html.erb_spec.rb +++ b/spec/views/accounts/show.html.erb_spec.rb @@ -11,6 +11,7 @@ :view_model, AccountShow.new( decrypted_pii: nil, personal_key: nil, decorated_user: decorated_user, + sp_session_request_url: nil, sp_name: nil, locked_for_session: false ), ) @@ -149,4 +150,27 @@ expect(user.email_addresses.size).to eq(5) end end + + context 'when a profile has just been re-activated with personal key during SP auth' do + let(:sp) { build(:service_provider, return_to_sp_url: 'https://www.example.com/auth') } + before do + assign( + :view_model, + AccountShow.new( + decrypted_pii: nil, personal_key: 'abc123', decorated_user: decorated_user, + sp_session_request_url: sp.return_to_sp_url, sp_name: sp.friendly_name, + locked_for_session: false + ), + ) + end + + it 'renders the link to continue to the SP' do + render + + expect(rendered).to have_link( + t('account.index.continue_to_service_provider', service_provider: sp.friendly_name), + href: sp.return_to_sp_url, + ) + end + end end diff --git a/spec/views/accounts/two_factor_authentication/show.html.erb_spec.rb b/spec/views/accounts/two_factor_authentication/show.html.erb_spec.rb index 58b129bf98c..bc2073e4d39 100644 --- a/spec/views/accounts/two_factor_authentication/show.html.erb_spec.rb +++ b/spec/views/accounts/two_factor_authentication/show.html.erb_spec.rb @@ -11,6 +11,7 @@ :view_model, AccountShow.new( decrypted_pii: nil, personal_key: nil, decorated_user: decorated_user, + sp_session_request_url: nil, sp_name: nil, locked_for_session: false ), ) @@ -33,6 +34,7 @@ :view_model, AccountShow.new( decrypted_pii: nil, personal_key: nil, decorated_user: decorated_user, + sp_session_request_url: nil, sp_name: nil, locked_for_session: false ), ) From ce0683ba113f6009ac6b03e4481be77edfa43f1c Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 27 Oct 2021 15:29:09 -0400 Subject: [PATCH 07/30] LG-5213: IAL2 document capture tips prior to submission (#5534) * wip * Extract FormStepsContinueButton from FormSteps Allow greater control of placement of Continue button * Move CaptureAttemptsTroubleshooting to DocumentsStep * Track metadata of last capture attempt To create different messaging based on failure attempt * Rename CaptureAttempts context as FailedCaptureAttempts **Why**: Because tracked only as failed attempts * Temporarily disable prompt on navigate make development less annoying * Fix TroubleshootingOptions markup Include list wrapper * Add capture attempts troubleshooting content * Trigger onFailedCaptureAttempt on failed capture attempt * Revert "Temporarily disable prompt on navigate" This reverts commit a143950c177da4fea1e4eb1b82124f0392289ac5. * Use warning component for attempts troubleshooting * useCounter: Add support for reset * Reset failed attempts after success * Warning: Margin between warning icon and page heading * Translate capture tips content * Add troubleshooting links * Taking the title out of form-steps and requiring each step to create its own. Added focus anchor at the top of step content. * Fixing a few things after comment and fixing a couple of tests. More test fixing to come. * Oops! Forgot to remove the title prop typescript doc. * It seems a few svgs could be further optimized. * start fixing formsteps specs * Fixed a few more tests. * A quick attempt at fixing the failing SubmissionInterstitial test. Thought it would work but it doesn't. * Assign Node global in spec helper * Restore behavior to move focus ahead of alert on errors * Temporary translations for beginning of step content * Specs asserting focus target label text * Spec for document steps troubleshooting * Update BlockLink spec description Moved from document-capture * Add TroubleshootingOptions specs * Restore SubmissionInterstitial autoFocus behavior https://github.com/18F/identity-idp/pull/5534/files#r735605388 * Add MarketingSiteContext displayName Fix failing spec, make visible to React DevTools * Specs for FailedCaptureAttemptsContext * Specs for MarketingSiteContext * More exhaustive DocumentStep failed attempts spec Test behavior of successful reset, no multi-display * Move documents footer specs to DocumentsStep Not managed at top-level anymore, step-specific * Add CaptureTroubleshooting component specs * Remove unnecessary async function Lint * Update document-capture-spec to click correct submit button Seems like this should've been failing already? * Combine maxCaptureAttemptsBeforeTips into AppRoot type destructure * merge main * Revert "merge main" This reverts commit eadf947f853c844fe32e772c90fb1499a34d6a4f. * Remove explicit focus anchor in favor of first content as anchor **Why**: Maintain as close to the current implementation as we can until further research and user testing can give us insight into optimal navigation experience. * Add spec for MarketingSite.help_document_capture_tips_url * Increase failed attempts before tips from 2 to 3 https://github.com/18F/identity-idp/pull/5534#pullrequestreview-791018516 * Increase vertical margin of try again to 2.5rem See: https://github.com/18F/identity-idp/pull/5534#pullrequestreview-791018516 Co-authored-by: amathews --- app/assets/images/idv/capture-tips-clean.svg | 1 + .../images/idv/capture-tips-lighting.svg | 1 + .../images/idv/capture-tips-surface.svg | 1 + .../components/block-link.jsx | 0 .../packages/components/block-link.spec.jsx | 4 +- app/javascript/packages/components/index.js | 2 + .../components/troubleshooting-options.jsx | 38 ++++ .../troubleshooting-options.spec.jsx | 34 ++++ .../components/acuant-capture.jsx | 9 +- .../components/capture-advice.jsx | 90 ++++++++++ .../components/capture-troubleshooting.jsx | 34 ++++ .../components/document-capture.jsx | 6 - .../components/documents-step.jsx | 13 +- .../components/form-steps.jsx | 162 ++++++++++-------- .../components/form-steps.scss | 7 + .../components/review-issues-step.jsx | 8 +- .../components/selfie-step.jsx | 4 + .../document-capture/components/warning.jsx | 76 ++++++++ .../context/failed-capture-attempts.jsx | 86 ++++++++++ .../document-capture/context/index.js | 5 + .../context/marketing-site.jsx | 15 ++ .../document-capture/hooks/use-counter.js | 5 +- app/javascript/packs/document-capture.jsx | 28 ++- app/services/marketing_site.rb | 8 + .../idv/shared/_document_capture.html.erb | 8 +- config/application.yml.default | 1 + config/locales/doc_auth/en.yml | 13 ++ config/locales/doc_auth/es.yml | 15 ++ config/locales/doc_auth/fr.yml | 16 ++ config/locales/idv/en.yml | 2 + config/locales/idv/es.yml | 2 + config/locales/idv/fr.yml | 2 + lib/identity_config.rb | 1 + .../capture-troubleshooting-spec.jsx | 40 +++++ .../components/document-capture-spec.jsx | 40 +---- .../components/documents-step-spec.jsx | 77 ++++++++- .../components/form-steps-spec.jsx | 41 +++-- .../context/failed-capture-attempts-spec.jsx | 48 ++++++ .../context/marketing-site-spec.jsx | 12 ++ .../hooks/use-counter-spec.jsx | 13 ++ spec/javascripts/spec_helper.js | 1 + spec/services/marketing_site_spec.rb | 18 ++ 42 files changed, 846 insertions(+), 141 deletions(-) create mode 100644 app/assets/images/idv/capture-tips-clean.svg create mode 100644 app/assets/images/idv/capture-tips-lighting.svg create mode 100644 app/assets/images/idv/capture-tips-surface.svg rename app/javascript/packages/{document-capture => }/components/block-link.jsx (100%) rename spec/javascripts/packages/document-capture/components/block-link-spec.jsx => app/javascript/packages/components/block-link.spec.jsx (85%) create mode 100644 app/javascript/packages/components/troubleshooting-options.jsx create mode 100644 app/javascript/packages/components/troubleshooting-options.spec.jsx create mode 100644 app/javascript/packages/document-capture/components/capture-advice.jsx create mode 100644 app/javascript/packages/document-capture/components/capture-troubleshooting.jsx create mode 100644 app/javascript/packages/document-capture/components/form-steps.scss create mode 100644 app/javascript/packages/document-capture/components/warning.jsx create mode 100644 app/javascript/packages/document-capture/context/failed-capture-attempts.jsx create mode 100644 app/javascript/packages/document-capture/context/marketing-site.jsx create mode 100644 spec/javascripts/packages/document-capture/components/capture-troubleshooting-spec.jsx create mode 100644 spec/javascripts/packages/document-capture/context/failed-capture-attempts-spec.jsx create mode 100644 spec/javascripts/packages/document-capture/context/marketing-site-spec.jsx diff --git a/app/assets/images/idv/capture-tips-clean.svg b/app/assets/images/idv/capture-tips-clean.svg new file mode 100644 index 00000000000..60b0eccdbf9 --- /dev/null +++ b/app/assets/images/idv/capture-tips-clean.svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/app/assets/images/idv/capture-tips-lighting.svg b/app/assets/images/idv/capture-tips-lighting.svg new file mode 100644 index 00000000000..aba228f49c2 --- /dev/null +++ b/app/assets/images/idv/capture-tips-lighting.svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/app/assets/images/idv/capture-tips-surface.svg b/app/assets/images/idv/capture-tips-surface.svg new file mode 100644 index 00000000000..e752997bb88 --- /dev/null +++ b/app/assets/images/idv/capture-tips-surface.svg @@ -0,0 +1 @@ + \ No newline at end of file diff --git a/app/javascript/packages/document-capture/components/block-link.jsx b/app/javascript/packages/components/block-link.jsx similarity index 100% rename from app/javascript/packages/document-capture/components/block-link.jsx rename to app/javascript/packages/components/block-link.jsx diff --git a/spec/javascripts/packages/document-capture/components/block-link-spec.jsx b/app/javascript/packages/components/block-link.spec.jsx similarity index 85% rename from spec/javascripts/packages/document-capture/components/block-link-spec.jsx rename to app/javascript/packages/components/block-link.spec.jsx index 5d1821ba03b..b37dcb0617a 100644 --- a/spec/javascripts/packages/document-capture/components/block-link-spec.jsx +++ b/app/javascript/packages/components/block-link.spec.jsx @@ -1,7 +1,7 @@ import { render } from '@testing-library/react'; -import BlockLink from '@18f/identity-document-capture/components/block-link'; +import BlockLink from './block-link'; -describe('document-capture/components/block-link', () => { +describe('BlockLink', () => { const linkText = 'link text'; const url = '/example'; diff --git a/app/javascript/packages/components/index.js b/app/javascript/packages/components/index.js index f3b5d26a59d..cae3d0acae7 100644 --- a/app/javascript/packages/components/index.js +++ b/app/javascript/packages/components/index.js @@ -1,2 +1,4 @@ export { default as Alert } from './alert'; +export { default as BlockLink } from './block-link'; export { default as Icon } from './icon'; +export { default as TroubleshootingOptions } from './troubleshooting-options'; diff --git a/app/javascript/packages/components/troubleshooting-options.jsx b/app/javascript/packages/components/troubleshooting-options.jsx new file mode 100644 index 00000000000..f6ff90c53b1 --- /dev/null +++ b/app/javascript/packages/components/troubleshooting-options.jsx @@ -0,0 +1,38 @@ +import { BlockLink } from '@18f/identity-components'; + +/** + * @typedef TroubleshootingOption + * + * @prop {string} url + * @prop {string|JSX.Element} text + * @prop {boolean=} isExternal + */ + +/** + * @typedef TroubleshootingOptionsProps + * + * @prop {string} heading + * @prop {TroubleshootingOption[]} options + */ + +/** + * @param {TroubleshootingOptionsProps} props + */ +function TroubleshootingOptions({ heading, options }) { + return ( +
+

{heading}

+
    + {options.map(({ url, text, isExternal }) => ( +
  • + + {text} + +
    • + ))} +
+
+ ); +} + +export default TroubleshootingOptions; diff --git a/app/javascript/packages/components/troubleshooting-options.spec.jsx b/app/javascript/packages/components/troubleshooting-options.spec.jsx new file mode 100644 index 00000000000..adfefbc76b7 --- /dev/null +++ b/app/javascript/packages/components/troubleshooting-options.spec.jsx @@ -0,0 +1,34 @@ +import { render } from '@testing-library/react'; +import TroubleshootingOptions from './troubleshooting-options'; + +describe('TroubleshootingOptions', () => { + it('renders a given heading', () => { + const { getByRole } = render(); + + const heading = getByRole('heading'); + + expect(heading.textContent).to.equal('Need help?'); + }); + + it('renders given options', () => { + const { getAllByRole } = render( + Option 1, url: 'https://example.com/1', isExternal: true }, + { text: 'Option 2', url: 'https://example.com/2' }, + ]} + />, + ); + + const links = /** @type {HTMLAnchorElement[]} */ (getAllByRole('link')); + + expect(links).to.have.lengthOf(2); + expect(links[0].textContent).to.equal('Option 1 links.new_window'); + expect(links[0].href).to.equal('https://example.com/1'); + expect(links[0].target).to.equal('_blank'); + expect(links[1].textContent).to.equal('Option 2'); + expect(links[1].href).to.equal('https://example.com/2'); + expect(links[1].target).to.be.empty(); + }); +}); diff --git a/app/javascript/packages/document-capture/components/acuant-capture.jsx b/app/javascript/packages/document-capture/components/acuant-capture.jsx index 43f6a51dbf4..08561a65d8a 100644 --- a/app/javascript/packages/document-capture/components/acuant-capture.jsx +++ b/app/javascript/packages/document-capture/components/acuant-capture.jsx @@ -10,6 +10,7 @@ import { import { useI18n } from '@18f/identity-react-i18n'; import AnalyticsContext from '../context/analytics'; import AcuantContext from '../context/acuant'; +import FailedCaptureAttemptsContext from '../context/failed-capture-attempts'; import AcuantCaptureCanvas from './acuant-capture-canvas'; import FileInput from './file-input'; import FullScreen from './full-screen'; @@ -272,6 +273,9 @@ function AcuantCapture( const { isMobile } = useContext(DeviceContext); const { t, formatHTML } = useI18n(); const [attempt, incrementAttempt] = useCounter(1); + const { onFailedCaptureAttempt, onResetFailedCaptureAttempts } = useContext( + FailedCaptureAttemptsContext, + ); const hasCapture = !isError && (isReady ? isCameraSupported : isMobile); useEffect(() => { // If capture had started before Acuant was ready, stop capture if readiness reveals that no @@ -297,7 +301,7 @@ function AcuantCapture( /** * Returns an analytics payload, decorated with common values. * - * @template P + * @template {ImageAnalyticsPayload|AcuantImageAnalyticsPayload} P * * @param {P} payload * @@ -480,6 +484,9 @@ function AcuantCapture( if (assessment === 'success') { onChangeAndResetError(data, analyticsPayload); + onResetFailedCaptureAttempts(); + } else { + onFailedCaptureAttempt({ isAssessedAsGlare, isAssessedAsBlurry }); } setIsCapturingEnvironment(false); diff --git a/app/javascript/packages/document-capture/components/capture-advice.jsx b/app/javascript/packages/document-capture/components/capture-advice.jsx new file mode 100644 index 00000000000..b94e9d58800 --- /dev/null +++ b/app/javascript/packages/document-capture/components/capture-advice.jsx @@ -0,0 +1,90 @@ +import { useContext } from 'react'; +import { useI18n } from '@18f/identity-react-i18n'; +import ServiceProviderContext from '../context/service-provider'; +import MarketingSiteContext from '../context/marketing-site'; +import useAsset from '../hooks/use-asset'; +import Warning from './warning'; + +/** @typedef {import('@18f/identity-components/troubleshooting-options').TroubleshootingOption} TroubleshootingOption */ + +/** + * @typedef CaptureAdviceProps + * + * @prop {() => void} onTryAgain + * @prop {boolean} isAssessedAsGlare + * @prop {boolean} isAssessedAsBlurry + */ + +/** + * @param {CaptureAdviceProps} props + */ +function CaptureAdvice({ onTryAgain, isAssessedAsGlare, isAssessedAsBlurry }) { + const { name: spName, getFailureToProofURL } = useContext(ServiceProviderContext); + const { documentCaptureTipsURL } = useContext(MarketingSiteContext); + const { getAssetPath } = useAsset(); + const { t } = useI18n(); + + return ( + +

+ {isAssessedAsGlare && t('doc_auth.tips.capture_troubleshooting_glare')} + {isAssessedAsBlurry && t('doc_auth.tips.capture_troubleshooting_blurry')}{' '} + {t('doc_auth.tips.capture_troubleshooting_lead')} +

+
    +
  • + {t('doc_auth.tips.capture_troubleshooting_surface_image')} + {t('doc_auth.tips.capture_troubleshooting_surface')} +
    • +
  • + {t('doc_auth.tips.capture_troubleshooting_lighting_image')} + {t('doc_auth.tips.capture_troubleshooting_lighting')} +
    • +
  • + {t('doc_auth.tips.capture_troubleshooting_clean_image')} + {t('doc_auth.tips.capture_troubleshooting_clean')} +
    • +
+ + ); +} + +export default CaptureAdvice; diff --git a/app/javascript/packages/document-capture/components/capture-troubleshooting.jsx b/app/javascript/packages/document-capture/components/capture-troubleshooting.jsx new file mode 100644 index 00000000000..5eaa04890d2 --- /dev/null +++ b/app/javascript/packages/document-capture/components/capture-troubleshooting.jsx @@ -0,0 +1,34 @@ +import { useContext, useState } from 'react'; +import FailedCaptureAttemptsContext from '../context/failed-capture-attempts'; +import CaptureAdvice from './capture-advice'; + +/** @typedef {import('react').ReactNode} ReactNode */ + +/** + * @typedef CaptureTroubleshootingProps + * + * @prop {ReactNode} children + */ + +/** + * @param {CaptureTroubleshootingProps} props + */ +function CaptureTroubleshooting({ children }) { + const [didShowTroubleshooting, setDidShowTroubleshooting] = useState(false); + const { failedCaptureAttempts, maxFailedAttemptsBeforeTips, lastAttemptMetadata } = useContext( + FailedCaptureAttemptsContext, + ); + const { isAssessedAsGlare, isAssessedAsBlurry } = lastAttemptMetadata; + + return failedCaptureAttempts >= maxFailedAttemptsBeforeTips && !didShowTroubleshooting ? ( + setDidShowTroubleshooting(true)} + isAssessedAsGlare={isAssessedAsGlare} + isAssessedAsBlurry={isAssessedAsBlurry} + /> + ) : ( + <>{children} + ); +} + +export default CaptureTroubleshooting; diff --git a/app/javascript/packages/document-capture/components/document-capture.jsx b/app/javascript/packages/document-capture/components/document-capture.jsx index 26bdf01672a..58c839fcb2f 100644 --- a/app/javascript/packages/document-capture/components/document-capture.jsx +++ b/app/javascript/packages/document-capture/components/document-capture.jsx @@ -9,7 +9,6 @@ import ReviewIssuesStep, { reviewIssuesStepValidator } from './review-issues-ste import ServiceProviderContext from '../context/service-provider'; import Submission from './submission'; import SubmissionStatus from './submission-status'; -import DesktopDocumentDisclosure from './desktop-document-disclosure'; import { RetrySubmissionError } from './submission-complete'; import { BackgroundEncryptedUploadError } from '../higher-order/with-background-encrypted-upload'; import SuspenseErrorBoundary from './suspense-error-boundary'; @@ -94,23 +93,18 @@ function DocumentCapture({ isAsyncForm = false, onStepChange }) { ? [ { name: 'review', - title: t('doc_auth.headings.review_issues'), form: ReviewIssuesStep, validator: reviewIssuesStepValidator, - footer: DesktopDocumentDisclosure, }, ] : /** @type {FormStep[]} */ ([ { name: 'documents', - title: t('doc_auth.headings.document_capture'), form: DocumentsStep, validator: documentsStepValidator, - footer: DesktopDocumentDisclosure, }, serviceProvider.isLivenessRequired && { name: 'selfie', - title: t('doc_auth.headings.selfie'), form: SelfieStep, validator: selfieStepValidator, }, diff --git a/app/javascript/packages/document-capture/components/documents-step.jsx b/app/javascript/packages/document-capture/components/documents-step.jsx index 2b1f5d635fa..ad0af23f809 100644 --- a/app/javascript/packages/document-capture/components/documents-step.jsx +++ b/app/javascript/packages/document-capture/components/documents-step.jsx @@ -1,10 +1,14 @@ import { useContext } from 'react'; import { useI18n } from '@18f/identity-react-i18n'; -import BlockLink from './block-link'; +import { BlockLink } from '@18f/identity-components'; +import { FormStepsContinueButton } from './form-steps'; import DocumentSideAcuantCapture from './document-side-acuant-capture'; import DeviceContext from '../context/device'; import ServiceProviderContext from '../context/service-provider'; import withBackgroundEncryptedUpload from '../higher-order/with-background-encrypted-upload'; +import DesktopDocumentDisclosure from './desktop-document-disclosure'; +import CaptureTroubleshooting from './capture-troubleshooting'; +import PageHeading from './page-heading'; /** * @typedef {'front'|'back'} DocumentSide @@ -48,7 +52,8 @@ function DocumentsStep({ const serviceProvider = useContext(ServiceProviderContext); return ( - <> + + {t('doc_auth.headings.document_capture')} {isMobile &&

{t('doc_auth.info.document_capture_intro_acknowledgment')}

}

{t('doc_auth.tips.document_capture_header_text')}

    @@ -75,7 +80,9 @@ function DocumentsStep({ onError={onError} /> ))} - + + + ); } diff --git a/app/javascript/packages/document-capture/components/form-steps.jsx b/app/javascript/packages/document-capture/components/form-steps.jsx index 40d06dbfba4..ad348dfb60f 100644 --- a/app/javascript/packages/document-capture/components/form-steps.jsx +++ b/app/javascript/packages/document-capture/components/form-steps.jsx @@ -1,14 +1,14 @@ -import { useEffect, useRef, useState } from 'react'; +import { useEffect, useRef, useState, createContext, useContext } from 'react'; import { Alert } from '@18f/identity-components'; import { useI18n } from '@18f/identity-react-i18n'; import Button from './button'; -import PageHeading from './page-heading'; import FormErrorMessage, { RequiredValueMissingError } from './form-error-message'; import PromptOnNavigate from './prompt-on-navigate'; import useHistoryParam from '../hooks/use-history-param'; import useForceRender from '../hooks/use-force-render'; import useDidUpdateEffect from '../hooks/use-did-update-effect'; import useIfStillMounted from '../hooks/use-if-still-mounted'; +import './form-steps.scss'; /** * @typedef FormStepError @@ -53,9 +53,7 @@ import useIfStillMounted from '../hooks/use-if-still-mounted'; * @typedef FormStep * * @prop {string} name Step name, used in history parameter. - * @prop {string} title Step title, shown as heading. * @prop {import('react').FC>>} form Step form component. - * @prop {import('react').FC=} footer Optional step footer component. * @prop {(object)=>boolean=} validator Optional function to validate values for the step */ @@ -78,6 +76,20 @@ import useIfStillMounted from '../hooks/use-if-still-mounted'; * @prop {()=>void=} onStepChange Callback triggered on step change. */ +/** + * @typedef FormStepsContext + * + * @prop {boolean} isLastStep + * @prop {boolean} canContinueToNextStep + */ + +const FormStepsContext = createContext( + /** @type {FormStepsContext} */ ({ + isLastStep: true, + canContinueToNextStep: true, + }), +); + /** * Returns the index of the step in the array which matches the given name. Returns `-1` if there is * no step found by that name. @@ -120,12 +132,9 @@ function FormSteps({ }) { const [values, setValues] = useState(initialValues); const [activeErrors, setActiveErrors] = useState(initialActiveErrors); - const firstAlertRef = useRef(/** @type {?HTMLElement} */ (null)); const formRef = useRef(/** @type {?HTMLFormElement} */ (null)); - const headingRef = useRef(/** @type {?HTMLHeadingElement} */ (null)); const [stepName, setStepName] = useHistoryParam('step', null); const [stepErrors, setStepErrors] = useState(/** @type {Error[]} */ ([])); - const { t } = useI18n(); const fields = useRef(/** @type {Record} */ ({})); const didSubmitWithErrors = useRef(false); const forceRender = useForceRender(); @@ -141,20 +150,33 @@ function FormSteps({ const stepIndex = Math.max(getStepIndexByName(steps, stepName), 0); const step = steps[stepIndex]; + /** + * After a change in content, maintain focus by resetting to the beginning of the new content. + */ + function focusFirstContent() { + const firstElementChild = formRef.current?.firstElementChild; + if (firstElementChild instanceof window.HTMLElement) { + firstElementChild.classList.add('form-steps__focus-anchor'); + firstElementChild.setAttribute('tabindex', '-1'); + firstElementChild.focus(); + } + } + useEffect(() => { // Treat explicit initial step the same as step transition, placing focus to header. - if (autoFocus && headingRef.current) { - headingRef.current.focus(); + if (autoFocus) { + focusFirstContent(); } }, []); useEffect(() => { - if (stepErrors.length && firstAlertRef.current) { - firstAlertRef.current.focus(); + if (stepErrors.length) { + focusFirstContent(); } }, [stepErrors]); useDidUpdateEffect(onStepChange, [step]); + useDidUpdateEffect(focusFirstContent, [step]); /** * Returns array of form errors for the current set of values. @@ -183,7 +205,7 @@ function FormSteps({ const isValidStep = step.validator?.(values) ?? true; const hasUnresolvedFieldErrors = activeErrors.length && activeErrors.length > unknownFieldErrors.length; - const canContinue = isValidStep && !hasUnresolvedFieldErrors; + const canContinueToNextStep = isValidStep && !hasUnresolvedFieldErrors; /** * Increments state to the next step, or calls onComplete callback if the current step is the last @@ -218,77 +240,75 @@ function FormSteps({ const { name: nextStepName } = steps[nextStepIndex]; setStepName(nextStepName); } - - headingRef.current?.focus(); } - const { form: Form, footer: Footer, name, title } = step; + const { form: Form, name } = step; const isLastStep = stepIndex + 1 === steps.length; return (
    {Object.keys(values).length > 0 && } - {stepErrors.concat(unknownFieldErrors.map(({ error }) => error)).map((error, i) => ( - + {stepErrors.concat(unknownFieldErrors.map(({ error }) => error)).map((error) => ( + ))} - - {title} - - { - setActiveErrors((prevActiveErrors) => - prevActiveErrors.filter(({ field }) => !(field in nextValuesPatch)), - ); - setValues((prevValues) => ({ ...prevValues, ...nextValuesPatch })); - })} - onError={ifStillMounted((error, { field } = {}) => { - if (field) { - setActiveErrors((prevActiveErrors) => prevActiveErrors.concat({ field, error })); - } else { - setStepErrors([error]); - } - })} - registerField={(field, options = {}) => { - if (!fields.current[field]) { - fields.current[field] = { - refCallback(fieldNode) { - fields.current[field].element = fieldNode; - - if (activeErrors.length) { - forceRender(); - } - }, - element: null, - isRequired: !!options.isRequired, - }; - } - - return fields.current[field].refCallback; - }} - /> - - {Footer &&