Skip to content

feat: Duplicate experiment key issue with multi feature flag #226

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Aug 4, 2021
Merged

feat: Duplicate experiment key issue with multi feature flag #226

merged 5 commits into from
Aug 4, 2021

Conversation

ozayr-zaviar
Copy link
Contributor

@ozayr-zaviar ozayr-zaviar commented Jul 19, 2021 •
edited
Loading

Summary

  • Fixed an issue related to duplicate experimentKey.
  • While trying to get variation from the variationKeyMap, it was unable to find because the latest experimentKey was overriding the previous one.

TestPlan

  • Run with FSC

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 97.13% when pulling c9f901a on uzair/uniqueexperimentkey into 39cfc1c on master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 97.13% when pulling c9f901a on uzair/uniqueexperimentkey into 39cfc1c on master.

@coveralls
Copy link

coveralls commented Jul 19, 2021 •
edited
Loading

Coverage Status

Coverage decreased (-0.2%) to 97.834% when pulling 473e0b2 on uzair/uniqueexperimentkey into 39cfc1c on master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.9%) to 97.13% when pulling c9f901a on uzair/uniqueexperimentkey into 39cfc1c on master.

zashraf1985
zashraf1985 approved these changes Jul 26, 2021
View reviewed changes
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.

Looks Good! Just added a comment to fix a redundant variable assignment.

src/Optimizely/Optimizely.php
} else {
$variationKey = null;
$ruleKey = null;
$experimentId = null;
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks redundant?

@msohailhussain msohailhussain marked this pull request as ready for review July 27, 2021 05:09
@msohailhussain msohailhussain requested a review from a team as a code owner July 27, 2021 05:09
@yasirfolio3
Copy link
Contributor

LGTM!

jaeopt
jaeopt requested changes Jul 28, 2021
View reviewed changes
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.

All changes look good. A couple of suggestions -

  1. can we add new test cases for duplicate keys (duplicate experiment keys, and/or duplicate delivery-rule keys)
  2. I found this line is still vulnerable to duplicate keys. Can you confirm it's ok?

    php-sdk/src/Optimizely/DecisionService/DecisionService.php

    Line 599 in 14e336c

    $variation = $projectConfig->getVariationFromId($experimentKey, $variationId);

@msohailhussain
Copy link
Contributor

All changes look good. A couple of suggestions -

  1. can we add new test cases for duplicate keys (duplicate experiment keys, and/or duplicate delivery-rule keys)
  2. I found this line is still vulnerable to duplicate keys. Can you confirm it's ok?

    php-sdk/src/Optimizely/DecisionService/DecisionService.php

    Line 599 in 14e336c

    $variation = $projectConfig->getVariationFromId($experimentKey, $variationId);

Thanks Jae. Will take care of it.

@msohailhussain msohailhussain requested a review from jaeopt August 4, 2021 05:04
jaeopt
jaeopt approved these changes Aug 4, 2021
View reviewed changes
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

@jaeopt jaeopt merged commit 122e6ab into master Aug 4, 2021
@jaeopt jaeopt deleted the uzair/uniqueexperimentkey branch August 4, 2021 16:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zashraf1985 zashraf1985 zashraf1985 approved these changes

@jaeopt jaeopt jaeopt approved these changes

Assignees

@ozayr-zaviar ozayr-zaviar

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ozayr-zaviar @coveralls @yasirfolio3 @msohailhussain @zashraf1985 @jaeopt