-
Notifications
You must be signed in to change notification settings - Fork 802
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
My Jetpack: Add Backup 'needs-attention' red bubble and notice banner #40512
My Jetpack: Add Backup 'needs-attention' red bubble and notice banner #40512
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 🤖 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! |
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.
When testing this, I noticed that the Backup day in the body of the notice is a day ahead of my local day (i.e., it tested this on Dec 9th, but it's saying Dec 10th was the last time it tried to backup the site)
I assume it's going off UTC time, how hard would it be to localize this? I updated my site's timezone just to be safe, but the day was still wrong.
Also, I don't know if we've had a notice like this before with this much content, the mobile styling needs to be addressed because this looks pretty bad 😅
(I think I designed this so that's on me)
Other than those things, this tested correctly and I think the code looks great 😄
I think that would be a great idea! As a user, I hate non-dismissible notices |
I'm just realizing that in my comment, I make it sound like this is All my idea, lol! 🤦♂️. This is unintentional and I want to clarify that @CodeyGuyDylan made this suggestion (allowing the notice to close) in a previous related PR (Displaying expired plan notices), and as I'm developing more of these notices, I'm agreeing more and more that it should be implemented. 😊 |
…ubble-needs-attention-notice
I did not remember suggesting this 😆 |
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.
I'm still seeing some design issues on the specific screen widths of 481px-585px, below is the worst case scenario:
I think this looks alright, we may want to confirm with design before shipping though if you haven't already, they may have some tweaks 😄
Note: I think we could potentially replace Save every change
with the error in this case 🤔 it looks a bit jumbled with both there
Hey @keoshi! 👋, Screenshot: |
Thanks for the ping @elliottprogrammer, appreciate it!
Love this and it more closely connects the card to the message above. Thanks for adding this!
This one I have some reservations about, just because it makes it look more technical than it should be. If we can indeed point to a specific error —
This way we lead with the problem and then explain why that happened. This will clean up the card and, by consequence, make the troubleshoot button stand out that much more. Also understand this could be a lot more work with mapping different sentences for every possible error state the API might throw, so feel free to push back on what I said above. :) Thanks for being such good shepherds of great UX, @elliottprogrammer and @CodeyGuyDylan ! |
Awesome, thanks @keoshi for your quick response and feedback 🙌 Yes good point, I agree including the error code makes it unnecessarily technical (and not so user friendly). We do provide the error code in this way in the "Backup failed" email we send to users, but it makes more sense there in that case because we are specifically telling the user that they should share the error code/message with their hosting provider who may be able to help them resolve the issue.. The Maybe the best route for now would be to just leave the error code/message out and not even mention it? So instead, if Rewind is throwing the error and unable to even attempt a backup, We can say, "Unable to create a backup", or if the Last backup attempt itself failed we can say, "The last backup attempt failed". So it would just be either one of those messages and the CTA that says "Troubleshoot", which links to: https://jetpack.com/support/backup/troubleshooting-jetpack-backup/ |
Update: I'm currently working to try to map all the possible Backup/Rewind error codes, and translate them into human readable descriptions... So we can at least give the user a basic idea/reason why the backup attempt failed. Standby... |
@elliottprogrammer Thanks for doing that!
Agreed this could either be tackled later or, anything that's super vague and/or doesn't make sense to expose, maybe we can just show a generic error?
In parallel with the above, I wonder if there's anyone of your team @jwebbdev that would be able to support Bryan. We could even create a FG page document these errors, what they mean, and what the human equivalent copy should be? Thanks in advance! |
Thanks Filipe! Yeah, so I found human readable error descriptions for the (wpcom/v1.1)
But where I'm not finding much is for the (wpcom/v2) What I'm trying for, is to come up with human readable descriptions for 'no-credentials', 'backups-deactivated', 'no-credentials-atomic', 'credential-error', 'http-only-error', 'not-accessible', 'Kill switch active', 'error'. (Basically all of them Except 'started', 'finished', and `error-will-retry'). |
I don't seem to be getting a response to the question above, so I asked this same question in the #yamato-backups channel in Slack, here: p1734441559123969-slack-CS8UYNPEE |
@CodeyGuyDylan , I've updated the code, to display more human readable descriptions (when a Backup failure occurs) within a tooltip in the Backup product card, as well as in the red bubble Notice/banner. (See screenshots:) I've updated the PR testing instructions & screenshots. 👍 |
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.
Thank you for all your hard work on getting human readable errors! I think it looks great and functions great!
Below is the only blocking change that I found, I left a few other comments but overall this looks great! I am going to throw an approval on this as I don't want to hold up the deployment any longer. If the tooltip issue gets fixed I think this is good to go 😄 But if you'd like me to check it out again after changes just let me know
I am seeing certain screen sizes where the tooltip is off the screen like this:
</InfoTooltip> | ||
</Text> | ||
<Text variant="body-small" className={ styles.error_description }> | ||
{ __( 'Check out our troubleshooting guide.', 'jetpack-my-jetpack' ) } |
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 is not a strong opinion, but I think this text may be unnecessary since the CTA says troubleshoot
. What do you think?
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.
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.
Is there a copy doc? I've been told the designs aren't always the be-all end-all when it comes to copy 😅
status: status, | ||
backup_status: lastBackupStatus, | ||
feature: 'jetpack-backup', | ||
message: '', //errorDescription, |
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.
Can we remove that commented out variable by chance?
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.
Good catch thanks!
* | ||
* @return {Date} a JavaScript Date object | ||
*/ | ||
function applyTimezone( date: string, offset: number ): Date { |
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 could be a good util function perhaps 🤔
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.
Yes, good point, moved to the utils folder. 👍
The base branch was changed.
This PR add a red bubble and notice/banner when the backup 'needs-attention' status is triggered (when a backup failure is detected).
Screenshot:
Fixes: https://github.com/Automattic/jetpack-roadmap/issues/1430
Proposed changes:
Other information:
Jetpack product discussion
Project Thread: pbNhbs-bJN-p2
Design post: p1HpG7-umC-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
Testing notes: To test this PR, basically we need to force a backup to fail. I tried various ways to make a backup fail, but most my attempts were unsuccessful. The only way I've been able to make a failed backup is by making the site unaccessible, and then queueing the backup to run from the MC Rewind Debugger. To make the site unaccessible, I simply killed my Jurassic Tube tunneling, but there are other various ways to do this such as by stopping Apache, blocking port 80, inserting
die()
orexit()
command in index.php, etc..There is another promising way of making a backup fail due to a "No such file" error, but I believe the method only works on a self-hosted site that is Not running within a Docker container. (I was not able to get it to work on my local dev environment or with Jurassic Ninja (confirmed it does not work on a JN site here: p1733501055164569/1733349201.389539-slack-CS8UYNPEE )
Instructions on how to carry out this method are here: Generating “No such file” errors during backup: pbuNQi-1Wi-p2
If you know of any other ways to trigger a backup failure, please let me know!
Testing steps:
wpcom/v2
, endpoint:/sites/239657125/rewind/backups?force=wpcom
, and verify the status of the last backup isnot-available
.Note: In a followup PR I think I should implement allowing the user to optionally close(x) each red bubble notice (persistent for a temporary amount of time).