Improve apps v2 upgrade UX

remoting/webapp/unittests/apps_v2_migration_unittest.js

+// Copyright 2014 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+(function() {
+
+'use strict';
+
+var originalWindowsStorage = null;
+var mockIsAppsV2 = null;
+var mockChromeStorage = {};
+
+function pass() {
+  ok(true);
+  QUnit.start();
+}
+
+function fail() {
+  ok(false);
+  QUnit.start();
+}
+
+/**
+ * @param {string} v1UserName
+ * @param {string} v1UserEmail
+ * @param {string} currentEmail
+ * @param {boolean} v1HasHost
+ */
+function setup_(v1UserName, v1UserEmail, currentEmail, v1HasHosts) {

[Jamie, 2015/01/20 22:32:02]

Assuming you need this because the module's setup method can't take parameters,
it would be better if as much as possible of the set-up code were moved into
that method, with the remainder having a more specific name (setMigrationData,
perhaps?)

+  chromeMocks.activate(['storage']);
+  originalWindowsStorage = window.localStorage;
+  mockIsAppsV2 = sinon.stub(base, 'isAppsV2');
+  remoting.identity = {};

[Jamie, 2015/01/20 22:32:01]

There is already a MockIdentity class you can use here.

[kelvinp, 2015/01/21 21:16:21]

The mock identify class mocks chrome.identity but not remoting.identity.

+
+  remoting.identity.getUserInfo = function(onDone, onError) {
+    return onDone(currentEmail);
+  };
+
+  window.localStorage['remoting-email'] = v1UserEmail;
+  window.localStorage['remoting-fullname'] = v1UserName;
+
+  mockIsAppsV2.returns(false);
+  remoting.AppsV2Migration.savePreferences(v1HasHosts);
+}
+
+module('AppsV2Migration', {
+  teardown: function() {
+    chromeMocks.restore();
+    window.localStorage = originalWindowsStorage;
+    remoting.identity = null;
+    mockIsAppsV2.restore();
+  }
+});
+
+QUnit.asyncTest(
+  'hasHostsInV1App() should reject the promise if v1 user has same identity',
+  function() {
+    setup_('v1userName', 'v2user@gmail.com', 'v2user@gmail.com', true);
+    mockIsAppsV2.returns(true);
+    remoting.AppsV2Migration.hasHostsInV1App().then(fail, pass);
+});
+
+QUnit.asyncTest(
+  'hasHostsInV1App() should reject the promise if v1 user has no hosts',
+  function() {
+    setup_('v1userName', 'v1user@gmail.com', 'v2user@gmail.com', false);
+    mockIsAppsV2.returns(true);
+    remoting.AppsV2Migration.hasHostsInV1App().then(fail, pass);
+});
+
+QUnit.asyncTest(
+  'hasHostsInV1App() should reject always reject the promise in v1',

[Jamie, 2015/01/20 22:32:01]

Duplicate "reject".

[kelvinp, 2015/01/21 21:16:21]

Done.

+  function() {
+    setup_('v1userName', 'v1user@gmail.com', 'v2user@gmail.com', true);
+    mockIsAppsV2.returns(false);
+    remoting.AppsV2Migration.hasHostsInV1App().then(fail, pass);
+});
+
+QUnit.asyncTest(
+  'hasHostsInV1App() should return v1 identity if v1 user has hosts',
+  function() {
+    setup_('v1userName', 'v1user@gmail.com', 'v2user@gmail.com', true);
+    mockIsAppsV2.returns(true);
+    remoting.AppsV2Migration.hasHostsInV1App().then(
+      function(result) {
+        QUnit.equal(result.email, 'v1user@gmail.com');
+        QUnit.equal(result.fullName, 'v1userName');
+        pass();
+      }, fail
+    );
+});
+
+QUnit.asyncTest(
+  'savePreferences() should clear the preferences on v2',
+  function() {
+    setup_('v1userName', 'v1user@gmail.com', 'v2user@gmail.com', true);
+    mockIsAppsV2.returns(true);
+    remoting.AppsV2Migration.savePreferences(true);

[Jamie, 2015/01/20 22:32:01]

As stated earlier, there is some subtlety to this method, and I think it's worth
testing the various (hasHost, isAppsV2) scenarios.

[kelvinp, 2015/01/21 21:16:21]

I have removed the hostHost parameter from the method, it is probably just worth
testing for the v1 and v2 case

+    remoting.AppsV2Migration.hasHostsInV1App().then(fail, pass);
+});
+
+})();