Skip to content

feat: Implement IGNORE_USER_PROFILE_SERVICE flag in decide options #642

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 7 commits into from
Jan 15, 2021

Conversation

yavorona
Copy link
Contributor

@yavorona yavorona commented Jan 12, 2021

Summary

  • Implemented IGNORE_USER_PROFILE_SERVICE flag in decide options

Test plan

  • Fixed affected unit tests
  • Added unit test coverage with IGNORE_USER_PROFILE_SERVICE decide option
  • Manual Testing

Note

This pr is to be finalized once #640 is merged

@yavorona yavorona added the WIP label Jan 12, 2021
@yavorona yavorona requested a review from a team as a code owner January 12, 2021 21:25
@yavorona yavorona self-assigned this Jan 12, 2021
@coveralls
Copy link

coveralls commented Jan 12, 2021

Coverage Status

Coverage increased (+0.006%) to 96.951% when pulling 396e0e2 on pnguen/ignore-user-profile-service into e6454b9 on master.

@yavorona yavorona removed the WIP label Jan 12, 2021
@yavorona yavorona force-pushed the pnguen/ignore-user-profile-service branch 2 times, most recently from 8944a95 to c57e416 Compare January 13, 2021 18:27
@yavorona yavorona removed their assignment Jan 13, 2021
@yavorona yavorona requested review from jaeopt and mjc1283 January 13, 2021 18:31
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. See a few changes suggested.

@@ -5621,6 +5621,50 @@ describe('lib/optimizely', function() {
);
});
});

describe('with IGNORE_USER_PROFILE_SERVICE flag in 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.

This test looks good. Can you do the same test with "IGNORE_USER_PROFILE" in defaultDecideOptions?

* @param {string} experimentKey
* @param {string} userId
* @param {Object} attributes
* @param {OptimizelyDecideOptions[]} options Decide options
Copy link
Contributor

Choose a reason for hiding this comment

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

Fix this type. Not an array.

var shouldIgnoreUPS = options[OptimizelyDecideOptions.IGNORE_USER_PROFILE_SERVICE];

// check for sticky bucketing if decide options do not include shouldIgnoreUPS
if (!shouldIgnoreUPS) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be used to bypass "saveUserProfile" as well.
I see a test case confirming this save bypass. Wondering how the test passed :)

Copy link
Contributor Author

@yavorona yavorona Jan 13, 2021

Choose a reason for hiding this comment

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

I believe "saveUserProfile" should not be called in this case since we are returning stored variation. I wrapped __saveUserProfile call below in if (!shouldIgnoreUPS) {..} statement and added a unit test to incorporate this comment.

@yavorona yavorona force-pushed the pnguen/ignore-user-profile-service branch 2 times, most recently from af78f6f to 4041f2b Compare January 13, 2021 23:43
@yavorona yavorona removed their assignment Jan 13, 2021
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 force-pushed the pnguen/ignore-user-profile-service branch from 4041f2b to 396e0e2 Compare January 14, 2021 23:49
@yavorona yavorona merged commit dc47f71 into master Jan 15, 2021
@yavorona yavorona deleted the pnguen/ignore-user-profile-service branch January 15, 2021 20:15
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