Feature Notification Center #73

oakbani · 2017-11-15T11:31:20Z

Contains changes of #71

Covered Unit Tests

Notification Listener

addListener Invalid params (invalid type, invalid callable) ✔️
addListener Verify added
- anaonymous function ✔️
- object function ✔️
- plain function ✔️
addNotificationForMultipleNotificationTypes ✔️
addNotificationForMultipleCallbacksForSingleNotificationType ✔️
addListener Verify already added is not re-added ✔️
removeListener
- Verify nothing is removed if id not found ✔️
- Verify removed ✔️
- Verify nothing is removed for the same id(for which a callback was removed) ✔️
clearListener
- Invalid ID ✔️
- Verify all removed for ID and remaining intact ✔️
clearAllListeners Verify all removed ✔️
fireNotification
- Exception case. Less num of args given than expected ✔️
- Verify all callables with args are executed and no other types are called ✔️
- Verify all callables without args are executed and no other types are called ✔️

Optimizely
Note: Separate sendImpression unit testing from activate unit tests and verify that activate calls/not calls send impression given scenario

sendImpression
- Verify calls fireNotification without attributes ✔️
- Verify calls fireNotification with attributes ✔️
track Verify calls fireNotification where relevant ✔️
isFeatureEnabled calls fireNotification both with source Experiment and source Rollout ✔️

optibot · 2017-11-15T11:31:22Z

Can one of the admins verify this patch?

coveralls · 2017-11-15T11:36:40Z

Coverage increased (+1.05%) to 94.195% when pulling 42a285a on msohailhussain:Feature-NotificationCenter into afa7a55 on optimizely:master.

coveralls · 2017-11-16T13:58:56Z

Coverage increased (+1.09%) to 94.234% when pulling bd218fc on msohailhussain:Feature-NotificationCenter into afa7a55 on optimizely:master.

thomaszurkan-optimizely · 2017-11-17T05:25:31Z

src/Optimizely/Notification/NotificationCenter.php

+     * @param  array  $args              Array of items to pass as arguments to the callback
+     *
+     */
+    public function fireNotifications($notification_type, array $args = [])


can we change this to sendNotifications?

thomaszurkan-optimizely · 2017-11-17T05:32:08Z

src/Optimizely/Optimizely.php

+                    $featureFlagKey,
+                    $userId,
+                    $attributes,
+                    $audience


we are passing back an array of audiences. it should be the audience that the user fit. right now it will be an array of one.

Okay. I am confused on why are we always picking the first audience. Please see my comment here
https://github.com/optimizely/python-sdk/pull/88/files/8d4c3b2476fc7e5e4f1dc8504fb7068e8065a327#diff-2340ceb056ad364022b798c358a6c8ffR399

thomaszurkan-optimizely · 2017-11-17T05:35:57Z

src/Optimizely/Optimizely.php

                $this->_logger->log(Logger::ERROR, sprintf(
                    'Unable to dispatch conversion event. Error %s', $exception->getMessage()));
            }



I like the sendImpression. Why not add a sendConversion as well?

sendImpression logic got separated from activate when we implemented isFeatureEnabled that had to send an impression event as well. Track is the only method right now that sends a conversion event. Separating out unit tests for sendConversion will require a lot of changes. Shall I do it in this PR or can we do it later?

We can do it later if necessary.

coveralls · 2017-11-17T11:17:37Z

Coverage increased (+1.03%) to 94.175% when pulling 898242f on msohailhussain:Feature-NotificationCenter into afa7a55 on optimizely:master.

coveralls · 2017-11-17T11:17:37Z

Coverage increased (+1.09%) to 94.234% when pulling 898242f on msohailhussain:Feature-NotificationCenter into afa7a55 on optimizely:master.

…-NotificationCenter # Conflicts: # src/Optimizely/Optimizely.php # src/Optimizely/ProjectConfig.php # tests/OptimizelyTest.php # tests/UtilsTests/VariableTypeUtilsTest.php

coveralls · 2017-11-21T08:05:57Z

Coverage decreased (-0.4%) to 94.234% when pulling ab37eb7 on msohailhussain:Feature-NotificationCenter into d0c5f4e on optimizely:master.

coveralls · 2017-11-21T08:26:29Z

Coverage decreased (-0.2%) to 94.453% when pulling e8863d6 on msohailhussain:Feature-NotificationCenter into d0c5f4e on optimizely:master.

coveralls · 2017-11-22T10:32:38Z

Coverage decreased (-0.3%) to 94.328% when pulling a18700d on msohailhussain:Feature-NotificationCenter into d0c5f4e on optimizely:master.

thomaszurkan-optimizely

I didn't see where you were testing that all the correct type arguments are passed for ACTIVATE, and TRACK callbacks. Did I miss something?

thomaszurkan-optimizely · 2017-11-27T19:30:01Z

src/Optimizely/Notification/NotificationType.php

+class NotificationType
+{
+    // format is EVENT: list of parameters to callback.
+    const ACTIVATE = "ACTIVATE:experiment, user_id,attributes, variation, event";


add space after attributes

thomaszurkan-optimizely · 2017-11-27T19:31:06Z

src/Optimizely/Optimizely.php

                $this->_logger->log(Logger::ERROR, sprintf(
                    'Unable to dispatch conversion event. Error %s', $exception->getMessage()));
            }



We can do it later if necessary.

oakbani · 2017-11-27T19:56:28Z

@thomaszurkan-optimizely
For activate, I verify that sendImpression is called with expected params such as here: https://github.com/optimizely/php-sdk/pull/73/files#diff-d1e79f065af60553c7d6afdd00433a85R394
I have added similar in each activate unit test both when sendImpression is expected and when it's not.

The unit tests for sendImpression verify that it's fireNotifications is called with expected params for ACTIVATE

For Track, I verify that fireNotifications is called with expected params. Such as here
https://github.com/optimizely/php-sdk/pull/73/files#diff-d1e79f065af60553c7d6afdd00433a85R1567
and similarly in other TRACK unit tests

…ilhussain/php-sdk into Feature-NotificationCenter

coveralls · 2017-11-27T20:06:17Z

Coverage decreased (-0.3%) to 94.328% when pulling c0a1ada on msohailhussain:Feature-NotificationCenter into d0c5f4e on optimizely:master.

coveralls · 2017-11-27T23:26:05Z

Coverage decreased (-0.3%) to 94.328% when pulling 146a886 on msohailhussain:Feature-NotificationCenter into d4a4aad on optimizely:master.

coveralls · 2017-11-27T23:26:06Z

Coverage decreased (-0.3%) to 94.328% when pulling 146a886 on msohailhussain:Feature-NotificationCenter into d4a4aad on optimizely:master.

coveralls · 2017-11-29T13:25:31Z

Coverage increased (+2.2%) to 96.844% when pulling a2e8bee on msohailhussain:Feature-NotificationCenter into d4a4aad on optimizely:master.

oakbani · 2017-11-29T13:31:36Z

src/Optimizely/ProjectConfig.php

        }

        $variationId = $experimentToVariationMap[$experimentId];
-        // check for null and empty string variation ID


We already enforce these checks in setForcedVariation

oakbani · 2017-11-29T13:32:07Z

src/Optimizely/ProjectConfig.php

-
        $variation = $this->getVariationFromId($experimentKey, $variationId);
        $variationKey = $variation->getKey();
-        // check if the variation exists in the datafile (a new variation is returned if it is not in the datafile)


Same as above

mikeproeng37 · 2017-11-29T18:10:40Z

E2E tests are passing. Merging in.

Feature Notification Center (#13)

42a285a

🖊️ Renamed notification type constants

bd218fc

thomaszurkan-optimizely suggested changes Nov 17, 2017

View reviewed changes

🖊️ Renamed fireNotifications to sendNot..

898242f

Merge remote-tracking branch 'remotes/optimizely/master' into Feature…

ab37eb7

…-NotificationCenter # Conflicts: # src/Optimizely/Optimizely.php # src/Optimizely/ProjectConfig.php # tests/OptimizelyTest.php # tests/UtilsTests/VariableTypeUtilsTest.php

🖊️ Code coverage nit

e8863d6

✏️ feature_experiment/rollout notifications removed

a18700d

thomaszurkan-optimizely suggested changes Nov 27, 2017

View reviewed changes

oakbani added 2 commits November 28, 2017 00:57

nit: Space

870cb5e

Merge branch 'Feature-NotificationCenter' of https://github.com/msoha…

c0a1ada

…ilhussain/php-sdk into Feature-NotificationCenter

thomaszurkan-optimizely approved these changes Nov 27, 2017

View reviewed changes

Merge branch 'master' into Feature-NotificationCenter

146a886

Improve Code Coverage

a2e8bee

oakbani commented Nov 29, 2017

View reviewed changes

mikeproeng37 merged commit 4cb2c35 into optimizely:master Nov 29, 2017

oakbani deleted the Feature-NotificationCenter branch November 30, 2017 02:53

Feature Notification Center #73

Feature Notification Center #73

Uh oh!

Conversation

oakbani commented Nov 15, 2017

Covered Unit Tests

Uh oh!

optibot commented Nov 15, 2017

Uh oh!

coveralls commented Nov 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coveralls commented Nov 17, 2017

Uh oh!

coveralls commented Nov 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 21, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 22, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

thomaszurkan-optimizely left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oakbani commented Nov 27, 2017

Uh oh!

coveralls commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 27, 2017

Uh oh!

coveralls commented Nov 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

coveralls commented Nov 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mikeproeng37 commented Nov 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

coveralls commented Nov 15, 2017 •

edited

Loading

coveralls commented Nov 16, 2017 •

edited

Loading

coveralls commented Nov 17, 2017 •

edited

Loading

coveralls commented Nov 21, 2017 •

edited

Loading

coveralls commented Nov 21, 2017 •

edited

Loading

coveralls commented Nov 22, 2017 •

edited

Loading

coveralls commented Nov 27, 2017 •

edited

Loading

coveralls commented Nov 27, 2017 •

edited

Loading

coveralls commented Nov 29, 2017 •

edited

Loading