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

Widgets don't load when URL has no initial hash and traffic section isn't available #5388

Closed
aaemnnosttv opened this issue Jun 17, 2022 · 4 comments
Labels
P0 High priority Type: Bug Something isn't working

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Jun 17, 2022

Bug Description

The dashboard navigation introduced with the unified dashboard controls the hash in the URL and if not set, sets it to the fist section: #traffic. Until now, this has worked fine because all users are guaranteed to have that section available. With dashboard sharing, it's possible that the Traffic section may not be present if Search Console and Analytics are not shared with a user's role.

In this situation, a URL without a hash will still get #traffic set by default which also has the effect of "activating" the useInViewSelect hooks that all API/reporting selectors use for lazy-loading. This results in the first actually-in-view section not being active.

This is not 100% reproducible and may also involve a race condition or other factors like not scrolling after the initial load.

Steps to reproduce

  1. Setup Site Kit and share only Idea Hub with a non-admin role
  2. Sign in as the non-admin user and go to the SK dashboard
  3. See Idea Hub stuck in a loading state
  4. Click the Content section tab to activate it
  5. See Idea Hub load as usual

Screenshots


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

Acceptance criteria

  • If no hash is present in the URL on the SK dashboard, the initial hash should be set to the first available section
    Note this may not be available right away so async resolution may be needed

Implementation Brief

  • Move and rename the following files to the new folder assets/js/components/DashboardNavigation.
    • assets/js/components/DashboardNavigation.js to assets/js/components/DashboardNavigation/Navigation.js, renaming the component in the process.
    • assets/js/components/DashboardNavigation.stories.js to assets/js/components/DashboardNavigation/index.stories.js
    • assets/js/components/DashboardNavigation.test.js to assets/js/components/DashboardNavigation/index.test.js
  • Create new file assets/js/components/DashboardNavigation/index.js which does the following:
    • Get the viewable modules via the getViewableModules selection of the core/user data store. Refer to viewableModules in assets/js/components/DashboardNavigation/Navigation.js
    • Return null if getViewableModules is undefined, else return Navigation imported from assets/js/components/DashboardNavigation/Navigation.js
  • Using assets/js/components/DashboardNavigation/Navigation.js,
    • Add a new function getDefaultChipID which does the following:
      • If viewOnlyDashboard is false, return ANCHOR_ID_TRAFFIC
      • if showTraffic is true, return ANCHOR_ID_TRAFFIC
      • if showContent is true, return ANCHOR_ID_CONTENT
      • if showSpeed is true, return ANCHOR_ID_SPEED
      • if showMonetization is true, return ANCHOR_ID_MONETIZATION
    • Add another function isValidChipID which accepts a parameter, chipID, which returns true based on the following:
      • if showTraffic is true and chipID is ANCHOR_ID_TRAFFIC
      • if showContent is true and chipID is ANCHOR_ID_CONTENT
      • if showSpeed is true and chipID is ANCHOR_ID_SPEED
      • if showMonetization is true and chipID is ANCHOR_ID_MONETIZATION
      • else return false.
    • Update the code within useMount,
      • when there's no initialHash, instead of hard coding the hash to ANCHOR_ID_TRAFFIC, get the default chip ID from getDefaultChipID and update the hash to the default chip ID.
      • if there is an initial hash, check if it's a valid chip ID via the isValidChipID. If the chip ID is not valid, update the hash to the default chip ID.

Test Coverage

  • Add new tests assets/js/components/DashboardNavigation/index.test.js to make sure the first chip ID is selected when no hash is selected

QA Brief

  • Enable Dashboard sharing and share only PageSpeed with all other users.
  • Sign in as a user who can view a shared dashboard with only PageSpeed Insights shared.
  • Navigate to the Site Kit dashboard.
  • Notice the URL hash is #speed not #traffic.

Using a normal Site Kit admin, the URL hash should still start with #traffic when viewing the Site Kit dashboard.

Changelog entry

  • Fix default Dashboard Navigation section for the view-only Dashboard.
@aaemnnosttv aaemnnosttv added Type: Bug Something isn't working P0 High priority labels Jun 17, 2022
@asvinb asvinb assigned asvinb and unassigned asvinb Jun 20, 2022
@tofumatt tofumatt self-assigned this Jun 22, 2022
@tofumatt
Copy link
Collaborator

Given the unit tests, component re-shuffling, and QA involved in this I feel like it should be bumped to an 11 estimate just in-case for planning, but other than that looks good.

IB ✅

@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Jun 22, 2022
@tofumatt tofumatt removed their assignment Jun 24, 2022
@asvinb asvinb assigned asvinb and unassigned asvinb Jun 24, 2022
@techanvil techanvil assigned techanvil and unassigned techanvil Jun 27, 2022
@wpdarren wpdarren self-assigned this Jun 27, 2022
@wpdarren
Copy link
Collaborator

QA Update: ⚠️

@tofumatt this passes the QAB, but going back to the original issue with Idea Hub, I decided to run through some additional testing. The Idea Hub widget did load with the initial content hash. I noticed two console errors though that don't appear when PageSpeed is the only permitted module.

/index.php/wp-json/google-site-kit/v1/modules/search-console/data/searchanalytics?dimensions=date&startDate=2022-05-02&endDate=2022-06-26&_locale=user:1 Failed to load resource: the server responded with a status of 403 ()

googlesitekit-api-dafe70321f686df3fda1.js:3 Google Site Kit API Error method:GET datapoint:searchanalytics type:modules identifier:search-console error:"Site Kit can’t access the relevant data from Search Console because you haven’t granted all permissions requested during setup." (anonymous) @ googlesitekit-api-dafe70321f686df3fda1.js:3

image

Should these errors be in appearing in the console? 🤔

@tofumatt
Copy link
Collaborator

@wpdarren Those errors, I believe, are what's being worked on in #5310… or possibly another permissions issue… there's a few of them! 😅

They're bugs but should be covered elsewhere I think. So I think this should be considered done if it passes the QA Brief, and permissions issues should be filed separately.

If that one isn't covered by any open issues in the Dashboard Sharing epic, please do add it to the list and assign me to define the ACs 👍🏻

@tofumatt tofumatt removed their assignment Jun 27, 2022
@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • URL hash appears for #speed as a user who can view a shared dashboard with only PageSpeed Insights shared.
  • I also tested the scenario in the initial description around Idea Hub and the widget loaded as expected. There are two console errors but it appears these will be picked up as part of Requests for settings fail for view-only users on dashboard #5310.
  • Using a normal Site Kit admin, the URL hash starts with #traffic when viewing the Site Kit dashboard.
Screenshots

image
image

@wpdarren wpdarren removed their assignment Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

6 participants