1065155 - When a call is received, check both Hawk sessions for pending calls, and keep track of session type to handle deleting call urls

Status: RESOLVED FIXED

(Hello (Loop) :: Client, enhancement)

Description

[Matthew N. [:MattN]]

(Quoting Adam Roach [:abr] from bug 1047667 comment #9)
> …
> This means that the Loop client, when it gets a push notification, will need
> to check both sessions (the browser one and the user one) to see which calls
> are pending.

Mark, does this make sense? Does it need to be broken down more?

Comment 1

[Matthew N. [:MattN]]

Perhaps this is a dupe of bug 1000761?

Comment 2

[Paul Kerr [:pkerr]]

When bug 1032700 lands, only one call will be passed on to the loop client with all other call notifications received from the push server explicitly rejected as busy. Given this behavior, how does this bug work when there is only ever on inbound pending call?

Comment 3

[Mark Banner (:standard8)]

The hardest part here is how to keep track of if a call URL is for a logged in user or not.

I guess, what we could do is:

- Extend MozLoopService#onHandleNotification to request /calls information for both anonymous session and FxA session (if appropriate).
- When saving the call data, tag the data with the sessionType
- This will then get passed to the conversation window
- The conversation window can then pass the information back to the DELETE request.

This would serve the current situation. If more come up later, we'd probably need to persist, but we can think about that scenario when it occurs.


Re bug 1000761, I think that might be a dupe but lets use this as the implementation one, and if it is, then I'll tidy it tomorrow.

Comment 4

[Mark Banner (:standard8)]

(In reply to Paul Kerr [:pkerr] from comment #2)
> When bug 1032700 lands, only one call will be passed on to the loop client
> with all other call notifications received from the push server explicitly
> rejected as busy. Given this behavior, how does this bug work when there is
> only ever on inbound pending call?

I'm not quite sure I understand what you mean, however, this bug should:

- check both session types when receiving a push notification
- for each call received (regardless of session type), if:
-- the conversation window is already active/open => reject the call
-- if the conversation window is not active/open => accept the call and open the window

Comment 6

[u279076]

Untracking this for QE verification. Please needinfo me to request manual testing.

Comment 7

[Paul Kerr [:pkerr]]

Attachment: Check both Hawk sessions when PushServer notification received. Record session type with call data

Comment 8

[Paul Kerr [:pkerr]]

Comment on attachment 8490416 [details] [diff] [review]
Check both Hawk sessions when PushServer notification received. Record session type with call data

Does this give you the call handling and hawk session type capture that you wanted?

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8490416 [details] [diff] [review]
Check both Hawk sessions when PushServer notification received. Record session type with call data

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

I just took a quick look, and I think this is doing the right thing. Adding the sessionType to the model will be useful iirc, as we may need it for the block functionality. In any case, its there if we do need it, aand it doesn't complicate things to have it there.

Comment 10

[Paul Kerr [:pkerr]]

Attachment: Check both Hawk sessions when PushServer notification received. Record session type with call data

clean up of function headers

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

f+=dmose.  I would suggest filing a separate bug to refactor this code before too long.  Right now the way the promises are chained and caught makes it pretty complicated to reason about what's going on, as it requires maintaining a fair bit of mental context.

Comment 12

[Mark Banner (:standard8)]

Comment on attachment 8490843 [details] [diff] [review]
Check both Hawk sessions when PushServer notification received. Record session type with call data

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

This looks good, as you know from irc, it works! Obviously tests would be good, but as long as the existing ones pass, I think we could land this first (once bug 1032700 lands, of course) and add tests after.

::: browser/components/loop/MozLoopService.jsm
@@ +541,5 @@
> +    this.hawkRequest(LOOP_SESSION_TYPE.FXA,
> +      "/calls?version=" + version, "GET").then(
> +        response => {
> +          this._processCalls(response, LOOP_SESSION_TYPE.FXA);
> +          this.hawkRequest(LOOP_SESSION_TYPE.GUEST,

To avoid the duplication here, you could probably put these in a function.

Comment 13

[Paul Kerr [:pkerr]]

Attachment: Check both Hawk sessions when PushServer notification received. Record session type with call data

Consolidated GET/calls hawkRequest code in a function.

Comment 14

[Paul Kerr [:pkerr]]

Carrying forward standard8 r+

Comment 15

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/289416a752ff

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/289416a752ff

Comment 17

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

Comment on attachment 8491775 [details] [diff] [review]
Check both Hawk sessions when PushServer notification received. Record session type with call data

Approval Request Comment
Uplift request for patches staged and tested on Fig

Comment 18

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/e3eb0b80e217

Comment 19

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

Comment on attachment 8491775 [details] [diff] [review]
Check both Hawk sessions when PushServer notification received. Record session type with call data

I worked with Randell and Maire on uplifting a large number of Loop bugs at once. All of the bugs have been staged on Fig and tested by QE before uplift to Aurora. As well, all of the bugs are isolated to the Loop client. Randell handled the uplift with my approval. I am adding approval to the bug after the fact for bookkeeping.

Comment 20

[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.