Closed Bug 1048162 Opened 5 years ago Closed 5 years ago

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

Categories

(Hello (Loop) :: Client, enhancement)

enhancement
Not set
Points:
5

Tracking

(firefox33 wontfix, firefox34+ verified, firefox35+ verified, firefox36 verified)

VERIFIED FIXED
mozilla36
Iteration:
36.1
Tracking Status
firefox33 --- wontfix
firefox34 + verified
firefox35 + verified
firefox36 --- verified
Blocking Flags:
backlog Fx34+

People

(Reporter: pauly, Unassigned)

References

Details

(Whiteboard: [loop-uplift][fxa][contacts][loop-outcall1])

User Story

https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#link-prompt

As a signed-in FF browser user I can call a contact not reachable via FxA so that I get prompted to share a call-back URL with him.

UX:

- Click Email link button
- Email (or all) button shows as disabled whilst the link is obtained
- Email "window" is opened
- Conversation window is closed.

In case of error:

- Close the conversation window
- Notify via the panel

Attachments

(4 files, 5 obsolete files)

35.99 KB, patch
standard8
: review+
Details | Diff | Splinter Review
78.83 KB, image/png
dhenein
: ui-review+
Details
14.97 KB, patch
standard8
: review+
Details | Diff | Splinter Review
24.46 KB, image/png
dhenein
: ui-review+
Details
No description provided.
This looks like a duplicate of bug 1000769?
Flags: needinfo?(paul.silaghi)
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 ?
Flags: needinfo?(paul.silaghi) → needinfo?(rtestard)
(In reply to Paul Silaghi, QA [:pauly] from comment #2)
> Does it cover the case when the call fails ?
due to other reasons?
Yes you're right - it does not.
I mark this bug as blocking bug 1000769 as it is a dependency rather than a duplicate.
Blocks: 1000769
Flags: needinfo?(rtestard)
Darrin, should the email link button appear for incoming (e.g. from anonymous link-clicker) as well as outgoing calls?
Severity: normal → enhancement
Depends on: 1047410
Flags: needinfo?(dhenein)
OS: Windows 7 → All
Hardware: x86_64 → All
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).
Flags: needinfo?(dhenein)
Blocks: 1066017
No longer blocks: 1000186, 1000769
Points: --- → 5
User Story: (updated)
Whiteboard: [loop-uplift][fxa][contacts]
Duplicate of this bug: 1000769
Whiteboard: [loop-uplift][fxa][contacts] → [loop-uplift][fxa][contacts][loop-outcall1]
Target Milestone: --- → mozilla35
Summary: Desktop client needs "email link" button on the "call failed" visual notification → Desktop client needs "email link" button on the "call failed" visual notification for outgoing calls to contacts
Assignee: nobody → standard8
Flags: qe-verify?
Flags: firefox-backlog+
backlog: --- → Fx35+
This is my experimental patch for the email button, notes in a few minutes.
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.
Assignee: standard8 → nobody
Iteration: --- → 36.1
Flags: needinfo?(dhenein)
Target Milestone: mozilla35 → mozilla36
Iteration: 36.1 → ---
I will be looking for someone to take this over for this cycle.
Iteration: --- → 36.1
(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.
Flags: needinfo?(dhenein)
Yes, that seems reasonable - I've updated the user story.
User Story: (updated)
Discussed with Maire, someone from the Loop team will be taking this bug.  Will be tracked in their IT 36.1
Flags: firefox-backlog+
Assignee: nobody → nperriault
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.
Attachment #8504899 - Attachment is obsolete: true
Attachment #8505549 - Flags: review?(standard8)
(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.
backlog: Fx35+ → Fx34+
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.
Attachment #8505549 - Flags: review?(standard8) → review-
Addressed comments.
Attachment #8505549 - Attachment is obsolete: true
Attachment #8506023 - Flags: review?(standard8)
Added missing bits.
Attachment #8506023 - Attachment is obsolete: true
Attachment #8506023 - Flags: review?(standard8)
Attachment #8506028 - Flags: review?(standard8)
Now closing the window after the email has been "composed".
Attachment #8506028 - Attachment is obsolete: true
Attachment #8506028 - Flags: review?(standard8)
Attachment #8506032 - Flags: review?(standard8)
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.
Attachment #8506032 - Attachment is obsolete: true
Attachment #8506032 - Flags: review?(standard8)
Attachment #8506051 - Flags: review?(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
Attachment #8506051 - Flags: review?(standard8) → review+
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.
Attachment #8506109 - Flags: review?(standard8)
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.
Attachment #8506110 - Flags: ui-review?(dhenein)
Attachment #8506110 - Flags: ui-review?(Mnovak)
Attachment #8506110 - Flags: ui-review+
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!
Flags: needinfo?(dhenein)
Flags: needinfo?(Mnovak)
Comment on attachment 8506053 [details]
Screeshot — Part 1

This looks fine to me :)
Flags: needinfo?(dhenein)
Attachment #8506053 - Flags: ui-review?(dhenein) → ui-review+
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
Attachment #8506051 - Attachment description: Part 1 - Add an 'Email Link' button to Loop desktop failed call view. → [checked in] Part 1 - Add an 'Email Link' button to Loop desktop failed call view.
Keywords: leave-open
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
Attachment #8506109 - Flags: review?(standard8) → review+
Status: NEW → RESOLVED
Closed: 5 years ago
Keywords: leave-open
Resolution: --- → FIXED
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
Attachment #8506051 - Flags: approval-mozilla-beta?
Attachment #8506051 - Flags: approval-mozilla-aurora?
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
Attachment #8506109 - Flags: approval-mozilla-beta?
Attachment #8506109 - Flags: approval-mozilla-aurora?
[Tracking Requested - why for this release]:
Requesting uplift.  See Comments 33 and 34.
Paul, can you please make sure this gets tested on Nightly?
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(paul.silaghi)
QA Contact: paul.silaghi
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)
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 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.
Attachment #8506051 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8506109 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tested and good on Aurora nightly build; windows and linux.
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.
Attachment #8506051 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8506109 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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?
Flags: needinfo?(Mnovak)
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!
(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?
Verified fixed 36.0a1 (2014-10-20) on Win 7, Ubuntu 14.04, OS X 10.9.5.
Status: RESOLVED → VERIFIED
Flags: needinfo?(paul.silaghi)
Paul, can you please verify this in the latest Beta build?
Flags: needinfo?(paul.silaghi)
Verified fixed FF 34b2 Ubuntu 14.04 x64
Flags: needinfo?(paul.silaghi)
(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 on attachment 8506110 [details]
Screenshot - Part 2 (contextual error message)

I've split out the string change to bug 1096223.
Attachment #8506110 - Flags: ui-review?(Mnovak)
You need to log in before you can comment on or make changes to this bug.