1074676 - As a user, I should be able to delete a room

Status: VERIFIED FIXED

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

Description

[Mark Banner (:standard8)]

Attachment: Expected UX

Comment 1

[Mark Banner (:standard8)]

We need UX/strings for the alert that's been mentioned in the updated story.

Also do we really want a system-modal alert? How should the panel behaviour be with respect to staying open etc?

Comment 2

[Darrin Henein [:darrin]]

The strings are available in the 35 strings spreadsheet, ping me if you don't have access. Not sure what other UX is required... we are using a system modal because it will be the easiest/fastest to implement. Also deleting a room is of such severity that it warrants a confirm dialog.

The panel should remain open while the user responds to the modal.

Comment 3

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

Estimated to 8 because of that comment from the spec[0]:

> Client implementation note: Before deleting a room, the client should check room membership and forceDisconnect() all current participants 

[0]: https://wiki.mozilla.org/Loop/Architecture/Rooms#DELETE_.2Frooms.2F.7Btoken.7D

Comment 4

[Mark Banner (:standard8)]

(In reply to Nicolas Perriault (:NiKo`) — needinfo me if you need my attention from comment #3)
> Estimated to 8 because of that comment from the spec[0]:
> 
> > Client implementation note: Before deleting a room, the client should check room membership and forceDisconnect() all current participants 
> 
> [0]:
> https://wiki.mozilla.org/Loop/Architecture/Rooms#DELETE_.2Frooms.2F.7Btoken.
> 7D

I think we might want to consider splitting this part out to a separate bug - especially as we haven't yet got the room window up and running.

This bug can then be a simple, delete and confirm, with appropriate UX.

Comment 5

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

Attachment: Delete a Loop room (WiP).

Works, untested. Submitting for early feedback.

Comment 6

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

Attachment: Delete a Loop room (WiP).

Updated patch with basic styling for the delete button. As a side note, we can't really use a native system confirm() prompt for deletion confirmation, because it has the side effect of closing the panel.

I could come with some tiny react component to display some handcrafted inline confirmation widget, or file a bug to prevent system modals to close the panel.

What do you think?

Comment 7

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

Attachment: Delete a Loop room.

This is now ready for rewiew, I think. Filed two follow-up bugs:

- Bug 1092953
- Bug 1092954

Try build: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=621fe7960732

Comment 8

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

Attachment: Delete a Loop room.

Fixed an issue with CSS. Removed use passed openRoom() method in RoomList component, as we're now using the dispatcher directly.

Comment 9

[Mike de Boer [:mikedeboer]]

Comment on attachment 8515891 [details] [diff] [review]
Delete a Loop room.

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

::: browser/components/loop/LoopRooms.jsm
@@ +52,5 @@
>    rooms: new Map(),
>  
> +  get sessionType() {
> +    return MozLoopService.userProfile ? LOOP_SESSION_TYPE.FXA :
> +                                        LOOP_SESSION_TYPE.GUEST;

nice move!

@@ +175,5 @@
> +  delete: function(roomToken, callback) {
> +    // XXX bug 1092954: Before deleting a room, the client should check room
> +    //     membership and forceDisconnect() all current participants.
> +    let room = this.rooms.get(roomToken);
> +    let deleteUri = "/rooms/" + encodeURIComponent(roomToken);

nit: rename this to `url`.

@@ +179,5 @@
> +    let deleteUri = "/rooms/" + encodeURIComponent(roomToken);
> +    MozLoopService.hawkRequest(this.sessionType, deleteUri, "DELETE")
> +      .then(response => {
> +        this.rooms.delete(roomToken);
> +        eventEmitter.emit("remove", room);

rename this event to `delete`

::: browser/components/loop/content/shared/img/svg/trash-16x16.svg
@@ +1,5 @@
> +<?xml version="1.0" encoding="utf-8"?>
> +<!-- Generator: Adobe Illustrator 18.1.0, SVG Export Plug-In . SVG Version: 6.00 Build 0)  -->
> +<svg version="1.1" id="Layer_1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0px" y="0px"
> +	 viewBox="0 0 16 16" enable-background="new 0 0 16 16" xml:space="preserve">
> +<circle fill-rule="evenodd" clip-rule="evenodd" fill="#D74345" cx="8" cy="8" r="8"/>

please add this shape to `icons-16x16.svg`. Lesser HTTP request.

::: browser/components/loop/content/shared/js/roomListStore.js
@@ +188,5 @@
>       * @param {String} eventName       The event name (unused).
>       * @param {Object} removedRoomData The removed room data.
>       */
>      _onRoomRemoved: function(eventName, removedRoomData) {
> +      console.log("_onRoomRemoved", removedRoomData);

You might want to remove this.

@@ +293,5 @@
> +     */
> +    deleteRoom: function(actionData) {
> +      this._mozLoop.rooms.delete(actionData.roomToken, function(err) {
> +        if (err) {
> +         this._dispatchAction(new sharedActions.DeleteRoomError({error: err}));

nit: indentation

::: browser/components/loop/jar.mn
@@ +48,5 @@
>    content/browser/loop/shared/img/svg/glyph-account-16x16.svg   (content/shared/img/svg/glyph-account-16x16.svg)
>    content/browser/loop/shared/img/svg/glyph-signin-16x16.svg    (content/shared/img/svg/glyph-signin-16x16.svg)
>    content/browser/loop/shared/img/svg/glyph-signout-16x16.svg   (content/shared/img/svg/glyph-signout-16x16.svg)
>    content/browser/loop/shared/img/svg/copy-16x16.svg            (content/shared/img/svg/copy-16x16.svg)
> +  content/browser/loop/shared/img/svg/trash-16x16.svg           (content/shared/img/svg/trash-16x16.svg)

remove this as well.

::: browser/components/loop/test/xpcshell/test_looprooms.js
@@ +1,5 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this file,
>   * You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
> +/* jshint esnext: true*/

Did we want this in our code?

@@ +180,5 @@
> +  let eventCalled = false;
> +  LoopRooms.once("remove", (e, room) => {
> +    eventCalled = true;
> +  });
> +  yield LoopRooms.promise("delete", "QzBbvGmIZWU");

Rename the event here too.

@@ +182,5 @@
> +    eventCalled = true;
> +  });
> +  yield LoopRooms.promise("delete", "QzBbvGmIZWU");
> +  Assert.ok(eventCalled, "remove event should have fired");
> +  let rooms = yield LoopRooms.promise("getAll");

Can't you do something like:

```js
let room = yield LoopRooms.promise("get", "QzBbvGmIZWU");
Assert.ok(!room, "Deleted room should be removed from the current list");
```

@@ +183,5 @@
> +  });
> +  yield LoopRooms.promise("delete", "QzBbvGmIZWU");
> +  Assert.ok(eventCalled, "remove event should have fired");
> +  let rooms = yield LoopRooms.promise("getAll");
> +  Assert.ok(!rooms.some((room) => room.roomToken === "QzBbvGmIZWU"),

nit: no need for `===`

Comment 10

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

Attachment: Delete a Loop room.

Addressed Mike's comments, expect this one:

> Can't you do something like:
>
> ```js
> let room = yield LoopRooms.promise("get", "QzBbvGmIZWU");
> Assert.ok(!room, "Deleted room should be removed from the current list");
> ```

This doesn't work, didn't really take the time to investigate why and kept the previous test which gets us covered anyway I think.

Comment 11

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

Attachment: Delete a Loop room.

Rebased on top of latest fx-team.

Comment 12

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

Attachment: Delete a Loop room.

Unbitrotted, again. Phew.

Comment 13

[Mark Banner (:standard8)]

Comment on attachment 8516084 [details] [diff] [review]
Delete a Loop room.

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

Looks good, r=Standard8

Comment 14

[Mark Banner (:standard8)]

https://hg.mozilla.org/integration/fx-team/rev/25c3e3a3f38c

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/25c3e3a3f38c

Comment 16

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/a6f34832bb46

Comment 17

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

Comment on attachment 8516084 [details] [diff] [review]
Delete a Loop room.

Approval Request Comment
Landed on aurora per IRC with lsblakk with a=loop-only

Comment 18

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 35b4, 36.0a2 (2014-12-17) Ubuntu 13.04