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: Add ODP Datafile Parsing and Audience Evaluation #765

Merged
merged 14 commits into from Aug 14, 2022

Conversation

zashraf1985
Copy link
Contributor

@zashraf1985 zashraf1985 commented Jun 2, 2022

Summary

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

  • Parses new datafile odp values (“publicKey” and “host”) from the “integrations” section. These values are used to send requests to the ODP server.

  • Parses 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.

Test plan

  • Added New unit tests
  • Existing Tests should pass

Issues

  • OASIS-8390

@coveralls
Copy link

coveralls commented Jun 2, 2022

Coverage Status

Coverage remained the same at 96.832% when pulling 9c01ed8 on zeeshan/ats-audience-targeting into fff21e0 on master.

@zashraf1985 zashraf1985 removed their assignment Jun 6, 2022
@zashraf1985 zashraf1985 marked this pull request as ready for review June 6, 2022 23:47
@zashraf1985 zashraf1985 requested a review from a team as a code owner June 6, 2022 23:47
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.

Qualified-type support looks good! I'd like to see UserContext passed down to audience evaluators. See my comments and let's chat about it.

Comment on lines 69 to 70
userAttributes: UserAttributes = {},
segments: string[] = [],
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 (unpacking and passing separate) better than passing down UserContext all the way? I see that we may need to use other fields (existing one like userId, or new ones to be added later) in evaluation later.

@@ -64,7 +66,8 @@ export class AudienceEvaluator {
evaluate(
audienceConditions: Array<string | string[]>,
audiencesById: { [id: string]: Audience },
userAttributes: UserAttributes = {}
userAttributes: UserAttributes = {},
segments: string[] = [],
Copy link
Contributor

Choose a reason for hiding this comment

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

add 'segments' in doc comments

@@ -99,14 +102,14 @@ export class AudienceEvaluator {
* @param {Condition} condition A single condition object to evaluate.
* @return {boolean|null} true if the condition is satisfied, null if a matcher is not found.
*/
evaluateConditionWithUserAttributes(userAttributes: UserAttributes, condition: Condition): boolean | null {
evaluateConditionWithUserAttributes(userAttributes: UserAttributes, segments: string[], condition: Condition): boolean | null {
Copy link
Contributor

Choose a reason for hiding this comment

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

update doc comments

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.

@@ -29,6 +29,7 @@ export default class OptimizelyUserContext {
private userId: string;
private attributes: UserAttributes;
private forcedDecisionsMap: { [key: string]: { [key: string]: OptimizelyForcedDecision } };
private qualifiedSegments: string[] = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we open {get, set} for this prop to public? They can override it with own segments for debugging or other purposes?

}

function qualifiedEvaluator(condition: Condition, user: OptimizelyUserContext): boolean {
//return user.getQualifiedSegments().indexOf(condition.value as string) > -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this comment

}
const audience = new AudienceEvaluator();

it('shoud evaluate to true if segment is found', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('shoud evaluate to true if segment is found', () => {
it('should evaluate to true if segment is found', () => {

fix same for others below

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

@opti-jnguyen opti-jnguyen changed the title feat: Add ODP Segments support in Audience Evaluation feat: Add ODP Datafile Parsing and Audience Evaluation Aug 10, 2022
@opti-jnguyen
Copy link
Contributor

@zashraf1985 - I believe I fixed the import issues, however although the Git Actions shows all tests passing there seems to be 2 failing / 2 pending checks still. What am I missing?

@opti-jnguyen opti-jnguyen merged commit e4b5dc9 into master Aug 14, 2022
@opti-jnguyen opti-jnguyen deleted the zeeshan/ats-audience-targeting branch August 14, 2022 15:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants