Skip to content

[FSSDK-11373] add holdout support and refactor decision logic in DefaultDecisionService #587

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 13 commits into
base: master
Choose a base branch
from

Conversation

muzahidul-opti
Copy link
Contributor

@muzahidul-opti muzahidul-opti commented Apr 22, 2025

Summary

  • Added support for holdout groups in decision-making logic.
  • Introduced ExperimentCore to replace Experiment in several methods across the codebase.
  • Refactored DefaultDecisionService for improved readability and holdout support.
  • Added the getVariationForHoldout method to evaluate holdout conditions and bucket users into holdouts.
  • Enhanced the OPTBucketer protocol by adding the new bucketToVariation method.
  • Introduced new test files, including BucketTests_HoldoutToVariation.swift, DecisionServiceTests_Features_Holdouts.swift, OptimizelyUserContextTests_Decide_Holdouts.swift, OptimizelyUserContextTests_Decide_With_Holdouts_Reasons.swift

Test plan

Issues

  • FSSDK-11373

@muzahidul-opti muzahidul-opti changed the title [FSSDK-11373] holdout bucketing logic added [FSSDK-11373] update DefaultDecisionService to support holdouts Apr 25, 2025
@muzahidul-opti muzahidul-opti marked this pull request as ready for review April 25, 2025 16:32
@muzahidul-opti muzahidul-opti requested a review from jaeopt April 25, 2025 16:32
@muzahidul-opti muzahidul-opti changed the title [FSSDK-11373] update DefaultDecisionService to support holdouts [FSSDK-11373] add holdout support and refactor decision logic in DefaultDecisionService Apr 25, 2025
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.

Doc enhancement looks good. A few more suggestions for clean up.

- Parameter bucketingId: The ID to bucket. This must be a non-null, non-empty string.
- Returns: The variation the bucketingId was bucketed into.
*/
func bucketToVariation(experiment: ExperimentCore,
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we adding new signature to this public protocol? This may break existing custom implementations on client side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right that adding a new method signature could potentially break clients' custom implementations. To mitigate this, we could consider either making the method optional or removing the new signature altogether.

Which approach do you think would better align with our clients' needs and use cases?

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.

LGTM

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