96431 - It's possible to write an essay in the Summary field.

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect, P2)

Description

[Jacob Steenhagen]

This is a spinoff of bug 92266.  Copying some info from a comment by Dave:
-----
<...> I was disturbed however, to discover that the summary and 
status_whiteboard fields on bugs are both mediumints in the database (which is 
32K), and no validation is performed within Bugzilla to make sure they fit 
anything.  <...>  The summary/status_whiteboard issue is probably good for 
another bug.  It's infrequent enough (22 bugs affected out of 96000 on b.m.o 
according to Myk) that it's probably not a rush issue. <...>
-----

This is intended to be the aformentioned "other bug".

PS, bug 92266 fixed the issue regarding dataloss on long summaries, but the 
summaries just shouldn't be that long (and neither should status_whiteboard).

Comment 1

[Dave Miller [:justdave]]

Just FYI, I meant "mediumtext" there, not "mediumint" :)

Comment 2

[Matthew Tuck [:CodeMachine]]

Here's my suggestion.

checksetup.pl attempts to reduce the size of the field to 256 characters. 
However, if it can't do so without truncating summaries, it will complain and
otherwise continue.

This way we don't annihilate any summaries, yet we pester the admin.

Comment 3

[Dave Miller [:justdave]]

We are currently trying to wrap up Bugzilla 2.16.  We are now close enough to
release time that anything that wasn't already ranked at P1 isn't going to make
the cut.  Thus this is being retargetted at 2.18.  If you strongly disagree with
this retargetting, please comment, however, be aware that we only have about 2
weeks left to review and test anything at this point, and we intend to devote
this time to the remaining bugs that were designated as release blockers.

Comment 4

[Alex Vincent [:WeirdAl]]

Adding URL which proves the point.

Comment 5

[Dave Miller [:justdave]]

*** Bug 129482 has been marked as a duplicate of this bug. ***

Comment 6

[Joel Peshkin]

These unloved bugs have been sitting untouched since June 2002 or longer.  If
nobody does anything else to them, they certainly won't make 2.18
Retargetting to 2.20.  If you really plan to push them right now, you might pull
them back in.

Comment 7

[timeless]

Attachment: shorten the field or complain about not being able to shorten it

Comment 8

[timeless]

Comment on attachment 144339 [details] [diff] [review]
shorten the field or complain about not being able to shorten it

this needs testing, the code should be right, but i don't have a database or
any experience hacking this stuff.

Comment 9

[Vlad Dascalu]

Comment on attachment 144339 [details] [diff] [review]
shorten the field or complain about not being able to shorten it

Commiting this on my localhost makes checksetup.pl die with:

Can't find string terminator "EOF" anywhere before EOF at ./checksetup.pl line
4276.

I think you've added "EOF;" instead of "EOF" or something like that:

+EOF;


Also, your SQL is invalid. I think you need an extraspace here, after the WHERE
statement:

+    $sth = $dbh->prepare("SELECT bug_id FROM bugs WHERE" .
+			  "length(short_desc) >= 256");



Nitpicks that you can ignore:

After the "Following may be a list:" and the actual list, we could add:

"You need to trim those summaries manually to be shorter, then rerun
checksetup.pl to update your DB configuration." or something like that.

Or maybe trim them by default and modify the message accordingly.


Also, what will happen when a user will insert an essay in the summary field?
Shouldn't he get an warning: "Warning: your summary was too long and it was
trimmed to 255 chars." or something like that?

Comment 10

[timeless]

Attachment: shorten the field or complain about not being able to shorten it; ask client to limit input to 255 chars

Comment 11

[Vlad Dascalu]

Comment on attachment 144441 [details] [diff] [review]
shorten the field or complain about not being able to shorten it; ask client to limit input to 255 chars

 }
+# 2004/03/19 - Summaries shouldn't be essays - see bug 96431

You should leave a newline between } and the # line, for consistency's sake
with the previous entries.

+	      maxlenth="255">

It's "maxlength" :-)

r=vladd with those changes

Comment 12

[Vlad Dascalu]

Oh, and you can leave a newline after the "DB configuration." line as well.

Comment 13

[Dave Miller [:justdave]]

Comment on attachment 144441 [details] [diff] [review]
shorten the field or complain about not being able to shorten it; ask client to limit input to 255 chars

>-             value="[% bug.short_desc FILTER html %]" size="60">
>+             value="[% bug.short_desc FILTER html %]" size="60"
>+             maxlenth="255">

I have reservations about this...  in most browsers I expect this will cause
the existing value to get truncated if you touch the bug (and probably won't
let you edit it).  Since we're making the change in checksetup fail until they
manually fix the existing ones, we should probably make the maxlength="255"
conditional on whether the existing summary is already that short or not.  You
have maxlength spelled wrong, btw.

Comment 14

[Max Kanat-Alexander]

How many summaries are there on b.m.o that are larger than 255 characters?

But anyhow, I suppose it's true, we do need to not input that maxlength param if
the summary is already longer than 255 characters. It shouldn't be too hard,
it's just a conditional on the length of the summary var.

Comment 15

[Frédéric Buclin]

timeless, are you still working on it??

Comment 16

[Max Kanat-Alexander]

This bug has not been touched by its owner in over six months, even though it is
targeted to 2.20, for which the freeze is 10 days away. Unsetting the target
milestone, on the assumption that nobody is actually working on it or has any
plans to soon.

If you are the owner, and you plan to work on the bug, please give it a real
target milestone. If you are the owner, and you do *not* plan to work on it,
please reassign it to [e-mail] or a .bugs component owner. If you are
*anybody*, and you get this comment, and *you* plan to work on the bug, please
reassign it to yourself if you have the ability.

Comment 17

[Max Kanat-Alexander]

Attachment: v3: Move overly-long summaries into Whiteboard

For this patch, if a summary is longer than 255 characters, it gets moved into the Status Whiteboard.

Asking the user to use Bugzilla in the *middle* of a checksetup run to fix the summaries probably is not a good idea, since their Bugzilla may not even be operational yet.

I couldn't just add the Summary as a comment, because I don't have a "user" to write the comment "from."

Comment 18

[Alex Vincent [:WeirdAl]]

Comment on attachment 206529 [details] [diff] [review]
v3: Move overly-long summaries into Whiteboard

>+    # Move extremely long summarries into the Status Whiteboard,

Nit:  summaries.

Comment 19

[Kevin Benton]

Comment on attachment 206529 [details] [diff] [review]
v3: Move overly-long summaries into Whiteboard

After a discussion with Max on the IRC regarding this implementation, this gets an r-.

It seems that these comments really belong in longdescs rather than the status_whiteboard field.  The user who made the change is actually available by looking at the reporter or the activity log to see where the summary was set to a value too long for the varchar(255) field.  We also talked about possibly chopping off the summary at the last word ending at or before the 252nd character then appending ... (elipsis) at the end to let users know that there's more to this summary but it wouldn't fit.  Since the summary really isn't an indication of status, I suggest using longdescs over status_whiteboard to handle the difference here.

Comment 20

[Max Kanat-Alexander]

Attachment: v4: Make the old summary a comment

Okay, now we put the old summary into longdescs. I just used the reporter -- I don't think we need to get too picky with who wrote the summary.

Comment 21

[Max Kanat-Alexander]

(In reply to comment #18)
> Nit:  summaries.

  Oh, thanks! :-) I'll have to fix that on checkin, if the latest patch gets r+.

Comment 22

[Frédéric Buclin]

Comment on attachment 206535 [details] [diff] [review]
v4: Make the old summary a comment

I haven't tested it, but it looks good.

Comment 23

[Frédéric Buclin]

Comment on attachment 206535 [details] [diff] [review]
v4: Make the old summary a comment

>Index: checksetup.pl

>+WARNING: Some of your bugs had summaries longer than 255 characters.
>+They have had their original summary copied into a comment, and then
>+the summary was truncated to 255 characters. The affected bug numbers were:

Nit: why not: "The affected bug numbers *are*:"?


As discussed on IRC with mkanat, we don't want to add an entry in the bugs_activity table when truncating a bug summary because we don't want to spam people about it. Moreover, we never update the bugs_activity table when renaming a product or a version from admin pages, for instance. If someone ever complains, we will fix that (maybe) in another bug.

r=LpSolit

Comment 24

[Frédéric Buclin]

Attachment: unbitrotten patch

Unbitrotten patch (which I used to test mkanat's patch)

Comment 25

[Myk Melez [:myk] [@mykmelez]]

This change needs to be prominently featured in the release notes.

Comment 26

[Max Kanat-Alexander]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.478; previous revision: 1.477
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.50; previous revision: 1.49
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.76; previous revision: 1.75
done
Checking in template/en/default/bug/create/create-guided.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create-guided.html.tmpl,v  <--  create-guided.html.tmpl
new revision: 1.29; previous revision: 1.28
done
Checking in template/en/default/bug/create/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/create/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.57; previous revision: 1.56
done

Comment 27

[Max Kanat-Alexander]

Added to the release notes as part of bug 349423.