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

Populate _experience.decisioning.propositionEventType for display and interact personalization events. #901

Merged
merged 2 commits into from
Sep 19, 2022

Conversation

dcottingham
Copy link
Contributor

@dcottingham dcottingham commented Sep 2, 2022

Description

Related Issue

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.

@jfkhoury jfkhoury requested review from XDex, carterworks and ninaceban and removed request for XDex and carterworks September 6, 2022 13:53
@@ -12,3 +12,7 @@ governing permissions and limitations under the License.

export const DISPLAY = "decisioning.propositionDisplay";
export const INTERACT = "decisioning.propositionInteract";
export const EventType = {
DISPLAY: "display",
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use the full event types decisioning.proposition...?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jfkhoury, using the existing eventType field with decisioning.proposition... isn't flexible enough for all customer use cases, since the existing eventType lives on the event and not the proposition. This wiki describes the problem and why both Target and AJO need something like propositionEventType.

Copy link
Contributor

@carterworks carterworks left a comment

Choose a reason for hiding this comment

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

I think I understand the use case. If I wanted to include something in the XDM decisions object for interactions, this is how I would do it.

@dcottingham
Copy link
Contributor Author

Backend changes for this are due next sprint (Sept 14-28). I've tested this change locally in sandbox app and verified it is backward compatible.
Screen Shot 2022-09-12 at 10 12 50 AM

Copy link
Contributor

@ninaceban ninaceban left a comment

Choose a reason for hiding this comment

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

@dcottingham looks good to me! Thank you!

I would add only one general comment, ideally when we augment the existing functionality and it should be backward compatible, we should add a new functional test that would test the new behavior and leave the existing ones test the old behavior.
cc: @jonsnyder @carterworks

@ninaceban
Copy link
Contributor

ninaceban commented Sep 14, 2022

@dcottingham is there a PR to update the documentation with the new way of sending display and click notifications? This is related to the notifications that are sent by the customer.

@dcottingham
Copy link
Contributor Author

@dcottingham looks good to me! Thank you!

I would add only one general comment, ideally when we augment the existing functionality and it should be backward compatible, we should add a new functional test that would test the new behavior and leave the existing ones test the old behavior. cc: @jonsnyder @carterworks

@ninaceban Do you want me to revert all the updates I made to the functional test cases and create some new ones then? I only added new assertions to the existing test cases. I hadn't removed or modified existing assertions. If I create new tests they will be functionally the same as the ones I updated, just different assertions. But happy to do that if that's the preferred approach.

@@ -12,3 +12,7 @@ governing permissions and limitations under the License.

export const DISPLAY = "decisioning.propositionDisplay";
export const INTERACT = "decisioning.propositionInteract";
export const EventType = {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is confusing. Perhaps call this proposition event type? Maybe in a separate file.

@jonsnyder
Copy link
Contributor

jonsnyder commented Sep 15, 2022

I think using the existing functional test is fine in this case. You are not adding functionality, but updating the format of the current functionality. The original tests will still be used to test against old versions of the Web SDK.

@jonsnyder jonsnyder merged commit 102b954 into main Sep 19, 2022
@jonsnyder jonsnyder deleted the PDCL-9192 branch September 19, 2022 16:27
@jonsnyder jonsnyder changed the title PDCL-9192 - add propositionEventType to experience event for better proposition interaction handling Populate _experience.decisioning.propositionEventType for display and interact personalization events. Sep 21, 2022
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.

5 participants