-
Notifications
You must be signed in to change notification settings - Fork 390
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
fix: (CXSPA-9098)-Coupon Codes is used in Coupon Campaign URL #19739
base: develop
Are you sure you want to change the base?
Conversation
spartacus Run #46459
Run Properties:
|
Project |
spartacus
|
Branch Review |
bugfix/CXSPA-9098
|
Run status |
Passed #46459
|
Run duration | 03m 55s |
Commit |
804095d71f ℹ️: Merge cd3b128e51bf3eba3c8fb8c050c075ce5f9f5c3b into 3a234205e59792f259e103c3686d...
|
Committer | crisli001 |
View all properties for this run ↗︎ |
Test results | |
---|---|
Failures |
0
|
Flaky |
3
|
Pending |
2
|
Skipped |
0
|
Passing |
125
|
View all changes introduced in this branch ↗︎ |
Merge Checks Failed
|
projects/core/src/user/connectors/customer-coupon/customer-coupon.connector.ts
Outdated
Show resolved
Hide resolved
...s/storefrontlib/cms-components/myaccount/my-coupons/claim-dialog/claim-dialog.component.html
Outdated
Show resolved
Hide resolved
...s/storefrontlib/cms-components/myaccount/my-coupons/claim-dialog/claim-dialog.component.html
Outdated
Show resolved
Hide resolved
...s/storefrontlib/cms-components/myaccount/my-coupons/claim-dialog/claim-dialog.component.html
Outdated
Show resolved
Hide resolved
...cts/storefrontlib/cms-components/myaccount/my-coupons/claim-dialog/claim-dialog.component.ts
Outdated
Show resolved
Hide resolved
...cts/storefrontlib/cms-components/myaccount/my-coupons/claim-dialog/claim-dialog.component.ts
Outdated
Show resolved
Hide resolved
...cts/storefrontlib/cms-components/myaccount/my-coupons/claim-dialog/claim-dialog.component.ts
Outdated
Show resolved
Hide resolved
projects/storefrontlib/cms-components/myaccount/my-coupons/my-coupons.component.ts
Outdated
Show resolved
Hide resolved
Merge Checks Failed
|
@@ -429,6 +429,7 @@ if (environment.cpq) { | |||
a11yWrapReviewOrderInSection: true, | |||
enableCarouselCategoryProducts: true, | |||
enableSecurePasswordValidation: true, | |||
enableClaimCustomerCouponWithCodeInRequestBody: true, |
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 is true that we want to enable all toggles in our demo app, but in this exact case, if I understand correctly, the API will be available starting from some future version.
If yes - we toggle set to true, our e2e tests will be broken because this functionality will not work before the servers update.
In this case i think it should be false.
I also thinking, maybe it is good to add this information to the toggle JSdoc? Some info telling that if you enabling this toggle you should have cx in version FP8
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, sorry for forget to tell you that we already integrated the new endpoints into Commerce since 2211.28, see following comment. And more details see https://sap-cx.slack.com/archives/C084MGZU2MP/p1733746463727399.
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, in this case it could be true, agree.
I run all e2e tests against your branch, and there are couple of e2e tests in different libs that start failing after this change.
Here you can check the result https://cloud.cypress.io/projects/k3nmep/runs/46437/overview?runStatus=FAILED&runStatus=CANCELLED&runStatus=ERRORED&runStatus=PASSED&runStatus=RUNNING&runStatus=NOTESTS&runStatus=TIMEDOUT&roarHideRunsWithDiffGroupsAndTags=1
The first one "Order history" is flaky and probably not related to your changes, it failed already earlier, but the other ones seems to be related to coupon.
Can you check it please on your side?
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, sure, will check and update
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.
Hi @rmch91 , I have checked the failed cypress test cases except "Order history" which think not related my changes, yeah, the other failed cases is related with my changes, so I have updated the case with a little changes, now after update local running shows it is ok:
I have no access to trigger the whole e2e cypress tests, to see whether the fix(update) for failed test cases work or not, could you help re-run the whole e2e cypress tests based on the fix branch after the cases updated?
Thanks a lot.
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.
Many thanks, I run it and waiting for the result
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.
...cts/storefrontapp-e2e-cypress/cypress/e2e/regression/coupons/my-coupons.e2e-spec-flaky.cy.ts
Show resolved
Hide resolved
...s/storefrontlib/cms-components/myaccount/my-coupons/claim-dialog/claim-dialog.component.html
Outdated
Show resolved
Hide resolved
...s/storefrontlib/cms-components/myaccount/my-coupons/claim-dialog/claim-dialog.component.html
Outdated
Show resolved
Hide resolved
Hi @crisli001. You add a comment for your toggle in feature-toggles.ts, there is nothing further you need to do for documentation. Thanks for asking 🙏 |
Merge Checks Failed
|
Hi @rmch91, I have updated again, could you help review again? Thanks a lot. |
Merge Checks Failed
|
Hi @rmch91 , I have updated cypress test cases, could you help re-run the whole cypress tests and review the PR again? |
… for SPA UI