Closed Bug 1092953 Opened 10 years ago Closed 9 years ago

Loop "delete a room" operation should ask end-user confirmation

Categories

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

defect
Points:
2

Tracking

(firefox35 verified, firefox36 verified, firefox37 verified)

VERIFIED FIXED
mozilla37
Iteration:
37.1
Tracking Status
firefox35 --- verified
firefox36 --- verified
firefox37 --- verified
backlog Fx35+

People

(Reporter: NiKo, Assigned: mikedeboer)

References

Details

(Whiteboard: [rooms])

Attachments

(3 files, 3 obsolete files)

We can't use a system confirm() modal prompt here, because of unwanted side effects:

- it closes (hides) the panel once the prompt is opened;
- it resets the CSS "active" state applied to any clicked element (the delete button), making styling using CSS animations really difficult (not impossible; just *very* difficult)

We should probably invent a tiny widget to require user confirmation inline, next to the delete button (an in-document html modal confirmation is another option). 

Note: With proper iconing, this shouldn't require new strings.
NI :darrin here.
Blocks: 1074676
Flags: needinfo?(dhenein)
Blocks: 1074671
backlog: --- → Fx35+
Priority: -- → P1
Whiteboard: [rooms]
Flags: needinfo?(dhenein) → needinfo?(sfranks)
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Iteration: --- → 36.3
Points: --- → 2
Flags: qe-verify+
Flags: firefox-backlog+
Flags: needinfo?(mmucci)
Mark, this is the bare minimum to get to into an acceptable state. There are too many possible side-effects when using an HTML prompt.
Attachment #8524619 - Flags: review?(standard8)
Added to IT 36.3
Flags: needinfo?(mmucci)
Flags: needinfo?(sfranks)
Attachment #8524820 - Flags: ui-review?(dhenein)
Comment on attachment 8524820 [details]
Room deletion confirmation

Looks good, I think this fits well with the design language already established and conveys the severity of the action.
Attachment #8524820 - Flags: ui-review?(dhenein) → ui-review+
Attachment #8524619 - Flags: review?(paolo.mozmail)
Comment on attachment 8524619 [details] [diff] [review]
Patch v1: show a modal confirm dialog when a user attempts to delete a room

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

Looks good, but please add a unit test for the panel change.

::: browser/components/loop/MozLoopAPI.jsm
@@ +357,5 @@
> +          buttonFlags = Services.prompt.STD_YES_NO_BUTTONS;
> +          callback = okButtonMessage;
> +          okButtonMessage = cancelButtonMessage = null;
> +        } else {
> +          buttonFlags = 

nit: trailing whitespace

::: browser/components/loop/content/js/panel.js
@@ +503,5 @@
>        }));
>        this.setState({urlCopied: true});
>      },
>  
>      handleDeleteButtonClick: function(event) {

Please can you add a unit test for this function.
Attachment #8524619 - Flags: review?(standard8) → review-
Comment on attachment 8524619 [details] [diff] [review]
Patch v1: show a modal confirm dialog when a user attempts to delete a room

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

Looks good as a stopgap in order not to regress the notification added for Contacts.

I agree that an in-line UI would be better, though it requires much more effort.

::: browser/components/loop/MozLoopAPI.jsm
@@ +355,5 @@
> +        // A simple yes/ no prompt does not have button captions.
> +        if (arguments.length == 2) {
> +          buttonFlags = Services.prompt.STD_YES_NO_BUTTONS;
> +          callback = okButtonMessage;
> +          okButtonMessage = cancelButtonMessage = null;

I'd rather have an explicit buttonTypes indication than an arguments.length detection.

Since the callback must be the last argument and we now need a variable number of arguments, I recommend changing the signature to function(options, callback).
Attachment #8524619 - Flags: review?(paolo.mozmail)
I'll add a mochitest in a separate patch.
Attachment #8524619 - Attachment is obsolete: true
Attachment #8526036 - Flags: review?(paolo.mozmail)
A mochitest is good, but we should also unit test panel.jsx in the style that we have been already - actually, I just realised, we have a test for this already:

http://hg.mozilla.org/mozilla-central/annotate/6ce1b906c690/browser/components/loop/test/desktop-local/panel_test.js#l736

I suspect that's broken with the current patch (I forgot to run the test suites).
Comment on attachment 8526036 [details] [diff] [review]
Patch v2: show a modal confirm dialog when a user attempts to delete a room

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

Thanks! r+ with the following change, and getting review on tests is required before landing.

::: browser/components/loop/content/js/contacts.js
@@ +407,5 @@
> +            message: mozL10n.get("confirm_delete_contact_alert"),
> +            buttons: [
> +              mozL10n.get("confirm_delete_contact_remove_button"),
> +              mozL10n.get("confirm_delete_contact_cancel_button")
> +            ]

I'd make clear which button is which.

buttons: {
  yes: "...",
  no: "...",
}

or

yesButton: "...",
noButton: "...",

I'd also throw if only one of these properties is specified.
Attachment #8526036 - Flags: review?(paolo.mozmail) → review+
Paolo, you mean like this?
Attachment #8526036 - Attachment is obsolete: true
Attachment #8526071 - Flags: review?(paolo.mozmail)
Comment on attachment 8526071 [details] [diff] [review]
Patch v3: show a modal confirm dialog when a user attempts to delete a room

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

::: browser/components/loop/MozLoopAPI.jsm
@@ +352,5 @@
>        writable: true,
> +      value: function(options, callback) {
> +        if (!("message" in options) ||
> +            !("okButton") in options ||
> +            !("cancelButton" in options)) {

Something strange is going on here...

@@ +364,3 @@
>              (Ci.nsIPrompt.BUTTON_POS_0 * Ci.nsIPrompt.BUTTON_TITLE_IS_STRING) +
>              (Ci.nsIPrompt.BUTTON_POS_1 * Ci.nsIPrompt.BUTTON_TITLE_IS_STRING);
> +        } else {

Maybe a simple way to do the check could be:

if (options.okButton && options.cancelButton)
  ...
else if (!options.okButton && !options.cancelButton)
  ...
else
  error

Rest looks good!
Attachment #8526071 - Flags: review?(paolo.mozmail)
Iteration: 36.3 → 37.1
Attachment #8526071 - Flags: review+
Attachment #8526071 - Flags: review+
Carrying over r=paolo.
Attachment #8526071 - Attachment is obsolete: true
Attachment #8529066 - Flags: review+
Comment on attachment 8529112 [details] [diff] [review]
Patch 2: pdate the room delete button test to take the confirm dialog into account

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

Right ideas, I think there's a few improvements that could be made. I'm giving you r+ since it is test-only code and I think you'll be able to get what I'm intending from the comments. If you want me to take another look at it at it later tonight I can do, alternately I'm sure NiKo will be happy to take a look tomorrow.

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +58,5 @@
>            callback(null, []);
>          },
>          on: sandbox.stub()
> +      },
> +      confirm: function(options, callback) {

if you make this a sandbox.stub() instead of a custom function, then below...

@@ +786,5 @@
>        it("should not display a delete button by default", function() {
>          expect(deleteButton).to.not.equal(null);
>        });
>  
>        it("should call the delete function when clicked", function() {

nit: would be good to change this to "should dispatch a delete action when confirmation is granted"

@@ +789,5 @@
>  
>        it("should call the delete function when clicked", function() {
>          sandbox.stub(dispatcher, "dispatch");
>  
> +        confirmResult = true;

you can make this:

navigator.mozLoop.confirm.callsArgWith(1, true);

You could possibly alias navigator.mozLoop.confirm to something like "mozLoopConfirmStub" to be clearer if you want.

@@ +794,3 @@
>          TestUtils.Simulate.click(deleteButton);
>  
> +        expect(confirmCalled).to.equal(1);

I don't think you need this here - if the confirm isn't called, then one of these tests are going to fail.

If you do want to keep it, then I suggest another test:

it("should confirm deletion")

which can check:

sinon.assert.calledOnce(navigator.mozLoop.confirm);

@@ +798,5 @@
>          sinon.assert.calledWithExactly(dispatcher.dispatch,
>            new sharedActions.DeleteRoom({roomToken: roomData.roomToken}));
>        });
> +
> +      it("should cancel the delete action when appropraite", function() {

nit: "should not dispatch an action when the confirmation is cancelled".

@@ +801,5 @@
> +
> +      it("should cancel the delete action when appropraite", function() {
> +        sandbox.stub(dispatcher, "dispatch");
> +
> +        confirmResult = false;

You can obviously do similar changes in this test - just change the parameter.
Attachment #8529112 - Flags: review?(standard8) → review+
https://hg.mozilla.org/mozilla-central/rev/5bd85d90593f
https://hg.mozilla.org/mozilla-central/rev/ab05a2c29e73
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Comment on attachment 8529066 [details] [diff] [review]
Patch 1.4: show a modal confirm dialog when a user attempts to delete a room

Approval Request Comment
[Feature/regressing bug #]: Rooms

[User impact if declined]: No confirmation asked for for room delete, and we can't use the normal modal dialog per bug.

[Describe test coverage new/current, TBPL]: Includes tests in patch 2

[Risks and why]: UI issue identified once we started using Rooms.  Relatively simple patch.  

[String/UUID change made/needed]: none
Attachment #8529066 - Flags: approval-mozilla-beta?
Attachment #8529066 - Flags: approval-mozilla-aurora?
Attachment #8529066 - Flags: approval-mozilla-beta?
Attachment #8529066 - Flags: approval-mozilla-beta+
Attachment #8529066 - Flags: approval-mozilla-aurora?
Attachment #8529066 - Flags: approval-mozilla-aurora+
Verified on Linux (Yes, No, room open or not, etc)
Verified on beta builds as well
Verified fixed 37.0a1 (2014-12-11) Win 7.
Also verified FF 36, 35 in comments 20, 22
You need to log in before you can comment on or make changes to this bug.