Closed Bug 1050779 Opened 10 years ago Closed 9 years ago

Handle XMPP Room chat invitations

Categories

(Chat Core :: XMPP, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gfritos, Assigned: abdelrahman)

References

()

Details

Attachments

(1 file, 4 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/36.0.1985.125 Safari/537.36

Steps to reproduce:

- on an other IM client (Pidgin for example) create a XMPP Room Chat
- invite user with IM XMPP account configured in Thunderbird


Actual results:

- no invitation received to join the Room Chat
- error in the console:
Error: Trying to create a conversation; buddy not present: private-room-4567@conference.jabber.hot-chilli.net
Source File : resource:///modules/xmpp.jsm
Line: 1134
Source File :
prpl-jabber: XMPPAccountPrototype.createConversation


Expected results:

Dialog to join the Room Chat.
Thanks for letting us know!
Blocks: 954959
Component: Instant Messaging → XMPP
OS: Windows 8.1 → All
Product: Thunderbird → Chat Core
Hardware: x86_64 → All
Version: 31 → trunk
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to Nicolas Lascombes from comment #0)
> Expected results:
> 
> Dialog to join the Room Chat.

This doesn't require a dialog, we can just do what IRC does: https://dxr.mozilla.org/comm-central/source/chat/protocols/irc/ircBase.jsm#180
Assignee: nobody → a.ahmed1026
Status: NEW → ASSIGNED
Attachment #8574811 - Flags: review?(aleth)
Summary: XMPP Room Chat invitation not received → Handle XMPP Room chat invitations
Comment on attachment 8574811 [details] [diff] [review]
rev 1 - receive chat room invitation

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

Looks like it's on the right track.

::: chat/protocols/xmpp/xmpp.jsm
@@ +1016,5 @@
>        this.WARN("received presence stanza for unknown buddy " + from);
>    },
>  
> +  // Returns null if not an invitation stanza, and an object
> +  // describing the invitation otherwise

Nit: Comments that are sentences should end in a period (.)

@@ +1023,5 @@
> +      if (!x)
> +        return null;
> +      let retVal = {};
> +
> +      // XEP-0045. Direct Invitation(7.8.1)

Nit: space after Invitation.

@@ +1026,5 @@
> +
> +      // XEP-0045. Direct Invitation(7.8.1)
> +      // Described in XEP-0249
> +      let jid = x.attributes["jid"];
> +      if (jid) {

The presence of a jid attribute is not the best distinguishing feature. Check the namespace is jabber:x:conference (as required for direct invites by XEP-2049). You might have to add it to xmpp-xml.jsm.

@@ +1028,5 @@
> +      // Described in XEP-0249
> +      let jid = x.attributes["jid"];
> +      if (jid) {
> +        retVal.type = "direct";
> +        retVal.jid = this.normalize(jid);

Add a comment saying this is the jid of the chatroom, not of the user sending the invite. Maybe the property name should be mucJid to make this clear.

@@ +1032,5 @@
> +        retVal.jid = this.normalize(jid);
> +        retVal.from = this.normalize(aStanza.attributes["from"]);
> +        retVal.password = x.attributes["password"];
> +        retVal.reason = x.attributes["reason"];
> +        retVal.continue = x.attributes["jid"];

not "jid"

@@ +1037,5 @@
> +        retVal.thread = x.attributes["thread"];
> +        return retVal;
> +      }
> +
> +      // XEP-0045. Mediated Invitation(7.8.2)

Add a comment explaining what mediated invitations are (i.e. sent by the chatroom on behalf of someone in the chatroom)

@@ +1039,5 @@
> +      }
> +
> +      // XEP-0045. Mediated Invitation(7.8.2)
> +      let invite = x.getElement(["invite"]);
> +      let from = invite.attributes["from"];

This will error if invite is null.

@@ +1045,5 @@
> +        return null;
> +      retVal.type = "mediated";
> +      retVal.jid = this.normalize(aStanza.attributes["from"]);
> +      retVal.from = this.normalize(from);
> +      retVal.password = x.getElement(["password"]).innerText;

Assumes a password exists, but the spec says it is optional.

@@ +1046,5 @@
> +      retVal.type = "mediated";
> +      retVal.jid = this.normalize(aStanza.attributes["from"]);
> +      retVal.from = this.normalize(from);
> +      retVal.password = x.getElement(["password"]).innerText;
> +      retVal.reason = invite.getElement(["reason"]).innerText;

Assumes a reason element exists, but the spec says it is optional. You should be careful about this kind of thing.

@@ +1107,5 @@
> +          let room = this._parseJID(invitation.jid);
> +          let chatRoomFields = this.getChatRoomDefaultFieldValues();
> +          chatRoomFields.setValue("room", room.node);
> +          chatRoomFields.setValue("server", room.domain);
> +          chatRoomFields.setValue("nick", this._jid.node);

This looks like duplication. getChatRoomDefaultFieldValues takes a parameter, so 
let chatRoomFields = this.getChatRoomDefaultFieldValues(invitation.jid) 
should work (follow the code to make sure).

@@ +1109,5 @@
> +          chatRoomFields.setValue("room", room.node);
> +          chatRoomFields.setValue("server", room.domain);
> +          chatRoomFields.setValue("nick", this._jid.node);
> +          chatRoomFields.setValue("password", invitation.password);
> +          this.joinChat(chatRoomFields);

You probably want to display a system message in this case too, to let the user know he was invited to the room etc. Otherwise it is not clear to the user what happened.

@@ +1117,5 @@
> +          body = _("conversation.chatRoom.invitationWithReason",
> +                   invitation.from, invitation.jid, invitation.reason);
> +        else
> +          body = _("conversation.chatRoom.invitationWithoutReason",
> +                   invitation.from, invitation.jid);

These should be system messages, not normal conversation messages.

@@ +1197,2 @@
>                 .split("/", 1)[0] // up to first slash
> +               .toLowerCase() : aJID;

I'm not sure this change is a good idea, as it may hide the fact that the code assumes there is a valid jid in situations when in fact it can be null. If there are situations where the spec says a jid is optional, then we have to handle that separately and figure out what to do instead. Otherwise we may end up trying to send messages to null jids etc.

Why did you make this change? Do you have test cases where an expected jid is null?
Attachment #8574811 - Flags: review?(aleth) → feedback+
(In reply to aleth [:aleth] from comment #4)
> Why did you make this change? Do you have test cases where an expected jid
> is null?

I did that to reduce checks before calling it. just pass parameter and it handles parameter if it is falsey.
anyway, in this patch I let normalize as is and added checks before calling it if needed.
Attachment #8574811 - Attachment is obsolete: true
Attachment #8575628 - Flags: review?(aleth)
Comment on attachment 8575628 [details] [diff] [review]
rev 2 - receive chat room invitation

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

Looks almost ready!

Please file a bug for implementing service discovery: XEP-0249 says clients must announce they support direct invites, so mention that in the bug so it is not forgotten.

::: AUTHORS
@@ +7,5 @@
>  contribution to Mozilla, see http://www.mozilla.org/credits/ .
>  
>  Aaron Kaluszka <ask@swva.net>
>  Aaron Leventhal <aaronl@netscape.com>
> +Abdelrhman Ahmed <a.ahmed1026@gmail.com>

You're welcome to add your name to this file, but please file a separate bug for that. It's not related to XMPP invites, and it has to be reviewed by a peer for the module that file lives in (not me).

::: chat/locales/en-US/xmpp.properties
@@ +68,5 @@
>  chatRoomField.server=_Server
>  chatRoomField.nick=_Nick
>  chatRoomField.password=_Password
>  
> +# LOCALIZATION NOTE (chatRoomField.chatRoom.*):

Let's use conversation.muc.* or conversation.* for these strings as it's shorter.

@@ +69,5 @@
>  chatRoomField.nick=_Nick
>  chatRoomField.password=_Password
>  
> +# LOCALIZATION NOTE (chatRoomField.chatRoom.*):
> +#   These are displayed in conversation as an invitation to chat room (MUC)

These are displayed as a system message when a chatroom invitation is received.

@@ +73,5 @@
> +#   These are displayed in conversation as an invitation to chat room (MUC)
> +#   %S is the inviter.
> +#   %S is the room.
> +#   %S is the reason.
> +conversation.chatRoom.invitationWithReason=%S invites you to join %S because %S

You should use %1$S, %2$S etc so localizers can use the parameters in whatever order is required by their language.

Also, let's make this "1 has invited you to join 2: 3" because the reason will usually be a separate sentence.

::: chat/protocols/xmpp/xmpp.jsm
@@ +1028,5 @@
> +      // Described in XEP-0249.
> +      // jid (chatroom) is required.
> +      // Password, reason, continue and thread are optional.
> +      if (x.uri == Stanza.NS.conference) {
> +        if (x.attributes["jid"]) {

Use if (!x.attributes["jid"]) and you can do without the else {..}

@@ +1034,5 @@
> +          retVal.mucJid = this.normalize(x.attributes["jid"]);
> +        }
> +        else {
> +          this.WARN("Invitation is received with missing attributes: " +
> +                    aStanza.convertToString());

As the stanza is already logged, you can just do this.WARN("Received an invitation with missing MUC jid.")

@@ +1040,5 @@
> +        }
> +        retVal.type = "direct";
> +        retVal.from = this.normalize(aStanza.attributes["from"]);
> +        if (x.attributes["password"])
> +          retVal.password = x.attributes["password"];

You don't need the if() here as retVal.password = undefined is perfectly OK.

@@ +1056,5 @@
> +      // jid (chatroom) and from (inviter) are required.
> +      // password and reason are optional.
> +      if (x.uri == Stanza.NS.muc_user) {
> +        let invite = x.getElement(["invite"]);
> +        if(!invite || !invite.attributes["from"]) {

Nit: space after if

@@ +1058,5 @@
> +      if (x.uri == Stanza.NS.muc_user) {
> +        let invite = x.getElement(["invite"]);
> +        if(!invite || !invite.attributes["from"]) {
> +          this.WARN("Invitation is received with missing attributes: " +
> +                    aStanza.convertToString());

see above

@@ +1063,5 @@
> +          return null;
> +        }
> +        retVal.type = "mediated";
> +
> +        // The jid of the chatroom.

You don't need this comment as you already added the same info in a comment a few lines earlier.

@@ +1066,5 @@
> +
> +        // The jid of the chatroom.
> +        retVal.mucJid = this.normalize(aStanza.attributes["from"]);
> +
> +        // The inviter.

Same here.

@@ +1072,5 @@
> +
> +        if (x.getElement(["password"]))
> +          retVal.password = x.getElement(["password"]).innerText;
> +        if (invite.getElement(["reason"]))
> +          retVal.reason = invite.getElement(["reason"]).innerText;

As you set continue and thread for direct invites, you should set it for mediated ones too for consistency.

@@ +1132,5 @@
> +      let invitation = this.parseInvitation(aStanza);
> +      if (invitation) {
> +        if (invitation.reason)
> +          body = _("conversation.chatRoom.invitationWithReason",
> +                   invitation.from, invitation.mucJid, invitation.reason);

Nit: {..} around this as it's more than one line of code.

@@ +1150,4 @@
>        let conv = this.createConversation(norm);
>        if (!conv)
>          return;
>        conv.incomingMessage(body, aStanza, date);

This will not put a message in the correct conversation for mediated invites, and it won't be a system message for all invites. Better to add the message directly in the if (invitation) block.
Attachment #8575628 - Flags: review?(aleth) → review-
Attachment #8575628 - Attachment is obsolete: true
Attachment #8576082 - Flags: review?(aleth)
Comment on attachment 8576082 [details] [diff] [review]
rev 3 - receive chat room invitation

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

Please mention XMPP in the commit message.

::: chat/locales/en-US/xmpp.properties
@@ +73,5 @@
> +#   These are displayed as a system message when a chatroom invitation is
> +#   received.
> +#   %1$S is the inviter.
> +#   %2$S is the room.
> +#   %3$S is the reason.

add here "(a message provided by the person sending the invite)"

::: chat/protocols/xmpp/xmpp.jsm
@@ +1036,5 @@
> +        retVal.type = "direct";
> +        retVal.mucJid = this.normalize(x.attributes["jid"]);
> +        retVal.from = this.normalize(aStanza.attributes["from"]);
> +        retVal.password = x.attributes["password"];
> +        if (x.attributes["reason"])

not needed, see previous comment

@@ +1038,5 @@
> +        retVal.from = this.normalize(aStanza.attributes["from"]);
> +        retVal.password = x.attributes["password"];
> +        if (x.attributes["reason"])
> +          retVal.reason = x.attributes["reason"];
> +        if (x.attributes["continue"])

nor here

@@ +1040,5 @@
> +        if (x.attributes["reason"])
> +          retVal.reason = x.attributes["reason"];
> +        if (x.attributes["continue"])
> +          retVal.continue = x.attributes["continue"];
> +        if (x.attributes["thread"])

or here

@@ +1058,5 @@
> +        }
> +        retVal.type = "mediated";
> +        retVal.mucJid = this.normalize(aStanza.attributes["from"]);
> +        retVal.from = this.normalize(invite.attributes["from"]);
> +        retVal.continue = undefined;

That's not correct - look at the spec. You have to check for a continue element.

@@ +1143,5 @@
> +        let conv = this.createConversation(invitation.from);
> +        if (conv)
> +          conv.writeMessage(invitation.from, body, {system: true});
> +        return;
> +      }

Nit: space after this line please
Attachment #8576082 - Flags: review?(aleth) → review-
Attachment #8576082 - Attachment is obsolete: true
Attachment #8576227 - Flags: review?(aleth)
Comment on attachment 8576227 [details] [diff] [review]
rev 4 - receive chat room invitation

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

::: chat/locales/en-US/xmpp.properties
@@ +70,5 @@
>  chatRoomField.password=_Password
>  
> +# LOCALIZATION NOTE (conversation.muc.*):
> +#   These are displayed as a system message when a chatroom invitation is
> +#   received (a message provided by the person sending the invite).

The bit in brackets was supposed to go after "is the reason" ;)

::: chat/protocols/xmpp/xmpp.jsm
@@ +1037,5 @@
> +        retVal.from = this.normalize(aStanza.attributes["from"]);
> +        retVal.password = x.attributes["password"];
> +          retVal.reason = x.attributes["reason"];
> +          retVal.continue = x.attributes["continue"];
> +          retVal.thread = x.attributes["thread"];

Nit: The indentation is wrong here

@@ +1059,5 @@
> +          retVal.thread = invite.getElement(["continue"]).attributes["thread"];
> +        }
> +        else {
> +          retVal.continue = false;
> +        }

This is OK, but how about removing some more duplication by writing it as

let continue = invite.getElement(["continue"]);
retVal.continue = !!continue;
if (continue)
  retVal.thread = continue.attributes["thread"];
Attachment #8576227 - Flags: review?(aleth) → review-
(In reply to aleth [:aleth] from comment #6)
> Comment on attachment 8575628 [details] [diff] [review]
> ::: AUTHORS
> @@ +7,5 @@
> >  contribution to Mozilla, see http://www.mozilla.org/credits/ .
> >  
> >  Aaron Kaluszka <ask@swva.net>
> >  Aaron Leventhal <aaronl@netscape.com>
> > +Abdelrhman Ahmed <a.ahmed1026@gmail.com>
> 
> You're welcome to add your name to this file, but please file a separate bug
> for that. It's not related to XMPP invites, and it has to be reviewed by a
> peer for the module that file lives in (not me).

Turns out it's OK after all to add the name this way - so please put this back in the next version of your patch! :-)
Attachment #8576227 - Attachment is obsolete: true
Attachment #8576957 - Flags: review?(aleth)
Comment on attachment 8576957 [details] [diff] [review]
rev 5 - receive chat room invitation

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

Great, thanks!
Attachment #8576957 - Flags: review?(aleth) → review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in before you can comment on or make changes to this bug.