67950 - Move the quip list into the database

Status: RESOLVED FIXED

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

Description

[Stephan Niemz]

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.

Comment 1

[Jacob Steenhagen]

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?

Comment 2

[Brian 'netdragon' Bober]

Ownership would be good if you could limit the number of quips each user could 
enter.

Comment 3

[Matthew Tuck [:CodeMachine]]

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.

Comment 4

[Matthew Tuck [:CodeMachine]]

D'oh!

Comment 5

[Hixie (not reading bugmail)]

oh boy

Comment 6

[Matthew Tuck [:CodeMachine]]

Taking QA.

Comment 7

[Dave Miller [:justdave]]

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.

Comment 8

[davef]

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... 

Comment 9

[davef]

Attachment: patch, attempt 1

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.

Comment 10

[timeless]

to patch author. i made comments on irc.

Comment 11

[J. Paul Reed [:preed]]

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.

Comment 12

[davef]

Attachment: patch, attempt 2

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. :)

Comment 13

[J. Paul Reed [:preed]]

Accidentally bumped the target milestone back up... sorry 'boot that; resetting
.

Comment 14

[J. Paul Reed [:preed]]

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 15

[Gervase Markham [:gerv]]

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, [e-mail], 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 [e-mail] - 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

Comment 16

[davef]

Attachment: patch, atttempt 3

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.

Comment 17

[J. Paul Reed [:preed]]

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.

Comment 18

[Gervase Markham [:gerv]]

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 [e-mail] - 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

Comment 19

[davef]

Attachment: patch, attempt 4

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..."

Comment 20

[J. Paul Reed [:preed]]

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

Comment 21

[Gervase Markham [:gerv]]

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

Comment 22

[Gervase Markham [:gerv]]

Attachment: Extra Patch

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

Comment 23

[Gervase Markham [: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

Comment 24

[Dawn Endico]

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.

Comment 25

[Dave Miller [:justdave]]

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 :)

Comment 26

[Dave Miller [:justdave]]

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.

Comment 27

[Matthew Tuck [:CodeMachine]]

Once the groups system gets rewritten we should add 'addquips' and 'removequips'
groups.

Comment 28

[victory <[e-mail]>]

Attachment: docs patch about 'Bugzilla Database Tables' section, for 2.18

Comment 29

[Vlad Dascalu]

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."

Comment 30

[victory <[e-mail]>]

Attachment: docs patch about 'Bugzilla Database Tables' section, for 2.18 v2