Closed Bug 1089722 Opened 10 years ago Closed 10 years ago

Loop roomListStore should validate deleted rooms


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



(firefox35 fixed, firefox36 fixed)

Tracking Status
firefox35 --- fixed
firefox36 --- fixed
backlog Fx35+


(Reporter: NiKo, Assigned: standard8)



(Whiteboard: [rooms])


(1 file, 1 obsolete file)

Spec ref:

Atm, the room schema defined in roomListStore.js won't validate deleted rooms; it should.
don't believe there's work here - but validation that it's done in initial rooms
Blocks: 1092954
backlog: --- → Fx35+
Priority: -- → P2
There's work; atm we can have the whole room list stop functioning if we ever get a deleted room entry from the server (through the mozLoop.rooms API).

The idea is *at least*:
- to stop throwing in case Room validation data fails
- or catch the exception appropriately
- or more simply to just log the error onto the browser console
Whiteboard: [rooms]
Priority: P2 → P1
We need to fix this before the next version of the server goes out. We're currently broken with the latest server master if you delete a room.
I took a look into this. The main problem we've got at the moment is that LoopRoomsInternal#delete removes the room from the list. When the getAll is then triggered from the push notification, it gets the deleted room details, sees the token isn't in the list, and assumes the room is being added.

Additionally, it'll add deleted rooms to the list of rooms.

Given that a deleted room is only given to us by the server for 27 mins (bug 1094838), then I don't think it makes sense to have a deleted room stored in the list of rooms.

Hence, this handles the deleted flag on a room. I tested across two clients and it worked fine. I'm not sure if at this stage we should work out what happens if a client is offline for > 30 mins, and then gets a room list without rooms - that feels like something we could save until later, depending on how we reload lists and things after a push re-registration.

Asking for feedback before I do some tests for this.
Attachment #8527693 - Flags: feedback?(mdeboer)
Assignee: nobody → standard8
Iteration: --- → 36.3
Points: --- → 2
Comment on attachment 8527693 [details] [diff] [review]
Handle rooms being deleted in LoopRooms#getAll.

Review of attachment 8527693 [details] [diff] [review]:

This looks a-ok, why didn't you request review for this?

You need to add a test for this in `browser/components/loop/test/xpcshell/test_looprooms.js`. Can you attach a separate patch for that?
Attachment #8527693 - Flags: feedback?(mdeboer) → review+
I hadn't requested review, as I wasn't sure it was complete, and I hadn't written tests.

In writing the tests, I discovered that we would have got duplicate delete events emitted - one from the delete function, one from the getAll when the push notification updates. I've gone for only triggering when we get the push notification - since we need that for when people are logged into more than one location.

We might want to do it the other way for more responsiveness on the UI, but I think as we've got the modal dialog, there's not much to do there.

I also discovered that we needed to do a similar code change in the get function (although we don't currently really use that anywhere iirc).
Attachment #8527693 - Attachment is obsolete: true
Attachment #8529061 - Flags: review?(mdeboer)
Comment on attachment 8529061 [details] [diff] [review]
Handle Loop rooms being deleted in the backend, and sending appropriate notifications v2

Review of attachment 8529061 [details] [diff] [review]:

Ship it!
Attachment #8529061 - Flags: review?(mdeboer) → review+
Target Milestone: --- → mozilla36
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment on attachment 8529061 [details] [diff] [review]
Handle Loop rooms being deleted in the backend, and sending appropriate notifications v2

Approval Request Comment
[Feature/regressing bug #]: Rooms (delete feature)

[User impact if declined]: Serious problems if there are deleted rooms in the list.

[Describe test coverage new/current, TBPL]: Includes tests; is on central, plus manual testing.

[Risks and why]: Handling of deleted rooms is required.  pretty low risk; mostly puts existing code inside an "if (deleted) { ... } else { old code }" and maintains the deleted state.

[String/UUID change made/needed]: none
Attachment #8529061 - Flags: approval-mozilla-aurora?
Flags: qe-verify-
Flags: in-moztrap-
Comment on attachment 8529061 [details] [diff] [review]
Handle Loop rooms being deleted in the backend, and sending appropriate notifications v2

Approval Request Comment
Move request to beta
Attachment #8529061 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Depends on: 1106538
Attachment #8529061 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.