Enable ECE for shipping subscriptions with free trial and sign up fee #9955
+32
−14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
asumaran
force-pushed
the
as-fix-ece-shipping-subs-with-free-trial-and-sign-up-fee
branch
from
bb1ee4b to
b3c6d87
Compare
|
Size Change: -6 B (0%) Total Size: 1.39 MB
ℹ️ View Unchanged
|
asumaran
requested review from
a team and
cesarcosta99
and removed request for
a team
|
@cesarcosta99 don't worry too much about the amounts in the payment sheet for now, as they'll be addressed later. It would be great if you could confirm that the amounts charged are correct. |
cesarcosta99
requested changes
Dec 17, 2024
There was a problem hiding this comment.
Changes look good, however, I spotted two potential issues that require some attention, being one a failing test instruction:
- Verify ECE is enabled for case 4
⚠️ - Checkout succeeded with correct amount charged ✅
- I found one issue where I clicked the ECE button for the first time, closed the payment sheet, then clicked on it again. I got a notice (see image) and the payment sheet didn't load again. Refreshing fixes the payment sheet state. I haven't checked whether this has been introduced in this PR (it doesn't seem like), but we gotta check and track this issue.
- Verify ECE is enabled for case 12
⚠️ - Checkout succeeded with correct amount charged ✅
- Same issue observed in case 4.
- Verify ECE is not enabled for Case 3 ✅
- Verify ECE is not enabled for Case 11 ❌
- I created a variable subscription with a single variation, non-virtual, with a trial period and no fee, and in the product page the ECE button remains visible after selecting that variation.
Comment on lines
+441
to
+442
| // Applies to Case 11 from matrix: | ||
| // https://github.com/Automattic/woocommerce-payments/issues/9771#issuecomment-2518829514 |
There was a problem hiding this comment.
This works well, but I wonder if adding more details to this comment here could help others understand the logic more quickly. I think linking to the PR makes sense, but just to provide additional context, not using as the main source of documentation.
Comment on lines
+767
to
+768
| // Applies to case 3 from matrix: https://github.com/Automattic/woocommerce-payments/issues/9771#issuecomment-2518829514. | ||
| // Note: This does not check variable subscriptions, as that will be handled on the frontend. |
@cesarcosta99 I’ve also started experiencing the same behavior while testing #9888. I’m going to run a |
@cesarcosta99 I’ve confirmed it’s working on my end—the button is hidden after selecting a variation. Could you double-check the subscription details? A video or screenshot would be super helpful.
I didn’t get the notice this time. I’ve updated my branch with develop just to be sure, but I’m seeing the same results. It doesn’t seem to be related to our codebase. |
|
I've tested with a variable subscription with all variations (Cases 3, 4, 11, and 12) and I get expected results. Please @cesarcosta99 double check the Case 11 please. |
@asumaran, case 11 passed after merging |
@cesarcosta99, thank you for identifying this issue. Pinpointing the problem has been a bit tedious. This error occurs when the response from I’m not entirely sure how to resolve this yet, as it’s affecting both regular and variable subscriptions. I’ll look into this further and see what we can do. FYI @bborman22 |
|
@cesarcosta99 I've uploaded a fix for the Google Pay issue, but I’d like to run tests across all subscription scenarios before you review it again. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of #9771
Changes proposed in this Pull Request
Enables ECE for subscriptions that require shipping with a free trial and sign-up fee.
Testing instructions
Note
In order to test this PR with the changes in
woocommerce-subscriptions-core(ref) you'd need to clone this repo and symlink the repo to your localsubscriptions-coredirectory inside the WooCommerce Subscriptions plugin:run
npm installandcomposer installif necessary.woocommerce-subscriptions-coreVerify ECE is enabled for case 4
Verify ECE is enabled for case 12
Verify ECE is not enabled for Case 3
Verify ECE is not enabled for Case 11
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge