Issue/13328 restore complete by zwarm · Pull Request #13779 · wordpress-mobile/WordPress-Android

zwarm · 2021-01-14T23:15:26Z

This PR implements the "complete" wizard step/view for the restore request and is mostly a clone of Backup Download complete. This PR also undoes the temporary navigations actions implemented in the progress PR #13778 .

Merge Instructions

Make sure Issue/13328 restore details #13773, Issue/13328 restore warning #13775 & Issue/13328 restore progress #13778 have been merged (sorry, but they need to build upon each other)
Remove Not Ready For Merge Label
Merge as normal

Cloned Classes
RestoreCompleteAdapter
RestoreCompleteFragment
RestoreCompleteStateListItemBuilder
RestoreCompleteViewModel

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
Tap the Confirm restore site on the Warning View
The Progress view will do it's thing - don't cancel out
Verify
(1) The complete view is shown when the restore is finished
(2) Tapping the Visit Site button to launch the site url in an external browser
(3) Tap the Done button to close the wizard

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.

…complete

peril-wordpress-mobile · 2021-01-14T23:15:36Z

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

peril-wordpress-mobile · 2021-01-14T23:15:40Z

	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-14T23:21:49Z

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

ashiagr

Thanks @zwarm! Since most of the classes were cloned and reviewed originally, I found everything fine overall. Left very few minor comments.

ashiagr · 2021-01-15T07:22:28Z

...ava/org/wordpress/android/ui/jetpack/restore/complete/RestoreCompleteStateListItemBuilder.kt

+                buildErrorIconState(),
+                buildErrorDescriptionState(),
+            buildErrorDoneActionState(onDoneClick)


Nitpick 🔍: Minor formatting required.

in 2d240e8

ashiagr · 2021-01-15T07:28:13Z

WordPress/src/main/res/layout/restore_complete_fragment.xml

+
+    <androidx.recyclerview.widget.RecyclerView
+        android:id="@+id/recycler_view"
+        android:layout_width="match_parent"


Nitpick 🔍:MATCH_PARENT is not recommended for widgets contained in a ConstraintLayout.

I'll address this during consolidation, as all fragments restore/backup will use the same fragment file. Thanks.

ashiagr · 2021-01-15T07:34:28Z

...Press/src/test/java/org/wordpress/android/ui/jetpack/restore/RestoreCompleteViewModelTest.kt

+        whenever(site.url).thenReturn("www.google.com")
+
+        viewModel.start(site, restoreState, parentViewModel)
+        whenever(site.url).thenReturn("www.google.com")


Nitpick 🔍: Duplicate stubbing?
whenever(site.url).thenReturn("www.google.com")

Removed in 2d240e8

ashiagr · 2021-01-15T07:38:50Z

...Press/src/test/java/org/wordpress/android/ui/jetpack/restore/RestoreCompleteViewModelTest.kt

+    }
+
+    @Test
+    fun `given visit site is clicked, then a navigationEvent is posted`() = test {


Suggested change

fun `given visit site is clicked, then a navigationEvent is posted`() = test {

fun `when visit site is clicked, then VisitSite navigationEvent is posted`() = test {

in 2d240e8

ashiagr · 2021-01-15T07:55:27Z

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

                when (this) {
                    is VisitSite -> {
-                        // todo annmarie add to ActivityLauncher
+                        ActivityLauncher.openUrlExternal(this@RestoreActivity, url)


Nitpick 🔍: Short branches can be put on the same line as the condition, without braces.

Ugh - for some reason my IDE pushes them back onto separate lines if I execute a clean up command. I'll have to Google this one so it stops happening. Thanks for pointing it out. in 2d240e8

…complete

zwarm · 2021-01-15T14:33:27Z

@ashiagr - With the exception of one xml comment, I pushed the fixes. Once CI finished I'll merge. Thanks again. 🙏

zwarm added 6 commits January 14, 2021 17:37

Add support for complete view

d1598bb

Add support for complete view

764baf3

Add complete view

6d5f2ac

Add and update tests

4e9f555

Merge branch 'issue/13328-restore-progress' into issue/13328-restore-…

7456221

…complete

update test

4040d3e

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 23:15

zwarm mentioned this pull request Jan 14, 2021

Jetpack Section: Restore #13328

Closed

30 tasks

zwarm added 2 commits January 14, 2021 18:30

Fix test

1472098

Handle onBackPressed for complete step

80ad40a

ashiagr self-assigned this Jan 15, 2021

ashiagr approved these changes Jan 15, 2021

View reviewed changes

ashiagr reviewed Jan 15, 2021

View reviewed changes

zwarm added 3 commits January 15, 2021 09:14

Merge branch 'issue/13328-restore-progress' into issue/13328-restore-…

46e2b3a

…complete

Update tests

2f7afd0

Minor spacing and naming fixes

2d240e8

Base automatically changed from issue/13328-restore-progress to develop January 15, 2021 15:09

zwarm merged commit 8e45117 into develop Jan 15, 2021

zwarm deleted the issue/13328-restore-complete branch January 15, 2021 15:39

	fun `given visit site is clicked, then a navigationEvent is posted`() = test {
	fun `when visit site is clicked, then VisitSite navigationEvent is posted`() = test {

Conversation

zwarm 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

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

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zwarm commented Jan 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

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

edited

Loading

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

edited

Loading