-
Notifications
You must be signed in to change notification settings - Fork 843
My Jetpack: update Backup card to inactive when backups are deactivated #46299
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
base: trunk
Are you sure you want to change the base?
Conversation
|
Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.
Interested in more tips and information?
|
|
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 🤖 Follow this PR Review Process:
If you have questions about anything, reach out in #jetpack-developers for guidance! |
Code Coverage SummaryCoverage changed in 3 files.
|
…ps-deactivated-card
cdcfcdb to
23b4128
Compare
23b4128 to
44d854d
Compare
162010f to
44d854d
Compare
|
Hi @Automattic/triforce! When you get a chance, I’d appreciate it if you could take a look at this PR. Your My Jetpack experience would be super helpful 🙌 |
manzoorwanijk
left a comment
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 looks good to me.
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.
Pull request overview
This PR addresses a UX issue where My Jetpack incorrectly displays "The last backup attempt failed" error state when backups are intentionally deactivated (status: backups-deactivated). The fix detects this specific status and displays a neutral, informative message with a support contact link instead of an error state.
Key changes:
- Override Backup product status to return INACTIVE instead of ERROR for deactivated backups
- Display informative message: "Backup was manually turned off. Please contact support to reactivate it."
- Link to support form with pre-filled subject and site URL
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
projects/packages/my-jetpack/src/products/class-backup.php |
Adds get_status() override to return INACTIVE for deactivated backups; modifies transient caching logic to cache all array statuses |
projects/packages/my-jetpack/_inc/components/product-cards-section/backup-card/index.jsx |
Adds deactivated state detection and renders new support contact message instead of error UI |
projects/packages/my-jetpack/_inc/components/product-card/index.tsx |
Adds useEffect to reset loading state when admin prop becomes false |
projects/packages/my-jetpack/changelog/fix-backup-deactivated-card |
Changelog entry documenting the fix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| public static function get_status() { | ||
| // Get the default status from parent. | ||
| $status = parent::get_status(); | ||
|
|
||
| // Check if backups are deactivated (not an error, just manually turned off). | ||
| $needs_attention = static::does_module_need_attention(); | ||
| if ( is_array( $needs_attention ) && 'backups-deactivated' === $needs_attention['data']['status'] ) { | ||
| // Return INACTIVE status for deactivated backups (neutral, not attention-grabbing). | ||
| return \Automattic\Jetpack\My_Jetpack\Products::STATUS_INACTIVE; | ||
| } | ||
|
|
||
| return $status; | ||
| } |
Copilot
AI
Dec 18, 2025
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.
The new get_status() method that handles the backups-deactivated scenario lacks test coverage. Since the Backup product has existing PHPUnit tests, this new behavior should be tested to ensure it correctly returns STATUS_INACTIVE when backups are deactivated and doesn't break other status scenarios.
| } | ||
|
|
||
| if ( is_array( $backup_failed_status ) && $backup_failed_status['type'] === 'error' ) { | ||
| if ( is_array( $backup_failed_status ) ) { |
Copilot
AI
Dec 18, 2025
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.
The condition was changed from checking if the array has type 'error' to always caching any array. This means non-error states like 'backups-deactivated' will now be cached with a 5-minute TTL instead of the 1-hour TTL. This creates inconsistency - error states should have shorter cache times for quicker updates, while stable states like deactivation should have longer cache times. Consider checking the backup status to determine appropriate cache duration.
|
|
||
| // Check if backups are deactivated (not an error, just manually turned off). | ||
| $needs_attention = static::does_module_need_attention(); | ||
| if ( is_array( $needs_attention ) && 'backups-deactivated' === $needs_attention['data']['status'] ) { |
Copilot
AI
Dec 18, 2025
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 logic has a potential issue: it checks the data structure without verifying that the 'data' key exists first. If 'data' is not set in the array, this will cause a PHP notice for accessing an undefined index. Add a check for isset($needs_attention['data']) before accessing the nested 'status' key.
| if ( is_array( $needs_attention ) && 'backups-deactivated' === $needs_attention['data']['status'] ) { | |
| if ( | |
| is_array( $needs_attention ) | |
| && isset( $needs_attention['data'] ) | |
| && is_array( $needs_attention['data'] ) | |
| && isset( $needs_attention['data']['status'] ) | |
| && 'backups-deactivated' === $needs_attention['data']['status'] | |
| ) { |
| const { status, doesModuleNeedAttention } = detail; | ||
| const lastBackupFailed = !! doesModuleNeedAttention; | ||
| const { status: lastBackupStatus } = doesModuleNeedAttention || {}; | ||
| const lastBackupStatus = doesModuleNeedAttention?.data?.status || {}; |
Copilot
AI
Dec 18, 2025
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.
The default value should be an empty string or undefined, not an empty object. The code assigns {} as a default when doesModuleNeedAttention?.data?.status is falsy, but later at line 95, it's compared to a string ('backups-deactivated'). An empty object {} will never equal a string, so this works, but it's semantically incorrect and could cause confusion. Consider using '' or undefined as the default value.
| const lastBackupStatus = doesModuleNeedAttention?.data?.status || {}; | |
| const lastBackupStatus = doesModuleNeedAttention?.data?.status || ''; |
| useEffect( () => { | ||
| if ( ! admin && isActionLoading ) { | ||
| setIsActionLoading( false ); | ||
| } | ||
| }, [ admin, isActionLoading ] ); |
Copilot
AI
Dec 18, 2025
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 useEffect creates an infinite loop risk. When admin becomes false and isActionLoading is true, the effect calls setIsActionLoading(false), which updates state and triggers a re-render. Since isActionLoading is in the dependency array, this could cause unnecessary re-renders. The effect should only run when admin changes from true to false, not on every render where both conditions are met. Consider using a ref to track the previous admin value or restructure the logic to avoid this pattern.
…ps-deactivated-card
Fixes BACKUP-298
When backups are intentionally deactivated (status:
backups-deactivated), My Jetpack shows "The last backup attempt failed" which is confusing because the backup isn't failing—it's just turned off.Proposed changes:
Other information:
Jetpack product discussion
https://linear.app/a8c/issue/BACKUP-298/#comment-85de1b46
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Note
The easiest way to test this is locally by mocking the backups deactivated scenario as described below. An alternative way is to test using a site with backups deactivated (that had attempted at least one backup), but this will require some internal setup.
tools/docker/mu-plugins/0-snippets.phpto mock the backups deactivated scenario:jetpack docker exec wp transient delete 'my-jetpack-backup-status'/wp-admin/admin.php?page=my-jetpackExpected behavior:
Validating other scenarios:
You can update the latest backup status away from
backups-deactivatedto ensure other scenarios works as expected, like:errorfinishedAfter updating the snippet, remember to delete the
my-jetpack-backup-statustransient :)