Skip to content

Commit

Permalink
fix: correct merging of contexts with targetingKey (#136)
Browse files Browse the repository at this point in the history
<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR
<!-- add the description of the PR here -->

Corrects the behavior of EvaluationContextMerger `merge()` when using
string `targetingKey`.

### Related Issues
<!-- add here the GitHub issue that this PR resolves if applicable -->

Fixes #135 

### Notes
<!-- any additional notes for this PR -->

### Follow-up Tasks
<!-- anything that is related to this PR but not done here should be
noted under this section -->
<!-- if there is a need for a new issue, please link it here -->

### How to test
<!-- if applicable, add testing instructions under this section -->

The `testEvaluationContextMergingTargetingKey` test in
EvaluationContextTest.php has been added. The test fails without the
proposed fix, as expected.

Signed-off-by: Tommaso Tofacchi <tofacchitommaso@gmail.com>
  • Loading branch information
mmito authored Oct 28, 2024
1 parent 712d8e4 commit 3f141d7
Show file tree
Hide file tree
Showing 2 changed files with 16 additions and 3 deletions.
6 changes: 3 additions & 3 deletions src/implementation/flags/EvaluationContextMerger.php
Original file line number Diff line number Diff line change
Expand Up @@ -47,10 +47,10 @@ public static function merge(?EvaluationContextInterface ...$contexts): Evaluati

/** @var ?string $newTargetingKey */
$newTargetingKey = null;
if (!is_null($calculatedTargetingKey) && strlen($calculatedTargetingKey) > 0) {
$newTargetingKey = $calculatedTargetingKey;
} elseif (!is_null($additionalTargetingKey) && strlen($additionalTargetingKey) > 0) {
if (!is_null($additionalTargetingKey) && strlen($additionalTargetingKey) > 0) {
$newTargetingKey = $additionalTargetingKey;
} elseif (!is_null($calculatedTargetingKey) && strlen($calculatedTargetingKey) > 0) {
$newTargetingKey = $calculatedTargetingKey;
}

$mergedAttributes = AttributesMerger::merge(
Expand Down
13 changes: 13 additions & 0 deletions tests/unit/EvaluationContextTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -127,4 +127,17 @@ public function testEvaluationContextMerging(): void

$this->assertEquals($expectedEvaluationContextAttributes, $actualEvaluationContextAttributes);
}

public function testEvaluationContextMergingTargetingKey(): void
{
$firstEvaluationContext = new EvaluationContext('default');
$secondEvaluationContext = new EvaluationContext('merged_key');

$expectedEvaluationContextAttributes = 'merged_key';

$actualEvaluationContext = EvaluationContext::merge($firstEvaluationContext, $secondEvaluationContext);
$actualEvaluationContextAttributes = $actualEvaluationContext->getTargetingKey();

$this->assertEquals($expectedEvaluationContextAttributes, $actualEvaluationContextAttributes);
}
}

0 comments on commit 3f141d7

Please sign in to comment.