Closed Bug 67950 Opened 24 years ago Closed 22 years ago

Move the quip list into the database

Categories

(Bugzilla :: Bugzilla-General, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
Bugzilla 2.18

People

(Reporter: st.n, Assigned: davef)

References

Details

Attachments

(2 files, 5 obsolete files)

Have a look at http://lxr.mozilla.org/mozilla/source/webtools/bugzilla/buglist.cgi#1138 : # This is stupid. We really really need to move the quip list into the DB! I couldn't find a bug about this yet, so I'm filing it now.
This one doesn't seem too dificult... change buglist.cgi to read from the database, the quip addition routing to write to it and checksetup.pl to convert. How complicated should the db be? Should we record who's adding the quips and allow them to edit/change them later?
Assignee: tara → jake
Blocks: 54703
Ownership would be good if you could limit the number of quips each user could enter.
I support this. It would let us easily: (1) prevent duplicates (2) if we date or number them, remove old quips if the admin desired a date cutoff or number of quips cutoff. I suggested both of these earlier on bug #13075. I think (1) is quite important. Moving to BZ3 as this is a schema change.
Component: Bugzilla → Bugzilla 3
Target Milestone: --- → Bugzilla 3.0
D'oh!
Assignee: jake → ian
QA Contact: matty → ian
oh boy
Taking QA.
QA Contact: ian → matty
The Bugzilla 3 component is going away. We're going to depend on the Milestones for this. At the time this component was created, we didn't have milestones for Bugzilla.
Component: Bugzilla 3 → Bugzilla
Component: Bugzilla → Bugzilla-General
Priority: -- → P3
Product: Webtools → Bugzilla
Version: other → unspecified
Since I need to remember this for the b.g.o upgrade, I figure I may as well code it up so I don't have to deal with it. Will provide a patch and a description of my approach in a few days. Please note that my goal is to be a simple translation of data/comments to using the DB, not to add stuff line associating quips with users or having them expire after X days...
Attached patch patch, attempt 1 (obsolete) (deleted) — Splinter Review
Okay, here's the first run at things. Done against the 2.16 branch, so if I need to redo things to make it work with head, please holler. Tested with the b.g.o quips file, and all was well.
to patch author. i made comments on irc.
Assignee: ian → davef
Target Milestone: Bugzilla 3.0 → Bugzilla 2.18
One thing I don't see in the patch above, which I think would be nice to have, is who created what quip (discussed in comment 1)... that way, if someone is bombarding your BZ installation with innapropriate quips, you know which account to lock. I don't care so much about the editing feature... I just like the paper trail.
Target Milestone: Bugzilla 2.18 → Bugzilla 3.0
Attached patch patch, attempt 2 (obsolete) (deleted) — Splinter Review
Righto, fixes for timeless's comments. Re: comment 11, the idea about adding a track of who added what quip, it's a nice idea, but first I'm simply trying to get the quips in the db, so that really belongs in another bug, and second an anonymous user can add quips, right now - the only way to make tracking the users useful is to include an additional config option that disallows anonymous comments... which brings me back to point 1. :)
Attachment #92122 - Attachment is obsolete: true
Accidentally bumped the target milestone back up... sorry 'boot that; resetting .
Target Milestone: Bugzilla 3.0 → Bugzilla 2.18
If you're going to make that a separate bug, I would at the very least make that user tracking portion part of the schema so the fix for that bug (quip adding tracking) doesn't have to mess with db schema again. I'd still vote for that feature to go into this patch... the UI to allow/disallow anonymous quips is understandably a new/different bug, but the effort required to add it to *this* bug is minimal (modify the INSERT statement, and grab the userid of the logged in user, or insert 0 if it's an anonymous user, for now), and the effort to gain ratio is big.
Comment on attachment 92130 [details] [diff] [review] patch, attempt 2 >- $quip ||= "Bugzilla would like to put a random quip here, but nobody has entered any."; >+ >+ $quip ||= "Bugzilla would like to put a random quip here, but no one has entered any."; Please move the alternative text into the relevant template, for localisation reasons. GetQuip() would then unconditionally return a quip, and the template would check Param('usequip') before printing the quip which was returned. >+# 2002-07-19, davef@tetsubo.com, bug 67950: >+# Store quips in the db. >+$table{quips} = >+ 'qpid mediumint not null auto_increment primary key, >+ quip mediumtext not null'; >+ Please call it quipid, for consistency with other tables. Also, given that there's a bug open about "mediumtext abuse", are we sure that's the correct size of field? preed is right - we should add a field for the user who creates the quip; it's a really trivial change. >+# 2002-07-15 davef@tetsubo.com - bug 67950 >+# Move quips to the db. >+# Two states: >+# data/comments is not empty && quips table is empty: >+# Copy the data over, and suggest that the administrator remove the >+# data/comments file. >+# data/comments is not empty && quips table is not empty: >+# do nothing with the data, but warn that both exist, and need to be manually >+# merged (as we don't know which is more current). If the admin wants the >+# quips table reloaded, s/he has to DELETE FROM quips, then re-run >+# checksetup. >+# Odds are of course that the quips table is more current, as nothing will be >+# added to data/comments anymore via bugzilla. >+if (GetFieldDef("quips", "qpid")) { >+ my $s1 = $dbh->prepare("SELECT COUNT(qpid) FROM quips"); >+ $s1->execute(); >+ >+ my ($quip_count) = $s1->fetchrow_array(); >+ if (-s 'data/comments' && open (COMMENTS, "<data/comments")) { >+ if (!$quip_count) { >+ print "Populating quips table from data/comments...\n"; >+ while (<COMMENTS>) { >+ chomp; >+ $dbh->do("INSERT INTO quips (quip) VALUES (" >+ . $dbh->quote($_) . ")"); >+ } >+ print "The data/comments file (used to store quips) has been\n >+copied into the database, so you can now delete the\n >+data/comments file at your leisure. Please note that\n >+if you do not delete the script, you will recieve a\n >+warning every time you run checksetup.pl\n"; >+ } else { >+ print "The data/comments file exists, but the quips table already\n >+has entries in it. Either the file has already been copied\n >+to the database, but you have not removed the data/comments\n >+file, or something strange has happened. If you wish to\n >+reload the quips table, delete all the data in the table\n >+and re-run checksetup.pl\n"; >+ } >+ close COMMENTS; >+ } >+} >+ Better approach is to remove data/comments for them. This saves all the problems with inconsistency that you are working around here. It's a perfectly reasonable thing to do. Basically, the logic should be: if (-e data/comments) { add the contents to the quips table. delete data/comments } >- open(COMMENTS, ">>data/comments"); >- print COMMENTS $comment . "\n"; >- close(COMMENTS); >+ SendSQL("INSERT INTO quips (quip) VALUES (" . SqlQuote($comment) . ")"); And here you'd just add in $::userid. >+# GetQuip copied from buglist.cgi - duplicated 'cause it seemed silly to put in globals.pl Just remove quip support from reports.cgi. This CGI is going away in the medium term anyway, and cutting-n-pasting code is bad. Gerv
Attachment #92130 - Flags: review-
Attached patch patch, atttempt 3 (obsolete) (deleted) — Splinter Review
Okay, dealt with all of Gerv's feedback. Some things that aren't exactly as he might have wanted: The mediumtext I changed to text, as tinytext (256 chars) seems too small. You're not talking about a serious size difference between them, though (1 byte per row). That discussion probably belongs in the mediumtext abuse bug, though. I added the userid field, but didn't do anything useful with it (other that put it in the db). I'm concerned about deleting data/comments without asking the user first (which is why I didn't delete it at all), but I defer to your judgement.
Attachment #92130 - Attachment is obsolete: true
Comment on attachment 92177 [details] [diff] [review] patch, atttempt 3 Looks good to me. I disagree with Gerv (what else is new) on the point that checksetup.pl should delete data/comments; I think it should note that administrators can do it, and urge them to do so, but I think the humans should be doing things like that. r=preed Aside: I can't remember if we need 1 or 2 reviews now, but get a 2nd review on this.
Attachment #92177 - Flags: review+
Comment on attachment 92177 [details] [diff] [review] patch, atttempt 3 There are precedents for deleting out-of-date data files, I think. Anyway, it's not as if the data is lost - we put it in the DB. And in the extremely unlikely case they want the file back, they can restore it from one of their frequent backups. :-P >+$table{quips} = >+ 'quipid mediumint not null auto_increment primary key, >+ userid mediumint not null default 0, >+ quip text not null'; "text" is fine. >+# 2002-07-15 davef@tetsubo.com - bug 67950 >+# Move quips to the db. >+if (GetFieldDef("quips", "quipid")) { >+ if (-s 'data/comments' && open (COMMENTS, "<data/comments")) { Tiny nit: extra space. >- [% IF quip %] >+ [% IF Param('usequip') %] >+ [% IF quip == "" %] >+ [% quip = "Bugzilla would like to put a random quip here, but no one has entered any." %] >+ [% END %] The usual way to accomplish this is: [% IF Param('usequip') %] [% DEFAULT quip = "Bugzilla..." %] [% END %] Then, if quip isn't set, it uses the default. But, this is great. Fix those tiny issues, attach the patch and I'll check it in. If you really don't want to delete data/comments, move it to data/comments.bak and update the print statement to say that's what you've done. Then, the "migrate if data/comments exists" heuristic works fine, but the file's still around if the admin really needs it. Gerv
Attachment #92177 - Flags: review+
Attached patch patch, attempt 4 (deleted) — Splinter Review
okay, hopefully this'll do it (and I don't have any more stupid typos) - I changed it to move the file to data/comments.bak, and to warn if the file exists, so people don't forget. :) Simply a safer option, there won't be a bunch of irate bugs being filed saying "I just reloaded my database, and found out I no longer have quips..."
Attachment #92177 - Attachment is obsolete: true
Attachment #92540 - Flags: review+
Comment on attachment 92540 [details] [diff] [review] patch, attempt 4 Looks good; you'll probably get a r2 from Gerv when he checks it in, but in case you need an r ;-) r=preed
Fixed. And about time too as well - thanks Dave :-) Checking in buglist.cgi; /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v <-- buglist.cgi new revision: 1.183; previous revision: 1.182 done Checking in checksetup.pl; /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v <-- checksetup.pl new revision: 1.164; previous revision: 1.163 done Checking in quips.cgi; /cvsroot/mozilla/webtools/bugzilla/quips.cgi,v <-- quips.cgi new revision: 1.8; previous revision: 1.7 done Checking in reports.cgi; /cvsroot/mozilla/webtools/bugzilla/reports.cgi,v <-- reports.cgi new revision: 1.55; previous revision: 1.54 done Checking in template/en/default/list/list.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/list/list.html.tmpl,v <-- list.html.tmpl new revision: 1.6; previous revision: 1.5 done Gerv
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Attached patch Extra Patch (deleted) — Splinter Review
Having actually tested this, a few changes were necessary: - a 0-length data/comments is valid (I had one), so -s has to be replaced with -e. - Some formatting issues. Checking in checksetup.pl; /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v <-- checksetup.pl new revision: 1.165; previous revision: 1.164 done Gerv
I know it's my fault also for checking it in without running them, but Dave - you really must ./runtests.sh before attaching a patch. We broke the tree :-| $::userid used only once in quips.cgi. Now fixed. Gerv
Here's the history of the quips file and why i'm going to turn off quips in b.m.o. Once upon a time the set of bugzilla users was small and people added quips that were actually funny and usually were inside jokes related to the code or to mozilla hackers. Quips were fun. As time passed, the number of bugzilla users grew by an order of magnitude or four, and quips weren't much fun any more. - People added 'not funny stuff' - People whined about inappropriate content. - We had some nut case who spammed the db with thousands of "jesus saves" so that 'jesus saves' showed up most of the time instead of a joke. - People added html to their quip to play games and see how badly they could mess up bug list output. - The last straw was when Jesse added the quip worm, some javascript that copied re-entered itself into the 'add quip' form. The quips file became an arms race between bugzilla hackers/administrators and people screwing around. We fixed some of the problems by hacking bugzilla but eventually it became clear that quips were a lot more effort than they were worth. I could have turned them off in bugzilla but I came up with a solution that let bugzilla continue to display quips, but removed the ability of people to add new ones, and thus all the problems. My solution was to make the quips file be unwritable by the bugzilla user. When people ran the cgi to add new quip the script failed silently. It seemed like a new quip had been added but it had actually evaporated into the ether. There were so many quips that it would have taken weeks to run across a newly added quip anyway. this wasnt on purpose, but it worked out well. No one whined about quips additions being turned off! This was sneaky, but oh well. The solution was inelegant and hacky, but it was easy and it works. Since quips were stored in a text file it was easy to search for and delete inappropriate ones. I still have to do this from time to time. I don't like the idea of putting quips in the database. - quips are jokes. they're not part of our data set. they're not configuration data. Our db is too big already. we don't need more bloat. - deleting quips will be a pain. - updating b.m.o to use this code will let people add quips once again and restart the arms race. When its time to update b.m.o to this code i'm going to set usequip to off. Although you guys could write code to address many of my issues. I wish you wouldn't because i think its a waste of time. There are too many important things to do, and in the end, there's no way to code around the issue that people are going to add inappropriate stuff and then people (like me) are going to be forced to waste precious time playing cop and editor again. As far as i'm concernred, rearchitecting quips is a collosal waste of time. Please don't do it. We have too many real issues that need addressing. Lets concentrate on those. Oops, too late.
Some people like to have some fun while they work. While quips might not work for mozilla.org anymore (this is probably really true, because I've seen how badly they've been abused here), there's lots of small installations (and a few other big ones but not quite as big as mozilla.org) that have a lot of fun with them. Sometimes when you're in the middle of some boring triage or trying to hack on that bug that just won't go away, a good quip can give you the laugh you need to keep your mind out of the boredom and get focused again. For the record, bugzilla.gnome.org did this to their installation (moving the quips into the database) quite a long time ago. They then contributed their code in hopes of making their upgrade easier when they decided they needed to get on the 2.16 bandwagon. I'm gonna steal a quote on this issue from JayPee in IRC because he said it better: "The whole point of open source software is to scratch an itch, and if that's their itch, then by all means..." The work done here paves the way for much easier manipulation of quips (not harder), and with not a heck of a lot of additional work gives you a degree of responsibility (you can see who the screwballs are now when they goof off). And if you don't want the garbage in your database, it can be turned off. It would probably be a 4 to 5 line patch to add a "quipsclosed" or some such param that would preserve mozilla.org's current behavior, if you want to keep the existing quips viewable without adding new ones. And if you want such a thing, I'll write it myself this coming weekend if nobody beats me to it. Enough of my time is wasted already, the 5 minutes it'll take me to write said patch won't kill anything. :-) This whole thing started because of a comment Terry left in the code :-) We all do love you Dawn, if it makes your life rough, let's fix it. :) If we didn't stop and code fun stuff once in a while we'd all go nuts before we got any of the important but tedious stuff done. (oh, wait, we're all already nuts, oh well :)
Blocks: 159627
And if you'd rather just leave it off on b.m.o, that's up to you. The issues will probably get dealt with eventually anyway, because other installations want to be able to control it. Even if we don't do it on b.m.o's account, it'll probably still happen somewhere down the line.
Once the groups system gets rewritten we should add 'addquips' and 'removequips' groups.
Comment on attachment 213981 [details] [diff] [review] docs patch about 'Bugzilla Database Tables' section, for 2.18 The off-topic change that is included only makes the docs statement "Here's an overview of what each table does." worse (see https://bugzilla.mozilla.org/attachment.cgi?action=diff&id=213981&collapsed=&headers=1&context=30 for extended context). +quips: This table stores quips, the author, whether approved or not. There might be some confusion on who is approved or not. Maybe modify it to: "... stores quips, their author, and their approval status."
Attachment #213981 - Flags: review?(documentation) → review-
Attachment #213981 - Attachment is obsolete: true
Attachment #214004 - Flags: review?(documentation)
Attachment #214004 - Flags: review?(documentation) → review+
Attachment #214004 - Attachment is obsolete: true
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: