openfoodfoundation / openfoodnetwork

Connect suppliers, distributors and consumers to trade local produce.
https://www.openfoodnetwork.org
GNU Affero General Public License v3.0
1.12k stars 724 forks source link

Tax not charged on Enterprise Fee, when tax category set to "Inherit from product" #10348

Closed filipefurtad0 closed 1 year ago

filipefurtad0 commented 1 year ago

Description

If an enterprise fee is set to Inherit From Product, then tax is not charged over the value of the fee.

Expected Behavior

The inherit from product option should work: tax should be charged over the value of the enterprise fee.

EDIT: to be defined.

Actual Behaviour

The inherit from product option is not working: tax is not charged over the value of the enterprise fee.

Steps to Reproduce

(tricky... here we go:)

As superadmin:

  1. Create a zone
  2. Create a tax category
  3. Create a tax rate linking the zone and the tax category

As admin:

  1. Create an enterprise fee with the Flat rate (per order) calculator and set the tax category to <tax_category_name> (created in 2.), with a non-zero amount.
  2. Create a product with a tax category having a tax rate for the zone created in 1.
  3. Create an order cycle: Add the product created in 2. (incoming and outgoing exchanges) and the enterprise fee created in 1. to outgoing exchange
  4. Create an for a customer with billing address within the zone (shopfront or backoffice) and with the product created in 5. (the order should contain only this product)

=> if all is well, you should have an order with tax applied to both the product and the enterprise fee (this is the correct behavior, but it is necessary to make sure this works, to observe the bug)

  1. Edit the enterprise fee: set it to Inherit from Product
  2. Repeat step 7

=> Notice tax was only charged on the product, not the enterprise fee.

Animated Gif/Screenshot

image

Workaround

Selecting the desired tax category, instead of using the Inherit from Product option.

Severity

bug-s3: a feature is broken but there is a workaround

Your Environment

Possible Fix

Not really a fix, but a "workaround" could be to remove this option from the UI, to prevent the bug. While doing so, we would need to make sure the blank option is selected by default.

filipefurtad0 commented 1 year ago

I'm not sure how representative this is to other instances, but in staging-UK about 1/3 of all enterprise fees are set to inherit_tax_category:

openfoodnetwork=> select count(*) from enterprise_fees where
inherits_tax_category IS true;
 count 
-------
    40
(1 row)

openfoodnetwork=> select count(*) from enterprise_fees;
 count 
-------
   118
(1 row)

From Metabase on FR-prod, the queries above rendered 235 / 547 ~ 40%.

EDIT: the impacted users need to have the inherit_tax_category AND the calculators which operate at the order level, e.g.Flat rate (per order), Price sack and Flexible rate; it should not affect Flat percent (per item), Flat rate (per item), Weight.

abdellani commented 1 year ago

@filipefurtad0

I was able to reproduce it. The problem is happening with incoming/outcoming exchanges of the order cycle.

abdellani commented 1 year ago

hi @filipefurtad0

After following the steps, we'll have an enterprise fee with:

I have two questions:

  1. How can I get a tax category for an order?
  2. What if the order has two products, and each product belongs to a different tax category? which tax category will be used by the enterprise fee?

I think that the source of the problem is Spree::TaxRate (not 100% sure)

https://github.com/openfoodfoundation/openfoodnetwork/blob/890ab6796ed87fc59eec3c99b9a773f19b330379/app/models/spree/tax_rate.rb#L40-L54

This is the part of the code used by OrderFeesHandler to create the enterprise fee-related taxes (tax_enterprise_fees!).

When 'inherit from product' is selected, the EnterpiseFee will have: 1. tax_category_id: nil 2. inherits_tax_category: true

#<EnterpriseFee:0x000055b128e20390
 id: 8,
 enterprise_id: 7,
 fee_type: "transport",
 name: "transport fee",
 created_at: Mon, 23 Jan 2023 20:49:19.703312000 AEDT +11:00,
 updated_at: Tue, 28 Feb 2023 19:41:38.249110000 AEDT +11:00,
 tax_category_id: nil,
 inherits_tax_category: true,
 deleted_at: nil> 

In the Spree::TaxRate, we only check if the list of applicable tax categories include the tax category of the enterprise fee (which is nil).

    def self.adjust(order, items)
....
      relevant_items, non_relevant_items = items.partition do |item|
        applicable_tax_categories.include?(item.tax_category)
      end
...
filipefurtad0 commented 1 year ago
I have two questions:

    How can I get a tax category for an order?
    What if the order has two products, and each product belongs to a different tax category? which tax category will be used by the enterprise fee?

Yes, indeed those are the questions which made me spot this bug in the first place. I should have been more explicit about this on the expected outcome (now edited) -> we've discussed this briefly here. Adding my thoughts on some options:

i) One way to prevent this situation to occur would be to change the UI and conditionally remove the option inherit from product option, when the fee is applied on the order level - or prevent this change from being saved. So, in practice, something like:

ii) Another option, would be to remove the option flat rate per order (and other order-based calculators, see below) once the user selects inherit from product, as the Tax Category. This would prevent the user from going back and forth.

I have not tested these combinations with inherit from product, but I assume we're having the same issue, or charging an inconsistent value.

filipefurtad0 commented 1 year ago

In summary, I'd say Inherit from Product should not be a valid choice for these calculators:

https://github.com/openfoodfoundation/openfoodnetwork/blob/890ab6796ed87fc59eec3c99b9a773f19b330379/spec/models/enterprise_fee_spec.rb#L84-L89

It looks like Flat Percent actually acts at the line item level, according to the spec above, this looks like this would need to be edited on the user guide:

image

abdellani commented 1 year ago

@filipefurtad0

This makes sense to me.

I added a validation that will prevent saving the enterprise fee if the wrong combination is selected.