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

Fixed an issue where rendered propositions were not included in display notifications. #1225

Merged
merged 5 commits into from
Jan 23, 2025

Conversation

carterworks
Copy link
Contributor

@carterworks carterworks commented Jan 8, 2025

Description

v2.20.0 with the changes related to automatic click tracking, introduced this line

  const renderItems = (renderers, meta) =>
    Promise.all(renderers.map((renderer) => renderer())).then((results) => {
      const successes = results.filter((result) => result);
      // as long as at least one renderer succeeds, we want to add the notification

Proposition renders would only be included in display notifications if they return something. But most/all proposition renderers return a Promise that resolves to undefined.

We just that filter and display notifications will flow again. I'm not sure why it was included, but we know that the Promise resolved successfully because we see if the status is fulfilled when using Promise.allSettled.

Related Issue

PDCL-12864: IncludeRenderedPropositions not working for propositions rendered with applyPropositions

Motivation and Context

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change which does not add functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • I have signed the Adobe Open Source CLA or I'm an Adobe employee.
  • I have made any necessary test changes and all tests pass.
  • I have run the Sandbox successfully.

@carterworks carterworks self-assigned this Jan 8, 2025
@carterworks
Copy link
Contributor Author

carterworks commented Jan 8, 2025

Functional test failure is unrelated. Failed in Safari only.

C8085776: At.js 2.x to Web SDK - Assert same session ID, edge cluster are used for both of the requests interact and delivery API

✖ C8085776: At.js 2.x to Web SDK - Assert same session ID, edge cluster are used for both of the requests interact and delivery API - C8085776: At.js 2.x to Web SDK - Assert same session ID, edge cluster are used for both of the requests interact and delivery API
1) AssertionError: expected 0 to deeply equal 1
+ expected - actual
-0
+1

Browser: Safari 17.1 / Ventura 13

56 | "C8085776: At.js 2.x to Web SDK - Assert same session ID, edge cluster are used " +
57 | "for both of the requests interact and delivery API",
58 | async () => {
59 | await t
60 | .expect(networkLogger.targetDeliveryEndpointLogs.count(() => true))
61 | > .eql(1);
62 | // Get delivery API request
63 | const deliveryRequest =
64 | networkLogger.targetDeliveryEndpointLogs.requests[0];
65 | await responseStatus(networkLogger.targetDeliveryEndpointLogs, [200, 207]);
66 | const { searchParams } = new URL(deliveryRequest.request.url);

at <anonymous> (/Volumes/Sauce/saucectl-runners/sauce-testcafe-runner/3.6.0/bundle/__project__/test/functional/specs/Migration/C8085776.js:61:8)
at asyncGeneratorStep (/Volumes/Sauce/saucectl-runners/sauce-testcafe-runner/3.6.0/bundle/__project__/test/functional/specs/Migration/C8085776.js:32:68)
at _next (/Volumes/Sauce/saucectl-runners/sauce-testcafe-runner/3.6.0/bundle/__project__/test/functional/specs/Migration/C8085776.js:32:68)
at <anonymous> (/Volumes/Sauce/saucectl-runners/sauce-testcafe-runner/3.6.0/bundle/__project__/test/functional/specs/Migration/C8085776.js:32:68)
at <anonymous> (/Volumes/Sauce/saucectl-runners/sauce-testcafe-runner/3.6.0/bundle/__project__/test/functional/specs/Migration/C8085776.js:32:68)

@jonsnyder jonsnyder merged commit 56b1a07 into main Jan 23, 2025
3 of 4 checks passed
@jonsnyder jonsnyder deleted the pdcl-12864-applypropositions-rendering branch January 23, 2025 15:31
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.

3 participants