thuang-1840-authn-prompt #1911

tihuan · 2020-10-07T03:47:32Z

https://app.zenhub.com/workspaces/single-cell-5e2a191dad828d52cc78b028/issues/chanzuckerberg/cellxgene/1840undefined

Adds /* eslint-disable camelcase -- Allowing snake_case from BE */ in client/src/components/menubar/authButtons.js <--- NO LONGER NEEDED!
Extracts LocalStorage helper functions into its own file client/src/components/util/localStorage.js, and reuse it in both cookie prompt and login prompt
Adds Popover for login prompt
Adds checkbox option to stop showing the login prompt next time the user visits the app

NOTE:

I did a slight variation of what the ticket describes, because now we're showing the <Popover /> over the Login button, so I thought adding another <Login /> button inside the <Popover /> would look weird. But please let me know what you think. Thank you!

QA steps:

Get <AuthButtons /> to show by changing the following in client/src/components/leftSidebar/topLeftLogoAndTitle.js:

BEFORE

{!userinfo.is_authenticated ? (
    <AuthButtons auth={auth} userinfo={userinfo} />
  ) : null}

AFTER

{true? (
    <AuthButtons auth={auth} userinfo={userinfo} />
  ) : null}

Play with the prompt!

Expected behaviors:

Check the checkbox and click "OK" button, refresh the page, and you should no longer see the prompt if you're not logged in
Any other conditions should still show the prompt after refreshing the page
Prompt should only show if authN is on, not logged in, and prompt LocalStorage cxg.LOGIN_PROMPT value is not "off"

UPDATE: NEW COPY

PTAL! Thank you!

tihuan · 2020-10-07T03:50:35Z

client/src/components/menubar/authButtons.js

-          data-testid="auth-button"
-          disabled={false}
-          href={!userinfo.is_authenticated ? auth.login : auth.logout}
+      <div style={{ position: "relative" }}>


Add a relative parent to wrap siblings <AuthButton /> and <div style={{ position: "absolute" }} />, so the "absolute" sibling can overlap with <AuthButton /> and position <Popover /> correctly

We can't just wrap Popover > Tooltip > AuthButton, because that disables <AuthButton />, which means clicking on the <AuthButton /> will only trigger the <Popover /> instead of navigating to the login/logout link

which means clicking on the will only trigger the instead of navigating to the login/logout link

typo :)

Ah thank you! Missing the closing ` 😆 Corrected!

codecov-io · 2020-10-07T03:55:30Z

Codecov Report

Merging #1911 into main will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1911   +/-   ##
=======================================
  Coverage   65.65%   65.65%           
=======================================
  Files          52       52           
  Lines        4234     4234           
=======================================
  Hits         2780     2780           
  Misses       1454     1454

Flag	Coverage Δ
#frontend	`100.00% <ø> (ø)`
#javascript	`100.00% <ø> (ø)`
#smokeTestAnnotations	`100.00% <ø> (?)`
#unitTest	`100.00% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6677d0d...e0cfc1a. Read the comment docs.

bmccandless

Looks good to me! Thanks @tihuan .

tihuan · 2020-10-07T16:44:18Z

Thanks so much for the quick review, Brian!!

PTAL for styling and cellxgene code best practices @pdugan20 @colinmegill @seve 😄 🙏

Thank you!

colinmegill · 2020-10-07T16:58:50Z

I think we need to clarify the copy.

Does this mean:

Log in now to save your annotations, but regardless of whether you do they'll be here when you come back
Log in now to save your annotations, so that they are here when you come back

seve

Local Storage, exciting! One small nit about ESLint. LGTM otherwise!

seve · 2020-10-07T16:41:55Z

client/src/components/menubar/authButtons.js

+        type="button"
+        data-testid="auth-button"
+        disabled={false}
+        href={!userinfo?.is_authenticated ? auth.login : auth.logout}


There is currently a bug in ESLint that's causing these lines to get errored. Instead of disabling ESLint, our current workaround is:

Suggested change

href={!userinfo?.is_authenticated ? auth.login : auth.logout}

href={!userinfo?.["is_authenticated"] ? auth.login : auth.logout}

eslint/eslint#13093

Ahh thank you! TIL! Will update!

seve · 2020-10-07T17:14:43Z

client/src/components/menubar/authButtons.js

-          data-testid="auth-button"
-          disabled={false}
-          href={!userinfo.is_authenticated ? auth.login : auth.logout}
+      <div style={{ position: "relative" }}>


which means clicking on the will only trigger the instead of navigating to the login/logout link

typo :)

lesliecodes · 2020-10-07T20:44:38Z

client/src/components/leftSidebar/topLeftLogoAndTitle.js

@@ -107,9 +107,9 @@ class LeftSideBar extends React.Component {
              userinfo,
            }}
          />
-          {!userinfo.is_authenticated ? (
+          {!userinfo.is_authenticated && (


🎉 I think we can even get rid of the ()

Suggested change

{!userinfo.is_authenticated && (

{!userinfo.is_authenticated && <AuthButtons auth={auth} userinfo={userinfo} />}

Ah good point! In this case, since prettier wraps code when the line is too long, it automatically does the following instead!

{!userinfo.is_authenticated && ( <AuthButtons auth={auth} userinfo={userinfo} /> )}

ooh nvm then!

lesliecodes · 2020-10-07T20:46:53Z

client/src/components/menubar/authButtons.js

+      <AnchorButton
+        type="button"
+        data-testid="auth-button"
+        disabled={false}


nit: I think we could just omit this if it's false

Good point! Done 😆

bmccandless · 2020-10-07T20:48:34Z

I think we need to clarify the copy.

Does this mean:

Log in now to save your annotations, but regardless of whether you do they'll be here when you come back

Log in now to save your annotations, so that they are here when you come back

How about: "Log in to create your own annotations. Please note, logging in later will refresh this page and you may lose progress."

lesliecodes

Looks great! Can we add a unit test for this? 🙏

tihuan · 2020-10-07T21:23:48Z

Thanks @lesliecodes ! cellxgene doesn't have unit test, but we can add an integration test after Seve's #1907 is merged, since it has the infrastructure code for running AuthN features locally!

lesliecodes

Thanks Timmy!

tihuan · 2020-10-07T22:03:02Z

Thanks Leslie!

I think we're close to being able to merge. Last bit on the buttons, pending decision in Slack thread 😆

tihuan · 2020-10-08T02:41:29Z

Latest iteration!

tihuan requested review from colinmegill, bmccandless and seve October 7, 2020 03:47

tihuan commented Oct 7, 2020

View reviewed changes

tihuan requested a review from pdugan20 October 7, 2020 03:50

bmccandless approved these changes Oct 7, 2020

View reviewed changes

seve approved these changes Oct 7, 2020

View reviewed changes

colinmegill approved these changes Oct 7, 2020

View reviewed changes

tihuan force-pushed the thuang-1840-authn-prompt branch from 35e5786 to c94120d Compare October 7, 2020 20:16

lesliecodes reviewed Oct 7, 2020

View reviewed changes

lesliecodes suggested changes Oct 7, 2020

View reviewed changes

lesliecodes approved these changes Oct 7, 2020

View reviewed changes

tihuan force-pushed the thuang-1840-authn-prompt branch 2 times, most recently from 5b351ad to 340c69f Compare October 8, 2020 02:22

tihuan force-pushed the thuang-1840-authn-prompt branch from 340c69f to 6fb5982 Compare October 8, 2020 23:44

thuang-1840-authn-prompt

e0cfc1a

tihuan force-pushed the thuang-1840-authn-prompt branch from 6fb5982 to e0cfc1a Compare October 8, 2020 23:47

tihuan merged commit c01a2c7 into main Oct 8, 2020

tihuan deleted the thuang-1840-authn-prompt branch October 8, 2020 23:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

thuang-1840-authn-prompt #1911

thuang-1840-authn-prompt #1911

tihuan commented Oct 7, 2020 •

edited

tihuan Oct 7, 2020 •

edited

seve Oct 7, 2020

tihuan Oct 7, 2020

codecov-io commented Oct 7, 2020 •

edited

bmccandless left a comment

tihuan commented Oct 7, 2020

colinmegill commented Oct 7, 2020

seve left a comment

seve Oct 7, 2020

seve Oct 7, 2020

tihuan Oct 7, 2020

seve Oct 7, 2020

lesliecodes Oct 7, 2020

tihuan Oct 7, 2020

lesliecodes Oct 7, 2020

lesliecodes Oct 7, 2020

tihuan Oct 7, 2020

bmccandless commented Oct 7, 2020

lesliecodes left a comment

tihuan commented Oct 7, 2020

lesliecodes left a comment

tihuan commented Oct 7, 2020

tihuan commented Oct 8, 2020

	href={!userinfo?.is_authenticated ? auth.login : auth.logout}
	href={!userinfo?.["is_authenticated"] ? auth.login : auth.logout}

	{!userinfo.is_authenticated && (
	{!userinfo.is_authenticated && <AuthButtons auth={auth} userinfo={userinfo} />}

thuang-1840-authn-prompt #1911

thuang-1840-authn-prompt #1911

Conversation

tihuan commented Oct 7, 2020 • edited

tihuan Oct 7, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 7, 2020 • edited

Codecov Report

bmccandless left a comment

Choose a reason for hiding this comment

tihuan commented Oct 7, 2020

colinmegill commented Oct 7, 2020

seve 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

bmccandless commented Oct 7, 2020

lesliecodes left a comment

Choose a reason for hiding this comment

tihuan commented Oct 7, 2020

lesliecodes left a comment

Choose a reason for hiding this comment

tihuan commented Oct 7, 2020

tihuan commented Oct 8, 2020

tihuan commented Oct 7, 2020 •

edited

tihuan Oct 7, 2020 •

edited

codecov-io commented Oct 7, 2020 •

edited