Skip to content

Jetpack Backup: show progress#15776

Merged
leandroalonso merged 12 commits intodevelopfrom
task/backup_status_cell
Feb 4, 2021
Merged

Jetpack Backup: show progress#15776
leandroalonso merged 12 commits intodevelopfrom
task/backup_status_cell

Conversation

@leandroalonso
Copy link
Copy Markdown
Contributor

@leandroalonso leandroalonso commented Feb 3, 2021

Part of #15191

This PR shows the backup progress in Activity Log and Backups screen.

To test

Creating a backup from the app

  1. Go to Activity Log or Backups screen
  2. Tap the 3 dots in the first available cell
  3. Tap "Download Backup"
  4. Tap "Create downloadable file"
  5. Tap "Ok notify me"
  6. Check that the progress cell is displayed and disappear once the backup is completed

Creating a backup from the site

  1. Go to your site and trigger a backup there
  2. In the app, open Activity Log or Backups Screen
  3. The progress bar should be displayed and disappear once the backup is completed

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.

@leandroalonso leandroalonso added this to the 16.7 milestone Feb 3, 2021
@leandroalonso leandroalonso self-assigned this Feb 3, 2021
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Feb 3, 2021 •
edited
Loading

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

@leandroalonso leandroalonso mentioned this pull request Feb 3, 2021
Merged
houndci-bot
houndci-bot reviewed Feb 3, 2021
View reviewed changes
@peril-wordpress-mobile
Copy link
Copy Markdown

peril-wordpress-mobile bot commented Feb 3, 2021 •
edited
Loading

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

emilylaguna
emilylaguna suggested changes Feb 3, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@emilylaguna emilylaguna left a comment

Choose a reason for hiding this comment

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

Heya! Looking good, I left some comments below. Also when I start the backup on .com, the once it's finished the status never updates on the app (see video)

backup.mov

WordPress/Classes/Stores/ActivityStore.swift
Comment on lines +548 to +550
} else {
// TODO: show download cell
}
Copy link
Copy Markdown
Contributor

@emilylaguna emilylaguna Feb 3, 2021

Choose a reason for hiding this comment

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

TODO coming soon?

Copy link
Copy Markdown
Contributor Author

@leandroalonso leandroalonso Feb 3, 2021

Choose a reason for hiding this comment

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

Yes!

WordPress/Classes/ViewRelated/Activity/BaseActivityListViewController.swift Outdated
}
// TODO: fetch backup status

self.viewModel.refresh()
Copy link
Copy Markdown
Contributor

@emilylaguna emilylaguna Feb 3, 2021

Choose a reason for hiding this comment

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

Should we remove the guard and just do:

Suggested change
self.viewModel.refresh()
self?.viewModel.refresh()

Copy link
Copy Markdown
Contributor Author

@leandroalonso leandroalonso Feb 3, 2021

Choose a reason for hiding this comment

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

Done in ed80398

@leandroalonso
Copy link
Copy Markdown
Contributor Author

leandroalonso commented Feb 3, 2021

@emilylaguna I'm not being able to reproduce this behavior here. :( (Atomic site, triggering the backup from the web)

@leandroalonso leandroalonso mentioned this pull request Feb 3, 2021
Closed
43 tasks
momo-ozawa
momo-ozawa approved these changes Feb 4, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@momo-ozawa momo-ozawa left a comment

Choose a reason for hiding this comment

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

@leandroalonso @emilylaguna
I was able to reproduce the error Emily was seeing exactly once, but that's it. I think I saw it on a JN self-hosted site. I tried again with the same site but wasn't able to reproduce the error.

The PR works as described otherwise, so I'm going to approve it.
I'm thinking we can keep an eye out for when this happens again, then address it in a future PR.

emilylaguna
emilylaguna approved these changes Feb 4, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@emilylaguna emilylaguna left a comment

Choose a reason for hiding this comment

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

LGTM! 🚀

@leandroalonso leandroalonso merged commit ac13c4a into develop Feb 4, 2021
@leandroalonso leandroalonso deleted the task/backup_status_cell branch February 4, 2021 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@emilylaguna emilylaguna emilylaguna approved these changes

@momo-ozawa momo-ozawa momo-ozawa approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@leandroalonso leandroalonso

Labels

Jetpack Mobile Jetpack [Type] Task

Projects

None yet

Milestone

16.7

Development

Successfully merging this pull request may close these issues.

4 participants

@leandroalonso @emilylaguna @houndci-bot @momo-ozawa