Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conditionally render authentication-dependent components for authenticated users #4819

Closed
aaemnnosttv opened this issue Feb 8, 2022 · 9 comments
Labels
P0 High priority Type: Enhancement Improvement of an existing feature

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Feb 8, 2022

Feature Description

Historically, Site Kit has always been limited to authenticated users, but with dashboard sharing this will no longer be the case so some components will need to be rendered conditionally that weren't before.

The Root component currently renders the PermissionsModal component which is only relevant for authenticated users because in the context of this component, "permissions" specifically refer to oAuth scopes which implies Google authentication. Going forward, this should only be rendered if the user is authenticated.

Similarly, the UnsatisfiedScopesAlert component is only relevant to render for authenticated users as well.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The following components should only be rendered if the current user is authenticated (signed-in with Google):
    • PermissionsModal
    • UnsatisfiedScopesAlert

Implementation Brief

  • Using assets/js/components/Root/index.js, render the PermissionsModal component only if the user has been authenticated.
  • Using assets/js/components/notifications/ErrorNotifications.js, render the UnsatisfiedScopesAlert component only if the user has been authenticated.
  • To check if the user has been authenticated in above components, query the user data store via the isAuthenticated selector.

Test Coverage

  • Add tests for the above components to make sure the the components are rendered/not rendered if the user is authenticated or not.

QA Brief

  • Make sure that PermissionModal and UnsatisfiedScopesAlert continues appearing like before. ie. appears for the authenticated user in the situations they should appear.
  • Make sure the regression spotted by @aaemnnosttv here is fixed.

Changelog entry

  • Ensure permissions modal only appears for authenticated users.
@aaemnnosttv aaemnnosttv added P0 High priority Type: Enhancement Improvement of an existing feature labels Feb 8, 2022
@aaemnnosttv aaemnnosttv self-assigned this Feb 8, 2022
@aaemnnosttv aaemnnosttv removed their assignment Mar 23, 2022
@aaemnnosttv aaemnnosttv changed the title Conditionally render PermissionsModal for authenticated users Conditionally render authentication-dependent components for authenticated users Mar 24, 2022
@asvinb asvinb assigned asvinb and unassigned asvinb Mar 24, 2022
@eugene-manuilov eugene-manuilov self-assigned this Mar 25, 2022
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Mar 25, 2022
@kuasha420 kuasha420 self-assigned this Apr 11, 2022
@kuasha420
Copy link
Contributor

@aaemnnosttv There's a little discussion regarding it on the Slack, just posting it here as well. As it's not possible to call useSelect in the Root component, will checking the authentication status inside the PermissionsModal and return null there if user is unauthenticated work here?

Cheers.

@aaemnnosttv
Copy link
Collaborator Author

will checking the authentication status inside the PermissionsModal and return null there if user is unauthenticated work here?

@kuasha420 that might be fine too – the only thing is that there may be side-effects from running resolvers or other hooks within PermissionsModal that we might want to avoid, in which case we could create an HOC or wrapper component around it to use within Root instead of using the PermissionsModal directly. We would just need an additional layer around that component that would allow for using useSelect to facilitate the conditional rendering.

@kuasha420
Copy link
Contributor

@aaemnnosttv I like that, will do! Thanks.

@kuasha420 kuasha420 removed their assignment Apr 12, 2022
@tofumatt tofumatt assigned tofumatt and kuasha420 and unassigned tofumatt Apr 12, 2022
@kuasha420 kuasha420 assigned tofumatt and unassigned kuasha420 Apr 12, 2022
@tofumatt tofumatt removed their assignment Apr 12, 2022
@aaemnnosttv
Copy link
Collaborator Author

QA ❌

37101d9 introduced a regression where loading the dashboard without a # fragment in the URL results in the page scrolling (to what seems to be the #traffic element).

image

This can be observed by loading the dashboard and then clicking "Dashboard" in the admin menu.

@kuasha420 kuasha420 assigned aaemnnosttv and unassigned kuasha420 Apr 15, 2022
@aaemnnosttv aaemnnosttv removed their assignment Apr 15, 2022
@wpdarren wpdarren self-assigned this Apr 18, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Apr 18, 2022

QA Update: ⚠️

@kuasha420 I can see the PermissionModal and UnsatisfiedScopesAlert components using the React developer tools plugin but feel a little more clarification on the QAB is needed:

ie. appears for the authenticated user in the situations they should appear.

I am seeing the components on the main dashboard with the dashboardSharing enabled and disabled. Is there anywhere that I should check to make sure that they appear as expected. Also, on the components, there's a message about them not in Strict mode. Unsure if that is expected (see screenshots)

@aaemnnosttv
Copy link
Collaborator Author

ie. appears for the authenticated user in the situations they should appear.

I am seeing the components on the main dashboard with the dashboardSharing enabled and disabled. Is there anywhere that I should check to make sure that they appear as expected. Also, on the components, there's a message about them not in Strict mode. Unsure if that is expected (see screenshots)

@wpdarren The changes here should be more/less unnoticeable as this mostly affects unauthenticated views which would only be the splash page for now. It's worth noting that PermissionsModal will be shown everywhere as before, it's the new AuthenticatedPermissionsModal which will be conditionally shown based on the user's authentication status.

Also, on the components, there's a message about them not in Strict mode. Unsure if that is expected (see screenshots)

We haven't integrated with Strict mode yet so you'll see the same notice from before as well, so not to worry there.

@kuasha420
Copy link
Contributor

@wpdarren I think Evan answered all your questions perfectly and I have nothing more to add here. This felt kinda like QA:Eng as we don't have much setup for non authenticated user, but just being careful about the QA:Eng trap here.

Also, yea, my bad about the PermissionsModal, is should've been AuthenticatedPermissionsModal inside the PermissionsModal component.

Cheers.

@kuasha420 kuasha420 removed their assignment Apr 19, 2022
@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

On the splash page.

  • The AuthenticatedPermissionsModal inside the PermissionsModal component appears for an authenticated user.

  • The UnsatisfiedScopesAlert continues appearing.

  • The issue reported by Evan is fixed. Checked with ZDS enabled and disabled.

Screenshots

image

image

@wpdarren wpdarren removed their assignment Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants