Skip to content

Conversation

mnoman09
Copy link
Contributor

@mnoman09 mnoman09 commented Sep 7, 2018

No description provided.

@coveralls
Copy link

coveralls commented Sep 7, 2018

Pull Request Test Coverage Report for Build 610

  • 10 of 10 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 88.972%

Totals Coverage Status
Change from base Build 606: 0.0%
Covered Lines: 2380
Relevant Lines: 2675

💛 - Coveralls

@msohailhussain msohailhussain changed the title filters out attributes with null values fix(nullfilter): filters out attributes with null values Sep 7, 2018
Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

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

Looks good mostly. I have a small nit

Map<String, String> actualValue = attributeCaptor.getValue();
assertThat(actualValue, hasEntry(attribute.getKey(), null));
assertNotSame(actualValue, hasEntry(attribute.getKey(), null));
logbackVerifier.expectMessage(Level.WARN, "Attribute(s) ["+attribute.getKey()+"] not in the datafile or has NULL value.");
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: spaces before and after the +

@mnoman09 mnoman09 self-assigned this Sep 7, 2018
Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

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

I commented on the actual ticket, but we are changing this to filter out null attribute values from being sent to the log tier so it should be done here: https://github.com/optimizely/java-sdk/blob/master/core-api/src/main/java/com/optimizely/ab/event/internal/EventFactory.java#L170

@mnoman09
Copy link
Contributor Author

@mikeng13 I made PR #214 so I think we can close this PR.

@thomaszurkan-optimizely
Copy link
Contributor

@mikeng13 . I'm closing this PR since they are now addressing the issue in a new PR.

@mikeproeng37 mikeproeng37 deleted the mnoman/removeNullValueAttributes branch September 21, 2018 22:35
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.

4 participants