Skip to content

[FSSDK-11522] feat: update decision service to apply ho #576

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

Open
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

muzahidul-opti
Copy link
Contributor

@muzahidul-opti muzahidul-opti commented Aug 6, 2025

Summary

  • getVariationForHoldout method added to DecisionService class.
  • Add holdout as allowed decision sources.
  • Add test cases for impression event.
  • Add test cases for decision listener.

Test plan

Issues

  • FSSDK-11522
  • FSSDK-11523

… use ExperimentCore instead of Experiment. Fix ActivateNotification, ExperimentUtils, ActivateNotificationListener, NotificationCenterTest to reflect these changes.
- Add logic to process holdout rules for feature flags
- Implement a method to determine variation for a holdout rule
- Create decision responses based on a holdout decision and its reasons
- Update logger messages for holdout rule evaluation
…ctConfigV4

- Change visibility of FEATURE_FLAG_BOOLEAN_FEATURE to public
- Change visibility of Variation VARIATION_HOLDOUT_VARIATION_OFF to public
- Update entityIds in HOLDOUT_BASIC_HOLDOUT and holdouts-project-config.json to use "$opt_dummy_variation_id" instead of hardcoded values
- Create test for checking if a user is in a variation for holdout feature when included in flags
- Implement test to verify exclusion of user from variation for holdout when specified as excluded from flags
- Add assertions to validate decision source, experiment, and variation for holdout feature
- Added public constant HOLDOUT_TYPEDAUDIENCE_HOLDOUT to class ValidProjectConfigV4
- Updated feature flag constants to public static in class ValidProjectConfigV4
- Updated holdout constants to public static in class ValidProjectConfigV4
- Created unit test userMeetsHoldoutAudienceConditions in DecisionServiceTest
…ith_holdout' to accurately reflect the functionality being tested
@muzahidul-opti muzahidul-opti marked this pull request as ready for review August 11, 2025 05:33
@muzahidul-opti muzahidul-opti requested a review from jaeopt August 11, 2025 12:40
- Implement createOptimizelyWithHoldouts method to create Optimizely instance with holdouts
- Add test for decisionNotification with holdout scenario
- Update decide_for_keys_with_holdout test for holdout variations
- Implement decide_all_with_holdout test for multiple flag keys with holdouts
- Create Optimizely instance with specific configuration
- Set up decision notification handler for optimalizely decision notifications
- Test decision notification logic with holdout context
- Validate decision notification data for various test scenarios
Copy link
Contributor

@jaeopt jaeopt 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. We should to keep the legacy API to avoid breaking changes if not necessary.

@@ -50,15 +50,15 @@ public final class ActivateNotification {
* @param variation - The variation that was returned from activate.
* @param event - The impression event that was triggered.
*/
public ActivateNotification(Experiment experiment, String userId, Map<String, ?> attributes, Variation variation, LogEvent event) {
public ActivateNotification(ExperimentCore experiment, String userId, Map<String, ?> attributes, Variation variation, LogEvent event) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we keep Experiment? I'm concerned about breaking changes in this public APIs.
This legacy API is not related to HOs, so no need to support HO type.

@@ -80,7 +82,7 @@ public final void handle(ActivateNotification message) {
* @param variation - The variation that was returned from activate.
* @param event - The impression event that was triggered.
*/
public abstract void onActivate(@Nonnull Experiment experiment,
public abstract void onActivate(@Nonnull ExperimentCore experiment,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here

@@ -40,7 +41,7 @@ public interface ActivateNotificationListenerInterface {
* @param variation - The variation that was returned from activate.
* @param event - The impression event that was triggered.
*/
public void onActivate(@Nonnull Experiment experiment,
public void onActivate(@Nonnull ExperimentCore experiment,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same - this will be breaking changes

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.

2 participants