sales tax totals by order report

[abdellani]

What? Why?

• Closes Tax Totals with Rates by Order Report #6849
  Need to merge this first Tax Totals with Rates by Producer Report #10175

What should we test?

• Described on: Tax Totals with Rates by Order Report #6849

Release notes

Changelog Category: User facing changes

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

[abdellani]

@filipefurtad0 @lin-d-hop

The second report is implemented now. The filtering logic is implemented on #10175

In the meantime, I'll implement the test described in this comment .
If you have any other scenarios in mind, please share them.

[abdellani]

I implemented the following tests:

Test 1

Tax type: added
1 order_cycle with 1 order.
The order has one variant.
Variant price 100$
Shipping cost 10$ (taxable)
the enterprise fees (distributor) cost 5$ (taxable)
Two tax rates:
State rate: 1.5%
Country rate: 2.5%

Tax Category	Tax rate	Total (tax excl)	Tax	Total (tax incl)
state	1.5%	115	1.73	116.73
country	2.5%	115	2.88	117.88
.	Total	115	4.61	119.61

Test 2

same as test 1, except that the tax is included

Tax Category	Tax rate	Total (tax excl)	Tax	Total (tax incl)
state	1.5%	110.5	1.7	112.2
country	2.5%	110.5	2.8	113.3
.	Total	110.5	4.5	115.0

Test 3

I added another order with variant = 200$ from another customer.

Tax Category	Tax rate	Total (tax excl)	Tax	Total (tax incl)
state	1.5%	110.5	1.7	112.2
country	2.5%	110.5	2.8	113.3
.	Total	110.5	4.5	115.0
state	1.5%	215.0	3.23	218.23
country	2.5%	215.0	5.38	220.38
.	Total	215.0	8.61	223.61

Test 4

I filter customer 1 from test 3

Test 5

I filter customer 2 from test 3

Test 6

I filter customers 1&2 from test 3

@filipefurtad0 are those enough? or do you have any suggestions?

[abdellani]

The main challenge that I faced was to trigger the tax calculation after adding manually the shipping address and the enterprise fees to the order.
In summary, the enterprise fee calculator relies on the order_cycle to calculate the enterprise fees.

When I used the factory to create the order, the order_cycle was nil. the fees_handler was instantiated with order_cycle = nil.
The fees_handler, cached on the order, was not able to return the right results.

openfoodnetwork/app/models/spree/order.rb

Lines 620 to 622 in ee70645

	def fee_handler
	@fee_handler ||= OrderFeesHandler.new(self)
	end

I fixed the problem by destroying the fees_handler after defining the order_cycle on the order.
I wouldn't face this issue if we delegated distributor & order_cycle to the order on OrderFeesHandler.

openfoodnetwork/app/services/order_fees_handler.rb

Lines 3 to 10 in ee70645

	class OrderFeesHandler
	attr_reader :order, :distributor, :order_cycle
	def initialize(order)
	@order = order
	@distributor = order.distributor
	@order_cycle = order.order_cycle
	end

can be replaced with

  attr_reader :order
  delegate :order_cycle,:distributor, to: :order
  def initialize(order)
    @order = order
  end

[filipefurtad0]

@filipefurtad0 are those enough? or do you have any suggestions?

These sound great already. I think in addition, it would be great to assure different roles (superadmin, enterprise, supplier) have access to the report data. I think it would be enough to test it at the controller/request level - this is currently done here, for other reports.

However, and similarly to what we've agreed before, we should not block the PR, and we can address this separately. Other than this one, no further suggestions from my side.

[abdellani]

@filipefurtad0

I added new tests to try accessing the report as an admin, a distributor, and a supplier.

When I use the supplier account, the controller redirects the request (status:302) instead of generating the report.

The following test confirms the tax reports are not relevant to producers

openfoodnetwork/spec/controllers/admin/reports_controller_spec.rb

Lines 155 to 167 in ee70645

	context "Supplier" do
	before { controller_login_as_enterprise_user [supplier1] }
	describe 'index' do
	it "loads reports relevant to producers" do
	spree_get :index
	report_types = assigns(:reports).keys
	expect(report_types).to include :orders_and_fulfillment,
	:products_and_inventory, :packing # and others
	expect(report_types).to_not include :sales_tax
	end
	end

Can you please confirm?

[filipefurtad0]

Many thanks @abdellani for adding the tests to the controller spec - I think this is really useful.

Good point: so far we keep tax reports not accessible for OC suppliers. But since this is a new report I'm not entirely sure.

I'll leave @lin-d-hop to confirm this one, as we've discussed otherwise here - but I might have misunderstood.

[lin-d-hop]

These reports are only relevant to people with shops - sells = true.
So producers that are only supplying other people's shops do not need to access these reports

[dacook]

I have to admit I'm not able to follow this one. Maybe it's because it's the end of my day.
I can't see much mention of suppliers in the issue, PR or this example.

From what I can understand, this grouping is an extra level of grouping that occurs before the grouping defined in rules. Is this defined here because it was too complex for that section?

[abdellani]

Yes, that's correct.

Every item returned by query_result will:

1. match one row in the final report that you'll see on the HTML page (if we don't count the summary rows)
2. be sent to the functions written on columns.

An item will look like

{
    [tax_rate_id, supplier_id, order_id] => order 
}

I needed that extra level of grouping because one order can much multiple :

1. tax rates.
2. supplier

when one of the columns' functions is called, we need to know which of the tax rates is targeted in the report row. This can only be determined from the first hash key.

Now that I think about it again, I can simplify the code by grouping the order instead of line items.

[dacook]

Hi @abdellani , I tried to review this but ran out of time and am having trouble understanding. I've asked one question (above) to get started, but perhaps it would be good to get somebody else's review in the meantime.

By the way, what's a good way to learn more about the reports framework? Is there a tutorial or reference anywhere?

[abdellani]

By the way, what's a good way to learn more about the reports framework? Is there a tutorial or reference anywhere?

Unfortunately no. I learned about it by solving the issues on the #9981

I was thinking about writing some documents (something that's not official, just in my personal blog :) ) that detail some parts of the system that are not documented.

[dacook]

Great job, this looked like a tricky report to pull together!

I've added a few comments mainly just to share my thoughts and learnings.
But I also have a couple of suggestions for method naming, marked with ±.

Otherwise all looks good to me!

[mkllnk]

Great.

[filipefurtad0]

Hey @abdellani ,

I'll basically try to cover scenarios not covered in the spec.

Access rights

Superadmin - should access 🟢
Hub (Distributor) - should access 🟢
Producer - should not access 🟢

Report output

For an order with applied:

• product tax
• shipping tax
• enterprise fees (order level)
• enterprise fees (line item level)
• enterprise fees applied on the producer as well as on the distributor level

we get the report:

This looks good. We can see all the fee types and associated tax.

Q1: ordering of the results - should we introduce any type of ordering, in the output? It seems to me that the ordering is random, or not following an evident pattern. I'd expect completion date, but I'm not sure if this would be useful.

Filters

Q2: I think the customer filter is not working... Should we address this or release as is?

The results above were obtained by applying the filters:

However, if we add the customers filter, then no results are returned:

Q3: I'm not sure what is the expected behavior when we filter by producer; Should get the orders with products supplied by a given producer, right? If so, then I think we're good.

In a way I was expecting to see the tax charged by a given supplier, when filtering by Producer (supplier). For example the VAT tax above, concerns Shipping and a Product from a supplier to that OC (notice the Tax Category on the product below):

I'm not entirely sure here.

Summary

From the three issues above:
Q1: results ordering -> improvement
Q2: broken customers filter -> likely blocker to release the feature
Q3: breakdown of taxes by producer / per order -> probably expected behavior

Requesting your feedback here @abdellani 🙏

[filipefurtad0]

Edit: regarding Q2

I think I've found out the issue:

I think this relates to how customers are scoped on that filter. There are several options for the same email address, so it's not possible to get all the orders for a given customer, by choosing one -> so, if we select all options with the same address then the filter works, and all the results for that address are returned.

[filipefurtad0]

This seems to occur in all the new reports,
So my proposal would be to merge this PR and address this issue separately.

I'm not entirely sure we should release it as is, as the first impression for the user is that the filter does not work (-> impacting support teams, maybe)

What do you think @abdellani @openfoodfoundation/train-drivers-product-owners ?

[RachL]

As I'm not really accross this work in detail yet, I will let @lin-d-hop answer 👍

[lin-d-hop]

Q1: A random ordering will cause questions from users.
I believe the most standard ordering we have is name/surname on the core model of the report. This report would be Customer Surname. Only add this if it does not add a significant performance implication.

Q2: Customer is a new filtering in this report and an important one for wholesale usage. The current implementation, from Filipes comments, appears too unintuitive to be useable. I would say we have 2 options:

1. If we do include the Customer filter, adjust so that it is higher in scope and a single entering of the email address will offer all results.
2. If that is not a very easy change (less than 2hrs), leave the customer filter off for now and we can add that in as a papercut later.

[abdellani]

Q1: The problem is that ::Permissions::Order (which is used on the other reports to load the orders after filtering them) doesn't support filtering by suppliers.

I'm using Reporting::LineItems to load the line items filtered by supplier. From the line items, I fetch the orders ([]#uniq is used to make sure that the orders are not duplicated).

          orders = report_line_items.list.map(&:order).uniq

This is why the orders are sorted differently from what you can see on the other reports.

Sorting the orders from the ruby code will be easy and I don't think this will make a big difference in the performance.

Q2: The second option is better since the issue is related to multiple reports.
The source of the problem is that on the reports we're filtering customers by id, but a customer can be stored multiple times on the database, and every record will have a different id.

Q3: if the supplier filter is used, the report will load orders with at least one line item produced by the selected supplier.

What's next on this PR?

I'll fix Q1.
Q2 is a separated issue now.
If Q3 is good, this PR will be good.

[abdellani]

I double-checked the report, the rows are already sorted. On the rules method, if sort_by is not set, the group_by key will be used to sort the rows.

openfoodnetwork/lib/reporting/report_rows_builder.rb

Lines 102 to 112 in 10d8e80

	def sort_groups_with_rule(groups, rule)
	groups.sort_by do |group_key, _items|
	# By default sort with the group_key if no sort_by rule is present
	if rule[:sort_by].present?
	rule[:sort_by].call(group_key)
	else
	# downcase for better comparaison
	group_key.is_a?(String) ? group_key.downcase : group_key.to_s
	end
	end.to_h
	end

In the report, it'll be

1. distributor name
2. order cycle name
3. order number

I found the same result on my local database.

Is that Ok? or do we need to sort them by customer's last name?

---

I added pending to the tests related to filtering by customer.

[mkllnk]

That looks good to me. This needs a new issue written up then. @abdellani You are probably the most knowledgable to write such an issue. Are you up for that?

[filipefurtad0]

In the report, it'll be

1. distributor name
2. order cycle name
3. order number

Ah, indeed - great, thanks for pointing this out @abdellani . I was able to verify this behavior, if I disregard some exceptions which I believe are related with some non-consistent orders/data in staging.

For example, this order https://staging.openfoodnetwork.org.uk/admin/orders/R106168541/edit has order cycle -> none:

And for that reason it appears at the top:

This an edge case which needs separate investigation. However, the rest of the ordering is consistent with what you describe.

I added pending to the tests related to filtering by customer.

Perfect, let's tackle it separately

Merging now!! 🎉 💪

[abdellani]

Hi @filipefurtad0

thank you for the feedback:pray:
I double-checked that order, the order_cycle is nil. This value will be converted to an empty string "" before sorting. This is why that order cycle is appearing first.

      group_key.is_a?(String) ? group_key.downcase : group_key.to_s 

[abdellani]

@mkllnk the issue is already reported by @filipefurtad0