Skip to content

UNOMI-482 SetPropertyAction eventArrivalTime#309

Closed
amitco1 wants to merge 1 commit intoapache:masterfrom
YotpoLtd:UNOMI-482-support-eventArrivalTime
Closed

UNOMI-482 SetPropertyAction eventArrivalTime#309
amitco1 wants to merge 1 commit intoapache:masterfrom
YotpoLtd:UNOMI-482-support-eventArrivalTime

Conversation

@amitco1
Copy link
Copy Markdown
Contributor

@amitco1 amitco1 commented May 10, 2021

…Time

PR Title format:

[UNOMI-XXX] Pull request title with JIRA reference

Please add a meaningful description for your change here


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.

@amitco1 amitco1 changed the title feat(UNOMI-482-setPropertyAction-eventArrivalTime):added eventArrival… UNOMI-482 May 10, 2021
@sergehuber sergehuber changed the title UNOMI-482 UNOMI-482 SetPropertyAction eventArrivalTime Jun 7, 2021
@sergehuber
Copy link
Copy Markdown
Contributor

@amitco1 Could you please provide integration tests for this ?

propertyValue = format.format(date);
}

if (propertyValue != null && propertyValue.equals("eventArrivalTime")) {
Copy link
Copy Markdown
Contributor

@jkevan jkevan Jul 1, 2021

Choose a reason for hiding this comment

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

I would rename "eventArrivalTime" to "timeStamp" or "actionExecutionTimeStamp" or "currentDate".
Because it's not really true that this timeStamp is the real event arrival time. Event could arrive sooner and the action being executed few seconds after.

@jkevan
Copy link
Copy Markdown
Contributor

jkevan commented Jul 1, 2021

Overal I approve this because of the explaination I provided on the ticket: https://issues.apache.org/jira/browse/UNOMI-482

Copy link
Copy Markdown
Contributor

@jkevan jkevan left a comment

Choose a reason for hiding this comment

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

just need to rename the "eventArrivalTime" to something more meaningful

@jkevan
Copy link
Copy Markdown
Contributor

jkevan commented Jul 20, 2021

@amitco1 would love your answer on this or we'll have to close the PR.

@jkevan
Copy link
Copy Markdown
Contributor

jkevan commented Jul 30, 2021

I'm working on implementing this in a proper way, it will be part of version 2.0.0-SNAPSHOT and 1.6.0-SNAPSHOT.
Basically I deprecated the "now" option (it will be still supported, but deprecated)
And I added this two action parameter:

  • setPropertyValueCurrentDate
  • setPropertyValueCurrentEventTimestamp

using the value of the parameter: setPropertyValue was not a good idea for people that would like to set the value: "now" or "eventArrivalTime". That is why I prefer to implement dedicated parameters to handle this cases.

@jkevan jkevan closed this Jul 30, 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