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.

Sign up for GitHub

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

Add Bugsnag notification if we reach tax rate refund code #12921

Merged
merged 2 commits into from
Oct 30, 2024
Merged

Add Bugsnag notification if we reach tax rate refund code #12921

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f024aff
Add Bugsnag notification if we reach tax rate refund code
rioug Oct 16, 2024
3f22e8c
Fix Bugsnag payload data
rioug Oct 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/models/spree/tax_rate.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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 ?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Expand Down
2 changes: 1 addition & 1 deletion spec/models/spree/tax_rate_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -392,7 +392,7 @@ module Spree
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
).twice.and_call_original

Spree::TaxRate.adjust(order, order.line_items)
end
Expand Down
Loading