Closed
Bug 1089722
Opened 10 years ago
Closed 10 years ago
Loop roomListStore should validate deleted rooms
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(firefox35 fixed, firefox36 fixed)
backlog | Fx35+ |
People
(Reporter: NiKo, Assigned: standard8)
References
Details
(Whiteboard: [rooms])
Attachments
(1 file, 1 obsolete file)
8.80 KB,
patch
|
mikedeboer
:
review+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
don't believe there's work here - but validation that it's done in initial rooms
Reporter | ||
Comment 2•10 years ago
|
||
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
Updated•10 years ago
|
Whiteboard: [rooms]
Updated•10 years ago
|
Priority: P2 → P1
Assignee | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
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 | ||
Updated•10 years ago
|
Assignee: nobody → standard8
Iteration: --- → 36.3
Points: --- → 2
Comment 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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+
Assignee | ||
Comment 8•10 years ago
|
||
Target Milestone: --- → mozilla36
Comment 9•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Comment 10•10 years ago
|
||
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?
Comment 11•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8529061 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 12•10 years ago
|
||
Updated•10 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•