-
Notifications
You must be signed in to change notification settings - Fork 809
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
Add/transient to cache backup api calls #41608
Conversation
Thank you for your PR! When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:
This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖 The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available. Follow this PR Review Process:
Still unsure? Reach out in #jetpack-developers for guidance! |
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
Code Coverage SummaryNo summary data is avilable for parent commit 4f69df2, cannot calculate coverage changes. 😴 If that commit is a feature branch rather than a trunk commit, this is expected. Otherwise, you might try re-running the Tests / Publish coverage data job on this PR once the corresponding job on the trunk commit has succeeded. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Cache for 5 minutes if there was an error with their last backup, that way they can check to see if it was fixed if they try troubleshooting it
- Cache for 1 hour if the previous backup was successful so that, if there is a backup with an error, they'll know about it within an hour if they go to My Jetpack
Ok yes, this ⬆️ seems reasonable. 👍
Tested, works as described. Looks all good, except for an unused and unnecessary variable constant that was left in class-initializer.php
that should be removed.
LGTM! 👍
@@ -65,6 +65,7 @@ class Initializer { | |||
const VIDEOPRESS_STATS_KEY = 'my-jetpack-videopress-stats'; | |||
const VIDEOPRESS_PERIOD_KEY = 'my-jetpack-videopress-period'; | |||
const MY_JETPACK_RED_BUBBLE_TRANSIENT_KEY = 'my-jetpack-red-bubble-transient'; | |||
const BACKUP_STATUS_TRANSIENT_KEY = 'my-jetpack-backup-status'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This constant is unused in this file. Was it left here in this file unintentionally?
I believe it should be removed. 😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ooh nice catch, thanks I'll remove that!
Proposed changes:
Currently we are making 2 API calls to WPCOM for the backup card (if a backup plan is active) on every page load
Other information:
Jetpack product discussion
N/A
Does this pull request change what data or activity we track or use?
No
Testing instructions:
/wp-admin/admin.php?page=jetpack-backup
and trigger a backup/rewind
and/rewind/backup
were madeIf you'd like to check the transient values, you can run this locally and add the following line to
projects/packages/my-jetpack/src/products/class-backup.php
line 249