Skip to content
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

Add CTA Scripts feature #17991

Merged
merged 35 commits into from
Oct 27, 2023
Merged

Add CTA Scripts feature #17991

merged 35 commits into from
Oct 27, 2023

Conversation

FollowTheFlo
Copy link
Contributor

@FollowTheFlo FollowTheFlo commented Oct 19, 2023

CXSPA-4532
tests coverage is 100% on all files
display CTA scripts on 3 pages: pdp, confirmationPage, orderDetails

  • added 1 new endpoint getCtaScripts
  • CtaScripts cpnt, CtaScripts service, CtaElement, CtaElement styling
  • modify opf resource loader service by adding reject callback

image

@FollowTheFlo FollowTheFlo requested a review from a team as a code owner October 19, 2023 13:48
@github-actions github-actions bot marked this pull request as draft October 19, 2023 13:48
opfCtaScriptsService.getCtaHtmlslList = createSpy().and.returnValue(
throwError('error')
);
createComponentInstance();
Copy link
Contributor Author

@FollowTheFlo FollowTheFlo Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to have opfCtaScriptsService.getCtaHtmlslList new value generated (line 55), I had to force creation of new component. I couldn't find other solutions.
It works if we mock getCtaHtmlslList with a rxJs Subject but it then causes issue with removing subscriptions, needs to pipe with take(1) and side effect if both 'it' would run in parallel...

opfCtaScriptsService.getCtaHtmlslList = createSpy().and.returnValue(
of(undefined)
);
createComponentInstance();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same situation as line 58 commented above

return loadedList;
}, []),
map((list) => {
return this.removeScriptTags(list);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove all script tags so they are not displayed in DOM (we don't sanitize html snippet). the script tags are already executed in prior step 'loadAndRunScript' so not needed anymore.

}
})
.catch(() => {
resolve(undefined);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All resolve(undefined) are use for the logic : if a resource file (either css or js) fails to load, don't display the html snippet in UI.

@@ -0,0 +1,4 @@
%cx-opf-cta-element {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding basic styling so CTA elements are separated vertically with 1rem (0.5+0.5).

protected removeScriptTags(htmls: string[]) {
return htmls.map((html) => {
const element = new DOMParser().parseFromString(html, 'text/html');
Array.from(element.getElementsByTagName('script')).forEach((script) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I spent a bit of time to look for removeChild solution, nothing was working, so I used the string manipulation solution with 'replace', it is even shorter.

@FollowTheFlo FollowTheFlo marked this pull request as ready for review October 20, 2023 17:41
@cypress
Copy link

cypress bot commented Oct 20, 2023

4 flaky tests on run #41756 ↗︎

0 119 2 0 Flakiness 4

Details:

Merge 1f0e5b0 into 955ad0f...
Project: spartacus Commit: a24c4412bc ℹ️
Status: Passed Duration: 06:56 💡
Started: Oct 26, 2023 8:19 PM Ended: Oct 26, 2023 8:26 PM
Flakiness  regression/variants/apparel-checkout-as-guest.core-e2e.cy.ts • 1 flaky test • B2C

View Output Video

Test Artifacts
Apparel - checkout as guest > Desktop > should perform checkout as guest, create an account and verify guest data Test Replay Output Screenshots Video
Flakiness  ssr/pages.core-e2e.cy.ts • 3 flaky tests • SSR

View Output Video

Test Artifacts
SSR > should render homepage Test Replay Output Screenshots Video
SSR > should render PLP Test Replay Output Screenshots Video
SSR > should render PDP Test Replay Output Screenshots Video

Review all test suite changes for PR #17991 ↗︎

switchMap((ctaScriptsRequest) => this.fetchCtaScripts(ctaScriptsRequest)),
switchMap((scriptslist) => this.runCtaScripts(scriptslist)),
finalize(() => {
this.clearResources();
Copy link
Contributor Author

@FollowTheFlo FollowTheFlo Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

avoid to clearResources in component ngOnDestroy fct.
cleaner solution.

@github-actions github-actions bot marked this pull request as draft October 20, 2023 20:40
@FollowTheFlo FollowTheFlo marked this pull request as ready for review October 20, 2023 21:23
@github-actions github-actions bot marked this pull request as draft October 20, 2023 21:46
@FollowTheFlo FollowTheFlo marked this pull request as ready for review October 23, 2023 19:23
@github-actions github-actions bot marked this pull request as draft October 25, 2023 15:43
@FollowTheFlo FollowTheFlo requested a review from rmch91 October 26, 2023 20:03
@FollowTheFlo FollowTheFlo marked this pull request as ready for review October 26, 2023 20:03
@FollowTheFlo FollowTheFlo merged commit 44c1008 into epic/opf Oct 27, 2023
21 checks passed
@FollowTheFlo FollowTheFlo deleted the feature/CXSPA-4532 branch October 27, 2023 15:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants