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

Simplify experience when additional scopes are required to view the dashboard #5497

Closed
aaemnnosttv opened this issue Jul 1, 2022 · 7 comments
Labels
Good First Issue Good first issue for new engineers P1 Medium priority Type: Enhancement Improvement of an existing feature

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Jul 1, 2022

Feature Description

Currently when additional scopes are required for simply displaying the dashboard, the experience is a bit disjointed, with multiple notices/errors/CTAs appearing simultaneously which makes for a bit of a chaotic UI:

image

This is easy to recreate:

  1. Setup Site Kit with no extra modules
  2. Sign in with a second admin, and connect Analytics
  3. Sign in with the first admin again, see above

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

Acceptance criteria

  • In the event of the user not having satisfied scopes, the permissions modal should not be displayed if the unsatisfied scopes that triggered the modal exactly match the unsatisfied scopes that are preloaded within _googlesitekitLegacyData.setup.unsatisfiedScopes.

Implementation Brief

  • Inside assets/js/components/PermissionsModal/AuthenticatedPermissionsModal.js:
    • Get unsatisfiedScopes by using getUnsatisfiedScopes selector
    • Return null early if all permissionsError.data.scopes exists in unsatisfiedScopes
    • If at least one scope from the permissionsError.data.scopes does not exist in the unsatisfiedScopes, then the permission modal should still be shown.

Test Coverage

  • VRT may need an update

QA Brief

  • Using recreate steps from AC make the errro occur again - the modal should not appear now

Changelog entry

  • Simplify experience when additional scopes are required to view the dashboard.
@aaemnnosttv aaemnnosttv added P1 Medium priority Type: Enhancement Improvement of an existing feature labels Jul 1, 2022
@aaemnnosttv aaemnnosttv self-assigned this Jul 1, 2022
@FlicHollis
Copy link
Collaborator

Hi @aaemnnosttv if you are not actively working on this, could you un-assign yourself so someone else can pick up? Thanks!

@aaemnnosttv aaemnnosttv removed their assignment Nov 9, 2022
@jimmymadon jimmymadon self-assigned this Nov 29, 2022
@jimmymadon
Copy link
Collaborator

@marrrmarrr @aaemnnosttv As I thought, we do have multiple "red box" errors if this happens. The UI is chaotic with the Dialog box (popup), a banner in the header and the red boxes all showing at once. We can definitely get rid of the Banner - even though it has some more detail, it pretty much conveys the same message as the error boxes. We could also get rid of the default popup that shows (although this behaviour is standard throughout the app if there is a lack of scope and we'll have to make a special case here). Then we can simply add a CTA within the error boxes. But there will be multiple CTAs in multiple error boxes - so not sure if that is "less overwhelming / disjointed".
Screenshot 2022-11-29 at 23 41 31

On a side note, the "Get help" link on this error simply takes us to the "Using the Troubleshooting mode plugin" page. Since scopes and permissions is a common error, perhaps we should have a dedicated section in the "Fixing common issues with Site Kit page". Saying this, the fix is explained in the error and is simply clicking a CTA. So simply hiding the "Get help" link here or linking it to something more generic than "Troubleshooting mode" such as "Site Kit support" may make more sense.

@jimmymadon
Copy link
Collaborator

@marrrmarrr We discussed this again in our AC Sync and decided to hide the modal and keep everything else as it is. So the banner will have a CTA and the red boxes remain unchanged.

c.c. @aaemnnosttv

@tofumatt
Copy link
Collaborator

tofumatt commented Jan 4, 2023

ACs 👍🏻

@tofumatt tofumatt removed their assignment Jan 4, 2023
@sashadoes sashadoes self-assigned this Jan 5, 2023
@sashadoes sashadoes added the Good First Issue Good first issue for new engineers label Jan 10, 2023
@sashadoes sashadoes removed their assignment Jan 10, 2023
@eugene-manuilov eugene-manuilov self-assigned this Jan 12, 2023
@eugene-manuilov
Copy link
Collaborator

  • Return null early If unsatisfiedScopes === permissionsError.data.scopes

@sashadoes since both variables are arrays, we need to check whether all scopes from the permissions error exist in the unsatisfied scopes array. If at least one scope from the error does not exist in the unsatisfied scopes, then the permission modal should still be shown.

@eugene-manuilov
Copy link
Collaborator

IB ✔️

@hussain-t hussain-t assigned hussain-t and sashadoes and unassigned hussain-t Jan 15, 2023
@sashadoes sashadoes assigned hussain-t and unassigned sashadoes Jan 16, 2023
@hussain-t hussain-t removed their assignment Jan 16, 2023
@wpdarren wpdarren self-assigned this Jan 16, 2023
@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • Using steps from AC the modal does not appear now

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers P1 Medium priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

9 participants