refactor(mailer): Increase comprehensibility of device description

[RPraneetha]

fix #193

[rfk]

Hi @RPraneetha, thanks for the contribution!

There's one tricky part of this bug that we should have said more words about over in #193 - localization. The current device description string is not localized because it doesn't contain any additional words. Now that we're saying "X on Y", we need to connect this to the translation machinery so that it can be localized into all supported locales.

Take a look at the _constructLocationString further down in the file for an example of what we'll need to do here.

The logic for _formatUserAgentInfo will have to change to do something similar, along the lines of:

• If there's a value for uaBrowser but not uaOS, then just return that as a string
• If there's a value of uaOS but not uaBrowser, then just return that as a string
• If there's a value for both, use translator.format(translator.gettext('%(uaBrowser)s on %(usOS)'), { uaBrowser: uaBrowser, uaOS: uaOS }) to render then together as a localizable string

The call to translator.gettext will ensure that the new message gets picked up by our translation toolchain and provided to localizers.

There is also a suite of tests that will need to be updated to match the new logic, at [1].

The strings that @ryanfeeley provided in [2] might make good additional test cases, if they're not covered by the ones already there.

[1] https://github.com/mozilla/fxa-auth-mailer/blob/master/test/local/mailer_tests.js#L481
[2] https://github.com/mozilla/fxa-auth-mailer/issues/193#issuecomment-240537533

[rfk]

(By the way, there's no need to close this PR and open a fresh you, you can just make additional commits to your git branch and push them, and they'll show up as modifications to this PR)

[RPraneetha]

Okay! I'll get working on that!

[RPraneetha]

@rfk Something like this?

[vladikoff]

line 143 seems really long, i suggest adding a line break right after on %(uaOS)%(uaOSVersion)'),

[vladikoff]

hm interesting that there is no space between %(uaOS)%(uaOSVersion) but tests still have a space between OS and Version

[vladikoff]

need a space between else and {

[vladikoff]

need a space before {

[vladikoff]

Running "eslint:app" (eslint) task
/home/travis/build/mozilla/fxa-auth-mailer/mailer.js
  141:1  error  Trailing spaces not allowed  no-trailing-spaces
✖ 1 problem (1 error, 0 warnings)

[vladikoff]

bring else if to the same line as 148 so it is: } else if (uaOS) {

[vladikoff]

@RPraneetha seems like there are test failures:

test/local/db_tests.js ................................ 3/3 1s
test/local/mailer_tests.js ........................ 123/127
  test user-agent info rendering
  not ok should be equal
    +++ found                                                          
    --- wanted                                                         
    -Firefox on Windows 8.1                                            
    +Firefox on %(uaOS) %(uaOSVersion)                                 
    compare: ===
    at:
      line: 461
      column: 11
      file: test/local/mailer_tests.js
    stack: >
      test/local/mailer_tests.js:461:11

      test/local/mailer_tests.js:456:5

You can check that locally by using npm test from fxa-auth-mailer directory.

[RPraneetha]

Sorry, committed by accident! Was previewing changes but clicked on commit by mistake.

[vladikoff]

Sorry, committed by accident! Was previewing changes but clicked on commit by mistake.

No worries!

[vladikoff]

Thanks for fixing up the tests, going to review this now in detail! 👍

[vladikoff]

@RPraneetha thank you!