Skip to content

Resolve "Fix translation of visibility levels" [RUN ALL RSPEC]

Manoj M J requested to merge 323233-fix-translation-of-visibility-levels into master

What does this MR do?

Fixes #323233 (closed), by using the s_() method for translation instead of using the N_() method.

Premise

I went digging into what actually is N_() and why we use N_() in some places for translation

I found:

  • This from the docs of the gem we use (fast_gettext)
N_() and Nn_(): make dynamic translations available to the parser.
In many instances, your strings will not be found by the ruby parsing. These methods allow for those strings to be discovered.

N_("active"); N_("inactive"); N_("paused") # possible value of status for parser to find.
Nn_("active", "inactive", "paused")        # alternative method
_("Your account is %{account_state}.") % { account_state: _(status) }

I really did not understand what they've meant by this so I went digging again.

  • I found this after a bit of Googling and it says
Translating Constants
Use N_() for translating Ruby constants, it does not translate the string at runtime but it will be found when creating the pot file. Then use _() when it needs to be translated later:

class Foo
  # ERROR_MSG will not be translated, but the string will be found by gettext
  # when creating the .pot file
  ERROR_MSG = N_("Something failed")
end

# translate the string
puts _(Foo::ERROR_MSG)
change_language
# here it will correctly use the new language
puts _(Foo::ERROR_MSG)

So, as I understand it, we need to use N_() only in cases of constant, because

  • N_() marks the string for translation, but does not translate it immediately on run time.
  • Words marked with N_(), needs to be translated again with _() wherever it is used for proper translation according to the user's current locale.

So, if we had used s_() instead of N_() for constants, it would have translated it immediately on loading of the class, which means that it will always remain in English. (if the default locale is set to en while booting up the Rails app)

Conclusion

So, now I wondered whether this rule would apply to class methods too, as Gitlab::VisibilityLevel.options is a class method.

To make sure N_() isn't needed for class methods, I did this in Rails console

Screen_Shot_2021-03-04_at_1.36.56_PM

So, it appears that is is safe to mark class methods for direct translation using s_() and not go via the process of

  • mark for translation with N_(),
  • and later translate with the use of s_()

I also grepped the codebase to make sure that we do not have some place else where we do something like

text = 'Internal'

s_("VisibilityLevel|#{text}")

so it seems safe to make this change.

Result

Before:


Gitlab::VisibilityLevel.options.keys

# ["VisibilityLevel|Internal", "VisibilityLevel|Private", "VisibilityLevel|Public"]

After:


Gitlab::VisibilityLevel.options.keys

# ["Internal", "Private", "Public"]

PS: Interestingly, I found another nice, right usage of N_() apart from usage in constants here. 💯

Screenshots (strongly suggested)

Does this MR meet the acceptance criteria?

Conformity

Availability and Testing

Security

If this MR contains changes to processing or storing of credentials or tokens, authorization and authentication methods and other items described in the security review guidelines:

  • Label as security and @ mention @gitlab-com/gl-security/appsec
  • The MR includes necessary changes to maintain consistency between UI, API, email, or other methods
  • Security reports checked/validated by a reviewer from the AppSec team
Edited by Manoj M J

Merge request reports