1020449 - Desktop client should show caller information on incoming calls

Status: VERIFIED FIXED

(Hello (Loop) :: Client, enhancement, P1)

Description

[Adam Roach [:abr]]

When generating a new Loop URL, the user is asked to identify the URL; however, this information is not displayed to the user when the URL is activated.

Comment 1

[Mark Banner (:standard8)]

Yep, we've never got around to implementing this, but we also thought the UX may be changing. Seems like it isn't so we should just treat this as an enhancement.

Comment 2

[Maire Reavy [:mreavy]]

Mark -- Can you provide an estimate for this?

Comment 3

[Mark Banner (:standard8)]

I've started looking at this, but until the Loop server API discussions are complete (I'm contributing more to them later today), I can't estimate it, as the complexity will depend on what we decide on with the APIs.

Comment 4

[Mark Banner (:standard8)]

Investigation done, I've updated the user story with the parts that I think are needed.

Comment 5

[Mark Banner (:standard8)]

Removed window title part of user story as that is bug 1029433

Comment 6

[:shell escalante]

https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-incoming

Comment 7

[Andrei Oprea [mention me on [e-mail]]]

For the callerId field the server returns a random string that is generated here [0]. I'm not sure if the backend server needs it (that function was added in to make up for the fact that we removed "insert conversation name field" from loop panel some time ago). Right now it's not really useful do display callerId and I would default to the callUrl.

[0] https://github.com/mozilla/gecko-dev/blob/fx-team/browser/components/loop/content/js/panel.jsx#L317

Comment 8

[Mark Banner (:standard8)]

(In reply to Andrei Oprea [:andreio] from comment #7)
> For the callerId field the server returns a random string that is generated
> here [0]. I'm not sure if the backend server needs it (that function was
> added in to make up for the fact that we removed "insert conversation name
> field" from loop panel some time ago). Right now it's not really useful do
> display callerId and I would default to the callUrl.

Having looked into this a bit - we can replace the randomly generated text with the empty string, for now.

Then, when we get the bug that allows the user to name the url, we can do the relevant PUT xhr.

We might want to do things differently in accounted mode, but I'll check that's addressed in separate bugs.

Comment 9

[Andrei Oprea [:andreio]]

Attachment: Show caller information on incoming call

Comment 10

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

Comment on attachment 8490924 [details] [diff] [review]
Show caller information on incoming call

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

This heads in the right direction, with some concerns though:

- I'm not sure displaying the call URL as a fallback is a good idea, UX wise
- The avatar is not showing up (broken image link — see upcoming capture)

::: browser/components/loop/content/js/conversation.jsx
@@ +113,5 @@
>      },
>  
> +    incomingCallIdentifier: function() {
> +      return this.props.model.get("callerId") ||
> +                this.props.model.get("callUrl");

How about implementing this in ConversationModel#getCallIdentifier()?

@@ +177,5 @@
>      }
>    });
>  
>    /**
> +   *

Nit: Empty comment.

@@ +201,5 @@
> +
> +    _formatCreationDate: function() {
> +      var date = (new Date(this.props.urlCreationDate * 1000));
> +      var options = {year: "numeric", month: "long", day: "numeric"};
> +      var timestamp = date.toLocaleDateString(navigator.language, options);

mind_blown.gif; I learned something amazing today, thanks.

Nit: Timestamp should be renamed formattedDate.

@@ +202,5 @@
> +    _formatCreationDate: function() {
> +      var date = (new Date(this.props.urlCreationDate * 1000));
> +      var options = {year: "numeric", month: "long", day: "numeric"};
> +      var timestamp = date.toLocaleDateString(navigator.language, options);
> +      this.setState({timestamp: timestamp});

We don't really need to carry this date string in state; rather, you could just:

_getFormattedCreationDate: function() {
  if (!use this.props.urlCreationDate) {
    return;
  }
  // ... compute string out of this.props.urlCreationDate
  return formattedDateAsString;
},

render: function() {
  return <p>{this._getFormattedCreationDate()}</p>;
}

@@ +206,5 @@
> +      this.setState({timestamp: timestamp});
> +    },
> +
> +    componentDidMount: function() {
> +      this._formatCreationDate();

Unneeded.

@@ +215,5 @@
> +        "fx-embedded-tiny-video-icon": true,
> +        "muted": !this.props.video
> +      });
> +      return (
> +        /*jshint ignore:start*/

Please remove this and configure your editor/IDE to use jsxhint.

@@ +234,5 @@
> +              </span>
> +            </p>
> +          </div>
> +        </div>
> +        /*jshint ignore:end*/

Please remove this and configure your editor/IDE to use jsxhint.

::: browser/components/loop/content/js/panel.jsx
@@ +322,5 @@
>       * Fetches a call URL.
>       */
>      _fetchCallUrl: function() {
>        this.setState({pending: true});
> +      // XXX using empty string as conversation identifier

This needs a bug number.

@@ +323,5 @@
>       */
>      _fetchCallUrl: function() {
>        this.setState({pending: true});
> +      // XXX using empty string as conversation identifier
> +      this.props.client.requestCallUrl("", this._onCallUrlReceived);

I'm surprised this doesn't trigger a server error; reading server API docs[0], it says:

“400 Bad Request: You forgot to pass the callerId, or it’s not valid;”

[0]: http://docs.services.mozilla.com/loop/apis.html#post-call-url

::: browser/components/loop/content/shared/js/models.js
@@ +146,5 @@
>          progressURL:    sessionData.progressURL,
>          websocketToken: sessionData.websocketToken.toString(16),
>          callType:       sessionData.callType || "audio-video",
> +        callToken:      sessionData.callToken,
> +        callUrl:        sessionData.callUrl,

Attaching the callUrl to the model seems strange. More importantly, I can't see where that information is ever set. This is probably because this is a WiP patch.

@@ +147,5 @@
>          websocketToken: sessionData.websocketToken.toString(16),
>          callType:       sessionData.callType || "audio-video",
> +        callToken:      sessionData.callToken,
> +        callUrl:        sessionData.callUrl,
> +        urlCreationDate: sessionData.urlCreationDate,

Nit: Could you please keep alignment here?

@@ +148,5 @@
>          callType:       sessionData.callType || "audio-video",
> +        callToken:      sessionData.callToken,
> +        callUrl:        sessionData.callUrl,
> +        urlCreationDate: sessionData.urlCreationDate,
> +        callerId:       sessionData.callerId

Same remark as for the callurl attribute.

::: browser/components/loop/ui/ui-showcase.jsx
@@ +159,5 @@
>  
>              <Example summary="Default / incoming audio only call" dashed="true" style={{width: "280px"}}>
>                <div className="fx-embedded">
>                  <IncomingCallView model={mockConversationModel}
> +                                  video={false} />

We probably want several variations of the component.

Comment 11

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

Attachment: broken_avatar_and_url.png

Sample capture.

Comment 12

[Andrei Oprea [:andreio]]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #10)
> Comment on attachment 8490924 [details] [diff] [review]
> Show caller information on incoming call
> 
> Review of attachment 8490924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This heads in the right direction, with some concerns though:
> 
> - I'm not sure displaying the call URL as a fallback is a good idea, UX wise

https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-incoming-unknown

Comment 13

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

(In reply to Andrei Oprea [:andreio] from comment #12)
> https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#call-incoming-unknown

I know, maybe you could decrease the font size a little so we can display the full url? Another trick would be to add a title attribute so we'd get a tooltip with the full url (hmm, ys, I know).

Needinfo :darrin here.

Comment 14

[Darrin Henein [:darrin]]

Well it seems like the entire window is thinner than designed (I know this is across all windows, not just this bug), I believe I proposed 300px. I suspect that until we get the proper width, a lot of the text-based UI is going to fall apart at some point. 

Not displaying the URL is a valid point, though it remains the only identifier for this call. I still think showing it is better than asking a user to answer an entirely unknown call.

Comment 15

[Andrei Oprea [:andreio]]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #10)
> Comment on attachment 8490924 [details] [diff] [review]
> Show caller information on incoming call
> 
> Review of attachment 8490924 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 
> @@ +323,5 @@
> >       */
> >      _fetchCallUrl: function() {
> >        this.setState({pending: true});
> > +      // XXX using empty string as conversation identifier
> > +      this.props.client.requestCallUrl("", this._onCallUrlReceived);
> 
> I'm surprised this doesn't trigger a server error; reading server API
> docs[0], it says:
> 
> “400 Bad Request: You forgot to pass the callerId, or it’s not valid;”
> 
> [0]: http://docs.services.mozilla.com/loop/apis.html#post-call-url

https://bugzilla.mozilla.org/show_bug.cgi?id=1020449#c8

> 
> ::: browser/components/loop/content/shared/js/models.js
> @@ +146,5 @@
> >          progressURL:    sessionData.progressURL,
> >          websocketToken: sessionData.websocketToken.toString(16),
> >          callType:       sessionData.callType || "audio-video",
> > +        callToken:      sessionData.callToken,
> > +        callUrl:        sessionData.callUrl,
> 
> Attaching the callUrl to the model seems strange. More importantly, I can't
> see where that information is ever set. This is probably because this is a
> WiP patch.

It gets set in setIncomingSessionData. Doesn't feel so strange to me because it is received in the same object as all the other attributes of the model and set in the same method (i.e. not going through loops to do this)

Comment 16

[Andrei Oprea [:andreio]]

Attachment: Show caller information on incoming call

Comment 17

[Andrei Oprea [:andreio]]

You can already see 2 variations of the incoming call identifier in the ui showcase (with video and without) I don't think another variation is possible. Made the avatar a prop because paths between production and ui-showcase are different. Also I think it will be useful when we get actual avatars too.

Will add tests after feedback because things might change.

Comment 18

[Mark Banner (:standard8)]

Attachment: WIP Loop should show caller information on incoming calls.

I've taken Andrei's patch and unbitrotted it and updated for handling all the different caller id views we now have possible.

This now just needs a few extra tests and then should be good to land.

Comment 19

[Mark Banner (:standard8)]

Attachment: Loop should show caller information on incoming calls.

Updated patch to add unit tests and fix a couple of issues. Should now display the correct information when we have it:

- Incoming Call Url data (the url token, date and audio/video)
- Outgoing Direct Call (the person's name)
- Incoming Direct Call (the person's email address - once bug 1079950 is fixed)

They all display a default avatar.

Comment 20

[Mark Banner (:standard8)]

Attachment: Incoming & Outgoing call views

Here's the new views - note I've only added the Avatar, video/audio icons, "id" and date.

The Incoming call is similar, but without the date (and without the name until bug 1079950 is fixed).

Comment 21

[Darrin Henein [:darrin]]

Comment on attachment 8502496 [details]
Incoming & Outgoing call views

For an in-progress view, looks good. Cancel button on outgoing in the spec is full width. Mic/video icons + date need more spacing between, as per spec. Mic icon in incoming call left edge looks misaligned with above URL fragment. Accept button icon is too close (I believe this was pointed out in the 34-polish bugs). 

I'm also questioning our decision to show only the hash vs the whole hello URL. Seeing it here makes me think it will just confuse users. The spec had a window width of 320px, there was lots of room for the whole url in that case.

Comment 22

[Mark Banner (:standard8)]

Attachment: Incoming & Outgoing call views v2

The two on the left are the outgoing and incoming url views. The incoming view has a mocked-up url of what it will look like next week.

The view on the right is what it will look like currently.

All are based on the current size of the panel.

I will file a follow-up bug for adjusting the size of the panel, and I'll make sure we've got something on file for the buttons at the bottom of the windows.

Comment 23

[Mark Banner (:standard8)]

Attachment: Loop should show caller information on incoming calls. v2

Updated patch to fix Darrin's comments wrt the layout of the caller id section. Also added a test for the model changes, that I'd missed previously.

Comment 24

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

Comment on attachment 8502669 [details] [diff] [review]
Loop should show caller information on incoming calls. v2

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

Looks good! r=me with comments addressed, nits are left up to you.

::: browser/components/loop/content/shared/css/common.css
@@ +275,5 @@
>  /* Misc */
>  
> +.font-bold {
> +  font-weight: bold;
> +}

Nit: Do we really want to tie the styling to our HTML semantics?

::: browser/components/loop/content/shared/js/utils.js
@@ +17,5 @@
>      AUDIO_ONLY: "audio"
>    };
>  
>    /**
> +   * Format a given date into an l10n-friendly string

Nit: Missing @param and @return tags.

@@ +19,5 @@
>  
>    /**
> +   * Format a given date into an l10n-friendly string
> +   */
> +  function formatDate(timestamp) {

I think navigator.language should be passed as an arg to ease testing.

More importantly, I didn't find any unit test for this.

@@ +24,5 @@
> +    var date = (new Date(timestamp * 1000));
> +    var options = {year: "numeric", month: "long", day: "numeric"};
> +    var formattedDate = date.toLocaleDateString(navigator.language, options);
> +
> +    return formattedDate;

Nit: Directly return date.toLocaleDateString(navigator.language, options);

Comment 25

[Mark Banner (:standard8)]

Attachment: Loop should show caller information on incoming calls. v3

Updated patch to fix the comments. I'm going to land this on fx-team, as discussion with Darrin over irc last night looked like it was promising for ui-review+. Minor tweaks to the layout we can deal with in further patches or a new bug.

Comment 26

[Mark Banner (:standard8)]

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

Comment 27

[Mark Banner (:standard8)]

This should get verification.

This bug implements caller id for:

- Incoming calls from link-clickers (shows the url, and the date the url was generated)
- Outgoing direct calls (shows the name of the contact)
- Incoming direct calls (will show the name of the contact *once* bug 1079950 is deployed to the servers)

This also implements a default avatar (no user-specific avatar), and also microphone and camera icons to indicate the incoming call type.

Comment 28

[Mark Banner (:standard8)]

Unfortunately I messed up the review comment changes and missed a quote, so I just landed an additional fix:

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

I've filed bug 1081058 where we can investigate ways of preventing this happening so often.

Comment 29

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

https://hg.mozilla.org/mozilla-central/rev/6c5372da2928
https://hg.mozilla.org/mozilla-central/rev/2a80e7609574

Comment 30

[Mark Banner (:standard8)]

Comment on attachment 8503048 [details] [diff] [review]
Loop should show caller information on incoming calls. v3

Approval Request Comment
[Feature/regressing bug #]: Part of the Loop work, implements proper caller ids for incoming and outgoing calls
[User impact if declined]: The user won't know who is calling them.
[Describe test coverage new/current, TBPL]: Landed on nightly, with unit test coverage 
[Risks and why]: Low, mainly layout and css work in content space. Only affects the views that display the information.
[String/UUID change made/needed]: None

Comment 31

[Darrin Henein [:darrin]]

Comment on attachment 8502648 [details]
Incoming & Outgoing call views v2

Looks good to me, expecting the 300px width bug to resolve most URL overflow issues.

Comment 32

[Mark Banner (:standard8)]

(In reply to Mark Banner (:standard8) from comment #27)
> This should get verification.
...
> - Incoming direct calls (will show the name of the contact *once* bug
> 1079950 is deployed to the servers)

To test with bug 1079950 on the development server:

- Set up two new profiles (or re-use existing ones), set loop.server preference to "https://loop-dev.stage.mozaws.net".
- After setting the preference, restart each FF instance.
- Log in to each one with different FxA accounts
- Create a contact on one of them with the email address of the other FxA account
- Start a video call to the contact.

On the incoming call on the callee's end, you'll see the email address of the contact.

Comment 33

[Maire Reavy [:mreavy]]

Hi Pauly, There is currently a bug in the production loop server; so as Mark indicates in Comment 32, you'll need to use the dev server (https://loop-dev.stage.mozaws.net).   We'd like to get this into Beta2 (or as early in Beta as possible).  As you can see the Beta approval flag is set.   

Can you test this with Bug 1029433?  (Both cover caller ID for the client.)

Comment 34

[Paul Silaghi, QA [:pauly]]

Email/URL for registered users/guests displayed in the layout.
Verified fixed FF 35.0a2 (2014-10-15) Win 7, OS X 10.9.5

Comment 35

[Paul Silaghi, QA [:pauly]]

FWIW, some letters are cropped: http://i.imgur.com/79uOZWP.png

Comment 36

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

Comment on attachment 8503048 [details] [diff] [review]
Loop should show caller information on incoming calls. v3

This is an mvp feature for Loop. The feature has already been verified in 35 and is contained to Loop. As we're very late for feature work, I want to see this in beta2. Beta+

Comment 37

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

https://hg.mozilla.org/releases/mozilla-beta/rev/742beda04394
https://hg.mozilla.org/releases/mozilla-beta/rev/0033bca3ce22

Comment 38

[u279076]

I'm deprioritizing this for further QE testing as this has already been verified in Nightly. We've got more important bugs that need testing right now but may loop back around to this if time allows.

Comment 39

[Mark Banner (:standard8)]

Note to testers: for a callee to receive the caller id, the caller must have last signed into FxA after the deployment time of 0.12.5 (Mon 27 Oct).

Comment 40

[Mark Banner (:standard8)]

Clearing in-testsuite requests for features that are being removed from Hello as part of te user journey work in bug 1209713.