Add invite and me commands for XMPP MUCs

RESOLVED FIXED in Instantbird 44

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: abdelrahman, Assigned: abdelrahman)

Tracking

trunk
Instantbird 44

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Add invite and me commands for XMPP MUCs
check XEP-0045 [1].
 
[1] http://xmpp.org/extensions/xep-0045.html#impl-client-irc
Assignee: nobody → a.ahmed1026
Status: NEW → ASSIGNED
Attachment #8662597 - Flags: review?(aleth)
Comment on attachment 8662597 [details] [diff] [review]
rev 1 - invite and me commands

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

::: chat/locales/en-US/xmpp.properties
@@ +73,5 @@
>  conversation.error.banKickCommandNotAllowed=You don't have the required privileges to remove this participant from the room.
>  conversation.error.banKickCommandConflict=Sorry, you can't remove yourself from the room.
> +conversation.error.inviteFailedForbidden=You don't have the required privileges to invite users to this room.
> +#   %S is the jid of user that is invited.
> +conversation.error.failedJIDNotFound=This user %S does not exist.

"Could not reach %S."

(It's possible there was some server-to-server problem.)

::: chat/protocols/xmpp/xmpp-commands.jsm
@@ +130,5 @@
> +    name: "invite",
> +    get helpString() { return _("command.invite", "invite"); },
> +    usageContext: Ci.imICommand.CMD_CONTEXT_CHAT,
> +    run: function(aMsg, aConv) {
> +      let params = aMsg.trim().split(/\s+/);

Why not use the splitInput helper function you already defined?

@@ +136,5 @@
> +        return false;
> +
> +      let conv = getConv(aConv);
> +      if (conv.left)
> +        return true;

return false;

(After all, we didn't end up inviting anyone.)

@@ +142,5 @@
> +      // Check user's jid is valid.
> +      let account = getAccount(aConv);
> +      let jid = account._parseJID(params[0]);
> +      if (!jid)
> +        return true;

Add a system message for this case.

@@ +159,5 @@
> +        return false;
> +
> +      let conv = getConv(aConv);
> +      if (conv.left)
> +        return true;

/me is like sending a normal message, so if that is tried while in a parted room it should fail like sending a normal message. I think that's what will happen if you simply remove this check.

@@ +164,5 @@
> +
> +      // XEP-0245: The /me Command.
> +      // We need to append "/me " in the first four characters of the message
> +      // body.
> +      conv.sendMsg("/me " + params);

Do we handle receiving this yet? (I didn't even know XMPP had a /me ;) )

::: chat/protocols/xmpp/xmpp.jsm
@@ +436,5 @@
>  
> +  // Invites a user to MUC conversation.
> +  invite: function(aJID, aMsg = null) {
> +    let forbidden = (aError) => {
> +      // XEP-0045 (7.8.2): Mediated Invitation.

Nit: Remove these two comments...

@@ +449,5 @@
> +      return true;
> +    };
> +    // ejabberd uses error not-allowed to indicate that this account does not
> +    // have the required privileges to invite users instead of forbidden error,
> +    // and this is not mentioned in the spec (XEP-0045).

Ah, ejabberd again! ;) Well spotted.

@@ +455,5 @@
> +                                                   notAllowed: forbidden,
> +                                                   itemNotFound: itemNotFound});
> +
> +    // XEP-0045 (7.8): Inviting Another User to a Room.
> +    // XEP-0045 (7.8.2): Mediated Invitation.

...and move this comment to the top.

@@ +1746,5 @@
>        let conv = this.createConversation(from);
>        if (conv)
>          conv.incomingMessage(null, aStanza);
>      }
> +    else if (x && x.uri == Stanza.NS.muc_user) {

Why not use the callback for this? Are there servers that don't include the id in a decline response?
Attachment #8662597 - Flags: review?(aleth) → review-
(In reply to aleth [:aleth] from comment #2)
> Do we handle receiving this yet? (I didn't even know XMPP had a /me ;) )
Yes, It's handled while preparing the message [1][2] to be printed.

> Why not use the callback for this? Are there servers that don't include the
> id in a decline response?
There is no evidence in spec (XEP-0045) states that the server responds with the same id of the invitation message, and the example that is provided uses different id for decline invitation.

[1] https://dxr.mozilla.org/comm-central/source/im/themes/messages/bubbles/main.css#108
[2] https://dxr.mozilla.org/comm-central/source/chat/modules/imThemes.jsm#378
Attachment #8662597 - Attachment is obsolete: true
Attachment #8664345 - Flags: review?(aleth)
Comment on attachment 8664345 [details] [diff] [review]
rev 2 - invite and me commands

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

Are you going to deal with the nick-with-spaces issue in another patch?

::: chat/locales/en-US/xmpp.properties
@@ +77,5 @@
> +conversation.error.inviteFailedForbidden=You don't have the required privileges to invite users to this room.
> +#   %S is the jid of user that is invited.
> +conversation.error.failedJIDNotFound=Could not reach %S.
> +#   %S is the jid that is invalid.
> +conversation.error.invalidJID=This jid %S is invaild.

%S is an invalid jid (Jabber identifiers must be of the form user@domain).

@@ +79,5 @@
> +conversation.error.failedJIDNotFound=Could not reach %S.
> +#   %S is the jid that is invalid.
> +conversation.error.invalidJID=This jid %S is invaild.
> +#   %S is the command name.
> +conversation.error.commandFailedNotInRoom=Could not execute %S command as you are no longer in the room.

You have to rejoin the room to be able to use this command.

::: chat/protocols/xmpp/xmpp-commands.jsm
@@ +95,5 @@
> +        conv.writeMessage(conv.name,
> +                          _("conversation.error.commandFailedNotInRoom", "topic"),
> +                          {system: true});
> +        return true;
> +      }

There's a ton of duplication here. How about moving the left check to getConv (getMUC?)

@@ +116,5 @@
> +                          _("conversation.error.commandFailedNotInRoom", "ban"),
> +                          {system: true});
> +        return true;
> +      }
> +      conv.ban(params[0], params.length > 1 ? params[1].trim() : null);

Why is this needed? params[1] looks like it's already trimmed in splitInput and if it doesn't exist it just passes undefined as an argument.

::: chat/protocols/xmpp/xmpp.jsm
@@ +1771,5 @@
>        let conv = this.createConversation(from);
>        if (conv)
>          conv.incomingMessage(null, aStanza);
>      }
> +    else if (x && x.uri == Stanza.NS.muc_user) {

Move what follows to a function on the MUC object.
Attachment #8664345 - Flags: review?(aleth) → review-
(In reply to aleth [:aleth] from comment #4)
> Are you going to deal with the nick-with-spaces issue in another patch?
Yes.

> Why is this needed? params[1] looks like it's already trimmed in splitInput
> and if it doesn't exist it just passes undefined as an argument.
I just handled case of second element in return array by using |trimLeft|.
Attachment #8664345 - Attachment is obsolete: true
Attachment #8664478 - Flags: review?(aleth)
Comment on attachment 8664478 [details] [diff] [review]
rev 3 - invite and me commands

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

Looks much better!

::: chat/protocols/xmpp/xmpp-commands.jsm
@@ +119,5 @@
>  
> +      let conv = getMUC(aConv);
> +      if (!conv)
> +        return true;
> +      conv.ban(...params);

if (conv)
  conv.ban(...params);

saves a line.

@@ +195,5 @@
>  
> +      let conv = getMUC(aConv);
> +      if (!conv)
> +        return true;
> +      conv.setNick(params[0]);

See above

::: chat/protocols/xmpp/xmpp.jsm
@@ +446,5 @@
> +    let itemNotFound = (aError) => {
> +      let message = _("conversation.error.failedJIDNotFound", aJID);
> +      this.writeMessage(this.name, message, {system: true, error: true});
> +      return true;
> +    };

You can simplify this now using https://hg.mozilla.org/comm-central/rev/a5679881ec4c
Attachment #8664478 - Flags: review?(aleth) → review-
Attachment #8664478 - Attachment is obsolete: true
Attachment #8666886 - Flags: review?(aleth)
Comment on attachment 8666886 [details] [diff] [review]
rev 4 - invite and me commands

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

Thanks!
Attachment #8666886 - Flags: review?(aleth) → review+
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Instantbird 44
You need to log in before you can comment on or make changes to this bug.