Rename "Sync Key" to "Recovery Key"

VERIFIED FIXED in mozilla9

Status

()

defect
VERIFIED FIXED
8 years ago
10 months ago

People

(Reporter: rnewman, Assigned: ally)

Tracking

unspecified
mozilla9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [verified in services])

Attachments

(1 attachment, 6 obsolete attachments)

Seems like a great idea to me. Consider.
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.
Yeah, this was me filing your idea via philikon :D  I take no credit!
ah, well I concur with myself then :) (happens occasionally)
Duplicate of this bug: 675744
Blocks: 675826
wip
Assignee: nobody → ally
Attachment #552229 - Flags: feedback?(philipp)
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.
Attachment #552229 - Flags: feedback?(philipp)
-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
Attachment #552229 - Attachment is obsolete: true
Attachment #552731 - Flags: feedback?(philipp)
(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.
(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
phil, please ignore for now.
Attachment #552731 - Attachment is obsolete: true
Attachment #552731 - Flags: feedback?(philipp)
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.
UL tested, js paths not fully tested.
Attachment #552765 - Attachment is obsolete: true
must remember to save & qrefresh
Attachment #552867 - Attachment is obsolete: true
Posted patch Part 1 (v2) : Rename Key (obsolete) — Splinter Review
tested to the best of my knowledge (not that that says much at this point)
Attachment #552868 - Attachment is obsolete: true
Attachment #554256 - Flags: feedback?(philipp)
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
Attachment #554256 - Flags: feedback?(philipp) → feedback+
(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.
(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. :)
Attachment #554256 - Attachment is obsolete: true
Attachment #554542 - Flags: review?(philipp)
Attachment #554542 - Flags: review?(philipp) → review+
QA: please verify that "Sync Key" doesn't appear in the UI. In particular, the key saving UI, prefs, and in setup.
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?
(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.
Filed Bug 681402 - Manage Accounts menu has My Sync Key for followup issue to be picked up by next weeks train.
Whiteboard: [fixed in services] → [verified in services]
http://hg.mozilla.org/mozilla-central/rev/858c7fa7af70
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
> -change.synckey.acceptButton = Change Sync Key
> +change.synckey.acceptButton = Change Recovery Key

ID for change.synckey.acceptButton has not been changed.
(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?
Depends on: 681519
(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
verified on m-c
Status: RESOLVED → VERIFIED
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).
No longer blocks: sync2sm
(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.
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.