[RW-4159][risk=no] Terms of Service UI and cleanup of create-account flow. #3075

gjuggler · 2020-02-03T17:45:24Z

The main goal of this PR was to create a Terms of Service acknowledgement UI based on mocks from the linked Jira ticket.

(During review, I'll keep this branch pushed to my gjordan App Engine version.)

The resulting UI looks like this (click "Create Account", then enter invitation key, then you land on this page):

and the "Next" button is enabled once they've read & clicked the checkboxes:

I couldn't help but dig into a bit of clean-up on the overall structure of the sign-in component & the various sub-components it renders. I did a few things to try and clarify the flow of data / control through the components, and improve our ability to adapt the set of sign-up forms over time. (This is partly in anticipation of launch: we're going to add a new "institutional affiliation" step, and remove the "invitation key check" step.)

The clean-up work significantly blew up the size of this PR – sorry about that! In return, I added a bunch of tests.

PR checklist

This PR meets the Acceptance Criteria in the JIRA story
The JIRA story has been moved to Dev Review
This PR includes appropriate unit tests
I have run and tested this change locally

gjuggler · 2020-02-03T22:13:37Z

Hey @blrubenstein and @NehaBroad I'd like to get both of your pairs of eyes on this PR, since it affects code you've recently (or are currently) involved in.

I opted not to do a "surgical insertion" of the TOS component here, since I felt there was some low-hanging fruit for cleanup.

Recommended flow for reviewing this PR:

Visit https://gjordan-dot-all-of-us-workbench-test.appspot.com/login and go through the create account flow to get a feel for the UI.
Review create-account-tos.tsx and its test, which focuses on the new component.
Review sign-in.tsx and its test, which contains the bulk of cleanups. The main thing I was trying to do here was add more type-safety around how we progress through each sub-component of the sign-up flow, and make it easier for us to change this over the next few weeks as we head towards launch.
Review account-creation-survey.tsx and account-creation.tsx, which have a handful of changes largely as side-effects of the cleanups to sign-in.tsx.
Ping me if you have any questions or would like to sit down and talk through any of the changes!

gjuggler · 2020-02-03T22:18:11Z

ui/src/app/pages/login/sign-in.tsx

-            </div>
-          </div>
+      const backgroundImages = StepToImageConfig[this.state.currentStep];
+      return <FlexColumn style={styles.signInContainer} data-test-id='sign-in-container'>


Dev note: it was quite a challenge to get the sign-in and Terms of Service components to be styled correctly to allow a "flexible" PDF panel with a sticky footer at the bottom to contain the checkboxes & next button. Ultimately, I took an approach of using nested FlexColumn components all the way down... this seemed to be minimally disruptive to the existing styling, though I haven't extensively tested this on very differently-sized screens. A bit of tire-kicking against my staged App Engine version would be very helpful here.

I couldn't break it - looks pretty nice to me.

calbach

Here's a first pass of comments, I only made it about half-way through. Will get back to it this afternoon.

calbach · 2020-02-04T18:26:44Z

ui/src/app/components/flex.tsx

+  // The object destructuring below serves two purposes: (1) allows users of this component to pass
+  // on styles which will override default FlexRow styles, and (2) allows other props passed to
+  // FlexRow to be passed onto the inner <div>. We exclude data-test-id from being passed on, since
+  // passing it onto the <div> would cause both the FlexRow and the <div> component to have the same


This doesn't gel with my understanding of React rendering, or maybe I'm misunderstanding how you're querying the structure via the dataTestId.

The comment here seems to imply that this React component results in two elements being rendered: a "FlexRow" and a "div", when in fact the div really is just the HTML rendering of the FlexRow component - and when rendered it's just a single div. Am I missing something?

Thanks for flagging this! It pushed me to better understand what's going on here.

The Enzyme docs (link) and this issue (link) describe the pain point better: Enzyme searches through both the DOM and the component tree. For components where we splat properties onto the inner DOM element, we end up with Enzyme returning two objects for a given 'data-test-id'.

Now that I read the Enzyme team's understanding of the issue and their reasoning here, I'm realizing that it's not unreasonable to require a test writer to specify whether they want to access the component or the rendered DOM element – in fact, it's better that way.

The trick here will just be around socializing this behavior, and clarifying what our approach should be in writing tests. Effectively, if a bare '[data-test-id="foo"]' search is yielding multiple nodes, the fix is to either (1) prefix with the component name, e.g. 'Checkbox[data-test-id ...', (2) prefix with a node tag, e.g. 'div[data-test-id ...]', or (3) add a call to .hostNodes() to exclude non-DOM nodes from the result (though this breaks when using shallow rendering).

I updated the code & tests here to reflect that. I filed https://precisionmedicineinitiative.atlassian.net/browse/RW-4409 to track further cleanup which would have blown up the scope of this PR too much. I'll also share a quick PSA at the eng sync.

calbach · 2020-02-05T18:13:43Z

ui/src/app/pages/login/account-creation/account-creation-tos.tsx

+     * This sets up the intersection listener which will change state when the user scrolls to the
+     * end of the document.
+     */
+    handleLastPageRender() {


private? (also for scrollToBottom?)

Done – sometimes I forget that real private methods are a thing now w/ Typescript...

calbach · 2020-02-05T18:18:01Z

ui/src/app/pages/login/account-creation/account-creation-tos.tsx

+                    loading=''
+                    className='terms-of-service__page'
+                    width={Math.max(500, this.props.windowSize.width * .75)}
+                    key={`page_${index + 1}`}


Thank you for adding this, I can't take any more log spam

calbach · 2020-02-05T18:18:51Z

ui/src/app/pages/login/account-creation/account-creation-tos.tsx

+                    renderAnnotationLayer={false}
+                    renderTextLayer={false}
+                    loading=''
+                    className='terms-of-service__page'


Is the __ a convention we've used elsewhere?

No, it's somewhat-standard block-element-modifier approach (https://css-tricks.com/bem-101/). I'm totally willing to back off on that one, maybe switch this to "tos-pdf-page". There's probably not too much to be done in setting a direction there before we loop back on our whole CSS-in-JS versus inline styles story, anyway.

ui/src/styles.css

ui/tsconfig.json

calbach · 2020-02-05T18:29:55Z

ui/src/app/pages/login/account-creation/account-creation.tsx

 export class AccountCreation extends React.Component<AccountCreationProps, AccountCreationState> {
  private usernameCheckTimeout: NodeJS.Timer;

  constructor(props: AccountCreationProps) {
+    if (props.profile.institutionalAffiliations.length !== 1) {
+      throw new Error('Profile must be pre-allocated with 1 institutional affiliation.');


I don't understand this precondition. Is this for the new account creation logic, or the old?

This is for both. I am trying to codify what seemed to be happening in the existing code, around L204 (where a single object was pre-allocated in the institutionalAffiliations array), L232 (where various logic and form-rendering was applied based on the first element of this array), and L347 (old coordinates, where updateInstitutionalAffiliation is updating the first element).

The major change made in this PR is that this component takes the entire Profile object from props and uses that as the initial state value, rather than creating its own version of an empty profile (L185).

I thought this assertion might help clarify that, plus the requirement that an institutional affiliation entry must have been pre-allocated. But maybe it just confuses things more... happy for some guidance on what changes might help a reader better understand what's going on here.

Thanks, this seems fine. I think the only thing that may be confusing here is that typically this profile is going to be passed in an empty state. My first thought on reading this is that perhaps we're expecting the affiliation to already be filled in. A comment here explaining that it will typically be just a placeholder empty affiliation might make this more clear.

calbach · 2020-02-05T18:43:46Z

ui/src/app/pages/login/account-creation/account-creation.tsx

-    this.updateInstitutionAffiliation('role', '');
-    this.updateInstitutionAffiliation('institution', '');
-    this.updateInstitutionAffiliation('other', '');
+    this.setState(currentState => {


nit: In general I think it is best practice to avoid directly mutating the innards of the state object, though this is probably a pretty innocuous case since it's happening in the state callback. I think the preferred approach here would be either lodash set or via shallow copy:

return { ...currentState.profile.institutionalAffiliations[0], role: '', institution: '', other: '' }

Thanks – I switched to use lodash here, there were a couple other places where I wasn't doing this right too.

(Side note: I'm a little unsatisfied with lodash as our solution for this pattern. I think it's some combination of (1) the painful lack of type safety, and (2) the horrible state of lodash-fp documentation. A quick bit of reading suggests https://github.com/immerjs/immer might offer a type-safe solution with nearly as concise syntax. I may track this as a new-tech opportunity for the future.)

My typical flow is:

Read normal lodash docs

Google lodash fp playground and poke at it until I can confirm understanding

Sometimes the mapping from lodash to a side-effect free functional interface is straightforward, other times it's very non-obvious.

Also there are some docs here, but I don't remember the quality:
https://gist.github.com/jfmengels/6b973b69c491375117dc

ui/src/app/pages/login/account-creation/account-creation-tos.spec.tsx

ui/src/app/pages/login/account-creation/account-creation-tos.tsx

calbach · 2020-02-06T02:26:18Z

ui/src/app/pages/login/sign-in.tsx


+export const StepToImageConfig = {
+  [SignInStep.LANDING]: {
+    backgroundImgSrc: '/assets/images/login-group.png',


There used to be images on the account creation page - was removal intentional?

No, I didn't intend to change any of the images. Is this something you're seeing when testing, or based on the code?

Nevermind. Existing behavior and it matches the updated mocks.

calbach · 2020-02-06T02:29:02Z

ui/src/app/pages/login/sign-in.tsx

-            </div>
-          </div>
+      const backgroundImages = StepToImageConfig[this.state.currentStep];
+      return <FlexColumn style={styles.signInContainer} data-test-id='sign-in-container'>


I couldn't break it - looks pretty nice to me.

ui/src/app/pages/login/sign-in.tsx

…& fix tests.

…nfig value before rendering.

…ons.

…rs in spec files.

gjuggler

Forgot to include: thanks @calbach for the helpful review!

calbach

Looks good, thanks for the thorough responses.

calbach · 2020-02-07T18:02:37Z

ui/src/app/pages/login/sign-in.tsx


+export const StepToImageConfig = {
+  [SignInStep.LANDING]: {
+    backgroundImgSrc: '/assets/images/login-group.png',


Nevermind. Existing behavior and it matches the updated mocks.

calbach · 2020-02-07T18:20:36Z

ui/src/app/pages/login/sign-in.tsx

+          zipCode: '',
+        },
+        institutionalAffiliations: [
+          // We only allow entering a single institutional affiliation from the creat account


s/creat/create/

calbach · 2020-02-07T18:26:13Z

ui/src/app/pages/login/account-creation/account-creation.tsx

 export class AccountCreation extends React.Component<AccountCreationProps, AccountCreationState> {
  private usernameCheckTimeout: NodeJS.Timer;

  constructor(props: AccountCreationProps) {
+    if (props.profile.institutionalAffiliations.length !== 1) {
+      throw new Error('Profile must be pre-allocated with 1 institutional affiliation.');


Thanks, this seems fine. I think the only thing that may be confusing here is that typically this profile is going to be passed in an empty state. My first thought on reading this is that perhaps we're expecting the affiliation to already be filled in. A comment here explaining that it will typically be just a placeholder empty affiliation might make this more clear.

gjuggler force-pushed the gj/tos-ui branch 2 times, most recently from c16aad4 to 7444007 Compare February 3, 2020 18:56

gjuggler marked this pull request as ready for review February 3, 2020 20:02

gjuggler requested a review from NehaBroad February 3, 2020 22:04

gjuggler commented Feb 3, 2020

View reviewed changes

gjuggler requested review from calbach and removed request for NehaBroad February 4, 2020 17:55

calbach reviewed Feb 5, 2020

View reviewed changes

calbach reviewed Feb 6, 2020

View reviewed changes

gjuggler added 14 commits February 6, 2020 10:56

Checkpoint on account creation cleanup.

0fd7867

Add an AccountCreationTos skeleton component.

b25963f

Use react-pdf for rendering PDFs, finish up styling of TOS page, add …

c74b3b6

…& fix tests.

Add a comment about the data-test-id prop passing.

238edfe

Add a withServerConfig HOC to ensure sign-in waits for a non-empty co…

9b50a4a

…nfig value before rendering.

Add sign-in tests to ensure account flow works with new and old versi…

f854a0d

…ons.

Point tsconfig to src/ directory as the root. This fixes some IJ erro…

3280362

…rs in spec files.

Minor change from PR self-review.

ea420df

Small tweaks to fix build.

b5b7fde

Fix a test and a DOM console warning.

bb18281

Add support for PDFs as static files.

4844bef

Remove unnecessary console.log

77dc4ce

Add typedef for StepToImageConfig.

a7de049

PR feedback: data-test-id changes, new PDF file, style fixes.

b0ccacb

gjuggler commented Feb 7, 2020

View reviewed changes

gjuggler force-pushed the gj/tos-ui branch from 7f90a23 to b0ccacb Compare February 7, 2020 14:11

gjuggler added 2 commits February 7, 2020 09:34

Imports / build fix.

3022ff0

Revert change to tsconfig.json

596d16a

calbach approved these changes Feb 7, 2020

View reviewed changes

Last bit of PR feedback cleanups.

3382e15

gjuggler merged commit c00ffc5 into master Feb 7, 2020

gjuggler deleted the gj/tos-ui branch February 28, 2020 21:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RW-4159][risk=no] Terms of Service UI and cleanup of create-account flow. #3075

[RW-4159][risk=no] Terms of Service UI and cleanup of create-account flow. #3075

gjuggler commented Feb 3, 2020 •

edited

Loading

gjuggler commented Feb 3, 2020

gjuggler Feb 3, 2020

calbach Feb 6, 2020

calbach left a comment

calbach Feb 4, 2020

gjuggler Feb 6, 2020

calbach Feb 5, 2020

gjuggler Feb 6, 2020

calbach Feb 5, 2020

calbach Feb 5, 2020

gjuggler Feb 6, 2020

calbach Feb 5, 2020

gjuggler Feb 6, 2020

calbach Feb 7, 2020

calbach Feb 5, 2020

gjuggler Feb 6, 2020

calbach Feb 7, 2020

calbach Feb 6, 2020

gjuggler Feb 7, 2020

calbach Feb 7, 2020

calbach Feb 6, 2020

gjuggler left a comment

calbach left a comment

calbach Feb 7, 2020

calbach Feb 7, 2020

calbach Feb 7, 2020

[RW-4159][risk=no] Terms of Service UI and cleanup of create-account flow. #3075

[RW-4159][risk=no] Terms of Service UI and cleanup of create-account flow. #3075

Conversation

gjuggler commented Feb 3, 2020 • edited Loading

gjuggler commented Feb 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjuggler left a comment

Choose a reason for hiding this comment

calbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gjuggler commented Feb 3, 2020 •

edited

Loading