Openfoodnetwork: [Rails 5.2] Failing specs on LocalizedNumber

[andrewpbrett]

There are a number of specs failing that look like

10) Calculator::FlexiRate extends LocalizedNumber behaves like a model using the LocalizedNumber module creates an error if the input to preferred_additional_item is invalid
192
      Failure/Error: expect(subject.errors[attribute]).to include(I18n.t('spree.localized_number.invalid_format'))
193
        expected [] to include "has an invalid format. Please enter a number."
194
      Shared Example Group: "a model using the LocalizedNumber module" called from ./spec/models/calculator/flexi_rate_spec.rb:36
195
      # ./spec/support/localized_number_helper.rb:20:in `block (3 levels) in <top (required)>'

They pass locally for me, however.

[andrewpbrett]

Okay, I can get these to fail locally by changing line 5 of localized_number_helper.rb to

allow(Spree::Config).to receive(:enable_localized_number?).and_return false

and running the spec. After changing it back to true, the spec still fails.

So it looks like that value is getting cached somehow.

[Matt-Yorkley]

Hmm... weird issues with Spree::Config and caching is pretty annoying.

I just had a look ahead in Spree 2.2 for some clues, and LocalizedNumber wasn't there! It was introduced much later in Spree 2.3, which means we must have backported it at some point. But... our implementation is quite different... :thinking:

Some background here: https://github.com/openfoodfoundation/openfoodnetwork/pull/1831

[andrewpbrett]

Well, good news... it turned out to not be related to cacheing 😂. The method (e.g. preferred_amount=) was never actually getting defined, because the localize_number method checks for connected? and does nothing if it's false....I'm still puzzling out why this works, and what it implies, but that PR should make a bunch of specs pass.

We could probably merge it to master if it turns out to be what we want to do.

[Matt-Yorkley]

That was added recently in the 5.2 branch to fix another crazy issue :see_no_evil:

This class calls #table_exists? which now causes the db:create task to fail fatally unless we use #connected? first as a guard. But yeah it looks like #connected? returns false in the test environment? Which is also a bit crazy...

[Matt-Yorkley]

Yep, rake db:create fails again now with that change :disappointed:

Caused by:
PG::ConnectionBad: FATAL:  database "open_food_network_test" does not exist
/home/user/Github/openfoodnetwork/lib/spree/localized_number.rb:17:in `block in localize_number'
/home/user/Github/openfoodnetwork/lib/spree/localized_number.rb:15:in `each'
/home/user/Github/openfoodnetwork/lib/spree/localized_number.rb:15:in `localize_number'
/home/user/Github/openfoodnetwork/app/models/calculator/flat_percent_item_total.rb:12:in `<class:FlatPercentItemTotal>'
/home/user/Github/openfoodnetwork/app/models/calculator/flat_percent_item_total.rb:7:in `<module:Calculator>'
/home/user/Github/openfoodnetwork/app/models/calculator/flat_percent_item_total.rb:6:in `<top (required)>'
/home/user/Github/openfoodnetwork/config/application.rb:94:in `block in <class:Application>'
/home/user/Github/openfoodnetwork/config/environment.rb:5:in `<top (required)>'
/home/user/.rbenv/versions/2.5.8/bin/bundle:23:in `load'

[andrewpbrett]

lol okay. So we could....

figure out why connected? returns false in the test env?
Throw a Rails.env != "test" in there? (just kidding... probably)
Rewrite the method to not call table_exists??
Redo the whole implementation to match Spree's? (also just kidding.... probably)

[Matt-Yorkley]

I can confirm connected? || Rails.env.test? fixes it, but...

none of these are great options.

[Matt-Yorkley]

The fact that #connected? returns false is bonkers, it means there's no database connection present at all when the app is being initialized in the test env. How can that be true? :sob: Maybe it's a DatabaseCleaner type issue?

[luisramos0]

Hello :wave:
What about just removing table_exists? and even column_names.include? (if necessary), if this method is called with the wrong attribute it will blow up at dev/test time anyway. In prod we wont see this being called with wrong attributes.

[Matt-Yorkley]

All the code in that #localize_number method is pretty hard to follow. I have a feeling it might be handling the fact that in some cases the methods passed to it are actual attributes on an AR model, and in some cases (like with preferences) they're not... they're delegated Spree::Preference methods, and might behave differently..?

[Matt-Yorkley]

The Spree version of this doesn't have the #localize_number helper at all, and changes the parsing behavior more explicitly in each model where it's used, like this:

def amount=(amount)
  self[:amount] = Spree::LocalizedNumber.parse(amount)
end

That way the methods aren't "automagically" rewritten at runtime via this mess...

But... interestingly Spree doesn't ever use LocalizedNumber on _preferences_...

[Matt-Yorkley]

Confirmed; removing table_exists? and column_names.include? fixes the db:create error but breaks the app code :x:

[Matt-Yorkley]

Also confirmed; replacing localize_number :attribute in AR models with the alternate attribute= setters (see above) fixes the related specs there :heavy_check_mark:, but it doesn't work with preferences (used in Calculators) :x:

[Matt-Yorkley]

There's the conservative "plaster over the cracks and hope it doesn't fall apart" approach, and then there's the "smash it apart and rebuild it" approach, and I think we might need to switch to the latter here with LocalizedNumber...

[luisramos0]

LOLOL
It made me think of the portuguese words for those things: picareta ou marreta. Funny words.

Can we use inheritance and create two localized number classes: LocalizaedNumberPreference and LocalizaedNumberAttribute? Just an idea.

[Matt-Yorkley]

Oooo... I think I've got a not-completely-terrible but still "plaster over the cracks" type solution: https://github.com/openfoodfoundation/openfoodnetwork/pull/7375

In situations like this my dad used to say: "it might not be perfect... but it's _close enough for Jazz_" :smile:

What do you think?