You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@unomi.apache.org by GitBox <gi...@apache.org> on 2021/07/30 11:12:58 UTC
[GitHub] [unomi] jkevan opened a new pull request #329: UNOMI-338, UNOMI-482: better implementation for lastVisit, firstVisit…
jkevan opened a new pull request #329:
URL: https://github.com/apache/unomi/pull/329
… and previousVisit properties
I would like to propose this new Implem for the handling of the lastVisit, firstVisit and previousVisit properties on the profile.
The precedent implem was not logical and error prone.
previous algorithm:
profile:
- firstVisit: 11/05/2021
- previousVisit: 13/05/2021
- lastVisit: 16/05/2021
In case Event comes with a timeStamp: 10/05/2021, but the event is submitted to the system at 20/05/2021.
Then the result was:
- firstVisit: 11/05/2021
- previousVisit: 16/05/2021
- lastVisit: 20/05/2021
This seem's not logical because the eventTimestamp is only source of truth regarding the chronology of the events and the visits. So we should not use the current date that the event is processed into the system.
So I propose this result instead:
- firstVisit: 10/05/2021
- previousVisit: 13/05/2021
- lastVisit: 16/05/2021
My implem is updating the 3 dates to respect the chronology of the events received (previous implem was only updating lastVisit with no sense values).
So depending on the event timeStamp, one of multiples of this 3 props will be updated accordingly to the eventTimeStamp.
The profile will hold this informations updated.
It have multiple benefits:
- more logic and easy to understand (only one source of truth for the chronology of events)
- better handling of events coming from the past (using pasted dates will be more easy to handle by the system)
- segmentation will work better for segment with condition on this 3 props.
Also this PR contains a simple improvement already asked for: https://issues.apache.org/jira/browse/UNOMI-482
To be able to set the currentDate in the SetPropertyAction.
SetPropertyAction with setPropertyValue: 'now' is now deprecated, I propose to use 'setPropertyValueCurrentEventTimestamp' or 'setPropertyValueCurrentDate' instead of 'setPropertyValue' that is more suitable for this need.
Also this PR contains the integration tests necessary to tests all this changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@unomi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [unomi] sergehuber commented on a change in pull request #329: UNOMI-338, UNOMI-482: better implementation for lastVisit, firstVisit…
Posted by GitBox <gi...@apache.org>.
sergehuber commented on a change in pull request #329:
URL: https://github.com/apache/unomi/pull/329#discussion_r679876215
##########
File path: itests/src/test/java/org/apache/unomi/itests/BasicIT.java
##########
@@ -265,7 +278,7 @@ private ContextRequest getContextRequestWithLoginEvent(CustomItem sourceSite, Ma
contextRequest.setSource(sourceSite);
contextRequest.setRequireSegments(false);
contextRequest.setEvents(Collections.singletonList(loginEvent));
- contextRequest.setRequiredProfileProperties(Arrays.asList(FIRST_NAME, LAST_NAME, EMAIL));
+ contextRequest.setRequiredProfileProperties(Arrays.asList(FIRST_NAME, LAST_NAME, EMAIL, "firstVisit", "lastVisit", "previousVisit"));
Review comment:
Could we use constants for those property names too ?
##########
File path: itests/src/test/java/org/apache/unomi/itests/BasicIT.java
##########
@@ -290,7 +303,7 @@ private ContextRequest getContextRequestWithPageViewEvent(CustomItem sourceSite,
contextRequest.setSource(customPageItem);
contextRequest.setRequireSegments(false);
contextRequest.setEvents(Collections.singletonList(pageViewEvent));
- contextRequest.setRequiredProfileProperties(Arrays.asList(FIRST_NAME, LAST_NAME, EMAIL));
+ contextRequest.setRequiredProfileProperties(Arrays.asList(FIRST_NAME, LAST_NAME, EMAIL, "firstVisit", "lastVisit", "previousVisit"));
Review comment:
Some comments as before, maybe use constants here ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@unomi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [unomi] jkevan merged pull request #329: UNOMI-338, UNOMI-482: better implementation for lastVisit, firstVisit…
Posted by GitBox <gi...@apache.org>.
jkevan merged pull request #329:
URL: https://github.com/apache/unomi/pull/329
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@unomi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org