3283 [Enterprise Fee Summary] Fix values when calculator is order-based #3337

kristinalim · 2019-01-16T13:06:58Z

What? Why?

Relates to #3283
Closes #2616

Because of constraints in the way we compute and store calculations for order-based calculators, there is no way currently to track the exchange to which an order-based exchange fee is associated, and to break down this fee per line item/variant/supplier.

So, we do not have an existing way to get "Fee Calc on Transfer Through" and "Tax Category" (when the Enterprise Fee itself does not have an associated tax category) for these exchange fees.

As proposed in #3283, we could use "Entire Orders through DISTRIBUTOR_NAME" for these order-absed exchange fees. For "Tax Category", we could use "Various" when there is no associated tax category to the enterprise fee.

What should we test?

Set up an OC with different enterprises for producer, coordinator, and distributor.
Set up an incoming and outgoing exchange for a variant with stock.
Fees for exchanges: For the incoming exchange and also for the outgoing exchange, add one enterprise fee with order-based calculator (such as "Flat Rate (per order)") for each of the producer, coordinator, and distributor. For testing purposes, one of these enterprise fees should assign a tax category, one should inherit the product's tax category, and one should have no tax category.
Coordinator fees: Add two "coordinator fees" with order-based calculator to the OC, not to an exchange. For testing purposes, one of these enterprise fees should assign a tax category, one should inherit the product's tax category, and one should have no tax category.
Create and complete an order for the variant.
Check the Enterprise Fee Summary.
For the 6 rows marked "Incoming" / "Outgoing":
- "Fee Calc on Transfer Through" for the exchange fees should show "Entire Orders through DISTRIBUTOR_NAME".
- For tax category:
  a. If the enterprise fee itself has a tax category, this should be used.
  b. If the enterprise fee inherits the product's tax category, this should be "Various".
  c. If the enterprise fee has no tax category, this should be blank.
For the rows marked "Coordinator":
- "Fee Calc on Transfer Through" should be "All". For tax category.
- For tax category: Same as in Step 6.

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 #3319 to be merged to the transitional branch first.

...ices/order_management/reports/enterprise_fee_summary/enterprise_fee_type_total_summarizer.rb

engines/order_management/app/services/order_management/reports/enterprise_fee_summary/scope.rb

sauloperez · 2019-01-22T12:32:07Z

...ement/reports/enterprise_fee_summary/data_representations/incoming_exchange_line_item_fee.rb

+module OrderManagement
+  module Reports
+    module EnterpriseFeeSummary
+      module DataRepresentations


What does this namespace provide other than making the nesting level pretty wide? These model classes won't clash with any other within the EnterpriseFeeSummary namespace, so I'd just remove DataRepresentations.

I disagree about removing DataRepresentations, @sauloperez.

The class ends up more deeply nested, but DataRepresentations I think makes it easier to find/ignore these classes and gives you an idea about their purpose.

If we reduce the nesting, I would want to rename the individual classes to reflect that they map DB query row attributes to report row attributes. For example, append "Representation" or "Presenter" (both terms actually from review of @mkllnk). Compared to this, I prefer nesting.

Let me know if you are okay with keeping this.

I like this way of using namespaces to organize your code! #java

to me, it's pretty obvious already. The class name is a noun named after a domain concept. What else could it be? But if you guys prefer it, I don't mind.

..._management/reports/enterprise_fee_summary/data_representations/associated_enterprise_fee.rb

.../order_management/app/services/order_management/reports/enterprise_fee_summary/summarizer.rb

spec/factories/orders.rb

sauloperez · 2019-01-22T12:43:59Z

spec/factories/orders.rb

+      after(:create) do |order, evaluator|
+        create(:line_item, order: order, variant: evaluator.variant)
+      end
+    end


do we really need this new trait? Spree itself has the order_with_line_items factory already. Other than that, this could be done by simply creating two of the three entities and letting FactoryBot create the third one. I'm just concerned about having too many purpose-specific factories that need to be maintained over time. I believe that is already the case in OFN.

I suppose you are talking about :order_with_totals_and_distribution:

factory :order_with_totals_and_distribution, :parent => :order do #possibly called :order_with_line_items in newer Spree

The problem I found with this is that it does too many things, including creating a product and a line item for the product. It's not reusable for the Enterprise Fee Report tests, which need explicit variant and line item setup. That's why I set up these factory traits which do fewer things for the factory object.

sauloperez · 2019-01-22T12:46:02Z

spec/factories/orders.rb

+      end
+    end
+
+    trait :completed do


Spree has the completed_order_with_totals factory, which we reused to build more complex ones in OFN. Can't we use just that one?

Same problem with this factory, @sauloperez.

engines/order_management/app/services/order_management/reports/enterprise_fee_summary/scope.rb

...ices/order_management/reports/enterprise_fee_summary/data_representations/coordinator_fee.rb

.../order_management/app/services/order_management/reports/enterprise_fee_summary/summarizer.rb

kristinalim · 2019-01-23T20:29:05Z

This is ready for another round of reviews. 🙂

mkllnk

Nice abstractions. :-)

kristinalim · 2019-01-30T10:06:47Z

I updated the issue description to also close issue #2616. This is the last PR addressing known issues in the report values.

kristinalim · 2019-01-31T16:52:54Z

Thanks, @luisramos0! Moving this to "Test Ready".

kirstenalarsen · 2019-02-09T02:27:08Z

YIHIYIPPEEEEIO!!! YES!!! Passed the tests and good to go! Fantastic test notes @kristinalim I was overwhelmed but when I looked closely and just worked through the steps it made perfect sense and was thorough and I am confident that it works. YAY! So excited! MERGEROONIE!

sauloperez · 2019-02-11T15:09:19Z

Sorry to ruin the party but I think it's better if you rebase it @kristinalim first

kristinalim · 2019-02-12T10:14:49Z

I've rebased, @sauloperez. Conflict now gone, and build is green. Thank you!

kristinalim · 2019-02-12T10:49:47Z

As discussed here #3415 (comment), @sauloperez I recommend merging #3415 first. And then I will change the base of this PR to master and rebase, to comply with the process. 👍

sauloperez · 2019-02-12T12:59:20Z

done. #3415 merged

This is not filling the "Fee Calc on Transfer Through" column. This will be addressed in another commit.

For exchange fees with calculator that is order-based: * "Fee Calc on Transfer Through" should show "Entire Orders through DISTRIBUTOR_NAME". * For tax category: a. If the enterprise fee itself has a tax category, this is used. b. If the enterprise fee inherits the product's tax category, this is "Various". c. If the enterprise fee has no tax, this is blank. For coordinator fees: * "Fee Calc on Transfer Through" should be "All". * For tax category: Same as abova.

The superclasses will include this module, instead of inheriting the class.

There is no more need to pass the summarizer.

kristinalim · 2019-02-12T16:29:13Z

Aha! 🙂 Finally, a green build.

I've rebased this already, and changed the base branch to "master". This is ready to go.

kristinalim self-assigned this Jan 16, 2019

kristinalim force-pushed the feature-enterprise_fees_report-order_based_calculator_bug branch from 8321c43 to 57ea1f9 Compare January 16, 2019 15:22

mkllnk reviewed Jan 16, 2019

View reviewed changes

...ices/order_management/reports/enterprise_fee_summary/enterprise_fee_type_total_summarizer.rb Outdated Show resolved Hide resolved

engines/order_management/app/services/order_management/reports/enterprise_fee_summary/scope.rb Show resolved Hide resolved

kristinalim mentioned this pull request Jan 17, 2019

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

Merged

kristinalim changed the title ~~3283 [Enterprise Fee Summary] (Pending) Fix values when calculator is order-based~~ 3283 [Enterprise Fee Summary] Fix values when calculator is order-based Jan 22, 2019

kristinalim mentioned this pull request Nov 18, 2022

[Enterprise Fee Summary] Rename join_scope in EnterpriseFeeSummary::Scope openfoodfoundation/wishlist#346

Open

sauloperez suggested changes Jan 22, 2019

View reviewed changes

mkllnk reviewed Jan 22, 2019

View reviewed changes

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

kristinalim force-pushed the feature-enterprise_fees_report-order_based_calculator_bug branch from ed8991a to 9e98b60 Compare January 23, 2019 19:28

mkllnk approved these changes Jan 24, 2019

View reviewed changes

This was referenced Jan 30, 2019

Build Enterprise Fee Summary Report #2616

Closed

[Enterprise Fee Summary] Merge "Enterprise Fee Summary" report to master #3415

Merged

HugsDaniel approved these changes Jan 31, 2019

View reviewed changes

luisramos0 approved these changes Jan 31, 2019

View reviewed changes

sauloperez approved these changes Feb 1, 2019

View reviewed changes

RachL added the pr-staged-au staging.openfoodnetwork.org.au label Feb 2, 2019

RachL assigned RachL and unassigned RachL Feb 2, 2019

RachL removed the pr-staged-au staging.openfoodnetwork.org.au label Feb 2, 2019

sigmundpetersen added the pr-staged-au staging.openfoodnetwork.org.au label Feb 8, 2019

RachL assigned kirstenalarsen Feb 8, 2019

luisramos0 force-pushed the feature/enterprise_fee_summary branch from 3200492 to b6be8c4 Compare February 8, 2019 21:27

kirstenalarsen removed the pr-staged-au staging.openfoodnetwork.org.au label Feb 9, 2019

kristinalim force-pushed the feature-enterprise_fees_report-order_based_calculator_bug branch from 9e98b60 to b85506d Compare February 12, 2019 09:04

kristinalim added 12 commits February 13, 2019 01:51

Add flat rate calculator traits to related factories

06ba518

Refactor SQL result summarizer for report

0212454

Add test for using order-based exchange fees

e550be9

This is not filling the "Fee Calc on Transfer Through" column. This will be addressed in another commit.

Clarify method name in report scope

8f65bee

Separate summarizing of SQL data by source

7a76355

Simplify name of SQL data summarizer class

373e3e1

Prepare to change AssociatedEnterpriseFee to module

b9c144b

The superclasses will include this module, instead of inheriting the class.

Include UsingEnterpriseFee instead of inheriting

664943a

Add descriptions for data representation classes

bc95560

Move data representation translations to class

ddc788e

Initialize data representation classes with data

5e8a336

There is no more need to pass the summarizer.

kristinalim force-pushed the feature-enterprise_fees_report-order_based_calculator_bug branch from b85506d to 5e8a336 Compare February 12, 2019 14:52

kristinalim changed the base branch from feature/enterprise_fee_summary to master February 12, 2019 14:52

sauloperez merged commit bda5eb1 into openfoodfoundation:master Feb 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3283 [Enterprise Fee Summary] Fix values when calculator is order-based #3337

3283 [Enterprise Fee Summary] Fix values when calculator is order-based #3337

kristinalim commented Jan 16, 2019 •

edited

sauloperez Jan 22, 2019

kristinalim Jan 23, 2019

luisramos0 Jan 31, 2019

sauloperez Feb 1, 2019

sauloperez Jan 22, 2019

kristinalim Jan 23, 2019

sauloperez Jan 22, 2019

kristinalim Jan 23, 2019

kristinalim commented Jan 23, 2019

mkllnk left a comment

kristinalim commented Jan 30, 2019

kristinalim commented Jan 31, 2019

kirstenalarsen commented Feb 9, 2019

sauloperez commented Feb 11, 2019 •

edited

kristinalim commented Feb 12, 2019

kristinalim commented Feb 12, 2019

sauloperez commented Feb 12, 2019

kristinalim commented Feb 12, 2019

3283 [Enterprise Fee Summary] Fix values when calculator is order-based #3337

3283 [Enterprise Fee Summary] Fix values when calculator is order-based #3337

Conversation

kristinalim commented Jan 16, 2019 • edited

What? Why?

What should we test?

Release notes

Dependencies

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kristinalim commented Jan 23, 2019

mkllnk left a comment

Choose a reason for hiding this comment

kristinalim commented Jan 30, 2019

kristinalim commented Jan 31, 2019

kirstenalarsen commented Feb 9, 2019

sauloperez commented Feb 11, 2019 • edited

kristinalim commented Feb 12, 2019

kristinalim commented Feb 12, 2019

sauloperez commented Feb 12, 2019

kristinalim commented Feb 12, 2019

kristinalim commented Jan 16, 2019 •

edited

sauloperez commented Feb 11, 2019 •

edited