Skip to content

Conversation

yavorona
Copy link
Contributor

Summary

  • update decisionSnapshot to include metadata object to support sending flag decisions

Test plan

Unit tests

Issues

@yavorona yavorona requested a review from a team as a code owner October 16, 2020 04:34
@yavorona yavorona removed their assignment Oct 16, 2020
@coveralls
Copy link

coveralls commented Oct 16, 2020

Coverage Status

Coverage remained the same at 96.629% when pulling f165098 on pnguen/event-processor-flag-decisions into 0576e46 on master.

Copy link
Contributor

@mjc1283 mjc1283 left a comment

Choose a reason for hiding this comment

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

LGTM - one question about layer/experiment/variation id being null inside ImpressionEvent.

Comment on lines +48 to +57
id: string | null
} | null

experiment: {
id: string
id: string | null
key: string
} | null

variation: {
id: string
id: string | null
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are these now allowed to be null?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When decision.experiment is not defined, we assign a ruleKey/experimentKey to an empty string, in which case I assign an experimentId and layerId to null. Similarly, the variationId is now null when variation is not defined. Please see the implementation here for details. Another way would be to assign these params to an empty string instead of null. LMKWYT @mjc1283

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, I think using null is ok.

@yavorona yavorona requested a review from mjc1283 October 16, 2020 18:28
@yavorona yavorona merged commit a24bba8 into master Oct 16, 2020
@yavorona yavorona deleted the pnguen/event-processor-flag-decisions branch October 16, 2020 21:17
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