Skip to content

[FSSDK-11167] Implement CMAB service #367

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

Conversation

FarhanAnjum-opti
Copy link
Contributor

Summary

DefaultCmabService implemented to support cmab experiments.

Test plan

Unit tests added.

Issues

FSSDK-11167

…alue structs (otherwise breaks in ruby version change)
Copy link
Contributor

@Mat001 Mat001 left a comment

Choose a reason for hiding this comment

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

Approved, but think if certain methods in cmab_service need to be private. They are private in python cmab_service

@@ -0,0 +1,153 @@
# frozen_string_literal: true
Copy link
Contributor

Choose a reason for hiding this comment

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

this intended?

CmabDecision.new(variation_id: variation_id, cmab_uuid: cmab_uuid)
end

def filter_attributes(project_config, user_context, rule_id)
Copy link
Contributor

Choose a reason for hiding this comment

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

python-sdk uses this as a private method. Do yuo want to use privates here too?
Same for
def get_cache_key(user_id, rule_id)
def hash_attributes(attributes)

ruby has stricter private method enforcement, can't call from outside. Not sure it's 100% necessary, think about it, see if using private methods would break any access from other parts..

Copy link
Contributor

Choose a reason for hiding this comment

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

If private, you'd need to update test calls to use send() for private methods

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