Skip to content
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

feat: odp datafile parsing and audience evaluation #303

Merged
merged 9 commits into from
Jul 25, 2022

Conversation

andrewleap-optimizely
Copy link
Contributor

@andrewleap-optimizely andrewleap-optimizely commented Jul 21, 2022

Summary

  • A new field integrations and new audience type odp.audiences will be added to the datafile schema for ODP integration.

  • parse the odp values (publicKey and host)

  • accept datafile with and without the integrations section or empty section as well.

  • support a new audience with the odp.audiences and qualified match.

  • support any (allowed) combination of audiences, including ODP and other existing audiences.

Ticket:

OASIS-8393

Test plan

Added/extended tests in:

  • datafile_project_config_spec.rb
  • audience_spec.rb
  • custom_attribute_condition_evaluator_spec.rb
  • optimizely_user_context_spec.rb
  • project_spec.rb

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.

Looks ok to me.
Just ine comment about comment indentaton.

def qualified_evaluator(condition)
# Evaluate the given match condition for the given user qaulified segments.
# Returns boolean true if condition value is in the user's qualified segments,
# false if the condition value is not in the user's qualified segments,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this wide indent a ruby commenting style? I don't know, just mentioning :)
Seems a bit weird?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just matched the existing style in this file. Looks like the intention was to line up the different return values visually. I have no strong feelings about it either way 🤷

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! A couple of small changes suggested.

lib/optimizely/decision_service.rb Outdated Show resolved Hide resolved
attributes ||= {}

custom_attr_condition_evaluator = CustomAttributeConditionEvaluator.new(attributes, logger)
custom_attr_condition_evaluator = CustomAttributeConditionEvaluator.new(user_context, logger)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure if we should fix them all but this name is not good any more "CustomAttributeConditionEvaluator" since it evaluates more than custom-attributes. We can consider something like "ConditionEvaluator" or "UserConditionEvaluator".

spec/project_spec.rb Show resolved Hide resolved
@@ -22,7 +22,7 @@

module Optimizely
class CustomAttributeConditionEvaluator
CUSTOM_ATTRIBUTE_CONDITION_TYPE = 'custom_attribute'
CONDITION_TYPES = %w[custom_attribute third_party_dimension].freeze
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what I meant by name change. It's not "custom attribute" evaluator any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea I agree, I made the change

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

Copy link
Contributor

@zashraf1985 zashraf1985 left a comment

Choose a reason for hiding this comment

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

LGTM! Great work @andrewleap-optimizely

@andrewleap-optimizely andrewleap-optimizely merged commit 1f3c89b into master Jul 25, 2022
@andrewleap-optimizely andrewleap-optimizely deleted the aleap/odp_datafile_parsing branch July 25, 2022 20:55
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.

4 participants