-
Notifications
You must be signed in to change notification settings - Fork 32
[FSSDK-11448] Java Implementation: Add Experiment ID and Variation ID to Decision Notification #566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FSSDK-11448] Java Implementation: Add Experiment ID and Variation ID to Decision Notification #566
Conversation
… to Decision Notification
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -97,9 +97,13 @@ public static ExperimentDecisionNotificationBuilder newExperimentDecisionNotific | |||
public static class ExperimentDecisionNotificationBuilder { | |||
public final static String EXPERIMENT_KEY = "experimentKey"; | |||
public final static String VARIATION_KEY = "variationKey"; | |||
public final static String EXPERIMENT_ID = "experimentId"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this class is responsible for activate api and other stuff that are failing in FSC. The createOptimizelyDecision method uses FlagDecisionNotificationBuilder. This class needs to stay unchanged.
decisionInfo.put(VARIATION_KEY, variation != null ? variation.getKey() : null); | ||
decisionInfo.put(VARIATION_ID, variationId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed.
@@ -141,7 +155,9 @@ public DecisionNotification build() { | |||
|
|||
decisionInfo = new HashMap<>(); | |||
decisionInfo.put(EXPERIMENT_KEY, experimentKey); | |||
decisionInfo.put(EXPERIMENT_ID, experimentId); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed.
this.variationId = variationId; | ||
return this; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably why FSC is getting null values as experiment_id and variation_id. Not needed.
|
||
private String type; | ||
private String experimentKey; | ||
private String experimentId; | ||
private String variationId; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
these also are not needed
As the FSC is passing, go ahead to merge the PR. |
Summary
experimentId
andvariationId
to decision notificationTest plan
Issues