Closed Bug 1020449 Opened 6 years ago Closed 5 years ago

Desktop client should show caller information on incoming calls

Categories

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

enhancement
Points:
5

Tracking

(firefox33 unaffected, firefox34+ fixed, firefox35 verified)

VERIFIED FIXED
mozilla35
Iteration:
35.3
Tracking Status
firefox33 --- unaffected
firefox34 + fixed
firefox35 --- verified

People

(Reporter: abr, Assigned: standard8)

References

Details

(Whiteboard: [loop-uplift][loop-inccall1])

User Story

As a callee, I should see either the URL or my specified identifier when I receive an incoming call from a link clicker.

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

Rough outline:

- Extend conversation model to save / record the callerId, callUrl and urlCreationDate from the GET /calls request
- Update view to display the callerId or the link url (callerId preferred)
- Update view to display the date of link generation

Param names from: https://docs.services.mozilla.com/loop/apis.html#get-calls-version-version

Attachments

(2 files, 7 obsolete files)

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.
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.
Severity: normal → enhancement
Mark -- Can you provide an estimate for this?
Flags: needinfo?(standard8)
Priority: -- → P2
Target Milestone: --- → mozilla33
Priority: P2 → P1
Whiteboard: [investigate, p=1]
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.
User Story: (updated)
Whiteboard: [investigate, p=1] → [investigate, p=1][need api discussions to be completed]
Blocks: 1029433
Investigation done, I've updated the user story with the parts that I think are needed.
User Story: (updated)
Flags: needinfo?(standard8)
Whiteboard: [investigate, p=1][need api discussions to be completed] → [p=?]
Removed window title part of user story as that is bug 1029433
User Story: (updated)
Depends on: 1025881
Whiteboard: [p=?] → [p=2]
User Story: (updated)
Blocks: 1030253
Target Milestone: mozilla33 → mozilla34
User Story: (updated)
Whiteboard: [p=2] → [p=2][loop-uplift]
Target Milestone: mozilla34 → mozilla35
Points: --- → 5
Whiteboard: [p=2][loop-uplift] → [loop-uplift]
User Story: (updated)
Summary: No indication of calling party ID → Desktop client should show caller information on incoming calls
Assignee: nobody → aoprea
User Story: (updated)
Summary: Desktop client should show caller information on incoming calls → No indication of calling party ID
User Story: (updated)
Summary: No indication of calling party ID → Desktop client should show caller information on incoming calls
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
Whiteboard: [loop-uplift] → [loop-uplift][loop-inccall1]
Assignee: aoprea → andrei.br92
(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.
Attachment #8490924 - Flags: feedback?(nperriault)
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.
Attachment #8490924 - Flags: feedback?(nperriault) → feedback+
(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
(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.
Flags: needinfo?(dhenein)
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.
Flags: needinfo?(dhenein)
(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)
Depends on: 1070045
Attachment #8490924 - Attachment is obsolete: true
Attachment #8492299 - Flags: feedback?(nperriault)
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.
Depends on: 1079950
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.
Attachment #8492299 - Attachment is obsolete: true
Attachment #8492299 - Flags: feedback?(nperriault)
Assignee: andrei.br92 → standard8
Blocks: 1066017
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.
Attachment #8502452 - Attachment is obsolete: true
Attachment #8502493 - Flags: review?(nperriault)
Attached image Incoming & Outgoing call views (obsolete) —
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).
Attachment #8491415 - Attachment is obsolete: true
Attachment #8502496 - Flags: ui-review?(dhenein)
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.
Attachment #8502496 - Flags: ui-review?(dhenein) → ui-review-
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.
Attachment #8502648 - Flags: ui-review?(dhenein)
Attachment #8502496 - Attachment is obsolete: true
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.
Attachment #8502493 - Attachment is obsolete: true
Attachment #8502493 - Flags: review?(nperriault)
Attachment #8502669 - Flags: review?(nperriault)
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);
Attachment #8502669 - Flags: review?(nperriault) → review+
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.
Attachment #8502669 - Attachment is obsolete: true
Attachment #8503048 - Flags: review+
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.
Flags: qe-verify+
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 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
Attachment #8503048 - Flags: approval-mozilla-beta?
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.
Attachment #8502648 - Flags: ui-review?(dhenein) → ui-review+
Blocks: 1047438
(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.
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.)
Flags: needinfo?(paul.silaghi)
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(paul.silaghi)
FWIW, some letters are cropped: http://i.imgur.com/79uOZWP.png
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+
Attachment #8503048 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Flags: qe-verify-
Flags: qe-verify+
Flags: in-testsuite?
Depends on: 1088006
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).
Iteration: --- → 35.3
Flags: in-moztrap+
Clearing in-testsuite requests for features that are being removed from Hello as part of te user journey work in bug 1209713.
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.