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

Expose recoverable modules to client #4527

Closed
aaemnnosttv opened this issue Dec 9, 2021 · 10 comments
Closed

Expose recoverable modules to client #4527

aaemnnosttv opened this issue Dec 9, 2021 · 10 comments
Labels
P0 High priority PHP Type: Enhancement Improvement of an existing feature

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Dec 9, 2021

Feature Description

Module recovery is a new concept in dashboard sharing whereby a shared module can be recovered in the event that the module owner is lost. Later, we'll use this data to alert admins and prompt those who are eligible to recover the module.


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

Acceptance criteria

  • The googlesitekit-dashboard-sharing-data script data should be extended to include a new recoverableModules property as a list of module slugs (string[]) that are recoverable
  • A recoverable module is a shareable module that meets any of the following criteria:
    • Has no owner identified by its ownerID
    • The user identified by the module's ownerID no longer (one of the following)
      • exists
      • is authenticated
      • lacks the necessary capability to share (i.e. AUTHENTICATE)
  • Note one or more of these cases could probably be covered by some overarching logic but it's important that all are accounted for

Implementation Brief

In Core/Assets/Assets.php update get_inline_dashboard_sharing_data() $inline_data add a property recoverableModules which will be an array of module strings.
First fetch the list of shareable modules get_shareable_modules() (introduced in #4521),

For each shareable module get the owner id get_owner_id()

public function get_owner_id();

If the owner ID doesn't correspond to a valid user ( get_user_by( 'ID', $owner_id ) )
https://developer.wordpress.org/reference/functions/get_user_by/
Add the module slug to the recoverableModules array.

If the owner is not authenticated, add the module slug to the recoverableModules array.

See

public function is_authenticated() {

Also ensure you're checking the module owner, and not the current user, code snippet:

$original_user_id = $this->user_options->get_user_id();
$this->user_options->switch_user( $user_id );
if ( $this->is_authenticated() ) {
$this->get_oauth_client()->refresh_profile_data( 30 * MINUTE_IN_SECONDS );
}
$this->user_options->switch_user( $original_user_id );

If the owner ID does not have the AUTHENTICATE capability, add the module slug to the recoverableModules array
user_can( $owner_id, Permissions::AUTHENTICATE );
https://developer.wordpress.org/reference/functions/user_can/

Test Coverage

  • N/A

QA Brief

  • Enable dashboardSharing via the tester plugin
  • Make sure you are in the Site Kit dashboard page
  • Open the browser developer console and type window._googlesitekitDashboardSharingData
  • Verify that you should get an array of roles and recoverableModules
  • Verify the recoverableModules array should have the module slugs if the criteria is met as mentioned in the AC
  • Refer the screenshot attached to the PR description
  • Disable dashboardSharing via the tester plugin
  • Type window._googlesitekitDashboardSharingData in the browser developer console
  • Verify that you should get undefined as a value of the above property

Update:

  • Mostly, the recoverable modules would be PSI and Idea Hub if active

Changelog entry

  • Expose recoverable modules information to clients.
@aaemnnosttv aaemnnosttv added P0 High priority Type: Enhancement Improvement of an existing feature PHP labels Dec 9, 2021
@aaemnnosttv aaemnnosttv self-assigned this Dec 9, 2021
@felixarntz felixarntz removed their assignment Dec 20, 2021
@eugene-manuilov eugene-manuilov self-assigned this Dec 22, 2021
@eugene-manuilov
Copy link
Collaborator

IB ✔️

@wpdarren
Copy link
Collaborator

wpdarren commented Jan 25, 2022

QA Update: ⚠️

@hussain-t @eugene-manuilov there are 4 recoverableModules, which are connected. I also have Optimize and Tag Manager connected on this site, should I see them in the list of modules too?

0: "search-console"
1: "adsense"
2: "analytics"
3: "pagespeed-insights"

image

@wpdarren wpdarren assigned hussain-t and unassigned wpdarren Jan 26, 2022
@aaemnnosttv
Copy link
Collaborator Author

@hussain-t I found the problem here – see #4670 (comment)

Would you please open a follow-up PR to fix it?

@wpdarren under normal circumstances, no modules should be shown here although due to the current definition of a recoverable module pagespeed-insights qualifies for this because it has no ownerID until it is shared. I'll open a follow-up issue to refine this logic but PSI (and Idea Hub when active) would be the only exceptions that are expected here.

To test a condition where a module would become recoverable, set up Analytics with a different user. Then change that user's role to be a non-admin. Analytics should then show up as a recoverable module.

@hussain-t
Copy link
Collaborator

@aaemnnosttv sure, I will do that. Thanks for the investigation!

@hussain-t
Copy link
Collaborator

@aaemnnosttv @wpdarren I have created a follow-up PR and updated the QAB.

@aaemnnosttv
Copy link
Collaborator Author

Thanks @hussain-t, please see my review as there are a few things there left to address.

@hussain-t hussain-t assigned aaemnnosttv and unassigned hussain-t Jan 27, 2022
@aaemnnosttv aaemnnosttv removed their assignment Jan 27, 2022
@wpdarren wpdarren self-assigned this Jan 27, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Jan 27, 2022

QA Update: ⚠️

@aaemnnosttv re. this:

To test a condition where a module would become recoverable, set up Analytics with a different user. Then change that user's role to be a non-admin. Analytics should then show up as a recoverable module.

I was going to run through this test but before even setting it up, I noticed Analytics, AdSense, Search console appear in the recoverableModules and from your previous comment, I am assuming this is not correct.

I'll open a follow-up issue to refine this logic but PSI (and Idea Hub when active) would be the only exceptions that are expected here.

This is what I see with all modules connected.

image

It's possible I am misunderstanding the AC. Thoughts?

@wpdarren wpdarren assigned aaemnnosttv and unassigned wpdarren Jan 27, 2022
@aaemnnosttv
Copy link
Collaborator Author

@wpdarren are you testing with the latest on main?

@aaemnnosttv aaemnnosttv assigned wpdarren and unassigned aaemnnosttv Jan 27, 2022
@wpdarren
Copy link
Collaborator

@aaemnnosttv I thought I was testing with latest main, but just refreshed it again and now only 2 modules appear. 😓 So, looks like it hadn't refreshed, or, still going through the merge.

@wpdarren
Copy link
Collaborator

QA Update: ✅

Verified:

  • An array of roles and recoverableModules appears when you type window._googlesitekitDashboardSharingData into the console. The PageSpeed and Idea Hub modules appear when connected.
  • Undefined appears as a value of the above property when you type window._googlesitekitDashboardSharingData into the console.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority PHP Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants