Show automigration message on the Top Site section, fix #2616 #2650

gasolin · 2017-05-31T07:23:00Z

UI works, unit test is on the way, I think its ready for the first round review

You can enable this in about:config browser.migrate.automigrate.inpage.ui.enabled once Bug 1361286 is landed

coveralls · 2017-05-31T07:31:43Z

Coverage remained the same at 87.84% when pulling dd8c983 on gasolin:automigration into 943f19e on mozilla:master.

k88hudson

The code looks great in general, awesome work so far! ⭐️

What is the best way to manually test this?

k88hudson · 2017-05-31T15:34:07Z

system-addon/common/Reducers.jsm

+function AutoMigrate(prevState = INITIAL_STATE.AutoMigrate, action) {
+  switch (action.type) {
+    case at.AUTOMIGRATE_HIDE: {
+      return Object.assign({}, prevState, {


This doesn't need to Object.assign if you are replacing every value, right?

k88hudson · 2017-05-31T15:34:07Z

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

+const {FormattedMessage, injectIntl} = require("react-intl");
+const {actionTypes: at, actionCreators: ac} = require("common/Actions.jsm");
+
+class AutoMigratePrompt extends React.Component {


You can just use a pure function here, i.e.

const AutoMigratePrompt = props => { // render stuff here };

k88hudson · 2017-05-31T15:34:07Z

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

+      (<div className="migrate-prompt">
+        <span>{this.props.AutoMigrate.msg}</span>
+        <div className="confirm">
+          <button onClick={this.props.onUndoClick.bind(this)}><FormattedMessage id="migrate_undo_button" /></button>


If you use a pure function, you might have to do onClick={() => props.onUndoClick()}

We should probably not do the pure function and bind the function in constructor as binding from render will cause render optimizations to fail:
https://github.com/yannickcr/eslint-plugin-react/blob/master/docs/rules/jsx-no-bind.md

Actually, if onUndoClick is a prop, this probably shouldn't be AutoMigratePrompt anyway? So instead just do onClick={this.props.onUndoClick}

actually yeah, the this isn't needed at all

k88hudson · 2017-05-31T15:34:07Z

system-addon/content-src/components/AutoMigratePrompt/_AutoMigratePrompt.scss

+  cursor: pointer;
+
+  &:hover {
+      box-shadow: 0 0 0 5px $faintest-black;


indentation is wrong here

k88hudson · 2017-05-31T15:34:07Z

system-addon/lib/AutoMigrateFeed.jsm

+          } else {
+            this.store.dispatch(ac.BroadcastToContent({type: at.AUTOMIGRATE_HIDE}));
+          }
+        }).catch(() => {});


Maybe Cu.reportError here

k88hudson · 2017-05-31T15:34:07Z

system-addon/lib/AutoMigrateFeed.jsm

+          }
+        }).catch(() => {});
+        break;
+      default:


This default block is not necessary

k88hudson · 2017-05-31T15:34:07Z

system-addon/lib/AutoMigrateFeed.jsm

+        this.store.dispatch(ac.BroadcastToContent({type: at.AUTOMIGRATE_HIDE}));
+        break;
+      case at.AUTOMIGRATE_UNDO_MIGRATION:
+        AutoMigrate.undoAutoMigration(this.window);


I think it might be better to use action._target.browser (that is, from the AUTOMIGRATE_UNDO_MIGRATION action), since there could be multiple windows open at once

k88hudson · 2017-05-31T15:34:07Z

system-addon/lib/AutoMigrateFeed.jsm

+      case at.NEW_TAB_VISIBLE:
+        AutoMigrate.shouldShowMigratePrompt(action._target.browser).then(prompt => {
+          if (prompt) {
+            this.window = action._target.browser.ownerGlobal;


See above comment on line 23, this might cause problems since there could be multiple windows open at once

k88hudson · 2017-05-31T15:34:07Z

system-addon/lib/AutoMigrateFeed.jsm

+              }));
+            }
+          } else {
+            this.store.dispatch(ac.BroadcastToContent({type: at.AUTOMIGRATE_HIDE}));


Could you perhaps do

else if (this.store.getState().Automigrate.display) { ... }

to prevent unnecessary messages/updates?

gasolin · 2017-06-01T03:11:15Z

Here's how to do the manual testing:

enable the pref browser.migrate.automigrate.inpage.ui.enabled in about:config,
and ./mach run with new profile

run ./mach run -P to create a new profile named 'something', then exit without running
run ./mach run -P something --migration to trigger the automigration UI

coveralls · 2017-06-01T04:01:20Z

Coverage remained the same at 87.84% when pulling 0d24c0a on gasolin:automigration into 943f19e on mozilla:master.

coveralls · 2017-06-01T10:47:39Z

Coverage remained the same at 87.84% when pulling 914669e on gasolin:automigration into 943f19e on mozilla:master.

coveralls · 2017-06-01T10:51:27Z

Coverage remained the same at 87.84% when pulling 914669e on gasolin:automigration into 943f19e on mozilla:master.

coveralls · 2017-06-02T03:52:07Z

Coverage remained the same at 87.84% when pulling dc922dc on gasolin:automigration into 18f2c9e on mozilla:master.

gasolin · 2017-06-05T06:29:23Z

tests added, ready for review again 😊

coveralls · 2017-06-05T06:33:34Z

Coverage remained the same at 87.845% when pulling 14e8b99 on gasolin:automigration into 36f24d5 on mozilla:master.

coveralls · 2017-06-05T07:04:31Z

Coverage remained the same at 87.845% when pulling 21b5e0b on gasolin:automigration into 36f24d5 on mozilla:master.

gasolin · 2017-06-05T07:08:36Z

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

+      </div>
+    </div>) : (<div className="migrate-prompt">
+      <FormattedMessage id="migrate_manual_import_msg" />&nbsp;
+      <FormattedMessage id="migrate_manual_import_link_msg" />&nbsp;


this message would be a link to trigger the manual import wizard, which will be implemented in bug 1361294
For l10n team friendly I think its better to split strings early before we actually implement that function.

I see that AutoMigrate will be passing over at least one of the messages. Any reason why it shouldn't pass over the manual import messages too? I do see that parts of it will be linked, but multiple message parts could be passed over too.

coveralls · 2017-06-05T07:10:09Z

Coverage remained the same at 87.845% when pulling 41927a3 on gasolin:automigration into 36f24d5 on mozilla:master.

gasolin · 2017-06-08T01:25:18Z

@k88hudson or @Mardak could you help review this patch?

Mardak

Looks like there's some untested code causing coverage failures that would have resulted in runtime failures if it were executed. There's also some string issues with implicit concatenation although I seem to recall running into some issues with needing dangerously setting html for nesting links? @k88hudson ?

Mardak · 2017-06-08T09:27:05Z

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

+    globals = new GlobalOverrider();
+    globals.set("AutoMigrate", {
+      keepAutoMigration: globals.sandbox.stub(),
+      shouldShowMigratePrompt: globals.sandbox.stub().resolves(true),


To get (100%) coverage tests passing, you'll need to test the false case of shouldShowMigratePrompt

Mardak · 2017-06-08T09:27:05Z

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

+      });
+
+      assert.calledOnce(global.AutoMigrate.shouldShowMigratePrompt);
+    });


Similarly for coverage, there should be additional tests for the various code paths handling NEW_TAB_VISIBLE

Mardak · 2017-06-08T09:27:05Z

locales/en-US/strings.properties

+migrate_manual_import_msg=No problem. We can start you off with a few popular sites instead. You can always
+# LOCALIZATION NOTE (migrate_manual_import_link_msg): This is shown as the link in message
+migrate_manual_import_link_msg=import your information
+migrate_manual_import_msg_trail=later.


These strings need to be composed without implicit space-concatenation. If the message structure can't be changed, it'll probably look like "No problem. … You can always {import_link} later." Although even that seems odd to localize given the fragment within a sentence. Perhaps if it can be just two separate but adjacent messages, e.g., "No problem. … popular sites instead." and "Import your information when you're ready." with the latter sentence being the clickable link.

I do see that you're just using the strings from https://mozilla.invisionapp.com/share/Y3BGDY88H#/screens/234912781 via https://bugzilla.mozilla.org/show_bug.cgi?id=1360109 so I've relayed these comments in a ni? there.

Commented https://bugzilla.mozilla.org/show_bug.cgi?id=1361294#c1

I see that flod says it won't be difficult to localize, so maybe we just need to fix the implicit concatenation issue with {replacement}. Although actually given that we have a before-link and after-link text, that should provide localizers with enough flexibility to work with? @flodolo

Ehh. I could see some locales wanting to just put "." in the trailing text, so if there's an implicit space to join, it would appear as "Stuff stuff later [link] ."

Empty spaces in .properties files are a pain. I would go with:

# LOCALIZATION NOTE (migrate_manual_import_link_msg): do not translate {link}, # it's replaced by an active link using migrate_manual_import_link as text migrate_manual_import_msg=No problem. We can start you off with a few popular sites instead. You can always {link} later. migrate_manual_import_link=import your information

Mardak · 2017-06-08T09:27:05Z

locales/en-US/strings.properties

+migrate_manual_import_link_msg=import your information
+migrate_manual_import_msg_trail=later.
+migrate_import_button=Import It
+migrate_undo_button=Don't Import


Behind the scenes, we're doing an undo; but that might be confusing to expose it that way to translators. Probably best to just call this migrate_dont_import_button

Mardak · 2017-06-08T09:27:05Z

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

+
+  return props.AutoMigrate.stage === 0 ?
+    (<div className="migrate-prompt">
+      <span>{props.AutoMigrate.msg}</span>


Just making sure, this msg should be the success message, e.g., "Dive right into Firefox! Import your favorite sites, bookmarks, history and passwords from Chrome" ? Maybe a more descriptive name will help -- migratedMessage ?

Mardak · 2017-06-08T09:27:05Z

system-addon/common/Reducers.jsm

+    case at.AUTOMIGRATE_IS_REVERTED: {
+      return {
+        display: true,
+        stage: 1,


@k88hudson not sure if you have preferences on how this stage / state should be handled. With just numbers as here? Or strings or enum-ish? Or I suppose completely different is just deriving both display and stage from the component and storing the state as "HIDDEN" "MIGRATED" "REVERTED" ?

Mardak · 2017-06-08T09:27:05Z

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

+  state => ({AutoMigrate: state.AutoMigrate}),
+  dispatch => ({
+    onImportClick: () => dispatch(ac.SendToMain({type: at.AUTOMIGRATE_MIGRATE_DONE})),
+    onUndoClick: () => dispatch(ac.SendToMain({type: at.AUTOMIGRATE_UNDO_MIGRATION}))


I suppose following this pattern, we would want to add a onManualImportClick ?

yes, it will be in the followup patch

Mardak · 2017-06-08T09:27:05Z

system-addon/lib/AutoMigrateFeed.jsm

+      case at.NEW_TAB_VISIBLE:
+        AutoMigrate.shouldShowMigratePrompt(action._target.browser).then(prompt => {
+          if (prompt) {
+            let browserName = Services.prefs.getStringPref("browser.migrate.automigrate.browser", "");


Why is there this pref logic when home/newtab usage doesn't check? https://dxr.mozilla.org/mozilla-central/search?q=%2Bref%3A%22AutoMigrate%23shouldShowMigratePrompt%22

The logic seems a bit odd in having Activity Stream seemingly randomly check a pref to do something different and somewhat overriding the just called shouldShowMigratePrompt. Is it possible to have that should method indicate what type of prompt should be shown instead of that a prompt should be shown?

Mardak · 2017-06-08T09:27:05Z

system-addon/lib/AutoMigrateFeed.jsm

+                data: {msg}
+              }));
+            }
+          } else if (this.store.getState().Automigrate.display) {


This looks like a typo? Uncapitalized "M" in AutoMigrate ?

Mardak · 2017-06-08T09:27:05Z

system-addon/lib/AutoMigrateFeed.jsm

+        AutoMigrate.undoAutoMigration(action._target.browser.ownerGlobal);
+        this.store.dispatch(ac.BroadcastToContent({type: at.AUTOMIGRATE_IS_REVERTED}));
+        break;
+      case at.NEW_TAB_VISIBLE:


Overall this logic seems a bit strange in that on visibility of a single new tab, there's broadcasts to all tabs?

coveralls · 2017-06-15T04:49:05Z

Coverage remained the same at 87.938% when pulling 723eb68 on gasolin:automigration into f24208f on mozilla:master.

gasolin · 2017-06-15T04:50:38Z

Rebased and added manual import wizard link PR. @Mardak @k88hudson Due to priority shift I have been asked to help on other photon things. So I will not continue to finish these patch. Still thanks for your help!

k88hudson · 2017-07-17T17:51:34Z

Closing this as automigration is no longer being impelmented as such.

k88hudson self-assigned this May 31, 2017

k88hudson suggested changes May 31, 2017

View changes

k88hudson added the PR / Needs work label May 31, 2017

k88hudson assigned gasolin and unassigned k88hudson May 31, 2017

gasolin force-pushed the gasolin:automigration branch from dd8c983 to 0d24c0a Jun 1, 2017

gasolin force-pushed the gasolin:automigration branch 2 times, most recently from 3239322 to 914669e Jun 1, 2017

gasolin force-pushed the gasolin:automigration branch from 914669e to dc922dc Jun 2, 2017

gasolin force-pushed the gasolin:automigration branch from dc922dc to 14e8b99 Jun 5, 2017

gasolin force-pushed the gasolin:automigration branch 2 times, most recently from 21b5e0b to 41927a3 Jun 5, 2017

gasolin reviewed Jun 5, 2017

View changes

Mardak suggested changes Jun 8, 2017

View changes

gasolin added 2 commits May 23, 2017

Show automigration message on the Top Site section, fix #2616

23df8d0

show import from other browser link

723eb68

gasolin force-pushed the gasolin:automigration branch from 41927a3 to 723eb68 Jun 15, 2017

Mardak mentioned this pull request Jun 28, 2017

Integrate manual migration UI and functionality into Activity Stream #2787

Closed

k88hudson closed this Jul 17, 2017

mozilla / activity-stream Archived

Show automigration message on the Top Site section, fix #2616 #2650

Show automigration message on the Top Site section, fix #2616 #2650

Conversation

gasolin commented May 31, 2017 • edited

coveralls commented May 31, 2017

k88hudson left a comment

This comment has been minimized.

k88hudson May 31, 2017 Member

This comment has been minimized.

k88hudson May 31, 2017 Member

This comment has been minimized.

k88hudson May 31, 2017 Member

This comment has been minimized.

Mardak May 31, 2017 • edited Member

This comment has been minimized.

Mardak May 31, 2017 • edited Member

This comment has been minimized.

k88hudson May 31, 2017 Member

This comment has been minimized.

k88hudson May 31, 2017 Member

This comment has been minimized.

k88hudson May 31, 2017 Member

This comment has been minimized.

k88hudson May 31, 2017 Member

This comment has been minimized.

k88hudson May 31, 2017 Member

This comment has been minimized.

gasolin Jun 1, 2017 Author Contributor

This comment has been minimized.

k88hudson May 31, 2017 Member

This comment has been minimized.

gasolin Jun 1, 2017 Author Contributor

This comment has been minimized.

k88hudson May 31, 2017 Member

This comment has been minimized.

gasolin Jun 1, 2017 Author Contributor

gasolin commented Jun 1, 2017

coveralls commented Jun 1, 2017

coveralls commented Jun 1, 2017

coveralls commented Jun 1, 2017

coveralls commented Jun 2, 2017

gasolin commented Jun 5, 2017

coveralls commented Jun 5, 2017

coveralls commented Jun 5, 2017

This comment has been minimized.

gasolin Jun 5, 2017 Author Contributor

This comment has been minimized.

Mardak Jun 8, 2017 Member

coveralls commented Jun 5, 2017

gasolin commented Jun 8, 2017

Mardak left a comment

This comment has been minimized.

Mardak Jun 8, 2017 Member

This comment has been minimized.

Mardak Jun 8, 2017 Member

This comment has been minimized.

Mardak Jun 8, 2017 Member

This comment has been minimized.

Mardak Jun 8, 2017 Member

This comment has been minimized.

Mardak Jun 8, 2017 Member

This comment has been minimized.

Mardak Jun 8, 2017 Member

This comment has been minimized.

flodolo Jun 8, 2017 Collaborator

This comment has been minimized.

Mardak Jun 8, 2017 Member

This comment has been minimized.

Mardak Jun 8, 2017 Member

This comment has been minimized.

Mardak Jun 8, 2017 Member

This comment has been minimized.

Mardak Jun 8, 2017 Member

This comment has been minimized.

gasolin Jun 9, 2017 Author Contributor

This comment has been minimized.

Mardak Jun 8, 2017 Member

This comment has been minimized.

Mardak Jun 8, 2017 Member

gasolin commented May 31, 2017 •

edited

k88hudson May 31, 2017
Member

k88hudson May 31, 2017
Member

k88hudson May 31, 2017
Member

Mardak May 31, 2017 •

edited

Member

Mardak May 31, 2017 •

edited

Member

k88hudson May 31, 2017
Member

k88hudson May 31, 2017
Member

k88hudson May 31, 2017
Member

k88hudson May 31, 2017
Member

k88hudson May 31, 2017
Member

gasolin Jun 1, 2017
Author Contributor

k88hudson May 31, 2017
Member

gasolin Jun 1, 2017
Author Contributor

k88hudson May 31, 2017
Member

gasolin Jun 1, 2017
Author Contributor

gasolin Jun 5, 2017
Author Contributor

Mardak Jun 8, 2017
Member

Mardak Jun 8, 2017
Member

Mardak Jun 8, 2017
Member

Mardak Jun 8, 2017
Member

Mardak Jun 8, 2017
Member

Mardak Jun 8, 2017
Member

Mardak Jun 8, 2017
Member

flodolo Jun 8, 2017
Collaborator

Mardak Jun 8, 2017
Member

Mardak Jun 8, 2017
Member

Mardak Jun 8, 2017
Member

Mardak Jun 8, 2017
Member

gasolin Jun 9, 2017
Author Contributor

Mardak Jun 8, 2017
Member

Mardak Jun 8, 2017
Member

Mardak Jun 8, 2017
Member