My Jetpack: Allow the "plugin(s) needs installed" notice to be closed with persistence. #41617
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! |
github-actions
bot
added
the
[Status] Needs Author Reply
Code Coverage SummaryCoverage changed in 2 files.
|
There was a problem hiding this comment.
Very minor nitpick, but the positioning of the X seems slightly off here
I think it has to do with the padding overlapping the X itself
But honestly it looks fine to the naked eye so it's your choice whether you want to change this or not 😅
This seems to work well and I love that you're adding it! I think my comment about the early return should be done, but it's a pretty small change so I'll go ahead and approve so you can merge after you've done that
| @@ -1157,7 +1157,9 @@ public static function alert_if_paid_plan_requires_plugin_install_or_activation( | |||
| } | |||
|
|
|||
| foreach ( $plugins_needing_installed_activated as $plan_slug => $plugins_requirements ) { | |||
| $red_bubble_slugs[ "$plan_slug--plugins_needing_installed_activated" ] = $plugins_requirements; | |||
| if ( empty( $_COOKIE[ "$plan_slug--plugins_needing_installed_dismissed" ] ) ) { | |||
There was a problem hiding this comment.
Nitpick, I think we could move this check to the top to avoid having to do all the above stuff in this function, just to optimize performance
Something like
if ( ! empty( $_COOKIE[ "$plan_slug--plugins_needing_installed_dismissed" ] ) ) {
$red_bubble_slugs;
}There's other checks like it above so I don't think it would be out of place, and it doesn't rely on any data (other than the cookie) so it can be first
There was a problem hiding this comment.
There's other checks like it above so I don't think it would be out of place, and it doesn't rely on any data (other than the cookie) so it can be first
Actually no I don't think I can move this check up to the top @CodeyGuyDylan ... because it does rely on the $plan_slug which is only available within the foreach loop. So I think it needs to remain where it is.. Unless I'm missing something? 🤔
There was a problem hiding this comment.
I am very sorry I somehow missed that part! However, I still think there could potentially be a better way to do this. This works, however, so I will approve it for now and leave it up to you (I'll approve for real this time 😅 )
The way it works now, a new cookie will be added for each notice that is dismissed per product with the structure of this
Name: {slug}--plugins_needing_installed_dismissed Value: 1
I think for cleaning up potential cookies and so that we can return earlier, it might be better to have a single cookie that handles all of these in a structure like this:
Name: plugins_needing_installed_dismissed Value: {slugs in comma separated string}
This would avoid having to create multiple cookies if they dismiss more than one, and then we could have a check like
$cookie_value = $_COOKIE[ "plugins_needing_installed_dismissed"]
if ( in_array( explode( ',', $cookie_value ), $plugins_needing_installed_activated ) ) {
return $red_bubble_slugs;
}However, I fully acknowledge that the amount of users dismissing multiple of these banners and the performance increase of this rework is minimal, so I won't push to hard to update it 😄
There was a problem hiding this comment.
Thanks for your additional feedback Dylan!
However, I went ahead and shipped it as-is..
Firstly, just for the simplicity of knowing that for each notice that is displayed to the user, if they click to dismiss it, there is a specific cookie for that particular notice that controls the dismissal of the notice and when the dismissal expires.
Secondly, I think the alternative approach you've described above will have some unwanted behavior, because the approach doesn't take into account the cookies expiration date. (i.e.- Currently the expiration is set to 14 days, meaning that if the user dismisses the notice, but then after 14 days if the user hasn't installed the required plugins needed for the particular paid plan that the notice is displaying for, then the notice will display to the user again). The alternative approach (using a shared cookie) means that the cookie expiration date will no longer be tied specifically to each notice individually.., If that makes sense.
|
Hey @CodeyGuyDylan, thanks very much for reviewing!
Changes were requested initially, so this PR still needs re-reviewed/approval. 🙏 |
🤦🏻 I apologize for that oversight in my review, not sure how I missed that. Thank you for pointing that out to me! |
| @@ -1157,7 +1157,9 @@ public static function alert_if_paid_plan_requires_plugin_install_or_activation( | |||
| } | |||
|
|
|||
| foreach ( $plugins_needing_installed_activated as $plan_slug => $plugins_requirements ) { | |||
| $red_bubble_slugs[ "$plan_slug--plugins_needing_installed_activated" ] = $plugins_requirements; | |||
| if ( empty( $_COOKIE[ "$plan_slug--plugins_needing_installed_dismissed" ] ) ) { | |||
There was a problem hiding this comment.
I am very sorry I somehow missed that part! However, I still think there could potentially be a better way to do this. This works, however, so I will approve it for now and leave it up to you (I'll approve for real this time 😅 )
The way it works now, a new cookie will be added for each notice that is dismissed per product with the structure of this
Name: {slug}--plugins_needing_installed_dismissed Value: 1
I think for cleaning up potential cookies and so that we can return earlier, it might be better to have a single cookie that handles all of these in a structure like this:
Name: plugins_needing_installed_dismissed Value: {slugs in comma separated string}
This would avoid having to create multiple cookies if they dismiss more than one, and then we could have a check like
$cookie_value = $_COOKIE[ "plugins_needing_installed_dismissed"]
if ( in_array( explode( ',', $cookie_value ), $plugins_needing_installed_activated ) ) {
return $red_bubble_slugs;
}However, I fully acknowledge that the amount of users dismissing multiple of these banners and the performance increase of this rework is minimal, so I won't push to hard to update it 😄
elliottprogrammer
deleted the
add/mj-notice-close-plugins-needs-installed
branch
github-actions
bot
removed
the
[Status] Needs Author Reply
This PR allows the "Needs plugin(s) installed" notice to be manually closed (by clicking the close(X) icon), and remain closed for a set period (14 days in this case).
Closes https://github.com/Automattic/jetpack-roadmap/issues/2294
Proposed changes:
Other information:
Jetpack product discussion
Part of overall task: https://github.com/Automattic/jetpack-roadmap/issues/2050
Does this pull request change what data or activity we track or use?
No
Testing instructions:
plugins/jetpackdirectory toplugins/jetpack-disabled