1048162 - Desktop client needs "email link" button on the "call failed" visual notification for outgoing calls to contacts

Status: VERIFIED FIXED

(Hello (Loop) :: Client, enhancement)

Comment 1

[Romain Testard [:RT]]

This looks like a duplicate of bug 1000769?

Comment 2

[Paul Silaghi, QA [:pauly]]

I'm not sure.
I filed this and bug 1048161 as a response to https://bugzilla.mozilla.org/show_bug.cgi?id=1000186#c7.
Bug 1000769 says in the summary "a contact not reachable via FxA". Does it cover the case when the call fails ?

Comment 3

[Paul Silaghi, QA [:pauly]]

(In reply to Paul Silaghi, QA [:pauly] from comment #2)
> Does it cover the case when the call fails ?
due to other reasons?

Comment 4

[Romain Testard [:RT]]

Yes you're right - it does not.
I mark this bug as blocking bug 1000769 as it is a dependency rather than a duplicate.

Comment 5

[Mark Banner (:standard8)]

Darrin, should the email link button appear for incoming (e.g. from anonymous link-clicker) as well as outgoing calls?

Comment 6

[Darrin Henein [:darrin]]

My immediate reaction is no... the email link was meant as a quick way to reach a contact if we already knew who they were but they weren't reachable by direct call. If an incoming call from anonymous fails, email link is no better than just using the panel to generate a new one (or wait for them to try again).

Comment 8

[Mark Banner (:standard8)]

Attachment: WIP Experimental patch for email button

This is my experimental patch for the email button, notes in a few minutes.

Comment 9

[Mark Banner (:standard8)]

In working on the initial experiment, there were a couple of UX questions came up:

1) How should we indicate when the user clicks on the button that we're doing something? We're getting a URL from the server, so there's going to be a little bit of time here.

2) If something goes wrong with getting the url (e.g. disconnected), how do we indicate it? Is there a way to handle this without changing strings, or should we just fail quietly in 34/35.

Darrin - can you help us out with those?


For the implementation itself:

- I was strongly thinking that we either need to trigger an action, or wrap the getting of the call url & opening email in a utility function
- Depending on the answer to 1) above, we might want to make this a small store, that we can then update the state of the button once we have got the URL.
- navigator.mozLoop.composeEmail needs an extension for email address
- the code will need to get the preferred email address from the contact, I don't think we have a utility function for that yet.

Unassigning, as I'm not actively working on this this week.

Comment 10

[Maire Reavy [:mreavy]]

I will be looking for someone to take this over for this cycle.

Comment 11

[Darrin Henein [:darrin]]

(In reply to Mark Banner (:standard8) from comment #9)
> In working on the initial experiment, there were a couple of UX questions
> came up:
> 
> 1) How should we indicate when the user clicks on the button that we're
> doing something? We're getting a URL from the server, so there's going to be
> a little bit of time here.

Hmm. Without adding strings or designing a new interaction, we could just disable/fade out the button (It should be a short enough time before the email opening or an error, right?)

> 2) If something goes wrong with getting the url (e.g. disconnected), how do
> we indicate it? Is there a way to handle this without changing strings, or
> should we just fail quietly in 34/35.

We should reuse the error notification in the panel (so change the icon to error and display our disconnected/etc. error there) and close the conversation window. Again, this is really our only option without adding strings or new UI.

Comment 12

[Mark Banner (:standard8)]

Yes, that seems reasonable - I've updated the user story.

Comment 13

[Marco Mucci [:MarcoM]]

Discussed with Maire, someone from the Loop team will be taking this bug.  Will be tracked in their IT 36.1

Comment 14

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Part 1 - Add an 'Email Link' button to Loop desktop failed call view.

This is the first part of the patch, I think it's landable as is and can be reviewed. Part 2 will address the error notification for the case where retrieving a new call URL fails (needs UX).

I'm assigning review to :Standard8 but if he's too busy, it's probably worth delegating to :dmose or :mikedeboer as I think this is urgent.

Comment 15

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #14)
> Part 2 will address the error notification for the case where
> retrieving a new call URL fails (needs UX).

It actually doesn't need UX if I read comment #11 correctly.

Comment 16

[Mark Banner (:standard8)]

Comment on attachment 8505549 [details] [diff] [review]
Part 1 - Add an 'Email Link' button to Loop desktop failed call view.

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

There's one part missing for the functional UX side - the conversation window should close once the email link has been generated and passed to mailto.

Also as discussed on irc, it feels like composeCallUrlEmail needs a test.

::: browser/components/loop/content/js/conversationViews.jsx
@@ +217,5 @@
> +      var emailLink = this.props.store.get("emailLink");
> +      var contactEmail = _getPreferredEmail(this.props.contact).value;
> +      sharedUtils.composeCallUrlEmail(emailLink, contactEmail);
> +      this.setState({
> +        emailLink: emailLink,

Do we really need to store the emailLink here?

::: browser/components/loop/content/shared/js/conversationStore.js
@@ +309,5 @@
> +     * can't be reached.
> +     */
> +    fetchEmailLink: function() {
> +      var callId = this.get("callId") || "";
> +      this.client.requestCallUrl(callId, function(err, callUrlData) {

The first parameter here should just be "" - we don't support call nicknames yet.

Comment 17

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Part 1 - Add an 'Email Link' button to Loop desktop failed call view.

Addressed comments.

Comment 18

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Part 1 - Add an 'Email Link' button to Loop desktop failed call view.

Added missing bits.

Comment 19

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Part 1 - Add an 'Email Link' button to Loop desktop failed call view.

Now closing the window after the email has been "composed".

Comment 20

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: [checked in] Part 1 - Add an 'Email Link' button to Loop desktop failed call view.

As per discussion on IRC, amending the patch with small CSS tweaks to properly render the buttons in the call failed view as well as in other areas; this doesn't visually break any component in the UI showcase. Will attach a screenshot.

Comment 21

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Screeshot — Part 1

Comment 22

[Mark Banner (:standard8)]

Comment on attachment 8506051 [details] [diff] [review]
[checked in] Part 1 - Add an 'Email Link' button to Loop desktop failed call view.

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

Looks good r=Standard8

Comment 23

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Part 2 - Display an error message if fetching an email link fails.

This second part suggests displaying the error message in case fetching an email link fails contextually, next to the place the initial action was triggered (the Email Link button in the conversation window).

:darrin would this be okay for you? I'll attach a screenshot next.

Comment 24

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

Attachment: Screenshot - Part 2 (contextual error message)

Comment 25

[Darrin Henein [:darrin]]

Comment on attachment 8506110 [details]
Screenshot - Part 2 (contextual error message)

This looks great, I actually like this better than using the panel error UI for errors like this. Will r? Matej for the string your using.

Comment 26

[Maire Reavy [:mreavy]]

Hi Darrin & Matej -- Can I get a ui-review from you both ASAP?  I'm looking to uplift this as soon as possible.

NOTE: Because we're uplifting, we can't change the string.  If you want to change the string, we can file a bug for Fx36 and that can ride the Fx36 train.  Thanks!

Comment 27

[Darrin Henein [:darrin]]

Comment on attachment 8506053 [details]
Screeshot — Part 1

This looks fine to me :)

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 8506051 [details] [diff] [review]
[checked in] Part 1 - Add an 'Email Link' button to Loop desktop failed call view.

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

Comment 29

[Mark Banner (:standard8)]

Comment on attachment 8506109 [details] [diff] [review]
Part 2 - Display an error message if fetching an email link fails.

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

Looks good, r=Standard8

Comment 30

[Mark Banner (:standard8)]

Checkin needed for part 2, try run:

https://tbpl.mozilla.org/?tree=Try&rev=e299999764e6
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e299999764e6

Comment 31

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

https://hg.mozilla.org/integration/fx-team/rev/6e7bdaa5a71f
(with no string changes)

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/80f3f56a6091
https://hg.mozilla.org/mozilla-central/rev/6e7bdaa5a71f

Comment 33

[Maire Reavy [:mreavy]]

Comment on attachment 8506051 [details] [diff] [review]
[checked in] Part 1 - Add an 'Email Link' button to Loop desktop failed call view.

Approval Request Comment
[Feature/regressing bug #]: Direct calling in Hello
[User impact if declined]: If the user is not available (or doesn't have an FxA account), there's no action available to contact him
[Describe test coverage new/current, TBPL]: landed on m-c, unit tests
[Risks and why]: Low risk outside of Hello.  Without the email button, the user has no way within the feature to contact the user if they don't have an FxA account or aren't logged in
[String/UUID change made/needed]: No Strings

Comment 34

[Maire Reavy [:mreavy]]

Comment on attachment 8506109 [details] [diff] [review]
Part 2 - Display an error message if fetching an email link fails.

Approval Request Comment
[Feature/regressing bug #]: Direct calling in Hello
[User impact if declined]: Handles errors if we are unable to fetch an email link
[Describe test coverage new/current, TBPL]: landed on m-c, unit tests
[Risks and why]: Low risk outside of Hello.  Errors would not be handled if we failed to get a link from the server, which would confuse the user.
[String/UUID change made/needed]: No Strings

Comment 35

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:
Requesting uplift.  See Comments 33 and 34.

Comment 36

[u279076]

Paul, can you please make sure this gets tested on Nightly?

Comment 37

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

Tested and works on Nightly 10/18 windows and linux.

Note I filed a separate bug about email buttons (which pre-existed this patch and likely has been there from the beginning)

Comment 38

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

Note: the email issue I hit was an unrelated issue caused by running the browser with --no-remote (as I normally do for all tests); see the bug link in see-also; it's a general browser issue that's existed for at least a decade, maybe longer.

Comment 39

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8506051 [details] [diff] [review]
[checked in] Part 1 - Add an 'Email Link' button to Loop desktop failed call view.

Thanks for testing jesup. Approved for Aurora. If everything goes well with your testing on Aurora on Sunday, we'll get this uplifted for beta2.

Comment 40

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/afeeb55003ea
https://hg.mozilla.org/releases/mozilla-aurora/rev/4f00fe5f6247

Comment 41

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

Tested and good on Aurora nightly build; windows and linux.

Comment 42

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

https://hg.mozilla.org/releases/mozilla-beta/rev/f705ffd06218
https://hg.mozilla.org/releases/mozilla-beta/rev/3fc523fcc7da

Comment 43

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8506051 [details] [diff] [review]
[checked in] Part 1 - Add an 'Email Link' button to Loop desktop failed call view.

Previously approved offline. Adding approval to the bug.

Comment 44

[Matej Novak [:matej]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #26)
> Hi Darrin & Matej -- Can I get a ui-review from you both ASAP?  I'm looking
> to uplift this as soon as possible.

Sorry, I was on PTO late last week. Is there anything you still need me to review?

Comment 45

[Maire Reavy [:mreavy]]

Hi Matej -- If you would like to change the string shown in "screenshot - part 2" for Fx36 or later, then please file a new bug.  Otherwise, there's nothing that I need.  Thanks!

Comment 46

[Matej Novak [:matej]]

(In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #45)
> Hi Matej -- If you would like to change the string shown in "screenshot -
> part 2" for Fx36 or later, then please file a new bug.  Otherwise, there's
> nothing that I need.  Thanks!

URL should be capitalized. Is that something we can still change?

Comment 47

[Paul Silaghi, QA [:pauly]]

Verified fixed 36.0a1 (2014-10-20) on Win 7, Ubuntu 14.04, OS X 10.9.5.

Comment 48

[u279076]

Paul, can you please verify this in the latest Beta build?

Comment 49

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 34b2 Ubuntu 14.04 x64

Comment 50

[Mark Banner (:standard8)]

(In reply to Matej Novak [:matej] from comment #46)
> (In reply to Maire Reavy [:mreavy] (Plz needinfo me) from comment #45)
> > Hi Matej -- If you would like to change the string shown in "screenshot -
> > part 2" for Fx36 or later, then please file a new bug.  Otherwise, there's
> > nothing that I need.  Thanks!
> 
> URL should be capitalized. Is that something we can still change?

As I understand it, yes, quoting from the best practices doc:

"If your changes are relevant only for English — for example, to correct a typographical error or to make letter case consistent — then there is generally no need to update the entity name."

So we can do it, but we should do so in a different bug.

Comment 51

[Mark Banner (:standard8)]

Comment on attachment 8506110 [details]
Screenshot - Part 2 (contextual error message)

I've split out the string change to bug 1096223.