1105728 - IRC /umode doesn't set user mode

Status: RESOLVED DUPLICATE of bug 1105660

(Chat Core :: IRC, defect)

Description

[momiga]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:33.0) Gecko/20100101 Firefox/33.0
Build ID: 20141016201439

Steps to reproduce:

Run "/umode -x" in the server window.


Actual results:

Server replies "-x is not online."


Expected results:

Server sets usermode to -x.

This is as according to "/help umode".

>umode (+|-)<new mode>: Set or unset a user mode.

Comment 1

[momiga]

Attachment: Patch to fix /umode.

This fixed /umode to exhibit the appropriate behavior.

It also allows the user mode to be queried by using it without arguments, i.e. "/umode".

Comment 2

[Patrick Cloke [:clokep]]

To add more information, this is because we're sending the mode directly as "MODE -x" instead of "MODE <your nick> -x".

Comment 3

[Patrick Cloke [:clokep]]

Comment on attachment 8529750 [details] [diff] [review]
Patch to fix /umode.

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

This looks correct, but there should be an easier way to get the nick. I'd also like to see tests written against this, it should be fairly easy to write tests for this code. Thanks! (http://mxr.mozilla.org/comm-central/source/chat/protocols/irc/test/test_ircMessage.js might be useful)

::: chat/protocols/irc/ircCommands.jsm
@@ +430,5 @@
>      name: "umode",
>      get helpString() _("command.umode", "umode"),
> +    run: function(aMsg, aConv) {
> +      let params = aMsg ? splitInput(aMsg) : [];
> +      params.unshift(getAccount(aConv)._nickname);

It looks like you should be able to unshift aConv.nick as in http://mxr.mozilla.org/comm-central/source/chat/protocols/irc/ircCommands.jsm#288

Comment 4

[momiga]

(In reply to Patrick Cloke [:clokep] from comment #3)
> It looks like you should be able to unshift aConv.nick as in
> http://mxr.mozilla.org/comm-central/source/chat/protocols/irc/ircCommands.
> jsm#288

It turns out that doesn't work because there is no field called nick. In fact, that's likely what was causing the exceptions in bug #1105660. Looking through the code, it seemed the only working way to get the current user's nick was the _nickname field.

I'll write some tests and add a patch for them tomorrow. Thanks!

Comment 5

[Patrick Cloke [:clokep]]

Comment on attachment 8529750 [details] [diff] [review]
Patch to fix /umode.

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

(In reply to momiga from comment #4)
> It turns out that doesn't work because there is no field called nick. In
> fact, that's likely what was causing the exceptions in bug #1105660. Looking
> through the code, it seemed the only working way to get the current user's
> nick was the _nickname field.
Ah, yeah it looks like throughout that file we use account._nickname. 

> I'll write some tests and add a patch for them tomorrow. Thanks!
Cool. You can attach those in the same patch or a separate one, whichever is easier.

Comment 6

[Patrick Cloke [:clokep]]

Removing assignment.

Comment 7

[Pavan Karthik [:matrixisreal]]

Hi,
I have started working on this,
Regarding the tests which need to be added, What exactly do we need to test for?
Thanks. :)

Comment 8

[Patrick Cloke [:clokep]]

The goal of the test would be to ensure that when the command runs the proper messages are sent to the server.

You'll want to test this with both aMsg being set and not set.

Comment 9

[Pavan Karthik [:matrixisreal]]

Attachment: v1.patch

I have just added few tests to what momiga has done.
But I'm still not sure about the use case of /umode command.
From your previous discussions and the current patch, I have assumed that umode doesnt accept nick(other users) as an argument.
so helpstring would be /umode (+|-)<newmodes>

Comment 10

[Patrick Cloke [:clokep]]

Comment on attachment 8839465 [details] [diff] [review]
v1.patch

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

(In reply to Pavan Karthik [:matrixisreal] from comment #9)
> But I'm still not sure about the use case of /umode command.
This is for changing your overall usermode on the server.

> From your previous discussions and the current patch, I have assumed that
> umode doesnt accept nick(other users) as an argument.
> so helpstring would be /umode (+|-)<newmodes>
It does not accept a nickname because it's for changing YOUR mode. I think this matches the help string we currently have, no?

Your tests look good overall! (Good job figuring out how to run commands, I wasn't fully sure how to test that myself.) Most of the comments I left are style things.

::: chat/protocols/irc/test/test_ircCommands.js
@@ +56,5 @@
> +  let conv = account.getConversation("newconv");
> +
> +  account.sendRawMessage = (aMessage) => {
> +    console.log(aMessage);
> +    do_check_eq(aMessage, testData[account._nickname]["expectedMessage"]);

Please use the newer equal() method from Assert.jsm: https://developer.mozilla.org/en/docs/Mozilla/JavaScript_code_modules/Assert.jsm#equal()

@@ +62,5 @@
> +
> +  // get the run command of umode.
> +  for (let command of commands) {
> +    if (command["name"] == "umode") {
> +      var runUserModeCommand = command["run"];

You can "break;" after you find this.

You should probably set runUserModeCommand to null before this loop, find it inside the loop and check after the loop to ensure it's not null (if it is, you'll want to throw an error).

@@ +67,5 @@
> +    }
> +  }
> +
> +  // change the nick and runUserModeCommand
> +  for (let test in testData) {

It doesn't look like you're really using the value of 'test' here, I suspect testData should really be an Array of Objects instead of an Object of Objects.

@@ +68,5 @@
> +  }
> +
> +  // change the nick and runUserModeCommand
> +  for (let test in testData) {
> +    account._nickname = testData[test]["nick"];

Instead of doing weird things with the nick, you can just set account._expectedMessage = testData[test].expectedMessage and then assert on that higher up.

@@ +70,5 @@
> +  // change the nick and runUserModeCommand
> +  for (let test in testData) {
> +    account._nickname = testData[test]["nick"];
> +    let msg = testData[test]["msg"];
> +    runUserModeCommand(msg, conv);

No reason to store 'msg' as a separate variable here.

::: chat/protocols/irc/test/xpcshell.ini
@@ +13,5 @@
>  [test_sendBufferedCommand.js]
>  [test_setMode.js]
>  [test_splitLongMessages.js]
>  [test_tryNewNick.js]
> +[test_ircCommands.js]

We usually keep these in alphabetical order!

Comment 11

[Pavan Karthik [:matrixisreal]]

Attachment: v1.1.patch

here it goes . :)

Is the testData OK?
I didnt find documentation about umode in internet .

Comment 12

[Patrick Cloke [:clokep]]

This was fixed at the same time as bug 1105660.