955525 - New contact does not get added to the correct tag

Status: RESOLVED FIXED

(Chat Core :: Yahoo! Messenger, defect)

Description

[aleth [:aleth]]

*** Original post on bio 2088 at 2013-08-02 11:27:00 UTC ***

STR
1) "Add contact", selecting an existing tag, say X.
2) New contact eventually shows up in a "Friends" tag which did not exist before.
3) This is the bad part: The tag X is now broken. All its existing contacts show up in the "other contacts" tag. It's impossible to select tag X from the context menu, even though it still shows up there.

Comment 1

[aleth [:aleth]]

*** Original post on bio 2088 at 2013-08-02 11:29:31 UTC ***

After a restart, the tag X is thankfully back to working as normal and its existing contacts are still associated with it.

Comment 2

[aleth [:aleth]]

*** Original post on bio 2088 at 2013-08-02 13:06:10 UTC ***

Also, post-restart, the contact added in 1) actually has both tag X and "Friends".

Comment 3

[Quentin Headen [:qheaden]]

*** Original post on bio 2088 at 2013-08-02 18:03:06 UTC ***

Okay. I'm pretty sure this has to do with my TODO here http://lxr.instantbird.org/instantbird/source/chat/protocols/yahoo/yahoo-session.jsm#857. I'll work on a fix for it.

Comment 4

[Quentin Headen [:qheaden]]

Attachment: Patch 1

*** Original post on bio 2088 as attmnt 2686 at 2013-08-08 18:34:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 5

[Patrick Cloke [:clokep]]

*** Original post on bio 2088 at 2013-08-09 20:05:05 UTC ***

Comment on attachment 8354455 [details] [diff] [review] (bio-attmnt 2686)
Patch 1

>diff --git a/chat/protocols/yahoo/yahoo-session.jsm b/chat/protocols/yahoo/yahoo-session.jsm
>--- a/chat/protocols/yahoo/yahoo-session.jsm
>+++ b/chat/protocols/yahoo/yahoo-session.jsm
>@@ -931,7 +931,10 @@ const YahooPacketHandler = {
>         packet.addValue(334, 0);
>         this._session.sendBinaryData(packet.toArrayBuffer());
> 
>-        // TODO - Possibly allow different tags to be used.
>+        // If someone wants to add us as a buddy, we authorize them and place
>+        // them under the Friends tag. If we are adding a buddy ourselves, this
>+        // method call will do nothing since the buddy will already be in our
>+        // friends list.
>         this._account.addBuddy(Services.tags.createTag("Friends"),
Why is this hard coded to friends? Doesn't the add buddy dialogue (Why is this a dialogue again? :P) let you specify the tag to add it to?

>diff --git a/chat/protocols/yahoo/yahoo.js b/chat/protocols/yahoo/yahoo.js
>--- a/chat/protocols/yahoo/yahoo.js
>+++ b/chat/protocols/yahoo/yahoo.js
>@@ -281,6 +281,9 @@ YahooAccount.prototype = {
> 
>   // Called when the user adds a new contact within Instantbird.
>   addBuddy: function(aTag, aName) {
>+    // Don't add duplicate buddies.
>+    if (this._buddies.has(aName))
>+      return;
When do we expect this to happen? It should be in the comment. (I.e. is this an error or an expected condition?)

Comment 6

[Patrick Cloke [:clokep]]

Comment on attachment 8354455 [details] [diff] [review]
Patch 1

*** Original change on bio 2088 attmnt 2686 at 2013-08-11 18:41:53 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 7

[Quentin Headen [:qheaden]]

Attachment: Patch 2

*** Original post on bio 2088 as attmnt 2693 at 2013-08-12 11:25:00 UTC ***

This patch localizes the group tag "Friends", but it still uses the default tag because it is for buddy authorization requests (when a buddy wants to be your friend). Right now, the buddy auth UI doesn't allow specifying of tags.

Comment 8

[Quentin Headen [:qheaden]]

Comment on attachment 8354455 [details] [diff] [review]
Patch 1

*** Original change on bio 2088 attmnt 2686 at 2013-08-12 11:25:06 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 9

[Patrick Cloke [:clokep]]

Comment on attachment 8354462 [details] [diff] [review]
Patch 2

*** Original change on bio 2088 attmnt 2693 at 2013-08-12 12:31:42 UTC ***

>-  // Called when the user adds a new contact within Instantbird.
>+  // Called when the user adds or authorizes a new contact within Instantbird.
I should have caught this during initial review, but since we're not touching this comment: it shouldn't refere to "Instantbird" since it's in chat/ and is used by Thunderbird as well. You can probably just remove the "within Instantbird" part.

Comment 10

[Quentin Headen [:qheaden]]

Attachment: Patch 3

*** Original post on bio 2088 as attmnt 2694 at 2013-08-12 12:39:00 UTC ***

Fixed.

Comment 11

[Quentin Headen [:qheaden]]

Comment on attachment 8354462 [details] [diff] [review]
Patch 2

*** Original change on bio 2088 attmnt 2693 at 2013-08-12 12:39:47 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 12

[Patrick Cloke [:clokep]]

Comment on attachment 8354463 [details] [diff] [review]
Patch 3

*** Original change on bio 2088 attmnt 2694 at 2013-08-12 13:27:06 UTC ***

>diff --git a/chat/protocols/yahoo/yahoo-session.jsm b/chat/protocols/yahoo/yahoo-session.jsm
>-        // TODO - Possibly allow different tags to be used.
>-        this._account.addBuddy(Services.tags.createTag("Friends"),
>-                               this.userName);
>+        // If someone wants to add us as a buddy, place them under the default
>+        // "Friends" tag. If we are adding someone as a buddy, this method will
>+        // do nothing since the buddy will already be in our list.
>+        let tag = Services.tags.createTag(_("buddy.auth.defaultGroup"));
>+        this._account.addBuddy(tag, this.userName);
We discussed on IRC how the tag creation should be moved to addBuddy.

Comment 13

[Quentin Headen [:qheaden]]

Attachment: Patch 4

*** Original post on bio 2088 as attmnt 2695 at 2013-08-12 17:33:00 UTC ***

> >diff --git a/chat/protocols/yahoo/yahoo-session.jsm b/chat/protocols/yahoo/yahoo-session.jsm
> >-        // TODO - Possibly allow different tags to be used.
> >-        this._account.addBuddy(Services.tags.createTag("Friends"),
> >-                               this.userName);
> >+        // If someone wants to add us as a buddy, place them under the default
> >+        // "Friends" tag. If we are adding someone as a buddy, this method will
> >+        // do nothing since the buddy will already be in our list.
> >+        let tag = Services.tags.createTag(_("buddy.auth.defaultGroup"));
> >+        this._account.addBuddy(tag, this.userName);
> We discussed on IRC how the tag creation should be moved to addBuddy.

We now see that addBuddy is an interface method, and as a result, it expects an already created tag to be passed as the first parameter. So I moved the duplication check outside of addBuddy using the new hasBuddy method, and if the buddy already exists, it prevents creation of the tag.

Comment 14

[Quentin Headen [:qheaden]]

Comment on attachment 8354463 [details] [diff] [review]
Patch 3

*** Original change on bio 2088 attmnt 2694 at 2013-08-12 17:33:00 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 15

[Patrick Cloke [:clokep]]

Comment on attachment 8354464 [details] [diff] [review]
Patch 4

*** Original change on bio 2088 attmnt 2695 at 2013-08-12 18:52:38 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 16

[Quentin Headen [:qheaden]]

Attachment: Patch 5

*** Original post on bio 2088 as attmnt 2705 at 2013-08-13 00:40:00 UTC ***

Small update from v4 of the patch that changes the default group from "Friends" to "Contacts".

Comment 17

[Quentin Headen [:qheaden]]

Comment on attachment 8354464 [details] [diff] [review]
Patch 4

*** Original change on bio 2088 attmnt 2695 at 2013-08-13 00:40:55 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 18

[Patrick Cloke [:clokep]]

Comment on attachment 8354474 [details] [diff] [review]
Patch 5

*** Original change on bio 2088 attmnt 2705 at 2013-08-13 00:46:49 UTC ***

OK, this looks good.

Let's file a follow up to not have a Contacts tag in every protocol.

Comment 19

[Patrick Cloke [:clokep]]

*** Original post on bio 2088 at 2013-08-14 13:49:15 UTC ***

http://hg.instantbird.org/instantbird/rev/1eea25566706

(In reply to comment #12)
> Let's file a follow up to not have a Contacts tag in every protocol.

This is bug 955543 (bio 2105).