-
Notifications
You must be signed in to change notification settings - Fork 483
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
P20-548: Fix translation of Level#available_callouts
#58814
Conversation
5616257
to
5eabd25
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there unit test coverage for this scenario?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes look great!.
It looks like there is some test coverage, although it would be ideal to have a test case for this scenario.
@@ -1,5 +1,4 @@ | |||
require 'test_reporter' | |||
require 'rspec' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
7ebeacf
to
edad59b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some questions. Nice tests!
let(:callout_text) {'expecte_callout_text'} | ||
let(:callout_qtip_config) {'expectd_callout_qtip_config'} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit-pick: "expected" is misspelled on both lines.
callout_text = (should_localize? && | ||
I18n.t(i18n_key, default: nil)) || | ||
callout_definition['callout_text'] | ||
callouts_i18n = should_localize? ? I18n.t(name, scope: %i[data callouts], default: {}).with_indifferent_access : {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it the with_indifferent_access
which fixes the issue reported in the JIRA?
Could you explain the root cause of this bug?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
with_indifferent_access
allows hash values to be accessed using either symbol (:key
) or string ('key'
) key types interchangeably, eliminating the need to worry about which type of key is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is that callout_definition['localization_key']
can be an empty string, which shouldn't be allowed. Consequently, I18n.t("data.callouts.#{name}.#{callout_definition['localization_key']}")
will return a hash of all values associated with the one-level-higher key:
[development] dashboard > I18n.t("data.callouts.courseD_maze_ramp5.", locale: :'fr-FR')
=> {:""=>"Ces blocs n'ont pas de nombres sur eux. Tu peux en utiliser autant que tu veux !"}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the I18n library does not allow a key to be an empty string, I modified the code to retrieve the i18n string from the i18n hash instead (as, in Ruby, a hash key can be any object).
context 'if it is localizable and i18n string exists' do | ||
let(:level_is_localizable) {true} | ||
|
||
let(:callout_localization_key) {''} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this matches what is in the bug report, right? {"": "some string"}
1a4ebfa
to
a8edbf2
Compare
a8edbf2
to
3df4b32
Compare
91b3e3f
to
1856970
Compare
Screenshot
Links