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

Woo Installer: Add error handling to transfer step #58331

Closed
2 tasks done
lsl opened this issue Nov 22, 2021 · 13 comments
Closed
2 tasks done

Woo Installer: Add error handling to transfer step #58331

lsl opened this issue Nov 22, 2021 · 13 comments
Assignees
Labels
[Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Woop WooCommerce on Plans Pod

Comments

@lsl
Copy link
Contributor

lsl commented Nov 22, 2021

The atomic transfer step in the woocommerce installer needs error handling.

  • On error, redirect back to the confirm step and display the error.
  • Investigate: There may now be a stuck transfer in place, confirm what these look like on the confirm screen it may be we need to add disabling to buttons on the confirm screen.

One semi-consistent way to trigger a transfer failure error in prod:

  • Initiate transfer using installer
  • Revert site without issuing early cleanup
  • Immediately attempt a re-transfer

If the clean up runs mid transfer (it should) you'll get a transfer error

@lsl
Copy link
Contributor Author

lsl commented Nov 24, 2021

Updated design for the confirm screen to work from, cc @SaxonF

pdhcfv-5D-p2#comment-120
r6eTqDpL2GGk96hcPLhsBb-fi-431%3A2447
#58024 (comment)

I'm not sure how we want to split this between the mobile ux ticket maybe we just close one and tackle it all out of here?

cc @allilevine / @Rolilink / @retrofox

@allilevine
Copy link
Member

I'm not sure how we want to split this between the mobile ux ticket maybe we just close one and tackle it all out of here?

cc @allilevine / @Rolilink / @retrofox

I think we should work on updating the design (and mobile view) separately. I updated #58387 to include both.

@Rolilink
Copy link
Contributor

I think we should work on updating the design (and mobile view) separately. I updated #58387 to include both.

This sounds like a good approach, I wonder if the design and mobile implementations will block each other but I don't think it is a big deal!.

@lsl
Copy link
Contributor Author

lsl commented Nov 25, 2021

Sounds good to me.

@retrofox
Copy link
Contributor

I'm not sure how we want to split this between the mobile ux ticket maybe we just close one and tackle it all out of here?
cc @allilevine / @Rolilink / @retrofox

I think we should work on updating the design (and mobile view) separately. I updated #58387 to include both.

Also, I think we could split up in landing page and transferring step?

Landing page Transferring step
image image

@retrofox
Copy link
Contributor

@lsl
Copy link
Contributor Author

lsl commented Dec 6, 2021

^ #58719

@lsl
Copy link
Contributor Author

lsl commented Dec 13, 2021

Some context around what remains for this in here: #58848 (comment)

@pablinos
Copy link
Contributor

pablinos commented Dec 15, 2021

It's not clear to me what we need to do to complete this. To begin with, I think all errors on the transfer step should lead to the progress bar being replaced with an error, which includes a way to contact support and a way to get back to the app - exit the flow.

As an HTTP error, an error response, or a timeout can all be treated as unrecoverable, the process should be aborted. People could probably come through the landing page to try again, but we'll not to make sure that the state is reset correctly.

This part of the above referenced comment, I'm not sure I fully understand:

I think we should just rely on errorNotice for request failures (e.g. this, not this), that pops up a notice bar in the top right of the page. For transfer failures we should set an error state and show it in place of the progress bar.

Once on the transfer step, I think all failures, request or otherwise, would need to be treated the same, because we're showing the progress bar at this point. Whatever has happened we would need to rely on the error state to abort the process. I think this comment is saying the same thing, but also that we can use the error notice when requests fail while we are not on the transfer step.

For the transfer step there's maybe a few initial things to look at:

  • Create an error page/state that can be shown in place of the progress bar
  • Have a way to display that error page if any failure occurs during HTTP requests.
  • Checking the transfer state and error property, and show the error page if it is stuck, or some other error has occurred
  • Checking the error property in the software install status, and showing the error page if it gets set
  • Implement a timeout for the software install step. This should be a really quick part of the process, so the timeout could be 15 seconds or so. As a first version, we could again show the error page rather than implementing a retry. A retry could be handled manually by restarting the flow.

@retrofox
Copy link
Contributor

Create an error page/state that can be shown in place of the progress bar

image

This is what we got now. It's almost the same Error component defined in the transfer step. Do you think it's good enough at least for now?

@retrofox
Copy link
Contributor

  • Have a way to display that error page if any failure occurs during HTTP requests.
  • Checking the transfer state and error property, and show the error page if it is stuck, or some other error has occurred

Those seem pretty similar to me. I'm testing with a broken WoA site (status is active). The transfer object defines its status with 500. I think we can check this property to catch it.

@retrofox
Copy link
Contributor

Checking the transfer state and error property, and show the error page if it is stuck, or some other error has occurred

Is this property defined? I'm testing with broken sites, and it doesn't seem to be.

@retrofox
Copy link
Contributor

First attempt for transfer: #59267

@retrofox retrofox added [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. and removed [Status] In Progress labels Dec 20, 2021
@lsl lsl closed this as completed Dec 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. Woop WooCommerce on Plans Pod
Projects
Status: 🎉 Done
Development

No branches or pull requests

5 participants