1089722 - Loop roomListStore should validate deleted rooms

Status: RESOLVED FIXED

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

Description

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

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

Atm, the room schema defined in roomListStore.js won't validate deleted rooms; it should.

Comment 1

[:shell escalante]

don't believe there's work here - but validation that it's done in initial rooms

Comment 2

[Nicolas Perriault (:NiKo`) — needinfo me if you need my attention]

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

Comment 3

[Mark Banner (:standard8)]

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.

Comment 4

[Mark Banner (:standard8)]

Attachment: Handle rooms being deleted in LoopRooms#getAll.

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.

Comment 5

[Mike de Boer [:mikedeboer]]

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?

Comment 6

[Mark Banner (:standard8)]

Attachment: Handle Loop rooms being deleted in the backend, and sending appropriate notifications v2

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

Comment 7

[Mike de Boer [:mikedeboer]]

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!

Comment 8

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/4ca1d6fffaf8

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4ca1d6fffaf8

Comment 10

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

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

Comment 11

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

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

Comment 12

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

https://hg.mozilla.org/releases/mozilla-beta/rev/4c2283a46749