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

3283 [Enterprise Fee Summary] Receiver as "Fee Calc on Transfer Through" for incoming exchange fees #3319

Conversation

kristinalim
Copy link
Member

What? Why?

Relates to #3283

As discussed in #3283, for enterprise fees added to an incoming exchange, "Fee Calc on Transfer Through" should be the name of the sender (the producer).

What should we test?

  1. Set up an OC with different enterprises for producer, coordinator, and distributor.
  2. Set up an incoming and outgoing exchange for a variant with stock.
  3. Add one enterprise fee each for the producer, coordinator, and distributor to the incoming exchange and also to the outgoing exchange.
  4. Create and complete an order for the variant.
  5. Check the Enterprise Fee Summary. There should be 8 rows for the order. For all 3 rows marked "Incoming", "Fee Calc on Transfer Through" should be the name of the producer. For all 3 rows marked "Outgoing", it should be the name of the distributor.

Release notes

No release notes needed as this would be packaged with the rest of the initial changes for the Enterprise Fee Summary.

Changelog Category: -

Dependencies

This requires #3312 to be merged to the transitional branch first.

@kristinalim kristinalim self-assigned this Jan 13, 2019
@kristinalim kristinalim changed the base branch from master to feature/enterprise_fee_summary January 13, 2019 01:59
@kristinalim kristinalim force-pushed the feature-enterprise_fees_report-receiver_as_fee_calc_on_transfer_through_for_incoming branch from 21765e1 to 04024b8 Compare January 13, 2019 02:03
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.

What you describe sounds logical. But I don't understand the label "Fee Calc on Transfer Through". What does that mean?

@kristinalim
Copy link
Member Author

@mkllnk The exact definition (for some scenarios) is still being clarified in #3283, but:

  1. For fees for incoming exchanges - If the data is available, it should be the sender in the exchange.
  2. For fees for outgoing exchanges - If the data is available, it should be the receiver in the exchange.

@mkllnk
Copy link
Member

mkllnk commented Jan 16, 2019

Ah, now I get the problem. Does that mean that a producer can effectively charge for items of other producers that are sold in the same order cycle by applying a fee for the whole order?

Can we find the fee via Spree::Adjustment#originator and get the enterprise from there?

Could another name for this column be "Charged by"?

@kristinalim
Copy link
Member Author

Does that mean that a producer can effectively charge for items of other producers that are sold in the same order cycle by applying a fee for the whole order?

@mkllnk Yes this is currently the case.

Can we find the fee via Spree::Adjustment#originator and get the enterprise from there?
Could another name for this column be "Charged by"?

@mkllnk Were you thinking about the recipient of the fee? This data is already in "Enterprise Owner".

"Fee Calc on Transfer Through" I believe is to tell to which enterprise you'd want to attribute the fee... Still checking this in Issue #3319, and will address other scenarios in PR #3337.

@mkllnk mkllnk added the pr-staged-au staging.openfoodnetwork.org.au label Jan 22, 2019
@mkllnk mkllnk force-pushed the feature-enterprise_fees_report-receiver_as_fee_calc_on_transfer_through_for_incoming branch 2 times, most recently from 27f7e27 to 04024b8 Compare January 22, 2019 06:18
@mkllnk
Copy link
Member

mkllnk commented Jan 22, 2019

@kristinalim I'm sorry, but I just stuffed up your branch for a bit. I tried to stage this pull request and it failed on merging master into it. I was surprised, because this pull request didn't indicate any merge conflicts. Anyway, I thought I should rebase it on master and I resolved the conflict. Now Github complained about a merge conflict and when I saw that I realised what I did. I forgot that this pull request is based on the feature branch. I think the feature branch has a merge conflict with master.

Luckily there is git reflog and I could restore the original version of this branch. But I think your feature branch needs rebasing or merging to resolve the conflict. You may need to rebase or merge all the other connected branches then as well.

@kristinalim
Copy link
Member Author

@mkllnk I see. I don't have permission to force-push to a branch in this repository, unfortunately. Could you do it? I can take care of rebasing the open PRs upon this feature branch.

I rebased and pushed to my fork:

git remote add kristinalim git@github.com:kristinalim/openfoodnetwork.git
git fetch kristinalim
git checkout kristinalim/feature/enterprise_fee_summary
git push upstream +HEAD:feature/enterprise_fee_summary

@mkllnk mkllnk force-pushed the feature/enterprise_fee_summary branch from b7131b4 to 545d482 Compare January 22, 2019 23:10
@mkllnk
Copy link
Member

mkllnk commented Jan 22, 2019

Done. Slightly different commands:

git fetch git@github.com:kristinalim/openfoodnetwork.git feature/enterprise_fee_summary
git push upstream +FETCH_HEAD:feature/enterprise_fee_summary

Add test for more complex scenario where there is a coordinator and
distributor fee for an incoming exchange, and a producer and coordinator
fee for an outgoing exchange.
@kristinalim kristinalim force-pushed the feature-enterprise_fees_report-receiver_as_fee_calc_on_transfer_through_for_incoming branch from 04024b8 to d466a4b Compare January 23, 2019 14:52
@kristinalim
Copy link
Member Author

This has been rebased already, @mkllnk. Thank you!

@kirstenalarsen
Copy link
Contributor

Hi. have been testing this. The report is still not showing the info in 'fee calc on transfer through' but as far as I can see everything is calculating correctly. So I'm going to call it and say that we will put the report in without the whole order transfer through fields showing. We've spent way too much time on this!! Let's see if it is a problem for people actually using it

https://docs.google.com/document/d/1qdPDPIJACoGTv_9IcxUlf3cPw_J2CxMP-1dVhVkEsdk/edit#

@mkllnk mkllnk removed the pr-staged-au staging.openfoodnetwork.org.au label Jan 31, 2019
@mkllnk mkllnk merged commit 4a8e8fd into openfoodfoundation:feature/enterprise_fee_summary Jan 31, 2019
@kristinalim
Copy link
Member Author

@kirstenalarsen This particular PR was not meant to fix the "Fee Calc on Transfer Through" issue. I pinged for a second review of #3337 which should fix this - hoping we can get merge that in the transitional branch before merging everything to master.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants