Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BackupScheduleSetting: improve BackupScheduled view using useNextBackupSchedule hook #95638

Merged
merged 8 commits into from
Oct 24, 2024

Conversation

Initsogar
Copy link
Contributor

@Initsogar Initsogar commented Oct 24, 2024

Related to https://github.com/Automattic/jetpack-backup-team/issues/715

This PR improves the precision of the next backup time estimation by leveraging the useNextBackupSchedule hook introduced in #95602 that returns the next scheduled backup date.

Proposed Changes

  • Removed the previous logic that calculated hours until the next backup using the last backup date and replaced it with precise scheduling data from the useNextBackupSchedule hook.
    • If there are any minutes left in the calculation, the hours are rounded up to the next hour.

CleanShot 2024-10-23 at 20 56 05@2x

Other minor changes:

  • Removed defaultProps on ActionsButton component, which is now deprecated. Update: I was going to remove this, but it seems we have some types issues related to this, so I prefer to address it in a new PR.
  • Refactor useNextBackupSchedule to named export instead of default export, so we could add more hooks in the same hooks file without issues.
  • Update schedule backup message in settings page as suggested in p1729507641013709-slack-C07LJAE7AVD
Scenario Before After
Time set by default CleanShot 2024-10-23 at 21 36 41@2x CleanShot 2024-10-23 at 21 37 07@2x
Time set by user CleanShot 2024-10-23 at 21 34 07@2x CleanShot 2024-10-23 at 21 32 16@2x

Why are these changes being made?

The previous logic considered the last completed backup to calculate a relative date, which is not always accurate, especially since we introduced on-demand backups.

https://github.com/Automattic/jetpack-backup-team/issues/715

Testing Instructions

  • Spin up a Jetpack Cloud live branch.
  • Pick a site with Jetpack VaultPress Backup plan with no backups today.
  • Navigate to the Backup page.
  • You should see the Backup Scheduled view and the time should be relative to the site scheduled time.
  • You can try updating the daily backup time setting and see how it affects the "In the next % hour/s" message.

Pre-merge Checklist

  • Has the general commit checklist been followed? (PCYsg-hS-p2)
  • Have you written new tests for your changes?
  • Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
  • Have you checked for TypeScript, React or other console errors?
  • Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
  • Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
  • For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-aUh-p2)?

- Replaced the calculation with precise scheduling data from the useNextBackupSchedule hook.
- Added logic to round up the hours when there are remaining minutes for more accurate time estimation.
@matticbot
Copy link
Contributor

This PR modifies the release build for the following Calypso Apps:

For info about this notification, see here: PCYsg-OT6-p2

  • notifications
  • wpcom-block-editor

To test WordPress.com changes, run install-plugin.sh $pluginSlug update/schedule-backups-upcoming-view on your sandbox.

@matticbot
Copy link
Contributor

matticbot commented Oct 24, 2024

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~227 bytes added 📈 [gzipped])

name                           parsed_size           gzip_size
jetpack-cloud-agency-sites-v2       +663 B  (+0.0%)     +154 B  (+0.0%)
backup                              +663 B  (+0.1%)     +154 B  (+0.0%)
a8c-for-agencies-sites              +663 B  (+0.0%)     +154 B  (+0.0%)
jetpack-cloud-settings              +139 B  (+0.0%)      +73 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

@Initsogar Initsogar marked this pull request as ready for review October 24, 2024 02:40
@Initsogar Initsogar requested a review from a team October 24, 2024 02:42
@matticbot matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 24, 2024
@Initsogar Initsogar added the [Feature] Backups The Jetpack Backup product label Oct 24, 2024
@Initsogar
Copy link
Contributor Author

@ilonagl here I included the changes on the changes on the scheduled setting message:

Scenario Before After
Time set by default CleanShot 2024-10-23 at 21 36 41@2x CleanShot 2024-10-23 at 21 37 07@2x
Time set by user CleanShot 2024-10-23 at 21 34 07@2x CleanShot 2024-10-23 at 21 32 16@2x

const { date: nextBackupDate } = useNextBackupSchedule();

// Calculate the time difference for hours and minutes
const hoursForNextBackup = nextBackupDate.diff( today, 'hours' );
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was testing this on Jetpack Cloud live branch and noted this error:

image

It seems nextBackupDate could be potentially null, so I'd need to add some guard checks here.

@Initsogar Initsogar merged commit d8bc948 into trunk Oct 24, 2024
11 checks passed
@Initsogar Initsogar deleted the update/schedule-backups-upcoming-view branch October 24, 2024 16:45
@github-actions github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Oct 24, 2024
@a8ci18n
Copy link

a8ci18n commented Oct 24, 2024

This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/16942926

Some locales (Hebrew, Japanese) have been temporarily machine-translated due to translator availability. All other translations are usually ready within a few days. Untranslated and machine-translated strings will be sent for translation next Monday and are expected to be completed by the following Friday.

Thank you @Initsogar for including a screenshot in the description! This is really helpful for our translators.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Backups The Jetpack Backup product
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants