Duplicate error-related messaging in Analytics settings when admin does not have access

[aaemnnosttv]

Bug Description

In #4825, we updated the edit interface for module settings to only allow editing module settings if the current user has access to the currently configured entity the module is connected with. However, requests are still being made for properties and profiles

Steps to reproduce

1. Set up Analytics using an account that another Google account does not have access to
2. Sign in with the second admin which lacks access
3. Edit settings for Analytics
4. See error message and info notices about lack of access

---

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

Acceptance criteria

• When an user does not have access to the configured Analytics entity, there should not also be errors displayed about that same lack of access
  • Selectors that would be known to invoke such failing requests that will fail should be prevented from running

Implementation Brief

• Within assets/js/modules/analytics/components/common/ProfileSelect.js, PropertySelect.js and assets/js/modules/analytics-4/components/common/PropertySelect.js:
  • ~~Default the hasModuleAccess prop to true (for clarity purposes as per the comments below).~~
  • Wrap the calls to select() the getProperties() and getProfiles() selectors with a check for when hasModuleAccess is specifically false. If it is, return null. E.g.

      const profiles = useSelect( ( select ) => {
  if ( hasModuleAccess === false ) {
  	return null;
  }
  return select( MODULES_ANALYTICS ).getProfiles(
  	accountID,
  	propertyID
  );
   );
  

Test Coverage

• Add coverage to existing JS (RTL) tests for the above components when hasModuleAccess is false (or undefined).

QA Brief

Changelog entry

[techanvil]

Hi @jimmymadon , a small but important detail - looking at the components mentioned in the IB, the hasModuleAccess prop is being explicitly checked for === false, rather than just for being falsey, so this should also be the case for the new hasModuleAccess checks described in the IB.

Also - in each case, when hasModuleAccess is false, the properties or profiles value is disregarded, so it's not really necessary to set to [] - I know this might be splitting hairs but I'd argue that setting to [] gives the impression it's intended to be used, maybe undefined would be better?

One final point, seeing as there are JS tests for each of the components, but presently no test for the hasModuleAccess === false case, I would suggest adding tests for each component to cover this case...

[jimmymadon]

@techanvil All good points - thanks! Have updated the IB. Also, 7 hours should still be a good estimate?

[techanvil]

Thanks @jimmymadon - the wording is good, to specify checking specifically for false. However, the code example is still not actually doing that - it's only checking for a truthy value, which means a falsey value of getModuleAccess will still result in the select being called.

The reason this distinction is important is that getModuleAccess doesn't have a default value set, but the default behaviour is for it to be assumed "true" unless explicitly set to false.

Maybe a better approach, in fact, would be to add a defaultProps = { getModuleAccess: true } for the components in question, as that's the implicitly desired behaviour... Basically at the moment if hasModuleAccess is true or undefined, it's being treated as "true". So setting it as a default would be much more self-descriptive.

One more point, and apologies for not realising this the first time around, but as undefined is used to indicate an unresolved selector, maybe it would be better instead to return null instead of undefined ?

Regarding the estimate - tbh looking at the IB as it stands now, I don't think 7 is too much of a stretch, what do you reckon? Bearing in mind the estimate is a range of 4-7, I think it was previously probably more at the lower end of the range, but now it could be at the higher end... I see no problem in bumping it up though if you think it's worth erring on the safe side...

[jimmymadon]

@techanvil - Sorry I missed out the "=== false" bit. (I should've copy-pasted it from my editor). As per this comment, we decided not to have a default true value to reduce the number of changes (if I remember correctly).

The reason this distinction is important is that getModuleAccess doesn't have a default value set, but the default behaviour is for it to be assumed "true" unless explicitly set to false.

I think this makes sense so lets go with adding a default value of true.

Returning null is fine too. In PropertySelect, the default value for properties was an empty array, hence I thought it should be fine to keep it so.

And exactly my thoughts on the estimate. Thanks!

[jimmymadon]

Thanks @techanvil - have updated the IB as per our discussions.

[techanvil]

Thanks @jimmymadon!

IB :white_check_mark:

[jimmymadon]

@techanvil I had a quick chat with @aaemnnosttv who reminded me that we didn't default hasModuleAccess to be true because, as we know, when the hasModuleAccess selector in the parent component hasn't been resolved, this prop would be passed down as undefined, which does not technically equate to true. The fact that we currently treat undefined and true the same way in these child components is a bit confusing - however, this works for now because we show Progress Bars instead of rendering any of these child <*Select> components when the selectors within <SettingsForm> are yet to be resolved. So if we were to use any of these common <*Select> components in the future where this is not the case, having a default value of true for this prop would essentially imply that the select input is accessible instead of implying that the component is still loading (and potentially could be disabled).

Hence it might be worth not to default this prop to true.

[techanvil]

@jimmymadon @aaemnnosttv thanks for the explanation here, I guess it does make sense to leave it without the default then...

[mohitwp]

QA Update ✅

Verified 👍

• Followed the steps to reproduce the issue and able to reproduce issue.
• Verified when google a/c have analytics access. User is able to edit analytics settings and no error notice is showing.
• Verified when google a/c doesn't have analytics access. User is not able to edit analytics settings and there are no errors displaying about that same lack of access and no requests are getting made.
• Also, verified for the scenario if user had previously access to analytics and if owner of the analytics remove that access for the user. In this case if user already logged in Site Kit Dashboard and tried to edit settings without refreshing the page then in this case notice appear and user not able to edit settings which is expected.

After refreshing or login again

https://user-images.githubusercontent.com/94359491/188546437-a06bf040-fd6a-42f2-88c2-c2b410420818.mp4

Without Refresh

https://user-images.githubusercontent.com/94359491/188546497-5eff861c-1a84-4176-ad61-0d2d864968a9.mp4