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

Fetch variant enterprise fees from order lineitems adjustments #11529 #13084

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

abdulazizali77
Copy link
Contributor

What? Why?

When the order exchange is removed, the variant enterprise fees cannot be summarized. The change uses the order line item adjustment information to get the variant enterprise fee ids instead.

What should we test?

  • Create an order with per item enterprise fee. Run the tax report by producer enterprise fees and note the resulting enterprise fees.
  • Remove the exchange of the order cycle of that order. Rerun the tax report by producer enterprise fees and note that the resulting enterprise fees are still the same/have not disappeared.

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

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

Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

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

Nice work. Thank you!

Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

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

Nice one !

@rioug rioug added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label Jan 21, 2025
@filipefurtad0 filipefurtad0 self-assigned this Jan 24, 2025
@filipefurtad0 filipefurtad0 added the pr-staged-fr staging.coopcircuits.fr label Jan 24, 2025
@filipefurtad0
Copy link
Contributor

Hey @abdulazizali77 ,

Many thanks for addressing this one!

I've reproduced the issue, before this PR:

Having the fee on the incoming section of the order cycle, adds it to the report:
image

Removing it from the order cycle, makes the fee untraceable and so, the report yields no results:

image

After staging this PR, I was expecting the fee to be seen again:

image

However, as seen in the pic above, this is not the case (I've double checked, that setting the fee back on the OC re-displays it in the report.)

What do you think @abdulazizali77 ? Would it be possible to have your opinion and perhaps a second look at this one?
Thank you much 🙏

@filipefurtad0 filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Jan 24, 2025
@abdulazizali77
Copy link
Contributor Author

@filipefurtad0 will look at it

@sigmundpetersen
Copy link
Contributor

Hi @abdulazizali77 👋
Did you want to continue on this one?

@abdulazizali77
Copy link
Contributor Author

abdulazizali77 commented Mar 20, 2025

Hi @abdulazizali77 👋 Did you want to continue on this one?

@sigmundpetersen While addressing the test failures, im only starting to scrutinize the logic of the By Producer report and looking at the other reports Summary and By Order both show Coordinator fees and Outgoing fees, but the By Producer only shows Incoming, why was that the case? At fitrst i thought only the Fees of Producer of the Variant/goods should be displayed but any Fee from any related Producer or Hub may be applied on incoming Variants, then why should any arbitrary fee applied in/as Outgoing and Coordinator Fee not be displayed in the By Producer report?

@sigmundpetersen
Copy link
Contributor

@filipefurtad0 @RachL @openfoodfoundation/reviewers @chahmedejaz any feedback on the above ☝️

@filipefurtad0
Copy link
Contributor

Hey @abdulazizali77,

why should any arbitrary fee applied in/as Outgoing and Coordinator Fee not be displayed in the By Producer report?

Only the incoming exchange section relates to the suppliers/producers (we use these terms interchangeably), so only the fees set in this section will appear at the By Producer Report.

See also the user guide for more info, I hope this helps.

It should be noted though, that currently, this report shows all fees set up on the incoming exchange, regardless of the owner of the fee (i.e., the enterprise which created it). In this aspect, Tax Report by Producer (left side, below) currently differs from the Pay your suppliers report (right side):

image

PS: and perhaps signalling @openfoodfoundation/train-drivers-product-owners, there are other differences between these reports, for example, setting up the producer to charge GST hides the tax parcels from the Pay your suppliers report, but leaves these visible, at the Tax Report By Producer, I wonder if we should not harmonize this behavior at some point:

image

Another difference is the present issue: removing the fee from the order cycle does not change the Pay your suppliers report 💪 It does so on Tax Report by Producer, as described on #11529.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
Status: In Progress ⚙
Development

Successfully merging this pull request may close these issues.

[Enterprise fees w/ Tax Report by Producer] Can't trace origin from enterprise fees
5 participants