Add Manual Migration component. #2858

piatra · 2017-07-14T14:49:13Z

Still waiting on spec for the messages, button text.

coveralls · 2017-07-14T14:54:30Z

Coverage remained the same at 87.576% when pulling e696d05 on piatra:bug-2787-manual-migratio into a4fd42a on mozilla:master.

coveralls · 2017-07-20T12:22:36Z

Coverage remained the same at 86.407% when pulling cd6e574 on piatra:bug-2787-manual-migratio into e847f1f on mozilla:master.

piatra · 2017-07-20T12:33:48Z

PR has r+ on design.

coveralls · 2017-07-20T12:36:47Z

Coverage remained the same at 86.407% when pulling d97102c on piatra:bug-2787-manual-migratio into e847f1f on mozilla:master.

Mardak

Overall looks to be functioning as expected except for the "Cancel" from wizard. A bunch of various nits/renamings.

Mardak · 2017-07-21T05:07:41Z

system-addon/content-src/components/Base/Base.jsx

@@ -49,6 +49,7 @@ class Base extends React.Component {
        <div className="outer-wrapper">
          <main>
            {prefs.showSearch && <Search />}
+            <ManualMigration />


Looks like we should be able to conditionally show this like the adjacent components. Probably !pref.migrationExpired ?

Or maybe even match the show pattern with showMigration and have it start true then goes false when expired.

Mardak · 2017-07-21T05:09:23Z

system-addon/content-src/components/ManualMigration/ManualMigration.jsx

+          <FormattedMessage id="manual_migration_explanation_msg" />
+        </p>
+        <div className="manual-migration-actions actions">
+          <button onClick={() => this._cancelTour()}>


Pass in a bound-from-outside-render method to avoid unnecessary render. See #2928 for examples

Mardak · 2017-07-21T05:11:28Z

system-addon/content-src/components/ManualMigration/ManualMigration.jsx

+
+  render() {
+    if (this.props.values.migrationExpired === true) {
+      return null;


No need for this check if Base handles not even creating the component.

Mardak · 2017-07-21T05:12:08Z

system-addon/content-src/components/ManualMigration/ManualMigration.jsx

+
+module.exports = connect(state => state.Prefs)(ManualMigration);
+module.exports._unconnected = ManualMigration;
+module.exports.ManualMigration = ManualMigration;


Any particular reason to export as both _unconnected and ManualMigration?

Mardak · 2017-07-21T05:18:45Z

system-addon/lib/ActivityStream.jsm

@@ -81,6 +82,21 @@ const PREFS_CONFIG = new Map([
      "provider_name": "Pocket",
      "provider_icon": "pocket"
    }`
+  }],
+  ["migrationExpired", {
+    title: "Boolean flag that will always be true once the migration period has passed.",


Not sure if this is better named as "migrationDismissed" or "migrationDone" or as it is right now could be fine. The description is not as accurate in that there's multiple ways that it becomes true.

Mardak · 2017-07-21T05:42:51Z

system-addon/lib/ActivityStream.jsm

@@ -20,6 +20,7 @@ const {TelemetryFeed} = Cu.import("resource://activity-stream/lib/TelemetryFeed.
 const {TopSitesFeed} = Cu.import("resource://activity-stream/lib/TopSitesFeed.jsm", {});
 const {DummySectionFeed} = Cu.import("resource://activity-stream/lib/DummySectionFeed.jsm", {});
 const {TopStoriesFeed} = Cu.import("resource://activity-stream/lib/TopStoriesFeed.jsm", {});
+const {ManualMigration} = Cu.import("resource://activity-stream/lib/ManualMigration.jsm", {});


nit: restore alphabetical

Mardak · 2017-07-21T05:46:53Z

system-addon/test/unit/lib/ManualMigration.test.js

+      isExpiredStub.returns(true);
+
+      instance.checkMigrationStatus(false);
+      assert.calledOnce(setStatusStub);


nit: newline between test and check

Mardak · 2017-07-21T05:47:58Z

system-addon/test/unit/lib/ManualMigration.test.js

+
+        assert.calledOnce(migrationSpy);
+
+        assert.calledTwice(fakePrefs.get);


nit: excess newline?

Mardak · 2017-07-21T05:49:44Z

locales/en-US/strings.properties

+
+# LOCALIZATION NOTE (manual_migration_explanation_msg): This message is show to encourage users to
+# import their browser profile from another browsers they might be using.
+manual_migration_explanation_msg=Try Firefox with your favorite sites, bookmarks, and passwords from another browser.


Let's just do manual_migration_explanation

Mardak · 2017-07-21T05:49:57Z

locales/en-US/strings.properties

+manual_migration_explanation_msg=Try Firefox with your favorite sites, bookmarks, and passwords from another browser.
+# LOCALIZATION NOTE (manual_migration_cancel_btn): This message is shown on a button that cancels the
+# process of importing a browser profile into Firefox.
+manual_migration_cancel_btn=No Thanks


And looks like the pattern is _button

piatra · 2017-07-22T16:09:18Z

@Mardak what would be the expected behavior of cancel compared to how the current implementation works.

Mardak · 2017-07-22T16:18:09Z

Bryant says cancel is the least important one but the spec says to also dismiss the manual migration message if the user hits cancel: #2787 (comment)

If there's no similar "Migration:Ended" event for "Migration:Cancel" then that would need to be added to mozilla-central, but this PR can land without that given its lower priority.

coveralls · 2017-07-23T14:34:17Z

Coverage remained the same at 86.407% when pulling 0583732 on piatra:bug-2787-manual-migratio into 75e5bec on mozilla:master.

coveralls · 2017-07-23T15:01:34Z

Coverage remained the same at 86.407% when pulling 61a2df0 on piatra:bug-2787-manual-migratio into 75e5bec on mozilla:master.

coveralls · 2017-07-24T12:33:57Z

Coverage remained the same at 86.407% when pulling 4b4bc06 on piatra:bug-2787-manual-migratio into 75e5bec on mozilla:master.

coveralls · 2017-07-24T12:50:47Z

Coverage remained the same at 86.407% when pulling fd24981 on piatra:bug-2787-manual-migratio into 75e5bec on mozilla:master.

coveralls · 2017-07-24T12:57:01Z

Coverage remained the same at 86.407% when pulling ca9508b on piatra:bug-2787-manual-migratio into 75e5bec on mozilla:master.

piatra · 2017-07-24T13:50:37Z

@Mardak can you have another look at this? Thanks.

Mardak

Almost there. Still a couple of issues with default prefs and binding in render

Mardak · 2017-07-24T17:18:25Z

system-addon/content-src/components/ManualMigration/ManualMigration.jsx

+          <FormattedMessage id="manual_migration_explanation" />
+        </p>
+        <div className="manual-migration-actions actions">
+          <button onClick={this._cancelTour(this.props.dispatch)}>


This does avoid the bind call directly within render but it's still binding a function so that every potential render of ManualMigration will force a rerendering because it's not === due to a new function. You should just do the usual bind in constructor.

Mardak · 2017-07-24T17:20:46Z

system-addon/lib/ActivityStream.jsm

+  ["migrationExpired", {
+    title: "Boolean flag that decides whether to show the migration message or not.",
+    value: false,
+    value_local_dev: false


The other review comment asked about why value_local_dev is needed. You addressed the last one but left these untouched.

Mardak · 2017-07-24T17:21:03Z

system-addon/lib/ActivityStream.jsm

+  ["migrationRemainingDays", {
+    title: "Number of days to show the manual migration message",
+    value: 4,
+    value_local_dev: 2


Same thing here value_local_dev to be explicit.

Mardak · 2017-07-24T17:21:17Z

system-addon/content-src/components/ManualMigration/ManualMigration.jsx

+          <button onClick={this._cancelTour(this.props.dispatch)}>
+            <FormattedMessage id="manual_migration_cancel_button" />
+          </button>
+          <button className="done" onClick={this._launchTour(this.props.dispatch)}>


This is also effectively binding.

Mardak · 2017-07-24T17:21:46Z

system-addon/content-src/components/ManualMigration/ManualMigration.jsx

+  }
+}
+
+module.exports = connect(state => state.Prefs)(ManualMigration);


Selecting Prefs shouldn't be needed anymore?

coveralls · 2017-07-24T18:14:06Z

Coverage remained the same at 86.407% when pulling 52c52ae on piatra:bug-2787-manual-migratio into 75e5bec on mozilla:master.

Mardak

Thanks!

as-pine-proxy · 2017-07-24T20:07:50Z

Treeherder: 8a13f449835f76758bc3a2a66ac7148103525d56

piatra added the PR / In progress don't review yet label Jul 14, 2017

piatra force-pushed the bug-2787-manual-migratio branch 3 times, most recently from a1833d7 to cd6e574 Compare July 20, 2017 12:17

piatra requested a review from Mardak July 20, 2017 12:33

piatra assigned piatra and Mardak and unassigned piatra Jul 20, 2017

piatra added PR / Needs review and removed PR / In progress don't review yet labels Jul 20, 2017

Mardak suggested changes Jul 21, 2017

View reviewed changes

Mardak assigned piatra and unassigned Mardak Jul 21, 2017

piatra force-pushed the bug-2787-manual-migratio branch from e374b0d to 0583732 Compare July 23, 2017 14:29

fix(systemaddon): Add manual migration component. Closes mozilla#2787

ca9508b

piatra force-pushed the bug-2787-manual-migratio branch from fd24981 to ca9508b Compare July 24, 2017 12:52

piatra assigned Mardak and unassigned piatra Jul 24, 2017

Mardak suggested changes Jul 24, 2017

View reviewed changes

Refactor event listeners

52c52ae

Mardak approved these changes Jul 24, 2017

View reviewed changes

Mardak merged commit 131c7d8 into mozilla:master Jul 24, 2017

as-pine-proxy added the pushed-to-pine label Jul 24, 2017

Mardak mentioned this pull request Jul 24, 2017

Merge latest strings from activity-stream changeset db17225800dff8a1432837fa39bbe489273ebf85 mozilla/activity-stream-l10n#5

Merged

Mardak added PR / R+ and removed PR / Needs review labels Jul 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Manual Migration component. #2858

Add Manual Migration component. #2858

piatra commented Jul 14, 2017

coveralls commented Jul 14, 2017

coveralls commented Jul 20, 2017

piatra commented Jul 20, 2017

coveralls commented Jul 20, 2017

Mardak left a comment

Mardak Jul 21, 2017

Mardak Jul 21, 2017

Mardak Jul 21, 2017

Mardak Jul 21, 2017

Mardak Jul 21, 2017

Mardak Jul 21, 2017

Mardak Jul 21, 2017

Mardak Jul 21, 2017

Mardak Jul 21, 2017

Mardak Jul 21, 2017

Mardak Jul 21, 2017

piatra commented Jul 22, 2017 •

edited

Loading

Mardak commented Jul 22, 2017

coveralls commented Jul 23, 2017

coveralls commented Jul 23, 2017

coveralls commented Jul 24, 2017

coveralls commented Jul 24, 2017

coveralls commented Jul 24, 2017

piatra commented Jul 24, 2017

Mardak left a comment

Mardak Jul 24, 2017

Mardak Jul 24, 2017

Mardak Jul 24, 2017

Mardak Jul 24, 2017

Mardak Jul 24, 2017

coveralls commented Jul 24, 2017

Mardak left a comment

as-pine-proxy commented Jul 24, 2017


		assert.calledOnce(migrationSpy);

		assert.calledTwice(fakePrefs.get);

Add Manual Migration component. #2858

Add Manual Migration component. #2858

Conversation

piatra commented Jul 14, 2017

coveralls commented Jul 14, 2017

coveralls commented Jul 20, 2017

piatra commented Jul 20, 2017

coveralls commented Jul 20, 2017

Mardak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

piatra commented Jul 22, 2017 • edited Loading

Mardak commented Jul 22, 2017

coveralls commented Jul 23, 2017

coveralls commented Jul 23, 2017

coveralls commented Jul 24, 2017

coveralls commented Jul 24, 2017

coveralls commented Jul 24, 2017

piatra commented Jul 24, 2017

Mardak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Jul 24, 2017

Mardak left a comment

Choose a reason for hiding this comment

as-pine-proxy commented Jul 24, 2017

piatra commented Jul 22, 2017 •

edited

Loading