Add IP anonymization to event payload. #42
Conversation
| private final String projectId; | ||
| private final String revision; | ||
| private final String version; | ||
| private final boolean anonymizeIP; |
There was a problem hiding this comment.
just for consistency, can you modify toString to include anonymizeIP as well?
| List<Experiment> experiments, List<Attribute> attributes, List<EventType> eventType, | ||
| List<Audience> audiences) { | ||
| this(accountId, projectId, version, revision, groups, experiments, attributes, eventType, audiences, | ||
| List<Audience> audiences, boolean anonymizeIP) { |
There was a problem hiding this comment.
we only need to add anonymizeIP to one ProjectConfig constructor. You can remove it here and call the other constructor with false as the default value.
There was a problem hiding this comment.
Will do. This will ensure that we always have it initialized to false unless explicitly passed in to the constructor.
| List<Group> groups = asList(randomPolicyGroup, overlappingPolicyGroup); | ||
|
|
||
| return new ProjectConfig("789", "1234", "1", "42", groups, experiments, attributes, events, audiences); | ||
| return new ProjectConfig("789", "1234", "1", "42", groups, experiments, attributes, events, audiences, true); |
There was a problem hiding this comment.
don't need to modify this if you change the constructor
|
|
||
| return new ProjectConfig("789", "1234", "1", "42", Collections.<Group>emptyList(), experiments, attributes, | ||
| events, Collections.<Audience>emptyList()); | ||
| events, Collections.<Audience>emptyList(), true); |
| List<Group> groups = asList(randomPolicyGroup, overlappingPolicyGroup); | ||
|
|
||
| return new ProjectConfig("789", "1234", "2", "42", groups, experiments, attributes, events, audiences); | ||
| return new ProjectConfig("789", "1234", "2", "42", groups, experiments, attributes, events, audiences, true); |
|
|
||
| return new ProjectConfig("789", "1234", "2", "42", Collections.<Group>emptyList(), experiments, attributes, | ||
| events, Collections.<Audience>emptyList()); | ||
| events, Collections.<Audience>emptyList(), true); |
| assertThat(impression.getVisitorId(), is(userId)); | ||
| assertThat((double)impression.getTimestamp(), closeTo((double)System.currentTimeMillis(), 60.0)); | ||
| assertFalse(impression.getIsGlobalHoldback()); | ||
| assertThat(impression.getAnonymizeIP(), is(projectConfig.getAnonymizeIP())); |
There was a problem hiding this comment.
let's make this comparison explicit. assertFalse in this case since it's a v2 datafile. We should also pass in v2 datafile to the eventbuilder as well as a v3 datafile since for v2, anonymizeIP should be false by default, while v3 will vary
There was a problem hiding this comment.
I don't think testing for that is needed here. This tests that events are built correctly and not the fact that IPAnon should be false by default as that should be tested elsewhere. We should just make sure that the event value corresponds to what the project config shows. That said, I'll add a test for this case on the ProjectConfigTest
| ", eventMetrics=" + eventMetrics + | ||
| ", eventFeatures=" + eventFeatures + | ||
| ", isGlobalHoldback=" + isGlobalHoldback + | ||
| ", anonymizeIP=" + anonymizeIP + |
There was a problem hiding this comment.
should we have constants for all these param values?
There was a problem hiding this comment.
Yeah we should at some point since we share these with Impression.java. Just don't think it's worth refactoring for this diff :)
vraja2
left a comment
vraja2
left a comment
There was a problem hiding this comment.
just had some small comments, thanks for addressing the feedback!
| this.userFeatures = userFeatures; | ||
| } | ||
|
|
||
| public boolean getAnonymizeIP() { return anonymizeIP; } |
There was a problem hiding this comment.
nit: can you format this to be on multiple lines?
|
|
||
| public boolean getAnonymizeIP() { return anonymizeIP; } | ||
|
|
||
| public void setAnonymizeIP(boolean anonymizeIP) { this.anonymizeIP = anonymizeIP; } |
There was a problem hiding this comment.
nit: can you format this to be on multiple lines?
| import static org.hamcrest.CoreMatchers.is; | ||
| import static org.junit.Assert.assertNull; | ||
| import static org.junit.Assert.assertThat; | ||
| import static org.junit.Assert.*; |
There was a problem hiding this comment.
let's keep explicit imports instead of wildcard imports
| assertThat(impression.getVisitorId(), is(userId)); | ||
| assertThat((double)impression.getTimestamp(), closeTo((double)System.currentTimeMillis(), 60.0)); | ||
| assertFalse(impression.getIsGlobalHoldback()); | ||
| assertThat(impression.getAnonymizeIP(), is(projectConfig.getAnonymizeIP())); |
Add IP anonymization to event payload.
@optimizely/fullstack-devs