Skip to content

feat: Add init with sdk key #189

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.

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

Merged
merged 5 commits into from
Apr 3, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
17 changes: 14 additions & 3 deletions src/Optimizely/Optimizely.php
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@
use Optimizely\Notification\NotificationCenter;
use Optimizely\Notification\NotificationType;
use Optimizely\OptimizelyConfig\OptimizelyConfigService;
use Optimizely\ProjectConfigManager\HTTPProjectConfigManager;
use Optimizely\ProjectConfigManager\ProjectConfigManagerInterface;
use Optimizely\ProjectConfigManager\StaticProjectConfigManager;
use Optimizely\UserProfile\UserProfileServiceInterface;
Expand Down Expand Up @@ -96,7 +97,7 @@ class Optimizely
/**
* @var ProjectConfigManagerInterface
*/
private $_projectConfigManager;
public $configManager;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this have to be public now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HTTPProjectConfigManager exposes a public method fetch, which the user can use to get the latest datafile from cdn. Previously, we were expecting the user to create it's own config manager and pass. Now, we will be creating httpconfigmanager given the sdk key so the user must be able to access it to fetch the latest datafile.


/**
* @var NotificationCenter
Expand All @@ -112,6 +113,7 @@ class Optimizely
* @param $errorHandler ErrorHandlerInterface
* @param $skipJsonValidation boolean representing whether JSON schema validation needs to be performed.
* @param $userProfileService UserProfileServiceInterface
* @param $sdkKey string uniquely identifying the datafile corresponding to project and environment combination. Must provide at least one of datafile or sdkKey.
* @param $configManager ProjectConfigManagerInterface provides ProjectConfig through getConfig method.
* @param $notificationCenter NotificationCenter
*/
Expand All @@ -122,6 +124,7 @@ public function __construct(
ErrorHandlerInterface $errorHandler = null,
$skipJsonValidation = false,
UserProfileServiceInterface $userProfileService = null,
$sdkKey = null,
Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't this break previous versions since the order changed? Shouldn't this become the last parameter?

Also ok if we file this as a breaking change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tried to be consistent with python and ruby sdks. I would recommend to go for a breaking change and make sdkKey the first param of constructor. What do you suggest?

ProjectConfigManagerInterface $configManager = null,
NotificationCenter $notificationCenter = null
) {
Expand All @@ -132,7 +135,15 @@ public function __construct(
$this->_eventBuilder = new EventBuilder($this->_logger);
$this->_decisionService = new DecisionService($this->_logger, $userProfileService);
$this->notificationCenter = $notificationCenter ?: new NotificationCenter($this->_logger, $this->_errorHandler);
$this->_projectConfigManager = $configManager ?: new StaticProjectConfigManager($datafile, $skipJsonValidation, $this->_logger, $this->_errorHandler);
$this->configManager = $configManager;

if ($this->configManager === null) {
if ($sdkKey) {
$this->configManager = new HTTPProjectConfigManager($sdkKey, null, null, true, $datafile, $skipJsonValidation, $this->_logger, $this->_errorHandler, $this->notificationCenter);
} else {
$this->configManager = new StaticProjectConfigManager($datafile, $skipJsonValidation, $this->_logger, $this->_errorHandler);
}
}
}

/**
Expand All @@ -141,7 +152,7 @@ public function __construct(
*/
protected function getConfig()
{
$config = $this->_projectConfigManager->getConfig();
$config = $this->configManager->getConfig();
return $config instanceof DatafileProjectConfig ? $config : null;
}

Expand Down
79 changes: 73 additions & 6 deletions tests/OptimizelyTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,11 @@
namespace Optimizely\Tests;

use Exception;
use GuzzleHttp\Client;
use GuzzleHttp\Handler\MockHandler;
use GuzzleHttp\HandlerStack;
use GuzzleHttp\Middleware;
use GuzzleHttp\Psr7\Response;
use Monolog\Logger;
use Optimizely\Config\DatafileProjectConfig;
use Optimizely\DecisionService\DecisionService;
Expand Down Expand Up @@ -63,9 +68,7 @@ private function setOptimizelyConfigObject($optimizely, $config, $configManager)
$projConfig->setAccessible(true);
$projConfig->setValue($configManager, $config);

$projConfigManager = new \ReflectionProperty(Optimizely::class, '_projectConfigManager');
$projConfigManager->setAccessible(true);
$projConfigManager->setValue($optimizely, $configManager);
$optimizely->configManager = $configManager;
}

public function setUp()
Expand Down Expand Up @@ -221,6 +224,72 @@ public function testInitDatafileInvalidFormat()
$this->expectOutputRegex('/Provided datafile is in an invalid format./');
}

public function testInitWithSdkKey()
{
$sdkKey = "some-sdk-key";
$optimizelyClient = new Optimizely(null, null, null, null, null, null, $sdkKey);

// client hasn't been mocked yet. Hence, activate should return null.
$actualVariation = $optimizelyClient->activate('test_experiment_integer_feature', 'test_user');
$this->assertNull($actualVariation);

// Mock http client to return a valid datafile
$mock = new MockHandler([
new Response(200, [], $this->datafile)
]);

$container = [];
$history = Middleware::history($container);
$handler = HandlerStack::create($mock);
$handler->push($history);

$client = new Client(['handler' => $handler]);
$httpClient = new \ReflectionProperty(HTTPProjectConfigManager::class, 'httpClient');
$httpClient->setAccessible(true);
$httpClient->setValue($optimizelyClient->configManager, $client);

// Fetch datafile
$optimizelyClient->configManager->fetch();

// activate should return expected variation.
$actualVariation = $optimizelyClient->activate('test_experiment_integer_feature', 'test_user');
$this->assertEquals('variation', $actualVariation);

// assert that https call is made to mock as expected.
$transaction = $container[0];
$this->assertEquals(
'https://cdn.optimizely.com/datafiles/some-sdk-key.json',
$transaction['request']->getUri()
);
}

public function testInitWithBothSdkKeyAndDatafile()
{
$sdkKey = "some-sdk-key";
$optimizelyClient = new Optimizely(DATAFILE, null, null, null, null, null, $sdkKey);

// client hasn't been mocked yet. Hence, config manager should return config of hardcoded
// datafile.
$this->assertEquals('15', $optimizelyClient->configManager->getConfig()->getRevision());


// Mock http client to return a valid datafile
$mock = new MockHandler([
new Response(200, [], $this->typedAudiencesDataFile)
]);

$handler = HandlerStack::create($mock);
$client = new Client(['handler' => $handler]);
$httpClient = new \ReflectionProperty(HTTPProjectConfigManager::class, 'httpClient');
$httpClient->setAccessible(true);
$httpClient->setValue($optimizelyClient->configManager, $client);

// Fetch datafile
$optimizelyClient->configManager->fetch();

$this->assertEquals('3', $optimizelyClient->configManager->getConfig()->getRevision());
}

public function testActivateInvalidOptimizelyObject()
{
$optimizelyMock = $this->getMockBuilder(Optimizely::class)
Expand Down Expand Up @@ -4471,9 +4540,7 @@ public function testGetConfigReturnsDatafileProjectConfigInstance()
->setMethods(array('getConfig'))
->getMock();

$projectConfigManager = new \ReflectionProperty(Optimizely::class, '_projectConfigManager');
$projectConfigManager->setAccessible(true);
$projectConfigManager->setValue($optlyObject, $projectConfigManagerMock);
$optlyObject->configManager = $projectConfigManagerMock;

$expectedProjectConfig = new DatafileProjectConfig($this->datafile, $this->loggerMock, new NoOpErrorHandler());

Expand Down