Is Feature Enabled and Feature Variable Accessor APIs #71
Conversation
commit 75224a4 Author: Owais <oakbani@folio3.com> Date: Thu Oct 26 18:14:31 2017 +0500 :memo: Final changes commit 75fa44d Author: Owais <oakbani@folio3.com> Date: Thu Oct 26 16:29:35 2017 +0500 # Conflicts: # tests/ProjectConfigTest.php # tests/UtilsTests/ValidatorTest.php commit a443056 Author: Owais <oakbani@folio3.com> Date: Thu Oct 26 12:21:16 2017 +0500 Partial fixes commit e03926b Author: Owais <oakbani@folio3.com> Date: Tue Oct 24 17:07:39 2017 +0500 Squashed commit of the following: commit 24569b8 Author: Owais <oakbani@folio3.com> Date: Fri Oct 20 18:18:55 2017 +0500 :pen: Fixed Code Coverage commit a6fa1e6 Author: Owais <oakbani@folio3.com> Date: Tue Oct 24 16:46:44 2017 +0500 :pen2: All Unit tests completed commit 72173d0 Author: Owais <oakbani@folio3.com> Date: Mon Oct 23 20:36:21 2017 +0500 :pen: 90% Unit tests done commit 9051dda Author: Owais <oakbani@folio3.com> Date: Mon Oct 23 18:41:04 2017 +0500 :pen: Unit tests done for feature experiment commit 8a5722d Author: Owais <oakbani@folio3.com> Date: Mon Oct 23 13:13:53 2017 +0500 :pen: Implementation First pass done commit 92dd219 Author: Owais <oakbani@folio3.com> Date: Fri Oct 20 17:32:04 2017 +0500 :pen: Feature Flag bucketing to Experiment done :memo: Todo: Rollout Logic commit ad15a2c Author: Owais <oakbani@folio3.com> Date: Fri Oct 20 12:52:47 2017 +0500 :pen: indentation commit dc41bf9 Author: Owais <oakbani@folio3.com> Date: Thu Oct 19 17:36:46 2017 +0500 :pencil2: Added Logger to Validator :pencil2: Added method defs commit 547b7ad Author: Owais <oakbani@folio3.com> Date: Thu Oct 19 17:11:22 2017 +0500 Feature Flags models and parsing from new v4 file
✏️ getVariableValueForType
🖊️ Logic slightly changed based on Decision 🖊️ nits addressed
# Conflicts: # src/Optimizely/Entity/Variation.php # src/Optimizely/ProjectConfig.php # src/Optimizely/Utils/Validator.php # tests/DecisionServiceTests/DecisionServiceTest.php # tests/ProjectConfigTest.php # tests/TestData.php
…ationForFeature # Conflicts: # src/Optimizely/Entity/Variation.php # src/Optimizely/ProjectConfig.php # src/Optimizely/Utils/Validator.php # tests/ProjectConfigTest.php # tests/TestData.php
…into FeatureFlag&Rollouts-newAPIs
|
Can one of the admins verify this patch? |
|
Will review this today. Thanks for sending across. |
aliabbasrizvi
left a comment
aliabbasrizvi
left a comment
There was a problem hiding this comment.
A note on the size of this change. It is at 1700 lines and seems to be doing a lot. A logical change could be to handle isFeatureEnabled in 1 change and variable accessors in another change.
I am going to finish reviewing this anyway, but it will go a lot faster if we keep the changes minimal in every change.
| class Decision | ||
| { | ||
| const DECISION_SOURCE_EXPERIMENT = 'experiment'; | ||
| const DECISION_SOURCE_ROLLOUT = 'rollout'; |
There was a problem hiding this comment.
Looks like 5 spaces and it should be 4 here and rest of the file is at 2 spaces which should be at 4. Please fix indentation.
There was a problem hiding this comment.
Please address this comment.
| { | ||
| return $this->_source; | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Add new line at the end
|
I see the reason why...perhaps branch off the other branch itself so that this review could be easier. |
aliabbasrizvi
left a comment
aliabbasrizvi
left a comment
There was a problem hiding this comment.
@oakbani if possible please diff against the other branch where most of the changes from this PR are captured.
| class Decision | ||
| { | ||
| const DECISION_SOURCE_EXPERIMENT = 'experiment'; | ||
| const DECISION_SOURCE_ROLLOUT = 'rollout'; |
There was a problem hiding this comment.
Please address this comment.
|
@aliabbasrizvi Please guide me on how can we effectively manage chained/stacked PRs in future. It became really troublesome for me to manage this one along with #69 and the ones preceding these. |
|
Hi @oakbani one thing may be that you push the branch first (with other changes) and then create a PR from this branch onto that other branch. That will help. |
|
@aliabbasrizvi , we don't have access to create a branch on Optimizely repo. So, if it's okay, in the given scenario in future, we can create a PR in our forked repo for diff and later direct it to Optimizely master. |
aliabbasrizvi
left a comment
aliabbasrizvi
left a comment
There was a problem hiding this comment.
There is a lack of unit tests here. Please add those. Generally things seem to be fine.
| * | ||
| * @return string Feature variable value / null | ||
| */ | ||
| public function getFeatureVariableValueForType( |
There was a problem hiding this comment.
And put it earlier in the file closer to other private functions.
There was a problem hiding this comment.
The issue at hand is that
- PHP Unit does not allow mocking private methods. I mock this method when testing the 4 API methods getVariationForFeatureString/Double etc.
- and if I make it protected, the method won't be directly callable from Optimizely Test class. Right now, since this method has the main and common logic for all of the 4 API methods. I have written all tests for this one. And just a couple of tests for each of the 4 API methods to ensure that they return exactly what this method returns.
The unit testing looks a lot structured this way. In the case of making it private, I will have to check for all unit tests of this method via one of the 4 API methods
…ision, repositioned getBucketingId
|
aliabbasrizvi
left a comment
aliabbasrizvi
left a comment
There was a problem hiding this comment.
Conditionally approving. Please address all comments before merging.
| $this->assertEquals($this->variationKeyControl, $variationKey, sprintf('Variation "%s" does not match expected user profile variation "%s".', $variationKey, $this->variationKeyControl)); | ||
| } | ||
|
|
||
| //should return nil and log a message when the feature flag's experiment ids array is empty |
There was a problem hiding this comment.
nit. Please put a single space after \\.
This comment applies to all statements in all changes in this file.
| ->with(Logger::INFO, | ||
| "The user 'user1' is not bucketed into any of the experiments using the feature 'boolean_feature'."); | ||
|
|
||
| $this->assertSame( |
There was a problem hiding this comment.
nit. Convention we have in assertion is to put expected value first and actual value second. Please use that here and else where.
| //make sure the user is not bucketed into the feature experiment | ||
| $this->decisionServiceMock->expects($this->at(0)) | ||
| ->method('getVariation') | ||
| ->will($this->returnValueMap($map)); |
There was a problem hiding this comment.
Indentation seems off. Please add 2 spaces on the subsequent lines here and else where in the various tests.
| null); | ||
| } | ||
|
|
||
| // should return the variation when the user is bucketed into a variation for the experiment on the feature flag |
There was a problem hiding this comment.
nit. Too many spaces after \\
| $expected_decision = [ | ||
| 'experiment' => $expected_experiment, | ||
| 'variation' => $expected_variation | ||
| ]; |
There was a problem hiding this comment.
Spacing is all off. Please fix.
|
|
||
| public function testValueCastingToInteger() | ||
| { | ||
| $this->assertSame($this->variableUtilObj->castStringToType('1000', 'integer'), 1000); |
There was a problem hiding this comment.
nit. Convention as stated above is to put expected, actual
src/Optimizely/Optimizely.php
Outdated
| * @param string User ID | ||
| * @param array Associative array of user attributes | ||
| */ | ||
| public function sendImpressionEvent($experimentKey, $variationKey, $userId, $attributes) |
There was a problem hiding this comment.
Styling changes required:
- This should be private method.
- Put private methods together earlier in the file under the constructor.
- Functions being referenced by another function should be placed before it.
There was a problem hiding this comment.
Made it protected so that it's mockable in Unit testing. Moved before public methods too
src/Optimizely/Optimizely.php
Outdated
| } | ||
|
|
||
| $feature_flag = $this->_config->getFeatureFlagFromKey($featureFlagKey); | ||
| if ($feature_flag == new FeatureFlag) { |
There was a problem hiding this comment.
Please rely on feature flag key and not on creating a new object.
| $this->_logger->log( | ||
| Logger::INFO, | ||
| "Variable '{$variableKey}' is not used in variation '{$variation->getKey()}' ". | ||
| "returning default value '{$variable_value}'." |
There was a problem hiding this comment.
Instead added comma after variation key
# Conflicts: # tests/DecisionServiceTests/DecisionServiceTest.php
# Conflicts: # src/Optimizely/Optimizely.php # tests/OptimizelyTest.php
|
@wangjoshuah @aliabbasrizvi if anything is not pending, can u plz merge this PR. |
|
build |
2 similar comments
|
build |
|
build |
…Flag&Rollouts-newAPIs # Conflicts: # tests/DecisionServiceTests/DecisionServiceTest.php
|
@wangjoshuah Merge conflicts resolved. |
|
build |
1 similar comment
|
build |
mikeproeng37
left a comment
mikeproeng37
left a comment
There was a problem hiding this comment.
Mostly looks good, please address remaining issues.
src/Optimizely/Optimizely.php
Outdated
| * | ||
| * @param $event string Event key representing the event which needs to be recorded. | ||
| * @param $userId string ID for user. | ||
| * @param $user string ID for user. |
There was a problem hiding this comment.
This is incorrect as the param name is still $userId
src/Optimizely/Optimizely.php
Outdated
| } | ||
|
|
||
| $feature_flag = $this->_config->getFeatureFlagFromKey($featureFlagKey); | ||
| if ($feature_flag == new FeatureFlag) { |
|
|
||
| public function testValueCastingToInteger() | ||
| { | ||
| $this->assertSame($this->variableUtilObj->castStringToType('1000', 'integer'), 1000); |
|
@mikeng13 issues addressed. |
7 participants
👂 Would also show differences of work done under decision service PR, until the other one is merged.