1176958 - Participants list does not delete old nick when a participant changes it in XMPP MUC

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Description

[Abdelrahman Ahmed [:abdelrahman]]

When a participant in a MUC changes his/her nick, the participants list does not delete the old nick. Check XEP-0045 (7.6) [1].

[1] http://xmpp.org/extensions/xep-0045.html#changenick

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 1 - update participants list

Comment 2

[Patrick Cloke [:clokep]]

Comment on attachment 8626886 [details] [diff] [review]
rev 1 - update participants list

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

::: chat/locales/en-US/xmpp.properties
@@ +103,5 @@
> +#   This is displayed as a system message when a participant changes his/her
> +#   nickname in a room.
> +#   %1$S is the old nick.
> +#   %2$S is the new nick.
> +conversation.muc.nick=%1$S is now known as %2$S.

I think "nick" is an IRC term, what does XMPP call this?

::: chat/protocols/xmpp/xmpp.jsm
@@ +321,5 @@
> +    participant.name = aNewNick;
> +    this.removeParticipant(aOldNick);
> +    this._participants.set(aNewNick, participant);
> +    this.notifyObservers(new nsSimpleEnumerator([participant]),
> +                         "chat-buddy-add");

The IRC code calls "chat-buddy-update", do you know the ramifications of removing the participant and then adding it back?

Comment 3

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 2 - update participants list

(In reply to Patrick Cloke [:clokep] from comment #2)
> I think "nick" is an IRC term, what does XMPP call this?

Also nick or nickname.

> The IRC code calls "chat-buddy-update", do you know the ramifications of
> removing the participant and then adding it back?

Yes, I know and I was searching for something like "chat-buddy-update".

Comment 4

[aleth [:aleth]]

Comment on attachment 8626933 [details] [diff] [review]
rev 2 - update participants list

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +244,5 @@
>          this.joining = false;
>          return true;
>        });
>      }
> +    else if (codes.indexOf("210") != -1) {

Can there be more than one code in the stanza? What happens then?

Comment 5

[Patrick Cloke [:clokep]]

I suspect that this code is SUPER similar to the IRC code [1]...could we abstract this to jsProtoHelper?

[1] http://mxr.mozilla.org/comm-central/source/chat/protocols/irc/irc.js#335

Comment 6

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #4)
> Can there be more than one code in the stanza? What happens then?

Yes, as it must include at least code "110", so that the user knows this presence refers to itself as an occupant.

(In reply to Patrick Cloke [:clokep] from comment #5)
> I suspect that this code is SUPER similar to the IRC code [1]...could we
> abstract this to jsProtoHelper?
> 
> [1] http://mxr.mozilla.org/comm-central/source/chat/protocols/irc/irc.js#335

Yes, that would be better.

Comment 7

[Abdelrahman Ahmed [:abdelrahman]]

Comment on attachment 8626933 [details] [diff] [review]
rev 2 - update participants list

># HG changeset patch
># User Abdelrhman Ahmed <[e-mail]>
># Parent  59ae2306da5684907c1e72e1e1c5a14d2088702a
>Bug 1176958 - Update participants list when a nick is changed in XMPP MUC. r=clokep
>
>diff --git a/chat/locales/en-US/xmpp.properties b/chat/locales/en-US/xmpp.properties
>--- a/chat/locales/en-US/xmpp.properties
>+++ b/chat/locales/en-US/xmpp.properties
>@@ -94,16 +94,29 @@ chatRoomField.password=_Password
> #   received.
> #   %1$S is the inviter.
> #   %2$S is the room.
> #   %3$S is the reason which is a message provided by the person sending the
> #   invitation.
> conversation.muc.invitationWithReason=%1$S has invited you to join %2$S: %3$S
> conversation.muc.invitationWithoutReason=%1$S has invited you to join %2$S
> 
>+# LOCALIZATION NOTE (conversation.muc.*):
>+#   This is displayed as a system message when a participant changes his/her
>+#   nickname in a room.
>+#   %1$S is the old nick.
>+#   %2$S is the new nick.
>+conversation.muc.nick=%1$S is now known as %2$S.
>+
>+# LOCALIZATION NOTE (conversation.muc.*):
>+#   This is displayed as a system message when you changes your nickname in
>+#   a room or service changes it for you due to its policies.
>+#   %S is your new nick.
>+conversation.muc.nick.you=You are now known as %S.
>+
> # LOCALIZATION NOTE (options.*):
> #   These are the protocol specific options shown in the account manager and
> #   account wizard windows.
> options.resource=Resource
> options.priority=Priority
> options.connectionSecurity=Connection security
> options.connectionSecurity.requireEncryption=Require encryption
> options.connectionSecurity.opportunisticTLS=Use encryption if available
>diff --git a/chat/protocols/xmpp/xmpp.jsm b/chat/protocols/xmpp/xmpp.jsm
>--- a/chat/protocols/xmpp/xmpp.jsm
>+++ b/chat/protocols/xmpp/xmpp.jsm
>@@ -196,18 +196,20 @@ const XMPPMUCConversationPrototype = {
>       if (codes.indexOf("303") != -1) {
>         // XEP-0045 (7.6): Changing Nickname.
>         // Service Updates Nick for user.
>         if (!item  || !item.attributes["nick"]) {
>           this.WARN("Received a MUC presence code 303 stanza without an item " +
>                     "element or a nick attribute.");
>           return;
>         }
>-        let participant = this._participants.get(nick);
>-        participant.name = item.attributes["nick"];
>+        let newNick = item.attributes["nick"];
>+        this.updateNick(nick, newNick);
>+        let message =  _("conversation.muc.nick", nick, newNick);
>+        this.writeMessage(this.name, message, {system: true});
>         return;
>       }
>       if (item && item.attributes["role"] == "none") {
>         // XEP-0045: the user is no longer an occupant.
>         this.removeParticipant(nick);
>         if (codes.indexOf("110") != -1) {
>           // XEP-0045 7.14: Self-presence.
>           // This presence refers to this account.
>@@ -238,16 +240,32 @@ const XMPPMUCConversationPrototype = {
> 
>         // XEP-0045: Service Informs New Room Owner of Success
>         // for instant and reserved rooms.
>         this.left = false;
>         this.joining = false;
>         return true;
>       });
>     }
>+    else if (codes.indexOf("210") != -1) {
>+      // XEP-0045 (7.6): Changing Nickname.
>+      // Service modifies this user's nickname in accordance with local service
>+      // policies.
>+      if (!item  || !item.attributes["nick"]) {
>+        this.WARN("Received a MUC presence code 210 stanza without an item " +
>+                  "element or a nick attribute.");
>+        return;
>+      }
>+      let newNick = item.attributes["nick"];
>+      this.updateNick(this.nick, newNick);
>+      this.nick = newNick;
>+      let message =  _("conversation.muc.nick.you", newNick);
>+      this.writeMessage(this.name, message, {system: true});
>+      return;
>+    }
>     else if (codes.indexOf("110") != -1) {
>       // XEP-0045: Room exists and joined successfully.
>       this.left = false;
>       this.joining = false;
>       // TODO (Bug 1172350): Implement Service Discovery Extensions (XEP-0128) to obtain
>       // configuration of this room.
>     }
> 
>@@ -292,16 +310,31 @@ const XMPPMUCConversationPrototype = {
>       this._messageIds.add(id);
>     }
>     this.writeMessage(from, aMsg, flags);
>   },
> 
>   getNormalizedChatBuddyName: function(aNick)
>     this._account.normalizeFullJid(this.name + "/" + aNick),
> 
>+  // Updates the nick of a participant in MUC conversation to a new one.
>+  updateNick: function(aOldNick, aNewNick) {
>+    if (!this._participants.has(aOldNick)) {
>+      this.ERROR("Trying to rename nick that doesn't exist! " + aOldNick +
>+                 " to " + aNewNick);
>+      return;
>+    }
>+
>+    let participant = this._participants.get(aOldNick);
>+    participant.name = aNewNick;
>+    this._participants.delete(aOldNick);
>+    this._participants.set(aNewNick, participant);
>+    this.notifyObservers(participant, "chat-buddy-update", aOldNick);
>+  },
>+
>   // Removes a participant from MUC conversation.
>   removeParticipant: function(aNick) {
>     if (!this._participants.has(aNick))
>       return;
> 
>     this._participants.delete(aNick);
>     let nickString = Cc["@mozilla.org/supports-string;1"]
>                        .createInstance(Ci.nsISupportsString);

Comment 8

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #6)
> Yes, that would be better.

We can also do that for methods: removeParticipant and removeAllParticipants.

Comment 9

[aleth [:aleth]]

Don't forget to also look at yahoo when moving those methods to jsProtoHelper.

Comment 10

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 3 - update participants list

Comment 11

[aleth [:aleth]]

Comment on attachment 8629114 [details] [diff] [review]
rev 3 - update participants list

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

::: chat/modules/jsProtoHelper.jsm
@@ +624,5 @@
> +
> +  // Updates the nick of a participant in conversation to a new one.
> +  updateNick: function(aOldNick, aNewNick) {
> +    let isParticipant = this._participants.has(aOldNick);
> +    if (this.normalizeNick(aOldNick) == this.normalizeNick(this.nick)) {

normalizeNick only exists in irc. This makes me suspect you never tested this patch...

Comment 12

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 4 - update participants list

(In reply to aleth [:aleth] from comment #11)
> normalizeNick only exists in irc. This makes me suspect you never tested
> this patch...

Sorry, I did not test that patch as I was in a hurry when I prepared it.

Comment 13

[aleth [:aleth]]

Comment on attachment 8631933 [details] [diff] [review]
rev 4 - update participants list

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

::: chat/modules/jsProtoHelper.jsm
@@ +614,5 @@
>      this._joining = aJoining;
>      this.notifyObservers(null, "update-conv-chatjoining");
>    },
>  
> +  normalizeNick: function(aNick) aNick,

Not sure about the wisdom of introducing yet another normalize function for all JS prpls...

Comment 14

[Patrick Cloke [:clokep]]

Comment on attachment 8631933 [details] [diff] [review]
rev 4 - update participants list

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

::: chat/modules/jsProtoHelper.jsm
@@ +614,5 @@
>      this._joining = aJoining;
>      this.notifyObservers(null, "update-conv-chatjoining");
>    },
>  
> +  normalizeNick: function(aNick) aNick,

Maybe instead of this we should have a third parameter "isOwnNick" which IRC can pass in as |this.normalizeNick(aOldNick) == this.normalizeNick(this.nick)| and XMPP can pass in as True/False depending on the situation?

@@ +651,5 @@
> +    this.notifyObservers(participant, "chat-buddy-update", aOldNick);
> +  },
> +
> +  // Removes a participant from conversation.
> +  removeParticipant: function(aNick) {

removeParticipant and removeAllParticipants were completely identical across different protocols, correct?

::: chat/protocols/xmpp/xmpp.jsm
@@ +203,5 @@
>          }
> +        let newNick = item.attributes["nick"];
> +        this.updateNick(nick, newNick);
> +        let message =  _("conversation.muc.nick", nick, newNick);
> +        this.writeMessage(this.name, message, {system: true});

Where does IRC do this message? I guess I'm a little surprised this isn't in the factored out code.

Minor nit: two spaces after the = sign on the line above this.

Comment 15

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 5 - update participants list

(In reply to Patrick Cloke [:clokep] from comment #14)
> Maybe instead of this we should have a third parameter "isOwnNick" which IRC
> can pass in as |this.normalizeNick(aOldNick) ==
> this.normalizeNick(this.nick)| and XMPP can pass in as True/False depending
> on the situation?
Yes, it can.

> removeParticipant and removeAllParticipants were completely identical across
> different protocols, correct?
For current protocols that we support, yes.

> Where does IRC do this message? I guess I'm a little surprised this isn't in
> the factored out code.
In (https://dxr.mozilla.org/comm-central/source/chat/protocols/irc/irc.js#1292)
I factored out the message as it is usually used with updateNick to be within it.

Comment 16

[Patrick Cloke [:clokep]]

Comment on attachment 8636027 [details] [diff] [review]
rev 5 - update participants list

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

Looks pretty good! If the answer to the question is no, then just a couple of nits in comments.

::: chat/locales/en-US/conversations.properties
@@ +62,5 @@
> +#   %1$S is the old nick.
> +#   %2$S is the new nick.
> +nickSet=%1$S is now known as %2$S.
> +# LOCALIZATION NOTE (nickSet.you):
> +#   This is displayed as a system message when you changes your nickname in

Nit: "when you change your nickname"

I think this comment might be clearer as "This is displayed as a system message when your nickname is changed."

::: chat/modules/jsProtoHelper.jsm
@@ +642,5 @@
> +    }
> +    else
> +      message = _("nickSet", aOldNick, aNewNick);
> +
> +    // Get the original ircParticipant and then remove it.

Nit: Fix this comment to not refer to IRC.

::: chat/protocols/yahoo/yahoo.js
@@ +288,5 @@
>      let conf = this._conferences.get(aRoom);
>      conf.removeParticipant(aUsername);
> +    conf.writeMessage(this._roomName,
> +                      _("system.message.conferenceLogoff", aName),
> +                      {system: true});

Can this string also be combined?

Comment 17

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 6 - update participants list

(In reply to Patrick Cloke [:clokep] from comment #16)
> Can this string also be combined?

After checking protocols, no. As the way that IRC uses can not be easily to combine, and also the reason for removing participant (kick, ban or part) needs to be determined by protocol that calls |removeParticipant| method.

Comment 18

[Patrick Cloke [:clokep]]

Comment on attachment 8638752 [details] [diff] [review]
rev 6 - update participants list

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #17)
> Created attachment 8638752 [details] [diff] [review]
> rev 6 - update participants list
> 
> (In reply to Patrick Cloke [:clokep] from comment #16)
> > Can this string also be combined?
> 
> After checking protocols, no. As the way that IRC uses can not be easily to
> combine, and also the reason for removing participant (kick, ban or part)
> needs to be determined by protocol that calls |removeParticipant| method.

Great! Looks good. :)

Comment 19

[aleth [:aleth]]

url:        https://hg.mozilla.org/comm-central/rev/aeca087abe829d1c64f3665de34deed05b3c72be
changeset:  aeca087abe829d1c64f3665de34deed05b3c72be
user:       Abdelrhman Ahmed <[e-mail]>
date:       Fri Jul 24 13:32:00 2015 +0200
description:
Bug 1176958 - Update participants list when a nick is changed in XMPP MUC. r=clokep a=aleth CLOSED TREE

Comment 20

[aleth [:aleth]]

url:        https://hg.mozilla.org/comm-central/rev/ae81bda0a5582a7d5d279f79c99f497324580147
changeset:  ae81bda0a5582a7d5d279f79c99f497324580147
user:       aleth <[e-mail]>
date:       Mon Jul 27 15:36:06 2015 +0200
description:
Back out aeca087abe82 (Bug 1176958 - Update participants list when a nick is changed) for producing error messages. a=aleth CLOSED TREE

Comment 21

[aleth [:aleth]]

Backed out as it produces "Failed to get nickSet" errors.

Comment 22

[Patrick Cloke [:clokep]]

Updating milestone from 1.6 to 42.

Comment 23

[aleth [:aleth]]

Comment on attachment 8638752 [details] [diff] [review]
rev 6 - update participants list

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

::: chat/protocols/irc/irc.js
@@ +1195,5 @@
>      let msg;
>      if (this.normalizeNick(aOldNick) == this.normalizeNick(this._nickname)) {
>        // Your nickname changed!
>        this._nickname = aNewNick;
> +      msg = _conv("nickSet.you", aNewNick);

Inline this in the !isChat case - the common case for IRC is that all conversations are MUCs, so we don't need to pull this outside the loop.

@@ +1206,4 @@
>        });
>      }
>      else {
> +      msg = _conv("nickSet", aOldNick, aNewNick);

Not used.

::: chat/protocols/irc/ircUtils.jsm
@@ +13,5 @@
>    l10nHelper("chrome://chat/locale/irc.properties")
>  );
>  
> +XPCOMUtils.defineLazyGetter(this, "_conv", function()
> +  l10nHelper("chrome://chat/locale/conversation.properties")

Typo causing errors.

Comment 24

[aleth [:aleth]]

Can we fix the outstanding issues and land this?

Comment 25

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #24)
> Can we fix the outstanding issues and land this?

Yes, sure.

Comment 26

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 7 - update participants list

Comment 27

[aleth [:aleth]]

Comment on attachment 8651903 [details] [diff] [review]
rev 7 - update participants list

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +346,5 @@
>          this.joining = false;
>          return true;
>        });
>      }
> +    else if (codes.indexOf("210") != -1) {

This looks like duplicated code, or am I missing something?

How about if (codes.indexof("210")... || codes.indexof("303") { ...
You can explain the difference in the comment.

Comment 28

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #27)
> This looks like duplicated code, or am I missing something?

According to spec XEP-0045, the presence of code 303 has type attribute and its value is unavailable, but code 210 does not. (check examples 50 and 51)

Comment 29

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #28)
> (In reply to aleth [:aleth] from comment #27)
> > This looks like duplicated code, or am I missing something?
> 
> According to spec XEP-0045, the presence of code 303 has type attribute and
> its value is unavailable, but code 210 does not. (check examples 50 and 51)

Ah, thanks.

What's inside the if clause is still duplicated code though. How about a local helper function (inside onPresenceStanza), something like 
let changeNick = item => { ... }; ?

(Avoiding duplicated code means avoiding future potential breakages because one copy is changed while the other is overlooked)

Comment 30

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 8 - update participants list

Comment 31

[aleth [:aleth]]

Comment on attachment 8651967 [details] [diff] [review]
rev 8 - update participants list

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

::: chat/protocols/xmpp/xmpp.jsm
@@ +260,5 @@
>        }
>        if (codes.indexOf("303") != -1) {
>          // XEP-0045 (7.6): Changing Nickname.
>          // Service Updates Nick for user.
>          if (!item  || !item.attributes["nick"]) {

Sorry if I wasn't clear, but you can move this if clause to changeNick too! (It's OK if the warning message says "303 or 210" as the stanza itself is also logged.)

Comment 32

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 9 - update participants list

Comment 33

[aleth [:aleth]]

Comment on attachment 8651985 [details] [diff] [review]
rev 9 - update participants list

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

Thanks!

Comment 34

[aleth [:aleth]]

This patch no longer applies, please rebase.

Comment 35

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 9 - update participants list

Comment 36

[aleth [:aleth]]

Comment on attachment 8654497 [details] [diff] [review]
rev 9 - update participants list

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

::: chat/protocols/irc/ircUtils.jsm
@@ +13,5 @@
>    l10nHelper("chrome://chat/locale/irc.properties")
>  );
>  
> +XPCOMUtils.defineLazyGetter(this, "_conv", function()
> +  l10nHelper("chrome://chat/locale/conversations.properties")

Unfortunately, expression closures are going to be deprecated (bug 1083458, bug 1151473), use an arrow function instead.

Comment 37

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: rev 10 - update participants list

Comment 38

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/48f86f412c85272ca4e57813f812f905570e6e3e
Bug 1176958 - Update participants list when a nick is changed in XMPP MUC. r=aleth,clokep