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

Use gross amount in return items #706

Merged
merged 1 commit into from Feb 26, 2016
Merged

Use gross amount in return items #706

merged 1 commit into from Feb 26, 2016

Conversation

mamhoff
Copy link
Contributor

@mamhoff mamhoff commented Jan 17, 2016

I believe the notion of pre_tax_amount is only present in the return items because
someone from a sales tax country wanted to use something excluding sales (additional) taxes.

This leads to unfortunate behaviour in VAT stores: While every other price or amount
is entered including VAT, for returns a store admin has to enter the return amount
excluding VAT. For most normal people, that's a complex, error-prone op, and one we
shouldn't make them do.

This PR replaces the pre_tax_amount with one called amount that behaves like the amount on line items: including included taxes, but excluding sales taxes.

Unfortunately, this entails a lot of naming changes.

This ports spree/spree#6852 to Solidus.

It's one large commit because all the naming changes affects everything :(

@mamhoff mamhoff changed the title WIP: Use gross amount in return items Use gross amount in return items Jan 17, 2016
@jhawthorn
Copy link
Contributor

I believe the notion of pre_tax_amount is only present in the return items because
someone from a sales tax country wanted to use something excluding sales (additional) taxes.

I believe the opposite to be true, pre_tax_amount was introduced purely for VAT countries (whether it's appropriate or helpful there is another discussion). When applying taxes without included rates pre_tax_amount == amount.

I'm 👍 and this looks good, but we'll need to be very clear in our release notes about what the behaviour change is as it will affect admin end-users.

@mamhoff
Copy link
Contributor Author

mamhoff commented Jan 19, 2016

The reason I believe that pre_tax_amount was introduced from a sales tax country perspective is that the form field for entering the data was labeled pre_tax_amount, indicating that the admin user should enter the reimbursement excluding sales tax, as sales tax would be added later. In a VAT country, I can not imagine the admin user actually wanting to do essentially this calculation in her head:

I want to reimburse 10 Euros. 
My local VAT rate is 19%. 
10 / 1.19 -->  8.40 
OK, I enter 8.40 in the form field. 

What about this as a release notes approach:

When creating a refund for an order including VAT, you now have to enter the amount you want to refund including VAT. 

The form field and the permitted attribute for the return item amount changes from `:pre_tax_amount` to :amount`. 

@cbrunsdon cbrunsdon mentioned this pull request Jan 25, 2016
execute(<<-SQL)
UPDATE spree_return_items
SET pre_tax_amount = pre_tax_amount + included_tax_total;
SQL
Copy link
Contributor

Choose a reason for hiding this comment

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

do we need to be worried about included_tax_total ever being nil? I'd had to nullify pre_tax_amount from included_tax_total being nil. Should we switch to WHERE pre_tax_amount is null to be safe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a safeguard clause that sets included_tax_total to 0 if NULL. The pre_tax_amount/ amount column has a NOT NULL db constraint set, so no worries on that side of the plus.

@cbrunsdon
Copy link
Contributor

I'm 👍 on this, sorry, missed the feedback to my migration question. Hope the rebase isn't too brutal... cough

I believe the notion of `pre_tax_amount` is only in the return items because
someone from a sales tax country wanted to use something with additional taxes.

This leads to unfortunate behaviour in VAT stores: While every other price or amount
is entered including VAT, for returns a store admin has to enter the return amount
excluding VAT. For most normal people, that's a complex, error-prone op, and one we
shouldn't make them do.

Unfortunately, this entails a lot of naming changes.
jhawthorn added a commit that referenced this pull request Feb 26, 2016
@jhawthorn jhawthorn merged commit 8967dd3 into solidusio:master Feb 26, 2016
@mamhoff mamhoff deleted the remove-pre-tax-amount branch March 1, 2016 12:39
@mamhoff mamhoff mentioned this pull request Mar 16, 2016
23 tasks
bbuchalter pushed a commit to TommyJohnWear/solidus that referenced this pull request Nov 18, 2016
This PR has a close relationship with solidusio#706, and solidusio#706 was still missing a
changelog entry.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants