site-kit-wp: Invalid language in "Insufficient permissions" errors for non-authenticated users
Bug Description
When sharing a view only version of a dashboard, Analytics permission errors (and possibly others) are output in the view only dashboard. This includes for administrators (if they selected the view only dashboard) and for non administrator roles.
This is similar to what was reported previously: https://github.com/google/site-kit-wp/issues/5551
Steps to reproduce
- Setup SK and connect Analytics. When connecting Analytics connect it to a property that another user has access to.
- Ensure the dashboard loads as expected, showing Analytics data or the zero state.
- From analytics.google.com remove access to the connected property (from another Google account, alternatively delete the connected property)
- Reload the Site Kit dashboard (you may need to wait 1 hour due to caches responses. You an switch the reporting period if so)
- You’ll notice the permissions error.
- Share the dashboard (specifically Analytics) with other users (admins or other roles)
- Login as one of those users. The error appears
Screenshots
Additional Context
- SK 1.88.0
- Latest version of the Tester plugin
- For administrators the “Request access” button also appears, even though the administrators are not signed into Google
- For editors the “Request access” button does not appear
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
- The error shown for insufficient permissions on the dashboard should use alternate language for non-authenticated users:
Heading
Access lost to {Module Name}
Body
The administrator sharing this module with you has lost access to the {Google Service} service, so you won’t be able to see stats from it on the Site Kit dashboard. You can contact them or another administrator to restore access.
- For authenticated admins the language should remain as today
- The button to Request access from a report error should only be shown to authenticated admins
Implementation Brief
- In
assets/js/components/ReportError.js
:- Update the
getMessage()
function:- Before executing the existing functionality, check if the current user is a view-only user. This can be done by using the
useViewOnly()
hook. If so:- Set the
title
variable as mentioned in the ACs as the heading (translatable). - From the function, return the string as mentioned in the ACs as the body (translatable).
- Both
{Module Name}
and{Google Service}
placeholders in the ACs should be replaced with the name of the module (module.name
).
- Set the
- Before executing the existing functionality, check if the current user is a view-only user. This can be done by using the
- Inside the
return
statement, display thediv.googlesitekit-error-cta-wrapper
element only if the current user is not a view-only user. - Add a Storybook story for the case where there is a
ReportError
with insufficient permissions but for a view-only user.
- Update the
Test Coverage
- In
assets/js/components/ReportError.test.js
:- Add test case(s) to cover the above scenario.
QA Brief
- Following Steps to reproduce make the error appear
- Check new text for view-only users
- Verify the “Request access” button doesn’t appear for a non-authenticated administrator
- Verify that the language change only shows up if it is an “Insufficient permissions” error.
Changelog entry
- Fix presentation of errors from Google APIs shown to view-only users.
About this issue
- Original URL
- State: closed
- Created 2 years ago
- Comments: 17 (1 by maintainers)
Commits related to this issue
- Merge pull request #6337 from google/fix/#6201-insufficient-permissions-followup Fix logic for view-only users in "Insufficient permissions" errors — committed to google/site-kit-wp by techanvil 2 years ago
- Merge branch 'main' into fix/#6201-merge-main-to-develop. — committed to google/site-kit-wp by nfmohit 2 years ago
- Merge branch 'develop' into fix/#6201-merge-main-to-develop. — committed to google/site-kit-wp by aaemnnosttv 2 years ago
- Merge pull request #6343 from google/fix/#6201-merge-main-to-develop — committed to google/site-kit-wp by aaemnnosttv 2 years ago
Hi @mohitwp, I have taken a look but am unable to reproduce this. For me, the correct “You are probably offline” error message still appears on the view-only Dashboard. I’ve tried with both an admin and a non-admin user, here’s a screenshot for the admin user:
Please can you double-check the steps you are taking to reproduce this? If you are still able to then please document the steps to reproduce in a bit more detail.
QA Update ✅
Thank you, @techanvil. I’m also not able to replicate this error now.
Authenticated - Logged-in user Admin
Non-Authenticated user - View only dashboard
@mohitwp @nfmohit this does look like it needs to be fixed. The changes here should only affect the insufficient permissions case, not other errors.
QA Update ❌
@nfmohit
I’ve noticed that for authenticated users if user lost access to internet then the error description is correct -
But for non authenticated users (View -only dashboard) error description is same as we are showing in case of insufficient permission error and Retry button is not showing. Also, the title is not bold.
Authenticatd user—Signed in Admin
Non Authenticated user (View only dashboard)
**On Latest- error appears in case of Data error.
@sashadoes @nfmohit @techanvil Looking at https://github.com/google/site-kit-wp/pull/6292/files#diff-b95a49b3fe9c5fd7c5b5f782eef76c5c71fd74299f4bdce4a7156144db48d621R90, I’m questioning whether the logic there is right? The ACs state that “The error shown for insufficient permissions” should change. But the logic in the PR puts the check before the check for whether the error even is an insufficient permissions error.
To be fair, that part already seems incorrect in the IB. Please take a look.
cc @eugene-manuilov @aaemnnosttv