Closed Bug 955525 Opened 6 years ago Closed 6 years ago

New contact does not get added to the correct tag

Categories

(Chat Core :: Yahoo! Messenger, defect, major)

defect
Not set
major

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: aleth, Assigned: qheaden)

Details

Attachments

(1 file, 4 obsolete files)

*** 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.
*** 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.
*** 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".
*** 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.
Attached patch Patch 1 (obsolete) — Splinter Review
*** 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 ***
Attachment #8354455 - Flags: review?(clokep)
Assignee: nobody → qheaden
Status: NEW → ASSIGNED
*** 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 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 ***
Attachment #8354455 - Flags: review?(clokep) → review-
Attached patch Patch 2 (obsolete) — Splinter Review
*** 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.
Attachment #8354462 - Flags: review?(clokep)
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 ***
Attachment #8354455 - Attachment is obsolete: true
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.
Attachment #8354462 - Flags: review?(clokep) → review-
Attached patch Patch 3 (obsolete) — Splinter Review
*** Original post on bio 2088 as attmnt 2694 at 2013-08-12 12:39:00 UTC ***

Fixed.
Attachment #8354463 - Flags: review?(clokep)
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 ***
Attachment #8354462 - Attachment is obsolete: true
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.
Attachment #8354463 - Flags: review?(clokep) → review-
Attached patch Patch 4 (obsolete) — Splinter Review
*** 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.
Attachment #8354464 - Flags: review?(clokep)
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 ***
Attachment #8354463 - Attachment is obsolete: true
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 ***
Attachment #8354464 - Flags: review?(clokep) → review+
Whiteboard: [checkin-needed]
Attached patch Patch 5Splinter Review
*** 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".
Attachment #8354474 - Flags: review?(clokep)
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 ***
Attachment #8354464 - Attachment is obsolete: true
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.
Attachment #8354474 - Flags: review?(clokep) → review+
*** 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).
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.