8000 feat: Added ODPManager implementation by mikechu-optimizely · Pull Request #322 · optimizely/csharp-sdk · GitHub
[go: up one dir, main page]
Skip to content

feat: Added ODPManager implementation #322

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

Merged
merged 18 commits into from
Dec 7, 2022
Merged

feat: Added ODPManager implementation #322

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
dd028bf
Initial ODP Manager
mikechu-optimizely Nov 23, 2022
a317407
Outline unit tests
mikechu-optimizely Nov 23, 2022
443164c
WIP Unit tests + changes to CUD
mikechu-optimizely Nov 28, 2022
faea784
Implement fetch, identify, and send events with Builder pattern
mikechu-optimizely Nov 28, 2022
191a6ec
WIP Fixing final unit test
mikechu-optimizely Nov 28, 2022
f6276bc
Finish unit tests
mikechu-optimizely Nov 29, 2022
4f629fa
Add documentation
mikechu-optimizely Nov 29, 2022
bbf8ac1
Lint fixes
mikechu-optimizely Nov 29, 2022
4a54b4c
Pull request code revisions + additional unit tests
mikechu-optimizely Dec 1, 2022
0995694
Lint fixes
mikechu-optimizely Dec 1, 2022
88b6684
Change to use string array over List
mikechu-optimizely Dec 1, 2022
29514ff
Clean usings
mikechu-optimizely Dec 1, 2022
22b04ae
Add new OdpConfig test
mikechu-optimizely Dec 1, 2022
dcafcee
Small refactors
mikechu-optimizely Dec 1, 2022
8747556
Pull request code review changes
mikechu-optimizely Dec 2, 2022
dfaa27b
Code review changes
mikechu-optimizely Dec 6, 2022
c528dce
Mark _odpConfig as volatile
mikechu-optimizely Dec 6, 2022
a5b28e6
Return `volatile` denotation
mikechu-optimizely Dec 7, 2022
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
Prev Previous commit
Next Next commit
Finish unit tests
  • Loading branch information
@mikechu-optimizely
mikechu-optimizely committed Nov 29, 2022
commit f6276bc9a88aaed5c8e45ea740737af9aae3057a
6 changes: 4 additions & 2 deletions OptimizelySDK.Tests/OdpTests/OdpManagerTest.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -210,14 +210,16 @@ public void ShouldDisableOdpThroughConfiguration()
_mockLogger.Verify(l =>
l.Log(LogLevel.DEBUG, "ODP event not dispatched (ODP not integrated)."), Times.Never);

_mockOdpEventManager.ResetCalls();
_mockLogger.ResetCalls();

manager.UpdateSettings(string.Empty, string.Empty, _emptySegmentsToCheck);

manager.SendEvent(TEST_EVENT_TYPE, TEST_EVENT_ACTION, _testEventIdentifiers,
_testEventData);
manager.Close();

// still only once
_mockOdpEventManager.Verify(e => e.SendEvent(It.IsAny<OdpEvent>()), Times.Once);
_mockOdpEventManager.Verify(e => e.SendEvent(It.IsAny<OdpEvent>()), Times.Never);
_mockLogger.Verify(l =>
l.Log(LogLevel.DEBUG, "ODP event not dispatched (ODP disabled)."), Times.Once);
_mockLogger.Verify(l =>
Expand Down
6 changes: 3 additions & 3 deletions OptimizelySDK/Odp/OdpManager.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ public bool UpdateSettings(string apiKey, string apiHost, List<string> segmentsT

public List<string> FetchQualifiedSegments(string userId, List<OdpSegmentOption> options)
{
if (!_enabled || SegmentManager == null)
if (!_enabled || SegmentManager == null || !_odpConfig.IsReady())
Copy link
Contributor

Choose a reason for hiding this comment

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

a minor comment,
SegmentManager == null || !_enabled || !_odpConfig.IsReady()

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 can re-arrange (I think that's what you're asking)

I thought that perhaps !_enabled would evaluate to true more often than the others, but happy to switch.

{
_logger.Log(LogLevel.ERROR, Constants.ODP_NOT_ENABLED_MESSAGE);
return null;
Expand All @@ -66,7 +66,7 @@ public List<string> FetchQualifiedSegments(string userId, List<OdpSegmentOption>

public void IdentifyUser(string userId)
{
if (!_enabled || EventManager == null)
if (!_enabled || EventManager == null || !_odpConfig.IsReady())
{
_logger.Log(LogLevel.DEBUG, "ODP identify event not dispatched (ODP disabled).");
return;
Expand All @@ -86,7 +86,7 @@ public void SendEvent(string type, string action, Dictionary<string, string> ide
Dictionary<string, object> data
)
{
if (!_enabled || EventManager == null)
if (!_enabled || EventManager == null || !_odpConfig.IsReady())
Copy link
Contributor

Choose a reason for hiding this comment

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

This validations should be in a single method and return true or false. please refactor it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My copy-paste finger started hurting, and I thought of that too.

L89, would have me injecting the Manager that should be checked for null so I just left it.

Let me play with some options....

{
_logger.Log(LogLevel.DEBUG, "ODP event not dispatched (ODP disabled).");
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
_logger.Log(LogLevel.DEBUG, "ODP event not dispatched (ODP disabled).");
_logger.Log(LogLevel.ERROR, "ODP event not dispatched (ODP disabled).");

Implicit event sending (IdentifyUser) should be DEBUG level, but when clients try to send events when ODP is not ready, it should be ERROR.

return;
Expand Down
0