Jetpack Section: Backup/Restore Error Handling by momo-ozawa · Pull Request #15673 · wordpress-mobile/WordPress-iOS

momo-ozawa · 2021-01-20T07:41:15Z

Description:

Added Restore Failed VC, Restore Status Failed VC and Backup Status Failed VC
Added new error cases for the restore flow

// restoreSite
func showNoInternetConnection()       // NEW: Displays alert
func showRestoreAlreadyRunning()      // NEW: Not implemented yet
func showRestoreRequestFailed()       // Displays notice, stays on the Restore Warning VC

// getRestoreStatus
func showRestoreFailed()              // NEW: Displays the Restore Failed VC 
func showRestoreStatusUpdateFailed()  // NEW: Displays the Restore Status Failed VC

Added new error cases for the backup flow

// prepareBackup
func showNoInternetConnection()       // NEW: Displays alert
func showBackupRequestFailed()        // Displays notice, stays on the Backup Options VC

// getBackupStatus
func showBackupStatusUpdateFailed()   // NEW: Displays the Backup Failed VC

Added a max retry count of 3x for getRestoreStatus / getBackupStatus

To test:

Restore: No Internet Connection

My Site > Activity Log > rewindable cell > tap ellipsis icon
Tap Restore
Tap Restore to this point
Turn off wifi, then tap Confirm
- ✅ Alert should be displayed

Backup: No Internet Connection

My Site > Activity Log > rewindable cell > tap ellipsis icon
Tap Download backup
Turn off wifi, then tap Create downloadable file
- ✅ Alert should be displayed

Restore: Restore Status Update Failed

My Site > Activity Log > rewindable cell > tap ellipsis icon
Tap Restore
Tap Restore to this point
Tap Confirm
While restore is running, turn off wifi
- ✅ Should transition to Restored Status Failed VC

Backup: Backup Status Update Failed

My Site > Activity Log > rewindable cell > tap ellipsis icon
Tap Download backup
Tap Create downloadable file
While backup is running, turn off wifi
- ✅ Should transition to Backup Status Failed VC

Restore: Restore Failed

I'm not sure how to trigger this, but you can temporarily route the "restore status update failed" error to display the Restore Failed VC.

PR submission checklist:

I have considered adding unit tests where possible.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

peril-wordpress-mobile · 2021-01-20T07:41:22Z

You can trigger an installable build for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2021-01-20T07:54:37Z

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

leandroalonso · 2021-01-20T14:26:22Z

@momo-ozawa I wasn't able to test the Restore: Restore Failed and Backup: Backup Failed test cases.

For me, the screen just sits there and nothing happens. I'm not sure if disabling wi-fi is the best option to test this... perhaps we can edit some code to simulate server-side errors?

@emilylaguna would you mind testing that?

emilylaguna · 2021-01-20T22:17:38Z

@leandroalonso I'm having the same issue as well. 😢

- Set max retry count to 3x - If we retry 3x and the get request still fails, show the status update failed screen

momo-ozawa · 2021-01-21T03:40:46Z

@leandroalonso @emilylaguna
Oops, sorry! I fixed some issues and now you should be able to test the new error cases. 🙏

momo-ozawa · 2021-01-21T12:36:41Z

...lated/Jetpack/Jetpack Restore/Restore Complete/JetpackBackupStatusFailedViewController.swift

+            messageTitle: NSLocalizedString("Hmm, we can't update the status of your backup.", comment: "Title for the Jetpack Backup Status Failed message."),
+            messageDescription: "No need to worry. We'll notify you when your backup is ready.",  // FIXME: Placholder text


@leandroalonso @emilylaguna (cc: @osullivanchris @zwarm)
Per our Slack discussion, the message title displays Hmm, we can't update the status of your backup.
Trying to come up with a good message description... let me know what you think. Open to suggestions. 👍

I think the message is probably ok. But I have some questions about the flows I'll add separately.

osullivanchris · 2021-01-21T13:40:47Z

Hi, wondering if I need a particular build number to test this out? I tried to install the latest one-off, but I'm not sure if I'm seeing the correct behaviour.

Thanks for the clear scenarios to follow.

Use case 1 and 2, I was able to get through them, but I saw snackbar/toast messages which doesn't look like what's in the screenshots. Honestly, I thought the snackbar is working well though.

Use case 2 and 3 didn't work for me, it just got stuck.

I am slightly concerned about the screenshots. I feel like the error states might need to be something on top of the UI, or appear as an end result in the flow, rather than adapting the copy of the current step. But that's why I really want to play with it, my assumption could be totally wrong until I actually see it in the flow.

leandroalonso · 2021-01-21T15:13:44Z

@osullivanchris use the build number 40744

You can test the changes on this Pull Request by downloading it from AppCenter here with build number: 40744. IPA is available here. If you need access to this, you can ask a maintainer to add you.

osullivanchris · 2021-01-21T16:37:22Z

Thanks @leandroalonso !

I can see that flows 1 and 2 changed to a dialog rather than a Snackbar/toast message. This is fine too. Though I thought the Snackbar was pretty good.

I cannot restore for some reason.

Looking at flow 4:

Backup: Backup Status Update Failed

I have a couple of issues

Where it lives in the flow was actually fine. The transition to a new deeper view made this clear to me. So I'm fine with that.
Not sure if we should use the same layout and icon which we were using as a 'warning' rather than an error in a previous step in the same flow
"No need to worry. We'll notify you when your backup is ready" subheading. Is this still true?
"System Update failed" is slightly strange page title.
Is it always a case of failing, or is there ever a case that it is running the process but we just don't know? I am guessing there's a case that its still happening, thus why we are saying we just don't know the status. Wondering if we need to treat a failure and an unknown differently

momo-ozawa · 2021-01-22T01:00:11Z

I can see that flows 1 and 2 changed to a dialog rather than a Snackbar/toast message.This is fine too. Though I thought the Snackbar was pretty good.

Actually, we do show a snackbar if we encounter an error after sending the initial restoreSite / prepareBackup request.

The "No connection" alert is shown if we can establish that there's no network connection before sending the initial restoreSite / prepareBackup request.

Timing	Error	Result
Before sending initial req	No network connection	Alert displayed
After sending initial req	Any	Snackbar displayed

I cannot restore for some reason.

Do you mean you can't proceed to the Restore Status screen?

momo-ozawa · 2021-01-22T01:46:33Z

Failure vs Unknown Status

Is it always a case of failing, or is there ever a case that it is running the process but we just don't know? I am guessing there's a case that its still happening, thus why we are saying we just don't know the status. Wondering if we need to treat a failure and an unknown differently

I do think we should distinguish between "failure" and "unknown status".

If we get a "status == .failed" response while we're performing the status check, we know for sure that it's a failure
If we get an error while we're performing the status check, we don't know the status. If it ends up succeeding at some point in the future, then we'll get a notification

Here's a quick overview of the current restore logic:

Request	Response	Action
Initial restoreSite req	Error	Display snackbar error, user is left in the same context
Initial restoreSite req	Success	Proceed to Restore Status view and send getStatus req
getStatus req	Error	Proceed to Done view (Unknown Status)^[1]
getStatus req	Success, status == .failed	Proceed to Done view (Failed)
getStatus req	Success, status == .done	Proceed to Done view (Complete)

^[1] Alternative presented in section below

What should we do if we end up in the Unknown Status state?

Not sure if we should use the same layout and icon which we were using as a 'warning' rather than an error in a previous step in the same flow

"System Update failed" is slightly strange page title.

I assumed that if we ended up in an "Unknown Status" state, we'd proceed to the Done view.

An alternative to the Done view: we could take users back to the activity log, and simply display a snackbar with the message Hmm, we can't update the status of your [restore | backup].

momo-ozawa · 2021-01-22T04:55:58Z

Now that I think of it, I guess the Hmm, we can't update... in Calypso is a "toast", so a "snackbar" would be the equivalent UI component in mobile 😌

I'll push the changes if I get an OK on the "snackbar" implementation!

RPReplay_Final1611289325.mov

osullivanchris · 2021-01-22T09:27:14Z

Do you mean you can't proceed to the Restore Status screen?

When I tapped restore, even online, I got an error.

I assumed that if we ended up in an "Unknown Status" state, we'd proceed to the Done view.

Yeah I didn't have a problem with going to the 'Done' view itself. It was just the information I was seeing there, plus the lack of distinction between unknown and failed. The table makes things clearer for me to understand, thanks for that.

I'll push the changes if I get an OK on the "snackbar" implementation!

So now, if it fails, it proceeds to the next step but telling the user it failed. If the status doesn't update, it will just stay there indefinitely, but if the user closes the modal, then they get the Snackbar message? I think it generally makes sense. The only two options I can think of are (a) should we show the snackbar while they are still in the modal rather than waiting until they close it, and (b) I wonder should we change the "0%" to say something like "Unknown progress" ... but better!

Now that I think of it, I guess the Hmm, we can't update... in Calypso is a "toast", so a "snackbar" would be the equivalent UI component in mobile 😌

I just use the words interchangeably so apologies for any confusion. I think 'toast' is a generic industry term, and Snackbar is what Material Design calls it.

Generally, if this isn't enough info and I'm slowing you down only getting one reply per day due to our timezone overlap, ping me on slack and we can jump on a call!

momo-ozawa · 2021-01-22T10:46:21Z

When I tapped restore, even online, I got an error.

Weree you testing on a simulator? I've noticed that the wifi gets wonky on the simulator... it doesn't come back online for me once I turn it off. :(

So now, if it fails, it proceeds to the next step but telling the user it failed.

Yes.

If the status doesn't update, it will just stay there indefinitely, but if the user closes the modal, then they get the Snackbar message?

Not exactly. In my "snackbar" solution, if the status doesn't update, the Progress view is dismissed automatically. Then once the user is back in the Activity Log, the Hmm, we can't update... message is displayed.

Sorry about the confusion! 🙏

Toast vs Snackbar

No worries!

Generally, if this isn't enough info and I'm slowing you down only getting one reply per day due to our timezone overlap, ping me on slack and we can jump on a call!

Thanks! Sounds good to me!

osullivanchris · 2021-01-25T08:05:09Z

Weree you testing on a simulator? I've noticed that the wifi gets wonky on the simulator... it doesn't come back online for me once I turn it off. :(

it was ok my device but maybe I wasn’t set up right!

Not exactly. In my "snackbar" solution, if the status doesn't update, the Progress view is dismissed automatically. Then once the user is back in the Activity Log, the Hmm, we can't update... message is displayed.

oh cool. That could work. I suppose they would have to close it themselves anyway if nothing happens. But I’m thinking the process could actually complete, we just don’t know about it...would we know if it’s done..?

leandroalonso

@momo-ozawa tested all the scenarios, it all worked as expected except for the backup. It seems that the error screen is triggered over and over again:

momo-ozawa · 2021-01-26T09:22:39Z

@leandroalonso

It seems that the error screen is triggered over and over again:

Yikes! Will take a look.

@osullivanchris and I are almost at a decision for what to do when the status update fails (Original comment: #15673 (comment)). When I push the final changes for the status update failure case, I'll also try to fix the issue you mentioned 👍

…ling # Conflicts: # WordPress/Classes/ViewRelated/Jetpack/Jetpack Restore/Restore Warning/Coordinators/JetpackRestoreWarningCoordinator.swift

momo-ozawa · 2021-01-27T02:42:27Z

@leandroalonso

Unknown Status

Chris came up with a full design view that shows a little more context than the standard "Complete Views"
For now, I just updated the message string 9655efb
I can update the status failed UI in a different PR (Worried that this PR is getting big/stale)

Download triggering repeatedly

I tried the download flow about 10 times, and wasn't able to reproduce the bug you were seeing
I can address this in a different PR, if you could let me know the steps to reproduce this bug 🙏

leandroalonso

I can update the status failed UI in a different PR (Worried that this PR is getting big/stale)

100% agree.

I can address this in a different PR, if you could let me know the steps to reproduce this bug 🙏

I'm still able to reproduce it here:

Generate one backup
As soon the progress bar is filled with some progress (in my case it goes right to 88%) turn off the wifi
Wait for a few seconds
It starts

The failure block in refreshBackupStatus is called over and over again and this "stacks" the view being presented. Eventually, this stops (after all the VCs are presented). Check this debug output:

$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus called
$$ refreshBackupStatus failure // Those failures all come at once
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure
$$ refreshBackupStatus failure

I'm wondering if that is perhaps just a Simulator behavior? We can address that in a future PR anyway. :)

Momo Ozawa added 7 commits January 20, 2021 11:44

Delete unused parameter

b09eb75

Add error handling for restoreSite and getRewindStatus

b5a6224

Add Jetpack Restore Failed VC

bb19284

Add Jetpack Backup Failed VC

92c4969

Configure tint color based on success/failure

a01c35c

Reorganize files into groups

b7dae5b

Display “No Internet Connection” alert

af1766f

momo-ozawa added [Type] Task Jetpack Mobile labels Jan 20, 2021

momo-ozawa requested review from emilylaguna and leandroalonso January 20, 2021 07:41

momo-ozawa self-assigned this Jan 20, 2021

momo-ozawa marked this pull request as draft January 20, 2021 07:42

momo-ozawa removed request for emilylaguna and leandroalonso January 20, 2021 07:42

Momo Ozawa added 5 commits January 21, 2021 12:13

Pass iconImageColor to RestoreCompleteView

5350335

Update error handling for get backup/restore status failed

3fae2e6

- Set max retry count to 3x - If we retry 3x and the get request still fails, show the status update failed screen

Add Restore Status Failed VC

a9782a8

Rename BackupFailed VC to BackupStatusFailed VC

990a2c1

Fix string capitalization

5636979

momo-ozawa requested review from emilylaguna and leandroalonso January 21, 2021 03:36

momo-ozawa marked this pull request as ready for review January 21, 2021 03:36

momo-ozawa added this to the 16.6 milestone Jan 21, 2021

momo-ozawa mentioned this pull request Jan 21, 2021

Jetpack Section: Backup and Restore detail #15191

Closed

43 tasks

momo-ozawa changed the title ~~Task/15191 backup restore more error handling~~ Jetpack Section: Backup/Restore Error Handling Jan 21, 2021

momo-ozawa mentioned this pull request Jan 21, 2021

Jetpack Section: Activity Detail Download Backup Button #15683

Merged

3 tasks

momo-ozawa commented Jan 21, 2021

View reviewed changes

momo-ozawa modified the milestones: 16.6, 16.7 Jan 25, 2021

leandroalonso reviewed Jan 25, 2021

View reviewed changes

Momo Ozawa added 2 commits January 27, 2021 10:43

Update status update failed strings

9655efb

Merge branch 'develop' into task/15191-backup-restore-more-error-hand…

7867362

…ling # Conflicts: # WordPress/Classes/ViewRelated/Jetpack/Jetpack Restore/Restore Warning/Coordinators/JetpackRestoreWarningCoordinator.swift

leandroalonso self-requested a review January 27, 2021 12:33

leandroalonso approved these changes Jan 27, 2021

View reviewed changes

momo-ozawa merged commit 551cbb0 into develop Jan 28, 2021

momo-ozawa deleted the task/15191-backup-restore-more-error-handling branch January 28, 2021 01:27

		messageTitle: NSLocalizedString("Hmm, we can't update the status of your backup.", comment: "Title for the Jetpack Backup Status Failed message."),
		messageDescription: "No need to worry. We'll notify you when your backup is ready.", // FIXME: Placholder text

Conversation

momo-ozawa commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

To test:

Restore: No Internet Connection

Backup: No Internet Connection

Restore: Restore Status Update Failed

Backup: Backup Status Update Failed

Restore: Restore Failed

Uh oh!

peril-wordpress-mobile bot commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

peril-wordpress-mobile bot commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

leandroalonso commented Jan 20, 2021

Uh oh!

emilylaguna commented Jan 20, 2021

Uh oh!

momo-ozawa commented Jan 21, 2021

Uh oh!

momo-ozawa Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

osullivanchris Jan 21, 2021

Choose a reason for hiding this comment

Uh oh!

osullivanchris commented Jan 21, 2021

Uh oh!

leandroalonso commented Jan 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

osullivanchris commented Jan 21, 2021

Uh oh!

momo-ozawa commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

momo-ozawa commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Failure vs Unknown Status

What should we do if we end up in the Unknown Status state?

Uh oh!

momo-ozawa commented Jan 22, 2021

Uh oh!

osullivanchris commented Jan 22, 2021

Uh oh!

momo-ozawa commented Jan 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

osullivanchris commented Jan 25, 2021

Uh oh!

leandroalonso left a comment

Choose a reason for hiding this comment

Uh oh!

momo-ozawa commented Jan 26, 2021

Uh oh!

momo-ozawa commented Jan 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Unknown Status

Download triggering repeatedly

Uh oh!

leandroalonso left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

momo-ozawa commented Jan 20, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Jan 20, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Jan 20, 2021 •

edited

Loading

leandroalonso commented Jan 21, 2021 •

edited

Loading

momo-ozawa commented Jan 22, 2021 •

edited

Loading

momo-ozawa commented Jan 22, 2021 •

edited

Loading

momo-ozawa commented Jan 22, 2021 •

edited

Loading

momo-ozawa commented Jan 27, 2021 •

edited

Loading