1038257 - Desktop client needs the ability to delete, block, and unblock a contact locally

Status: VERIFIED FIXED

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

Description

[Neil Deakin]

The UI is section 6 of https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#contacts

1. Add a context menu item for a contact 'Remove Contact'.
2. When selected, remove the contact from the store.
3. Remove the contact from the displayed list. It will also need to be removed from any other lists where the contacts appear.
4. Selecting the contact from the list and pressing delete should also delete the contact.

Since deleting a contact loses information, the user should be able to undo this operation, or be asked for confirmation beforehand.

Comment 1

[Madhava Enros [:madhava]]

Two points of clarification:

for 1 -- "Remove Contact" is in the dropdown menu next to the call button. Is this what you mean by context menu?

for 4 -- it looks like this was discussed in bug 1038272 and the conclusion was for the delete key _not_ to remove a selected contact.

Comment 2

[Romain Testard [:RT]]

(In reply to Neil Deakin from comment #0)
> The UI is section 6 of
> https://people.mozilla.org/~dhenein/labs/loop-mvp-spec/#contacts
> 
> 1. Add a context menu item for a contact 'Remove Contact'.
Please note this only applies to contacts which were added locally (i.e not applicable to Google contacts)

> 2. When selected, remove the contact from the store.
> 3. Remove the contact from the displayed list. It will also need to be
> removed from any other lists where the contacts appear.
> 4. Selecting the contact from the list and pressing delete should also
> delete the contact.
The delete key won't be supported for MVP
> 
> Since deleting a contact loses information, the user should be able to undo
> this operation, or be asked for confirmation beforehand.
Yes, Darrin suggests a system confirm/cancel dialog as per https://bugzilla.mozilla.org/show_bug.cgi?id=1038272#c2

Comment 3

[Mike de Boer [:mikedeboer]]

(In reply to Madhava Enros [:madhava] from comment #1)
> Two points of clarification:
> 
> for 1 -- "Remove Contact" is in the dropdown menu next to the call button.
> Is this what you mean by context menu?

Yes.

Comment 5

[Maire Reavy [:mreavy]]

Updated points per conversation with Mike

Comment 6

[u279076]

Tracking this for QE verification.

Comment 7

[:shell escalante]

There was a string added for a "confirm delete" message after selecting "Remove Contact".  The localization info was: confirm_delete_contact_alert=Are you sure you want to delete this contact?

Comment 8

[:Paolo Amadini]

Attachment: The patch

Comment 9

[:Paolo Amadini]

Note that blocking and unblocking are included in this patch.

Comment 10

[Maire Reavy [:mreavy]]

(In reply to :Paolo Amadini from comment #9)
> Note that blocking and unblocking are included in this patch.

Thanks for the update, Paolo.  So, would it make sense to close Bug 1017052 (blocking a contact) and Bug 1021729 (unblocking a contact) as duplicates of this bug?

Comment 11

[Mike de Boer [:mikedeboer]]

No, they'll be resolved - fixed once this bug is fixed. They're already marked as blocking, so that's all we need right now :)

Comment 12

[Mike de Boer [:mikedeboer]]

Comment on attachment 8496904 [details] [diff] [review]
The patch

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

Please see my comment below... the other change look good to me!

::: browser/components/loop/content/js/contacts.jsx
@@ +75,5 @@
>                onClick={this.onItemClick} data-action="edit">
>              <i className="icon icon-edit" />
>              {mozL10n.get("edit_contact_menu_button")}
>            </li>
> +          {this.props.blocked ?

This looks a bit bulky to me... what about the following:

``js
let blockAction = this.props.blocked ? "unblock" : "block";

return (
  <li className="dropdown-menu-item"
      onClick={this.onItemClick} data-action={blockAction}>
    <i className={"icon icon-" + blockAction"} />
    {mozL10n.get(blockAction + "_contact_menu_button")}
  </li>
)
```

Comment 13

[:Paolo Amadini]

Attachment: Updated patch

Comment 14

[Mike de Boer [:mikedeboer]]

Comment on attachment 8497392 [details] [diff] [review]
Updated patch

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

\o/ r=me

Comment 15

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/6d875de038d4

Comment 16

[:Paolo Amadini]

This bug now tracks the verification work for the block/unblock functions as well.

Comment 19

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

https://hg.mozilla.org/mozilla-central/rev/6d875de038d4

Comment 20

[Paul Silaghi, QA [:pauly]]

(In reply to sescalante from comment #7)
> There was a string added for a "confirm delete" message after selecting
> "Remove Contact".  The localization info was:
> confirm_delete_contact_alert=Are you sure you want to delete this contact?
I don't see any confirmation message when removing contacts

Comment 21

[:Paolo Amadini]

(In reply to Paul Silaghi, QA [:pauly] from comment #20)
> (In reply to sescalante from comment #7)
> > There was a string added for a "confirm delete" message after selecting
> > "Remove Contact".  The localization info was:
> > confirm_delete_contact_alert=Are you sure you want to delete this contact?
> I don't see any confirmation message when removing contacts

No, we didn't implement a confirmation message. We don't have a way to select multiple contacts and delete them in the current user interface, this means that when users want to delete multiple contacts, they would have to confirm deleting each manually added contact individually.

I think the confirmation message for each contact assumed mass deletions were an alternative.

Darrin, given the low amount of data associated to each contact, do you think not showing a confirmation is the correct UI for the first version? Note that imported contacts can't be deleted locally anyways (the menu item is disabled).

Comment 22

[Darrin Henein [:darrin]]

(In reply to :Paolo Amadini from comment #21)
> Darrin, given the low amount of data associated to each contact, do you
> think not showing a confirmation is the correct UI for the first version?
> Note that imported contacts can't be deleted locally anyways (the menu item
> is disabled).

I still think a confirmation is correct, given that some data loss is happening.

Comment 23

[:Paolo Amadini]

(In reply to Darrin Henein [:darrin] from comment #22)
> I still think a confirmation is correct, given that some data loss is
> happening.

Filed bug 1077412.

Comment 24

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 35.0a1 (2014-10-03) Win 7, OS X 10.9.5, Ubuntu 13.04

Comment 25

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

Comment on attachment 8497392 [details] [diff] [review]
Updated patch

Approval Request Comment
Part of the staged Loop aurora second uplift set

Comment 26

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

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

Comment 27

[Paul Silaghi, QA [:pauly]]

Verified fixed FF 34b1 Win 7

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8497392 [details] [diff] [review]
Updated patch

Already landed in aurora, approving for posterity

Comment 29

[u279076]

Does this have developer automation coverage?

Comment 30

[:Paolo Amadini]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #29)
> Does this have developer automation coverage?

Not that I know of. Bug 1073468 is still open.