Skip to content

Conversation

zwarm
Copy link
Contributor

@zwarm zwarm commented Feb 24, 2021

Fixes #14030

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
light dark backup-download

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
Copy link

peril-wordpress-mobile bot commented Feb 24, 2021

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

@peril-wordpress-mobile
Copy link

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

Generated by 🚫 dangerJS

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Feb 24, 2021

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

@zwarm zwarm requested a review from osullivanchris February 24, 2021 20:32
@ParaskP7 ParaskP7 self-assigned this Feb 25, 2021
@ParaskP7
Copy link
Contributor

👋 @zwarm !

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

  1. When backup finishes: This is just a typo, correct?
  2. 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
Copy link
Contributor

👋 @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:

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

Everything works as expected, kudos! 🌟

Copy link

@osullivanchris osullivanchris left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@zwarm
Copy link
Contributor Author

zwarm commented Feb 25, 2021

👋 @zwarm !

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

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

Yes 🤦

  1. 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
Copy link
Contributor

👋 @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! 🙏

Copy link
Contributor

@ParaskP7 ParaskP7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.