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.

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

Scope config option to isolate translations #140

Merged
merged 2 commits into from
Dec 16, 2024
Merged

Conversation

vipera
Copy link
Contributor

@vipera vipera commented Dec 1, 2022

In my usage of this gem, I ran into a situation where I'd like two very related applications that are organizationally distinct but part of the same domain (e.g. public web application and its admin/management application) to connect to the same database.

However, I'd like them not to share translation data, and found that difficult to do without overriding methods in I18n::Backend::ActiveRecord. So instead, I added this feature to my fork, and wanted to see what you think about it and whether you'd be interested in having it in the main codebase.

Essentially I added a scope field to the migration and if the configuration option is set, then all reads/writes will be limited to only those records with that particular scope identifier. I wanted to maintain compatibility with existing tables that don't have a scope, or even newly created tables where someone wants to remove the scope field from the migration if they won't be using it.

I've added two tests showing how I think the behaviour should work and a readme paragraph. Thanks taking the time to look this over ☺️

@vipera vipera force-pushed the scope branch 2 times, most recently from 7bae168 to 91be629 Compare December 1, 2022 06:10
@vipera vipera changed the title Scope config option to isolate` translations Scope config option to isolate translations Dec 2, 2022
@timfjord
Copy link
Collaborator

timfjord commented Dec 5, 2022

It is looking good 👍🏻, there are some failed tests though

@vipera
Copy link
Contributor Author

vipera commented Dec 5, 2022

Thanks for your input, Tim!

I glanced over the pipeline logs and it looks like tests are failing on the master branch too for the same reasons. As usual, test pipelines stop being reliable after a while...

I can look into the details to try and fix it, but I don't think it's related to the change I made.

@timfjord
Copy link
Collaborator

timfjord commented Dec 5, 2022

Hmm, indeed, master is broken.
It would be great to fix the tests(if you have bandwidth of course) otherwise, I will try to fix master so we can go ahead with this is PR

- Add Rails 8 to test matrix
- Add excludes for incompatable Ruby/Rails combinations
- Fix sqlite3 dependencies for older Rails versions
Allow the entire backend to use translations only from a subset of
the translation records.
@vipera
Copy link
Contributor Author

vipera commented Dec 9, 2024

@timfjord I've rebased onto the latest master, but the CI pipeline had issues again so I went ahead and made some changes. It was mostly just an issue with sqlite3, which needed to be fixed to a specific version once again.

While I was at it, I added Ruby 3.3 and Rails 8 to the matrix. For Ruby head, I got an error I don't really understand for Rails 6 & 7, so I excluded these combinations from the matrix. Here's the error from the failing pipeline in case you want to take a look. Since it's head, it could change with the next commit, and I thought having a working pipeline for now might be the better option.

/home/runner/work/i18n-active_record/i18n-active_record/test/active_record_test.rb:177:in '<class:WithCustomTranslationModel>': uninitialized constant I18n::Backend::ActiveRecord (NameError)

    class CustomTranslation < I18n::Backend::ActiveRecord::Translation
                                           ^^^^^^^^^^^^^^
	from /home/runner/work/i18n-active_record/i18n-active_record/test/active_record_test.rb:176:in '<class:I18nBackendActiveRecordTest>'
	from /home/runner/work/i18n-active_record/i18n-active_record/test/active_record_test.rb:5:in '<top (required)>'
	from /home/runner/.rubies/ruby-head/lib/ruby/3.4.0+1/bundled_gems.rb:83:in 'Kernel.require'
	from /home/runner/.rubies/ruby-head/lib/ruby/3.4.0+1/bundled_gems.rb:83:in 'block (2 levels) in Kernel#replace_require'
	from /home/runner/work/i18n-active_record/i18n-active_record/vendor/bundle/ruby/3.4.0+1/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:21:in 'block in <main>'
	from /home/runner/work/i18n-active_record/i18n-active_record/vendor/bundle/ruby/3.4.0+1/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:6:in 'Array#select'
	from /home/runner/work/i18n-active_record/i18n-active_record/vendor/bundle/ruby/3.4.0+1/gems/rake-13.2.1/lib/rake/rake_test_loader.rb:6:in '<main>'

Other than that, I made almost no other changes, so I'd appreciate a merge if you're still satisfied with the implementation 😃

@timfjord timfjord merged commit b22bd9f into svenfuchs:master Dec 16, 2024
24 checks passed
@timfjord
Copy link
Collaborator

@vipera Thanks for your contribution and for fixing the CI 💚

@timfjord
Copy link
Collaborator

Released as v1.4.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants