Combine personalization display notifications together #1025

jonsnyder · 2023-08-23T16:17:34Z

Description

Changes to functionality:

Create a new proposition format for empty viewChange notifications.
When sending an event for a view change with no decisions, the display notification is included in the original event.
When rendering specific scopes or surfaces, and page wide decisions, these notifications are combined.

Additionally, this PR includes extensive refactoring of the personalization component. The key changes are:

I created a proposition object that keeps track of renderers, display notification formatting, and returned proposition formatting. The business logic for this was scattered across many different files.
I overhauled the view decisions cache. It now accepts all the returned propositions and updates its cache, then returns the propositions for the current view along with any propositions that do not go in the decisions cache (not view scope type.) Additionally, when a view cache is empty for a view name, it returns a synthesized proposition to communicate an empty view change.
I split out rendering into a set of handlers that can be easily extended for future automatic rendering possibilities. This could be extended to easily allow non-personalization Web SDK components to handle automatic rendering.
I added component level tests for personalization so that I could compare the current functionality to the refactor.

As much as possible, I followed the current functionality; however, there may be opportunities to simplify the logic if our test cases don't represent real use cases.

Related Issue

https://jira.corp.adobe.com/browse/PDCL-9860

Motivation and Context

This is a prerequisite for building top and bottom of page features. This depends on changes to Target upstream to accept unified display notifications.

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.

…notifications

src/components/Personalization/handlers/createHtmlContentHandler.js

jonsnyder · 2023-08-23T16:24:22Z

src/components/Personalization/handlers/proposition.js

+            renderAttempted: false
+          });
+        }
+      }


Splitting out a proposition into one with the items that were rendered and one with the items that were not rendered may not be necessary anymore. It depends if experience edge will ever return a proposition where this is needed.

I don't understand the use case described here, could you please elaborate?

In the return value to sendEvent we return "propositions" and "decisions". decisions includes all propositions that were not rendered. propositions includes all the propositions with a "renderAttempted" flag added. If we ever render only some of the items in a proposition, how should we return the proposition in "propositions" and "decisions"? Currently and with this refactor we split the proposition into two (one with the items that were rendered and one with the other items), and then return them in "propositions" and "decisions" appropriately. This logic may not be necessary if we never run into this situation.

src/components/Personalization/index.js

jonsnyder · 2023-08-23T16:28:33Z

test/functional/specs/Personalization/C205528.js

@@ -49,6 +49,6 @@ test("Test C205528: A redirect offer should redirect the page to the URL in the
  } catch (e) {
    // an exception will be thrown because a redirect will be executed within the Alloy Client Function
  } finally {
-    await t.expect(redirectLogger.requests.length).eql(1);
+    await t.expect(redirectLogger.count(() => true)).eql(1);


I had intermittent test failures with this test case related to timing issues. Using .count(...) triggers test cafe to retry this assertion until it succeeds or times out.

jonsnyder · 2023-08-23T16:30:07Z

test/functional/specs/Personalization/C6364798.js

@@ -230,30 +229,41 @@ const simulateViewChangeForNonExistingView = async alloy => {
    }
  });

-  const noViewViewChangeRequest = networkLogger.edgeEndpointLogs.requests[4];
+  const noViewViewChangeRequest = networkLogger.edgeEndpointLogs.requests[3];


With combined display notifications, this test now runs with 2 fewer network requests.

jonsnyder · 2023-08-23T16:31:23Z

test/functional/specs/Privacy/IAB/C224677.js

+    .expect(
+      networkLogger.edgeEndpointLogs.count(
+        ({ response: { statusCode } }) =>
+          statusCode === 200 || statusCode === 207


I was also getting intermittent failures here. Sometimes Konductor is returning a 200 and sometimes a 207. Maybe this happens when an upstream call fails?

yes, if one of the Upstreams returns an error, Konductor returns Multi Status code and the errors along with the handles.

jonsnyder · 2023-08-23T16:32:47Z

rollup.test.config.js

@@ -48,6 +48,7 @@ if (argv.reporters && argv.reporters.split(",").includes("coverage")) {

 module.exports = {
  output: {
+    sourcemap: true,


This makes debugging tests a lot easier. I'm not sure why we didn't have it included before.

jasonwaters · 2023-08-28T15:26:46Z

src/components/Personalization/index.js

+  const preprocess = createPreprocess([remapHeadOffers, remapCustomCodeOffers]);
+
+  const noOpHandler = () => undefined;
+  const domActionHandler = createDomActionHandler({


I like the idea of action handlers like you added for DOM actions and HTML actions, etc. But it feels like a leaky abstraction since each one has the same modules object passed in (the dom actions modules).

I'd expect a different modules object for each. For instance...

initDomActionsModules might include actions like customCode, setText, setAttribute, setImageSource, setStyle, move, resize, rearrange, remove, insertAfter, insertBefore

And something like initHTMLActionsModules might include setHtml, replaceHtml, prependHtml, appendHtml (guessing on some of these).

And for in app messages something like initMessagingActionsModules to handle this.

The idea here is to separate each set of actions to a specific schema.

This is a good suggestion. I filed a ticket https://jira.corp.adobe.com/browse/PDCL-11018. Let's discuss this next month.

src/components/Personalization/index.js

jasonwaters · 2023-08-28T15:34:12Z

src/components/Personalization/handlers/createHtmlContentHandler.js

+    items = []
+  } = proposition.getHandle();
+
+  items.forEach((item, index) => {


There seems to be a lot of duplication in HTMLContentHandler , DomActionHandler, and others. Is that by design?

Talking about the order of rendering: Redirect and DOM Action schema items should always be first.

ninaceban

Excellent job @jonsnyder! I really like the new structure of the personalization component.

However there are a few things I have discovered:

there is an issue in how we handle HTML offers in target global mbox ( I tested against main and it works fine, however it fails in this PR)
I see flickering, I guess showContainers() gets called prematurely in some places
I was looking at the handlerChain and it seems that every handler iterates thru proposition.items, is this by design? In the previous version we were grouping the propositions.

ninaceban · 2023-09-05T14:07:20Z

src/components/Personalization/createViewCacheManager.js

-        if (viewStorage === undefined) {
-          viewStorage = {};
+  const getViewPropositions = (currentViewStorage, viewName) => {
+    const viewPropositions = currentViewStorage[viewName];


It seems that viewCacheManager is not responsible only for SPA cache anymore, so it should be renamed or split out.

ninaceban · 2023-09-05T14:48:58Z

src/components/Personalization/index.js

+  const render = createRender({
+    handleChain: htmlContentHandler,
+    collect,
+    executeRedirect: url => window.location.replace(url),


Maybe extract this in a function and pass it here.
Also, I noticed there are a few places where window.location is used and a helper createGetPageLocation returns the window.location. I would extract it in utils and reuse.

ninaceban · 2023-09-07T21:10:44Z

src/components/Personalization/handlers/createDomActionHandler.js

+      proposition.includeInDisplayNotification();
+      proposition.addRenderer(index, () => undefined);
+    }
+    const { type, selector } = data || {};


For a use-case of a form-based composed HTML offer for global mbox we receive a proposition item that looks like this:

{ "type": "setHtml", "format": "application/vnd.adobe.target.dom-action", "content": "\"Hello world\"", "selector": "head", "prehidingSelector": "head" }

When this is rendered, we get a blank HTML head tag which results in a broken page since all the styles and scripts are removed.
As far as I can tell previously we had a logic that transformed for head selector the setHtml action type into appendHtml, to avoid messing up the HTML head.

ninaceban · 2023-09-07T21:12:57Z

src/components/Personalization/handlers/proposition.js

+        const errorMessage = `Failed to execute action ${details}. ${message} ${
+          stack ? `\n ${stack}` : ""
+        }`;
+        logger.error(errorMessage);


We didn't log an error before, it was a warning.

ninaceban · 2023-09-07T21:14:31Z

src/components/Personalization/handlers/proposition.js

+            renderAttempted: false
+          });
+        }
+      }


I don't understand the use case described here, could you please elaborate?

jfkhoury · 2023-09-11T15:07:47Z

Awesome work @jonsnyder !!!

jonsnyder · 2023-09-14T14:52:12Z

Closed in favor of #1036

jonsnyder added 10 commits February 6, 2023 12:52

WIP add handlers for each kind of activity

22d1d81

Finalize personalization handlers

efe6c16

Merge remote-tracking branch 'origin/main' into personalizationRefactor

6c33b2d

Add component tests with support for fetch, viewChange, and apply

3aeff9d

Refactor handlePropositions to render

63cdfb0

Remove unused files

4c337ec

fix some tests

f648376

Update all unit tests

1cc8e88

Fix functional tests, and fix issues found when running functional tests

520a0c8

Remove fetch related features, only including refactor and combining …

8a97a0b

…notifications

jonsnyder commented Aug 23, 2023

View reviewed changes

jonsnyder requested review from jasonwaters, ninaceban, shandilya3 and carterworks August 23, 2023 16:35

jasonwaters reviewed Aug 28, 2023

View reviewed changes

carterworks approved these changes Aug 28, 2023

View reviewed changes

jonsnyder added 2 commits August 29, 2023 08:12

Add unit tests for new handler files

76038af

Add showContainers call after rendering in fetch data handler

daea723

ninaceban requested changes Sep 7, 2023

View reviewed changes

jonsnyder mentioned this pull request Sep 14, 2023

Support for top and bottom of page events. Combine personalization display notifications together. #1036

Merged

7 tasks

jonsnyder closed this Sep 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Combine personalization display notifications together #1025

Combine personalization display notifications together #1025

jonsnyder commented Aug 23, 2023

jonsnyder Aug 23, 2023

ninaceban Sep 7, 2023

jonsnyder Sep 14, 2023

jonsnyder Aug 23, 2023

jonsnyder Aug 23, 2023

jonsnyder Aug 23, 2023

ninaceban Sep 4, 2023 •

edited

Loading

jonsnyder Aug 23, 2023

jasonwaters Aug 28, 2023

jonsnyder Sep 14, 2023

jasonwaters Aug 28, 2023

ninaceban Sep 4, 2023 •

edited

Loading

ninaceban left a comment

ninaceban Sep 5, 2023

ninaceban Sep 5, 2023

ninaceban Sep 7, 2023

ninaceban Sep 7, 2023

ninaceban Sep 7, 2023

jfkhoury commented Sep 11, 2023

jonsnyder commented Sep 14, 2023

Combine personalization display notifications together #1025

Combine personalization display notifications together #1025

Conversation

jonsnyder commented Aug 23, 2023

Description

Related Issue

Motivation and Context

Screenshots (if appropriate):

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninaceban Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ninaceban Sep 4, 2023 • edited Loading

Choose a reason for hiding this comment

ninaceban left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jfkhoury commented Sep 11, 2023

jonsnyder commented Sep 14, 2023

ninaceban Sep 4, 2023 •

edited

Loading

ninaceban Sep 4, 2023 •

edited

Loading