656492 - Rename "Sync Key" to "Recovery Key"

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

Seems like a great idea to me. Consider.

Comment 1

[Alex Faaborg [:faaborg] (Firefox UX)]

Yeah, I was just thinking the same thing as well.  Now that we have j-pake, it is really only used in recovery situations.  Also, it makes the sentence:

"you should backup your recovery key"

make sense, all by itself.

Comment 2

[Richard Newman [:rnewman]]

Yeah, this was me filing your idea via philikon :D  I take no credit!

Comment 3

[Alex Faaborg [:faaborg] (Firefox UX)]

ah, well I concur with myself then :) (happens occasionally)

Comment 5

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v0) : Rename Key (WIP)

wip

Comment 6

[Philipp von Weitershausen [:philikon]]

Comment on attachment 552229 [details] [diff] [review]
Part 1 (v0) : Rename Key (WIP)

I only see string changes but no changes to the code that actually uses the strings (it will have to be changed whenever it uses a string that now has a differently named ID).


>diff --git a/browser/locales/en-US/chrome/browser/syncGenericChange.properties b/browser/locales/en-US/chrome/browser/syncGenericChange.properties
>--- a/browser/locales/en-US/chrome/browser/syncGenericChange.properties
>+++ b/browser/locales/en-US/chrome/browser/syncGenericChange.properties
>@@ -1,35 +1,35 @@
>-# LOCALIZATION NOTE (change.password.title): This (and associated change.password/passphrase) are used when the user elects to change their password.
>+LOCALIZATION NOTE (change.password.title): This (and associated change.password/passphrase) are used when the user elects to change their password.

Why did you remove the comment here?

> change.password.title = Change your Password
> change.password.acceptButton = Change Password
> change.password.status.active = Changing your passwordâ¦
> change.password.status.success = Your password has been changed.
> change.password.status.error = There was an error changing your password.
> 
>-change.password2.introText = Your password must be at least 8 characters long.  It cannot be the same as either your user name or your Sync Key.
>+change.password2.introText = Your password must be at least 8 characters long.  It cannot be the same as either your user name or your Recovery Key.

Changing a string means you also have to change its ID. You probably just want to increment number there (change.password3.introText). This of course means you also need to find all the places in code where the string is accessed.

>-change.synckey2.title = My Sync Key
>-change.synckey.acceptButton = Change Sync Key
>-change.synckey.label = Changing Sync Key and uploading local data, please waitâ¦
>-change.synckey2.error = There was an error while changing your Sync Key!
>-change.synckey2.success = Your Sync Key was successfully changed!
>+change.recoverykey2.title = My Recovery Key
>+change.synckey.acceptButton = Change Recovery Key
>+change.recoverykey.label = Changing Recovery Key and uploading local data, please waitâ¦
>+change.recoverykey2.error = There was an error while changing your Recovery Key!
>+change.reciverykey2.success = Your Recovery Key was successfully changed!

No need for the '2' if you're changing the ID already (synckey -> recoverykey). Also, you have a typo in the last one there.

>-save.synckey.title = Save Sync Key
>-save.default.label = Firefox Sync Key.html
>+save.recoverykey.title = Save Recovery Key
>+save.default.label = Firefox Recovery Key.html

Like above, you can't keep save.default.label. Needs to be something else. You can simply add a '2', e.g. save.default2.label, or you say save.recoverykey.defaultfilename or something which is actually a better way to describe the string.

>diff --git a/mobile/locales/en-US/chrome/sync.dtd b/mobile/locales/en-US/chrome/sync.dtd
>--- a/mobile/locales/en-US/chrome/sync.dtd
>+++ b/mobile/locales/en-US/chrome/sync.dtd
>@@ -9,14 +9,14 @@
> <!ENTITY sync.syncNow               "Sync Now">
> 
> <!ENTITY sync.setup.title           "Connect to Sync">
> <!ENTITY sync.setup.jpake           "From a Firefox Sync-connected computer, go to Sync options and select &#x0022;Add a device&#x0022;">
> <!ENTITY sync.fallback              "I'm not near my computerâ¦">
> <!ENTITY sync.setup.manual          "Enter your Sync account information">
> <!ENTITY sync.account               "Account Name">
> <!ENTITY sync.password              "Password">
>-<!ENTITY sync.syncKey               "Sync Key">
>+<!ENTITY sync.recoveryKey               "Recovery Key">

Please align the strings when possible.

> <!ENTITY sync.customServer          "Use custom server">
> <!ENTITY sync.serverURL             "Server URL">
> <!ENTITY sync.setup.connect         "Connect">
> <!ENTITY sync.setup.cancel          "Cancel">
> <!ENTITY sync.setup.tutorial        "Show me how">
>diff --git a/services/sync/locales/en-US/errors.properties b/services/sync/locales/en-US/errors.properties
>--- a/services/sync/locales/en-US/errors.properties
>+++ b/services/sync/locales/en-US/errors.properties
>@@ -1,28 +1,28 @@
> error.login.reason.network      = Failed to connect to the server
>-error.login.reason.synckey      = Wrong Sync Key
>+error.login.reason.recoverykey      = Wrong Recovery Key
> error.login.reason.account      = Incorrect account name or password
> error.login.reason.no_username  = Missing account name
> error.login.reason.no_password2 = Missing password
>-error.login.reason.no_synckey   = No saved Sync Key to use
>+error.login.reason.no_recoverykey   = No saved Recovery Key to use

The string ID is also an error value that's also defined in services/sync/modules/constants.js. You will have to change it there, too.

Comment 7

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v1) : Rename Key (WIP)

-no testing
  - *how* do I fully test this?
-could not find consuming code for (which makes me very suspicious):
  new.synckey.status.incorrect 
  setup.newSyncKeyPage.description.label
  change.synckey.sameAsSyncKey 
  change.synckey.sameAsPassword
  change.synckey.sameAsUsername
  change.synckey.sameAsEmail
  change.synckey.tooShort
  change.password.pwSameAsSyncKey

Comment 8

[Mike Connor [:mconnor]]

(In reply to :Ally Naaktgeboren from comment #7)

> -could not find consuming code for (which makes me very suspicious):
>   new.synckey.status.incorrect 
>   setup.newSyncKeyPage.description.label
>   change.synckey.sameAsSyncKey 
>   change.synckey.sameAsPassword
>   change.synckey.sameAsUsername
>   change.synckey.sameAsEmail
>   change.synckey.tooShort
>   change.password.pwSameAsSyncKey

I think all of the change.* are irrelevant once we stopped letting people manually create keys.

Comment 9

[Philipp von Weitershausen [:philikon]]

(In reply to :Ally Naaktgeboren from comment #7)
> Created attachment 552731 [details] [diff] [review]
> Part 1 (v1) : Rename Key (WIP)
> 
> -no testing
>   - *how* do I fully test this?

By going to all the UI that uses those strings and verifying it isn't broken. XUL will yell at you very early for missing strings. JS will break at runtime (watch the Error Console).

> -could not find consuming code for (which makes me very suspicious):
>   new.synckey.status.incorrect 

Looks like a stale string. Remove it.

>   setup.newSyncKeyPage.description.label

browser/base/content/syncSetup.xul:199

>   change.synckey.sameAsSyncKey 
>   change.synckey.sameAsPassword
>   change.synckey.sameAsUsername
>   change.synckey.sameAsEmail
>   change.synckey.tooShort

We no longer let users choose their own Sync Keys, so we can just get rid of these.

>   change.password.pwSameAsSyncKey

browser/base/content/syncUtils.js:246

Comment 10

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v1.1) : Rename Key (WIP)

phil, please ignore for now.

Comment 11

[Philipp von Weitershausen [:philikon]]

Comment on attachment 552731 [details] [diff] [review]
Part 1 (v1) : Rename Key (WIP)

I've only skimmed this, but it looks pretty good. Two small things:

>-<!ENTITY addDevice.dialog.syncKey.label     "To activate your device you will need to enter your Sync Key. Please print or save this key and take it with you.">
>+<!ENTITY addDevice.dialog.syncKey.label     "To activate your device you will need to enter your Recovery Key. Please print or save this key and take it with you.">

You forgot to change the string ID here.

>-save.synckey.title = Save Sync Key
>-save.default.label = Firefox Sync Key.html
>+save.recoverykey.defaultfilename.title = Save Recovery Key

This string has nothing to do with the default filename. It is the title for the Save dialog. I would leave it as 'save.recoverykey.title' like you had it before.

Comment 12

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v1.2) : Rename Key (WIP)

UL tested, js paths not fully tested.

Comment 13

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v1.2) : Rename Key (WIP)

must remember to save & qrefresh

Comment 14

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v2) : Rename Key

tested to the best of my knowledge (not that that says much at this point)

Comment 15

[Philipp von Weitershausen [:philikon]]

Comment on attachment 554256 [details] [diff] [review]
Part 1 (v2) : Rename Key

Looks good!

>-<!ENTITY syncGenerateNewKey.label  "Generate a new key">
>+<!ENTITY recoveryGenerateNewKey.label  "Generate a new key">

Why did you change this string's ID? Greedy search'n'replace regex? :) Seems unnecessary, just creates unnecessary churn for localizers. Please revert.

>-save.synckey.title = Save Sync Key
>-save.default.label = Firefox Sync Key.html
>+save.recoverykey.title = Save Recovery Key
>+save.recoverykey.label = Firefox Recovery Key.html

You must've misunderstood my comment 11 which was only about 'save.recoverykey.title'. Please rename 'save.default.label' to 'save.recoverykey.defaultfilename' as you had it before.

r=me with those addressed

Comment 16

[Allison Naaktgeboren :ally]

(In reply to Philipp von Weitershausen [:philikon] from comment #15)
> Comment on attachment 554256 [details] [diff] [review]
> Part 1 (v2) : Rename Key
> 
> Looks good!
> 
> >-<!ENTITY syncGenerateNewKey.label  "Generate a new key">
> >+<!ENTITY recoveryGenerateNewKey.label  "Generate a new key">
> 
> Why did you change this string's ID? Greedy search'n'replace regex? :) Seems
> unnecessary, just creates unnecessary churn for localizers. Please revert.

I did to keep the key names consistent.

Comment 17

[Philipp von Weitershausen [:philikon]]

(In reply to :Ally Naaktgeboren from comment #16)
> > >-<!ENTITY syncGenerateNewKey.label  "Generate a new key">
> > >+<!ENTITY recoveryGenerateNewKey.label  "Generate a new key">
> > 
> > Why did you change this string's ID? Greedy search'n'replace regex? :) Seems
> > unnecessary, just creates unnecessary churn for localizers. Please revert.
> 
> I did to keep the key names consistent.

I don't see a violation of consistency as the sequence "sync key" or a variation thereof doesn't occur in this string or the string ID. Also, my previous argument about unnecessary churn for localizers still stands. Please revert. :)

Comment 18

[Allison Naaktgeboren :ally]

Attachment: Part 1 (v3) : Rename Key

Comment 19

[Philipp von Weitershausen [:philikon]]

http://hg.mozilla.org/services/services-central/rev/858c7fa7af70

Comment 20

[Richard Newman [:rnewman]]

QA: please verify that "Sync Key" doesn't appear in the UI. In particular, the key saving UI, prefs, and in setup.

Comment 21

[Tracy Walker [:tracy]]

Setup dialogs and save dialog from Prefs look good.  However, the menu item under Manage Accounts still reads My Sync Key. Do you want a follow up bug for that?

Comment 22

[Richard Newman [:rnewman]]

(In reply to Tracy Walker from comment #21)
> Do you want a follow up bug for that?

Yup; no sense holding the train for this.

Comment 23

[Tracy Walker [:tracy]]

Filed Bug 681402 - Manage Accounts menu has My Sync Key for followup issue to be picked up by next weeks train.

Comment 24

[Richard Newman [:rnewman]]

http://hg.mozilla.org/mozilla-central/rev/858c7fa7af70

Comment 25

[Alexander L. Slovesnik]

> -change.synckey.acceptButton = Change Sync Key
> +change.synckey.acceptButton = Change Recovery Key

ID for change.synckey.acceptButton has not been changed.

Comment 26

[Philipp von Weitershausen [:philikon]]

(In reply to Alexander L. Slovesnik from comment #25)
> > -change.synckey.acceptButton = Change Sync Key
> > +change.synckey.acceptButton = Change Recovery Key
> 
> ID for change.synckey.acceptButton has not been changed.

Thanks for spotting this, Alexander! That's on me as a reviewer. Can you file a follow-up bug for this, please?

Comment 27

[Alexander L. Slovesnik]

(In reply to Philipp von Weitershausen [:philikon] from comment #26)
> (In reply to Alexander L. Slovesnik from comment #25)
> > > -change.synckey.acceptButton = Change Sync Key
> > > +change.synckey.acceptButton = Change Recovery Key
> > 
> > ID for change.synckey.acceptButton has not been changed.
> 
> Thanks for spotting this, Alexander! That's on me as a reviewer. Can you
> file a follow-up bug for this, please?

Filed Bug 681519

Comment 28

[Tracy Walker [:tracy]]

verified on m-c

Comment 29

[Jens Hatlak (:InvisibleSmiley)]

AFAICS you guys missed one change.password.pwSameAsSyncKey occurrence in syncSetup.js (I'll leave that one to you since I need to hurry and port this one to SM in time for the Aurora uplift).

Comment 30

[Philipp von Weitershausen [:philikon]]

(In reply to Jens Hatlak (:InvisibleSmiley) from comment #29)
> AFAICS you guys missed one change.password.pwSameAsSyncKey occurrence in
> syncSetup.js (I'll leave that one to you since I need to hurry and port this
> one to SM in time for the Aurora uplift).

Thanks! Filed bug 689832.