Introduce Bot Filtering #106
Conversation
lib/optimizely/event_builder.rb
Outdated
| visitor_attributes.push(feature) | ||
| end | ||
| # Append Bot Filtering Attribute | ||
| if bot_filtering.is_a?(TrueClass) || bot_filtering.is_a?(FalseClass) |
There was a problem hiding this comment.
I think we can directly check if it is true or false. No value in doing class comparison.
lib/optimizely/project_config.rb
Outdated
| end | ||
| return attribute['id'] | ||
| end | ||
| return attribute_key if has_reserved_prefix && attribute_key != Helpers::Constants::CONTROL_ATTRIBUTES['BOT_FILTERING'] |
There was a problem hiding this comment.
nit. Like in other SDKs we can omit the special check for bot filtering.
spec/event_builder_spec.rb
Outdated
| key: 'browser_type', | ||
| type: 'custom', | ||
| value: 'firefox' | ||
| }, entity_id: Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'], |
There was a problem hiding this comment.
Styling seems incorrect. Too many spaces.
There was a problem hiding this comment.
Comment not addressed
| expect(spy_logger).to have_received(:log).with(Logger::WARN, | ||
| "Attribute '$opt_bot' unexpectedly has reserved prefix '$opt_'; using attribute ID instead of reserved attribute name.") | ||
| end | ||
| it 'should return attribute ID when provided attribute key is valid' do |
There was a problem hiding this comment.
Please put a new line between each of these individual tests.
lib/optimizely/project_config.rb
Outdated
| end | ||
| return attribute['id'] | ||
| end | ||
| return attribute_key if has_reserved_prefix && attribute_key != Helpers::Constants::CONTROL_ATTRIBUTES['BOT_FILTERING'] |
There was a problem hiding this comment.
Like in other SDKs, requesting to not special case bot filtering.
|
@aliabbasrizvi Please review the changes. |
aliabbasrizvi
left a comment
aliabbasrizvi
left a comment
There was a problem hiding this comment.
Need fix for unit test.
spec/event_builder_spec.rb
Outdated
| key: 'browser_type', | ||
| type: 'custom', | ||
| value: 'firefox' | ||
| }, entity_id: Optimizely::Helpers::Constants::CONTROL_ATTRIBUTES['BUCKETING_ID'], |
There was a problem hiding this comment.
Comment not addressed
| # with right params when user agent attribute is provided and | ||
| # bot filtering is enabled | ||
|
|
||
| @expected_impression_params[:visitors][0][:attributes].unshift( |
There was a problem hiding this comment.
Does not have bot filtering param listed. Please update test.
|
@rashidsp can you resolve conflicts? |
aliabbasrizvi
left a comment
aliabbasrizvi
left a comment
There was a problem hiding this comment.
LGTM
Please resolve merge conflicts.
|
Looks good. |
No description provided.