Closed Bug 1069962 Opened 5 years ago Closed 5 years ago

Add support for Gravatar avatars in the Contacts List

Categories

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

defect
Points:
5

Tracking

(firefox38+ verified, firefox39 verified)

VERIFIED FIXED
mozilla39
Iteration:
38.3 - 23 Feb
Tracking Status
firefox38 + verified
firefox39 --- verified
Blocking Flags:
backlog backlog+

People

(Reporter: mikedeboer, Assigned: mikedeboer)

References

()

Details

(Whiteboard: [contacts][strings])

User Story

As a desktop client user, I want to see the avatars of my contacts who have a gravatar account so I can identify them better.

UX provided in https://bugzilla.mozilla.org/show_bug.cgi?id=1082950

Acceptance criteria:
* If a user has contacts in his contact list, a promotional area above the contact list gets displayed to prompt the user whether he wants to use profile avatars or not
* A link to the Mozilla privacy notice is displayed as part of the prompt
* The user has 3 options:
- Close the prompt. The message prompt will not be displayed to the user anymore and the user won't use contact list avatars.
- "No thanks" . The message prompt will not be displayed to the user anymore and the user won't use contact list avatars.
- "Use profile icons". The message prompt will not be displayed to the user anymore and the user will use contact list avatars, for his current contacts as well as future contacts.

Attachments

(3 files, 5 obsolete files)

See summary.

This bug needs a privacy review, eventually.

Already has a patch, carrying over r=paolo.
Attachment #8492201 - Flags: review+
Flags: qe-verify+
Flags: firefox-backlog+
Flags: needinfo?(mmucci)
Added to IT 35.2
Flags: needinfo?(mmucci)
https://hg.mozilla.org/mozilla-central/rev/b26c709330d6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Whiteboard: [contacts] [qa+] [loop-uplift] → [contacts][loop-uplift]
Could I please get clearer steps to reproduce for testing purposes?
Flags: needinfo?(mdeboer)
QA Contact: anthony.s.hughes
Whiteboard: [contacts][loop-uplift] → [contacts][loop-uplift][fig:wontverify]
Status: RESOLVED → REOPENED
Flags: needinfo?(mdeboer)
Resolution: FIXED → ---
Status: REOPENED → NEW
Blocks: 1075544
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.
Target Milestone: mozilla35 → mozilla36
Iteration: 35.2 → ---
Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/c840d5b6e55f
Target Milestone: mozilla36 → ---
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
Attachment #8492201 - Flags: approval-mozilla-aurora?
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
Attachment #8492201 - Flags: approval-mozilla-aurora?
I don't think this should be marked fixed since this bug is tracking adding support which was backed out for 34 and 35.
Whiteboard: [contacts][loop-uplift][fig:wontverify] → [contacts]
backlog: --- → Fx36?
find the dupe on this - need UX.  37 if we don't get UX in next 2 weeks.
Flags: needinfo?(sescalante)
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.
backlog: Fx36? → Fx36+
Depends on: 1082950
Flags: needinfo?(sescalante)
Priority: -- → P3
Whiteboard: [contacts] → [contacts][strings]
backlog: Fx36+ → Fx37?
Forgot to reset the priority when I marked this Fx37?
Priority: P3 → --
backlog: Fx37? → Fx37+
Priority: -- → P3
User Story: (updated)
User Story: (updated)
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.
backlog: Fx37+ → Fx38+
Once this lands, we'll want to inform Mika so our privacy notice can be updated.
(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?
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?)
Flags: needinfo?(rtestard)
Flags: needinfo?(mdeboer)
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.
Flags: needinfo?(rtestard)
backlog: Fx38+ → Fx37+
Priority: P3 → P2
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
First we'll need to implement the UX as specified in bug 1082950, before we can move forward with showing the gravatar profile pics.
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(mdeboer)
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?
See Also: → 1118262
because of strings it needs to push to 38
backlog: Fx37+ → Fx38+
Adjusted the point value to include the UI update in bug 1082950. I will work on it during this iteration.
Status: NEW → ASSIGNED
Iteration: --- → 38.1 - 26 Jan
Points: 1 → 5
Flags: needinfo?(mdeboer)
Duplicate of this bug: 1118262
Attachment #8492201 - Attachment is obsolete: true
Attachment #8551282 - Flags: review?(standard8)
Attachment #8551282 - Flags: review?(standard8) → review?(nperriault)
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.
Attachment #8551282 - Flags: review?(nperriault) → review?(standard8)
Iteration: 38.1 - 26 Jan → 38.3 - 23 Feb
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.
Attachment #8551282 - Flags: review?(standard8) → review+
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!
Attachment #8566254 - Flags: review?(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.
Pushed strings patch with localization note to fx-team: https://hg.mozilla.org/integration/fx-team/rev/3abfc3b9ab90
backlog: Fx38+ → backlog+
Rank: 21
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.
Attachment #8566254 - Flags: review?(standard8) → review-
Attachment #8566254 - Attachment is obsolete: true
Attachment #8568498 - Flags: review?(standard8)
Attached patch Part 3: tests (obsolete) — Splinter Review
Attachment #8568499 - Flags: review?(standard8)
Blocks: 1086556
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.
Attachment #8568498 - Flags: review?(standard8) → review+
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.
Attachment #8568499 - Flags: review?(standard8) → review-
Changed to https:// scheme URLs. Carrying over r=Standard8
Attachment #8568498 - Attachment is obsolete: true
Attachment #8568542 - Flags: review+
Attached patch Part 3.1: tests (obsolete) — Splinter Review
Attachment #8568499 - Attachment is obsolete: true
Attachment #8568543 - Flags: review?(standard8)
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.
Attached patch Part 3.2: testsSplinter Review
Unbitrotted version.
Attachment #8568543 - Attachment is obsolete: true
Attachment #8568543 - Flags: review?(standard8)
Attachment #8569861 - Flags: review?(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
Attachment #8569861 - Flags: review?(standard8) → review+
Keywords: leave-open
Duplicate of this bug: 1086556
https://hg.mozilla.org/mozilla-central/rev/aedc83c7caec
https://hg.mozilla.org/mozilla-central/rev/3d7e0dce7b94
Status: ASSIGNED → RESOLVED
Closed: 5 years ago5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
[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 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.
Attachment #8568542 - Flags: approval-mozilla-aurora?
Comment on attachment 8569861 [details] [diff] [review]
Part 3.2: tests

See comment 50 for approval request.
Attachment #8569861 - Flags: approval-mozilla-aurora?
Attachment #8568542 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8569861 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Forgot to mark this as tracking for 38.
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)?
Status: RESOLVED → VERIFIED
Flags: needinfo?(mdeboer)
(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.
Flags: needinfo?(mdeboer)
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.
Flags: qe-verify+ → qe-verify-
You need to log in before you can comment on or make changes to this bug.