-
Notifications
You must be signed in to change notification settings - Fork 805
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 red bubble and warning Notice when paid plans are expired or expiring soon. #40115
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! |
Thanks for reviewing @CodeyGuyDylan! 🙌
Yes good catch, thank you! fixed this. 👍
Yes, for expiring-soon products I'm sending the user straight to checkout with the renewal in the cart. For expired products, I'm sending then to the purchase management page so they can either renew the product, or choose to remove it if they wish.
I left a comment about this in the previous PR related to the age of the expiring products notice. Let me know what you think.
Good catch, thanks! This is fixed. 👍 |
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 functionality looks good to me and it works as described. I left some minor comments, but I can mark it as approved already.
Thanks for addressing the other comments, the feature works well.
@@ -133,7 +133,6 @@ public static function get_products_classes() { | |||
'protect' => Products\Protect::class, | |||
'videopress' => Products\Videopress::class, | |||
'stats' => Products\Stats::class, | |||
'ai' => Products\Jetpack_Ai::class, |
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 not sure if whether it's safe to remove ai
here. I remember there was a reason to keep, but I don't remember what the reason was lol
I tried to test it in different scenarios and it works as expected so it might have changed, but I wanted to mention this anyway.
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.
Ok, thanks @IanRamosC! I left a comment to the author on the PR where this duplicate class was added originally, and he stated the same thing but doesn't remember exactly what the reason was either. (See: #35910 (review))
I added it back for now, just in case.. And also added a code comment(s) referencing the original PR for future reference. 👍
); | ||
|
||
// Notice Content | ||
switch ( product ) { |
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 feel like this switch will get bigger and bigger as we add more products with specific messaging. I was thinking if we could have the notice text content in one place and a somewhat generic function that uses that text content to build the JSX a bit more dynamically. I don't have a strong opinion right now, though. I'm mostly interested in having this code a bit easier to extend.
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.
Yeah I initially had the notice content split up into 2 files, one for expired products, and one for expiring soon, which simplified each file quite a bit, but then it was suggested that I combine the files into one, adding more conditionals and so forth possibly making things a bit more confusing to extend. But, given the nature of each product maybe needing different text, different number of paragraphs, different jsx structure, and possibly displaying other differences (such as icons, images, etc), I fear trying to combine and generalize/optimize any more on this file... For now anyway until we know more exactly what the content of each product will look like. We can always clean up or refactor this file more as needed, as we (maybe) eventually add more content for each specific product. 👍😃
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.
That was my suggestion 😅 I tend to try to combine files when possible, but that's not always the best solution so my apologies for not thinking of the downsides of it
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 PR adds red bubble functionality and displays a warning/error Notice in My Jetpack when any paid plans are expiring soon or expired.
Fixes https://github.com/Automattic/jetpack-roadmap/issues/1937
Proposed changes:
Other information:
Jetpack product discussion
Project Thread: pbNhbs-bJN-p2
Design Thread: p1HpG7-umC-p2
Does this pull request change what data or activity we track or use?
No
Testing instructions:
"Expiring soon" Notice for Jetpack Security:
"Expired Notice" for Jetpack Security: