Improve experience with recoverable modules for view-only users

[eclarke1]

Feature Description

Bug bash issue: https://app.asana.com/0/1202258919887896/1202445528223671 please refer to Asana issue for background

Improve the messaging & experience if an admin has connected a module, shared access, and then disconnects. If an admin has connected a module (ie. Analytics), shared view only access with other roles (include admins), and then disconnects they'll be presented with an option to "Set Up" Site Kit or to access "View only" data.

If they choose to only view the data they'll encounter the below message, which isn't the most suitable (screenshot):

Data error in Analytics
Site Kit can’t access the relevant data from Analytics because you haven’t granted all permissions requested during setup.

The reason is not suitable is that the user did provide access previously but they disconnected. They do actually have access to the Analytics property but they need to re-do the plugin setup, and there is no obvious link to do so (after choosing the "view only" dashboard).

The only way for the admin to re-do plugin set up is to click on the "View only" link and then "Sign in with Google" option. This isn't very obvious, and would likely lead to many admins resetting the plugin.

Recording of experience

Improved recording of experience - showing all 3 CTA's on the dashboard after disconnecting

Steps to recreate:

1. Connect SK with Google Analytics
2. Share Google Analytics with all roles
3. Disconnect from Site Kit
4. You'll be present with the option to set up Site Kit once more, this time with 3 options (which can be confusing)
5. Select the option to "View limited data"
6. You'll encounter the error, with no obvious way out other then that mentioned above

Related to #5354

---

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

Acceptance criteria

• When a module is in a recoverable state, all widgets associated with it (via their registration) should be replaced by a new component when viewed in the context of the view-only dashboard.
  • The recoverable state should look similar to the Data Unavailable state of the widget but has different title and message.
  • The title should be Data Unavailable
  • The message should be {Module} data was previously shared by an admin who no longer has access. Please contact another admin to restore it.
    • For widgets that require multiple recoverable modules, the message should read slightly differently: The data for the following modules was previously shared by an admin who no longer has access: {Module1, Module2, ...}. Please contact another admin to restore it.
• The new component when rendered should be treated as a special widget state, similar to gathering data, which when multiple adjacent widgets render the same state, they are combined into a single widget automatically
• Unlike other special widget states, this should be handled outside of the registered widget components but handled automatically by the widget infrastructure based on a widget's registered modules and the current set of recoverable modules

Implementation Brief

• Add a new RecoverableModules component which takes moduleSlugs (string[] module slugs) as a prop
  • Renders a CTA with the title and message as defined in the AC informing about the recoverable modules
• Add a new WidgetRecoverableModules component, similar to WidgetReportError which renders RecoverableModules (passing through its props to it) and uses the useWidgetStateEffect hook internally to set the widget state on mount using RecoverableModules as the component and moduleSlugs as the metadata
• Update WidgetRenderer to conditionally override the widgetElement with a WidgetRecoverableModules if widget.modules contains any recoverable modules, if so, passing the intersection of module slugs as props.moduleSlugs to WidgetRecoverableModules
  • Note this should only happen in a view-only context (useViewOnly)
• Add the RecoverableModules to the list of special widget states

Test Coverage

• Add Jest tests to cover the basics of this functionality (essentially the alternate component rendered in place of the widget's registered component);
• A basic test covering widget combination for adjacent widgets with the same state + metadata

QA Brief

• Site up Site Kit and enable the dashboardSharing feature flag.
• Put the Search Console and PageSpeed Insights modules into the recoverable state. This can be achieved by sharing them as a user and then disconnecting that user from Site Kit.
• View the Dashboard in view-only mode with a user whose role the modules were shared with.
• Widgets for the recoverable modules should be rendered using the RecoverableModules component:

Changelog entry

• Update view-only dashboard to use a new placeholder for widgets that rely on recoverable modules.

[eugene-manuilov]

Copying over Evan's comment from the asana ticket:

This has a few interesting cases to think through.

When a module owner disconnects, if the module was shared, it would become recoverable.

Currently, recoverable modules are still included in the view only dashboard. Perhaps they shouldn't be, because the CTA might be difficult to provide good guidance for (if the module owner is gone, we would have to direct them to another authenticated admin to recover the module).

If we decide to exclude recoverable modules from the shared dashboard, then it's possible for view-only users to land on an empty dashboard (if all the viewable modules are recoverable), in which case we'd need some kind of "oops, there's actually nothing here for you" state.

One idea might be to unshare all the modules owned by a user when they disconnect, but the above would still be relevant because a module can become recoverable in other ways too.

[eugene-manuilov]

@aaemnnosttv what do you think if we update widgets to display a special state for recoverable modules instead of hiding widgets? In this case, a widget will say that the corresponding module has been configured by an admin that has disconnected his account and the module needs to be re-configured to continue showing their widgets. It will explain the problem and motivate users to reach out to their admins to re-configure the module. What do you think?

cc @felixarntz @marrrmarrr

[aaemnnosttv]

what do you think if we update widgets to display a special state for recoverable modules instead of hiding widgets?

@eugene-manuilov I think this is the best approach for now.

[eclarke1]

Assigning back to @eugene-manuilov to please add AC

[eugene-manuilov]

Thanks, @aaemnnosttv. Does AC look good to you?

[aaemnnosttv]

Thanks @eugene-manuilov! This is a good start although this issue is specific to the experience for view-only users. For authenticated admins, they may have access themselves and on the backend if the module is recoverable, the request will be made with the current user's token instead of the owner. E.g. if SC was shared with admins and then became recoverable, all admins signed in with Google should still be able to see the data via their own access. Authenticated admins will already see the notice to recover any recoverable modules as well so we don't need to duplicate this information in the widgets as well.

[felixarntz]

@aaemnnosttv I only have higher-level feedback on the IB: I'm wondering, shouldn't we rather make this RecoverableModule and have it per module? I realize that it could be multiple modules that need to be recovered, but I think together with that combination logic that approach makes more sense to me since not all modules may need to be recovered at the same time.

So I think the per-module approach is more targeted and also better in line with the other special states we have so far which are typically per module. What do you think about that?

[aaemnnosttv]

shouldn't we rather make this RecoverableModule and have it per module? I realize that it could be multiple modules that need to be recovered, but I think together with that combination logic that approach makes more sense to me since not all modules may need to be recovered at the same time.

@felixarntz The problem I see is that this component would replace a widget which doesn't necessarily have a 1:1 relationship with a module (even though most do). If we made it singular to one module, if a widget had multiple recoverable modules it makes sense to surface all of those dependencies, otherwise a user may recover a single module and then still not see the module they are trying to "unlock". Otherwise it could result in a kind of moving target.

If a module isn't truly necessary for a widget to function, it probably shouldn't declare it in its modules when registering. Similar to the concept of "viewable modules", a user needs to have read access to all modules a widget is associated with to see it on the shared dashboard.

I think together with that combination logic that approach makes more sense to me since not all modules may need to be recovered at the same time.

Each widget would only surface the recoverable modules it depends on, not all recoverable modules on the dashboard. Maybe that part wasn't clear?

[felixarntz]

@aaemnnosttv

Each widget would only surface the recoverable modules it depends on, not all recoverable modules on the dashboard. Maybe that part wasn't clear?

Ahh, that makes total sense. I hadn't thought about that a widget can be associated with multiple modules. If we only group them based on the modules that each individual widget is tied to, then this sounds good.

IB ✅

[tofumatt]

Oh, looks like this is good to go into the backlog, maybe there was a Zenhub error here.

I had a look over and this looks good though, so another IB ✅

[techanvil]

Hi @aaemnnosttv, as I'm implementing this I've run into a slight hiccup with regard to the requirement to combine the widgets.

At present the shouldCombineAllWidgets function is designed to only combine widgets which share the same module (singular), and it accordingly checks metadata.moduleSlug for the associated module.

site-kit-wp/assets/js/googlesitekit/widgets/util/combine-widgets.js

Line 57 in bec8b62

const module = widgetState?.metadata?.moduleSlug;

However this issue introduces a new metadata item, a list of modules, metadata.moduleSlugs.

I suspect we should amend shouldCombineAllWidgets to handle either moduleSlug or moduleSlugs, so widgets can be combined if they share the same set of modules rather than singular module.

Does this sound right to you? It seems fairly trivial but I didn't want to just go ahead with the change unilaterally as it's modifying the stated intent of the function, and there are also tests to consider...

[aaemnnosttv]

Thanks for raising this @techanvil – I think at one point the engine combined widgets based on equivalent metadata rather than checking specific properties within it so I missed this detail when writing the IB.

In the interest of not increasing the scope here (and because most widgets have only a single module), I would suggest we join the widget's modules into a single moduleSlug string when constructing the metadata, essentially shoehorning it into the current logic. This is a bit of a hack but it should be harmless and have the intended effect for most widgets. We can then open a follow-up issue for how we want to handle this for multi-module widgets.

[techanvil]

Thanks @aaemnnosttv, sounds like a plan.

Update: I have created a follow-up issue for the multiple modules case: #5466

[mohitwp]

QA Update ✅

Verified :