10000 Feature Notification Center by mfahadahmed · Pull Request #35 · optimizely/csharp-sdk · GitHub
[go: up one dir, main page]
Skip to content

Feature Notification Center #35

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Nov 27, 2017
Merged

Feature Notification Center #35

merged 8 commits into from
Nov 27, 2017

Conversation

mfahadahmed
Copy link
Contributor
@mfahadahmed mfahadahmed commented Nov 15, 2017 •
edited by kellyroach-optimizely
Loading

OASIS-1861 Implement notification listeners

@optibot
Copy link
Contributor
optibot commented Nov 15, 2017

Can one of the admins verify this patch?

thomaszurkan-optimizely
thomaszurkan-optimizely suggested changes Nov 17, 2017
View reviewed changes
OptimizelySDK/Optimizely.cs Outdated
@@ -262,6 +265,8 @@ public void Track(string eventKey, string userId, UserAttributes userAttributes
Logger.Log(LogLevel.ERROR, string.Format("Unable to dispatch conversion event. Error {0}", exception.Message));
}

NotificationCenter.FireNotifications(NotificationCenter.NotificationType.Track, eventKey, userId,
Copy link

Choose a reason for hiding this comment

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

please change to SendNotifications

Sorry, something went wrong.

OptimizelySDK/Optimizely.cs Outdated
}

NotificationCenter.FireNotifications(NotificationCenter.NotificationType.FeatureRollout, featureKey, userId,
userAttributes, audience);
Copy link

Choose a reason for hiding this comment

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

pass back an array of audiences should be an array of one for now

kellyroach-optimizely
kellyroach-optimizely suggested changes Nov 17, 2017
View reviewed changes
OptimizelySDK.Tests/NotificationTests/NotificationCenterTests.cs Outdated
private TestNotificationCallbacks TestNotificationCallbacks;
private NotificationCenter.NotificationType NotificationTypeActivate = NotificationCenter.NotificationType.Activate;
private NotificationCenter.NotificationType NotificationTypeTrack = NotificationCenter.NotificationType.Track;
private NotificationCenter.NotificationType NotificationTypeFeatureExperiment = NotificationCenter.NotificationType.FeatureExperiment;
Copy link
Contributor

Choose a reason for hiding this comment

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

There is a fourth non-mobile NotificationType enum value FeatureRollout beyond Activate, Track, FeatureExperiment . I do not see FeatureRollout being tested in NotificationCenterTests.cs .

Copy link
Contributor

Choose a reason for hiding this comment

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

Please check here.
https://github.com/optimizely/csharp-sdk/pull/35/files#diff-8660e0a3c8615b111f08b9e4ab4f9b36R1689
, I have added more tests, you can see in the latest check-in.

@kellyroach-optimizely
Copy link
Contributor
kellyroach-optimizely commented Nov 17, 2017 •
edited
Loading

TEST: Test / Debug / "All Tests"

ISSUE: TestBucketLogsCorrectlyWhenUserProfileFailsToSave

            LoggerMock.Verify(l => l.Log(LogLevel.ERROR, string.Format
                ("Failed to save variation \"{0}\" of experiment \"{1}\" for user \"{2}\".", UserProfileId, variation.Id, experiment.Id))
                , Times.Once);
==>
Expected invocation on the mock once, but was 0 times: l => l.Log
(LogLevel.ERROR, "Failed to save variation "userProfileId" of experiment
"276" for user "223".")

@kellyroach-optimizely
Copy link
Contributor
kellyroach-optimizely commented Nov 17, 2017 •
edited
Loading

CODE COVERAGE WEAKNESSES ELSEWHERE IN SDK
Items which unit tests aren't hitting according to "Test" / " Analyze Code Coverage" / "All Tests"
https://docs.microsoft.com/en-us/visualstudio/test/using-code-coverage-to-determine-how-much-code-is-being-tested

FIRST: DefaultErrorHandler.cs
DefaultErrorHandler.cs HandleError has a bad score. Why?
These lines are never exercised by unit tests:
Logger.Log(LogLevel.ERROR, exception.Message);
throw exception;

SECOND: ConfigParser.cs
A GenerateMap method in ConfigParser.cs which is never used.

THIRD: OptimizelyException.cs
InvalidInputException and InvalidRolloutException are never
exercised.

@kellyroach-optimizely
Copy link
Contributor

C# EVENT BRAINSTORM

Why isn't Optimizely's "NotificationCenter.cs" coded around Microsoft C# events?

QUESTION: Should we be using C# events?
Handling and Raising Events
https://docs.microsoft.com/en-us/dotnet/standard/events/
How to: Raise and Consume Events
https://docs.microsoft.com/en-us/dotnet/standard/events/how-to-raise-and-consume-events
ANSWER: This Microsoft alternative must at least be considered as
an alternative to the NotificationCenter.cs code which I am seeing in this
pull request.

I would like to see a study and evaluation of a design using Microsoft C# event
approach for "notifications" before traveling down the current NotificationCenter.cs
route. This should at least be discussed.

@mikeproeng37
Copy link
Contributor

As mentioned today during meeting, we are going to maintain the current generic notificationCenter and not code around the platform-specific NotificationCenter provided by Microsoft.

kellyroach-optimizely
kellyroach-optimizely approved these changes Nov 20, 2017
View reviewed changes
OptimizelySDK/Notifications/NotificationCenter.cs Outdated
public int AddNotification(NotificationType notificationType, FeatureRolloutCallback featureRolloutCallback)
{
if (!IsNotificationTypeValid(notificationType, NotificationType.FeatureRollout))
return 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

There isn't a unit test for this line.

Copy link
Contributor
@msohailhussain msohailhussain Nov 21, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Please check it's already added here.
https://github.com/optimizely/csharp-sdk/pull/35/files/857b9344c2d95e2ee4580481757baf1d495d8802#diff-486ac263b47f8642a94df74943c6036bR84

OptimizelySDK/Notifications/NotificationCenter.cs
}
}

return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

There isn't a unit test for this line.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just added in the recent commit.
79d6a9a#diff-486ac263b47f8642a94df74943c6036bR50

OptimizelySDK/Notifications/NotificationCenter.cs
return 0;

return AddNotification(notificationType, (object)activateCallback);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

MANY AddNotification(...) METHODS

There are multiple AddNotification(...) methods in the NotificationCenter class.
If we were using Microsoft C# events, we would just be "+=" handlers for 4 different
events.

Copy link
Contributor

Choose a reason for hiding this comment

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

I may remove overloaded AddNotification methods but it is typed safe that's why I added it. Otherwise AddNotification(NotificationType, Delegate) can be used as an argument type to support all subtype delegates.
Using Microsoft C# events, approach will be different for Add/Remove and no notificationId & notificationtype concept would be there. Here approach is same with other SDKs.
D7AF

@msohailh
TestBucketLogsCorrectlyWhenUserProfileFailsToSave fixed.
79d6a9a 
Added one more test case for FeatureRollout Notification type.
Removed ConfigParser method which was not being used.
@msohailhussain
Copy link
Contributor

ISSUE: TestBucketLogsCorrectlyWhenUserProfileFailsToSave - FIXED
Code Coverage
DefaultErrorHandler.cs Unit test written
ConfigParser.cs GenerateMap removed

OptimizelyException.cs (Exception class not used)
InvalidInputException - @mikeng13 I am not sure but can you refer any SDK using InvalidInputException?
InvalidRolloutException - GetRolloutFromId in ProjectConfig being used.

mikeproeng37
mikeproeng37 reviewed Nov 21, 2017
View reviewed changes
OptimizelySDK.Tests/NotificationTests/NotificationCenterTests.cs Outdated

private NotificationType NotificationTypeActivate = NotificationType.Activate;
private NotificationType NotificationTypeTrack = NotificationType.Track;
private NotificationType NotificationTypeFeatureExperiment = NotificationType.FeatureExperiment;
Copy link
Contributor

Choose a reason for hiding this comment

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

@msohailh we've decided to exclude these notification types for now as we need to determine better verbiage/naming for them. So for now just keep Activate and Track

@msohailhussain
Copy link
Contributor

@mikeng13 I have removed FeatureExperiment and FeatureRollout.

thomaszurkan-optimizely
thomaszurkan-optimizely suggested changes Nov 27, 2017
View reviewed changes
Copy link
@thomaszurkan-optimizely thomaszurkan-optimizely left a comment

Choose a reason for hiding this comment

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

Looks pretty good. I have a couple of comments/questions.

OptimizelySDK.Tests/NotificationTests/NotificationCenterTests.cs Outdated
notificationCallbackMock.Setup(nc => nc.TestAnotherActivateCallback(It.IsAny<Experiment>(),
It.IsAny<string>(), It.IsAny<UserAttributes>(), It.IsAny<Variation>(), It.IsAny<LogEvent>()));

notificationCallbackMock.Setup(nc => nc.TestFeatureRolloutCallback(It.IsAny<string>(), It.IsAny<string>(),
Copy link

Choose a reason for hiding this comment

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

please remove feature rollout and feature experiment callbacks. they will be added later if you can keep them in branch somewhere

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed featurerollout and featureexperiment callbacks. 4b096d1

OptimizelySDK.Tests/NotificationTests/NotificationCenterTests.cs
NotificationCenter.AddNotification(NotificationTypeTrack, notificationCallbackMock.Object.TestTrackCallback);

// Fire decision type notifications.
NotificationCenter.SendNotifications(NotificationTypeActivate, config.GetExperimentFromKey("test_experiment"),
Copy link

Choose a reason for hiding this comment

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

The LogEvent would never be null. So maybe mock it?

Copy link
Contributor

Choose a reason for hiding this comment

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

mocked it. Please check. 0e80f49

@thomaszurkan-optimizely thomaszurkan-optimizely merged commit ecbf657 into optimizely:master Nov 27, 2017
kellyroach-optimizely pushed a commit that referenced this pull request Jan 22, 2018
@mfahadahmed
Eventdispatcherissue (#35)
fdaad27 
Fixed Timing issue with Activate() & Track() being called sequentially
kellyroach-optimizely pushed a commit that referenced this pull request Jan 22, 2018 
Fixed Timing issue with Activate() & Track() being called sequentially
@mfahadahmed mfahadahmed deleted the feature_notification_center branch March 6, 2018 14:09
kellyroach-optimizely added a commit that referenced this pull request Mar 29, 2018
@kellyroach-optimizely
Revert "Eventdispatcherissue (#35) (#54)"
bdaa7be 
This reverts commit 5df7cfb.
kellyroach-optimizely added a commit that referenced this pull request Apr 3, 2018
@kellyroach-optimizely
OASIS-2504 [C#] Release 2.0.0-beta1 (#71)
b62fc68 
* Revert "Eventdispatcherissue (#35) (#54)"

This reverts commit 5df7cfb.

* Bump version to 2.0 beta.

* Update OptimizelySDK.nuspec

* AssemblyVersion = 2.0.0.0

* Improved 1.3.1 DemoApp

* Updated OptimizelySDK.Package and OptimizelySDK.nuspec

* keypair.snk for *.sln, *.csproj

* CHANGELOG.md corrections

* SDK_VERSION synch with NuGet package version.

* Make 'UserAttributes userAttributes = null' optional in 5 API's

* Update verifysn.ps1 and DemoApp.csproj to 2.0.0-beta1
kellyroach-optimizely added a commit that referenced this pull request Apr 3, 2018
@kellyroach-optimizely
OASIS-2504 [C#] Release 2.0.0-beta1 (#71)
617feed 
* Revert "Eventdispatcherissue (#35) (#54)"

This reverts commit 5df7cfb.

* Bump version to 2.0 beta.

* Update OptimizelySDK.nuspec

* AssemblyVersion = 2.0.0.0

* Improved 1.3.1 DemoApp

* Updated OptimizelySDK.Package and OptimizelySDK.nuspec

* keypair.snk for *.sln, *.csproj

* CHANGELOG.md corrections

* SDK_VERSION synch with NuGet package version.

* Make 'UserAttributes userAttributes = null' optional in 5 API's

* Update verifysn.ps1 and DemoApp.csproj to 2.0.0-beta1
@kellyroach-optimizely kellyroach-optimizely mentioned this pull request Apr 3, 2018
Merged
kellyroach-optimizely added a commit that referenced this pull request Apr 6, 2018
@kellyroach-optimizely
OASIS-2504 [C#] Release 2.0.0-beta1 (#72)
0fff35b 
* OASIS-2504 [C#] Release 2.0.0-beta1 (#71)

* Revert "Eventdispatcherissue (#35) (#54)"

This reverts commit 5df7cfb.

* Bump version to 2.0 beta.

* Update OptimizelySDK.nuspec

* AssemblyVersion = 2.0.0.0

* Improved 1.3.1 DemoApp

* Updated OptimizelySDK.Package and OptimizelySDK.nuspec

* keypair.snk for *.sln, *.csproj

* CHANGELOG.md corrections

* SDK_VERSION synch with NuGet package version.

* Make 'UserAttributes userAttributes = null' optional in 5 API's

* Update verifysn.ps1 and DemoApp.csproj to 2.0.0-beta1

* Correct 2.0.0-beta1 release date in CHANGELOG.md
kellyroach-optimizely added a commit that referenced this pull request Jun 20, 2018
@kellyroach-optimizely
2.0.1 (#86)
caa791c 
* GetEnabledFeatures now returns sorted features list. (#69)

* GetEnabledFeatures now returns sorted features list ignoring case

* Removes sorting from GetEnabledFeatures method. (#73)

* OASIS-2504 [C#] Release 2.0.0-beta1 (#72)

* OASIS-2504 [C#] Release 2.0.0-beta1 (#71)

* Revert "Eventdispatcherissue (#35) (#54)"

This reverts commit 5df7cfb.

* Bump version to 2.0 beta.

* Update OptimizelySDK.nuspec

* AssemblyVersion = 2.0.0.0

* Improved 1.3.1 DemoApp

* Updated OptimizelySDK.Package and OptimizelySDK.nuspec

* keypair.snk for *.sln, *.csproj

* CHANGELOG.md corrections

* SDK_VERSION synch with NuGet package version.

* Make 'UserAttributes userAttributes = null' optional in 5 API's

* Update verifysn.ps1 and DemoApp.csproj to 2.0.0-beta1

* Correct 2.0.0-beta1 release date in CHANGELOG.md

* 2.0.0-beta1 --> 2.0.0 (#74) (#75)

* Input validation in Activate, Track and GetVariation methods. (#76)

* Input validation in Activate, Track and GetVariation methods.

* Replaced InputType enum with string constants.

* Updates unit tests.

* Fix impression sent from feature experiment variation toggled off.

* Revert "Fix impression sent from feature experiment variation toggled off."

This reverts commit c943747.

* Fix impression sent from feature experiment variation toggled off. (#84)

* Update OptimizelySDK.DemoApp.csproj OptimizelySDK Reference

* Update AssemblyVersion, AssemblyFileVersion, AssemblyInformationalVersion

* Update the OptimizelySDK.Package/OptimizelySDK.nuspec

* Upgrade DemoApp to OptimizelySDK 2.0.1
kellyroach-optimizely added a commit that referenced this pull request Jun 21, 2018
@kellyroach-optimizely
2.0.1 (#86)
dce781c 
* GetEnabledFeatures now returns sorted features list. (#69)

* GetEnabledFeatures now returns sorted features list ignoring case

* Removes sorting from GetEnabledFeatures method. (#73)

* OASIS-2504 [C#] Release 2.0.0-beta1 (#72)

* OASIS-2504 [C#] Release 2.0.0-beta1 (#71)

* Revert "Eventdispatcherissue (#35) (#54)"

This reverts commit 5df7cfb.

* Bump version to 2.0 beta.

* Update OptimizelySDK.nuspec

* AssemblyVersion = 2.0.0.0

* Improved 1.3.1 DemoApp

* Updated OptimizelySDK.Package and OptimizelySDK.nuspec

* keypair.snk for *.sln, *.csproj

* CHANGELOG.md corrections

* SDK_VERSION synch with NuGet package version.

* Make 'UserAttributes userAttributes = null' optional in 5 API's

* Update verifysn.ps1 and DemoApp.csproj to 2.0.0-beta1

* Correct 2.0.0-beta1 release date in CHANGELOG.md

* 2.0.0-beta1 --> 2.0.0 (#74) (#75)

* Input validation in Activate, Track and GetVariation methods. (#76)

* Input validation in Activate, Track and GetVariation methods.

* Replaced InputType enum with string constants.

* Updates unit tests.

* Fix impression sent from feature experiment variation toggled off.

* Revert "Fix impression sent from feature experiment variation toggled off."

This reverts commit c943747.

* Fix impression sent from feature experiment variation toggled off. (#84)

* Update OptimizelySDK.DemoApp.csproj OptimizelySDK Reference

* Update AssemblyVersion, AssemblyFileVersion, AssemblyInformationalVersion

* Update the OptimizelySDK.Package/OptimizelySDK.nuspec

* Upgrade DemoApp to Optimizely
BDA7
SDK 2.0.1
kellyroach-optimizely added a commit that referenced this pull request Jun 21, 2018
@kellyroach-optimizely
Kroach/oasis 3134 release master (#87)
da13545 
* 2.0.1 (#86)

* GetEnabledFeatures now returns sorted features list. (#69)

* GetEnabledFeatures now returns sorted features list ignoring case

* Removes sorting from GetEnabledFeatures method. (#73)

* OASIS-2504 [C#] Release 2.0.0-beta1 (#72)

* OASIS-2504 [C#] Release 2.0.0-beta1 (#71)

* Revert "Eventdispatcherissue (#35) (#54)"

This reverts commit 5df7cfb.

* Bump version to 2.0 beta.

* Update OptimizelySDK.nuspec

* AssemblyVersion = 2.0.0.0

* Improved 1.3.1 DemoApp

* Updated OptimizelySDK.Package and OptimizelySDK.nuspec

* keypair.snk for *.sln, *.csproj

* CHANGELOG.md corrections

* SDK_VERSION synch with NuGet package version.

* Make 'UserAttributes userAttributes = null' optional in 5 API's

* Update verifysn.ps1 and DemoApp.csproj to 2.0.0-beta1

* Correct 2.0.0-beta1 release date in CHANGELOG.md

* 2.0.0-beta1 --> 2.0.0 (#74) (#75)

* Input validation in Activate, Track and GetVariation methods. (#76)

* Input validation in Activate, Track and GetVariation methods.

* Replaced InputType enum with string constants.

* Updates unit tests.

* Fix impression sent from feature experiment variation toggled off.

* Revert "Fix impression sent from feature experiment variation toggled off."

This reverts commit c943747.

* Fix impression sent from feature experiment variation toggled off. (#84)

* Update OptimizelySDK.DemoApp.csproj OptimizelySDK Reference

* Update AssemblyVersion, AssemblyFileVersion, AssemblyInformationalVersion

* Update the OptimizelySDK.Package/OptimizelySDK.nuspec

* Upgrade DemoApp to OptimizelySDK 2.0.1

* Update verifysn.ps1 to 2.0.1

* Update CHANGELOG.md to OptimizelySDK 2.0.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mikeproeng37 mikeproeng37 mikeproeng37 left review comments

@msohailhussain msohailhussain msohailhussain left review comments

@thomaszurkan-optimizely thomaszurkan-optimizely thomaszurkan-optimizely approved these changes

@kellyroach-optimizely kellyroach-optimizely kellyroach-optimizely approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@mfahadahmed @optibot @kellyroach-optimizely @mikeproeng37 @msohailhussain @thomaszurkan-optimizely @msohailh
0