Skip to content

LG-11260: docauth not ready#9506

Merged
dawei-nava merged 8 commits intomainfrom
dwang/LG-11260-docauth-not-ready
Nov 7, 2023
Merged

LG-11260: docauth not ready#9506
dawei-nava merged 8 commits intomainfrom
dwang/LG-11260-docauth-not-ready

Conversation

@dawei-nava
Copy link
Contributor

@dawei-nava dawei-nava commented Nov 1, 2023

🎫 Ticket

LG-11260: "I'm not ready" experience

🛠 Summary of changes

Add a section of UI for user not ready to upload photo on capture screen, links to exit to service provider or account page.
Also a add flag to control rendering of this UI block.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.

  • Step 1: Enter doc auth workflow, without a SP.
  • Step 2: On image submission screen
  • Step 3: check new section of "not ready"
  • Step 4: Repeat previous steps with a SP.

👀 Screenshots

Without SP:

English:
NoSP-En

Spanish
NoSP-Es

French:
NoSP-Fr

With SP:

English:
Sp-En

Spanish:
SP-Es

French:
Sp-Fr

@dawei-nava dawei-nava marked this pull request as ready for review November 6, 2023 16:25
@dawei-nava dawei-nava force-pushed the dwang/LG-11260-docauth-not-ready branch from 7f69188 to 8be539f Compare November 6, 2023 16:34
Copy link

@kellular kellular left a comment

Choose a reason for hiding this comment

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

Noticed that one word was missing (see my comment). Otherwise LGTM.

@dawei-nava dawei-nava force-pushed the dwang/LG-11260-docauth-not-ready branch from 3dddac9 to bf02384 Compare November 7, 2023 16:02
Copy link
Contributor

@charleyf charleyf left a comment

Choose a reason for hiding this comment

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

I tested this without an SP and it works as expected. Not sure how to test with an SP so I did not do that.

Left a comment about the react code formatting, but basically this PR does what's expected so I'm approving.

Comment on lines +47 to +48
{header}
{content}
Copy link
Contributor

Choose a reason for hiding this comment

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

This works, but there are two ways to make it more in line with our FE patterns:

One, put everything inline. These are small enough that this is probably the easiest choice:

<>
      <h2 className="h3">{t('doc_auth.not_ready.header')}</h2>
      <p>
        {spName
          ? t('doc_auth.not_ready.content_sp', {
              sp_name: spName,
              app_name: appName,
            })
          : t('doc_auth.not_ready.content_nosp', {
              app_name: appName,
            })}
      </p>
...etc...
</>

Two make these into components. This is the "Correct according to React" way and probably the most reusable way to do this:

// These go outside the `DocumentCaptureNotReady` component
const NotReadyHeader = ( {text} ) => <h2 className="h3">{ text }</h2>;
const NotReadyContent = ( {text} ) => <p> { text } </p>

// This goes in the `DocumentCaptureNotReady` component
const headerText = t('doc_auth.not_ready.header')
const contentText = spName
        ? t('doc_auth.not_ready.content_sp', {
            sp_name: spName,
            app_name: appName,
          })
        : t('doc_auth.not_ready.content_nosp', {
            app_name: appName,
          })
// This is in the main return
<>
      <NotReadyHeader text={headerText} />
      <NotReadyContent text={contentText} />
...etc...
</>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charleyf I usually do SP testing from local by http://localhost:3000/test/saml/login, this only SAML though but it does provide SP information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@charleyf, indeed they are small, I put them inline.

Copy link
Contributor

Choose a reason for hiding this comment

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

@charleyf I usually do SP testing from local by http://localhost:3000/test/saml/login, this only SAML though but it does provide SP information.

Whoa, TIL this exists!

On a related note, I proposed some documentation to refer to sample apps in #9558, but that test controller could save a few steps as long as you can remember/bookmark the URL.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@aduth, it is in the rails route. 😄

Copy link
Contributor Author

@dawei-nava dawei-nava Nov 7, 2023

Choose a reason for hiding this comment

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

Should we also have a /test/oidc/login route? We do have /openid_connect/authorize, but need to pass params manually.

Copy link
Contributor

Choose a reason for hiding this comment

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

It could probably make sense to have one if we think it'd be useful, even just to maintain balance between the two protocols 😄

changelog:  User-Facing Improvements, Doc Auth, Not ready experience.
@dawei-nava dawei-nava merged commit 8cc4eb6 into main Nov 7, 2023
@dawei-nava dawei-nava deleted the dwang/LG-11260-docauth-not-ready branch November 7, 2023 20:55
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.

5 participants