Add ban, kick, invite, msg, nick commands for XMPP MUCs check XEP-0045 [1] for syntax of them in XMPP. [1] http://xmpp.org/extensions/xep-0045.html#impl-client-irc

Comment on attachment 8654121 [details] [diff] [review] rev 1 - ban and kick commands Review of attachment 8654121 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/locales/en-US/xmpp.properties @@ +66,5 @@ > # %2$S is the text of the message that wasn't delivered. > conversation.error.sendFailedAsRecipientNotInRoom=Message could not be sent to %1$S as the recipient is no longer in the room: %2$S > # These are displayed in a conversation as a system error message. > conversation.error.remoteServerNotFound=Could not reach the recipient's server > +conversation.error.banKickCommandUnavailable=Could not perform command as service for this command is not available at this moment. Which service? (When does this happen?) @@ +67,5 @@ > conversation.error.sendFailedAsRecipientNotInRoom=Message could not be sent to %1$S as the recipient is no longer in the room: %2$S > # These are displayed in a conversation as a system error message. > conversation.error.remoteServerNotFound=Could not reach the recipient's server > +conversation.error.banKickCommandUnavailable=Could not perform command as service for this command is not available at this moment. > +conversation.error.banKickCommandNotAllowed=Could not perform command as this participant has higher affiliation. "You don't have the required privileges to remove this participant from the room." @@ +68,5 @@ > # These are displayed in a conversation as a system error message. > conversation.error.remoteServerNotFound=Could not reach the recipient's server > +conversation.error.banKickCommandUnavailable=Could not perform command as service for this command is not available at this moment. > +conversation.error.banKickCommandNotAllowed=Could not perform command as this participant has higher affiliation. > +conversation.error.banKickCommandConflict=Could not perform command as service declines kicking yourself from the room. "Sorry, you can't ban yourself from the room." @@ +209,5 @@ > # These are the help messages for each command. > command.join3=%S [&lt;room&gt;[@&lt;server&gt;][/&lt;nick&gt;]] [&lt;password&gt;]: Join a room, optionally providing a different server, or nickname, or the room password. > command.part2=%S [&lt;message&gt;]: Leave the current room with an optional message. > command.topic=%S [&lt;new topic&gt;]: Set this room's topic. > +command.ban=%S &lt;nick&gt;[&lt;message&gt;]: Ban a participant with that nickname from the current room with an optional message. "Ban someone from the room. You must be a room administrator to do this." @@ +210,5 @@ > command.join3=%S [&lt;room&gt;[@&lt;server&gt;][/&lt;nick&gt;]] [&lt;password&gt;]: Join a room, optionally providing a different server, or nickname, or the room password. > command.part2=%S [&lt;message&gt;]: Leave the current room with an optional message. > command.topic=%S [&lt;new topic&gt;]: Set this room's topic. > +command.ban=%S &lt;nick&gt;[&lt;message&gt;]: Ban a participant with that nickname from the current room with an optional message. > +command.kick=%S &lt;nick&gt;[&lt;message&gt;]: Kick a participant with that nickname from the current room with an optional message. "Remove someone from the room. You must be a room moderator to do this." ::: chat/protocols/xmpp/xmpp-commands.jsm @@ +78,5 @@ > + name: "ban", > + get helpString() _("command.ban", "ban"), > + usageContext: Ci.imICommand.CMD_CONTEXT_CHAT, > + run: function(aMsg, aConv) { > + let params = aMsg.trim().split(/\s+/); Won't this mess up the message? @@ +93,5 @@ > + name: "kick", > + get helpString() _("command.kick", "kick"), > + usageContext: Ci.imICommand.CMD_CONTEXT_CHAT, > + run: function(aMsg, aConv) { > + let params = aMsg.trim().split(/\s+/); Same here. ::: chat/protocols/xmpp/xmpp.jsm @@ +439,5 @@ > + // Bans a participant from MUC conversation. > + ban: function(aNickName, aMsg = null) { > + // XEP-0045 (9.1): Banning a User. > + let participant = this._participants.get(aNickName); > + if (!participant || !participant.accountJid) { Two separate error messages would be better here: "<nick> is not in the room." "You can't ban participants from anonymous rooms. Try /kick instead." @@ +482,5 @@ > + break; > + default: > + this.WARN("Unhandled error as a result of performing ban or kick" + > + "command: " + aStanza.convertToString()); > + break; You can just return false and there should be an automatic warning. @@ +484,5 @@ > + this.WARN("Unhandled error as a result of performing ban or kick" + > + "command: " + aStanza.convertToString()); > + break; > + } > + if (message) Not needed if you return false early. @@ +487,5 @@ > + } > + if (message) > + this.writeMessage(this.name, message, {system: true}); > + } > + return true; Do you need to check type==result here? ::: im/content/credits.xhtml @@ +188,5 @@ > </div> > > <div class="creditsGroup"> > <h3>&credit.contributors;</h3> > + <div class="credit">Abdelrhman Ahmed</div> Separate bug please ;)

(In reply to aleth [:aleth] from comment #2) > Which service? (When does this happen?) I used this for testing to check that messages follow the spec to perform commands and commands are executed well. > Won't this mess up the message? Yes, you are right if the user provides a message includes white-spaces. > Do you need to check type==result here? I think we expect two responses of type (error or result). I'm not sure if service can respond with different type (I did not see that in spec). BTW, I added the check to be able to catch responses that we do not handle. > Separate bug please ;) OK, sorry for that.

Comment on attachment 8656284 [details] [diff] [review] rev 2 - ban and kick commands Review of attachment 8656284 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp-commands.jsm @@ +22,5 @@ > return getConv(aConv)._account; > } > > +// Return array of split string on the first white-space if exists, or string. > +// aString parameter should not be empty. It's better if you handle the case when it's empty, or just whitespace. You can return an empty array. @@ +25,5 @@ > +// Return array of split string on the first white-space if exists, or string. > +// aString parameter should not be empty. > +function splitInput(aString) { > + let params = []; > + let offset = aMsg.indexOf(" "); You haven't tested this, have you? ;) It's a goood idea to trimLeft and trimRight before splitting to avoid accidental whitespaces. @@ +96,5 @@ > + name: "ban", > + get helpString() _("command.ban", "ban"), > + usageContext: Ci.imICommand.CMD_CONTEXT_CHAT, > + run: function(aMsg, aConv) { > + if (!aMsg.length) It's better to check this after a trim(). If you do that in splitInput, you can do if !params.length.

Comment on attachment 8658333 [details] [diff] [review] rev 3 - ban and kick commands Review of attachment 8658333 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/xmpp/xmpp-commands.jsm @@ +97,5 @@ > } > + }, > + { > + name: "ban", > + get helpString() _("command.ban", "ban"), Where is this string defined? @@ +112,5 @@ > + } > + }, > + { > + name: "kick", > + get helpString() _("command.kick", "kick"), Missing string?

(In reply to aleth [:aleth] from comment #6) > Where is this string defined? > Missing string? Sorry for that, there was a problem in patch and I had to copy it.

https://hg.mozilla.org/comm-central/rev/d883ab3d5cee57c89e16e49aa1c975f7188bb159 Bug 1172354 - Implement ban and kick commands for XMPP rooms. r=aleth a=aleth CLOSED TREE