Fix Site Health notices and add basic E2E tests#6598
Conversation
techanvil
left a comment
There was a problem hiding this comment.
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.
The body of this test is the same as the one below - I'm guessing this call to setupSiteKit() needs to be changed/removed.
| array( | ||
| $module_slug => array( | ||
| 'sharedRoles' => array( 'editor' ), | ||
| 'management' => 'owner', |
There was a problem hiding this comment.
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.
I thought about that before and decided to go this way but I don't see a reason not to; SGTM 👍
| 'verification_status', | ||
| 'connected_user_count', | ||
| 'active_modules', | ||
| 'recoverable_modules', |
There was a problem hiding this comment.
It could be worth adding an additional test case here to cover the Dashboard Sharing fields...
techanvil
left a comment
There was a problem hiding this comment.
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