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

Bad requests are being swallowed #126

Open
ericsaupe opened this issue Jun 10, 2019 · 5 comments

Comments

Projects
None yet
2 participants
@ericsaupe
Copy link
Contributor

commented Jun 10, 2019

https://github.com/boomerdigital/solidus_avatax_certified/blob/master/app/models/spree/avalara_transaction.rb#L63

If the response is bad there is no way of knowing that anything went wrong. I think returning the 0 taxes is a good move for the user but makes debugging and fixing issues difficult for the developer.

@dhonig

This comment has been minimized.

Copy link

commented Jun 10, 2019

@ericsaupe yes....This one bothers me. Yet I think its hard to think of a good fallback strategy that suits everyone. Do you have any reccomendations for what the service should do? Maybe just write the error to the logs? In production you might wish to calculating a sensible default, consult a tax table for certain states or in fact return 0 and eat the tax rather than lose the sale. Interested to hear what your team is doing and then perhaps we can get @acreilly to introduce some improvements here.

@ericsaupe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Something like this would be fine, I'm open to changing the logging text. I just need some way of seeing that something happened in a log file or anywhere I can check and monitor for issues and resolve them quickly.

if response.error?
  logger.error "Failed tax calculation #{response.result}"
  return { 'totalTax' => 0.0 }
end
@dhonig

This comment has been minimized.

Copy link

commented Jun 10, 2019

@ericsaupe at one point we used to generate an avatax log. @acreilly can tell you what happened to this. Feel free to send a PR or @acreilly will tackle this on her next cycle of maintenance. Thanks for reporting!

@ericsaupe

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

The avatax log is still there, the code above would use that afaik

@dhonig

This comment has been minimized.

Copy link

commented Jun 10, 2019

K: couldn't remember if we removed that log. thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.