Skip to content
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

bot filtering #79

Merged
merged 7 commits into from
Jun 27, 2018
Merged

bot filtering #79

merged 7 commits into from
Jun 27, 2018

Conversation

msohailhussain
Copy link
Contributor

No description provided.

@msohailhussain
Copy link
Contributor Author

build

1 similar comment
@msohailhussain
Copy link
Contributor Author

build

aliabbasrizvi
aliabbasrizvi suggested changes Jun 12, 2018
View reviewed changes
Copy link

@aliabbasrizvi aliabbasrizvi left a comment

Choose a reason for hiding this comment

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

Mostly looks good. Some changes needed.

OptimizelySDK.Tests/EventTests/EventBuilderTest.cs
@@ -1,3 +1,19 @@
/*
* Copyright 2017-2018, Optimizely
Copy link

Choose a reason for hiding this comment

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

Thanks for adding this.

OptimizelySDK.Tests/EventTests/EventBuilderTest.cs Outdated
@@ -16,7 +32,7 @@ public class EventBuilderTest
private string TestUserId = string.Empty;
private ProjectConfig Config;
private EventBuilder EventBuilder;

Copy link

Choose a reason for hiding this comment

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

nit. Seems unnecessary spaces got put in there. Lets undo.

OptimizelySDK/Logger/ILogger.cs Outdated
@@ -20,6 +20,7 @@ public enum LogLevel
DEBUG,
INFO,
ERROR,
WARN,
Copy link

Choose a reason for hiding this comment

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

WARN should go above ERROR IMO

Copy link

Choose a reason for hiding this comment

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

Also, update year

OptimizelySDK/ProjectConfig.cs
/// </summary>
/// <param name="attributeKey">Key of the Attribute</param>
/// <returns>Attribute ID corresponding to the provided attribute key. Attribute key if it is a reserved attribute</returns>
public string GetAttributeId(string attributeKey)
Copy link

Choose a reason for hiding this comment

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

Please add unit tests for this new module

Copy link
Contributor

@mfahadahmed mfahadahmed Jun 13, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Already covered here:

csharp-sdk/OptimizelySDK.Tests/ProjectConfigTest.cs

Line 836 in 3bd25d5

public void TestGetAttributeIdWithReservedPrefix()

csharp-sdk/OptimizelySDK.Tests/ProjectConfigTest.cs

Line 859 in 3bd25d5

public void TestGetAttributeIdWithInvalidAttributeKey()

Copy link

Choose a reason for hiding this comment

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

Oh, good. GitHub does not render large files by default and hence missed it.

OptimizelySDK.Tests/TestData.json Outdated
@@ -634,5 +634,6 @@
"key": "purchase"
}],
"revision": "15",
"anonymizeIP": "false"
"anonymizeIP": "false",
Copy link

Choose a reason for hiding this comment

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

This will always be a boolean value

OptimizelySDK.Tests/TestData.json Outdated
@@ -634,5 +634,6 @@
"key": "purchase"
}],
"revision": "15",
"anonymizeIP": "false"
"anonymizeIP": "false",
"botFiltering": "true"
Copy link

Choose a reason for hiding this comment

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

This will always be a boolean value.

OptimizelySDK.Tests/EventTests/EventBuilderTest.cs Outdated
{"account_id", "1592310167" },
{"client_name", "csharp-sdk" },
{"client_version", Optimizely.SDK_VERSION },
{"revision", 15 },
Copy link

Choose a reason for hiding this comment

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

Can you point me to where revision is set? It should be what it is in the datafile i.e. it should be string.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://github.com/optimizely/csharp-sdk/blob/master/OptimizelySDK/ProjectConfig.cs#L31
https://github.com/optimizely/csharp-sdk/blob/master/OptimizelySDK/ProjectConfig.cs#L266
We are deserializing it and it is set automatically.

Copy link

Choose a reason for hiding this comment

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

Bot revision and version need to be string and not int. Can you please fix that as well.

Thanks.

aliabbasrizvi
aliabbasrizvi suggested changes Jun 26, 2018
View reviewed changes
Copy link

@aliabbasrizvi aliabbasrizvi left a comment

Choose a reason for hiding this comment

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

Almost there. Last set of small changes needed.

OptimizelySDK/Event/Builder/EventBuilder.cs Outdated
public const string RESERVED_ATTRIBUTE_KEY_BUCKETING_ID_EVENT_PARAM_KEY = "optimizely_bucketing_id";
public const string BOT_FILTERING_ATTRIBUTE = "$opt_bot_filtering";

public const string USER_AGENT_ATTRIBUTE = "$opt_user_agent";
Copy link

Choose a reason for hiding this comment

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

nit. Like in other SDKs, lets consolidate all of the "special" attributes i.e. bucketing ID, bot filtering and user agent attributes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Done.

OptimizelySDK/ProjectConfig.cs
/// </summary>
/// <param name="attributeKey">Key of the Attribute</param>
/// <returns>Attribute ID corresponding to the provided attribute key. Attribute key if it is a reserved attribute</returns>
public string GetAttributeId(string attributeKey)
Copy link

Choose a reason for hiding this comment

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

Oh, good. GitHub does not render large files by default and hence missed it.

OptimizelySDK.Tests/ProjectConfigTest.cs Outdated
Assert.AreEqual(Config.GetAttributeId(EventBuilder.USER_AGENT_ATTRIBUTE), EventBuilder.USER_AGENT_ATTRIBUTE);

// Verify that attribute Id is returned for attribute key with reserved prefix that does not exist in datafile.
Assert.AreEqual(Config.GetAttributeId(EventBuilder.USER_AGENT_ATTRIBUTE), EventBuilder.USER_AGENT_ATTRIBUTE);
Copy link

Choose a reason for hiding this comment

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

nit. This test is no different from the previous test. Let's have a separate example. Like lets test for $opt_reserved_prefix_attribute

OptimizelySDK.Tests/TestData.json Outdated
@@ -634,5 +634,6 @@
"key": "purchase"
}],
"revision": "15",
"anonymizeIP": "false"
"anonymizeIP": false,
"botFiltering": true
}
Copy link

Choose a reason for hiding this comment

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

nit. Add new line at end of file.

OptimizelySDK/ProjectConfig.cs
return attributeKey;
}

Logger.Log(LogLevel.ERROR, $@"Attribute key ""{attributeKey}"" is not in datafile.");
Copy link

Choose a reason for hiding this comment

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

nit. Just single pair of double quotes should be good right?

Copy link
Contributor

Choose a reason for hiding this comment

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

In verbatim strings, 2 double-quotes are used as an escape character like double backslashes in normal strings. The final string will contain attribute key in single double-quotes.

@mfahadahmed mfahadahmed self-assigned this Jun 27, 2018
aliabbasrizvi
aliabbasrizvi reviewed Jun 27, 2018
View reviewed changes
OptimizelySDK/Utils/ReservedAttribute.cs Outdated

namespace OptimizelySDK.Utils
{
public class ReservedAttribute
Copy link

Choose a reason for hiding this comment

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

nit. Call this ControlAttribute like in other SDKs

aliabbasrizvi
aliabbasrizvi approved these changes Jun 27, 2018
View reviewed changes
Copy link

@aliabbasrizvi aliabbasrizvi left a comment

Choose a reason for hiding this comment

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

Small changes needed. LGTM.

OptimizelySDK/Utils/ReservedAttribute.cs Outdated
{
public class ReservedAttribute
{
public const string BOT_FILTERING_ATTRIBUTE = "$opt_bot_filtering";
Copy link

Choose a reason for hiding this comment

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

nit. Alphabetize.

@aliabbasrizvi aliabbasrizvi merged commit 5939e2b into master Jun 27, 2018
@aliabbasrizvi aliabbasrizvi deleted the sohail/botfiltering branch June 27, 2018 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mfahadahmed mfahadahmed mfahadahmed left review comments

@aliabbasrizvi aliabbasrizvi aliabbasrizvi approved these changes

Assignees

@mfahadahmed mfahadahmed

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@msohailhussain @aliabbasrizvi @mfahadahmed