1069962 - Add support for Gravatar avatars in the Contacts List

Status: VERIFIED FIXED

(Hello (Loop) :: Client, defect, P2)

Description

[Mike de Boer [:mikedeboer]]

Attachment: Patch v1: add support for Gravatar avatars

See summary.

This bug needs a privacy review, eventually.

Already has a patch, carrying over r=paolo.

Comment 1

[Marco Mucci [:MarcoM]]

Added to IT 35.2

Comment 2

[Mike de Boer [:mikedeboer]]

Pushed to fx-team as https://hg.mozilla.org/integration/fx-team/rev/b26c709330d6

Comment 3

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

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

Comment 4

[u279076]

Could I please get clearer steps to reproduce for testing purposes?

Comment 5

[Romain Testard [:RT]]

Per discussions with legal (Mika) this requires a door hanger in the UX to provide the user with choice of whether or not to use Gravatar (and share his contact's emails with Gravatar).

Given the on-going efforts it is not realistic to deliver this in Fx34 (new strings) or Fx35 (requires UX) - this is then being pushed to Fx36.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/c840d5b6e55f

Comment 7

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

Comment on attachment 8492201 [details] [diff] [review]
Patch v1: add support for Gravatar avatars

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 8

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

Comment on attachment 8492201 [details] [diff] [review]
Patch v1: add support for Gravatar avatars

Correction: backed out on m-c for now, so backed out on fig as well

Comment 9

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/abcc867400dc

Comment 10

[u279076]

I don't think this should be marked fixed since this bug is tracking adding support which was backed out for 34 and 35.

Comment 11

[:shell escalante]

find the dupe on this - need UX.  37 if we don't get UX in next 2 weeks.

Comment 12

[:shell escalante]

we are blocked from adding gravatars pending outcome of UX bug 1082950.  was determined that we would need to prompt for permission to use gravatar, during privacy review, and only send looking if granted.

this may be a dupe (or may not) of bugs off the gravatar UX bug.  putting this bug as depending on the gravatar UX permission.

Comment 13

[Gregory Szorc [:gps]]

https://hg.mozilla.org/projects/fig/rev/dd6fa9cd5b4b

Comment 14

[Maire Reavy [:mreavy]]

Forgot to reset the priority when I marked this Fx37?

Comment 15

[Maire Reavy [:mreavy]]

At this point, we'll do this during the Fx38 time cycle.  We may choose to uplift once the work is complete, but for now, I'm moving this to Fx38.

Comment 16

[Maire Reavy [:mreavy]]

Once this lands, we'll want to inform Mika so our privacy notice can be updated.

Comment 17

[Romain Testard [:RT]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #15)
> At this point, we'll do this during the Fx38 time cycle.  We may choose to
> uplift once the work is complete, but for now, I'm moving this to Fx38.

Maire, we had it planned and communicated for Fx37 cycle - why is this now being moved to Fx38?

Comment 18

[Maire Reavy [:mreavy]]

We have one week left in Fx37, and it was prioritized as a P3.  If Product needs this for Fx37, we should make this a P2 and bump it toward the top of the trello board.  

For Romain -- What is driving this? Who is asking for it?  

For Mike -- if this is prioritized, can you implement and land this bug this week (before Fx37 uplifts)?  (The initial point value was P=1.  Is that estimate still accurate?)

Comment 19

[Romain Testard [:RT]]

It was a P3 for Fx36. When it got moved to Fx37 we should have discussed the priority.
I don't see this one as a blocker - the only issue I have is we had it on the shared roadmap (my understanding was that most of the dev was done). If this is indeed a P=1 I'd rather have it on Fx37, if it's more complex agreed we should move it to Fx38.

Comment 20

[:shell escalante]

we'd like to land strings so we can land this bug in 37, knowing that the patch will take some patching because of the time idle.  also did need info on bug 1082950 to Mika on if the Learn More should point to our privacy notice or to gravatar's.  will update user story on this bug - but that shouldn't block strings this week or moving forward on bug

Comment 21

[Mike de Boer [:mikedeboer]]

First we'll need to implement the UX as specified in bug 1082950, before we can move forward with showing the gravatar profile pics.

Comment 22

[Maire Reavy [:mreavy]]

(In reply to Mike de Boer [:mikedeboer] from comment #21)
> First we'll need to implement the UX as specified in bug 1082950, before we
> can move forward with showing the gravatar profile pics.

Mike -- Can we use this bug to implement that UX and update the bug point value to match?  Also, do you want to take/own this bug (given the new scope) or should I/Gavin find someone else?  Thanks.

Comment 23

[:shell escalante]

i added bug 1118262 for implementing the permissions - if that wanted to be separate. either way, can we land strings before 37 uplift and so the bugs needed for the gravatar functionality can be uplifted to aurora?

Comment 24

[:shell escalante]

because of strings it needs to push to 38

Comment 25

[Mike de Boer [:mikedeboer]]

Adjusted the point value to include the UI update in bug 1082950. I will work on it during this iteration.

Comment 27

[Mike de Boer [:mikedeboer]]

Attachment: Part 1: fix the ui-showcase to load assets using relative paths

Comment 28

[Mike de Boer [:mikedeboer]]

Comment on attachment 8551282 [details] [diff] [review]
Part 1: fix the ui-showcase to load assets using relative paths

This has been sitting here quite a while. I think it's really useful to have, so requesting review from Mark.

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 8551282 [details] [diff] [review]
Part 1: fix the ui-showcase to load assets using relative paths

The relative paths still work with the standalone server, so r=me.

For some reason the 14x14 icons are overlapping onto the previous line, but it doesn't seem that simple to fix.

There's a bit of bitrot to fix but should be simplish to do.

Comment 30

[Mike de Boer [:mikedeboer]]

Pushed part 1 to fx-team: https://hg.mozilla.org/integration/fx-team/rev/fb12667e0147

Comment 31

[Ryan VanderMeulen [:RyanVM]]

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

Comment 32

[Mike de Boer [:mikedeboer]]

Attachment: Part 2: show a promo area in the contacts list for Gravatars

I'm posting this patch without unit tests at this point, they'll be arriving shortly.
I'm requesting review now, because I'd like to land _at least_ the strings before 38 (current nightly) gets uplifted to aurora. IOW, I'd be happy with an r+ on just the strings, Mark!

Comment 33

[Mark Banner (:standard8)]

Comment on attachment 8566254 [details] [diff] [review]
Part 2: show a promo area in the contacts list for Gravatars

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

r=Standard8 for just the string changes. I'll look at reviewing the rest of the bug asap, but need to go in more detail than I've got time for today.

::: browser/locales/en-US/chrome/browser/loop/loop.properties
@@ +167,5 @@
>  ## https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#contacts
>  video_call_menu_button=Video Conversation
>  
> +gravatars_promo_message=You can automatically add profile icons to your contacts \
> +  by sharing their email addresses with Gravatar. {{learn_more}}.

This should have a localisation note for the {{learn_more}} part, explaining that its a link with the text content of gravatars_promo_message_learnmore.

Comment 34

[Mike de Boer [:mikedeboer]]

Pushed strings patch with localization note to fx-team: https://hg.mozilla.org/integration/fx-team/rev/3abfc3b9ab90

Comment 35

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

https://hg.mozilla.org/mozilla-central/rev/3abfc3b9ab90

Comment 36

[Mark Banner (:standard8)]

Comment on attachment 8566254 [details] [diff] [review]
Part 2: show a promo area in the contacts list for Gravatars

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

I think the text is too close to the 'x' button (a full-height character would overlap) - I can see that causing a problem for L10n, so I think we need to adjust it a bit.

Otherwise, this is almost there apart from a comment, and adding tests. Not r+ at the moment, as it doesn't pass tests in itself, and I'd like to see it once the text/close button is adjusted.

::: browser/components/loop/MozLoopAPI.jsm
@@ +747,5 @@
> +
> +        // Do the MD5 dance.
> +        let hasher = Cc["@mozilla.org/security/hash;1"]
> +                       .createInstance(Ci.nsICryptoHash);
> +        hasher.init(Ci.nsICryptoHash.MD5);

Its a shame there isn't a content function available to do this, especially with pdfjs (and maybe shumway) seemingly having their own implementations...

::: browser/components/loop/content/js/contacts.jsx
@@ +106,5 @@
> +    },
> +
> +    render: function() {
> +      if (!this.state.showMe) {
> +        return (<span/>);

You should be able to just return null here. I believe we do it in other places - unless there's a specific reason not to do so, in which case, please add a comment.

Comment 37

[Mike de Boer [:mikedeboer]]

Attachment: Part 2.1: show a promo area in the contacts list for Gravatars

Comment 38

[Mike de Boer [:mikedeboer]]

Attachment: Part 3: tests

Comment 39

[Mark Banner (:standard8)]

Comment on attachment 8568498 [details] [diff] [review]
Part 2.1: show a promo area in the contacts list for Gravatars

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +757,5 @@
> +        // Convert the binary hash data to a hex string.
> +        let md5Email = [toHexString(hash.charCodeAt(i)) for (i in hash)].join("");
> +
> +        // Compose the Gravatar URL.
> +        return "http://www.gravatar.com/avatar/" + md5Email + ".jpg?default=blank&s=" + size;

A thought: should this be a (cached?) pref value for the url?

I would say we should make this https as well, but given the extra CSP work, lets make sure we follow-up with bug 1086556 straight after this one.

Comment 40

[Mark Banner (:standard8)]

Comment on attachment 8568499 [details] [diff] [review]
Part 3: tests

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

Looking good, a few comments to address though.

::: browser/components/loop/test/desktop-local/contacts_test.js
@@ +13,5 @@
>  
>    var fakeAddContactButtonText = "Fake Add Contact";
>    var fakeEditContactButtonText = "Fake Edit Contact";
>    var fakeDoneButtonText = "Fake Done";
> +  var fakeContacts = [{

Normally we initialise these variables in the beforeEach to ensure they don't get altered in one test and affect another. However, as you're creating a copy of it, I think it might just be worth a short comment explaining that's why this is different.

@@ +131,5 @@
>    afterEach(function() {
>      loop.shared.mixins.setRootObject(window);
>      sandbox.restore();
> +    delete navigator.mozLoop.getLoopPref;
> +    delete navigator.mozLoop.getUserAvatar;

Please change this to just delete navigator.mozLoop completely, which is what we should be doing here (well actually, we should be using fakeMozLoop etc, but that's for another bug).

@@ +136,5 @@
>    });
>  
> +  describe("GravatarsPromo", function() {
> +    afterEach(function() {
> +      listView = null;

As you've bumped listView up a level, you can bump the nulling up a level as well (same for the afterEach in the Contacts List section)

@@ +156,5 @@
> +        React.createElement(loop.contacts.ContactsList, {
> +          notifications: notifications
> +        }));
> +
> +      var promo =  listView.getDOMNode().querySelector(".contacts-gravatar-promo");

nit: unnecessary extra space.

@@ +191,5 @@
> +
> +      React.addons.TestUtils.Simulate.click(listView.getDOMNode().querySelector(
> +        ".contacts-gravatar-promo .button-accept"));
> +
> +      sinon.assert.calledTwice(navigator.mozLoop.setLoopPref);

Please move the pref check out to a separate test, and also check the values that it is called with.

Just reading through, its not immediately clear why it would be called twice, but explicitly testing the values will aid that understanding, and ensure that the correct thing is tested.

@@ +206,5 @@
> +
> +      React.addons.TestUtils.Simulate.click(listView.getDOMNode().querySelector(
> +        ".contacts-gravatar-promo .button-close"));
> +
> +      sinon.assert.calledOnce(navigator.mozLoop.setLoopPref);

Again, please separate out the pref check versus the promo check, and as before, do an additional check of the values that it is called with.

Comment 41

[Mike de Boer [:mikedeboer]]

Attachment: Part 2.2: show a promo area in the contacts list for Gravatars

Changed to https:// scheme URLs. Carrying over r=Standard8

Comment 42

[Mike de Boer [:mikedeboer]]

Attachment: Part 3.1: tests

Comment 43

[Mike de Boer [:mikedeboer]]

Comment on attachment 8568543 [details] [diff] [review]
Part 3.1: tests

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

::: browser/components/loop/test/desktop-local/contacts_test.js
@@ +13,5 @@
>  
>    var fakeAddContactButtonText = "Fake Add Contact";
>    var fakeEditContactButtonText = "Fake Edit Contact";
>    var fakeDoneButtonText = "Fake Done";
> +  var fakeContacts = [{

nit: I added the comment to explain the declaration of the contacts locally.

Comment 44

[Mike de Boer [:mikedeboer]]

Attachment: Part 3.2: tests

Unbitrotted version.

Comment 45

[Mark Banner (:standard8)]

Comment on attachment 8569861 [details] [diff] [review]
Part 3.2: tests

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

Looks good, one minor comment to make things a little cleaner. r=Standard8.

::: browser/components/loop/test/desktop-local/contacts_test.js
@@ +207,5 @@
> +      expect(call.args[0]).to.equal("contacts.gravatars.promo");
> +      expect(call.args[1]).to.equal(false);
> +      call = navigator.mozLoop.setLoopPref.secondCall;
> +      expect(call.args[0]).to.equal("contacts.gravatars.show");
> +      expect(call.args[1]).to.equal(true);

You can just use:

      sinon.assert.calledWithExactly(navigator.mozLoop.setLoopPref, "contacts.gravatars.promo", false);
      sinon.assert.calledWithExactly(navigator.mozLoop.setLoopPref, "contacts.gravatars.show", true);

for these - sinon iterates through the different calls - calledTwice checks it is called exactly the right amount, and then these check the values are appropriate

Comment 46

[Mike de Boer [:mikedeboer]]

Pushed with comment addressed to fx-team:

https://hg.mozilla.org/integration/fx-team/rev/aedc83c7caec
https://hg.mozilla.org/integration/fx-team/rev/3d7e0dce7b94

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aedc83c7caec
https://hg.mozilla.org/mozilla-central/rev/3d7e0dce7b94

Comment 49

[Mike de Boer [:mikedeboer]]

[Tracking Requested - why for this release]: I'd like to have this feature uplifted to Fx38, as it is planned for that release. The strings are already on aurora, so there's no particular risk with the two patches that need to be uplifted.

Comment 50

[Mike de Boer [:mikedeboer]]

Comment on attachment 8568542 [details] [diff] [review]
Part 2.2: show a promo area in the contacts list for Gravatars

Approval Request Comment
[Feature/regressing bug #]: Loop/ Hello product feature
[User impact if declined]: Originally, the design of the contacts tab inside the Loop panel contained room to show the avatar of a contact - by using the Gravatar service. Due to privacy concerns we pulled the avatars feature from the initial release. This bug re-adds the feature with an explicit opt-in interstitial permission request prompt.
[Describe test coverage new/current, TreeHerder]: Landed on m-c, tests pass.
[Risks and why]: minor
[String/UUID change made/needed]: n/a.

Comment 51

[Mike de Boer [:mikedeboer]]

Comment on attachment 8569861 [details] [diff] [review]
Part 3.2: tests

See comment 50 for approval request.

Comment 52

[Mike de Boer [:mikedeboer]]

Thanks! Pushed to mozilla-aurora as:

https://hg.mozilla.org/releases/mozilla-aurora/rev/26c502f841b3
https://hg.mozilla.org/releases/mozilla-aurora/rev/fa85f159d3fb

Comment 53

[Liz Henry (:lizzard) (relman/hg->git project)]

Forgot to mark this as tracking for 38.

Comment 54

[Bogdan Maris, Desktop QA]

Verified that the Gravatars appear when 'Use profile icons' is hit and other scenarios, also tested prefs 'loop.contacts.gravatars.show' and 'loop.contacts.gravatars.promo' and they work as expected. Testing was done on Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit using Firefox 38.0 RC build 2.

Will Gravatars be supported in conversation window as well (when a call is made)?

Comment 55

[Mike de Boer [:mikedeboer]]

(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #54)
> Will Gravatars be supported in conversation window as well (when a call is
> made)?

Thanks for verifying! Gravatars should/ will not be shown in the conversation window. Perhaps in a future release, but that depends on a re-design of that window.

Comment 56

[Ada [:adalucinet]]

Verified fixed using 39.0b1 build 2 (20150523155636), under the following platforms: Windows 7 64-bit, Mac OS X 10.9.5 and Ubuntu 14.04 32-bit.