Skip to content

Jetpack Section: Backup / Restore Already Running#15689

Merged
momo-ozawa merged 14 commits intodevelopfrom
task/15191-restore-already-running
Jan 28, 2021
Merged

Jetpack Section: Backup / Restore Already Running#15689
momo-ozawa merged 14 commits intodevelopfrom
task/15191-restore-already-running

Conversation

@momo-ozawa
Copy link
Copy Markdown
Contributor

@momo-ozawa momo-ozawa commented Jan 22, 2021 •
edited
Loading

Part of #15191

Description:

  • Added error handling for "restore already running" / "backup already running"
  • Updated styling for RestoreStatusView

Merge instructions:

To test:

Restore

  1. My Site > Activity Log > rewindable item
  2. Tap Restore
  3. Tap Restore to this point
  4. Tap Confirm
    • ✅ Progress value should be displayed on the left side
    • ✅ Progress title should be displayed on the right side
  5. Tap OK, notify me!
    • ✅ Should return to the activity log
    • ✅RewindStatusRow should display the progress
  6. Start another restore by repeating steps 2~4
    • ✅ There's a restore currently in progress, please wait before starting the next one should be displayed

Backup

Since the create downloadable backup process is so quick, I recommend testing this process by using both the app and Calypso.

  1. [App] My Site > Activity Log > rewindable item
  2. [App] Tap Download backup
  3. [Web] Go to wordpress.com > Activity log > rewindable item
  4. [Web] Kick off a download backup process
  5. [App] Before the backup on wordpress.com finishes, tap Create downloadable file
    • ✅ There's a backup currently being prepared, please wait before starting the next one should be displayed
Updated styling Restore already running Backup already running
IMG_1148 Simulator Screen Shot - iPhone 12 Pro - 2021-01-25 at 20 06 44 Simulator Screen Shot - iPhone 12 Pro - 2021-01-25 at 20 09 30

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.

Momo Ozawa added 7 commits January 22, 2021 14:33
Pass ActivityStore to Backup/Restore Status
900e8b7
Display restore is already running notice
7e5d8fe 
If restore is already running, dismiss the Restore Status VC and display a notice
Resume restore status update on activity log
5b958ca 
If the “OK, notify me” button is tapped, dismiss the Restore VC and trigger the ActivityStore to start fetching the rewind status. This will then trigger the “rewind cell” to be displayed in the Activity Log VC.
Rename getRewindStatus for clarity
49521e9 
So we can differentiate between getCurrentRewindStatus and fetchRewindStatus.
Add interface to resume backup status update on activity log
9e7efef
Update RewindStatusRow text from “rewind” to “restore”
548f703
Update styling for RestoreStatusView
614d1b9
@momo-ozawa momo-ozawa added this to the 16.6 milestone Jan 22, 2021
@momo-ozawa momo-ozawa self-assigned this Jan 22, 2021
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 22, 2021 •
edited
Loading

You can trigger an installable build for these changes by visiting CircleCI here.

@momo-ozawa momo-ozawa mentioned this pull request Jan 22, 2021
Closed
43 tasks
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Jan 22, 2021 •
edited
Loading

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

@leandroalonso
Copy link
Copy Markdown
Contributor

leandroalonso commented Jan 22, 2021

@momo-ozawa I tested the described steps, but I find it a little bit odd that:

  • When starting from the web I wasn't able to see the progress (even after refreshing the list)
  • When dismissing the screen, I also wasn't able to see the progress (only if I tapped on the "Notify me later" button)

I'd like to propose a change that perhaps might fix that:

  • If you call store.fetchRewindStatus(site: site) from ActivityListViewModel.refresh both the aforementioned cases would be fixed
  • It still doesn't cover the case when you return from a backup/restore, but there are a few ways you can fix that: by using a completion block and/or delegate you can trigger the fetchRewindStatus and voilá!
  • This will also remove the need to inject the store into the coordinators/VCs, removing the coupling and simplifying the code

Let me know what do you think. :)

Momo Ozawa added 2 commits January 25, 2021 13:22
Fetch rewind status on refresh
98e59cb
Add didFinishViewing delegate
a152ede 
Instead of passing ActivityStore, use delegate.
@momo-ozawa momo-ozawa modified the milestones: 16.6, 16.7 Jan 25, 2021
@momo-ozawa momo-ozawa mentioned this pull request Jan 25, 2021
Merged
@momo-ozawa
Copy link
Copy Markdown
Contributor Author

momo-ozawa commented Jan 25, 2021

@leandroalonso

If you call store.fetchRewindStatus(site: site) from ActivityListViewModel.refresh both the aforementioned cases would be fixed

Fixed! 98e59cb

It still doesn't cover the case when you return from a backup/restore, but there are a few ways you can fix that: by using a completion block and/or delegate you can trigger the fetchRewindStatus and voilá!

This will also remove the need to inject the store into the coordinators/VCs, removing the coupling and simplifying the code

Yeah a delegate feels better, thanks for the suggestion! a152ede

@momo-ozawa momo-ozawa changed the title Jetpack Section: Restore Already Running Jetpack Section: Backup / Restore Already Running Jan 25, 2021
@momo-ozawa
Copy link
Copy Markdown
Contributor Author

momo-ozawa commented Jan 25, 2021 •
edited
Loading

Backup / Restore FYI

How to tell if there’s a backup / restore already in progress:

  • When there’s a restore already in progress, and you send a second POST restore request, the job_id in the response is equal to 0
  • When there’s a backup already in progress, and you send a second POST prepare backup request, the rewindID you sent in the request doesn’t match the rewindID returned in the response. The response rewindID matches the rewindID of the backup that's already in progress.

You can test this on Calypso by having two windows open.

leandroalonso
leandroalonso approved these changes Jan 25, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@leandroalonso leandroalonso left a comment

Choose a reason for hiding this comment

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

It all works as described!

I'm still with this issue tho:

When dismissing the screen, I also wasn't able to see the progress (only if I tapped on the "Notify me later" button)

I don't think it's a blocker, we can address it in a subsequent task.

@leandroalonso leandroalonso mentioned this pull request Jan 26, 2021
Merged
Base automatically changed from task/15191-activity-detail-backup to develop January 28, 2021 01:29
Merge branch 'develop' into task/15191-restore-already-running
9e687e1 
# Conflicts:
#	Podfile
#	Podfile.lock
#	WordPress/Classes/ViewRelated/Jetpack/Jetpack Restore/Restore Warning/Coordinators/JetpackRestoreWarningCoordinator.swift
@momo-ozawa momo-ozawa merged commit 55ea99d into develop Jan 28, 2021
@momo-ozawa momo-ozawa deleted the task/15191-restore-already-running branch January 28, 2021 05:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@leandroalonso leandroalonso leandroalonso approved these changes

@emilylaguna emilylaguna Awaiting requested review from emilylaguna

Assignees

@momo-ozawa momo-ozawa

Labels

Jetpack Mobile [Type] Task

Projects

None yet

Milestone

16.7

Development

Successfully merging this pull request may close these issues.

2 participants

@momo-ozawa @leandroalonso