Skip to content

feat: Implement decideAll api #635

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

Merged
merged 11 commits into from
Dec 11, 2020
Merged

Conversation

yavorona
Copy link
Contributor

Summary

  • Implemented decideAll and decideForKeys

Test plan

  • Manual testing + unit tests

Issues

@yavorona yavorona removed their assignment Dec 10, 2020
@yavorona yavorona requested review from jaeopt and mjc1283 December 10, 2020 22:44
@coveralls
Copy link

coveralls commented Dec 10, 2020

Coverage Status

Coverage decreased (-0.0005%) to 96.764% when pulling adc3349 on pnguen/implement-decideAll-api into 132587e on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.02%) to 96.747% when pulling 92e9b2d on pnguen/implement-decideAll-api into 132587e on master.

@yavorona yavorona force-pushed the pnguen/implement-decideAll-api branch from 2d902fb to 2767d02 Compare December 10, 2020 23:07
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 more tests suggested.

userId,
});
var decisionMap = user.decideAll();
assert.deepEqual(decisionMap, fakeDecisionMap);
Copy link
Contributor

Choose a reason for hiding this comment

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

Same. Can we validate "userId" and "options" passing down to fakeOptimizely ok?

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 updated tests to validate the parameters fakeOptimizely is called with including options and user, where userId is an attribute of user.

userId,
});
var decisionMap = user.decideForKeys([ flagKey1, flagKey2 ]);
assert.deepEqual(decisionMap, fakeDecisionMap);
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 validate "userId" and "options" passing down to fakeOptimizely ok?

});
});

describe('with ENABLED_FLAGS_ONLY flag in default decide options', function() {
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 add the same test with ENABLED_FLAGS_ONLY in decide call parameter?

Comment on lines 1598 to 1604
* Returns a key-map of decision results for multiple flag keys and a user context.
* If the SDK finds an error for a key, the response will include a decision for the key showing reasons for the error.
* The SDK will always return key-mapped decisions. When it cannot process requests, it will return an empty map after logging the errors.
* @param {OptimizelyUserContext} user A user context associated with this OptimizelyClient
* @param {string[]} keys An array of flag keys for which decisions will be made.
* @param {OptimizelyDecideOptions[]} options An array of options for decision-making.
* @return {[key: string]: OptimizelyDecision} All decision results mapped by flag keys.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggest using "object" rather than key-map or map.

sinon.assert.calledThrice(optlyInstance.eventDispatcher.dispatchEvent);
});

it('should return decision results map with only enabled flags and excluded variables when EXCLUDE_VARIABLES_FLAG is passed in', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it supposed to return all flags (not only enabled flags)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It should return enabled flags only because [ OptimizelyDecideOptions. ENABLED_FLAGS_ONLY ] is passed in defaultDecideOptions when instantiating optimizely client

}

const allDecideOptions = this.getAllDecideOptions(options);
let optimizelyDecision: OptimizelyDecision;
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: suggest moving optimizelyDecision inside the scope that uses it

@yavorona yavorona force-pushed the pnguen/implement-decideAll-api branch from ac9a96b to 5d91f24 Compare December 11, 2020 22:24
@yavorona yavorona force-pushed the pnguen/implement-decideAll-api branch from 5b09e01 to b9fab5e Compare December 11, 2020 22:56
@yavorona yavorona removed their assignment Dec 11, 2020
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

@yavorona yavorona merged commit 599b694 into master Dec 11, 2020
@yavorona yavorona deleted the pnguen/implement-decideAll-api branch December 11, 2020 23:42
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