Fix/14030 backup download notice by zwarm · Pull Request #14155 · wordpress-mobile/WordPress-Android

zwarm · 2021-02-25T22:19:32Z

This PR adds the Backup Download Banner to the activity log list and backup list. The details of this PR were copied from the closed PR #14147. With the exception of styles and layouts, comments from that PR are addressed in this one.

Notable changes ( It may be easiest to review this PR by commits and I'm available if reviewer wants to pair review)

activity_log_list_notice_item.xml - New layout file for the notice banner
styles.xml & strings.xml - Added new styles for the banner and a few strings
ActivityLogAdapter, ActivitiyLogDiffCallback - both updated to support the Notice list item type and the new NoticeItemViewHolder, so that the view will actually show in the list
Updated Complete to include url and validUntil and GetBackupDownloadStatusUseCase to send those values downstream
Created PostDismissBackupDownloadUseCase which is post the dismiss request to the server. If there is an error, we log it and move on.
Four new tracking collections points were added to track button taps from the notice banner. Separate tracks for activity log list notice vs backup list notice.
JETPACK_BACKUP_DOWNLOAD_FILE_NOTICE_DOWNLOAD_TAPPED -
JETPACK_BACKUP_DOWNLOAD_FILE_NOTICE_DISMISSED_TAPPED,
ACTIVITY_LOG_DOWNLOAD_FILE_NOTICE_DOWNLOAD_TAPPED,
ACTIVITY_LOG_DOWNLOAD_FILE_NOTICE_DISMISSED_TAPPED
Added a couple of new function to ActivityLogTracker to make the tracks call reusable
Added a new navigation event for DownloadBackupFile, which will forward the user to the web browser for file download. Updated the observer in ActivityLogListFragment to handle the browser launch
Added a JetpackBackupDownloadActionState which will let the calling activity know which state the download is in on return. This was a precursor to showing the correct messaging when returning to the activity log list or backup list. Changes were made to BackupDownloadFragment and BackupDownloadViewModel to support this new intent extra.
The majority of the changes occur in ActivityLogViewModel, which include:
- Updating backupDownloadEvent with a displayNotice field, so when reloadEvents executes it uses this property to determine if the notice banner should be shown. Also included downloadId, url, and validUntil.
- Removed the showBackupDownloadFinishedMessage from within reloadEvents, the snackbar is replaced with the notice banner
- Added a new function to build the notice list item
- Added two new functions to handle the download and dismiss button clicks from within the notice banner
- Updated onQueryBackupDownloadStatus to use the JetpackBackupDownloadActionState to determine which snackbar message to be shown. Note that in the case of returning to the lists, we do want to show the snackbar in addition to the notice banner.
- Updated handleBackupDownloadStatusForProgress to no longer check if the progress item is shown, we always kick off a reloadEvents
- Updated handleBackupDownloadStatusForComplete to reloadEvents of the progressItem is not shown
Updated and/or added tests

Merge Instructions:

Review FluxC PR and merge
** PLEASE DO NOT MERGE: This PR merge is dependent on the proper merge of this FluxC PR **
FYI: After approval of both PRs, the process will follow this flow:
Open 2 FluxC PRs to merge release/1.11.1 back to develop and trunk, and add a tag 1.11.1 and GitHub release.
Replace FluxC hash with the 1.11.1 tag in WPAndroid build.gradle
Remove the "Not Ready for Merge label"
Merge this PR in release/16.8

light	dark	notification

To test:

Scenario.1 : BackupDownload Flow - Manual Exit

Open app
Launch Activity Log screen,
Find a rewindable item from the list, click on the ellipsis menu item and select Download backup to launch the Backup Download screen,
Click the Create downloadable file button.
Navigate back manually (by clicking the X toolbar button or pressing/swiping back).
You should automatically be navigated back to the Activity Log screen and a backup status will be triggered,
The backup progress item should immediately appears.
A snackbar should immediately appear informing that the backup just started.
Wait for backup to finish while having the Activity Log screen
When backup finishes

The backup progress item should disappear
The backup download notice should appear

Scenario.2 : BackupDownload Flow - Wait for flow to end and then exit

Open app
Launch Activity Log screen,
Find a rewindable item from the list, click on the ellipsis menu item and select Download backup to launch the Backup Download screen,
Click the Create downloadable file button.
Wait for the backup download flow to complete keeping the backup flow process open
After the backup completes, navigate back manually (by clicking the X toolbar button or pressing/swiping back)

A snackbar should immediately appear informing that the backup is complete
The backup download notice should appear

Scenario.3 (Notice banner download)

Run a backup using scenario 1 or 2 above
Click on the "Download" button within the notice banner
Note that a browser is launched and the download starts. In some cases, you may be prompted to verify the download.

Scenario.4 (Notice banner dismiss)

Run a backup using scenario 1 or 2 above
Click on the "Dismiss" button within the notice banner
Note the the Notice banner disappears

Scenario.5 (Notification detail tap)

Run a backup using scenario 1 or 2 above
Wait for the notification to show up in the Android device
Tap on the notification to open the Notification Detail view
Tap on the Sites Backups link within the notification detail
Note that you are navigated to the Backup list and that the notice banner is visible

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.

…ivity

…o viewmodel

peril-wordpress-mobile · 2021-02-25T22:19:38Z

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

peril-wordpress-mobile · 2021-02-25T22:19:39Z

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

Generated by 🚫 dangerJS

zwarm · 2021-02-25T22:25:44Z

WordPress/src/main/java/org/wordpress/android/viewmodel/activitylog/ActivityLogViewModel.kt

+        activityLogTracker.trackDownloadBackupDismissButtonClicked(rewindableOnly)
+        viewModelScope.launch {
+            // reload events first to remove item - convey progress
+            reloadEvents(backupDownloadEvent = BackupDownloadEvent(displayNotice = false, displayProgress = false))


@ParaskP7 - Please note that I moved the reloadEvents call before the dismiss call because I didn't want to leave room for the button to be tapped more than once. It appears that all UI refreshes call reloadEvents and disabling the button would to have called the method anyway. Plus I tested with a disable and it made the view jumpy because the transition happens quickly.

👋 @zwarm !

Thanks for guiding me through your thought process. 🙏

zwarm · 2021-02-25T22:32:57Z

WordPress/src/test/java/org/wordpress/android/viewmodel/activitylog/ActivityLogViewModelTest.kt

-        viewModel.onQueryBackupDownloadStatus(REWIND_ID, DOWNLOAD_ID)
+        viewModel.onQueryBackupDownloadStatus(REWIND_ID, DOWNLOAD_ID, JetpackBackupDownloadActionState.COMPLETE.id)

+        assertThat(viewModel.events.value?.first() as? Notice).isNotNull


@ParaskP7 - This may not be ideal, but it gets the job done - for now. I'm having trouble creating a Notice item to match what is actually created because of the click delegates. I'm open to suggestions, but it can wait after the next beta. Thanks

👋 @zwarm !

Definitely, will also have a quick look. 🙏

peril-wordpress-mobile · 2021-02-25T22:56:58Z

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

ParaskP7

👋 @zwarm !

I have reviewed and tested this PR as per the instructions, everything works exactly as expected, once more, kudos on a wonderful job! 🌟 🌟 🌟

I have left some extra comments for you to consider (also copy-pasted a couple from the previous #14147 PR for your convenience). I am going to approve this PR anyway, since none is blocking. I am NOT going to merge this PR yet to give you some time to apply any of my suggestions. However, feel free to ignore them and merge the PR yourself.

WordPress/src/main/java/org/wordpress/android/viewmodel/activitylog/ActivityLogViewModel.kt

ParaskP7 · 2021-02-26T08:56:02Z

WordPress/src/main/java/org/wordpress/android/viewmodel/activitylog/ActivityLogViewModel.kt

@@ -663,16 +708,21 @@ class ActivityLogViewModel @Inject constructor(
    }

    private fun handleBackupDownloadStatusForComplete(state: BackupDownloadRequestState.Complete) {


Warning (⚠️): Consider adding a test for the new logic added here.

PS: I copy-pasted that from the sibling PR since I didn't resolve that yet there.

I guess I missed this one, probably not going to done in time for the beta though. Looks like we also need to add a test for handleBackupDownloadStatusForProgress, I don't see that explicitly called out either. I'll drop it on my to-do list. Thanks

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

ParaskP7 · 2021-02-26T13:01:08Z

👋 @zwarm !

Just checked your latest 3 commits and LGTM, still approved from my side. 💪

ParaskP7 · 2021-02-26T15:41:02Z

👋 @zwarm !

Just checked your latest commit and LGTM, still approved from my side. 💪

…kup-download-notice

zwarm added 20 commits February 25, 2021 10:03

Add tracking for notice button taps

cec80f1

Common class for backup download actions

9d022e6

Add use case and test for dismissing backup download notice

d38fdd5

New layout, strings, and styles for backup download notice

11c76c1

Add validUntil to complete response. Update unit test

2b6f01b

Add JetpackBackupDownloadActionState to support return to calling act…

ed675fb

…ivity

Add support for new Notice item type

581505f

Add navigation event for DownloadFile

99cc4b0

Add support for download file navigation event, passing action type t…

a5dc432

…o viewmodel

Add support for backup download notice

2b00298

Add/remove/modified unit tests

46f4d08

Set footnote to visible in progress view

e8213be

Refactor for readability

388aec7

Move downloadable file valid test to use case

2e5e27a

Move cancel branch to multi-line for consistency

3a78c78

Update format and line wraps

1956f16

Change signature of aciton buttons

2e9d5ed

Refactor code and update tests

860876a

Fix detekt

3c7d095

Update FluxC version with hash

8641b6e

zwarm added [Type] Task [Status] Not Ready for Merge labels Feb 25, 2021

zwarm added this to the 16.8 ❄️ milestone Feb 25, 2021

zwarm requested a review from ParaskP7 February 25, 2021 22:19

zwarm mentioned this pull request Feb 25, 2021

jetpack: backup download notice banner #14147

Closed

3 tasks

zwarm commented Feb 25, 2021

View reviewed changes

ParaskP7 self-assigned this Feb 26, 2021

ParaskP7 approved these changes Feb 26, 2021

View reviewed changes

zwarm added 3 commits February 26, 2021 07:27

Remove tools:text, as it was no longer needed

e6a9abb

Refactor dismissNoticeClicked function

faf8070

Remove publish param from showBackupDownloadFinishedMessage

8e270e9

zwarm mentioned this pull request Feb 26, 2021

Release/1.11.1 to trunk wordpress-mobile/WordPress-FluxC-Android#1897

Merged

Update fluxC version

9b838bb

zwarm removed the [Status] Not Ready for Merge label Feb 26, 2021

zwarm added 4 commits February 26, 2021 11:38

Remove blank line

51ea5af

Update fluxC version

1b5bd30

Merge remote-tracking branch 'origin/release/16.8' into fix/14030-bac…

8234c30

…kup-download-notice

Update FluxC version

8f104b4

zwarm merged commit f6c9332 into release/16.8 Feb 26, 2021

zwarm deleted the fix/14030-backup-download-notice branch February 26, 2021 20:31

zwarm mentioned this pull request Jul 22, 2021

Jetpack Section: Backup Download #13329

Closed

34 tasks

		@@ -663,16 +708,21 @@ class ActivityLogViewModel @Inject constructor(
		}

		private fun handleBackupDownloadStatusForComplete(state: BackupDownloadRequestState.Complete) {

Conversation

zwarm commented Feb 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Scenario.1 : BackupDownload Flow - Manual Exit

Scenario.2 : BackupDownload Flow - Wait for flow to end and then exit

Scenario.3 (Notice banner download)

Scenario.4 (Notice banner dismiss)

Scenario.5 (Notification detail tap)

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

zwarm Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

ParaskP7 Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

zwarm Feb 25, 2021

Choose a reason for hiding this comment

Uh oh!

ParaskP7 Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

ParaskP7 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!

Uh oh!

Uh oh!

ParaskP7 Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

zwarm Feb 26, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ParaskP7 commented Feb 26, 2021

Uh oh!

ParaskP7 commented Feb 26, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

zwarm commented Feb 25, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Feb 25, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Feb 25, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Feb 25, 2021 •

edited

Loading

ParaskP7 left a comment •

edited

Loading