[Flower Farms] - Pay Suppliers Report #12879

chahmedejaz · 2024-09-30T22:34:40Z

⚠️ Please use clockify code #12476 Flower Farms ⚠️

What? Why?

Closes [Report] As an admin, I should access a new report: "Pay Suppliers" #12776
This PR adds the new report named "Pay Suppliers" as described in the attached issue

What should we test?

As mentioned in the issue
As discussed in comments:
- Only supplier's per item fees should be included

Release notes

Changelog Category (reviewers may add a label for the release notes):

User facing changes
API changes (V0, V1, DFC or Webhook)
Technical changes only
Feature toggled

dacook

Great work! This looks good, but I have a ❓ query and a ± request for a change.

Also some other comments, but they are less important.

lib/reporting/reports/suppliers/helpers/columns_helper.rb

dacook · 2024-10-08T00:23:15Z

lib/reporting/reports/suppliers/helpers/columns_helper.rb

+          end
+
+          def order_number
+            proc { |line_items| order(line_items).number }


❓I could be wrong, but it looks like there could be multiple orders grouped together, so I don't think we should show an order number here, should we?

Yes, you are right. Now, that I think of it and reviewed the report again, each order and their respective order cycles should have been displayed here.

Doing this fix now.

It's fixed here 8ee8b37

Great!
± Now I think the line item helpers are no longer needed: variant, order, supplier, distributor, item_order_cycle

lib/reporting/reports/suppliers/helpers/line_items_access_helper.rb

spec/lib/reports/suppliers/pay_your_suppliers_report_spec.rb

chahmedejaz · 2024-10-08T21:52:47Z

@dacook - I've addressed your comments, please review. Thanks

rioug

Looks good @chahmedejaz ! But I think we can improve the testing a little bit.

spec/system/admin/reports/pay_your_suppliers_spec.rb

rioug · 2024-10-08T23:19:01Z

lib/reporting/reports/suppliers/base.rb

+                line_items.each do |line_item|
+                  summary_hash[:total_excl_vat_and_fees] += total_excl_vat_and_fees.call(line_item)
+                  summary_hash[:total_excl_vat] += total_excl_vat.call(line_item)
+                  summary_hash[:total_fees_excl_vat] += total_fees_excl_vat.call(line_item)
+                  summary_hash[:total_vat_on_fees] += total_vat_on_fees.call(line_item)
+                  summary_hash[:total_tax] += total_tax.call(line_item)
+                  summary_hash[:total] += total.call(line_item)
+                end


We are only testing with one order and one line item. I think we need to update the spec to use more than on order and more than one line item in order for this to be properly tested.

It has been addressed here: 03c8e2c#diff-36e7c39c383994d08652e0192d25c4f629d776cc106cb7cec3268bbb8cbd59bf

dacook · 2024-10-09T03:20:50Z

lib/reporting/reports/suppliers/helpers/line_items_access_helper.rb

+          def variant(line_item)
+            line_item.variant
          end


Now most of these helpers are no longer required and can be removed ;)

Not as of now, I think they are used in multiple places 😅

dacook · 2024-10-10T03:50:46Z

I think this addresses Gaetan's feedback sufficiently. Ready for manual testing.

drummer83 · 2024-10-16T21:42:02Z

Hi @chahmedejaz, @RachL and @tschumilas,

Wow, there's a lot to think of and to test here. Overall it's looking really great already. I found two things which I would like to have your confirmation for (marked ❓) and two things around fees need updating, I think (marked ❌).

Here is a quick summary:

Report is added to the report list for admins and super admins ✔️
Each order is listed (no grouping) ✔️
Date format ✔️
The requested columns are available for selection. ✔️
Output of pdf, xls and csv are working ✔️
Filtering of date range is working ✔️
Test taxes (blank if supplier is not tax registered)
- Not quite: Even enterprise has "charges tax = no" active, then still the tax is shown in the report, if the product has a tax applied. This is strange, but it's displayed in the same way on the order details page, so I think it's ok. ❓
Test fees (blank if no fees applied)
- Some fees are assigned to the wrong enterprise. ❌
- Taxes on fees are not displayed. ❌
Payment fees and shipping fees are not considered in the report --> I think that's ok ❓

Regarding assignment of fees

This is the fee I have set up:

It is applied to the distributor of the order cycle:

In the order details we can see that it's charged by the distributor (Konrad Testhub):

The fee is added to supplier of the product (not the distributor, who is charging the fee). I think this leads to incorrect results if you want to pay your suppliers.

❓ It's tricky to solve. Do we have to list those line items again in the area of the distributor? And then only list the fee there?

Regarding taxes on fees

Taxes on fees are not displayed (all zero):

Unfortunately we are not displaying those taxes in the order details, but doing the calculation shows that it is included in 2.78 $:

Also the report "Enterprise Fees With Tax Report By Order" shows that the tax was calculated:

Open for testing

Test more filter options, like order cycle, hub, supplier.

Conclusion

I will move this back to "In progress" but before working on it, let's hear the feedback. Adding the feedback needed label too.
Thanks for the great work so far! 🙏

tschumilas · 2024-10-17T12:18:38Z

Test taxes (blank if supplier is not tax registered)
Not quite: Even enterprise has "charges tax = no" active, then still the tax is shown in the report, if the product has a tax applied. This is strange, but it's displayed in the same way on the order details page, so I think it's ok. ❓

This is a report to payout suppliers. So it is supplier enterprise fees and supplier tax status (registered or not) that matters. The tax shown may be different than the taxes (on fees or products) shown in other reports. (This is why we need this particular report)

2 scenerios are possible in Canada:

(1) The supplier is tax registered AND the distributor (hub) is tax registered. The distributor (hub) will collect tax on taxable products. The hub will pay the supplier these taxes the hub collected, so the hub needs this number. (The tax registered supplier will remit this tax to the government)
(2) The supplier is NOT tax registered but the distributor (hub) IS tax registered. The hub collects tax on the sale (so yes, it will be on the order, and in other reports) but the hub will NOT payout this tax to the non-registered supplier. (The tax registered hub remits the tax to the government themselves). So the field for tax in this payout report should show no tax when the supplier is Not registered (eventhough the order and other reports show the tax).
Scenerio (3) - the supplier IS tax registered, but the hub is NOT tax registered is quirky and shouldn't really happen. Because the seller (distributor) is not tax registered, no tax is collected on sales. So no tax can be paid out to the supplier. So if the hub is not tax registered this report would have to show 0 tax collected.

RachL · 2024-10-21T14:59:09Z

@chahmedejaz is it clear for you on how to proceed here? Let me know if you'd need a call.

chahmedejaz · 2024-10-21T15:33:09Z

Thanks @drummer83 and @RachL

I'm looking into the issue to understand it properly, I validated the fees part, I think it's worked fine for me. I'm a bit confused on whose fees should be included? Supplier's or the distributor's?
Other than that I think I may need a call but I'll let you know pretty soon. Thanks

tschumilas · 2024-10-29T16:44:11Z

In response to the question - whose fees should be included - supplier fees, distributor fees or both?
I would say this report should only include the Supplier fees. This is a report to give a hub the amount they need to payout to suppliers. Hubs do not payout their enterprise fees (ie - their 'mark up') to suppliers.

But maybe there are other use cases elsewhere? I'm going to post in instance managers to see.

chahmedejaz · 2024-11-01T14:57:08Z

Thanks for the response @tschumilas. Sorry for the delay, I'm just back from my wedding vacation.

Anyhow, as per my understanding, of:

I would say this report should only include the Supplier fees.

Do we need to include only the green highlighted enterprise fees?

CC: @RachL , @drummer83

Co-authored-by: David Cook <[email protected]>

- wrong enterprise fees - always 0 tax on fees

…er.rb Co-authored-by: David Cook <[email protected]>

RachL · 2024-11-15T10:36:23Z

I don't know why this was not merged, but I agree we can move to production and test with real-life cases now, so I've rebased and will merge with bypass.

KUDOS @chahmedejaz @drummer83 @filipefurtad0 this was a tough one

filipefurtad0 · 2024-11-15T14:59:22Z

I don't know why this was not merged

Forgot to click the button 😅
Thanks @RachL!! 🎉

tschumilas · 2024-11-26T15:58:42Z

I think we have 2 issues with this report.

Maybe this is a translation issue? But the column labelled "Total Tax ($)" is calculating the tax on just the product - column I * tax rate (.13 in this example). So I think this column should be 'Product Tax'. So is this my mislabeling in transifex?

Second issues is trickier - and maybe my mistake at the beginning. A hub collects taxes on the sale. But the hub will only payout these taxes to a supplier IF the supplier is tax registered. That is, if 'charges tax = yes' If for a given supplier, 'charges tax' = no, then NO taxes should be paid out by the hub to this supplier , so columns L and M should be blank.

So - we as a minimum, we need this report to have a column that indicates if the supplier's tax registered status. This should be the 'charges tax' status. Maybe in other places, the ACN/ABN number is this - but here in Canada, an enterprise can have a business number but not be tax registered.

If we have a 'charges tax' column (with yes/no), then the hub manager can 'do the math' and payout or not payout the taxes.
Better yet - would be if we did that for the hub manager, and left columns L & M (the tax columns) blank if supplier charges tax = no.

In the testing notes above @drummer83 noted this same issue during testing:
Test taxes (blank if supplier is not tax registered)
Not quite: Even enterprise has "charges tax = no" active, then still the tax is shown in the report, if the product has a tax applied. This is strange, but it's displayed in the same way on the order details page, so I think it's ok. ❓

Here's an output with a single supplier who does not charge tax. This is their business setting:

And here is the report output for a charges tax = 0 supplier

RachL · 2024-11-26T20:11:08Z

@tschumilas thank you for this detailed testing!

regarding the tax, can I clarify who is the owner of the fee in your example? I'm a bit confused why the tax on the fee is greater than the total tax 🤔

Regarding the naming, I don't think this is how we've specified it in the issue: https://docs.google.com/spreadsheets/d/1XDi3whY2h2DnHQtYS0Fbogr_VTJmYhAXzt1p9ElBBbM/edit?gid=0#gid=0

The total tax was (to my understanding) the sum of all taxes, including fees. So that total excl. tax + total tax = total.

But maybe what's missing is actually a column showing the total product tax?

chahmedejaz · 2024-11-26T20:56:55Z

The total tax was (to my understanding) the sum of all taxes, including fees. So that total excl. tax + total tax = total.

The total tax does not include the tax on fees. Maybe that's what is missing here 😅

Moreover, as per this:

So - we as a minimum, we need this report to have a column that indicates if the supplier's tax registered status. This should be the 'charges tax' status. Maybe in other places, the ACN/ABN number is this - but here in Canada, an enterprise can have a business number but not be tax registered.

I think we need another column in the report to show the tax status of the supplier.

tschumilas · 2024-11-28T15:08:57Z

so if I understand - right now on the example above - 'total tax' is the tax on product, sorry I missed that. Yes that calculates. .13 (the tax rate on for this product) * 21.69 (total excluding fees and taxes ) - total tax (2.82)
Now I get it.
I'm going to make a note that this total tax figure will not add up to be the total taxes on an order (because an order will have per order fees, shipping fees and taxes on those ..., and taxes on an order will include hub fees and their taxes as well). So thats good.

And, it makes sense that total tax in this report (tax on product) can be less than total tax on fees. In the example above there are very steep per item shipping fees - so thats why. (These are supplier fees - which is why they are showing in this report - so all good there.)

As an aside - I've been thinking of trying to make a section for the user guide dedicated to understanding reports. We have a not of new ones, that are very specific. I know I'm getting the mixed up - and questions on reports is now the most common area of questioning I'm getting from users.

So yes we need a column to show tax status of the supplier. Perfect!

RachL · 2024-11-28T16:08:16Z

@tschumilas maybe we should have a quick call about it?

I'm thinking it would be clearer to have a column called product tax + a total tax column. so we have the full breakdown.

We can add a column indicating if the producer is tax registered, but to me, if the producer is not tax registered, then the product tax column is 0. So I think we have a bug in the example you've shared?

tschumilas · 2024-11-29T13:59:55Z

I agree with you @RachL and maybe we should have a call because I'm also wanting make sure this report is useful outside of Canada. But I'm not clear how hubs where taxes are included in prices issue payouts. So my thining was to have all the numbers show - even when the supplier is not tax registered - and the hub can decide which total to payout (without or with tax). It seems its more generally useable that way.

RachL · 2024-11-29T14:08:59Z

just a quick answer:

and the hub can decide which total to payout (without or with tax). It seems its more generally usable that way.

I don't think there is an OFN country currently that pays taxes to suppliers if they are not registered. So, the CA use case is as far as I know the general use case; So let's make it easy for our users 👍

RachL · 2024-12-02T15:52:41Z

@chahmedejaz I've created #13013 in order to add the columns.

Theresa has sent me on slack the fees set up in the example above, I will try to set them up on a staging server to reproduce the problem of tax columns not at 0 when producer is not tax registered and create an issue accordingly.

chahmedejaz · 2024-12-02T16:04:33Z

Sure, Rachel 👍🏻

github-actions bot assigned chahmedejaz Sep 30, 2024

chahmedejaz force-pushed the task/12776-pay-suppliers-report branch from 271aa44 to bbed8f2 Compare September 30, 2024 22:51

chahmedejaz marked this pull request as ready for review October 7, 2024 10:14

dacook self-requested a review October 7, 2024 23:43

dacook requested changes Oct 8, 2024

View reviewed changes

chahmedejaz force-pushed the task/12776-pay-suppliers-report branch from b793d82 to 8ee8b37 Compare October 8, 2024 21:31

chahmedejaz requested a review from dacook October 8, 2024 21:52

rioug requested changes Oct 8, 2024

View reviewed changes

dacook reviewed Oct 9, 2024

View reviewed changes

chahmedejaz requested review from dacook and rioug October 10, 2024 01:04

dacook approved these changes Oct 10, 2024

View reviewed changes

drummer83 force-pushed the task/12776-pay-suppliers-report branch from 03c8e2c to 9c70867 Compare October 12, 2024 16:28

drummer83 added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Oct 12, 2024

drummer83 self-assigned this Oct 16, 2024

drummer83 added the pr-staged-au staging.openfoodnetwork.org.au label Oct 16, 2024

drummer83 force-pushed the task/12776-pay-suppliers-report branch from 9c70867 to 7f150c7 Compare October 16, 2024 19:28

drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Oct 16, 2024

drummer83 removed their assignment Oct 16, 2024

drummer83 added the feedback-needed label Oct 16, 2024

chahmedejaz force-pushed the task/12776-pay-suppliers-report branch from 7f150c7 to 25611fd Compare November 1, 2024 15:13

chahmedejaz and others added 16 commits November 15, 2024 11:09

fix rubocop lines issue

13a614a

fix specs

77f9c65

add systems spec

654263a

Update lib/reporting/reports/suppliers/helpers/columns_helper.rb

458c8f7

Co-authored-by: David Cook <[email protected]>

add slash for abn acn

68c0d98

use to_date for locale based formating

ec0d2d3

12776: fix missing order numbers

1fbdf25

update specs to have more line items

ed559b5

fix reported issues:

298c0e8

- wrong enterprise fees - always 0 tax on fees

12776: remove unnecessary include

f3e086a

12776: refactor supplier_adjustments method

bc57447

12776: use EnterpriseFeeCalculator in specs

e2d999d

12776: rename vat to tax

9bcdac8

12776: refactor spec

6b3b29a

Update lib/reporting/reports/suppliers/helpers/line_items_access_help…

4965e2b

…er.rb Co-authored-by: David Cook <[email protected]>

12776: fix included tax on fees

ed76852

RachL force-pushed the task/12776-pay-suppliers-report branch from 7a715ea to ed76852 Compare November 15, 2024 10:09

RachL merged commit db79af4 into openfoodfoundation:master Nov 15, 2024
51 checks passed

RachL mentioned this pull request Jan 17, 2025

Add columns to "pay your supplier" report #13037

Open

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Flower Farms] - Pay Suppliers Report #12879

[Flower Farms] - Pay Suppliers Report #12879

chahmedejaz commented Sep 30, 2024 •

edited

Loading

dacook left a comment

dacook Oct 8, 2024

chahmedejaz Oct 8, 2024

chahmedejaz Oct 8, 2024

chahmedejaz Oct 8, 2024 •

edited

Loading

dacook Nov 13, 2024

chahmedejaz commented Oct 8, 2024

rioug left a comment

rioug Oct 8, 2024

chahmedejaz Oct 10, 2024

dacook Oct 9, 2024

chahmedejaz Oct 10, 2024 •

edited

Loading

dacook commented Oct 10, 2024

drummer83 commented Oct 16, 2024

tschumilas commented Oct 17, 2024

RachL commented Oct 21, 2024

chahmedejaz commented Oct 21, 2024

tschumilas commented Oct 29, 2024

chahmedejaz commented Nov 1, 2024

RachL commented Nov 15, 2024 •

edited

Loading

filipefurtad0 commented Nov 15, 2024

tschumilas commented Nov 26, 2024

RachL commented Nov 26, 2024

chahmedejaz commented Nov 26, 2024 •

edited

Loading

tschumilas commented Nov 28, 2024

RachL commented Nov 28, 2024 •

edited

Loading

tschumilas commented Nov 29, 2024

RachL commented Nov 29, 2024

RachL commented Dec 2, 2024 •

edited

Loading

chahmedejaz commented Dec 2, 2024

[Flower Farms] - Pay Suppliers Report #12879

[Flower Farms] - Pay Suppliers Report #12879

Conversation

chahmedejaz commented Sep 30, 2024 • edited Loading

What? Why?

What should we test?

Release notes

dacook left a comment

Choose a reason for hiding this comment

dacook Oct 8, 2024

Choose a reason for hiding this comment

chahmedejaz Oct 8, 2024

Choose a reason for hiding this comment

chahmedejaz Oct 8, 2024

Choose a reason for hiding this comment

chahmedejaz Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

dacook Nov 13, 2024

Choose a reason for hiding this comment

chahmedejaz commented Oct 8, 2024

rioug left a comment

Choose a reason for hiding this comment

rioug Oct 8, 2024

Choose a reason for hiding this comment

chahmedejaz Oct 10, 2024

Choose a reason for hiding this comment

dacook Oct 9, 2024

Choose a reason for hiding this comment

chahmedejaz Oct 10, 2024 • edited Loading

Choose a reason for hiding this comment

dacook commented Oct 10, 2024

drummer83 commented Oct 16, 2024

Regarding assignment of fees

Regarding taxes on fees

Open for testing

Conclusion

tschumilas commented Oct 17, 2024

RachL commented Oct 21, 2024

chahmedejaz commented Oct 21, 2024

tschumilas commented Oct 29, 2024

chahmedejaz commented Nov 1, 2024

RachL commented Nov 15, 2024 • edited Loading

filipefurtad0 commented Nov 15, 2024

tschumilas commented Nov 26, 2024

RachL commented Nov 26, 2024

chahmedejaz commented Nov 26, 2024 • edited Loading

tschumilas commented Nov 28, 2024

RachL commented Nov 28, 2024 • edited Loading

tschumilas commented Nov 29, 2024

RachL commented Nov 29, 2024

RachL commented Dec 2, 2024 • edited Loading

chahmedejaz commented Dec 2, 2024

chahmedejaz commented Sep 30, 2024 •

edited

Loading

chahmedejaz Oct 8, 2024 •

edited

Loading

chahmedejaz Oct 10, 2024 •

edited

Loading

RachL commented Nov 15, 2024 •

edited

Loading

chahmedejaz commented Nov 26, 2024 •

edited

Loading

RachL commented Nov 28, 2024 •

edited

Loading

RachL commented Dec 2, 2024 •

edited

Loading