1281722 - Implement Direct MUC Invitations (XEP-0249)

Status: RESOLVED FIXED

(Chat Core :: XMPP, defect)

Comment 1

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v1 - Implement Direct MUC Invitations

I think it would be better to have one command "invite" for MUC and normal conversation, but the problem will be |helpString| as we will have two messages for each and we can't determine what the current conversation is.

Comment 2

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #1)
> I think it would be better to have one command "invite" for MUC and normal
> conversation, but the problem will be |helpString| as we will have two
> messages for each and we can't determine what the current conversation is.

What exactly fails if you register two "invite" commands, one with CONTEXT_IM and one with CONTEXT_CHAT?

Comment 3

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #2)
> What exactly fails if you register two "invite" commands, one with
> CONTEXT_IM and one with CONTEXT_CHAT?

Oh, I see we register them by name and prplId only. Never mind.

Comment 4

[aleth [:aleth]]

To clarify: /inviteto is for 1:1 conversations, but NOT for inviting additional people to that conversation (turning it into a MUC) but for inviting who you are speaking to to some room (which may or may not exist)?

Comment 5

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #4)
> To clarify: /inviteto is for 1:1 conversations, but NOT for inviting
> additional people to that conversation (turning it into a MUC) but for
> inviting who you are speaking to to some room (which may or may not exist)?

Yes.

Comment 6

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #5)
> (In reply to aleth [:aleth] from comment #4)
> > To clarify: /inviteto is for 1:1 conversations, but NOT for inviting
> > additional people to that conversation (turning it into a MUC) but for
> > inviting who you are speaking to to some room (which may or may not exist)?
> 
> Yes.

OK, then I don't think calling that /inviteto is a problem. It fits and it will be rarely used anyway.

Comment 7

[aleth [:aleth]]

Comment on attachment 8764796 [details] [diff] [review]
v1 - Implement Direct MUC Invitations

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

::: chat/locales/en-US/xmpp.properties
@@ +250,5 @@
>  command.topic=%S [<new topic>]: Set this room's topic.
>  command.ban=%S <nick>[<message>]: Ban someone from the room. You must be a room administrator to do this.
>  command.kick=%S <nick>[<message>]: Remove someone from the room. You must be a room moderator to do this.
>  command.invite=%S <jid>[<message>]: Invite a user to join the current room with an optional message.
> +command.inviteto=%S <room jid>[<password>][<message>]: Invite a user to join a room with an optional password or message.

How can we distinguish between a password and a message?

::: chat/protocols/xmpp/xmpp-commands.jsm
@@ +84,5 @@
>    return splitParams;
>  }
>  
> +// calls invite method for given aConv and aParams if jid is valid, otherwise
> +// shows a system message for invaild jid and return.

I don't think you need this comment, that's what you'd expect this command to do ;)

Instead you need a comment explaining what the expected contents of aParams is, as I am really confused by this patch.

e.g. does it always contain the MUC jid?

@@ +203,2 @@
>  
> +      return true;

A better pattern is "return invite(params, conv)"

Comment 8

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #7) 
> How can we distinguish between a password and a message?

If the password contains spaces, we will not be able to distinguish between them!

Comment 9

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #8)
> (In reply to aleth [:aleth] from comment #7) 
> > How can we distinguish between a password and a message?
> 
> If the password contains spaces, we will not be able to distinguish between
> them!

It's worse than that: The first word of any message could be a password ;)

Comment 10

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #9)
> (In reply to Abdelrhman Ahmed [:abdelrhman] from comment #8)
> > (In reply to aleth [:aleth] from comment #7) 
> > > How can we distinguish between a password and a message?
> > 
> > If the password contains spaces, we will not be able to distinguish between
> > them!
> 
> It's worse than that: The first word of any message could be a password ;)

Can we use one of them instead of both as they are optional?

Comment 11

[aleth [:aleth]]

As I suggested on IRC, I don't really think we need the message in the command (there are other obvious ways to send messages ;)

Comment 12

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v2 - Implement Direct MUC Invitations

Comment 13

[aleth [:aleth]]

Comment on attachment 8765867 [details] [diff] [review]
v2 - Implement Direct MUC Invitations

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

::: chat/locales/en-US/xmpp.properties
@@ +250,5 @@
>  command.topic=%S [<new topic>]: Set this room's topic.
>  command.ban=%S <nick>[<message>]: Ban someone from the room. You must be a room administrator to do this.
>  command.kick=%S <nick>[<message>]: Remove someone from the room. You must be a room moderator to do this.
>  command.invite=%S <jid>[<message>]: Invite a user to join the current room with an optional message.
> +command.inviteto=%S <room jid>[<password>]: Invite a user to join a room with an optional password.

Invite your conversation partner to join a room, together with its password if required.

::: chat/protocols/xmpp/xmpp-commands.jsm
@@ +83,5 @@
>      splitParams.push(msg.trimLeft());
>    return splitParams;
>  }
>  
> +// aParams is an array of command parameters and the first element

Yes but this is command-independent so you should instead say what this function expects in that array, i.e. that it should match what MUC.invite and IM.invite expect as parameters as it is just passed through. For example
// Checks the first entry in the aParams array is a valid jid, then passes it to aConv.invite().

@@ +211,5 @@
> +      let params = splitInput(aMsg);
> +      if (!params.length)
> +        return false;
> +      else if (params.length > 1) {
> +        let optionalParams = splitInput(params.pop());

why this?

::: chat/protocols/xmpp/xmpp.jsm
@@ +659,5 @@
>      this.writeMessage(who, aMsg, {outgoing: true, _alias: alias});
>      delete this._typingState;
>    },
>  
> +  // Invites a user to MUC conversation.

// Invites the contact to a MUC room.

@@ +660,5 @@
>      delete this._typingState;
>    },
>  
> +  // Invites a user to MUC conversation.
> +  invite: function(aJID, aPassword = null) {

aRoomJid, to avoid confusion with the MUC version of invite().

@@ +1785,5 @@
>        return;
>      }
>  
> +    let invitation = this.parseInvitation(aStanza);
> +    if (invitation) {

Can these invitation messages have body elements?

@@ +1796,5 @@
> +        msg += ".password";
> +      let params =
> +        [invitation.from, invitation.mucJid, invitation.password,
> +         invitation.reason].filter(s => s);
> +      body = _(msg, ...params);

Why is this still using body after the move? It's confusing as it's usually part of the stanza.

Comment 14

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v3 - Implement Direct MUC Invitations

(In reply to aleth [:aleth] from comment #13) 
> @@ +211,5 @@
> > +      let params = splitInput(aMsg);
> > +      if (!params.length)
> > +        return false;
> > +      else if (params.length > 1) {
> > +        let optionalParams = splitInput(params.pop());
> 
> why this?

Not needed. 

> Can these invitation messages have body elements?

Yes.

> Why is this still using body after the move? It's confusing as it's usually
> part of the stanza.

You are right. fixed in this patch.

Comment 15

[aleth [:aleth]]

(In reply to Abdelrhman Ahmed [:abdelrhman] from comment #14)
> > Can these invitation messages have body elements?
> 
> Yes.

What's in the body in that case?

Comment 16

[Abdelrahman Ahmed [:abdelrahman]]

(In reply to aleth [:aleth] from comment #15)
> (In reply to Abdelrhman Ahmed [:abdelrhman] from comment #14)
> > > Can these invitation messages have body elements?
> > 
> > Yes.
> 
> What's in the body in that case?

This is an example of real server (Mediated invitation)
> <message xmlns="jabber:client" from="INVITER" to="INVITEE" type="normal">
>  <x xmlns="http://jabber.org/protocol/muc#user">
>   <invite xmlns="http://jabber.org/protocol/muc#user" from="INVITER">
>    <reason xmlns="http://jabber.org/protocol/muc#user"/>
>   </invite>
>  </x>
>  <x xmlns="jabber:x:conference" jid="ROOMJID"/>
>  <body xmlns="jabber:client">
>   INVITER invites you to the room ROOMJID
>  </body>
> </message>

Comment 17

[aleth [:aleth]]

Comment on attachment 8766241 [details] [diff] [review]
v3 - Implement Direct MUC Invitations

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

::: chat/protocols/xmpp/xmpp-commands.jsm
@@ +210,5 @@
> +    usageContext: Ci.imICommand.CMD_CONTEXT_IM,
> +    run: function(aMsg, aConv) {
> +      let params = splitInput(aMsg);
> +      if (!params.length)
> +        return false;

Now this looks duplicated with "invite" so you can move it to the shared invite function.

It can help to look at the diff before posting it to the bug to spot such opportunities for deduplication which happen because the surrounding code has been improved. Sometimes it becomes obvious only when reading the changes in a different way (outside the usual code editor where you get used to seeing them).

::: chat/protocols/xmpp/xmpp.jsm
@@ +669,5 @@
> +    let attrs = {jid: aRoomJid};
> +    if (aPassword)
> +      attrs.password = aPassword;
> +
> +    let x = Stanza.node("x", Stanza.NS.conference, attrs);

We should simplify this here and in the future by

let x = Stanza.node("x", Stanza.NS..., { jid: aRoomJid, password: aPassword });

together with ensuring (with a test) that the xmpp-xml.jsm XML generator correctly ignores attributes which have null or undefined values.

I mention the second part because I suspect xmpp-xml (which currently has no tests and is quite old in parts) may behave badly in such edge cases.

Comment 18

[Abdelrahman Ahmed [:abdelrahman]]

Attachment: v4 - Implement Direct MUC Invitations

(In reply to aleth [:aleth] from comment #17)
> It can help to look at the diff before posting it to the bug to spot such
> opportunities for deduplication which happen because the surrounding code
> has been improved. Sometimes it becomes obvious only when reading the
> changes in a different way (outside the usual code editor where you get used
> to seeing them).

OK.

This patch requires patch in bug 1283139 to be landed first.

Comment 19

[Abdelrahman Ahmed [:abdelrahman]]

https://hg.mozilla.org/comm-central/rev/8ed9ff2cedd3