-
Notifications
You must be signed in to change notification settings - Fork 800
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
Protect card: Add the Scan/Threats status column (with tooltip) #38165
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. Once your PR is ready for review, check one last time that all required checks appearing at the bottom of this PR are passing or skipped. Jetpack plugin: The Jetpack plugin has different release cadences depending on the platform:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Backup plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Boost plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Search plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Social plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Starter Plugin plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Protect plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Videopress plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. Migration plugin:
If you have any questions about the release process, please ask in the #jetpack-releases channel on Slack. |
Oh nice! I forgot about that! Ok, thanks! 👍
Ok yeah, I was initially going to handle that in a future PR, but I went ahead and removed it in this PR. 👍
Yeah, I was also going to tackle that in a future PR too, but I went ahead and handled it here is this PR too. 👍
No that is not expected. I updated the testing instructions slightly to first only change the Akismet version (for the free scan), and then add the $eicar signature to trigger a critical threat (for the paid scan). I performed the updated steps myself and it worked as expected.
Yeah sure, I agree we could try to further simplify, but I think the effort should be applied after I get most all the overall functionality & logic of the entire card in place. You see, I say this just because while I'm building this out, I'm often changing or even simplifying some things from the PR before it, and additionally, my main goal at first is to try to get a basic MVP done and in front of users as quickly as possible. Then after that, we can iterate and improve on it, over and over as much as we want. Thats was my thinking anyway. 🤷♂️ 😉 Thanks so much for the review @robertsreberski! I've addressed all your feedback. Would you mind taking a look again to confirm you don't see any other issues? Much appreciated! 🙌 |
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.
Code looks a lot better thanks!
Lovely that you collaborated with @CodeyGuyDylan on translation strings - the final idea is really neat!
I've left a bit more comments, but the code is almost there 🏁
@@ -31,7 +31,7 @@ type ScanItem = { | |||
checked: boolean; | |||
name: string; | |||
slug: string; | |||
threats: string[]; | |||
threats: object[]; |
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 define the object here, or the structure is unknown?
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, will do. 👍
|
||
&__status-text { | ||
margin-right: 1px; | ||
letter-spacing: -0.24px; |
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.
Do we need such specific spacing?
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.
It was exactly per the design. I'll leave a comment. 👍
|
||
&__heading { | ||
font-size: var(--font-body-extra-small); | ||
color: var(--jp-gray-100); | ||
font-weight: 500; | ||
margin-bottom: 10px; |
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 reuse --spacing-base
?
}; | ||
|
||
const noDescription = useCallback( () => null, [] ); |
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.
Maybe we can leave a comment for the future reference that if we gonna remove description to more cards, we should consider extending <ProductCard />
functionality to support that.
Clever workaround here though! 🙌
if ( is_array( $purchases_data ) && ! empty( $purchases_data ) ) { | ||
foreach ( $purchases_data as $purchase ) { | ||
// Protect is available as jetpack_scan product and as part of the Security or Complete plan. | ||
if ( strpos( $purchase->product_slug, 'jetpack_scan' ) !== false || str_starts_with( $purchase->product_slug, 'jetpack_security' ) || str_starts_with( $purchase->product_slug, 'jetpack_complete' ) ) { |
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.
We do we use strpos()
for jetpack_scan
product slug, but str_starts_with()
for others?
Also, we could break the condition down to multiple lines, can be more readable that way 🤔
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.
We do we use
strpos()
for jetpack_scan product slug, butstr_starts_with()
for others?
I'm not quite sure, to be honest... This is just exactly how it is also done in many other of the product classes, so I just copied it over and did the same (just to play it safe). More specifically, the classes for Anti-spam, Boost, Creator, Search, Social, Stats, VideoPress, and now Protect all do it exactly like this in the has_paid_plan_for_product()
function. 🤷♂️
Yes good idea, I'll break it down to be more readable. 👍
...ects/packages/my-jetpack/_inc/components/product-cards-section/protect-card/scan-threats.tsx
Outdated
Show resolved
Hide resolved
...ects/packages/my-jetpack/_inc/components/product-cards-section/protect-card/scan-threats.tsx
Outdated
Show resolved
Hide resolved
...ects/packages/my-jetpack/_inc/components/product-cards-section/protect-card/scan-threats.tsx
Outdated
Show resolved
Hide resolved
...ects/packages/my-jetpack/_inc/components/product-cards-section/protect-card/scan-threats.tsx
Outdated
Show resolved
Hide resolved
Also, thank you @elliottprogrammer for clarifying the instructions regarding testing. I do get 1 treat now on the free version. From what I understand, the previous treat (wrong version of Akismet) still persists on the website, why is it secure then? It might be a question to the Scan team, but maybe you have an idea 🤔 |
@robertsreberski, |
Thanks again for the review @robertsreberski! I've addressed your additional feedback. Let me know if you see anything else or have any other suggestions for improvement. Thanks Robert!! 🙌 |
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.
Thanks for working on the updates @elliottprogrammer ! They look very good, and the PR tests well. 🟢
The base branch was changed.
This PR populates the Scan/Threats section on the My Jetpack Protect card with the current scan status from Protect.
See Design P2: p1HpG7-rFA-p2 (Figma link is within the post)
Screenshots are shown below in the testing instructions.
Proposed changes:
Note: Adding a "loading/currently scanning" state in the My Jetpack Protect product card is not included in this PR and will be handled in a future PR. We need to add the Scan API js-package first.
Other information:
Jetpack product discussion
Project Thread: pbNhbs-aP6-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
You can reference the link to the Figma design in the design P2 post: p1HpG7-rFA-p
(tampermonkey does not shorthand the direct Figma link for some reason, so you'll need to get it from the design post).
/wp-admin/plugin-editor.php
akismet.php
file:vim htdocs/wp-content-plugins/akismet/akismet.php
Version:
" to:3.1.4
define( 'AKISMET_VERSION', '3.1.4' );
Note: Adding a "loading/currently scanning" state in the My Jetpack Protect product card is not included in this PR and will be handled in a future PR.
akismet.php
):-
$eicar = "X5O!P%@AP[4\PZX54(P^)7CC)7}$EICAR-CRITICAL-ANTIVIRUS-TEST-FILE!$H+H*";
Again note: adding a "loading/currently scanning" state in the My Jetpack Protect product card is not included in this PR and will be handled in a future PR.