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

Make ActiveJob locale aware #20800

Merged
merged 1 commit into from
Aug 4, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions activejob/CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,3 +1,10 @@
* Include I18n.locale into job serialization/deserialization and use it around
`perform`.

Fixes #20799.

*Johannes Opper*

* Allow `DelayedJob`, `Sidekiq`, `qu`, and `que` to report the job id back to
`ActiveJob::Base` as `provider_job_id`.

Expand Down
2 changes: 2 additions & 0 deletions activejob/lib/active_job/base.rb
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
require 'active_job/execution'
require 'active_job/callbacks'
require 'active_job/logging'
require 'active_job/translation'

module ActiveJob #:nodoc:
# = Active Job
Expand Down Expand Up @@ -60,6 +61,7 @@ class Base
include Execution
include Callbacks
include Logging
include Translation

ActiveSupport.run_load_hooks(:active_job, self)
end
Expand Down
7 changes: 6 additions & 1 deletion activejob/lib/active_job/core.rb
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ module Core

# ID optionally provided by adapter
attr_accessor :provider_job_id

# I18n.locale to be used during the job.
attr_accessor :locale
Copy link
Member

Choose a reason for hiding this comment

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

Don't you think that this is too general name? I'm currently upgrading to Rails 4.2.4 and getting errors like:

NoMethodError:
  private method `locale' called for #<SomeJob:0x007f1053505900>

In my job, I have a private method locale defined

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, it'll be tough to fix until another release. To me locale sounds like the most appropriate name.

end

# These methods will be included into any Active Job object, adding
Expand Down Expand Up @@ -68,7 +71,8 @@ def serialize
'job_class' => self.class.name,
'job_id' => job_id,
'queue_name' => queue_name,
'arguments' => serialize_arguments(arguments)
'arguments' => serialize_arguments(arguments),
'locale' => I18n.locale
}
end

Expand Down Expand Up @@ -96,6 +100,7 @@ def deserialize(job_data)
self.job_id = job_data['job_id']
self.queue_name = job_data['queue_name']
self.serialized_arguments = job_data['arguments']
self.locale = job_data['locale'] || I18n.locale
end

private
Expand Down
11 changes: 11 additions & 0 deletions activejob/lib/active_job/translation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
module ActiveJob
module Translation #:nodoc:
extend ActiveSupport::Concern

included do
around_perform do |job, block, _|
I18n.with_locale(job.locale, &block)
end
end
end
end
16 changes: 16 additions & 0 deletions activejob/test/cases/job_serialization_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,20 @@ class JobSerializationTest < ActiveSupport::TestCase
GidJob.perform_later @person
assert_equal "Person with ID: 5", JobBuffer.last_value
end

test 'serialize includes current locale' do
assert_equal :en, HelloJob.new.serialize['locale']
end

test 'deserialize sets locale' do
job = HelloJob.new
job.deserialize 'locale' => :es
assert_equal :es, job.locale
end

test 'deserialize sets default locale' do
job = HelloJob.new
job.deserialize({})
assert_equal :en, job.locale
end
end
20 changes: 20 additions & 0 deletions activejob/test/cases/translation_test.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
require 'helper'
require 'jobs/translated_hello_job'

class TranslationTest < ActiveSupport::TestCase
setup do
JobBuffer.clear
I18n.available_locales = [:en, :de]
@job = TranslatedHelloJob.new('Johannes')
end

teardown do
I18n.available_locales = [:en]
end

test 'it performs the job in the given locale' do
@job.locale = :de
@job.perform_now
Copy link
Member

Choose a reason for hiding this comment

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

Sorry if I'm missing something but since the original issue was on #perform_later, there should certainly be a test for it, no ?

However with the following:

test 'it performs the job in the given locale' do
  I18n.locale = :de
  TranslatedHelloJob.perform_later('Johannes')
  assert_equal "Johannes says Guten Tag", JobBuffer.last_value
end

the only failure is with Sucker Punch on my machine without your patch ; that seems strange.

Copy link
Member

Choose a reason for hiding this comment

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

@robin850 in the regular tests adapters are set to inline mode so when perform_later is called the adapters are executing the job immediately. SP probably still forks in inline mode. We did had some integration tests which starts adapters workers and I would test in there that the locale is set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robin850 Thanks for having a look at it. From my point of view the test 'it performs the job in the given locale' do does a good job to test this behavior: it serializes, deserializes and executes the job, expecting that the outcome is localized.

Could you outline which test you'd like to see? I'm happy to add it then! :)

Copy link
Member

Choose a reason for hiding this comment

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

Yep I agree, the test does a good job, we should keep it. My comment was just about adding a regression test for the case you're describing in your commit message (i.e. with #perform_later). Maybe we can add a test (under test/integration as pointed by Cristian) along the lines of:

test 'current I18n.locale is kept running #perform_later' do
  skip if adapter_is?(:inline)

  I18n.locale = :de
  TranslatedHelloJob.perform_later('Johannes')
  wait_for_jobs_to_finish_for(5.seconds)

  assert_equal "Johannes says Guten Tag", JobBuffer.last_value
end

What do you think ?

Copy link
Member

Choose a reason for hiding this comment

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

@xijo be aware that JobBuffer won't work on the integration tests as the jobs are executed on another process. You can use the existing system in place to write content to a file(see activejob/test/support/integration/dummy_app_template.rb, activejob/test/support/integration/test_case_helpers.rb, activejob/test/integration/queuing_test.rb)

assert_equal "Johannes says Guten Tag", JobBuffer.last_value
Copy link
Contributor

Choose a reason for hiding this comment

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

Way easier to spot 👍

end
end
17 changes: 17 additions & 0 deletions activejob/test/integration/queuing_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -68,4 +68,21 @@ class QueuingTest < ActiveSupport::TestCase
refute delayed_test_job.provider_job_id.nil?,
'Provider job id should by set for delayed jobs by provider'
end

test 'current locale is kept while running perform_later' do
skip if adapter_is?(:inline)

begin
I18n.available_locales = [:en, :de]
I18n.locale = :de

TestJob.perform_later @id
wait_for_jobs_to_finish_for(5.seconds)
assert job_executed
assert_equal 'de', job_output
ensure
I18n.available_locales = [:en]
I18n.locale = :en
end
end
end
10 changes: 10 additions & 0 deletions activejob/test/jobs/translated_hello_job.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
require_relative '../support/job_buffer'

class TranslatedHelloJob < ActiveJob::Base
def perform(greeter = "David")
translations = { en: 'Hello', de: 'Guten Tag' }
hello = translations[I18n.locale]

JobBuffer.add("#{greeter} says #{hello}")
end
end
6 changes: 5 additions & 1 deletion activejob/test/support/integration/dummy_app_template.rb
Original file line number Diff line number Diff line change
Expand Up @@ -8,13 +8,17 @@
JobsManager.current_manager.setup
CODE

initializer 'i18n.rb', <<-CODE
I18n.available_locales = [:en, :de]
CODE

file 'app/jobs/test_job.rb', <<-CODE
class TestJob < ActiveJob::Base
queue_as :integration_tests

def perform(x)
File.open(Rails.root.join("tmp/\#{x}"), "w+") do |f|
f.write x
f.write I18n.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.

@kaspth I added the integration test and used the fact that the content of the file wasn't asserted before. I thought that might be slightly better than adding a whole new job for this one test.

Copy link
Contributor

Choose a reason for hiding this comment

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

As long as it doesn't break other tests, this could probably work. What do you think, @cristianbica @robin850?

Copy link
Member

Choose a reason for hiding this comment

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

It's ok. We're not asserting the content of the file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool 👍

end
end
end
Expand Down
4 changes: 4 additions & 0 deletions activejob/test/support/integration/test_case_helpers.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,4 +45,8 @@ def wait_for_jobs_to_finish_for(seconds=60)
def job_executed
Dummy::Application.root.join("tmp/#{@id}").exist?
end

def job_output
File.read Dummy::Application.root.join("tmp/#{@id}")
end
end
13 changes: 13 additions & 0 deletions guides/source/active_job_basics.md
Original file line number Diff line number Diff line change
Expand Up @@ -280,6 +280,19 @@ UserMailer.welcome(@user).deliver_later
```


Internationalization
--------------------

Each job uses the `I18n.locale` set when the job was created. Useful if you send
emails asynchronously:

```ruby
I18n.locale = :eo
Copy link
Contributor

Choose a reason for hiding this comment

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

Add a newline after this.


UserMailer.welcome(@user).deliver_later # Email will be localized to Esparanto.
```


GlobalID
--------

Expand Down