Clean up unused calculator currency data

[dacook]

What? Why?

• Closes Fee calculators: Remove the currency input field #10272

This deletes currency data that was hidden in #10318

Dev test

Run the migration and query the db to check results (see below comment)

What should we test?

This deletes values from the database directly, filtering for "calculator" and "currency".

Check that other preferences are still present, particularly for calculators (eg the saved enterprise fee 'amount' still shows after deployment)

Release notes

Changelog Category: Technical changes

Dependencies

• Remove unused currency from calculators #10318
• 10272 Remove currency check on shipping methods #10510

[filipefurtad0]

Hey @dacook ,

Signalling some conflicts in this PR - could you address them please? Thanks!

[dacook]

Thanks Filipe, done.
However I didn't mean to put this in Test Ready. It shouldn't be merged until #10318 has been deployed first, so I will move back to In Dev until the next release.

[dacook]

DB query / Dev test

I thought it worth demonstrating how I tested the query.

1. List all calculator preferences
You can see calculator currency, but also amounts and flat rates.
Note that some values are serialised as YAML, and others as a single value. Doesn't seem to matter though.

openfoodnetwork=> select id, key, value_type, substr(value, 0, 32) as value from spree_preferences where key like '/calculator%' limit 5;
  id   |                         key                         | value_type |   value
-------+-----------------------------------------------------+------------+-----------
 15646 | /calculator/flat_rate/currency/1826                 | string     | --- AUD  +
       |                                                     |            | ...      +
       |                                                     |            |
 15647 | /calculator/flat_rate/amount/1826                   | decimal    | --- 100.0+
       |                                                     |            | ...      +
       |                                                     |            |
 15644 | /calculator/per_item/currency/2009                  | string     | --- AUD  +
       |                                                     |            | ...      +
       |                                                     |            |
 15645 | /calculator/per_item/amount/2009                    | decimal    | --- 5.0  +
       |                                                     |            | ...      +
       |                                                     |            |
    65 | /calculator/flat_percent_item_total/flat_percent/28 | decimal    | 5.0

2. Limit query to currency only
Only currency preferences are selected, yay!

openfoodnetwork=> select id, key, value_type, substr(value, 0, 32) as value from spree_preferences where key like '/calculator%/currency/%' limit 5;
  id   |                 key                 | value_type |  value
-------+-------------------------------------+------------+---------
 15646 | /calculator/flat_rate/currency/1826 | string     | --- AUD+
       |                                     |            | ...    +
       |                                     |            |
 15644 | /calculator/per_item/currency/2009  | string     | --- AUD+
       |                                     |            | ...    +
       |                                     |            |
   317 | /calculator/flat_rate/currency/357  | string     | AUD
   337 | /calculator/per_item/currency/369   | string     | AUD
   373 | /calculator/flat_rate/currency/398  | string     | AUD

3. After running migration on dev
All currency preferences are gone:

open_food_network_dev=# select id, key, value_type, substr(value, 0, 32) as value from spree_preferences where key like '/calculator%/currency/%' limit 5;
 id | key | value_type | value
----+-----+------------+-------
(0 rows)

All other preferences still exist:

open_food_network_dev=# select id, key, value_type, substr(value, 0, 32) as value from spree_preferences where key like '/calculator%' limit 5;
  id   |                         key                         | value_type |   value
-------+-----------------------------------------------------+------------+-----------
 15647 | /calculator/flat_rate/amount/1826                   | decimal    | --- 100.0+
       |                                                     |            | ...      +
       |                                                     |            |
 15645 | /calculator/per_item/amount/2009                    | decimal    | --- 5.0  +
       |                                                     |            | ...      +
       |                                                     |            |
    65 | /calculator/flat_percent_item_total/flat_percent/28 | decimal    | 5.0
    71 | /calculator/flat_percent_item_total/flat_percent/40 | decimal    | 100.0
    64 | /calculator/flat_percent_item_total/flat_percent/27 | decimal    | 140.0
(5 rows)

[jibees]

Thanks! (and what a nice how to test section! 👏 )

As it delete data, I'd invoke a second review on that one!

[mkllnk]

Great. Such a simple pull request and I still find things to talk about. 😉

I was just about to approve this and then I was wondering if the code is still storing the default currency here. How do we test that this is safe? Do we add a database condition on the key name and see if the specs blow up? That would only be a temporary test. Tricky. 🤷

And I found a file which looks like it's still using this value: engines/order_management/app/services/order_management/stock/estimator.rb

Could you have another look?

[mkllnk]

This query is so simple and so minimal on Rails use that I would have executed plain SQL. But this is good, too. I think it's just a recent preference of mine after years of trying to railsify everything I'm now trying to reduce everything to the bare minimum. And I think that execute would have given you the output of rows deleted out of the box.

[dacook]

Good point, using ActiveRecord doesn't give any advantage here, although perhaps helps avoid errors in writing SQL directly.
I can see that's what Ana did recently too.

[mkllnk]

I often wonder what's best practice here. I tend to not raise an error. So if I want to repeat it, I can rollback and migrate without problem. Or maybe you want to roll back to an older migration and don't need this.

It doesn't really matter, I'm bike-shedding here, but I think that when the rollback doesn't do any harm either then we don't need to raise. If you did raise though then you can pass a message to the error instead of writing a comment. Or do a puts to explain why we can't reverse.

[dacook]

That's a good point too. Because I'm here now, I'll update it.

[dacook]

And I found a file which looks like it's still using this value: engines/order_management/app/services/order_management/stock/estimator.rb

🤭 Oh my, good find!
From what I can see, this is presence-safe and only filters out shipping methods if there's a preference that conflicts with it. So if there's any current conflicts, removing this code, or the preference data, would result in it appearing unexpectedly.

Removing code is undo-able, so we should test and deploy that change before deleting data. I'll create a new PR. I should have known this cleanup would blow out..

I've also searched the codebase more thoroughly now, and cannot find any other remaining references to a calculator preference called 'currency'.