Closed Bug 1335442 Opened 6 years ago Closed 6 years ago

Update the messages in the automigration undo import notification bar

Categories

(Firefox :: General, defect)

Product:
Firefox
Component:
General
Version:
52 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
Firefox 54
Tracking Flags:
Tracking Status
firefox52 --- fixed
firefox-esr52 --- fixed
firefox53 --- fixed
firefox54 --- verified

People

(Reporter: verdi, Assigned: Gijs)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1335442 - deal correctly with not importing anything,
59 bytes, text/x-review-board-request
jaws
: review+
Details
Bug 1335442 - update wording of automigration notification bar and add tests,
59 bytes, text/x-review-board-request
jaws
: review+
Details
Aurora/beta patch
20.21 KB, patch
Gijs
: review+
jcristau
: approval-mozilla-aurora+
jcristau
: approval-mozilla-beta+
Details | Diff | Splinter Review 
We have a new string for the automigration infobar:

"Pick up where you left off. We've imported these sites and your bookmarks, history, and passwords from [Browser] into Firefox."

Buttons:
[Ok, Got it]
[No Thanks]
(In reply to Verdi [:verdi] from comment #0)
> We have a new string for the automigration infobar:
> 
> "Pick up where you left off. We've imported these sites and your bookmarks,
> history, and passwords from [Browser] into Firefox."

I have a question about this. We still show the message on about:home as well as about:newtab (also/particularly when we experiment on beta). Additionally, it's really not easy to determine whether the sites on about:newtab will be strictly from the user's imported data, or only the default set, or a mix - certainly not from the code showing this notification. So I think the "these sites" bit is confusing, and I would like to get rid of it. The suggested wording here is already tricky to implement because it already requires keeping track of exactly what we import (can't tell the user "we imported your passwords from Chrome" if we, well, didn't (for instance, because the user had no saved Chrome passwords), unless we want to set off user paranoia. I would prefer not to add even more complexity.
Flags: needinfo?(mverdi)
(I've left the strings as suggested for now, but it should be trivial to update them if we do it *before landing* this change.)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Michelle and I talked about this today. We'd like to go with the string in comment 0 for now. We tested it against other versions, including the current string, and it performed best. If you want to use version without "these sites" for about:home that would be great but if we don't think it's critical if you feel the added complexity isn't worth it.
Flags: needinfo?(mverdi)
(In reply to Verdi [:verdi] from comment #6)
> Michelle and I talked about this today. We'd like to go with the string in
> comment 0 for now. We tested it against other versions, including the
> current string, and it performed best. If you want to use version without
> "these sites" for about:home that would be great but if we don't think it's
> critical if you feel the added complexity isn't worth it.

OK. I'm going to continue to omit the "these sites" for the passwords case, given that then we're *sure* there's no sites showing up on about:newtab that have been imported. :-)

Given Dão's PTO, going to move the reviews to Jared.
Summary: Update the strings in the automigration infobar → Update the messages in the automigration undo import notification bar
Attachment #8833287 - Flags: review?(dao+bmo) → review?(jaws)
Attachment #8833288 - Flags: review?(dao+bmo) → review?(jaws)
Comment on attachment 8833287 [details]
Bug 1335442 - deal correctly with not importing anything,

https://reviewboard.mozilla.org/r/109536/#review111766

::: browser/components/migration/AutoMigrate.jsm:417
(Diff revision 1)
>    },
>  
>    _dejsonifyUndoState(state) {
>      state = JSON.parse(state);
> +    if (!state) {
> +      return new Map([]);

self-nit: `new Map()`
Comment on attachment 8833287 [details]
Bug 1335442 - deal correctly with not importing anything,

https://reviewboard.mozilla.org/r/109536/#review112158

::: browser/components/migration/tests/unit/test_automigration.js:212
(Diff revision 1)
> +  let storedLogins = Services.logins.findLogins({}, "www.mozilla.org",
> +                                                "http://www.mozilla.org", null);
> +  Assert.equal(storedLogins.length, 0, "Should have no logins");

Can you do this also before the undo to confirm the login was added?
Attachment #8833287 - Flags: review?(jaws) → review+
Comment on attachment 8833288 [details]
Bug 1335442 - update wording of automigration notification bar and add tests,

https://reviewboard.mozilla.org/r/109538/#review112160

::: browser/components/migration/tests/browser/browser_undo_notification_wording.js:43
(Diff revision 2)
> +  for (let subset of kSubsets) {
> +    let state = new Map(subset.map(item => [item, [{}]]));
> +    scope.AutoMigrate._setImportedItemPrefFromState(state);
> +    let tab = yield BrowserTestUtils.openNewForegroundTab(gBrowser, url, false);
> +    let browser = tab.linkedBrowser;
> +    

i think the whitespace will fail eslint

::: browser/locales/en-US/chrome/browser/migration/migration.properties:76
(Diff revision 2)
> -automigration.undo.unknownBrowserMessage = We automatically imported your data from another browser. Would you like to keep it?
> -automigration.undo.keep.label            = Keep
> -automigration.undo.keep.accesskey        = K
> -automigration.undo.dontkeep.label        = Don’t Keep
> -automigration.undo.dontkeep.accesskey    = D
> +automigration.undo.message.bookmarks     = Pick up where you left off. We've imported these sites and your bookmarks from %1$S into %2$S.
> +automigration.undo.message.visits        = Pick up where you left off. We've imported these sites and your history from %1$S into %2$S.
> +automigration.undo.message.logins        = Pick up where you left off. We've imported your passwords from %1$S into %2$S.
> +automigration.undo.message.bookmarks.visits = Pick up where you left off. We've imported these sites and your bookmarks and history from %1$S into %2$S.
> +automigration.undo.message.logins.visits    = Pick up where you left off. We've imported these sites and your history and passwords from %1$S into %2$S.
> +automigration.undo.message.bookmarks.logins = Pick up where you left off. We've imported these sites and your bookmarks and passwords from %1$S into %2$S.

can you please order these alphabetically?

::: browser/locales/en-US/chrome/browser/migration/migration.properties:76
(Diff revision 2)
>  
>  128_firefox=Windows and Tabs
>  
>  # Automigration undo notification.
> -automigration.undo.message               = We automatically imported your data from %S. Would you like to keep it?
> -automigration.undo.unknownBrowserMessage = We automatically imported your data from another browser. Would you like to keep it?
> +# %1$S will be replaced with the name of the browser we imported from, %2$S will be replaced with brandShortName
> +automigration.undo.message.bookmarks     = Pick up where you left off. We've imported these sites and your bookmarks from %1$S into %2$S.

Pretty sure these strings will fail the unicode-quotes test that we have.
Attachment #8833288 - Flags: review?(jaws) → review+
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d122656ee44a
deal correctly with not importing anything, r=jaws
https://hg.mozilla.org/integration/autoland/rev/7a024b0cedfc
update wording of automigration notification bar and add tests, r=jaws
https://hg.mozilla.org/mozilla-central/rev/d122656ee44a
https://hg.mozilla.org/mozilla-central/rev/7a024b0cedfc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
NOTE: do NOT uplift to beta until beta 8 has gone to build.

Approval Request Comment
[Feature/Bug causing the regression]: automigration tests on beta/funnelcakes
[User impact if declined]: suboptimal wording of notification bars users get
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: locally with an m-c build, I see the new strings. But no formal verification has happened.
[Needs manual test from QE? If yes, steps to reproduce]: STR:

1. open about:config on a clean named profile
2. turn the browser.migrate.automigrate.enabled pref to true
3. on your machine, make IE/Chrome/Safari/Edge the default browser
4. close nightly and run it from the commandline with:

path/to/firefox.exe --no-remote -P <profile> -migration

[List of other uplifts needed for the feature/fix]: needs the patch from bug 1335709 to be backed out (after beta 8)
[Is the change risky?]: no
[Why is the change risky/not risky?]: it has automated tests and pertains to a feature we don't ship to general release
[String changes made/needed]: there are hardcoded strings in these patches. They won't be localized. The feature won't be enabled except for en-US users on release. It is currently also enabled on beta for everyone. It will be turned off on beta after beta 8.
Attachment #8839185 - Flags: review+
Attachment #8839185 - Flags: approval-mozilla-beta?
Attachment #8839185 - Flags: approval-mozilla-aurora?
Attachment #8839185 - Attachment is patch: true
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Hi :flod,
For string changes made/needed part, is it ok for you?
Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(brindusa.tot)
(In reply to Gerry Chang [:gchang] from comment #16)
> Hi :flod,
> For string changes made/needed part, is it ok for you?

Yes, there's no impact for l10n (strings are not exposed to localization in the patch for aurora/beta).
Flags: needinfo?(francesco.lodolo)
I just backed out 1335709 from beta, so this is now good to go once it has approval.
Comment on attachment 8839185 [details] [diff] [review]
Aurora/beta patch

changes to automigration, aurora53+, beta52+

Doesn't look like any of this affects the default configurations.
Attachment #8839185 - Flags: approval-mozilla-beta?
Attachment #8839185 - Flags: approval-mozilla-beta+
Attachment #8839185 - Flags: approval-mozilla-aurora?
Attachment #8839185 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/diff/7a024b0cedfc/browser/locales/en-US/chrome/browser/migration/migration.properties
> +# %1$S will be replaced with the name of the browser we imported from, %2$S will be replaced with brandShortName
> +automigration.undo.message.all              = Pick up where you left off. We’ve imported these sites and your bookmarks, history and passwords from %1$S into %2$S.
> +automigration.undo.message.bookmarks        = Pick up where you left off. We’ve imported these sites and your bookmarks from %1$S into %2$S.
> +automigration.undo.message.bookmarks.logins = Pick up where you left off. We’ve imported these sites and your bookmarks and passwords from %1$S into %2$S.
> +automigration.undo.message.bookmarks.visits = Pick up where you left off. We’ve imported these sites and your bookmarks and history from %1$S into %2$S.
> +automigration.undo.message.logins           = Pick up where you left off. We’ve imported your passwords from %1$S into %2$S.
> +automigration.undo.message.logins.visits    = Pick up where you left off. We’ve imported these sites and your history and passwords from %1$S into %2$S.
> +automigration.undo.message.visits           = Pick up where you left off. We’ve imported these sites and your history from %1$S into %2$S.
> +automigration.undo.keep2.label            = OK, Got it
> +automigration.undo.keep2.accesskey        = O
> +automigration.undo.dontkeep2.label        = No Thanks
> +automigration.undo.dontkeep2.accesskey    = N
> +automigration.undo.unknownbrowser         = Unknown Browser

Unfortunately the "Unknown Browser" replacement construct doesn't work at all for Polish because we need helper word ("application") before %1$S ("We’ve imported your passwords from application Unknown Browser into application Firefox").
(In reply to Stefan Plewako [:stef] from comment #22)
> https://hg.mozilla.org/mozilla-central/diff/7a024b0cedfc/browser/locales/en-
> US/chrome/browser/migration/migration.properties
> > +# %1$S will be replaced with the name of the browser we imported from, %2$S will be replaced with brandShortName
> > +automigration.undo.message.all              = Pick up where you left off. We’ve imported these sites and your bookmarks, history and passwords from %1$S into %2$S.
> > +automigration.undo.message.bookmarks        = Pick up where you left off. We’ve imported these sites and your bookmarks from %1$S into %2$S.
> > +automigration.undo.message.bookmarks.logins = Pick up where you left off. We’ve imported these sites and your bookmarks and passwords from %1$S into %2$S.
> > +automigration.undo.message.bookmarks.visits = Pick up where you left off. We’ve imported these sites and your bookmarks and history from %1$S into %2$S.
> > +automigration.undo.message.logins           = Pick up where you left off. We’ve imported your passwords from %1$S into %2$S.
> > +automigration.undo.message.logins.visits    = Pick up where you left off. We’ve imported these sites and your history and passwords from %1$S into %2$S.
> > +automigration.undo.message.visits           = Pick up where you left off. We’ve imported these sites and your history from %1$S into %2$S.
> > +automigration.undo.keep2.label            = OK, Got it
> > +automigration.undo.keep2.accesskey        = O
> > +automigration.undo.dontkeep2.label        = No Thanks
> > +automigration.undo.dontkeep2.accesskey    = N
> > +automigration.undo.unknownbrowser         = Unknown Browser
> 
> Unfortunately the "Unknown Browser" replacement construct doesn't work at
> all for Polish because we need helper word ("application") before %1$S
> ("We’ve imported your passwords from application Unknown Browser into
> application Firefox").

It doesn't work amazingly in English as it is. The intention of capitalizing is that we fill in a name, and in that sense it should function the same as if the string was "360SE" or "Chrome" or "Internet Explorer" or "Microsoft Edge". Obviously I don't speak Polish, but I don't see why, if "Unknown Browser" doesn't work, "Google Chrome" *does* work. They're both arbitrary named identifiers. Why is one any better than the other?

The alternative is adding another 6 strings (and associated code) for a case that we should basically never hit (we would need to be importing from a browser which we don't know about (impossible), or fail to store the information of the browser from which we import, which essentially shouldn't happen), which IMO was the wrong trade-off, so that's not what I implemented.
(In reply to :Gijs from comment #23)
> It doesn't work amazingly in English as it is. The intention of capitalizing
> is that we fill in a name, and in that sense it should function the same as
> if the string was "360SE" or "Chrome" or "Internet Explorer" or "Microsoft
> Edge". Obviously I don't speak Polish, but I don't see why, if "Unknown
> Browser" doesn't work, "Google Chrome" *does* work. They're both arbitrary
> named identifiers. Why is one any better than the other?

Because of helper word and declension it sounds worse in Polish while in English it almost (except capitalizing) forms natural sentence.

> The alternative is adding another 6 strings (and associated code) for a case
> that we should basically never hit (we would need to be importing from a
> browser which we don't know about (impossible)

I don't suggest adding another 6 strings but is that a browser then (or a thing)?

> or fail to store the
> information of the browser from which we import, which essentially shouldn't
> happen), which IMO was the wrong trade-off, so that's not what I implemented.

There are different types of information that could be stored, like name, path and so on…
Verified as fixed on Windows 10 with latest Nightly 54.0a1, Build ID 20170221030205. Received new steps from Gijs:

1. Create new profile.
2. In the Firefox installation folder, create new folder distribution, and add in it distribution.ini file with content from https://bug1326252.bmoattachments.org/attachment.cgi?id=8825884
3. Make IE/Chrome/Safari/Edge the default browser
4. close nightly and run it from the commandline with:
path/to/firefox.exe --no-remote -P <profile> -migration

The new message from automigration infobar is displayed: "Pick up where you left off. We've imported these sites and your bookmarks, history, and passwords from Google Chrome into Nightly"
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
Flags: needinfo?(brindusa.tot)
You need to log in before you can comment on or make changes to this bug.