-
-
Notifications
You must be signed in to change notification settings - Fork 730
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
[Vouchers] Fix tax included in price amount #11593
Changes from 7 commits
8254bd9
8d639c1
1a66f3d
7f2c1fe
74c8f06
f83e78a
f4fde0a
477ca39
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,12 +15,27 @@ | |
helper.validated_input("test", "foo", type: :email) | ||
end | ||
|
||
describe "displaying the tax total for an order" do | ||
let(:order) { double(:order, total_tax: 123.45, currency: 'AUD') } | ||
describe "#display_checkout_tax_total" do | ||
subject(:display_checkout_tax_total) { helper.display_checkout_tax_total(order) } | ||
|
||
let(:order) { instance_double(Spree::Order, total_tax: 123.45, currency: 'AUD') } | ||
let(:service) { instance_double(VoucherAdjustmentsService, voucher_included_tax: ) } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would have set up real data and used the real VoucherAdjustmentsService. What if the method But maybe that's just opinion :) If it's because the test data is complicated to set up, then maybe we need to create a factory for it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
But in general I agree that mocking is to be avoided if possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, I didn't know that! |
||
let(:voucher_included_tax) { 0.0 } | ||
|
||
before do | ||
allow(VoucherAdjustmentsService).to receive(:new).and_return(service) | ||
end | ||
|
||
it "retrieves the total tax on the order" do | ||
expect(helper.display_checkout_tax_total(order)).to eq(Spree::Money.new(123.45, | ||
currency: 'AUD')) | ||
expect(display_checkout_tax_total).to eq(Spree::Money.new(123.45, currency: 'AUD')) | ||
end | ||
|
||
context "with a voucher" do | ||
let(:voucher_included_tax) { -0.45 } | ||
|
||
it "displays the discounted total tax" do | ||
expect(display_checkout_tax_total).to eq(Spree::Money.new(123.00, currency: 'AUD')) | ||
end | ||
end | ||
end | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should
voucher_included_tax_representations(order)
be the last one in the list? It looks like it was before.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought that that was looking odd. It seems clearer to me to put the voucher tax under the voucher.