Skip to content

UNOMI-446 improve increment action and make it generic#265

Merged
Taybou merged 4 commits intomasterfrom
UNOMI-446-increment-action
Mar 29, 2021
Merged

UNOMI-446 improve increment action and make it generic#265
Taybou merged 4 commits intomasterfrom
UNOMI-446-increment-action

Conversation

@Taybou
Copy link
Copy Markdown
Contributor

@Taybou Taybou commented Mar 18, 2021


Please following this checklist to help us incorporate your contribution quickly and easily:

  • Make sure there is a JIRA issue filed
    for the change (usually before you start working on it). Trivial changes like typos do not
    require a JIRA issue. Your pull request should address just this issue, without pulling in other changes.
  • Format the pull request title like [UNOMI-XXX] - Title of the pull request
  • Provide integration tests for your changes, especially if you are changing the behavior of existing code or adding
    significant new parts of code.
  • Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
    Copy the description to the related JIRA issue
  • Run mvn clean install -P integration-tests to make sure basic checks pass. A more thorough check will be
    performed on your pull request automatically.

Trivial changes like typos do not require a JIRA issue (javadoc, project build changes, small doc changes, comments...).

If this is your first contribution, you have to read the Contribution Guidelines

If your pull request is about ~20 lines of code you don't need to sign an Individual Contributor License Agreement
if you are unsure please ask on the developers list.

To make clear that you license your contribution under the Apache License Version 2.0, January 2004
you have to acknowledge this by using the following check-box.

@Taybou Taybou force-pushed the UNOMI-446-increment-action branch 3 times, most recently from 6384ba5 to f5ac910 Compare March 18, 2021 14:53
@Taybou Taybou changed the title UNOMI-446 improve increment action and make it generic WIP | UNOMI-446 improve increment action and make it generic Mar 19, 2021
@Taybou Taybou force-pushed the UNOMI-446-increment-action branch from f5ac910 to 3188425 Compare March 22, 2021 12:04
@Taybou Taybou changed the title WIP | UNOMI-446 improve increment action and make it generic UNOMI-446 improve increment action and make it generic Mar 22, 2021
@Taybou Taybou force-pushed the UNOMI-446-increment-action branch from 3188425 to 48a0294 Compare March 22, 2021 12:06
Copy link
Copy Markdown
Contributor

@sergehuber sergehuber left a comment

Choose a reason for hiding this comment

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

Hello Taybou, I found a few issues/improvements that I put directly in the comments. Thanks.

@jkevan
Copy link
Copy Markdown
Contributor

jkevan commented Mar 22, 2021

Small update on what we discussed about in the call:

  • Do not touch the unomi IncrementInterestAction. This action have his own logic and cannot be generify with property incrementation, also the interest incrementation from JExperience is not using this action, it's actually using a script and the setPropertyAction.
  • Create a new Action: IncrementPropertyAction:
    • propertyName: to be able to increment or create a single prop (properties.pageViewCount)
    • rootPropertyName: to be able to increment all props under this one: (ex: profile.properties.interest)
    • incrementWithValues: to be able to increment properties with the values coming from the event. (ex: event.properties.interest)
    • storeInSession: to be able to target the profile or the session.

Real exemple coming from JEXperience:

the increment page count:

"actions": [
    {
      "parameterValues": {
        "setPropertyName": "properties.pageViewCount",
        "setPropertyValue": "script::r = profile.properties['pageViewCount']; if (r == null) { profile.properties['pageViewCount'] = []; profile.properties.pageViewCount = [event.scope : 1] } else { if (r[event.scope] != null) { r[event.scope] = r[event.scope] + 1 } else { r[event.scope] = 1 }} r",
        "storeInSession": false
      },
      "type": "setPropertyAction"
    }
  ]

We should be able to do it with this kind of conf:
type: incrementPropertyAction
paramerterValues:

  • propertyName: properties.pageViewCount
  • storeInSession: false

The increment interest from jexperience:

"actions": [
    {
      "parameterValues": {
        "setPropertyName": "properties.interests",
        "setPropertyValue": "script::r = profile.properties['interests']; foreach(interest : event.target.properties['interests'].entrySet()) { if (r == null) { r = [interest.key: interest.value] } else if (r[interest.key] != null) { r[interest.key] = r[interest.key] + interest.value } else { r[interest.key] = interest.value } } r",
        "storeInSession": false
      },
      "type": "setPropertyAction"
    }
  ]

We should be able to replace it, with:
type:incrementPropertyAction
paramerterValues:

  • rootPropertyName: properties.interest
  • incrementWithEventValues: target.properties.interest
  • storeInSession: false

This way we will be able to replace the script usage we have in the rules in JExperience to use a generic increment action.
The Existing IncrementInterestAction is missleading, and confusing because there is also interest incrementation in jexperience. But the both are completely differents.

@Taybou Taybou force-pushed the UNOMI-446-increment-action branch 3 times, most recently from 2bfde31 to 3d5fdf3 Compare March 24, 2021 16:47
@Taybou Taybou requested review from jkevan and sergehuber March 26, 2021 11:50
@Taybou Taybou force-pushed the UNOMI-446-increment-action branch from a71469c to 31316c3 Compare March 29, 2021 13:56
@Taybou Taybou merged commit 486aff8 into master Mar 29, 2021
@Taybou Taybou deleted the UNOMI-446-increment-action branch March 29, 2021 14:00
Taybou added a commit that referenced this pull request Mar 29, 2021
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