Tax Totals with Rates by Producer Report

[abdellani]

What? Why?

• Closes Tax Totals with Rates by Producer Report #6848

What should we test?

• described on: Tax Totals with Rates by Producer Report #6848

Release notes

Changelog Category: User facing changes

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

Dependencies

Documentation updates

[mkllnk]

Can you add a spec which covers this new report?

Otherwise the code looks fine. A bit confusing but that's due to the design of the reports.

[mkllnk]

It looks like we should make this class shorter. Otherwise we should add it to the rubocop todo list so that other devs can use rubocop without seeing known issue they didn't cause.

[abdellani]

@mkllnk

1. when I started to work on filtering, I was not able to filter by supplier_id using ::Permissions::Order, I needed to use Reporting::LineItems.
2. In the view, we suppose that all the subreports shared the same filtering options. In the requirement, we have Sales Tax Totals By Producer has different filters than the other subreports Tax Types & Tax Rates.

I suggest trying to load the subreport filtering partial (if it's implemented). I don't like rescue nil because it'll make the partial fail silently.

    = render partial: "admin/reports/filters/#{@report_subtype}", locals: { f: f } rescue nil

Is it better to add an instance variable to the report to tell if the subreport has additional filtering options?

3. I can filter the line items by tax_rate, but in the report, we'll have the tax rates that are connected to the line items.

If we use the same example from the discussion:
I set the tax rate field to 'state',
the line items will match 'state' and 'country'
the final report will show 'state' and 'country'

Is it ok to show both?
Or I can fix the problem by adding additional filtering in the query_result, but I think it will make the code more complex.
What do you think?

[abdellani]

@mkllnk

I'm thinking what the best way to test the report.

I suggest system tests with some scenarios. I started with the simplest scenario where we have:

  #  1 producer
  #  1 distributor
  #  1 product that costs 100$
  #  1 order with 1 line item
  #  the line item match 2 tax rates: country (10%) and state (10%)
  #  shipment and enterprise fees are taxable

Do you think that system tests are enough?
Do we need to test more complex scenarios? I personally that we need to do that, but there are too many possibilities.

[lin-d-hop]

I would say it would be good to have system tests with more complex scenarios for this report. @filipefurtad0 might have some suggestions :)

[filipefurtad0]

In addition to the comments I have some questions:

I'm not sure but should this report be accessible only to superadmin? Or should it be accessible when logged in as distributors/suppliers as well?

There are some acceptance criteria which we might need to cover for in request specs, especially that the data is retrievable through API. Should this work through API v1 or v0?

[abdellani]

In addition to the comments I have some questions:

I'm not sure but should this report be accessible only to superadmin? Or should it be accessible when logged in as distributors/suppliers as well?

There are some acceptance criteria which we might need to cover for in request specs, especially that the data is retrievable through API. Should this work through API v1 or v0?

I don't know.

Maybe @lin-d-hop can answer those questions?

[abdellani]

@mkllnk
I tried the following queries:

query 1 returns the ideal result.
query 2 is very similar to what we have in the report
query 3, I tried to adapt query 2 to produce the same result as query 1

# query 1  => returns 2 rows 
Spree::LineItem.where(id: 9)
.order("supplier.name", "product.name", "variant.display_name")
.left_joins(variant:[product: [:supplier,{tax_category: :tax_rates}]])

# query 2 (similar to the one used in the report) => returns 1 row !!
Spree::LineItem.where(id: 9)
  .includes(variant: [product: :supplier]).references(:line_items)
  .order("supplier.name", "product.name", "variant.display_name")
  .left_joins(variant:[product: [:supplier,{tax_category: :tax_rates}]])

# query 3 => returns 1 row 
Spree::LineItem.where(id: 9)
  .includes(variant: [product: :supplier]).references(:line_items)
  .order("supplier.name", "product.name", "variant.display_name")
  .joins('left outer join spree_tax_categories category on category.deleted_at is null and category.id = product.tax_category_id')
  .joins('left outer join spree_tax_rates tax_rate  on tax_rate.deleted_at is null and tax_rate.tax_category_id = category.id')

If you compare query 1 and query 3, you'll see that the only difference is in the SELECT part, the parts that come after FROM are identical. I don't see any DISTINCT or UNIQUE in query 3, but it still returns 1 row.

queries.zip

[mkllnk]

Is Rails reducing two rows to one or is psql also returning one row only?

If Rails can't do it then maybe we have to let the join go.

[abdellani]

when I run the query directly in the database, I get 2 rows.
I think that the rails is doing it.

If I add pluck(:id) to the third query, I'll get 2 rows.

Spree::LineItem.where(id: 9)
  .includes(variant: [product: :supplier]).references(:line_items)
  .order("supplier.name", "product.name", "variant.display_name")
  .joins('left outer join spree_tax_categories category on category.deleted_at is null and category.id = product.tax_category_id')
  .joins('left outer join spree_tax_rates tax_rate  on tax_rate.deleted_at is null and tax_rate.tax_category_id = category.id')
  .pluck(:id)

 => [9, 9]                                                                                                                                            

[abdellani]

I removed the SQL join query and tried to simplify the previous code used to group the line items.

[mkllnk]

Great. Let's move forward with this.

[mkllnk]

Why did you choose pluck over select here?

[abdellani]

because pluck is faster than select, it doesn't create an active record instance.

benchmark

[mkllnk]

Oh, I thought it's used within another query and therefore just a sub-query within the database instead of going though Rails.

[filipefurtad0]

Hey @abdellani ,

I've tested this PR for an instance/server with added- (left) and included-taxes (right). I've overlapped the respective invoices. The (preliminary) results for 1 order only are shown below:

The main reason is: accessing this report broke the page very frequently (error 504). Even when:

• logged in as an enterprise (not superadmin)
• using filters to limit the enterprise, order cycle, dates
  the report would hang and eventually timeout.

I've checked different browsers (sessions, and after deleting cookies). But observed timeouts nonetheless. I wonder if we should release this with a feature toggle.

I'm moving it to ready to go, but not merging - I'm not confident we should release it this week already, till we consider the performance issue.

Aside from the performance concerns, this looks good to go 👍
EDIT: I'm also thinking there are other relevant scenarios here. So I've moved back to Test Ready.

[filipefurtad0]

Ok, this might also be a relevant test case:

• an order cycle with three different suppliers

Logged in as the distributor,
We can access the tax values for each supplier 🟢

Logged in as one of the suppliers - producer shop profile,
We can see the order cycle on the OC drop-down, but the query yields no results 🟢

Logged in as one of the suppliers - supplier only profile,
Does not have access the tax report section:

Great 💪

[filipefurtad0]

Ok, I've just ran an unrelated report in master, and still I'm getting timeouts regularly. So I can't really point out this code as the cause.

I'd say we're good to merge @abdellani 👍

[sigmundpetersen]

Is the Background report feature activated? Could this be causing the timeouts?
Ref #10338

[filipefurtad0]

Is the Background report feature activated? Could this be causing the timeouts?

Thanks @sigmundpetersen - deactivating it seems to prevent timouts in master indeed 👍 I'll stage this PR again and double-check for timeouts (without this feature toggle on).

[filipefurtad0]

Yup, I can confirm that after enabling background_reports I consistently get a 504;

If I run the report as an enterprise owner, then rendering is successful.

I was only able to blow things up 💥 as superadmin, and by running the report without any filters or date restrictions - I think this is acceptable. If filters or date restrictions are added then the report renders as expected 👍

Merging - thanks @abdellani!