google / site-kit-wp

Site Kit is a one-stop solution for WordPress users to use everything Google has to offer to make them successful on the web.
https://sitekit.withgoogle.com
Apache License 2.0
1.21k stars 278 forks source link

Improve module Edit Settings form when owner loses access to the module #7588

Open jamesozzie opened 10 months ago

jamesozzie commented 10 months ago

Bug Description

If an user doesn't have access to the connected SC property, they'll see a warning on the Site Kit dashboard as per the below:

image

If they try to edit the Search Console module, and if this is the only Search Console property available, there will be a dropdown with nothing available for selection. It's not possible to confirm any changes.

image

Improve this scenario by adding some textual context, or a "Request accsses" button as it appears on the dashboard.

Steps to reproduce

  1. Set up Site Kit with Analytics on a site that already is verified in Search Console (ie. http://mysite.com). Use this verified property alone, with no other verified versions of that site (ie. the insecure property or the www prefixed property are not verified independently of Site Kit). Site Kit will use this existing verification method.
  2. Ensure data appears on the Site Kit dashboard.
  3. Access Search Console and unverify your Google account from the property.
  4. Revisit your Site Kit dashboard. An expected Search Console permissions error will appear, with a request access button.
  5. Visit the Search Console settings page and a a dropdown will appear which isn't very informative of the issue.

Screenshots

Additional Context


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

Acceptance criteria

Implementation Brief

Test Coverage

QA Brief

Changelog entry

jimmymadon commented 3 weeks ago

I have renamed this issue as this affects all modules and not simply Search Console. It is possible that the current owner of a module, i.e. the person that first set up the module, can lose access to the module for which the "Insufficient Permissions" errors are displayed in widgets on the dashboard.

However, when VIEWING settings, certain modules like Analytics do throw an error whereas Search Console does not as the current saved settings are simply displayed from the database.

Screenshot 2024-06-17 at 22 25 14

Now, when EDITING settings, all drop-downs end up being blank and there is no proper error message on some forms.

Screenshot 2024-06-17 at 22 25 39 Screenshot 2024-06-17 at 22 26 14

If the logged in user is not the owner of a module, we always display a well designed error message and simply display the saved settings values (and no additional information which cannot be fetched if the user has no permissions).

Screenshot 2024-06-17 at 22 14 38

This is because in https://github.com/google/site-kit-wp/issues/4825 and other related issues, we first check if the current logged in user is the owner of a module. If they are, we do NOT check if the user has access to this module or not. This is then used to display the above behaviour (error + saved settings values only).

The module owner check was done mainly to avoid changing all our tests/E2E tests (see point 2 in this comment). Should we handle the case where the current logged in user themselves don't have access?

tofumatt commented 2 weeks ago

@jimmymadon Checking to ensure the current user doesn't have access makes sense. It'd be great if we could do it only when there's an error if it'll save an otherwise redundant request (that's probably rare), but that's more of an IB detail.

I think it makes sense to write up ACs for that scenario 👍🏻

jimmymadon commented 1 week ago

@tofumatt Thanks - I've drafted some ACs here. c.c.ing @aaemnnosttv as he drafted the original ACs for #4825 and advocated for a "module owner" check BEFORE doing a "module access" check.

eclarke1 commented 1 week ago

Can this please have a priority label added @jimmymadon or @tofumatt thank you