minimul / qbo_api

Ruby JSON-only client for QuickBooks Online API v3. Built on top of the Faraday gem.
MIT License
85 stars 45 forks source link

Clean up application code with some simple QboApi::Error changes #136

Closed mculp closed 2 months ago

mculp commented 3 months ago

hey @minimul, we’re working on an issue where there are certain exception types and error code combinations that we want to prompt the user to reauthenticate, and we noticed a couple things that we think would improve the gem and application code using the gem.

Proposal 1 - Remove possibility for fault to be nil

I think if we made a few changes to the QboApi::Error class, that would make application code a lot cleaner.

For example, currently fault can be nil on an QboApi::Error instance, which seems like an extreme edge case, but still one we have to handle if we want to check error codes.

Can it or should it really ever happen?

What if at the very least, it was an empty hash?

class QboApi
  class Error < StandardError
    attr_reader :fault

-    def initialize(errors = nil)
+    def initialize(errors = {})
-      if errors
+      @fault = errors
+      super(errors[:error_body]) 
-      end
    end
  end

  # Exception Definitions
  # ...
end

Actually, as I’m typing this I’m realizing in the current code, if errors is ever nil, then I believe the super(errors[:error_body]) line would raise a no method found on nil exception when it tries to access nil[:error_body]. So this could actually be a preemptive bug fix.

Proposal 2 - Add helper methods for each key/value pair in the QBO error response

Add methods for each key in the QBO response:

  1. fault_type
  2. error_code
  3. error_message
  4. error_detail
class QboApi
  class Error < StandardError
    attr_reader :fault

    def initialize(errors = {})
      @fault = errors
      super(errors[:error_body])
    end

    def fault_type
      fault.dig(:error_body, 0, :fault_type)
    end

    def error_code
      fault.dig(:error_body, 0, :error_code)
    end

    def error_message
      fault.dig(:error_body, 0, :error_message)
    end

    def error_detail
      fault.dig(:error_body, 0, :error_detail)
    end
  end

  # Exception Definitions
  # ...
end

Then application code could be as simple as:

rescue QboApi::BadRequest => e
  if e.error_code.in?([5020, 6190])
    # ask customer to reauthenticate
  end
end

If you think this is a good idea, we’ll whip up a PR.

Thank you!

cc @dudleysr @ahlau

minimul commented 2 months ago

@mculp

Both proposals sound great and I'll merge a PR around them.

The only concern I'd have in advance is not having any breaking changes.