Add Bugsnag notification if we reach tax rate refund code #12921
Conversation
rioug
added
the
technical changes only
The original Spree code allow for a tax adjustment to be considered a refund in a specific scenario: - instance is using inclusive tax - instance that applies different tax rate in different tax zones This scenario should not happen with how our instances are configured More info: openfoodfoundation#6565 (comment)
rioug
force-pushed
the
12908-error-when-tax-refund
branch
from
7239a64 to
f024aff
Compare
spec/models/spree/tax_rate_spec.rb
Outdated
| expect(Bugsnag).to receive(:notify).with( | ||
| "Notice: Tax refund should not be possible, please check the default zone and " \ | ||
| "the tax rate zone configuration" | ||
| ).twice |
There was a problem hiding this comment.
I recently started adding and_call_original to Bugsnag expectations so that the code is actually run and errors in the metadata block are raised. That's only a theory though. I don't know if it would fail properly. But if my comment above is correct, you have the perfect case to test it.
There was a problem hiding this comment.
Unfortunately and_call_original did not pick up the error. I wouldn't be surprised if they rescue errors in the notify block so they can still report the error even if something is wrong.
| @@ -110,7 +110,7 @@ def compute_amount(item) | |||
| "Notice: Tax refund should not be possible, please check the default zone and " \ | |||
| "the tax rate zone configuration" | |||
| ) do |payload| | |||
| payload.add_metadata :order_tax_zone, order.tax_zone | |||
| payload.add_metadata :order_tax_zone, item.order.tax_zone | |||
| payload.add_metadata :tax_rate_zone, zone | |||
| payload.add_metadata :default_zone, Zone.default_tax | |||
| end | |||
There was a problem hiding this comment.
If we don't support this code path, shouldn't we also raise an application exception, and remove the next couple of lines? Or are we not sure enough about it to change the behaviour just yet..?
Sure, we don't want more snails, but snails are better than hidden problems. Instance managers are not going to be seeing the bugsnag error so won't know if it's happening unless someone notices and tells them.
There was a problem hiding this comment.
Or are we not sure enough about it to change the behaviour just yet..?
My idea was to add Bugsnag to confirm it's not happening in production, I don't feel confident enough yet to remove the code. We should be able to remove the code if we don't see the snails after a month or two.
I am not sure about raising an exception, it would probably block placing an order if it gets triggered. I guess we could remove "the refund" and just to a normal compute, that would avoid the wrong tax rate. What do you think ?
There was a problem hiding this comment.
Thats right, I thought it would be better to block the order, rather than have an order with wrong details.
But I'm not sure I fully understand the context, so am ok with just adding Bugsnag for now, if it helps gain understanding of what's happening.
| @@ -110,7 +110,7 @@ def compute_amount(item) | |||
| "Notice: Tax refund should not be possible, please check the default zone and " \ | |||
| "the tax rate zone configuration" | |||
| ) do |payload| | |||
| payload.add_metadata :order_tax_zone, order.tax_zone | |||
| payload.add_metadata :order_tax_zone, item.order.tax_zone | |||
| payload.add_metadata :tax_rate_zone, zone | |||
| payload.add_metadata :default_zone, Zone.default_tax | |||
| end | |||
There was a problem hiding this comment.
Thats right, I thought it would be better to block the order, rather than have an order with wrong details.
But I'm not sure I fully understand the context, so am ok with just adding Bugsnag for now, if it helps gain understanding of what's happening.
What? Why?
This was discovered when investigating #12908
The original Spree code allow for a tax adjustment to be considered a refund in a specific scenario:
This scenario should not happen with how our instances are configured More info: #6565 (comment)
What should we test?
Green specs
Release notes
Changelog Category (reviewers may add a label for the release notes):
The title of the pull request will be included in the release notes.