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.

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

EventBuilder uses the same logger as Project #122

Merged
merged 1 commit into from
Sep 19, 2018

Conversation

bcobb
Copy link
Contributor

@bcobb bcobb commented Aug 28, 2018

👋 This recently caused a bit of noise for us 😅 after upgrading our SDK. It looks like Optimizely::Project is intended to work without a custom logger, but tracking events with tags started raising NoMethodError:

$GEM_HOME/optimizely-sdk-2.1.0/lib/optimizely/helpers/event_tag_utils.rb:52:in `get_revenue_value': undefined method `log' for nil:NilClass (NoMethodError)
    from $GEM_HOME/optimizely-sdk-2.1.0/lib/optimizely/event_builder.rb:220:in `block in get_conversion_params'
    from $GEM_HOME/optimizely-sdk-2.1.0/lib/optimizely/event_builder.rb:202:in `each'
    from $GEM_HOME/optimizely-sdk-2.1.0/lib/optimizely/event_builder.rb:202:in `get_conversion_params'
    from $GEM_HOME/optimizely-sdk-2.1.0/lib/optimizely/event_builder.rb:155:in `create_conversion_event'
    from $GEM_HOME/optimizely-sdk-2.1.0/lib/optimizely.rb:230:in `track'

  • Previously, EventBuilder was instantiated by Project with the
    value of the logger local. However, when Project was not
    instantiated with a custom logger, EventBuilder would be
    instantiated with nil logger, which would cause track to raise a
    NoMethodError when called with tags.
  • This commit changes the Project constructor to pass the @logger
    instance variable to EventBuilder, so that they are using the same
    logger, custom or otherwise.
  • The added spec fails prior to the constructor change

@optibot
Copy link

optibot commented Aug 28, 2018

Can one of the admins verify this patch?

1 similar comment
@optibot
Copy link

optibot commented Aug 28, 2018

Can one of the admins verify this patch?

@aliabbasrizvi
Copy link
Contributor

@bcobb thank you for sending this in and taking care of signing the CLA. Will take a look at this and get back to you.

- Previously, `EventBuilder` was instantiated by `Project` with the
  value of the `logger` local. However, when `Project` was not
  instantiated with a custom `logger`, `EventBuilder` would be
  instantiated with `nil` logger, which would cause `track` to raise a
  `NoMethodError` when called with tags.
- This commit changes the `Project` constructor to pass the `@logger`
  instance variable to `EventBuilder`, so that they are using the same
  logger, custom or otherwise.
- The added spec fails prior to the constructor change
@bcobb bcobb force-pushed the fix-event-builder-logger branch from 6216f99 to aeea680 Compare August 29, 2018 18:43
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling aeea680 on harrystech:fix-event-builder-logger into ffb910c on optimizely:master.

Copy link
Contributor

@mikeproeng37 mikeproeng37 left a comment

Choose a reason for hiding this comment

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

lgtm

@mikeproeng37
Copy link
Contributor

PR fails tests, but after merging master into the locally checked out branch, tests were passing again as evidenced in #126. Going to merge in.

@mikeproeng37 mikeproeng37 merged commit d638d20 into optimizely:master Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants