Closed
Bug 955677
Opened 10 years ago
Closed 10 years ago
Nick truncated if already in use during connection registration
Categories
(Chat Core :: IRC, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
1.6
People
(Reporter: florian, Assigned: clokep)
References
()
Details
Attachments
(1 file, 2 obsolete files)
11.23 KB,
patch
|
clokep
:
review+
|
Details | Diff | Splinter Review |
*** Original post on bio 2229 at 2013-10-24 16:02:00 UTC *** clokep told me to file a bug ;)
Reporter | ||
Updated•10 years ago
|
Summary: Nick flo-retina changed to flo-reti1 if flo-retina was in used → Nick flo-retina changed to flo-reti1 if flo-retina was in use
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Comment 1•10 years ago
|
||
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".
Summary: Nick flo-retina changed to flo-reti1 if flo-retina was in use → Nick truncated if already in use during connection registration
Assignee | ||
Comment 2•10 years ago
|
||
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•10 years ago
|
||
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.
Attachment #8474141 -
Flags: review?(aleth) → review-
Assignee | ||
Comment 4•10 years ago
|
||
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!
Attachment #8474141 -
Attachment is obsolete: true
Attachment #8475620 -
Flags: review?(aleth)
Comment 5•10 years ago
|
||
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?
Attachment #8475620 -
Flags: review?(aleth) → review+
Assignee | ||
Comment 6•10 years ago
|
||
Correct, those tests were replaced. Forgot to remove them, sorry!
Attachment #8475620 -
Attachment is obsolete: true
Attachment #8475832 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/comm-central/rev/13eadd28a2db
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
You need to log in
before you can comment on or make changes to this bug.
Description
•