Closed Bug 1289231 Opened 8 years ago Closed 8 years ago

Show a notification bar on about:home that allows the user to undo the migration

Categories

(Firefox :: Migration, defect, P3)

Product:
Firefox
Component:
Migration
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 51
Tracking Flags:
Tracking Status
firefox49 --- fixed
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(2 files)

Bug 1289231 - show 'undo' notification bar,
58 bytes, text/x-review-board-request
Dolske
: review+
lizzard
: approval-mozilla-aurora+
lizzard
: approval-mozilla-beta+
Details
Bug 1289231 - part 2: remove notification after it's been displayed for 3 days,
58 bytes, text/x-review-board-request
Dolske
: review+
Details 
See bug 1289172 for rationale.
I'll try and write up a thing that mimics bug 1280381's logic to also remove the undo stuff when the notification bar has been shown on 3 successive days, but I figured it wouldn't hurt to put this version up for initial review, esp. for wording. Verdi, I used:

"We automatically imported your data from <browser name>? Would you like to keep it? [Keep] [Don't Keep]"

for the notification bar, but I'm obviously happy to take wording suggestions? :-)
Flags: needinfo?(mverdi)
Comment on attachment 8774514 [details]
Bug 1289231 - show 'undo' notification bar,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/66952/diff/1-2/
(In reply to :Gijs Kruitbosch (Gone July 28 - Aug 11) from comment #2)
> I'll try and write up a thing that mimics bug 1280381's logic to also remove
> the undo stuff when the notification bar has been shown on 3 successive
> days, but I figured it wouldn't hurt to put this version up for initial
> review, esp. for wording. Verdi, I used:
> 
> "We automatically imported your data from <browser name>? Would you like to
> keep it? [Keep] [Don't Keep]"
> 
> for the notification bar, but I'm obviously happy to take wording
> suggestions? :-)

Can we use:
"We automatically imported your data from <Browser Name> so you can quickly get to your favorite sites. Would you like to keep it? [Keep] [Don't Keep]"
Flags: needinfo?(mverdi)
Comment on attachment 8774514 [details]
Bug 1289231 - show 'undo' notification bar,

https://reviewboard.mozilla.org/r/66952/#review67888

::: browser/locales/en-US/chrome/browser/migration/migration.properties:17
(Diff revision 2)
> +# Sentence fragment to replace a browser's name if we don't know it:
> +sourceNameAnotherBrowser=another browser

Eh, this feels like a pretty sketchy thing to do, L10N wise. I suspect it would be better to simply have a version of automigration.undo.message for this case.

OTOH, this is a fairly edge-case error (we always set the pref for the name), so probably not worth worrying about. [Or maybe MigrationUtils.getBrowserName() should just know how to return "Unknown Browser" when it's given a null or unknown name.]
Attachment #8774514 - Flags: review?(dolske) → review+
Comment on attachment 8774711 [details]
Bug 1289231 - part 2: remove notification after it's been displayed for 3 days,

https://reviewboard.mozilla.org/r/67154/#review67892

::: browser/app/profile/firefox.js:1456
(Diff revision 1)
>  pref("browser.laterrun.enabled", false);
>  
>  pref("browser.migrate.automigrate.enabled", false);
> +// 4 here means the suggestion notification will be automatically
> +// hidden the 4th day, so it will actually be shown on 3 different days.
> +pref("browser.migrate.automigrate.daysBeforeHidingUndoPrompt", 4);

"Hide" is kinda confusing here. We're just simply not showing it after X days...

".daysToOfferUndo"?

::: browser/components/migration/AutoMigrate.jsm:293
(Diff revision 1)
>        if (!notificationBox || notificationBox.getNotificationWithValue("abouthome-automigration-undo")) {
>          return;
>        }
>  
> +      // At this stage we're committed to show the prompt - unless we shouldn't:
> +      if (!this.shouldStillShowUndoPrompt()) {

I think this should really short-circuit earlier on (unless I'm missing some fine detail of how the prefs work)... Every time about:home loads for the life of the profile we're going to do all the work to construct this notification, only to abort adding it at the last second. That kind of sucks.

It would be preferable to have a simple, robust check at the top to kill the whole thing. eg "if (Date.now > pref.migrateTime + pref.daysToOffer) return" kind of thing.

Especially with the current undo functionality being possibly destructive, I'm also wary of having someone clear an obscure pref, getting the offer again, and having it nuke lots of non-imported data.

::: browser/components/migration/AutoMigrate.jsm:330
(Diff revision 1)
>        );
>      });
>    },
>  
> +  shouldStillShowUndoPrompt() {
> +    let date = parseInt((new Date()).toLocaleFormat("%Y%m%d"));

*sigh*

I get you're just copying this from bug 1280381, but time/date parsing like this is a pet peeve of mine. It's almost _always_ better to work with simple unix-epoch seconds, as there's a lot of complexity in the time-string parsing that can break in unexpected ways.

Against my better judgement I'll just hold my nose this time.
Attachment #8774711 - Flags: review?(dolske) → review-
Priority: -- → P3
Comment on attachment 8774711 [details]
Bug 1289231 - part 2: remove notification after it's been displayed for 3 days,

https://reviewboard.mozilla.org/r/67154/#review67892

> I think this should really short-circuit earlier on (unless I'm missing some fine detail of how the prefs work)... Every time about:home loads for the life of the profile we're going to do all the work to construct this notification, only to abort adding it at the last second. That kind of sucks.
> 
> It would be preferable to have a simple, robust check at the top to kill the whole thing. eg "if (Date.now > pref.migrateTime + pref.daysToOffer) return" kind of thing.
> 
> Especially with the current undo functionality being possibly destructive, I'm also wary of having someone clear an obscure pref, getting the offer again, and having it nuke lots of non-imported data.

Figured out on IRC that this should be OK, and also that inside the if block we call `removeUndoOption()` as soon as we can no longer show the prompt, which will remove the prefs that enable the undo option, which will then cause us to bail out immediately in the `canUndo` method and return false, based on which we'd bail from this callback.
Comment on attachment 8774711 [details]
Bug 1289231 - part 2: remove notification after it's been displayed for 3 days,

https://reviewboard.mozilla.org/r/67154/#review70476

::: browser/components/migration/AutoMigrate.jsm:339
(Diff revision 2)
>        );
>      });
>    },
>  
> +  shouldStillShowUndoPrompt() {
> +    let date = new Date();

s/date/(now|today)/

::: browser/components/migration/AutoMigrate.jsm:343
(Diff revision 2)
> +  shouldStillShowUndoPrompt() {
> +    let date = new Date();
> +    // Round down to midnight:
> +    date = new Date(date.getFullYear(), date.getMonth(), date.getDate());
> +    let previousDateStr = Preferences.get(kAutoMigrateLastUndoPromptDatePref, "0");
> +    let previousDate = new Date(parseInt(previousDateStr, 10));

This is wrong. You're creating a Date that's about 20,160 seconds after 1970.

 > new Date(20160818);
 < Date 1970-01-01T05:36:00.818Z

It happens to work because you're creating another Date (for today) that's a few hundreds of milliseconds after that 1970s Date.

This is exactly why I don't like code that tries to do time processing this way!

I would have just written this as such:

const DAYS_TO_SHOW = 3;
const HOURS_BETWEEN_NAGS = 24;

function automigrate() {
  var now = Math.round(Date.now() / 1000);
  Preferences.set("migrateTime", now);
}

function shouldStillShowUndoPrompt() {
  var now = Math.round(Date.now() / 1000);
  
  var migrateTime = Preferences.get("migrateTime", 0);
  var lastPromptTime = Preferences.get("lastPromptTime", 0);
  
  var daysSinceMigrate = (now - migrateTime) / 86400;
  var hoursSinceLastPrompt = (now - lastPromptTime) / 3600;
  
  if (daysSinceMigrate > DAYS_TO_SHOW)
    return false;

  if (hoursSinceLastPrompt < HOURS_BETWEEN_NAGS)
    return false;

  // somewhere:
  Preferences.set("lastPromptTime", now);

  return true;
}

Note that this also removes the quirk of getting one prompt at 11:59:59pm, opening about:home again, and getting a second prompt at 12:00:01am (because it's the next day).

[And I'd actually probably change HOURS_BETWEEN_NAGS to 18, just to break out of any interaction with someone's diurnal cycle.]

Your choice on fixing.
Attachment #8774711 - Flags: review?(dolske) → review+
Comment on attachment 8774711 [details]
Bug 1289231 - part 2: remove notification after it's been displayed for 3 days,

https://reviewboard.mozilla.org/r/67154/#review70654

::: browser/components/migration/AutoMigrate.jsm:343
(Diff revision 2)
> +  shouldStillShowUndoPrompt() {
> +    let date = new Date();
> +    // Round down to midnight:
> +    date = new Date(date.getFullYear(), date.getMonth(), date.getDate());
> +    let previousDateStr = Preferences.get(kAutoMigrateLastUndoPromptDatePref, "0");
> +    let previousDate = new Date(parseInt(previousDateStr, 10));

Err, no? I removed the stringifying to YYYYmmdd thing entirely, and just store a unix timestamp in the pref. I store it as a string because int prefs are dumb and can't cope with ints as large as unix timestamps. That's what Date.valueOf().toString() returns. There's nothing in there by default, which is why I'm using "0" as the fallback value here (which obviously gets you unix epoch). I tested the code, at no point is something like "20160818" written anymore, it just writes unix timestamps.

Your alternative fix would bail out if the user didn't start Firefox for 3 days, and the user might never have seen the prompt.

The other date constructor is using `new Date(year, month, date)` as 3 separate arguments, and also works correctly, AFAICT.

There is no logic here at all to avoid you getting prompted multiple times per day, because the location bar code doesn't either. You just get prompted repeatedly - all it keeps track of is whether you got prompted that day (or never did something where the prompt would have shown). So there's no real difference between being prompted several times on day X and then again shortly after midnight, or something like that.

IOW, the code ensures you get prompted on exactly 3 days (irrespective of how far apart those 3 days are in time). You could have been prompted 3 times or 3000 times, but we don't care - you must have been prompted at least once on 3 separate days.
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7b78eeb92288
show 'undo' notification bar, r=dolske
https://hg.mozilla.org/integration/mozilla-inbound/rev/fab4dd6db1f5
part 2: remove notification after it's been displayed for 3 days, r=dolske
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f728394b92ce
followup: fix quote in dontkeep string, rs=bustage
Status: NEW → ASSIGNED
status-firefox49: --- → affected
status-firefox51: --- → affected
Whiteboard: [migration-needs-uplift]
Whiteboard: [migration-needs-uplift]
Comment on attachment 8774514 [details]
Bug 1289231 - show 'undo' notification bar,

Approval Request Comment
[Feature/regressing bug #]: testing automigration on en-US beta releases before Firefox 51 hits beta.
[User impact if declined]: we can't test automigration on beta before 51 hits beta; there might be issues uplifting other funnelcake-related patches to 49/50.
[Describe test coverage new/current, TreeHerder]: There is currently no test coverage for the "undo" UI component of automigration, but there is automated test coverage for the actual "under the hood" undo feature itself, as detailed in the relevant other bugs + uplift requests.
[Risks and why]: low risk for general release, because all of this is /still/ behind the main automigration pref (which we will keep turned off for release).
[String/UUID change made/needed]: The patches as-is here contain string additions. The plan is to do a variation of this patch that puts the new (English) strings in a separate file that isn't in the locale/ directory, and to only enable the feature for builds running the en-US localization. This will mean there will be no l10n impact. I checked this plan with :flod who said that this means the l10n team would be fine with that.

I'm requesting uplift before the aurora/beta patch is ready in order to clear out the set of bugs that I initially set migration-needs-uplift on in the whiteboard.

This patch is not required for the funnelcake itself, but it will simplify uplifting some of the later changes that *do* affect the funnelcake. If we choose not to uplift this patch but uplift the rest of the funnelcake patches, that works for the funnelcake but I'll likely have to resolve conflicts on beta for some of the other patches, and the Firefox team will not be able to run our own checks on this code in close-to-production environments until 51 is in beta. That then means that if we need to change anything, especially if such changes impact UI / localization, that actual release of the feature would be pushed back to 52 or 53, which could potentially seriously affect efforts to improve user retention. Considering all that, and the risk for the 49/50 releases themselves being very low because the feature won't be turned on there, I think uplifting this is the right choice.
Attachment #8774514 - Flags: approval-mozilla-beta?
Attachment #8774514 - Flags: approval-mozilla-aurora?
Blocks: 1297479
flod: So, are you OK-ing these string changes for 49 and 50 ?  It sounds like the UI may not be invoked even for the funnelcake tests in 49 . And if it is, it is only offered to en-US.
Flags: needinfo?(francesco.lodolo)
Comment on attachment 8774514 [details]
Bug 1289231 - show 'undo' notification bar,

Let's just do this in order to get the automigration code uplifted.
I see flod signing off on it on yesterday's discussion on fx-team with gijs.
Attachment #8774514 - Flags: approval-mozilla-beta?
Attachment #8774514 - Flags: approval-mozilla-beta+
Attachment #8774514 - Flags: approval-mozilla-aurora?
Attachment #8774514 - Flags: approval-mozilla-aurora+
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #18)
> I see flod signing off on it on yesterday's discussion on fx-team with gijs.

Confirmed. The experiments are going to run only for en-US, and the strings in these patches are outside of expected paths, so they're invisible to l10n.
Flags: needinfo?(francesco.lodolo)
Flags: needinfo?(majed.rifat)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: