Move all Calculators from spree to OFN and out of the Spree namespace #5613

luisramos0 · 2020-06-15T20:32:28Z

What? Why?

Closes #5687
Something I wanted to do for a long time.
We move all needed code from core/app/models/spree/calculator to OFN. And then we move what's in app/models/spree/calculator to app/models/calculator where there are already 2 calculators 👌

What should we test?

First of all we need to verify that the calculators defined before this PR is deployed will still work correctly after the deployment of this PR. For example, create an order with some fees (with specific calculators, for example, flat_rate). After deploying this PR, go to admin/orders and press "Update and recalculate fees", the fees should still be correct.

We need to make a simple verification of each calculator in each part of the app where they are used. Maybe not a multiplication of these two things but for example:

test FlatPercentItemTotal and FatRate and PerItem calculators on a shipping method
test PriceSack on a payment method
test FlexiRate on an enterprise fee
test DefaultTax calculator in Tax Rates admin/tax_rates/ verify the tax is still applied. The code was just moved, not changed, so we don't really need to test all the details here.

Release notes

Changelog Category: Changed
Simplified a lot the calculators code used to calculate taxes and fees in OFN.

Matt-Yorkley · 2020-06-17T11:13:22Z

Relevant failing specs and a merge conflict, moving to in dev. Looks good though 😄

luisramos0 · 2020-06-17T18:52:31Z

yes, there was a problem in the last refactoring and also conflicts with #5492, all resolved now. Ready for review again.

sauloperez

Neat! 👏

db/migrate/20200616162646_move_all_calculators_outside_the_spree_namespace.rb

luisramos0 · 2020-06-23T10:26:40Z

rebased to resolve conflicts.

luisramos0 · 2020-06-27T10:59:42Z

rebased to resolve conflict in schema.rb

Matt-Yorkley · 2020-07-07T10:21:13Z

More conflicts...

…to be used by the app and make spree specs use calculators outside the spree namespace

spree code and is not necessary here now)

luisramos0 · 2020-07-07T16:27:59Z

ok, rebased to resolve conflicts. Ready for review again.

luisramos0 · 2020-07-07T16:52:39Z

build is good, just flaky.

sauloperez · 2020-07-09T15:00:39Z

I just rebuilt it to get a green build. I think we can move it to test ready afterwards.

luisramos0 · 2020-07-09T15:18:21Z

It needs a second review, right?

sauloperez · 2020-07-09T17:15:46Z

yes, it does but... last time we heard from someone else was 22 days ago and I think most of the changes are rather simple.

luisramos0 · 2020-07-09T19:43:51Z

🎉 🎉 🎉 🎉

filipefurtad0 · 2020-07-13T13:39:21Z

Hey @luisramos0 ,

Thank your for the detailed testing notes!

Before staging this PR I've set enterprise fees, of all available kinds:

p1 - Flat percent (per item)
p2 - Weight (per kg)
p3 - Flat rate (per order)
p4 - Flat rate (per item)
p5 - Flexible rate (per item)
p6 - Price Sack

Test cases/steps:

Added these fees to the respective Order Cycle and placed an order (5 items of each product).
Checked the order confirmation

Then, I staged this PR:

Clicked "Updated and recalculate fees" on the previously placed order: the order total remains the same as before - OK
Placed a similar order as before (5 items of each product): the order total remains the same as before - OK
Observed that the fees applied per order - p3, p4, p6 - appear as "Admin & Handling" on /cart /checkout and invoice - OK

After setting "charges GST" in the for that hub, and by changing the Tax category of the different enterprise fees (inheriting from product or others) apply taxes over:

products and enterprise fees
only products
only enterprise fees

This worked as expected.

Also checked if calculators still work correctly on shipping/payment methods. I followed your testing notes:

checked FlatPercentItemTotal and FatRate and PerItem calculators on a shipping method
tested PriceSack and FlexibleRate on a payment methods

The calculators rendered the expected value for each order; fees operate on the price per item without including the respective enterprise fee, for example, Price Sack, or flat-percent - this is a known issue:

"flat percent" calculator in shipping/payment method only applies to products total (excl. ent. fee) Flat percent calculator in shipping/payment method only applies to products total (excl. ent. fee) #2003
Also, to check cross functionality (between this PR and previous versions of the app) I recalculated various orders, from different hubs, as a superadmin. Everything looked ok.

This PR really touches a lot of corners of the app!
But looks ready to go, so one step closer to "bye bye Spree" 🎉
Thanks Luis!

Please note that this PR was tested with the following failing spec:

rspec ./spec/models/variant_override_spec.rb:36 # VariantOverride scopes fetching variant overrides indexed by variant does not include overrides for soft-deleted variants

luisramos0 · 2020-07-13T14:16:40Z

bam 💥 , merged
thanks Filipe!

sauloperez · 2020-07-13T14:18:50Z

luisramos0 changed the title ~~Calculators~~ Move all Calculators from spree to OFN and out of the Spree namespace Jun 15, 2020

luisramos0 self-assigned this Jun 15, 2020

luisramos0 marked this pull request as draft June 15, 2020 21:23

luisramos0 marked this pull request as ready for review June 16, 2020 16:51

luisramos0 force-pushed the calculators branch 2 times, most recently from fbfc5a8 to 2f28c39 Compare June 17, 2020 18:52

luisramos0 mentioned this pull request Jun 17, 2020

[Rails 4.1] Fix spec/models/spree/adjustment_spec.rb (4 broken specs) #5632

Closed

sauloperez approved these changes Jun 23, 2020

View reviewed changes

db/migrate/20200616162646_move_all_calculators_outside_the_spree_namespace.rb Show resolved Hide resolved

luisramos0 force-pushed the calculators branch 2 times, most recently from c2c5090 to 1da1a39 Compare June 23, 2020 10:26

luisramos0 force-pushed the calculators branch from 1da1a39 to 249755c Compare June 27, 2020 10:59

luisramos0 added 14 commits July 7, 2020 17:20

Bring calculators from spree_core

c2898ba

Fix indentation and extract Calculator as a separate module declaration

6b04df0

Merge decorators into original classes brought from spree_core

f2a46d2

Fix some rubocop issues

a544102

Remove dead code related to promotions, we dont have promotions in OFN

9c3eb37

Move all calculators outside the spree namespace

5e8438c

Move calculator specs out of the spree namespace

9c7cb23

Remove Spree namespace from DefaultTax

6a94168

Define DefaultTax calculator outside of spree namespace the tax_rate …

f625462

…to be used by the app and make spree specs use calculators outside the spree namespace

Migrate calculators to outside spree namespace

e225c5c

Make migration a bit easier to read

eedf31e

Extract line_items_total from compute_order

4b12a5f

Extract per_item_fees_total out of compute_order

21120dd

Extract per_order_fees_total out of compute_order

b096717

luisramos0 added 7 commits July 7, 2020 17:26

Extract applicable_rate? and thus resolve rubocop complexity issue

fbe0a32

Early exit if max is zero to make method shorted

70432e3

Remove unnecessary variable (this sum and += come from the original

68359d4

spree code and is not necessary here now)

Extract compute_for and thus resolve the rubocop complexity issue

d92f977

Fix rubucop issue in enterprise_fee model

0b79e7c

Convert calculators in new spec

99e12b6

Fix long lines in new spec

5e6739c

luisramos0 force-pushed the calculators branch from 249755c to 5e6739c Compare July 7, 2020 16:27

Matt-Yorkley approved these changes Jul 9, 2020

View reviewed changes

filipefurtad0 self-assigned this Jul 12, 2020

filipefurtad0 added pr-staged-fr staging.coopcircuits.fr and removed pr-staged-fr staging.coopcircuits.fr labels Jul 12, 2020

filipefurtad0 removed the pr-staged-fr staging.coopcircuits.fr label Jul 13, 2020

luisramos0 merged commit 9c8318d into openfoodfoundation:master Jul 13, 2020

luisramos0 deleted the calculators branch July 13, 2020 14:16

luisramos0 mentioned this pull request Jul 28, 2020

Orders showing payment state as 'paid' when clicked into they change to 'pending' #5836

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move all Calculators from spree to OFN and out of the Spree namespace #5613

Move all Calculators from spree to OFN and out of the Spree namespace #5613

luisramos0 commented Jun 15, 2020 •

edited

Loading

Matt-Yorkley commented Jun 17, 2020

luisramos0 commented Jun 17, 2020

sauloperez left a comment

luisramos0 commented Jun 23, 2020

luisramos0 commented Jun 27, 2020

Matt-Yorkley commented Jul 7, 2020

luisramos0 commented Jul 7, 2020

luisramos0 commented Jul 7, 2020

sauloperez commented Jul 9, 2020

luisramos0 commented Jul 9, 2020

sauloperez commented Jul 9, 2020 •

edited

Loading

luisramos0 commented Jul 9, 2020

filipefurtad0 commented Jul 13, 2020

luisramos0 commented Jul 13, 2020

sauloperez commented Jul 13, 2020

Move all Calculators from spree to OFN and out of the Spree namespace #5613

Move all Calculators from spree to OFN and out of the Spree namespace #5613

Conversation

luisramos0 commented Jun 15, 2020 • edited Loading

What? Why?

What should we test?

Release notes

Matt-Yorkley commented Jun 17, 2020

luisramos0 commented Jun 17, 2020

sauloperez left a comment

Choose a reason for hiding this comment

luisramos0 commented Jun 23, 2020

luisramos0 commented Jun 27, 2020

Matt-Yorkley commented Jul 7, 2020

luisramos0 commented Jul 7, 2020

luisramos0 commented Jul 7, 2020

sauloperez commented Jul 9, 2020

luisramos0 commented Jul 9, 2020

sauloperez commented Jul 9, 2020 • edited Loading

luisramos0 commented Jul 9, 2020

filipefurtad0 commented Jul 13, 2020

luisramos0 commented Jul 13, 2020

sauloperez commented Jul 13, 2020

luisramos0 commented Jun 15, 2020 •

edited

Loading

sauloperez commented Jul 9, 2020 •

edited

Loading