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

Cleanup reset site kit code, change test accordingly. #2595

Merged
merged 5 commits into from
Jan 8, 2021

Conversation

kostyalmm
Copy link
Contributor

@kostyalmm kostyalmm commented Jan 8, 2021

Summary

Addresses issue #2478

Relevant technical choices

  • Updated state management while resetting site kit
  • Updated tests accordingly for reset

Checklist

  • My code is tested and passes existing unit tests.
  • My code has an appropriate set of unit tests which all pass.
  • My code is backward-compatible with WordPress 4.7 and PHP 5.6.
  • My code follows the WordPress coding standards.
  • My code has proper inline documentation.
  • I have added a QA Brief on the issue linked above.
  • I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

@google-cla google-cla bot added the cla: yes label Jan 8, 2021
Copy link
Collaborator

@aaemnnosttv aaemnnosttv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, just two small things to address for this to make sense when we come back to look at this in the future 😄

@@ -85,7 +85,7 @@ describe( 'core/site reset', () => {

// After a successful reset, `connection` should be `undefined` again.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is no longer accurate.

Suggested change
// After a successful reset, `connection` should be `undefined` again.
// After a successful reset, `connection` state will be updated on the next page load.

Comment on lines 86 to 88
// After a successful reset, `connection` should be `undefined` again.
const connection = await registry.select( STORE_NAME ).getConnection();
expect( connection ).toEqual( undefined );
expect( connection ).toEqual( { connected: true, resettable: true } );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The name of this test should also be updated to be more accurate as well.
How about: it "resets connection on server only" ? I think this would be a bit more clear as to what is going on in combination with the updated comment.
Unfortunately GitHub won't let me suggest this change because it's out of range of the current changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. Sounds good.

Copy link
Collaborator

@aaemnnosttv aaemnnosttv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants