jetpack icon indicating copy to clipboard operation
jetpack copied to clipboard

My Jetpack: Add "Expired" & "Expires soon" statuses to product cards

Open elliottprogrammer opened this issue 1 year ago • 2 comments

WIP: Work in progress...

Fixes #

Proposed changes:

Other information:

  • [ ] Have you written new tests for your changes, if applicable?
  • [ ] Have you checked the E2E test CI results, and verified that your changes do not break them?
  • [ ] Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

  • Go to '..'

elliottprogrammer avatar Oct 18 '24 05:10 elliottprogrammer

Are you an Automattician? Please test your changes on all WordPress.com environments to help mitigate accidental explosions.

  • To test on WoA, go to the Plugins menu on a WordPress.com Simple site. Click on the "Upload" button and follow the upgrade flow to be able to upload, install, and activate the Jetpack Beta plugin. Once the plugin is active, go to Jetpack > Jetpack Beta, select your plugin, and enable the add/mj-expired-products-cards branch.

  • To test on Simple, run the following command on your sandbox:

    bin/jetpack-downloader test jetpack add/mj-expired-products-cards
    

Interested in more tips and information?

  • In your local development environment, use the jetpack rsync command to sync your changes to a WoA dev blog.
  • Read more about our development workflow here: PCYsg-eg0-p2
  • Figure out when your changes will be shipped to customers here: PCYsg-eg5-p2

github-actions[bot] avatar Oct 18 '24 05:10 github-actions[bot]

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

  • :white_check_mark: Include a description of your PR changes.
  • :white_check_mark: Add a "[Status]" label (In Progress, Needs Team Review, ...).
  • :white_check_mark: Add testing instructions.
  • :white_check_mark: Specify whether this PR includes any changes to data or privacy.
  • :white_check_mark: Add changelog entries to affected projects

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 :robot:


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:

  1. Ensure all required checks appearing at the bottom of this PR are passing.
  2. Choose a review path based on your changes:
    • A. Team Review: add the "[Status] Needs Team Review" label
      • For most changes, including minor cross-team impacts.
      • Example: Updating a team-specific component or a small change to a shared library.
    • B. Crew Review: add the "[Status] Needs Review" label
      • For significant changes to core functionality.
      • Example: Major updates to a shared library or complex features.
    • C. Both: Start with Team, then request Crew
      • For complex changes or when you need extra confidence.
      • Example: Refactor affecting multiple systems.
  3. Get at least one approval before merging.

Still unsure? Reach out in #jetpack-developers for guidance!

github-actions[bot] avatar Oct 18 '24 05:10 github-actions[bot]

It seems like you need to update the PR before merging

IanRamosC avatar Oct 25 '24 00:10 IanRamosC

First, thanks very much for the reviews @CodeyGuyDylan! 😃 Oh, my apologies Dylan, I meant to address/answer these points/feedback from your first review.

I mentioned this is the other PR but wanted to make sure it didn't get lost. I don't think we should be showing the "expired" status if it's been expired for a very long time. My gut reaction is we probably shouldn't show it if it's been expired for longer than a month

For not showing a product as "expired" after it's been expired for a certain amount of time, here's my thoughts: If we don't show when the product is expired (after a certain time), then what status do we show it as then? Just "Active"?, or "Needs plan"? For products that require a paid plan to use them, if we show them as Active, it won't actually be active because it's expired. But we show the product as "Needs plan" instead, I don't think the user can re-purchase the same plan until they "Remove" the expired one first. Something about showing a particular product card (like Backup, or whatever) like nothing is wrong with it, but then when the user clicks on the card to use it, it's not working properly because the plan subscription has been expired. It just seems like it might be opening a big can of worms.. When a product subscription expires, the user really should either 1.- Renew the subscription, Or 2.- Remove it (in the purchase management section). If they Remove the subscription, the expiration status and notice goes away. If they Renew the subscription, the status and notice goes away. Thats why when a product is expired, the Notice CTA, I pointed to the purchase/subscription management section on wordpress.com where the user can either "Remove" the subscription or "Renew" it. However if the product is expiring soon, the Notice CTA points straight to checkout with the product renewal in the cart. I noticed that Atomic sites also don't hide the plan expiration notice (after any amount of time) either. In fact Atomic sites have the expiration notice displaying all over the place, on many different pages all throughout wp-admin (Every one of the settings pages (14 pages), the WP Dashboard page, the Hosting -> home page, the Jetpack - Akismet page, and others I think.), where I believe the thinking is that the user is expected to either renew the subscription, or remove it, if they want the notice to go away. (see screenshots: Here's only a few, out of many, many:)

Screen Shot 2024-11-25 at 20 16 56 Screen Shot 2024-11-25 at 20 18 29 Screen Shot 2024-11-25 at 20 19 50

Anyway, so let me know your thoughts. Let me know if maybe I'm missing something, or maybe over-thinking it?

I did however make a change in the code where I added in the ability to hide/don't show the expiration status after 2 months after the expiration date. I didn't turn it on by default. But it would only be a matter of passing in a true value into a function call, if we should decide to go that route. 😉🤷‍♂️

I noticed the "needs connection" error shows as a red error. I'm wondering if it should be yellow since the status is also yellow? Also, if the "Needs connection" error has a red or maybe yellow border, should the "Needs plan" status also have one?

Hmm, I changed/fixed this prior to your review.. I wonder if your branch (or Jetpack Beta branch) was not up to the latest? I changed the product card border back to how it was originally, where connection warnings and other warnings/errors do not show a red or yellow border around the product cards. Only the expired/expiring products show the red/yellow borders around them. 🙂

elliottprogrammer avatar Nov 26 '24 03:11 elliottprogrammer

Thats why when a product is expired, the Notice CTA, I pointed to the purchase/subscription management section on wordpress.com where the user can either "Remove" the subscription or "Renew" it.

This is a good point and maybe fixes the issue I had with it 🤔 I don't know that, as a user, I would find it clear that the way to make the notification go away is to remove the subscription altogether, though I guess I would probably try it eventually. (I hate notifications 😆). I think we are probably okay to keep it how it is then if that's the case. Could we add to tracks the ability to see how long a subscription has been expired when viewing a card with an expired plan? That way we could check in tracks later to see if users are leaving that notification there for long periods of time or not later on.

In fact Atomic sites have the expiration notice displaying all over the place

Personally I don't think that's very good UX either 😅. I am not every user but personally that would bug the heck out of me

CodeyGuyDylan avatar Nov 27 '24 17:11 CodeyGuyDylan

@CodeyGuyDylan ,

I don't know that, as a user, I would find it clear that the way to make the notification go away is to remove the subscription altogether, though I guess I would probably try it eventually. (I hate notifications 😆)

Yeah, so true, I think we all hate notifications! 😆

Looking into it and thinking about this further, I'm noticing there's a way to show a close(X) icon on the notice! I'll look now to see how much it would take to close the expired/expiring notices with some persistence! This would be ideal, I think. Do you agree? 😃

Could we add to tracks the ability to see how long a subscription has been expired when viewing a card with an expired plan? That way we could check in tracks later to see if users are leaving that notification there for long periods of time or not later on.

This is a great idea! Yes, I'll do this! 🙌

In fact Atomic sites have the expiration notice displaying all over the place

Personally I don't think that's very good UX either 😅. I am not every user but personally that would bug the heck out of me

Yeah, I agree.

elliottprogrammer avatar Nov 27 '24 18:11 elliottprogrammer

More of a question instead of a requested change. In the scenario below, Boost, Social, and Protect all have expired plans. I see that some statuses override the expired plan status such as these. I think that is fine, as it wouldn't work anyway without the plugin being installed or active, so the statuses shown are still integral to getting the plugin to work. I just wanted to make sure it was expected behavior

@CodeyGuyDylan, Yes, that is expected behavior. There are some statuses that have a higher priority and they override the expiring plans notices. Yeah all the statuses have a particular, sensible order in the Product::get_status() function. 👍

elliottprogrammer avatar Nov 27 '24 19:11 elliottprogrammer