If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

PushHandler registration logic should be isolated from rooms implementation details

RESOLVED INCOMPLETE

Status

Hello (Loop)
Client
P5
enhancement
Rank:
55
RESOLVED INCOMPLETE
3 years ago
11 months ago

People

(Reporter: standard8, Unassigned)

Tracking

unspecified
Points:
5
Bug Flags:
firefox-backlog +
qe-verify -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [tech-debt])

User Story

* When a room-related push notification is received, request GET /rooms/ from the server and update the store.

https://wiki.mozilla.org/Loop/Architecture/Rooms#GET_.2Frooms

* Provide a mechanism so that updates can be pushed to the panel/conversation views via MozLoopAPI

Attachments

(2 attachments, 8 obsolete attachments)

17.83 KB, patch
Details | Diff | Splinter Review
48.47 KB, patch
MattN
: feedback-
Details | Diff | Splinter Review
Comment hidden (empty)
(Reporter)

Updated

3 years ago
User Story: (updated)
(Reporter)

Updated

3 years ago
Blocks: 1074666
(Reporter)

Updated

3 years ago
Blocks: 1074672
(Reporter)

Updated

3 years ago
Severity: normal → enhancement
> * Provide a mechanism so that updates can be pushed to the panel/conversation views via MozLoopAPI      

We can use notifyStatusChanged for this or do a similar thing where an observer notification is converted into a DOM Event.

Updated

3 years ago
backlog: --- → Fx35+

Comment 2

3 years ago
Created attachment 8507645 [details] [diff] [review]
Update rooms store on notification

WIP: need to complete unit tests

Updated

3 years ago
Assignee: nobody → pkerr
Status: NEW → ASSIGNED

Comment 3

3 years ago
Is there a notification when a room is deleted? What would such an event look like in the room list returned when I perform a GET rooms/?version=xxxx after receiving a notification? Just seeing a room missing from a version-ed request would only imply that nothing had changed.
Flags: needinfo?(adam)

Comment 4

3 years ago
(In reply to Paul Kerr [:pkerr] from comment #3)
> Is there a notification when a room is deleted? What would such an event
> look like in the room list returned when I perform a GET rooms/?version=xxxx
> after receiving a notification? Just seeing a room missing from a version-ed
> request would only imply that nothing had changed.

Ooh, good question. You'll definitely see a push notification, but the means of conveying that the room is gone aren't currently defined.

What I would propose is adding an new field, "deleted". If present and true, it indicates that the room associated with the indicated token is gone; basically, something like:

HTTP/1.1 200 OK
Connection: keep-alive
Content-Length: 30
Content-Type: application/json; charset=utf-8
Date: Wed, 16 Jul 2014 13:23:04 GMT
ETag: W/"1e-2896316483"
Timestamp: 1405516984

[
    {
        "roomToken": "_nxD4V4FflQ",
        "deleted: true
    }
]

Alexis / Paul -- does that sound like a reasonable approach to you?
Flags: needinfo?(pkerr)
Flags: needinfo?(alexis+bugs)
Flags: needinfo?(adam)

Comment 5

3 years ago
That is clear indication on the notification side. It works for me.
Flags: needinfo?(pkerr)
I would rather use the 410 GONE status rather than having this "deleted" attribute. Also, I believe that after some time the rooms will be just deleted from the server and as such we would return a 404 as well.

Would this approach work for you?
Flags: needinfo?(alexis+bugs) → needinfo?(pkerr)

Comment 7

3 years ago
(In reply to Alexis Metaireau (:alexis) from comment #6)
> I would rather use the 410 GONE status rather than having this "deleted"
> attribute. Also, I believe that after some time the rooms will be just
> deleted from the server and as such we would return a 404 as well.
> 
> Would this approach work for you?

Ah, sorry -- I should have been clearer in my request.

For |GET /rooms/3jKS_Els9IU HTTP/1.1|, what you propose makes perfect sense. I would expect something like a 410 or 404 in that case.

We're talking about |GET /rooms?version=<version> HTTP/1.1|, where we might find out about changes to several resources at a time. For example, the response might need to say "room X has been deleted, and room Y has been added." With my proposal, this would look like:

[
    {
        "roomToken": "_nxD4V4FflQ",
        "deleted": true
    },
    {
        "roomToken": "QzBbvGmIZWU",
        "roomName": "Room Y",
        "roomUrl": "http://localhost:3000/rooms/QzBbvGmIZWU",
        "maxSize": 2,
        "currSize": 0,
        "ctime": 1405517418
    }
]

With that clarification, does what I'm proposing make more sense?
Flags: needinfo?(alexis+bugs)

Comment 8

3 years ago
So, the sequence would be:
1) notification is sent via the PushServer, incrementing the version xxxx.
2) loop client requests a list of updated rooms via GET /rooms?version=xxxx.
3) loop client makes a GET /rooms/<token> call for each of the rooms found in the list returned from the previous call.
4) a GET /rooms/<token> request will receive a 410 GONE status if the room with the token has been deleted.

Since that is the flow the client goes through currently to determine what has been changed the 410 can be handled if it works as I have detailed above.
Flags: needinfo?(pkerr)

Comment 9

3 years ago
(In reply to Paul Kerr [:pkerr] from comment #8)
> So, the sequence would be:
> 1) notification is sent via the PushServer, incrementing the version xxxx.
> 2) loop client requests a list of updated rooms via GET /rooms?version=xxxx.
> 3) loop client makes a GET /rooms/<token> call for each of the rooms found
> in the list returned from the previous call.
> 4) a GET /rooms/<token> request will receive a 410 GONE status if the room
> with the token has been deleted.
> 
> Since that is the flow the client goes through currently to determine what
> has been changed the 410 can be handled if it works as I have detailed above.

There are a couple of problems here.

The first is that the information from the single |GET /rooms?version=xxxx| call should return enough information for you to handle processing of any room that you're not in. If you find a need to iterate through the rooms and do a |GET /rooms/<token>| on each of them, then I've botched the API and we need to fix |GET /rooms?version=xxxx|. I really don't want a client with five rooms to do six GETs just because they got a single push notification -- that's a recipe for disaster.

The second is that you're proposing that |GET /rooms?version=xxxx| should show the presence of a room that doesn't actually exist any longer, in a way that's indistinguishable from an existing room. That's just going to cause confusion.

When it comes down to it, the "?version=xxxx" variation is a shortcut for getting the whole state, akin to asking for a patch on a codebase rather than getting the whole codebase. Right now, the defined protocol allows communicating *additions* and *changes*. What it's missing is a format for *deletions*. We need to add one, and the proposal I'm making is the simplest way of doing so that I can easily devise.

Tagging pkerr for needinfo, mostly to verify that we don't currently need to iterate through all of a user's rooms on a push notification, and to confirm that anything that leads us down a path of doing should be flagged to me so we can discuss alternate approaches.
Flags: needinfo?(pkerr)

Comment 10

3 years ago
The client does not run through the entire list of rooms when a notification is received. It runs through only those rooms that are returned from the /rooms?version request. That is, those rooms that have changed in that version.

I feel that getting the room details is necessary since all that I can tell from the room list is if the room name or currSize has changed. But, even if currSize is the same the participants could have changed with one leaving and another entering. I can tell which room is active by looking at the currSize but the code at this point has no idea what the UI is displaying. It sends the update details on to registered entities in the content side.
Flags: needinfo?(pkerr)

Comment 11

3 years ago
(In reply to Paul Kerr [:pkerr] from comment #10)
> The client does not run through the entire list of rooms when a notification
> is received. It runs through only those rooms that are returned from the
> /rooms?version request. That is, those rooms that have changed in that
> version.
> 
> I feel that getting the room details is necessary since all that I can tell
> from the room list is if the room name or currSize has changed. But, even if
> currSize is the same the participants could have changed with one leaving
> and another entering. I can tell which room is active by looking at the
> currSize but the code at this point has no idea what the UI is displaying.
> It sends the update details on to registered entities in the content side.

Okay, so it's looking increasingly like the aggregated state needs to simply be an array of the exact same state that would be returned by getting each room individually. I worry about the server performance implications of kicking off multiple HTTP transactions in response to a simple push, which is why we moved to have different push endpoints for different kinds of state.

Alexis: are you okay with unifying the formats here, where the body of |GET /rooms/version=xxxx| is simply an array containing objects that look just like those you get from |GET /rooms/{token}|? We'd add "token" to the objects, but they'd otherwise be as defined here: https://wiki.mozilla.org/Loop/Architecture/Rooms#GET_.2Frooms.2F.7Btoken.7D
We currently aren't storing any history for the rooms, so returning their state is probably a good thing to do.

I would need to know for how long do we need to store the history of a room after it's been deleted on the server side ?

Apart from that, it seems completely okay to return the list of rooms and their state *since the version was issued*.
Flags: needinfo?(alexis+bugs)

Comment 13

3 years ago
Created attachment 8508763 [details] [diff] [review]
Update rooms store on notification

WIP

Updated

3 years ago
Attachment #8507645 - Attachment is obsolete: true
Depends on: 1087041

Comment 14

3 years ago
I've updated the API description to match the outcome of this discussion. Please double-check my changes to make sure they match your understanding: https://wiki.mozilla.org/index.php?title=Loop%2FArchitecture%2FRooms&diff=1027249&oldid=1023877
Flags: needinfo?(pkerr)
Flags: needinfo?(alexis+bugs)

Comment 15

3 years ago
Yes, this change looks good to me. A one pass operation will help the client implementation.
Flags: needinfo?(pkerr)
Yes that reflects my understanding, perfect!

For implementation, I need to know for how long we need to hold this information on the server.

Thanks!
Flags: needinfo?(alexis+bugs) → needinfo?(adam)

Comment 17

3 years ago
(In reply to Alexis Metaireau (:alexis) from comment #16)
> For implementation, I need to know for how long we need to hold this
> information on the server.

tl;dr: If the server doesn't need to keep this state for any other reason, use 27 minutes. If the client asks for a version older than 27 minutes ago, return an error with a new, distinct code. If the client gets this new code, it needs to pull a full list of rooms instead of using "?version=<version>".

----

In previous conversations, you'd mentioned the use of "410 Gone" when people try to use a deleted room, which is presumably a different code than when people try to use a room that has never existed (which I assume will return a 404). If this is your intention, then  you'll need a tombstone for the room for a pretty long time (on the order of days or weeks), which would be more than sufficient for the purpose of this bug.

If you're not holding the state to distinguish between "deleted rooms" and "tokens that were never a room" for that (or some other) reason, then we're down to what this bug requires. In normal operation, the requirement is that the state is around for long enough for the client to get the push notification and then perform the GET. These operations are all over TCP, which means that technically each of these messages can be delayed by network loss for up to 9 minutes. There are three TCP operations in involved here (Loop Server to Simple Push Server; Simple Push Server to client; and client to Loop Server), so the theoretical maximum is 27 minutes.

That said, these things always find a way to operate outside the theoretical maximum, and we should be able to handle such situations. Since we're discarding information necessary to know about a proper delta after some period of time, then we need to indicate that we *can't* communicate the changes as a delta, and instead ask the client to retrieve a full set of data; basically, an error code that means "I can't tell you what has changed since that version, so you need to just ask for the whole state again."
Flags: needinfo?(adam)
Thanks for the answer,

If that's possible, I would like to drop the fact that we know a room had been deleted after some amount of time, and that's what I'm asking for here. In other words, after some time the server will always answer 404, even if the room existed a long time ago, because that doesn't make sense to keep this information for ever.

What I'm trying to find out is what should be the maximum time we keep this information. During this period, we will be able to answer the GET on rooms with the "deleted = true" attribute, and after this time, the information will not be present (and if accessed, a room will just say it doesn't exist).

Would that be acceptable, and if yes, what would be the time we need to store this information? 3*9 seems like the time needed for the client to be alerted and get the information it needs, so we could use this TTL value, I believe.

Comment 19

3 years ago
Created attachment 8510572 [details] [diff] [review]
Update rooms store on notification

ebased to apply after fx-team (assumed to already incorporate 1074663) after 1074664 and 1074699: fx-team -> 664 -> 699 -> 665

Updated

3 years ago
Attachment #8508763 - Attachment is obsolete: true

Comment 20

3 years ago
WIP: still needs to handle a room delete as part of a server notification. Need to track the decision of the server team as to the final disposition of handling a delete.

Comment 21

3 years ago
Created attachment 8510760 [details] [diff] [review]
Update rooms store on notification

Updated

3 years ago
Attachment #8510572 - Attachment is obsolete: true
Paul, is it OK if I steal this one?
Assignee: pkerr → mdeboer
Iteration: --- → 36.2
Points: --- → 3
Flags: firefox-backlog+
Created attachment 8511891 [details] [diff] [review]
WIP: new push handler

Updated

3 years ago
Flags: qe-verify?
Flags: qe-verify? → qe-verify-
Created attachment 8512247 [details] [diff] [review]
Part 1: refactor PushHandler to encapsulate more of the registration flow, simplify the API and more

Matt, do you have some time today to take a look at this to see if you like this approach and have some/ any suggestions to improve things, possibly?
I know it's a largish patch; I'll chop it up once it's review time.
Attachment #8511891 - Attachment is obsolete: true
Attachment #8512247 - Flags: feedback?(MattN+bmo)
Created attachment 8512253 [details] [diff] [review]
Part 1: refactor PushHandler to encapsulate more of the registration flow, simplify the API and more
Attachment #8512247 - Attachment is obsolete: true
Attachment #8512247 - Flags: feedback?(MattN+bmo)
Attachment #8512253 - Flags: feedback?(MattN+bmo)
Comment on attachment 8512247 [details] [diff] [review]
Part 1: refactor PushHandler to encapsulate more of the registration flow, simplify the API and more

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

::: browser/components/loop/LoopPushHandler.jsm
@@ +339,5 @@
> +      for (let [fieldName, channelID] of fieldsToRegister) {
> +        payload[fieldName] = gChannels.get(channelID);
> +      }
> +
> +      yield MozLoopService.registerWithLoopServer(sessionType, payload);

I dislike this. As I said in bug 1079198:
"I don't think onPushRegistered should call registerWithLoopServer directly so we can properly fix bug 1076428 since right now it doesn't know whether to do a Guest or FxA registration."

No push-related methods should call registerWithLoopServer. This should be the job of MozLoopService init/setup method IMO. This push module should only know about push stuff.
Attachment #8512247 - Attachment is obsolete: false
Attachment #8512247 - Attachment is obsolete: true
Attachment #8512247 - Flags: feedback-
Comment on attachment 8512253 [details] [diff] [review]
Part 1: refactor PushHandler to encapsulate more of the registration flow, simplify the API and more

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

::: browser/components/loop/LoopPushHandler.jsm
@@ +10,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Timer.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "MozLoopService",
> +                                        "resource:///modules/loop/MozLoopService.jsm");

This approach also leads to a circular dependency.
Attachment #8512253 - Flags: feedback?(MattN+bmo) → feedback-
(In reply to Matthew N. [:MattN] (behind on reviews - focused on Loop) from comment #26)
> No push-related methods should call registerWithLoopServer. This should be
> the job of MozLoopService init/setup method IMO. This push module should
> only know about push stuff.

I agree; should MozLoopService then in turn ask the PushHandler for the URLs to register/ fields to pass in the request?
Yes, or the promise returned by the successful registration can have the URLs as a resolution value.

Updated

3 years ago
Priority: -- → P1

Updated

3 years ago
Iteration: 36.2 → ---
Created attachment 8512631 [details] [diff] [review]
Part 1: rename MozLoopPushHandler to LoopPushHandler and refresh its implementation
Attachment #8512253 - Attachment is obsolete: true
Attachment #8512631 - Flags: review?(standard8)
To review this part 1, it might be best to take `LoopPushHandler.jsm` after you applied the patch and review the file in its entirety. Splinter might not give you the right view of what moved where.
Created attachment 8512645 [details] [diff] [review]
Part 1: rename MozLoopPushHandler to LoopPushHandler and refresh its implementation

Unit test updates will be part 2.
Attachment #8512631 - Attachment is obsolete: true
Attachment #8512631 - Flags: review?(standard8)
Attachment #8512645 - Flags: review?(standard8)
Comment on attachment 8512645 [details] [diff] [review]
Part 1: rename MozLoopPushHandler to LoopPushHandler and refresh its implementation

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

::: browser/components/loop/LoopPushHandler.jsm
@@ +42,5 @@
> +let gInitDone = false;
> +// A flag which signifies that we've connected with the push server.
> +let gConnected = false;
> +// Maps channel IDs to their respective push end-point.
> +let gChannels = new Map();

Why make these all globals instead of "private" underscore-prefix members of the objects? This makes it harder to check the state of these variables in tests and doesn't actually hide them from extensions.

Also, while working on another bug I found that the gChannels map should IMO describe what it's holding, not what the keys are so therefore I think this should be gEndpoints.

::: browser/components/loop/LoopRooms.jsm
@@ +260,5 @@
>  Object.freeze(LoopRoomsInternal);
>  
> +const kChannelIDs = new Map([
> +  ["FxA", "6add272a-d316-477c-8335-f00f73dfde71"],
> +  ["guest", "19d3f799-a8f3-4328-9822-b7cd02765832"]

Nit: keep the case consistent. One is capitalized, while the other is not. For LOOP_SESSION_TYPE they all all-caps since they are consts.

::: browser/components/loop/MozLoopService.jsm
@@ +32,5 @@
>  Cu.import("resource://gre/modules/Task.jsm");
>  Cu.import("resource://gre/modules/FxAccountsOAuthClient.jsm");
>  
> +Cu.import("resource:///modules/loop/LoopCalls.jsm");
> +Cu.import("resource:///modules/loop/LoopRooms.jsm");

Keep these lazy so they aren't loaded when we do early returns in the initialize method.

@@ +334,5 @@
> +            gRegisteredDeferred.resolve("registered to guest status");
> +            // No need to clear the promise here, everything was good, so we don't need
> +            // to re-register.
> +          }, error => {throw error});
> +      }, error => {throw error}).catch(error => {

Can't ", error => {throw error}" be deleted? If not, you're missing a semicolon after |throw error|.

@@ +480,5 @@
>    /**
>     * Registers with the Loop server either as a guest or a FxA user.
>     *
>     * @param {LOOP_SESSION_TYPE} sessionType The type of session e.g. guest or FxA
> +   * @param {Boolean} pushUrls The push url given by the push server.

Is this intentional? The description doesn't match the type.
Attachment #8512645 - Flags: feedback-
Attachment #8512645 - Flags: review?(standard8)
Created attachment 8512832 [details] [diff] [review]
Part 1.1: rename MozLoopPushHandler to LoopPushHandler and refresh its implementation

What do you think of this?
Attachment #8512645 - Attachment is obsolete: true
Attachment #8512832 - Flags: feedback?(MattN+bmo)

Comment 35

3 years ago
I like the move to a promise-based API. Side-effects to consider:

There are a number of xpcshell and mochi tests that rely on the ability to override the pushUrl (of which there used to be only one) for the push channels to trigger a number of test conditions. Another set of tests uses a mock version of the PushHandler that is found in both tests/xpcshell/head.js and tests/mochitest/head.js. (see bug 1074663 for the effected files.) Is there a plan to handle the broken tests?
(In reply to Paul Kerr [:pkerr] from comment #35)
> I like the move to a promise-based API. 

\o/

> There are a number of xpcshell and mochi tests that rely on the ability to
> override the pushUrl (of which there used to be only one) for the push
> channels to trigger a number of test conditions. Another set of tests uses a
> mock version of the PushHandler that is found in both tests/xpcshell/head.js
> and tests/mochitest/head.js. (see bug 1074663 for the effected files.) Is
> there a plan to handle the broken tests?

Yeah, that'd be part 2, which I'm not really looking forward too... Part 1 should make the test bits easier to read, I hope.

Comment 37

3 years ago
Comment on attachment 8512832 [details] [diff] [review]
Part 1.1: rename MozLoopPushHandler to LoopPushHandler and refresh its implementation

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

::: browser/components/loop/LoopPushHandler.jsm
@@ +199,5 @@
> +        this._connected = true;
> +        if (this._UAID !== msg.uaid) {
> +          this._UAID = msg.uaid;
> +        }
> +        eventEmitter.emit("connected");

There seems to be a step missing here. If the UAID returned in the "hello" response is not the same as the stored version, then registration with the push server of all the channels, regardless of having been already registered, must be performed. And, if the URLs returned from the new PushServer registration have changed, registration with the LoopServer would need another cycle.

Even if the UAID is the same, any new channels that may have been passed in through a register() call should be registered once the connection has been re-established. (In the case where MozLoopService delays FxA registration until user login, for example.)

Basically, the design goal for the PushHandler module is to silently re-establish the PushServer connection across network drops, so the open socket -> hello -> register cycle must be able to re-run at any point. (see bug 1028869 for the next planned extension of the retry logic using a 30 minute ping.)
(In reply to Paul Kerr [:pkerr] from comment #37)
> There seems to be a step missing here. If the UAID returned in the "hello"
> response is not the same as the stored version, then registration with the
> push server of all the channels, regardless of having been already
> registered, must be performed. And, if the URLs returned from the new
> PushServer registration have changed, registration with the LoopServer would
> need another cycle.

Very true! thanks for catching that! The last part of 'registration with the LoopServer would need another cycle' is also missing in the current implementation... are you trying to say that this should be added?

Comment 39

3 years ago
(In reply to Mike de Boer [:mikedeboer] from comment #38)
> (In reply to Paul Kerr [:pkerr] from comment #37)
> > There seems to be a step missing here. If the UAID returned in the "hello"
> > response is not the same as the stored version, then registration with the
> > push server of all the channels, regardless of having been already
> > registered, must be performed. And, if the URLs returned from the new
> > PushServer registration have changed, registration with the LoopServer would
> > need another cycle.
> 
> Very true! thanks for catching that! The last part of 'registration with the
> LoopServer would need another cycle' is also missing in the current
> implementation... are you trying to say that this should be added?

It is something that should be done, but it is not currently the responsibility of the PushHandler. The way this is signaled is by LoopPushHandler calling the registration callback again with the new push URL. However, MozLoopService does not do anything in this case. In almost all cases I have seen in testing with the PushServer, sending a "hello" after a network disconnect, with a list of channels that have already been registered, will result in the PushServer simply accepting the old assignments. But, the PushServer is permitted to generate a fresh assignment.
Comment on attachment 8512832 [details] [diff] [review]
Part 1.1: rename MozLoopPushHandler to LoopPushHandler and refresh its implementation

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

It seems like this needs significant work to address the following:

(Quoting Paul Kerr [:pkerr] from comment #37)
> Basically, the design goal for the PushHandler module is to silently
> re-establish the PushServer connection across network drops, so the open
> socket -> hello -> register cycle must be able to re-run at any point. (see
> bug 1028869 for the next planned extension of the retry logic using a 30
> minute ping.)

::: browser/components/loop/MozLoopService.jsm
@@ +324,5 @@
>      // it back to null on error.
>      let result = gRegisteredDeferred.promise;
>  
> +    // Refer to LoopCalls _and_ LoopRooms, to make sure they are loaded.
> +    LoopCalls, LoopRooms;

Why do we need this? Is there some code that runs upon import?
Attachment #8512832 - Flags: feedback?(MattN+bmo) → feedback-

Comment 41

3 years ago
For reference and background, here is the initial bug calling for the retry logic: bug 1002414
No longer depends on: 1087041
Since I already made the LoopRooms class aware and deal with push notifications in bug 1089547, this bug is not needed anymore to obtain that functionality. I updated the bug fields to reflect reality.
Assignee: mdeboer → nobody
No longer blocks: 1074666, 1074672
Status: ASSIGNED → NEW
backlog: Fx35+ → ---
Points: 3 → 5
No longer depends on: 1074663, 1074664
Priority: P1 → P4
Summary: Update rooms store on push notifications → PushHandler registration logic should be isolated from calls and rooms implementation details
Whiteboard: [rooms]
(Reporter)

Updated

3 years ago
Whiteboard: [tech-debt]

Updated

3 years ago
backlog: --- → Fx37?
We have been checking this for FxOS Loop client and the answer format for retrieving deleted rooms is what we needed. However, we have some doubts about the specific behaviour for getting deleted rooms. When a device has received a push notification with version=x for a deleted room, what does it need to invoke to retrieve the list of deleted rooms?

 A/ GET /rooms?version=x 
 B/ GET /rooms?version=y (where y is a previous version to x and the previous version notification received by the client)

In the approach is B/, in that case, when a client registers for the first time, it needs to know the current "version" number so that it can check for deleted rooms when the first notification is received. 

Additionally, and kind of related, when multiple clients register with the same Identity, would they share "version" in the notifications received or will they have independent version counters?
Flags: needinfo?(adam)
(Reporter)

Comment 44

3 years ago
(In reply to Daniel Coloma:dcoloma from comment #43)
> We have been checking this for FxOS Loop client and the answer format for
> retrieving deleted rooms is what we needed. However, we have some doubts
> about the specific behaviour for getting deleted rooms. When a device has
> received a push notification with version=x for a deleted room, what does it
> need to invoke to retrieve the list of deleted rooms?

This is probably not the right bug to be discussing this on. I'd recommend an email thread or a separate bug.

What I would say is that from the desktop perspective, we're treating the rooms list as a cache within desktop - if the user restarts (and probably at some stage, if a reconnection happens), then we get the list afresh.

For push notifications, we always use the version provided in the notification, and that gives us the info about the deleted rooms (in the latest version of the server, the current production 0.13 doesn't).
Flags: needinfo?(adam)
(Reporter)

Updated

3 years ago
backlog: Fx37? → tech-debt
Priority: P4 → P5

Updated

3 years ago
Rank: 55
(Reporter)

Updated

2 years ago
Summary: PushHandler registration logic should be isolated from calls and rooms implementation details → PushHandler registration logic should be isolated from rooms implementation details
Whiteboard: [tech-debt] → [tech-debt][triage]
Whiteboard: [tech-debt][triage] → [tech-debt]
(Reporter)

Comment 45

11 months ago
Support for Hello/Loop has been discontinued.

https://support.mozilla.org/kb/hello-status

Hence closing the old bugs. Thank you for your support.
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → INCOMPLETE
You need to log in before you can comment on or make changes to this bug.