Fix reimbursement_category_name ending up in a translation missing error

[elia]

Description

While debugging the reason for which after the upgrade to Solidus 2.9 an application was still marking store credit with the "Gift Card" store credit category, I found that Spree::StoreCreditCategory.reimbursement_category_name contained the string "translation missing: en.spree.store_credit_category.default", although the I18n key was already set to "Reimbursement".

The problem is that the I18n lookup is done before the Rails app is able to load the application translations.

Checklist:

• I have followed Pull Request guidelines
• I have added a detailed description into each commit message
• I have updated Guides and README accordingly to this change (if needed)
• I have added tests to cover this change (if needed)
• I have attached screenshots to this PR for visual changes (if needed)

[spaghetticode]

After some investigation with @elia we discovered the problem is generated when there's a decorator for the class Spree::StoreCreditCategory defined in the store code.

Basically the issue happens because decorator files are loaded before I18n translations are completely initialized.

[kennyadsl]

@spaghetticode not sure if you are saying that this not needed anymore and it's an issue of how you are decorating files in your application. If it's needed we can take a look at the default option of class_attribute but I think it does the exact same thing and won't solve the issue.

[spaghetticode]

@kennyadsl I think the fix is still needed, it happens to every store that uses a _decorator.rb file to decorate Spree::StoreCreditCategory.
I agree that using the default option will not solve the issue, though I did not try it... just guessing.

Elia's solution works great but is specific for this situation; I'd prefer a more generic solution that would work for all instances where this problem could happen, both present and future.

One solution that comes to my mind would be to load decorators after loading I18n translations... but not sure if this is feasible, so what we have now may be a good compromise... @elia any thoughts on this?

[elia]

@kennyadsl @spaghetticode I would avoid setting a value (unless static) until after the initialization, can be this way or via an actual initializer. So the options I see are:

• Lazily assign as it's done in this PR
• Move the default value to an initializer
• Use some sort of code instead of a display value like name to fetch the record
• Turn the configuration value into a proc that can be easily loaded and fully customizable without resorting to decorators, that BTW can be used to configure not just the name but the whole fetching strategy.

Since I also object to falling back to .first if no category is found I would say that the last option is superior because it can easily default to the current behavior and scale up to very thick code for fetching the reimbursement category.

[kennyadsl]

@elia maybe we can just redefine the getter? This way the preference would work with the default value no matter where you call it?

[elia]

@kennyadsl I changed route altogether and moved the responsibility of fetching the store credit category for a reimbursement to the reimbursement itself via Spree::Reimbursement#store_credit_category that in turn relies on Spree::Reimbursement.store_credit_category_proc.

Hope this makes more sense and looks cleaner (especially once we'll throw away the deprecated stuff).

[kennyadsl]

@elia did you have some time to take a look at the comments @aldesantis left? 🙏

[filippoliverani]

@elia did you have some time to take a look at the comments @aldesantis left? 🙏

I've agreed with @elia to take on the PR, I'm pushing some changes to address @aldesantis' comments 👍

[aldesantis]

@elia @filippoliverani this looks good overall. Could you guys just squash the commits? 🙏

[elia]

@elia @filippoliverani this looks good overall. Could you guys just squash the commits? 🙏

@aldesantis done ✅

[kennyadsl]

I generally dislike silencing deprecations outside of the specs. What if the method we are silencing calls another deprecated method?

Since it turned out that silence is not yet thread-safe, I like it even less.

I'd prefer to explore an alternative.

[DanielePalombo]

This is the changelog provided from @elia

Fixed how the default reimbursement store-credit category is fetched
Before this change the store-credit category for reimbursement was fetched by name using a missing translation (i.e. en.spree.store_credit_category.default) that resulted in the name "Default". If no category was found the code fell back on the first category from the database, which wasn't guaranteed to be the right one. Trying to update the translation to the desired category name was also useless due to how code was loaded.
Now it's possible to disable this legacy behavior and switch to a simpler one, in which the code will look for a CreditCategory named "Reimbursement". Here's a list of checks and fixes you can perform to ensure you can enable the correct implementation:

• If you don't use reimbursements you're good to go, skip below to Disabling the legacy behavior
• Ensure you din't decorate or patched any of the involved code, especially:
  • Spree::StoreCreditCategory.reimbursement_category
  • Spree::StoreCreditCategory.reimbursement_category_name
• Ensure your "production" environment is already returning the correct category, you can assess that by running this in your "production" console: Spree::StoreCreditCategory.reimbursement_category(nil).name == "Reimbursement"
  Disabling the legacy behavior
  If everything is sound, or you are ok with a different category name for newly created reimbursement store credits you can switch to the new behavior by configuring this Solidus preference in your spree.rb initializer:

Spree.config do |config|
  config.use_legacy_store_credit_reimbursement_category_name = false
end

If you had modifications in your codebase consider disabling the legacy behavior and porting them to a simple overwrite of Spree::Reimbursement#store_credit_category.
The legacy behavior will be removed in the next major version of Solidus.

[kennyadsl]

@aldesantis @spaghetticode can you please review again when you have time?

[spaghetticode]

Code looks good to me, I left a note for a possible improvement, but I'm blocking because the last commit needs to be squashed.

[spaghetticode]

I think you can use the new reimbursement trait here

[spaghetticode]

@elia no need to squash the second commit, looks legit 👍
Thank you!