Issue/13328 restore warning by zwarm · Pull Request #13775 · wordpress-mobile/WordPress-Android

zwarm · 2021-01-14T16:38:16Z

This PR implements the "warning" wizard step/view for the restore request.

The basic architecture is a clone of BackupDownloadDetailsViewModel with regards to making the request. What differs is the handling of the onBackPressed and error events. I'm on the fence as to whether this should be an actual wizard step or just a different view in the Details step. I am going to leave it as it for this PR, as I can adjust how the errors are handled separately.

Merge Instructions

Make sure Issue/13328 restore details #13773 has been merged
Remove Not Ready For Merge Label
Merge as normal

Cloned Classes
RestoreWarningAdapter
RestoreWarningFragment
RestoreRequestState
RestoreWarningStateListItemBuilder
restore_warning_fragment.xml

Notes:

Ignore styling - this is being handled separately
Ignore unit test placement in packages - this is being handled separately
RestoreActivity & RestoreViewModel (+unit tests) will continue to get updated as I move throughout the steps of the wizard

To test:
Warning: The Restore site button is hooked up, so if you press it your site will be backed up!

Launch the app with RestoreFeatureConfig flag on and the BackupFeatureFlag on
Navigate to ActivityLog
Tap the more menu
Select the restore option
Tap the Restore Site button on the Details View
Verify
(1) The warning view is shown
(2) Tapping on X or back button returns you to the details view
(3) Tapping the restore site button will kick off a restore. You'll see a snackbar message, but the next page won't show because it hasn't been implemented

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.

…arning

peril-wordpress-mobile · 2021-01-14T16:38:25Z

	Warnings
⚠️	PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

peril-wordpress-mobile · 2021-01-14T16:46:51Z

You can test the changes on this Pull Request by downloading the APK here.

peril-wordpress-mobile · 2021-01-14T19:16:45Z

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

ashiagr

Thanks @zwarm! Since most of the classes were cloned and reviewed originally, I found everything fine overall. I just left one comment for PostRestoreUseCase.

Also few suggestions (not related to the PR):

How about we show a warning on the "Restore" screen that "This will override and remove all content created after this point" (similar to web version) before we try restore. I know you just referred the mockups so cc'ing @osullivanchris. I think it's an important message to be displayed to the user.

On unchecking all restore items on the "Restore" screen, we might want to disable "Restore site" action button. I'm not sure what will restore if all items are unchecked. The button gets disabled on the web, just fyi.

ashiagr · 2021-01-15T05:32:18Z

WordPress/src/main/java/org/wordpress/android/ui/jetpack/restore/usecases/PostRestoreUseCase.kt

+        if (!networkUtilsWrapper.isNetworkAvailable()) {
+            NetworkUnavailable
+        }
+
+        val result = activityLogStore.rewind(RewindPayload(site, rewindId, types))


❓: After NetworkUnavailable is fired, control will go to the next line and RemoteRequestFailure will be fired and only this last state will be sent to the VM ignoring NetworkUnavailable. Can we re-confirm this behaviour? We might want to add unit tests for use case to check this.

@ashiagr - this all goes back to "should warning be a step" or really a state in details because we show network error as a snackbar back on details view in backup. I left this as an issue to address outside of this PR - handling "back" with error in the wizard.

WordPress/src/test/java/org/wordpress/android/ui/jetpack/restore/RestoreWarningViewModelTest.kt

osullivanchris · 2021-01-15T10:31:57Z

How about we show a warning on the "Restore" screen that "This will override and remove all content created after this point" (similar to web version) before we try restore. I know you just referred the mockups so cc'ing @osullivanchris. I think it's an important message to be displayed to the user.

Hey thanks for the ping @ashiagr!

I think we are having pretty much the same discussion over here? I hadn't seen this message on web. Seems like a good one to include. Similar to the iOS discussion did you mean to replace the warning message I had in there, or have this in addition?

ashiagr · 2021-01-15T11:27:08Z

I think we are having pretty much the same discussion over here? I hadn't seen this message on web. Seems like a good one to include. Similar to the iOS discussion did you mean to replace the warning message I had in there, or have this in addition?

Yeah I meant the same warning. We can just reuse the one shown in the web version.

…arning

zwarm · 2021-01-15T13:23:04Z

How about we show a warning on the "Restore" screen that "This will override and remove all content created after this point" (similar to web version) before we try restore. I know you just referred the mockups so cc'ing @osullivanchris. I think it's an important message to be displayed to the user.

I'm with you on this and I agree that we should closely match the language on the web.

On unchecking all restore items on the "Restore" screen, we might want to disable "Restore site" action button. I'm not sure what will restore if all items are unchecked. The button gets disabled on the web, just fyi.

I can definitely implement this. @osullivanchris - just say the word

zwarm · 2021-01-15T13:25:19Z

@ashiagr Thanks for the review, I pushed the minor comments. Leaving the UI message changes as a separate task. I'll merge this PR when CI completes

osullivanchris · 2021-01-15T13:37:10Z

I'm with you on this and I agree that we should closely match the language on the web.

Agree but still not 100% sure what we are proposing. Are we changing text, or adding an additional line of text? Can you show me what we're talking about, or need me to mock up? Just not exactly clear on this.

I can definitely implement this. @osullivanchris - just say the word

Yep making the restore action disabled when nothing is selected makes sense to me. Go for it.

ashiagr · 2021-01-15T13:57:45Z

Agree but still not 100% sure what we are proposing. Are we changing text, or adding an additional line of text? Can you show me what we're talking about, or need me to mock up? Just not exactly clear on this.

I was looking for the message on the screen with the checkboxes 🤦‍♀️ since it appeared on clicking checkboxes on the web, I think it's fine to use the existing warning message unless iOS wants any changes.

osullivanchris · 2021-01-15T14:04:42Z

Discussed with @ashiagr on slack and realised the word 'All' is the only thing causing conflict for me now. Its the same as web. But it seems wrong to tell people that we are doing "All" whether they select some, or all, boxes. Hope that makes sense :) Will revert with same on the iOS issue

zwarm added 11 commits January 13, 2021 19:02

Add dagger components

61dd723

Hook in warning

101eb48

Add use case

6d75755

Warning view supporting classes

436da95

Strings and layouts for warning

c118ad2

Merge branch 'issue/13328-restore-details' into issue/13328-restore-w…

2336d4c

…arning

Set toolbar home as close

5ee1a54

Merge branch 'issue/13328-restore-details' into issue/13328-restore-w…

356430c

…arning

Remove log line

3490793

Add and update tests for warning

43bbfb8

Merge branch 'issue/13328-restore-details' into issue/13328-restore-w…

dfe12e5

…arning

zwarm added [Status] Not Ready for Merge labels Jan 14, 2021

zwarm added this to the 16.6 milestone Jan 14, 2021

zwarm requested a review from ashiagr January 14, 2021 16:38

Fix lint

86dc21f

This was referenced Jan 14, 2021

Jetpack Section: Restore #13328

Closed

Issue/13328 restore progress #13778

Merged

Issue/13328 restore complete #13779

Merged

ashiagr self-assigned this Jan 15, 2021

ashiagr reviewed Jan 15, 2021

View reviewed changes

zwarm added 2 commits January 15, 2021 08:14

Merge branch 'issue/13328-restore-details' into issue/13328-restore-w…

100b4a9

…arning

Refactor invoke into method

57d0630

update tests after merge

3d5a552

osullivanchris mentioned this pull request Jan 15, 2021

Jetpack Section: Restore Status and Restore Complete wordpress-mobile/WordPress-iOS#15640

Merged

3 tasks

ashiagr self-requested a review January 15, 2021 14:13

ashiagr approved these changes Jan 15, 2021

View reviewed changes

Base automatically changed from issue/13328-restore-details to develop January 15, 2021 14:23

zwarm merged commit 2d1e606 into develop Jan 15, 2021

zwarm deleted the issue/13328-restore-warning branch January 15, 2021 14:34

Conversation

zwarm commented Jan 14, 2021

Uh oh!

peril-wordpress-mobile bot commented Jan 14, 2021

Uh oh!

peril-wordpress-mobile bot commented Jan 14, 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 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ashiagr left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zwarm Jan 15, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

osullivanchris commented Jan 15, 2021

Uh oh!

ashiagr commented Jan 15, 2021

Uh oh!

zwarm commented Jan 15, 2021

Uh oh!

zwarm commented Jan 15, 2021

Uh oh!

osullivanchris commented Jan 15, 2021

Uh oh!

ashiagr commented Jan 15, 2021

Uh oh!

osullivanchris commented Jan 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

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

edited

Loading

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

edited

Loading

ashiagr left a comment •

edited

Loading

ashiagr Jan 15, 2021 •

edited

Loading