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

RESOLVED FIXED in Firefox 34

Status

enhancement
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: MattN, Assigned: pkerr)

Tracking

unspecified
mozilla35
Points:
5
Dependency tree / graph
Bug Flags:
firefox-backlog +
in-moztrap -
qe-verify -

Firefox Tracking Flags

(firefox34 fixed, firefox35 fixed)

Details

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

User Story

See comment 3 for main suggestion of implementation.

As a signed-in FF browser user, I can be reached by other FxA Loop users on my registered e-mail address so that they can call me directly if my e-mail address figures in their contact list.
As a signed-in FF browser user, I can be reached by link clickers on my registered e-mail address so that they can call me back as they click a URL I generated

Attachments

(1 attachment, 2 obsolete attachments)

(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?
Flags: needinfo?(standard8)
No longer blocks: 1065052
Depends on: 1065052
Perhaps this is a dupe of bug 1000761?
Flags: qe-verify?
Flags: firefox-backlog+
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?
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.
Points: --- → 5
Flags: needinfo?(standard8)
Summary: When a call is received, check both Hawk sessions for pending calls → When a call is received, check both Hawk sessions for pending calls, and keep track of session type to handle deleting call urls
(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
User Story: (updated)
Depends on: 1032700
Assignee: nobody → pkerr
Blocks: 1066014
Severity: normal → enhancement
Iteration: --- → 35.1
User Story: (updated)
Target Milestone: --- → mozilla35
Duplicate of this bug: 1000761
Whiteboard: [loop-uplift]
Whiteboard: [loop-uplift] → [loop-uplift][loop-inccall1]
Untracking this for QE verification. Please needinfo me to request manual testing.
Flags: qe-verify? → qe-verify-
Iteration: 35.1 → ---
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?
Attachment #8490416 - Flags: feedback?(dmose)
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.
Attachment #8490416 - Attachment is obsolete: true
Attachment #8490416 - Flags: feedback?(dmose)
Attachment #8490843 - Flags: review?(dmose)
Attachment #8490843 - Flags: review?(dmose) → review?(standard8)
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 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.
Attachment #8490843 - Flags: review?(standard8) → review+
Consolidated GET/calls hawkRequest code in a function.
Attachment #8490843 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Carrying forward standard8 r+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/289416a752ff
Flags: in-testsuite?
Keywords: checkin-needed
Whiteboard: [loop-uplift][loop-inccall1] → [loop-uplift][loop-inccall1][fixed-in-fx-team]
Blocks: 1070065
https://hg.mozilla.org/mozilla-central/rev/289416a752ff
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [loop-uplift][loop-inccall1][fixed-in-fx-team] → [loop-uplift][loop-inccall1]
Iteration: --- → 35.2
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
Attachment #8491775 - Flags: approval-mozilla-aurora?
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.
Attachment #8491775 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.