-
Notifications
You must be signed in to change notification settings - Fork 286
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
Fix Site Health notices and add basic E2E tests #6598
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good @aaemnnosttv and works well. I have left a few comments on the tests, please take a look.
|
||
describe( 'Site Health', () => { | ||
it( 'adds debug data to the info tab when Site Kit is active but not setup', async () => { | ||
await setupSiteKit(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The body of this test is the same as the one below - I'm guessing this call to setupSiteKit()
needs to be changed/removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, thanks!
array( | ||
$module_slug => array( | ||
'sharedRoles' => array( 'editor' ), | ||
'management' => 'owner', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth changing the value here to all_admins
to further distinguish this from the defaults...
'management' => 'owner', | |
'management' => 'all_admins', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that before and decided to go this way but I don't see a reason not to; SGTM 👍
@@ -55,7 +55,6 @@ public function test_register() { | |||
'verification_status', | |||
'connected_user_count', | |||
'active_modules', | |||
'recoverable_modules', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be worth adding an additional test case here to cover the Dashboard Sharing fields...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one @aaemnnosttv, LGTM!
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist