Fix NoMethodError in Admin::ProductsV3#index - Only when using the hu.yml locale

[chahmedejaz]

What? Why?

• Closes NoMethodError in Admin::ProductsV3#index - Only when using the hu.yml locale #12739

• This PR looks for other areas where the following fix needs to be implemented:

• [BUU] fix error 500 on hungary instance #12700

• The reported issue is caused by the same issue fixed above, but in a different place

What should we test?

• Change the language to Hungarian ( or should be tested on Hungarian instance)
• Go to the new products page
• It should not throw the error

Release notes

Changelog Category (reviewers may add a label for the release notes):

• User facing changes
• API changes (V0, V1, DFC or Webhook)
• Technical changes only
• Feature toggled

[chahmedejaz]

HEADS-UP: In the future, whenever we are using Rails rounding off methods like number_to_rounded or number_with_precision with precision: nil, we would have to use significant: false as well. Otherwise, it would break for hu locale, and may break for any others we don't know yet. Thanks

[mkllnk]

According to the documentation, significant is false by default. So this wouldn't change anything. Do you have a test for this?

https://api.rubyonrails.org/classes/ActiveSupport/NumberHelper.html#method-i-number_to_rounded

[dacook]

This error keeps popping up on the Hungary instance when we don't explicitly set significant false (despite what the documentation says).
I don't think we've got to the bottom of the reason yet.
@rioug spent a little time trying to get to the bottom of it, I think he wasn't able to conclusively call it a Rails bug. So it's possibly something related to the OFN codebase somehow.

It does seem likely that this could happen again (anybody could use number_with_precision in the future, and not realise about this obscure issue). But I'm not sure how best to proceed. We could try monkey-patching, but that's not a great solution..

It would be better to understand why it's happening first. A couple of thoughts:

• replicate with a spec
• look further into the rails source code to understand what's happening

Note that there's a few HU-specific variables that could be related. I'm guessing it's the system locale:
https://github.com/openfoodfoundation/ofn-install/blob/master/inventory/group_vars/hu.yml

I started digging into the source code, but it seems that significant always defaults to false, so I don't know what's happening or where the locale comes into play.
https://github.com/rails/rails/blob/main/activesupport/lib/active_support/number_helper/number_converter.rb

[dacook]

In the interest of fixing the bug, I think we should go ahead with this fix, but it would be worth more investigation to see if we can find out the cause.

@chahmedejaz have you been able to replicate the issue in your development environment? Would you mind trying, in case you can find out the cause? (Maybe try a spike for one hour maximum?)

[chahmedejaz]

In the interest of fixing the bug, I think we should go ahead with this fix, but it would be worth more investigation to see if we can find out the cause.

@chahmedejaz have you been able to replicate the issue in your development environment? Would you mind trying, in case you can find out the cause? (Maybe try a spike for one hour maximum?)

Yes, David, when I set the locale to hu, it gets reproduced in my dev environment. Let me do a spike on whether it's the locale or our locale-specific configurations. Thanks

[filipefurtad0]

Hey @chahmedejaz,
some merge conflicts appeared for this PR. Can you take a look? Thanks 🙏

[chahmedejaz]

Hey @chahmedejaz, some merge conflicts appeared for this PR. Can you take a look? Thanks 🙏

Hi @filipefurtad0 - The conflict has been resolved. Thanks.

[chahmedejaz]

In the interest of fixing the bug, I think we should go ahead with this fix, but it would be worth more investigation to see if we can find out the cause.

@chahmedejaz have you been able to replicate the issue in your development environment? Would you mind trying, in case you can find out the cause? (Maybe try a spike for one hour maximum?)

Hi @dacook - I was able to reproduce this in the dev environment of our app and another one as well:

Moreover, I did a spike on the root cause, and here is the findings:
https://www.loom.com/share/1a9092cfde2e451b8388f9ca1aabd5da

Please let me know whether you want to take this PR as an opportunity to remove this gemx and remove our fix from all the places or if we should do that in another issue. Thanks.

Edit:

• Sorry, I just realized the rails-i18n is just a collection of locale-based data for formatting various things.
• Removing the gem may affect the locale-based number, currency, unit, or date-time formatting that our app might be using.
• Another solution to this maybe to add significant: false in our app's hu.yml or any other locale which may have significant: true in rails-i18n

[filipefurtad0]

Hey @chahmedejaz ,

Thanks for the heads up on this one. I've checked that:

• the PR deploys correctly
• the products page renders correctly with the locales in staging-FR - I can't really make test it for HU locale

So, on my end this would be good to go. But as just agreed via DM, I'll leave it up to you @chahmedejaz and reviewers to decide whether you wish to merge and go for this solution, or rather try something else. Moved to Ready to go, and added the feedback needed label.

Thanks!

[mkllnk]

Thanks for the investigation. That's really interesting.

I'll merge this pull request because it will fix a bug immediately.

I'm not sure about the way forward though. It's true that rails-i18n is not maintained by the Rails team, so it's not "official". But it does contain some localisation data that we may be missing otherwise. One example are error messages like "name can't be blank".

If we remove the rails-i18n gem, we may find more places were we need to add our own translations. That may be a good thing because then anyone can translate it with Transifex in the OFN context.

I'm worried that we may create more work for ourselves. The rails-i18n gem is actively maintained and contains a lot of knowledge. But then, looking at the reverse dependencies, I can't see any significant projects. So there are not many other (funded) organisations contributing to this.

So my vote would be for removing in a future PR. What do others think, @dacook, @rioug.

[chahmedejaz]

I'm not sure about the way forward though. It's true that rails-i18n is not maintained by the Rails team, so it's not "official". But it does contain some localisation data that we may be missing otherwise. One example are error messages like "name can't be blank".

Yes, exactly I just realized this, and looking at Rails default behavior and the hu.yml locale data in rails-i18n, I think it's a bug in the gem because it's overriding the default behavior. I'm opening an issue, maybe we could get this resolved in the gem itself or may have some thoughts on it from the maintainers.
Moreover, For the time being in the next PR, we could add number.format.significant to false in our app's hu.yml to make it work.

I'm worried that we may create more work for ourselves. The rails-i18n gem is actively maintained and contains a lot of knowledge. But then, looking at the reverse dependencies, I can't see any significant projects. So there are not many other (funded) organisations contributing to this.

Agreed

Sorry for the back and forth 😅

[dacook]

So my vote would be for removing in a future PR. What do others think, @dacook, @rioug.

I have to admit I didn't realise this was an unofficial gem. I'm unclear as to what benefit it brings: is it that it provides translations for Rails default validation messages? Or perhaps it does more..

I guess it helps fill in the gaps when managing an app that can be used in a large number of languages. Something that projects like ours really benefit from! But if larger internationalised projects aren't using it, then there must be drawbacks as well. So my questions are:

1. What benefits does this gem bring (what time is it saving for developers and/or translators)? It seems we're not clear, but it

   may affect the locale-based number, currency, unit, or date-time formatting that our app might be using.

2. Are we big enough that we can manage this ourselves without the help of the gem?

I don't know if it's worth investigating further. If Ahmed's issue is a simple bug fix, then hopefully we can keep on using the gem for several more years without further issues...

Until that issue gets resolved in the gem, I like the idea of adding the config in hu.yml to compensate. So I suggest we do what Ahmed suggests:

For the time being in the next PR, we could add number.format.significant to false in our app's hu.yml to make it work.

[rioug]

Thanks for getting to the bottom of this @chahmedejaz !
Like David said it's hard to know how useful the gem is, without knowing that I wouldn't want to remove the gem for now. I guess we could try to remove it and see which test fails and fixed them, but that's likely time consuming.

As much as it would be good to remove a dependency, I don't think we are in a position right now to do so. So I vote to keep it. Should we open at tech debt issue to track this ?

[chahmedejaz]

Thanks, David and Gaetan for the feedback.

And yes @rioug - I think we should open an issue for this one to track it. In that fix, we would be removing our existing fix from all the places and adding number.format.significant to false for hu.yml. Thanks