Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Use column_fallback plugin for storing default locale's translations in original columns #12040

Merged
merged 12 commits into from
May 16, 2024
Merged

Use column_fallback plugin for storing default locale's translations in original columns #12040

merged 12 commits into from
May 16, 2024

Conversation

mad-eel
Copy link
Contributor

@mad-eel mad-eel commented May 14, 2024

Scope:

@mad-eel
Use column_fallback plugin from mobility:
6131234 
* Use original column for the translatable fields in the default locale
* Remove migrations for transferring and deleting translatable data
* Fixes #12019
mad-eel
mad-eel commented May 15, 2024
View reviewed changes
core/lib/spree/core/controller_helpers/locale.rb
@@ -28,6 +28,10 @@ def set_fallback_locale
Spree::Locales::SetFallbackLocaleForStore.new.call(store: current_store)
end

def default_locale
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@damianlegawiec Shouldn't we assign the default_locale to I18n.default_locale here: https://github.com/spree/spree/blob/main/core/lib/spree/core/controller_helpers/locale.rb#L21?

If a store has the default_locale set to pl then the current_locale will also be pl (unless picked otherwise). So we're still not using any translations and the I18n.default_locale should also be pl.

@mad-eel mad-eel force-pushed the feature/use-original-columns-for-default-translations branch from d051d79 to 129700b Compare May 16, 2024 07:54
@damianlegawiec damianlegawiec marked this pull request as ready for review May 16, 2024 09:51
@damianlegawiec damianlegawiec merged commit dd13beb into main May 16, 2024
13 of 15 checks passed
@pkatek
Copy link

pkatek commented Jun 6, 2024

@damianlegawiec @mad-eel But here's where the error went :/
See that the old migrations have been removed in the application and there is no unique_permalink_per_locale for new stores, but for old stores that are updating, it is already gone because they have a migration that assumes this change installed. And now there is a problem because I have this index set up unique_permalink_per_locale in spree_taxon_translations, to fix it now I have to do my own migration which will remove the indicated indexes and this will cause problems with other parameters in the future. In the same PR, there were also changes regarding the name and other translation parameters. So there has been a huge discrepancy between the store I had on 4.7. But we have a problem.

@pkatek
Copy link

pkatek commented Jun 6, 2024

As a general rule, migrations must not be removed in applications. You just need to add a new one that removes the previous change. Currently, if someone has created a store, they can't fix it without reinstalling the store.

@pkatek
Copy link

pkatek commented Jun 7, 2024 •
edited 

class RemoveUniquePermalinkPerLocaleIndexFromSpreeTaxonTranslations < ActiveRecord::Migration[7.1]
  def self.up
    if index_exists?(:spree_taxon_translations, [:locale, :permalink], name: 'unique_permalink_per_locale', unique: true)
      remove_index :spree_taxon_translations, name: 'unique_permalink_per_locale'
    end
  end

  def self.down
    add_index :spree_taxon_translations, [:locale, :permalink], unique: true, name: 'unique_permalink_per_locale', if_not_exists: true
  end
end

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@damianlegawiec damianlegawiec damianlegawiec approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Same Taxonomies in different stores
3 participants
@mad-eel @pkatek @damianlegawiec