jetpack: backup download notice banner by zwarm · Pull Request #14147 · wordpress-mobile/WordPress-Android

zwarm · 2021-02-24T20:09:13Z

This PR adds the Backup Download Banner to the activity log list and backup list.

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:

Still need to decide which milestone to base against, so please do not merge yet
Review FluxC PR merge and create a tag in FluxC develop
Replace FluxC hash with tag from develop in build.gradle
Remove the "Not Ready for Merge label"
Merge this PR

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

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-02-24T20:09:19Z

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

peril-wordpress-mobile · 2021-02-24T20:09:20Z

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

Generated by 🚫 dangerJS

peril-wordpress-mobile · 2021-02-24T20:19:59Z

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

ParaskP7 · 2021-02-25T12:31:49Z

👋 @zwarm !

I have some questions on the second scenario: Scenario.2 : BackupDownload Flow - Wait for flow to end and then exit

When backup finishes: This is just a typo, correct?
A snackbar should immediately appear informing that the backup is complete: Since in the first scenario, this snackbar is not displayed, we might probably want to do the same here, meaning never showing the finished message?

ParaskP7 · 2021-02-25T12:34:50Z

👋 @zwarm !

I also tested a fifth scenario, given that was on the video you shared, opening the Download Backup notification and clicking the site's Backups link for both:

When the Backup Download notice is showing (not previously dismissed), and
When the Backup Download notice was dismissed.

Everything works as expected, kudos! 🌟

osullivanchris

I went through the 4 flows and tested on tablet and dark mode. All looks exactly as expected, nice work 👌 😃

zwarm · 2021-02-25T13:23:38Z

👋 @zwarm !

I have some questions on the second scenario: Scenario.2 : BackupDownload Flow - Wait for flow to end and then exit

When backup finishes: This is just a typo, correct?

Yes 🤦

A snackbar should immediately appear informing that the backup is complete: Since in the first scenario, this snackbar is not displayed, we might probably want to do the same here, meaning never showing the finished message?

I decided to leave it because we are returning from another screen, where as in scenario 1 we are already in the context of the list. I could just as easily remove it here too. We need a tie breaker.

ParaskP7 · 2021-02-25T13:58:27Z

👋 @zwarm !

I decided to leave it because we are returning from another screen, where as in scenario 1 we are already in the context of the list. I could just as easily remove it here too. We need a tie breaker.

Actually, thinking about it more and having you explained it it makes sense, let's leave it as is for now! 🙏

ParaskP7

👋 @zwarm !

I have reviewed and tested this PR as per the instructions, everything almost works as expected, amazing job dealing with this huge fat task! 🌟 🌟 🌟

I have left some minor (🔍) comments and suggestions (💡) for you to consider. Also, I have left a handful of questions (❓) I would like to discuss with you. Most importantly, I have left some warnings (⚠️), but mainly around missing tests. Finally, there is one blocker (🚫) that I would really like us to fix before merging this to develop.

🙏

...rg/wordpress/android/ui/jetpack/backup/download/usecases/PostDismissBackupDownloadUseCase.kt

...ess/src/main/java/org/wordpress/android/ui/jetpack/backup/download/BackupDownloadFragment.kt

...ss/src/main/java/org/wordpress/android/ui/jetpack/backup/download/BackupDownloadViewModel.kt

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

ParaskP7 · 2021-02-25T15:12:44Z

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

+        tools:text="We successfully created a backup download file from October 21, 2020 at 3:40 AM."
+        android:text="@string/activity_log_backup_download_notice_description_with_two_params"/>
+
+    <androidx.constraintlayout.widget.ConstraintLayout


Question (❓): Couldn't the same result be achieved with only one layer of ConstraintLayout?

Going to leave this as is for now, if I have time I will revisit

ParaskP7 · 2021-02-25T15:13:58Z

WordPress/src/main/res/values/styles.xml

    </style>
+
+    <!-- ActivityLog List Backup Download Notice Item -->
+    <style name="ActivityLogList.Notice.Item.Label" parent="android:Widget.TextView">


Suggestion (💡): I usually suggest that the layout_width and layout_height to not be part of a style since they are totally dependent on the individual screen and how a specific element is positions in it. Also, I consider the layout_margin to be included in this category. However, the padding can be indeed considered part of the style.

PS: I see both approached being used on our styles, as such I am okay with anyway you decide to proceed with that. This was just me sharing my thoughts since I would have done it the way I described it.

Going to leave this as is for this PR, as there are multiple approaches to using styles. We should have a follow up on future direction overall. Thanks

Thanks for considering it @zwarm ! 🌟

Cc @ashiagr here so that we can setup a quick call on that to first discuss it amongst us. 🙏

...ordpress/android/ui/jetpack/backup/download/usecases/PostDismissBackupDownloadUseCaseTest.kt

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

zwarm · 2021-02-25T22:20:12Z

@ParaskP7 - This PR was closed and replaced with #14155

zwarm added 16 commits February 23, 2021 14:38

Add notice item layout, styles, and strings

2c75c23

Add notice view type and list item holder

29369e4

Add support for notice list item

79ff64a

Add validUntil to Complete request state

99cd531

New use case for dismissing backup download file download

2fd65c5

Add tracking for download and dismiss button taps

369cbea

Add DownloadBackupFile navigation event

520a72e

Add blank line spacer:

38c2054

Add DownloadBackupFile navigation event to navigationEvents observer

9243372

Add tests for use case

6b5139f

Fix broken tests

0d19bc9

Introduce new action state for return to calling activity

a0b22db

Add support for BackupDownload notice item

b427216

Set progress footnote to visible

fdfd91b

Add, remove, and update tests for backup download

51b3740

Update fluxC version to point to commit

e0d2f32

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

zwarm added this to the 16.9 milestone Feb 24, 2021

zwarm requested a review from ParaskP7 February 24, 2021 20:09

zwarm self-assigned this Feb 24, 2021

zwarm requested a review from osullivanchris February 24, 2021 20:32

Fix failing test

84d812a

ParaskP7 self-assigned this Feb 25, 2021

osullivanchris approved these changes Feb 25, 2021

View reviewed changes

ParaskP7 requested changes Feb 25, 2021

View reviewed changes

zwarm mentioned this pull request Feb 25, 2021

Fix/14030 backup download notice #14155

Merged

3 tasks

zwarm closed this Feb 25, 2021

zwarm deleted the issue/14030-backup-download-notice branch February 25, 2021 22:20

Conversation

zwarm commented Feb 24, 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)

Uh oh!

peril-wordpress-mobile bot commented Feb 24, 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 24, 2021

Uh oh!

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

Uh oh!

Uh oh!

ParaskP7 commented Feb 25, 2021

Uh oh!

ParaskP7 commented Feb 25, 2021

Uh oh!

osullivanchris left a comment

Choose a reason for hiding this comment

Uh oh!

zwarm commented Feb 25, 2021

Uh oh!

ParaskP7 commented Feb 25, 2021

Uh oh!

ParaskP7 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ParaskP7 Feb 25, 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 25, 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!

Uh oh!

Uh oh!

Uh oh!

zwarm commented Feb 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

zwarm commented Feb 24, 2021 •

edited

Loading

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

edited

Loading

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

edited

Loading