Loop roomListStore should validate deleted rooms

RESOLVED FIXED in Firefox 35

Status

Hello (Loop)
Client
P1
normal
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: NiKo, Assigned: standard8)

Tracking

unspecified
mozilla36
Points:
2
Dependency tree / graph
Bug Flags:
in-testsuite +
in-moztrap -
qe-verify -

Firefox Tracking Flags

(firefox35 fixed, firefox36 fixed)

Details

(Whiteboard: [rooms])

Attachments

(1 attachment, 1 obsolete attachment)

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

4 years ago
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]

Updated

4 years ago
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.
Created attachment 8527693 [details] [diff] [review]
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.
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+
Created attachment 8529061 [details] [diff] [review]
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).
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+
https://hg.mozilla.org/mozilla-central/rev/4ca1d6fffaf8
Status: NEW → RESOLVED
Last Resolved: 4 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?
status-firefox35: --- → affected
status-firefox36: --- → fixed
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+

Updated

4 years ago
status-firefox35: affected → fixed
You need to log in before you can comment on or make changes to this bug.