Skip to content
This repository has been archived by the owner on Sep 12, 2019. It is now read-only.

Set pre_tax_amount on line items and shipments when generating sales invoice #6

Merged
merged 3 commits into from
Aug 10, 2015
Merged

Set pre_tax_amount on line items and shipments when generating sales invoice #6

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6e7d270
Set pre_tax_amount on line items when generating sales invoice
Aug 7, 2015
d5c7361
Round discounted_amount by 2
Aug 7, 2015
c654508
Set the pre_tax_amount on the order’s shipments
richardnuno Aug 10, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions app/models/spree_avatax/sales_invoice.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@ def generate(order)

return if order.completed? || !SpreeAvatax::Shared.taxable_order?(order)

taxable_records = order.line_items + order.shipments
taxable_records.each do |taxable_record|
taxable_record.update_column(:pre_tax_amount, taxable_record.discounted_amount.round(2))
end
Copy link
Member

Choose a reason for hiding this comment

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

I'm a little freaked out by this. Invoice generation is causing line item amount and shipment to be updated. That's not the sort of behaviour that I would expect when generating and invoice.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gmacdougall it does a whole bunch more than this:

solidus_avatax/app/models/spree_avatax/sales_shared.rb

Lines 32 to 52 in 647b1bf

record.update_column(:pre_tax_amount, record.discounted_amount)
tax = BigDecimal.new(tax_line[:tax]).abs
record.adjustments.tax.create!({
adjustable: record,
amount: tax,
order: order,
label: Spree.t(:avatax_label),
included: false, # would be true for VAT
source: Spree::TaxRate.avatax_the_one_rate,
state: 'closed', # this tells spree not to automatically recalculate avatax tax adjustments
})
Spree::ItemAdjustments.new(record).update
record.save!
end
Spree::OrderUpdater.new(order).update
order.save!

Are you saying we should have more comments on the generate method? Or that a more descriptive name would be nice? Or that you wish we had a better way to integrate w/ taxes in Solidus? Or something else?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gmacdougall this was already happening later on, this is just to ensure that the values are still set in case the invoice cannot be generated for any reason (e.g. Avatax network timeout). Storing the pre_tax_amount is even overriden given we handle it at a different time as the comment mentions.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure where the responsibility should live, but I was taken aback by an invoice generator updating records on the thing it is generating an invoice for.

It might not be feasible to fix now, but I does seem a bit strange to me.

Copy link
Contributor

Choose a reason for hiding this comment

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

That seems like a reasonable concern and, all things considered, I think that falls under the "bad name" category because I think the most pragmatic way to make that better would be to rename this method calculate_and_set_taxes (or something like that) and deprecate the name generate.

But in any case this PR isn't a significant change as far as how it mutates the order so how about we open an issue or a separate PR around that.

Copy link
Member

Choose a reason for hiding this comment

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

Fair enough. I didn't have a lot of context around this, so it just seemed a bit odd to me on first glance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @gmacdougall. @jordan-brough Could you take care of opening up the issue and merging this in please?

Copy link
Contributor

Choose a reason for hiding this comment

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

@richardnuno sure, here's the issue: #7


result = SpreeAvatax::SalesShared.get_tax(order, DOC_TYPE)
# run this immediately to ensure that everything matches up before modifying the database
tax_line_data = SpreeAvatax::SalesShared.build_tax_line_data(order, result)
Expand Down
27 changes: 27 additions & 0 deletions spec/models/spree_avatax/sales_invoice_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,11 +181,38 @@
context 'when an error occurs' do
let(:error) { StandardError.new('just testing') }
let!(:gettax_stub) { }
let(:order) do
create(:order_with_line_items,
line_items_count: 2,
ship_address: create(:address, {
address1: "1234 Way",
address2: "",
city: "New York",
state: state,
country: country,
zipcode: "10010",
phone: "1111111111",
}))
end

before do
expect(SpreeAvatax::SalesShared)
.to receive(:get_tax)
.and_raise(error)

order.line_items.update_all(pre_tax_amount: nil)
order.reload
end

it 'sets the pre_tax_amount on each line item in the order' do
expect{ subject }.to raise_error(error)
expect(order.line_items.first.pre_tax_amount.to_f).to equal(order.line_items.first.discounted_amount.to_f)
expect(order.line_items.last.pre_tax_amount.to_f).to equal(order.line_items.last.discounted_amount.to_f)
end

it 'sets the pre_tax_amount on each shipment in the order' do
expect{ subject }.to raise_error(error)
expect(order.shipments.first.pre_tax_amount.to_f).to equal(order.shipments.first.discounted_amount.to_f)
end

context 'when an error_handler is not defined' do
Expand Down