-
Notifications
You must be signed in to change notification settings - Fork 28
[FSSDK-11578] Ruby: Update impression event handling and send notification for global holdout #376
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-11578] Ruby: Update impression event handling and send notification for global holdout #376
Conversation
…ation for global holdout
…esra/FSSDK-11578_impression_and_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.
This pull request introduces improvements to how holdouts are handled for feature flags in the Optimizely codebase. The changes focus on ensuring that only active holdouts are considered, updating the logic to use flag IDs instead of keys, and improving the robustness of variation selection for holdouts. Additionally, new test data for holdouts has been added to the spec parameters. LGTM
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.
This does appear to improve impression event handling, global holdout notifications, adds test cases and rearranges holdout tests. Don't see any bugs.
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 w minor comment
| end | ||
|
|
||
| def get_holdouts_for_flag(flag_key) | ||
| def get_holdouts_for_flag(flag_id) |
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.
HAven't done event handling yet for holdouts myself, but I wonder here, it could be a potential bug if flag_key is used accidentally instead of flag_id in this PR. Are there any checks needed to make sure we use flag_id?
I'm not 100% sure, just pointing to this in case a check is needed. Does swift sdk has any additional guards maybe etc
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.
We can a comment for better clarification. For Swift flag_id is none optional, so there is no way to pass null value.
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.
Looks good to me
| end | ||
|
|
||
| def get_holdouts_for_flag(flag_key) | ||
| def get_holdouts_for_flag(flag_id) |
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.
We can a comment for better clarification. For Swift flag_id is none optional, so there is no way to pass null value.
Summary
Test plan
PR checks
Issues