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
Invalid URL error on confirming site kit reset after disconnecting #2478
Comments
@eclarke1 Added this to Sprint 40 as well. |
Ideally we wouldn't display a blank screen in this case. To me, the problem here is due to the Really, the reset action probably shouldn't clear out any state (since this will happen naturally on the next page load anyways) but instead maybe just set some Thoughts @felixarntz @tofumatt ?
Changes here SGTM 👍 |
Agreed; removing all of the state seems overkill and will lead to errors, I wouldn't want us returning a blank screen as described above. Having an "isResetting" state would allow a screen to display that info as well instead of the blank page. 👍🏻 |
Agreed with @tofumatt, we don't need to reset anything in JS since there'll be a new pageload following anyway. |
@asvinb let's update the reset action ( |
@aaemnnosttv IB updated as per your comment. |
Ah yes, I forgot about that action @asvinb but that is the selector we would use 😄 It's currently a wrapper for the Regarding the tests, there will be some changes needed in |
Updated the test coverage section of the IB to account for the test changes we'll need. Also nudged the estimate since this is a GFI and is slightly less straightforward than before. IB ✅ |
@aaemnnosttv this section asks for changes to "it does not reset local connection if reset request fails" but I don't think anything changes in that one as the state will be same as in the start, for which a test already exists. site-kit-wp/assets/js/googlesitekit/datastore/site/reset.test.js Lines 114 to 116 in df00ff3
Also, it asks to add a "new test", what exactly is the new test? I don't think we need a new test here. 🤔 |
@kostyalmm I suppose we could keep the "does not reset local connection if reset request fails" test, but it's a bit redundant perhaps since it shouldn't reset the connection if it succeeds either (in the reducer that is). It will still reset things on the server but won't affect the client until the page is reloaded.
It would simply test that state was not changed even if the reset request was successful. Make sense? |
QA Update: Pass ✅Can confirm that when disconnecting and resetting data on Site Kit, the error message. The modal behaves as expected and the site is disconnected and reset. |
Bug Description
I have encountered an error during smoke testing the latest release (
1.22.0
) which happened after I reset the disconnected site kit. The error itself doesn't seem to block redirect that happens after we confirm resetting which is good. However, it may confuse users, so we still need to address it.Steps to reproduce
Preserve log
option in your browser console;Disconnect
button in the user menu;Reset Site Kit
button shows up, click on it to reset the plugin.Screenshots
There's also a screencast that I have recorded, sorry unable to upload it here due to file size https://d.pr/v/6KsggE
Additional Context
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
Implementation Brief
/assets/js/components/ResetButton.js
handleUnlinkConfirm
function to remove the line where the state is updated to hide the dialog.assets/js/googlesitekit/datastore/site/reset.js
reducerCallback
fromfetchResetStore
Test Coverage
assets/js/googlesitekit/datastore/site/reset.test.js
which expect state to be changed as a result of the reset to expect that state does not change from the reset actionVisual Regression Changes
QA Brief
Follow the steps under "Steps to reproduce" section, the error should not exist anymore. Also the modal must not close at all even after XHR request to backend is completed.
Changelog entry
The text was updated successfully, but these errors were encountered: