Skip to content

Commit

Permalink
Fintechqb 1967 enable service level logging (#600)
Browse files Browse the repository at this point in the history
* Add total_tax_specified xml_accessor to TransactionTaxDetail

In a recent update to the QBO API, if a company that uses automated sales tax has an invoice that has no tax amount, we need to specify TotalTaxSpecified = false. Not passing this results in an error from QBO indicating that sales tax was miscalculated.

* Accidentally commited a binding.pry

* Add condense_logs to quickbooks config & update logs to only log once per request and once per response if enabled

* Update variable names & move condense_logs? checks to logging util

* Update Quickbooks.log to log without condition & move condition to logging utility to allow per-service-instance logging capability

* Update README to include instance configurability demo

* Update log settings check to check for ivar definition instead of just memoizing to handle @log explicitly being set to false for an instance

---------

Co-authored-by: Daniel Ho <[email protected]>
  • Loading branch information
andymond and danielnho22 authored Jun 20, 2023
1 parent dd300ed commit 6d6422d
Show file tree
Hide file tree
Showing 4 changed files with 72 additions and 6 deletions.
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -758,9 +758,17 @@ JSON]( https://github.com/ruckus/quickbooks-ruby/issues/257#issuecomment-1268344

## Logging

Set the default log enablement:
```ruby
Quickbooks.log = true
```

Configure a service instance to log (or not) independently:
```ruby
customer_service = Quickbooks::Service::Customer.new
customer_service.log = true
```

By default, logging is directed at STDOUT, but another target may be defined, e.g. in Rails
```ruby
Quickbooks.logger = Rails.logger
Expand Down
6 changes: 2 additions & 4 deletions lib/quickbooks-ruby.rb
Original file line number Diff line number Diff line change
Expand Up @@ -248,10 +248,8 @@ def log_xml_pretty_print?
end

def log(msg)
if log?
logger.info(msg)
logger.flush if logger.respond_to?(:flush)
end
logger.info(msg)
logger.flush if logger.respond_to?(:flush)
end
end # << self

Expand Down
6 changes: 4 additions & 2 deletions lib/quickbooks/util/logging.rb
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
module Quickbooks
module Util
module Logging
attr_writer :log

def log(msg)
::Quickbooks.log(msg)
::Quickbooks.log(msg) if log?
end

def log_multiple(messages)
Expand All @@ -14,7 +16,7 @@ def log_multiple(messages)
end

def log?
::Quickbooks.log?
defined?(@log) ? @log : ::Quickbooks.log?
end

def condense_logs?
Expand Down
58 changes: 58 additions & 0 deletions spec/lib/quickbooks/util/logging_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
describe Quickbooks::Util::Logging do
before { Quickbooks.log = general_log_setting }
let(:general_log_setting) { true }
let(:dummy_class) { Class.new { include Quickbooks::Util::Logging } }

describe 'log' do
context 'when one service needs to log but general logging is disabled' do
let(:general_log_setting) { false }
let(:control_instance) { instance = dummy_class.new }
let(:loggable_instance) do
instance = dummy_class.new
instance.log = true
instance
end

it 'allows one service instance to log without affecting other service instances' do
expect(Quickbooks.log?).to be(false)
expect(control_instance.log?).to be(false)
expect(loggable_instance.log?).to be(true)
end

it 'does not log if disabled' do
expect(Quickbooks).not_to receive(:log)
control_instance.log('test message')
end

it 'does log if enabled' do
expect(Quickbooks).to receive(:log)
loggable_instance.log('test message')
end
end

context 'when one service needs to not log but general logging is enabled' do
let(:control_instance) { instance = dummy_class.new }
let(:unloggable_instance) do
instance = dummy_class.new
instance.log = false
instance
end

it 'allows one service instance to log without affecting other service instances' do
expect(Quickbooks.log?).to be(true)
expect(control_instance.log?).to be(true)
expect(unloggable_instance.log?).to be(false)
end

it 'does not log if disabled' do
expect(Quickbooks).not_to receive(:log)
unloggable_instance.log('test message')
end

it 'does log if enabled' do
expect(Quickbooks).to receive(:log)
control_instance.log('test message')
end
end
end
end

6 comments on commit 6d6422d

@andymond
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruckus hey there!

Do you plan to bump the 1.x.x version to make new additions available?

Is 2-stable going to continue to merge functionality changes from master, or will it be merged to master in the nearish future?

Thanks!
Andy

@ruckus
Copy link
Owner

@ruckus ruckus commented on 6d6422d Jul 12, 2023

Choose a reason for hiding this comment

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

Hi @andymond

I just released both 2.0.3 and 1.0.22.

The whole thing is kind of a mess and supporting both branches is not a long term solution. Maybe for sanity sake I will need to break out 2-stable into its own gem, like quickbooks-ruby-nextgen or whatever. Horrible name, but you get the idea.

Too bad there was no ChatGPT assistant to manage all of this 🥲

@ashkulz
Copy link

Choose a reason for hiding this comment

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

@ruckus I can't find any commits on 2-stable and it looks like this was the only change merged since then.

Maybe you forgot to push the updates to the branch?

@andymond
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ruckus thanks for doing that!

I guess I would think these breaking changes would be OK on master since it's a major version upgrade, projects using the gem w/o a version lock should have some kind of protection via tests that would catch this before it sets anything on fire. I know our project would be OK w quickbooks-ruby only supporting 2.6+!

@ruckus
Copy link
Owner

@ruckus ruckus commented on 6d6422d Jul 13, 2023

Choose a reason for hiding this comment

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

Hi @andymond you're right, I did forget to push to 2-stable 🤦 . I just did

@ashkulz
Copy link

Choose a reason for hiding this comment

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

@ruckus you got the version numbers wrong in the commit message and changelog, it should be 2.0.3 instead of 2.0.2.

Please sign in to comment.