Skip to content

fix(decision-service): Adds bucketing id in getVariationForRollout method. - #200

Merged
Mike Ng (mikeproeng37) merged 5 commits into
masterfrom
fahad/bucketing-id-fix
Jan 14, 2019
Merged

fix(decision-service): Adds bucketing id in getVariationForRollout method.#200
Mike Ng (mikeproeng37) merged 5 commits into
masterfrom
fahad/bucketing-id-fix

Conversation

@mfahadahmed

@mfahadahmed Muhammad Fahad Ahmed (mfahadahmed) commented Dec 14, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

Summary

  • Adds getBucketingId call in getVariationForRollout method to bucket user with provided bucketing id.

@coveralls

Coveralls (coveralls) commented Dec 14, 2018

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.0005%) to 97.239% when pulling 046db6c on fahad/bucketing-id-fix into 599858b on master.

1 similar comment
@coveralls

Coveralls (coveralls) commented Dec 14, 2018

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.0005%) to 97.239% when pulling 046db6c on fahad/bucketing-id-fix into 599858b on master.

@coveralls

Coveralls (coveralls) commented Dec 14, 2018 •
edited
Loading

Copy link
Copy Markdown

Coverage Status

Coverage increased (+0.01%) to 97.279% when pulling ea1cbfe on fahad/bucketing-id-fix into cfea9ce on master.

@mfahadahmed

Muhammad Fahad Ahmed (mfahadahmed) commented Dec 17, 2018

Copy link
Copy Markdown
Contributor Author

Closing this PR as Bucketing ID is not supported in the rollout yet.

@aliabbasrizvi

Ali Abbas Rizvi (aliabbasrizvi) commented Dec 20, 2018

Copy link
Copy Markdown
Contributor

Muhammad Fahad Ahmed (@mfahadahmed) Mirza Sohail Hussain (@msohailhussain) I see that this was closed with a comment and then re-opened. I do not recall if bucketing ID is supported for rollout, so may be this change is not needed and PR can be closed?

aliabbasrizvi
Ali Abbas Rizvi (aliabbasrizvi) suggested changes Dec 21, 2018
View reviewed changes

@aliabbasrizvi Ali Abbas Rizvi (aliabbasrizvi) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Synced offline with mikeng13
Since this is a change to our bucketing, I'd like to see some unit tests which confirm that __buildBucketerParams is called with the output of _getBucketingId .

Setting to revise.

mikeproeng37
Mike Ng (mikeproeng37) approved these changes Jan 11, 2019
View reviewed changes

@mikeproeng37 Mike Ng (mikeproeng37) left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@mikeproeng37

Mike Ng (mikeproeng37) commented Jan 14, 2019

Copy link
Copy Markdown
Contributor

Ran the corresponding Compat tests: https://github.com/optimizely/fullstack-sdk-compatibility-suite/pull/187

Test run: https://travis-ci.com/optimizely/fullstack-sdk-compatibility-suite/builds/97281183

@mikeproeng37
Mike Ng (mikeproeng37) merged commit 9812af8 into master Jan 14, 2019
@mikeproeng37
Mike Ng (mikeproeng37) deleted the fahad/bucketing-id-fix branch January 14, 2019 21:12
Owais Akbani (oakbani) pushed a commit that referenced this pull request Apr 17, 2019
@mfahadahmed @oakbani
fix(decision-service): Adds bucketing id in getVariationForRollout me…
d6c3276 
…thod. (#200)

(cherry picked from commit 9812af8)
@rashidsp Rashid Siddique (rashidsp) mentioned this pull request Apr 18, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@mikeproeng37 Mike Ng (mikeproeng37) mikeproeng37 approved these changes
@aliabbasrizvi Ali Abbas Rizvi (aliabbasrizvi) aliabbasrizvi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@mfahadahmed @coveralls @aliabbasrizvi @mikeproeng37