Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions src/Optimizely/ProjectConfig.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
/**
* Copyright 2016, 2018, Optimizely
* Copyright 2016, 2018-2019 Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -455,7 +455,7 @@ public function getEvent($eventKey)
* @param $audienceId string ID of the audience.
*
* @return Audience Entity corresponding to the ID.
* Dummy entity is returned if ID is invalid.
* Null is returned if ID is invalid.
*/
public function getAudience($audienceId)
{
Expand All @@ -465,7 +465,8 @@ public function getAudience($audienceId)

$this->_logger->log(Logger::ERROR, sprintf('Audience ID "%s" is not in datafile.', $audienceId));
$this->_errorHandler->handleError(new InvalidAudienceException('Provided audience is not in datafile.'));
return new Audience();

return null;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I will keep the previous behavior.

}

/**
Expand Down
7 changes: 6 additions & 1 deletion src/Optimizely/Utils/Validator.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
/**
* Copyright 2016-2018, Optimizely
* Copyright 2016-2019, Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -160,7 +160,12 @@ public static function isUserInExperiment($config, $experiment, $userAttributes)

$evaluateAudience = function($audienceId) use ($config, $evaluateCustomAttr) {
$conditionTreeEvaluator = new ConditionTreeEvaluator();

$audience = $config->getAudience($audienceId);
if ($audience === null) {
Copy link
Contributor

@aliabbasrizvi aliabbasrizvi Jan 8, 2019

Choose a reason for hiding this comment

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

I think I have a preference to keep previous behavior and check if $audience->getId equals null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why do you prefer it this way? I don't see any advantage to returning empty entity objects. Instead, they have caused issues. #107
Thus we have replaced returning empty entity by null at multiple places.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead, they have caused issues. #107

I see your point. Like I said it is a preference and I am not married to the idea.
Regarding that issue, I would say that it was a bug and the implementer should have done due diligence.

return null;
}

return $conditionTreeEvaluator->evaluate($audience->getConditionsList(), $evaluateCustomAttr);
};

Expand Down
4 changes: 2 additions & 2 deletions tests/ProjectConfigTest.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
/**
* Copyright 2016-2018, Optimizely
* Copyright 2016-2019, Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -546,7 +546,7 @@ public function testGetAudienceInvalidKey()
->method('handleError')
->with(new InvalidAudienceException('Provided audience is not in datafile.'));

$this->assertEquals(new Audience(), $this->config->getAudience('invalid_id'));
$this->assertNull($this->config->getAudience('invalid_id'));
}

public function testGetAttributeValidKey()
Expand Down
21 changes: 20 additions & 1 deletion tests/UtilsTests/ValidatorTest.php
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<?php
/**
* Copyright 2016-2018, Optimizely
* Copyright 2016-2019, Optimizely
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -346,6 +346,25 @@ public function testIsUserInExperimentWithAudienceConditionsSetToAudienceIdStrin
);
}

public function testIsUserInExperimentWithUnknownAudienceId()
{
$config = new ProjectConfig(DATAFILE, $this->loggerMock, new NoOpErrorHandler());
$experiment = $config->getExperimentFromKey('test_experiment');

// Both audience Ids and audience conditions exist. Audience Ids is ignored.
$experiment->setAudienceIds([]);
$experiment->setAudienceConditions(["or", "unknown_audience_id", "7718080042"]);

// User qualifies for audience with ID "7718080042".
$this->assertTrue(
Validator::isUserInExperiment(
$config,
$experiment,
['device_type' => 'iPhone', 'location' => 'San Francisco']
)
);
}

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a pretty minimalist test case, but I think it's good enough. @aliabbasrizvi thoughts?

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 is fine, but no way do I know that iPhone San Francisco users are in audience 7718080042.

May be add more comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, yes, we could make it clearer that the experiment activates because the user passes the conditions for the known audience

// test that isUserInExperiment evaluates simple audience.
public function testIsUserInExperimentWithSimpleAudience()
{
Expand Down