Site-kit-wp: Invalid URL error on confirming site kit reset after disconnecting

[eugene-manuilov]

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

1. Enable the Preserve log option in your browser console;
2. Set up and connect the site kit on your site;
3. Disconnect it by clicking on Disconnect button in the user menu;
4. Once the splash screen appears and the Reset Site Kit button shows up, click on it to reset the plugin.
5. See the error for a few seconds before you will be redirected and check the console log for more details.

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

• PHP Version: 7.4.12
• OS: [e.g. iOS] Ubuntu
• Browser [e.g. chrome, safari] Google Chrome
• Plugin Version [e.g. 22] 1.22.0
• Device: [e.g. iPhone6] Desktop computer

---

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

Acceptance criteria

• No JS error should occur / flash up in the background after having clicked the Reset button.

Implementation Brief

• Using /assets/js/components/ResetButton.js
  
  
  
  • Edit the handleUnlinkConfirm function to remove the line where the state is updated to hide the dialog.
    
    setDialogActive( false )
  
  
  • This is causing a console error where the component has already been unmounted but we are trying to update the state. Since we are redirecting the user to another page, we can omit updating the state to hide the dialog.
  
  

• Using assets/js/googlesitekit/datastore/site/reset.js

  • Remove reducerCallback from fetchResetStore
    

    Test Coverage

• Replace relevant tests in 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 action

  • The one exception here is the state related to the fetch request progress, but we should be able to assert that the state is the same before the reset request and once the request is finished.
  • i.e. replace tests for "it resets connection" as well as "it does not reset local connection if reset request fails" (since it does not reset any state even if successful) with a new test as described above

Visual Regression Changes

• There should be no visual 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

• Fix JavaScript error triggered upon resetting the plugin's data.

[felixarntz]

@eclarke1 Added this to Sprint 40 as well.

[aaemnnosttv]

@asvinb

Using /assets/js/components/setup/SetupUsingProxy.js, return null if siteURL is null

Ideally we wouldn't display a blank screen in this case. To me, the problem here is due to the reset action resetting the state to an empty object which clears out the reference URL that raises the error in the component that expects it to be a valid URL (a reasonable assumption I think). IMO the reference URL and other non-connection related data shouldn't be affected by this action since they're not really related to what is being reset (credentials, access tokens, options, etc).

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 isResetting: true state.

Thoughts @felixarntz @tofumatt ?

Using /assets/js/components/ResetButton.js

Changes here SGTM 👍

[tofumatt]

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 isResetting: true state.

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. 👍🏻

[felixarntz]

Agreed with @tofumatt, we don't need to reset anything in JS since there'll be a new pageload following anyway.

[aaemnnosttv]

@asvinb let's update the reset action (assets/js/googlesitekit/datastore/site/reset.js) to not wipe out the state and leave that to the page load instead. We can probably just remove the reducerCallback to fetchResetStore unless we need to actually select some state to know if we are resetting or not (in which case we'd need to add a selector too).

[asvinb]

@aaemnnosttv IB updated as per your comment.
As for the selector, I see we already have the isDoingReset selector which we could probably use.

[aaemnnosttv]

Ah yes, I forgot about that action @asvinb but that is the selector we would use 😄 It's currently a wrapper for the isFetchingReset selector which merely indicates that the reset API request is in progress. It looks like we're not currently using this selector so it's probably not worth changing the way it works as part of this issue. We can revisit the reset-related datastore parts in another issue if needed but I think this is all we need here.

Regarding the tests, there will be some changes needed in assets/js/googlesitekit/datastore/site/reset.test.js where we no longer expect that state will change as a direct result of this action. I'll update this part of the IB.

[aaemnnosttv]

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 ✅

[kostyalmm]

i.e. replace tests for "it resets connection" as well as "it does not reset local connection if reset request fails" (since it does not reset any state even if successful) with a new test as described above

@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.
https://github.com/google/site-kit-wp/blob/df00ff35dff8b25b5b1b05674ce1310a6041eea4/assets/js/googlesitekit/datastore/site/reset.test.js#L114-L116

Also, it asks to add a "new test", what exactly is the new test? I don't think we need a new test here. 🤔

[aaemnnosttv]

@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.

Also, it asks to add a "new test", what exactly is the new test? I don't think we need a new test here.

It would simply test that state was not changed even if the reset request was successful. Make sense?

[wpdarren]

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.