Skip to content

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

Merged
jaeopt merged 5 commits intomasterfrom
uzair/uniqueexperimentkey
Aug 4, 2021
Merged

feat: Duplicate experiment key issue with multi feature flag#226
jaeopt merged 5 commits intomasterfrom
uzair/uniqueexperimentkey

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

coveralls commented Jul 19, 2021

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

coveralls commented Jul 19, 2021

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

coveralls commented Jul 19, 2021

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

@zashraf1985 zashraf1985 Jul 26, 2021

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

yasirfolio3 commented Jul 27, 2021

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

msohailhussain commented Aug 2, 2021

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

@jaeopt jaeopt jaeopt approved these changes

+1 more reviewer

@zashraf1985 zashraf1985 zashraf1985 approved these changes

Reviewers whose approvals may not affect merge requirements

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