1069816 - Desktop client needs the ability to import contacts

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect)

Description

[Mike de Boer [:mikedeboer]]

This bug is about hooking up the UI with the work done in bug 972079, which implements a CardDAV import service to retrieve a list of contacts from an external source.

Work that is required here is
1. Add a button to the contacts tab as shown in the MVP spec (see URL)
2. Add a small form that allows the user to provide credentials to their Google account
3. Upon submitting the form, the import should start and the contacts list augmented with the new contacts

Comment 1

[Mike de Boer [:mikedeboer]]

Marco, can you add this bug to the current iteration?

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: implement Google contacts import class

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: add unit tests for the GoogleImporter class

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3: WIP enable import button in the contacts list

Needs more work to show a spinner when an import is in progress, as the spec dictates.

Comment 5

[Adam Roach [:abr]]

I'm marking this confidential, at least for the moment, because the attached patch has API keys in it that are presumably not supposed to be made public.

Comment 6

[Adam Roach [:abr]]

Comment on attachment 8495971 [details] [diff] [review]
Patch 1: implement Google contacts import class

Review of attachment 8495971 [details] [diff] [review]:
-----------------------------------------------------------------

I'm going to keep looking at this patch, but I wanted to get the ball rolling on the client-id and client-secret handling sooner rather than later.

::: browser/app/profile/firefox.js
@@ +1615,5 @@
>  pref("loop.feedback.product", "Loop");
>  pref("loop.debug.websocket", false);
>  pref("loop.debug.sdk", false);
> +pref("loop.oauth.google.client_id", "448087681106-7tcq1ftf8se04orgl9blntc11kapco0l.apps.googleusercontent.com");
> +pref("loop.oauth.google.client_secret", "bYPxAJQvE2sdfdqWA71J35Lh");

We can't write these into the firefox.js file -- or anywhere else in the tree -- because they're not, well, secret if you do that.

I believe that the way we currently handle this is with private files used in the production of the official binaries, set up through configuration; see, for example:

http://dxr.mozilla.org/mozilla-central/source/configure.in#3959

Also:
https://bugzilla.mozilla.org/show_bug.cgi?id=882485
https://bugzilla.mozilla.org/show_bug.cgi?id=977448
https://bugzilla.mozilla.org/show_bug.cgi?id=1014367

I would assume releng knows how this works -- also, Doug Turner seems to know a lot about this.

To avoid having to do special voodoo when building Firefox locally, I would propose reading prefs like "loop.oauth.google.client_id_overrride" and "loop.oauth.google.client_secret_override"; and, if they're not present, using the ones made available through the build-system-defined symbols. Then, you can drop these values into local prefs in the profile you're using for testing.

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Adam Roach [:abr] from comment #5)
> I'm marking this confidential, at least for the moment, because the attached
> patch has API keys in it that are presumably not supposed to be made public.

I left the credentials in there intentionally, because I wanted you to be able to apply the patch. I'll remove the account after the review cycles have passed.

I worked on the Translation feature, which uses the Bing service, which uses a similar setup. I'll copy that so that the official credentials will be in the Releng build config.

Comment 8

[Adam Roach [:abr]]

Comment on attachment 8495971 [details] [diff] [review]
Patch 1: implement Google contacts import class

Review of attachment 8495971 [details] [diff] [review]:
-----------------------------------------------------------------

Looking mostly good, but there are a few places that still need some fix-up.

The biggest issue is with code that's not in here: we need logic that handles the situation where a contact has been removed from the Google address book. Since we don't allow local modifications, this will cause an unremovable "ghost entry" to be stuck in the user's local address book. Basically, during the import, you need to iterate the local database for all Google-imported contacts, and remove any that aren't present in the set received from Google.

::: browser/components/loop/GoogleImporter.jsm
@@ +40,5 @@
> +  for (let [field, nodeName] of fieldMap) {
> +    let nodeList = ns ? node.getElementsByTagNameNS(ns, nodeName) :
> +                        node.getElementsByTagName(nodeName);
> +    if (nodeList.length) {
> +      let value = nodeList[0].firstChild.nodeValue;

Doesn't this mean that we'll only get the first value for potentially multivalued fields? I would think the function documentation should highlight this caveat.

@@ +49,5 @@
> +};
> +
> +/**
> + * Fetch the preferred email address of a contact. Returns the first address
> + * when no preferred flag is set.

Document or fix: "...throws an exception if the email list is empty."

@@ +65,5 @@
> +  });
> +  return email;
> +};
> +
> +let AUTH_WIN, AUTH_SECRET;

I'm a little confused about your selected naming conventions here -- typically, an all-caps symbol is used to indicate a const variable. These do not appear to be const -- perhaps gAuthWin and gAuthSecret?

@@ +83,5 @@
> + * Authentication is performed using an OAuth strategy which is loaded in a popup
> + * window.
> + *
> + * @param {Object} urls Map of API endpoint URLs to the Google Services. This is
> + *                      made configurable for unit tests.

The idea behind having options passed to the importer when "startImport" is called is to avoid constuctor parameters (so that values can change on a per-invocation basis). Just as a matter of precedent, you might want to consider moving these to be part of the options object.

@@ +102,5 @@
> +   *    pair.
> +   * 2. Get the tokenset from the Google service, using the authentication code
> +   *    that was retrieved in task 1.
> +   * 3. Fetch all the contacts from the Google service, using the OAuth tokenset
> +   *    that wa retrieved in task 2.

s/wa/was/

@@ +152,5 @@
> +      } catch (ex) {
> +        callback(ex);
> +        return;
> +      }
> +      

Trailing whitespace

@@ +154,5 @@
> +        return;
> +      }
> +      
> +      callback(null, stats);
> +    }.bind(this));

Given that Task returns a promise, this seems a bit long winded. One of the things I most like about using Tasks is that you don't need to check for exceptions everywhere -- an uncaught exception causes the Task's promise to be rejected. I think you can replace this with something more of the form:

startImport: function(options, callback, db, windowRef) {
  Task.spawn(function* () {
    let code = yield this._promiseAuthCode(windowRef);
    let tokenSet = yield this._promiseTokenSet(code);
    let contactEntries = yield this._promiseContactEntries(tokenSet);
    return yield this._processContacts(contactEntries, db);
  }.bind(this)).then(result => callback(null, result),
                     error => callback(error)).
                then(null, ex => log.error(ex.fileName + ":" + ex.lineNumber + ": " + ex.message));
}

@@ +174,5 @@
> +
> +    windowRef = windowRef.get();
> +    if (!windowRef) {
> +      deferred.reject(new Error("Google import requires a reference to a chrome window"));
> +      return;

This won't do what you think it will do. Returning something other than a promise here will cause the yield up in startImport to assign the returned value (null) to "code" and then continue execution.

I think what you mean is "throw new Error(...);", which will cause the task to abort, and its promise to be rejected.

@@ +189,5 @@
> +      pollTimeout = setTimeout(() => {
> +        pollTimeout = null;
> +        if (!AUTH_WIN || AUTH_WIN.closed) {
> +          AUTH_WIN = null;
> +          return;

I think you need to call deferred.reject here, right? Otherwise the Task promise never gets resolved if the user closes the window prior to granting permission.

@@ +208,5 @@
> +          deferred.resolve(message.trim());
> +        } else {
> +          deferred.reject(new Error("Unknown response from Google"));
> +        }
> +      }, 20);

20 seems unnecessarily aggressive here -- I'm worried about the performance implications of checking so frequently. I believe we could bump this to 200 without any real user-perceived lag after authorization.

@@ +445,5 @@
> +          value: emailNode.getAttribute("address")
> +        });
> +      }
> +    } else {
> +      // Contacts without email address(es) are not useful to us at all.

This isn't true -- when making direct calls to FirefoxOS Loop users, the primary identifier will be phone numbers. We should be fully prepared to deal with importing contacts that lack an email address but which have a phone number.

@@ +463,5 @@
> +          value: phoneNode.firstChild.nodeValue
> +        });
> +      }
> +    }
> +

*Here* is where I would check that the contact has at least an email address or a phone number.

@@ +482,5 @@
> +      // Contacts without an email address are not useful to us at all.
> +      if (!contact.email || !contact.email.length) {
> +        return null;
> +      }
> +      contact.name = [getPreferredEmail(contact).value];

I'd actually try for some more friendly options before reaching for the email address. It's pretty common for companies to be filed with no name, but with an affiliation. If you look at the CardDav importer, it does:

    // Basic sanity checking: make sure the name field isn't empty
    if (!("name" in contact) || contact.name[0].length == 0) {
      if (("familyName" in contact) && ("givenName" in contact)) {
        // First, try to synthesize a full name from the name fields.
        // Ordering is culturally sensitive, but we don't have
        // cultural origin information available here. The best we
        // can really do is "family, given additional"
        contact.name = [contact.familyName[0] + ", " + contact.givenName[0]];
        if (("additionalName" in contact)) {
          contact.name[0] += " " + contact.additionalName[0];
        }
      } else {
        if (nickname) {
          contact.name = [nickname];
        } else if ("familyName" in contact) {
          contact.name = [contact.familyName[0]];
        } else if ("givenName" in contact) {
          contact.name = [contact.givenName[0]];
        } else if ("org" in contact) {
          contact.name = [contact.org[0]];
        } else if ("email" in contact) {
          contact.name = [contact.email[0].value];
        } else if ("tel" in contact) {
          contact.name = [contact.tel[0].value];
        }
      }
    }

I would suggest you do something similar.

@@ +484,5 @@
> +        return null;
> +      }
> +      contact.name = [getPreferredEmail(contact).value];
> +    }
> +

...although I'll note that you appear to be importing contacts that have neither a phone number nor an email address. These are, in fact, of limited utility, and you may want to exclude them from the imported contacts.

Comment 9

[Adam Roach [:abr]]

Comment on attachment 8495971 [details] [diff] [review]
Patch 1: implement Google contacts import class

Review of attachment 8495971 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/GoogleImporter.jsm
@@ +357,5 @@
> +          return;
> +        }
> +
> +        // Check if the contact is already present in the database.
> +        db.getByServiceId(contact.id, function(err, obj) {

Sorry; I missed this in my review: This is going to have bad behavior on re-import. If a field has changed in Google, we want it to be changed locally, rather than having the value ignored. In fact, to prevent ghost fields from hanging around, what you really want to do here is remove the old local entry and replace it entirely with the new one. Compare with CardDavImporter:

        let existing = yield this._dbPromise(db, "getByServiceId", contact.id);
        if (existing) {
          yield this._dbPromise(db, "remove", existing._guid);
        }

Comment 10

[Adam Roach [:abr]]

Comment on attachment 8495971 [details] [diff] [review]
Patch 1: implement Google contacts import class

Review of attachment 8495971 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/GoogleImporter.jsm
@@ +215,5 @@
> +    let url = this.urls.oauth + "auth?response_type=code";
> +    for (let param of ["client_id", "redirect_uri", "scope"]) {
> +      url += "&" + param + "=" + encodeURIComponent(Services.prefs.getCharPref("loop.oauth.google." + param));
> +    }
> +    AUTH_SECRET = uuidgen.generateUUID().toString();

One last question -- why do we store this? We don't seem to actually verify it anywhere. I don't claim to understand the OAuth flow 100%, but my guess is that we're supposed to check this once permission has been granted?

Comment 11

[Adam Roach [:abr]]

Comment on attachment 8495973 [details] [diff] [review]
Patch 2: add unit tests for the GoogleImporter class

Review of attachment 8495973 [details] [diff] [review]:
-----------------------------------------------------------------

This looks mostly good, although it really needs to run the import twice and make sure you don't end up importing entries more than once. Clearing the review flag until the rest of the files are in here. :)

::: browser/components/loop/test/mochitest/browser.ini
@@ +2,5 @@
>  support-files =
> +    fixtures/google_auth.txt
> +    fixtures/google_contacts.txt
> +    fixtures/google_token.txt
> +    google_service.sjs

I think you may have forgotten to add these files to the patch.

Comment 12

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: implement Google contacts import class

Adam, this also includes an approach to dynamically load and/ or override the Google API credentials; it's the same as the translation project implemented not too long ago.

A next step would be to have 'someone' provision a Google OAuth Application that we can distribute with Fx. Do you know who I can contact for this?

Comment 13

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: add unit tests for the GoogleImporter class

Now with the files hg added :)

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.1: enable import button in the contacts list

Comment 15

[Maire Reavy [:mreavy]]

(In reply to Mike de Boer [:mikedeboer] from comment #12)
> Created attachment 8497787 [details] [diff] [review]
> Patch 1.1: implement Google contacts import class
> 
> Adam, this also includes an approach to dynamically load and/ or override
> the Google API credentials; it's the same as the translation project
> implemented not too long ago.
> 
> A next step would be to have 'someone' provision a Google OAuth Application
> that we can distribute with Fx. Do you know who I can contact for this?

Mike -- What are the implications of doing this?  Is this something you suggest for Fx34 or "for the future"?

Comment 16

[Mike de Boer [:mikedeboer]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #15)
> Mike -- What are the implications of doing this?  Is this something you
> suggest for Fx34 or "for the future"?

For Fx34, for sure!

Comment 17

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.2: implement Google contacts import class

Adam, pardon the noise, but I made some adjustments the the authentication window polling task.

Comment 18

[Adam Roach [:abr]]

Comment on attachment 8497787 [details] [diff] [review]
Patch 1.1: implement Google contacts import class

Review of attachment 8497787 [details] [diff] [review]:
-----------------------------------------------------------------

Looking really good, but we still have some rough edges here regarding error handling and the contact reimportation and purging logic. It should be a pretty small set of changes, but I'd like to look at this again. Re-review should also be fast (assuming you don't do any major refactoring. :) )

::: browser/components/loop/GoogleImporter.jsm
@@ +59,5 @@
> + * Helper function that reads the type of (email-)address or phone number from an
> + * XMLDOMNode.
> + *
> + * @param {XMLDOMNode} node
> + * @returns String that depicts the type of 

This line of documentation seems to be incomplete.

@@ +79,5 @@
> + * when no preferred flag is set.
> + *
> + * @param {Object} contact The contact object to check for email adresses.
> + * @throws An Error when no email addresses are listed for this contact.
> + */

I believe we want this for phone as well -- I'd refactor as a "getPreferred" that takes an array as its parameter; and then a "getPreferredEmail" that basically calls "getPreferred(contact.email)" and a "getPreferredPhone" that calls "getPreferred(contact.tel)"

@@ +204,5 @@
> +        clearTimeout(pollTimeout);
> +        pollTimeout = setTimeout(() => {
> +          pollTimeout = null;
> +          if (!gAuthWindow || gAuthWindow.closed) {
> +            throw new Error("Popup window was closed before authentication succeeded");

I don't think this does what you want it to. The stack at this point is going to be rooted in the timeout handler, not the Task in startImport. Throwing an exception won't make it to the right place. I believe you want:

reject(new Error("Popup window was closed before authentication succeeded"));

@@ +263,5 @@
> +      request.setRequestHeader("Content-Type", "application/x-www-form-urlencoded");
> +
> +      request.onload = function() {
> +        if (request.status < 400) {
> +          let tokenSet = JSON.parse(request.responseText);

I think you want to do a "try {... JSON.parse ...} catch (e) {reject(e);}" block here, to ensure that the Task finds out if we throw an error in the JSON parsing.

@@ +338,5 @@
> +   * @param {LoopContacts} db             Instance of the LoopContacts database
> +   *                                      object, which will store the newly found
> +   *                                      contacts.
> +   * @returns An `Error` object upon failure or an Object with statistics in the
> +   *          following format: `{ total: 25, success: 13 }`.

update documentation to reflect "ids" field

@@ +352,5 @@
> +    for (let entry of contactEntries) {
> +      if (++processed % kContactsChunkSize === 0) {
> +        // Skip a beat every time we processed a chunk.
> +        yield new Promise(resolve => Services.tm.currentThread.dispatch(resolve,
> +                                       Ci.nsIThread.DISPATCH_NORMAL));

This is very elegant. I'm filing it away as a concise and clear way to yield CPU cycles in the middle of a large task.

On the other hand, I'm not sure it's necessary in *this* case, since you're yielding cycles waiting for DB responses for each entry.

@@ +356,5 @@
> +                                       Ci.nsIThread.DISPATCH_NORMAL));
> +      }
> +
> +      let contact = this._processContactFields(entry);
> +      if (typeof contact == "string") {

I don't think this is what we want. The way you've written this, we won't initially import a contact that lacks contact information. But, if the Google entry is subsequently modified to remove all contact information, we'll leave the contact in the contact list. That's only a minor defect; the major defect is that this entry will contain old contact information that the user clearly intended to remove.

I think the code in here needs to be of the form:

  let existing = yield db.promise("getByServiceId", contact);
  if (existing) {
    yield db.promise("remove", existing._guid);
  }

@@ +379,5 @@
> +   * Parse an XML node to map the appropriate data to MozContact field equivalents.
> +   *
> +   * @param {XMLDOMNode} entry The contact XML node in Google format to process.
> +   * @returns `null` if the contact entry appears to be invalid or an Object containing
> +   *          all the contact data found in the XML.

"...or the contact ID if the contact is valid but not useful for importation."

@@ +496,5 @@
> +            email = getPreferredEmail(contact);
> +          } catch (ex) {}
> +          if (email) {
> +            contact.name = [contact.email[0].value];
> +          } else if ("tel" in contact) {

I believe we'll want the same processing as we do on email: use the preferred phone number rather than the first one.

@@ +518,5 @@
> +   */
> +  _purgeContacts: Task.async(function* (ids, db) {
> +    let contacts = yield db.promise("getAll");
> +    for (let [id, contact] of Iterator(contacts)) {
> +      if (!ids[contact.id]) {

You need to be really careful here that you don't strip out locally-added contacts and contacts imported from other data sources.

if ((contact.category.indexOf("google") >= 0) && !ids[contact.id]) {

::: browser/components/loop/LoopContacts.jsm
@@ +803,5 @@
>    }
>  });
>  
> +let kWindowRef = null;
> +

This appears unused...? I think you want to remove it.

Comment 19

[Adam Roach [:abr]]

(In reply to Mike de Boer [:mikedeboer] from comment #17)
> Created attachment 8498148 [details] [diff] [review]
> Patch 1.2: implement Google contacts import class
> 
> Adam, pardon the noise, but I made some adjustments the the authentication
> window polling task.

I agree that this is a readability improvement. I have no comments on the changes to _promiseAuthCode (which is, I assume, the only difference between the patches -- splinter has no idea what's going on).

Comment 20

[Adam Roach [:abr]]

Comment on attachment 8498148 [details] [diff] [review]
Patch 1.2: implement Google contacts import class

See comment 18 for review.

Comment 21

[:Paolo Amadini]

Comment on attachment 8497789 [details] [diff] [review]
Patch 3.1: enable import button in the contacts list

I think we should enable the button for Nightly users only once we have the full functionality in place, including error reporting and the "in progress" state of the button. It should not be too difficult to get these.

Have you defined the re-entering behavior?

Comment 22

[Adam Roach [:abr]]

Comment on attachment 8498148 [details] [diff] [review]
Patch 1.2: implement Google contacts import class

Review of attachment 8498148 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/loop/GoogleImporter.jsm
@@ +515,5 @@
> +   */
> +  _purgeContacts: Task.async(function* (ids, db) {
> +    let contacts = yield db.promise("getAll");
> +    for (let [id, contact] of Iterator(contacts)) {
> +      if (!ids[contact.id]) {

In talking with Callek over IRC, I realized that my earlier solution (of checking whether the "group" field contains "google") still has the pitfall that importing contacts from one google account and then another will have the very unfortunate side-effect of removing all the contacts from the first one.

So I think perhaps we need to instead compare contact.id itself -- we should only consider a contact a candidate to be removed if it starts with ("http://www.google.com/m8/feeds/contacts/" + user_email_address). This will serve the purpose of both not removing non-google entries, *and* prevent one account from interacting poorly with another.

Comment 23

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.3: implement Google contacts import class

Comment 24

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.2: add unit tests for the GoogleImporter class

Comment 25

[Mike de Boer [:mikedeboer]]

(In reply to :Paolo Amadini from comment #21)
> Have you defined the re-entering behavior?

Yes, the re-entering behavior has been resolved and dealt with in Patch 1. I'll add the progress indicator as well as disabling the button while an import is busy.

Comment 26

[Mike de Boer [:mikedeboer]]

Attachment: Patch 3.2: enable import button in the contacts list

I think this should be enough; error reporting and/ or showing statistics after import is something we could/ should in a followup for Fx35.
This would require new strings anyway.

Comment 27

[Adam Roach [:abr]]

Comment on attachment 8498379 [details] [diff] [review]
Patch 1.3: implement Google contacts import class

Review of attachment 8498379 [details] [diff] [review]:
-----------------------------------------------------------------

r+, but it would be nice to fix the error text below before landing.

::: browser/components/loop/GoogleImporter.jsm
@@ +83,5 @@
> + * @throws An Error when no (preferred) entries are listed for this contact.
> + */
> +const getPreferred = function(contact, which = "email") {
> +  if (!(which in contact) || !contact[which].length) {
> +    throw new Error("No email addresses available.");

Nit: throw new Error("No " + which + " fields available.")

Comment 28

[Adam Roach [:abr]]

Comment on attachment 8498380 [details] [diff] [review]
Patch 2.2: add unit tests for the GoogleImporter class

Review of attachment 8498380 [details] [diff] [review]:
-----------------------------------------------------------------

r+, contingent on opening another bug.

Overall, I'm concerned about coverage here -- we don't check behavior on importing when contacts have been added or removed from Google's database, and there is no testing of error situations. I don't want to hold up landing this bug, but would like to see another one opened to handle re-import and rainy-day cases.

Suggestions for improvement below.

::: browser/components/loop/test/mochitest/browser_GoogleImporter.js
@@ +69,5 @@
> +  Assert.equal(c.email[0].type, "other", "Email type should match");
> +  Assert.equal(c.email[0].value, "lycnix", "Email should match");
> +  Assert.equal(c.email[0].pref, true, "Pref should match");
> +  Assert.equal(c.category[0], "google", "Category should match");
> +  Assert.equal(c.id, "http://www.google.com/m8/feeds/contacts/tester%40mochi.com/base/7", "UID should match and be scoped to provider");

Is there some reason we're still not testing reimport? It could be as simple as:

yield promiseImport();
Assert.equal(Object.keys(mockDb._store).length, 5, "Database should contain only five contact after reimport");

::: browser/components/loop/test/mochitest/google_service.sjs
@@ +90,5 @@
> +  if (!req.hasHeader("Authorization"))
> +    throw new HTTPError(401, "No Authorization header provided.");
> +
> +  let auth = req.getHeader("Authorization");
> +  if (!auth.startsWith("Bearer "))

Why don't you compare with "Bearer test-token"? We know what the bearer token will be, and it would be nice to double-check that it's being provided correctly here.

@@ +149,5 @@
> +    let inputStream = getInputStream(kBasePath + "google_contacts.txt");
> +    res.bodyOutputStream.writeFrom(inputStream, inputStream.available());
> +    inputStream.close();
> +  }
> +};

It seems to me that this could be a lot cleaner by defining a helper function:

function respondWithFile (res, fileName, mimeType) {
  res.setStatusLine("1.1", 200, "OK");
  res.setHeader("Content-Type", mimeType);
 
  let inputStream = getInputStream(kBasePath + fileName);
  res.bodyOutputStream.writeFrom(inputStream, inputStream.available());
  inputStream.close();
}

::: browser/components/loop/test/mochitest/head.js
@@ +241,5 @@
> +    callback(null);
> +  },
> +  promise: function(method, ...params) {
> +    return new Promise(resolve => {
> +      this[method](...params, (err, res) => resolve(res));

We actually do have some error returns in the mock database. You probably want to go ahead and handle them, so that mochi tests can also perform negative testing.

Comment 29

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 8498392 [details] [diff] [review]
Patch 3.2: enable import button in the contacts list

Maire asked if I could take on this review, so witching the flag.

Comment 30

[Adam Roach [:abr]]

Comment on attachment 8498392 [details] [diff] [review]
Patch 3.2: enable import button in the contacts list

Review of attachment 8498392 [details] [diff] [review]:
-----------------------------------------------------------------

Maire asked me to pick up this review if I had time, so I'm taking it off Paolo's hands.

r+ contingent on opening two new bugs (or opening one bug and adding a spinner)

::: browser/components/loop/content/js/contacts.jsx
@@ +279,5 @@
> +      navigator.mozLoop.startImport({
> +        service: "google"
> +      }, (err, stats) => {
> +        this.setState({ importBusy: false });
> +        // TODO: proper error and success reporting.

Add a bug number to this TODO.

@@ +338,5 @@
>              <ButtonGroup>
> +              <Button caption={this.state.importBusy
> +                               ? mozL10n.get("importing_contacts_progress_button")
> +                               : mozL10n.get("import_contacts_button")}
> +                      disabled={this.state.importBusy}

The UX design calls for a spinner on the button here as well. Please add a spinner, or open a new bug to add a spinner and indicate its number in a "TODO" comment here.

Comment 31

[Mike de Boer [:mikedeboer]]

Thanks for all the help!

Pushed to fx-team as:
https://hg.mozilla.org/integration/fx-team/rev/1d4b89d2fe4a
https://hg.mozilla.org/integration/fx-team/rev/e14db252186c
https://hg.mozilla.org/integration/fx-team/rev/eee08fb2a4a4

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1d4b89d2fe4a
https://hg.mozilla.org/mozilla-central/rev/e14db252186c
https://hg.mozilla.org/mozilla-central/rev/eee08fb2a4a4

Comment 33

[u279076]

Will verify in Nightly, will not verify in Fig unless we get a new build pre-uplift.

Comment 34

[u279076]

When I test this in today's Nightly I get a 401 error returned from Google. Is that what bug 1075675 (landed today) is meant to address?

Comment 35

[Maire Reavy [:mreavy]]

Yes, that's correct. The patch from Bug 1075675 is needed to verify this.

Comment 36

[Maire Reavy [:mreavy]]

FYI: There was a releng issue for Windows.  Catlee has kicked off a try for the bustage fix: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3bd866e3607b

Mac and Linux Nightlies should work now.

NOTE: In the process of our own testing, we found bug 1080094 and bug 1079959.  A fix for Bug 1079959 has already landed on fx-team, and the fix for bug 1080094 will land on fx-team as soon as the tree reopens (it already has an r+).

Comment 37

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

This still appears to be an issue on Linux, 35.0a1 (2014-10-09):
"DEPRECATION WARNING: _getTabForBrowser` is now deprecated, please use `getTabForBrowser
You may find more details about this deprecation at: https://developer.mozilla.org/docs/Mozilla/Tech/XUL/Method/getTabForBrowser
chrome://browser/content/tabbrowser.xml 416 _getTabForBrowser
chrome://browser/content/tabbrowser.xml 388 _getTabForContentWindow
chrome://browser/content/tabbrowser.xml 3394 onxblDOMTitleChanged
null 0 null
 Deprecated.jsm:79
this.editor is null urlbarBindings.xml:189
DEPRECATION WARNING: _getTabForBrowser` is now deprecated, please use `getTabForBrowser
You may find more details about this deprecation at: https://developer.mozilla.org/docs/Mozilla/Tech/XUL/Method/getTabForBrowser
chrome://browser/content/tabbrowser.xml 416 _getTabForBrowser
chrome://browser/content/tabbrowser.xml 388 _getTabForContentWindow
chrome://browser/content/tabbrowser.xml 3394 onxblDOMTitleChanged
null 0 null
 Deprecated.jsm:79
An unbalanced tree was written using document.write() causing data from the network to be reparsed. For more information https://developer.mozilla.org/en/Optimizing_Your_Pages_for_Speculative_Parsing auth:244
this.editor is null urlbarBindings.xml:189
DEPRECATION WARNING: _getTabForBrowser` is now deprecated, please use `getTabForBrowser
You may find more details about this deprecation at: https://developer.mozilla.org/docs/Mozilla/Tech/XUL/Method/getTabForBrowser
chrome://browser/content/tabbrowser.xml 416 _getTabForBrowser
chrome://browser/content/tabbrowser.xml 388 _getTabForContentWindow
chrome://browser/content/tabbrowser.xml 3394 onxblDOMTitleChanged
null 0 null
 Deprecated.jsm:79
POST https://accounts.google.com/o/oauth2/token [HTTP/2.0 400 Bad Request 146ms]
1412859323430	Loop.Importer.Google	ERROR	resource://app/modules/loop/GoogleImporter.jsm:269: 400 Bad Request Log.jsm:749"

Works fine on Mac.

Comment 38

[Mike de Boer [:mikedeboer]]

Attachment: Patch 4: hotfix

Comment 39

[Adam Roach [:abr]]

Comment on attachment 8502530 [details] [diff] [review]
Patch 4: hotfix

Review of attachment 8502530 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.

Comment 40

[Maire Reavy [:mreavy]]

try push with hotfix:  https://tbpl.mozilla.org/?tree=Try&rev=9243539a8d47

Comment 41

[Adam Roach [:abr]]

https://hg.mozilla.org/integration/fx-team/rev/b197c72ec2c2

Comment 42

[u279076]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #40)
> try push with hotfix:  https://tbpl.mozilla.org/?tree=Try&rev=9243539a8d47

Using the above try build I am now able to import Google contacts on Windows XP, Windows 8.1, Ubuntu 14.04, and Mac OS X 10.9.

(In reply to Florin Mezei, QA (:FlorinMezei) from comment #37)
> "DEPRECATION WARNING: _getTabForBrowser` is now deprecated, please use
> `getTabForBrowser
> You may find more details about this deprecation at:
> https://developer.mozilla.org/docs/Mozilla/Tech/XUL/Method/getTabForBrowser
> chrome://browser/content/tabbrowser.xml 416 _getTabForBrowser
> chrome://browser/content/tabbrowser.xml 388 _getTabForContentWindow
> chrome://browser/content/tabbrowser.xml 3394 onxblDOMTitleChanged
> null 0 null
>  Deprecated.jsm:79
> this.editor is null urlbarBindings.xml:189
> DEPRECATION WARNING: _getTabForBrowser` is now deprecated, please use
> `getTabForBrowser
> You may find more details about this deprecation at:
> https://developer.mozilla.org/docs/Mozilla/Tech/XUL/Method/getTabForBrowser
> chrome://browser/content/tabbrowser.xml 416 _getTabForBrowser
> chrome://browser/content/tabbrowser.xml 388 _getTabForContentWindow
> chrome://browser/content/tabbrowser.xml 3394 onxblDOMTitleChanged
> null 0 null
>  Deprecated.jsm:79
> An unbalanced tree was written using document.write() causing data from the
> network to be reparsed. For more information
> https://developer.mozilla.org/en/
> Optimizing_Your_Pages_for_Speculative_Parsing auth:244
> this.editor is null urlbarBindings.xml:189
> DEPRECATION WARNING: _getTabForBrowser` is now deprecated, please use
> `getTabForBrowser
> You may find more details about this deprecation at:
> https://developer.mozilla.org/docs/Mozilla/Tech/XUL/Method/getTabForBrowser
> chrome://browser/content/tabbrowser.xml 416 _getTabForBrowser
> chrome://browser/content/tabbrowser.xml 388 _getTabForContentWindow
> chrome://browser/content/tabbrowser.xml 3394 onxblDOMTitleChanged
> null 0 null
>  Deprecated.jsm:79

I think these errors are a red herring and not related to Loop.

> POST https://accounts.google.com/o/oauth2/token [HTTP/2.0 400 Bad Request
> 146ms]
> 1412859323430	Loop.Importer.Google	ERROR
> resource://app/modules/loop/GoogleImporter.jsm:269: 400 Bad Request
> Log.jsm:749"

I'm not seeing this error anymore on any platform using the try build from comment 40.

Comment 43

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8498379 [details] [diff] [review]
Patch 1.3: implement Google contacts import class

Approval Request Comment
Part of the staged Loop aurora second uplift set

Comment 44

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b197c72ec2c2

Comment 45

[Paul Silaghi, QA [:pauly]]

35.0a1 (2014-10-10) Win 7, Ubuntu 13.04, OS X 10.9.5
The import works now on all platforms, I'm calling this verified.
Found a new issue - bug 1081130

Comment 46

[Randell Jesup [:jesup] (needinfo me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c17712a1a653
https://hg.mozilla.org/releases/mozilla-aurora/rev/4ba581107e6f
https://hg.mozilla.org/releases/mozilla-aurora/rev/e15661e2c496
https://hg.mozilla.org/releases/mozilla-aurora/rev/f0b953a9d3a5
https://hg.mozilla.org/releases/mozilla-aurora/rev/eaa672372900

Comment 47

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 34b1 Win 7

Comment 48

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8498379 [details] [diff] [review]
Patch 1.3: implement Google contacts import class

Post landed approval (already landed)

Comment 49

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8498380 [details] [diff] [review]
Patch 2.2: add unit tests for the GoogleImporter class

Post landed approval (already landed)

Comment 50

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8498392 [details] [diff] [review]
Patch 3.2: enable import button in the contacts list

Post landed approval (already landed)

Comment 51

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8502530 [details] [diff] [review]
Patch 4: hotfix

Post landed approval (already landed)