955677 - Nick truncated if already in use during connection registration

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Florian Quèze [:florian]]

*** Original post on bio 2229 at 2013-10-24 16:02:00 UTC ***

clokep told me to file a bug ;)

Comment 1

[Patrick Cloke [:clokep]]

We fallback to the default maximum length of an IRC nick (8 characters) if we're told a nick is invalid during connection registration (I don't think we're told WHY it is, just that it is). E.g. "flo-retina changed to flo-reti1 if flo-retina was in use".

Comment 2

[Patrick Cloke [:clokep]]

Attachment: Patch v1

This changes from trying to anticipate when we'd send too long of a nick to just sending one and seeing when the nick returned to us is shorter than the nick that we sent.

Comment 3

[aleth [:aleth]]

Comment on attachment 8474141 [details] [diff] [review]
Patch v1

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

Thanks for fixing this!

I'm glad this has tests already.

::: chat/protocols/irc/irc.js
@@ +1235,5 @@
> +    // nickname with the returned nickname and check for truncation.
> +    if (aOldNick.length < this._sentNickname.length) {
> +      // The nick will be too long, overwrite the end of the nick instead of
> +      // appending.
> +      print("FOOBAR");

Possibly, but I suspect all these prints are superfluous to requirements ;)

::: chat/protocols/irc/test/test_tryNewNick.js
@@ +50,5 @@
> +}
> +
> +// This tests a bunch of cases near the max length by maintaining the state
> +// through a series of test nicks.
> +function test_maxLength() {

I don't think this test is being run, which is a good way to make it pass.

Comment 4

[Patrick Cloke [:clokep]]

Attachment: Patch v2

I had to modify the tests a bit to pass, but I'm fairly certain they were mistakes in the tests though. Please make sure the tests make sense!

Comment 5

[aleth [:aleth]]

Comment on attachment 8475620 [details] [diff] [review]
Patch v2

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

Looks good!

::: chat/protocols/irc/test/test_tryNewNick.js
@@ +28,5 @@
>      "clo1kep01": "clo1kep02",
>      "clo1kep09": "clo1kep10",
>  
>      // Some to test the max length.
> +    //"abcdefgh9": "abcdefgh10",

With a maxlength of 9, naively this should not work. Comment? Or are these obsolete now due to the separate test?

Comment 6

[Patrick Cloke [:clokep]]

Attachment: Patch v2.1

Correct, those tests were replaced. Forgot to remove them, sorry!

Comment 7

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/13eadd28a2db