Similar to general summary reports (bug #12282), summary charts would make the current summary charts more flexible so you could use a general querying mechanism to produce a chart.

I've realised that a general querying mechanism to this is probably next to impossible, due to the fact it needs historical data, but it is still worthwhile to discuss what sort of information might be historically recorded to get the most benefit for the least amount of space.

tara@tequilarista.org is the new owner of Bugzilla and Bonsai. (For details, see my posting in netscape.public.mozilla.webtools, news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)

Well, we do store historical data in "View Bug Activity", so it might be possible, but probably really difficult. See bug #6682 for something that provides a subset of this functionality.

Here's a plan how this could be done: 1. Set up a second instance of bugzilla _without_ an http server, and _without_ a working sendmail configuration. 2. Tweak it so that you can define an arbitrary timestamp as "now". 3. Populate it with a snapshot of the original bugzilla. If the comments don't matter, it's probably sufficient to copy each bug's activity log together with the current values of the fields tracked in that activity log, plus the creation date and the reporter. If the "mirror" bugzilla is an "external" one, the information can be collected with a single large buglist query, plus a call to show_activity.cgi for each bug (afterwards). Choose a value that for the "current time" (T) that is less than the timestamp of the buglist query. For example, T could be set to 0:00 of today. The "current state" (i.e. value for field X) then can be computed as follows: - If there are recorded changes of field X in the activity log with a timestamp _before_ T, then use the "new" value of the last such change. - If there are recorded changes of field X in the activity log with a timestamp _after_ T, then use the "old" value of the first sucht change. - If there aren't any recorded changes of field X in the activity log, then then use the value from the buglist query. 4. Set everything up so that collectstats will do the queries you want. 5. Repeat 5.1) Run collectstats.pl 5.2) Set the new time T := T - 1 day 5.3) Undo all changes from the activity log dated later than T, and delete them from the activity log. If you are running with full comments etc., also remove all comments, attachments, bugs etc. with creation dates later than T. Until the data tables are all empty, or T is far enough in the past. 6. Sort the files in data/mining so that the order of the lines is corrected (by reversing all lines). 6.a. (optional) Merge the new files with the ones from the original Bugzilla. 7. Generate charts from the files as usual. One possible usage scenario could be this: collect new queries during weekdays, and execute them all in one go over the weekend. The second Bugzilla instance could run on a different machine than the first one. I won't implement this soon, and probably I won't ever have time for this, but maybe someone else will pick it up, and of course I'd like to know if the above p lan would work or not

Adding default QA contact to all open Webtools/Bugzilla bugs lacking one. Sorry for the spam.

Adding Gerv since he's planning on extending the graphing mechanism. :) See his post in .webtools: news://news.mozilla.org:119/3B388EB9.9C09B7D%40univ.ox.ac.uk The approach I described above is more generic since it can generate data that hasn't been "collected" in the past, but probably it's less practicable. Gerv, please post a link to your bug here, once there is one.

This bug will probably depend on Gerv's work.

I don't have a bug for my work yet. I think that what I propose will cover the 99% case for this - because the administrator can define charts at will for things that need tracking. OK, so you won't get it unless you explicitly define it, but that just requires a bit of planning. However, thoughts on how I could architect this to fit in with a totally generic mechanism like this one (if ever implemented) would be welcome. Gerv

It'd be really hard for this to be retrospective. There are at least two examples that I know of where we had corruption on the bugs activity table.

The only retrospectivity will be that the default queries will match the current hard-coded ones and the data from the data files will be read into the DB. We don't have to mess with bugs_activity. Gerv

Moving to new Bugzilla product ....

I'm fixing this along with bug 105491. I'm blocked on the checkin of the templatised query.cgi. Gerv

Gerv, could you estimate the amount of time it would take you or another Bugzilla hacker to implement this feature?

Myk: this one's a bit harder. Basically, you need to - work out how you are going to store the queries in the DB - work out how you are going to offer the user a choice among a large number of queries without UI problems - implement the two above things - migrate from old format to new DB tables - get collectstats.pl to collect new data in new DB tables - Produce interface for choosing and showing charts of arbitrary numbers of queries - work out how the interface for defining charts will work - implement that I've implemented all but the last two bullets of this, but was waiting for query.cgi's templatisation to get checked in before progressing - because it's parts of query.cgi which will be used to define the charts. Gerv

Gerv, how's this going. Shouldn't it be moved to 2.18?

Let's leave it at Future for the moment :-) I do have some code, but it's not ready. Gerv

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

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

Accepting. This work has been estimated at 20 hours, of which I have done 2.25 so far. It may be finished by the end of the year. Gerv

In the spirit of "Bugzilla as backup mechanism", and just to show I'm not idle, here's a 73k patch implementing http://www.mozilla.org.uk/temp/graph/ . It's not quite ready for review - there are a few loose ends to tie up - and the obvious next move is to get it up on landfill and wait a few days for it to acquire some data to play with. But here it is. Gerv

A few questions about what you have so far.... 1) In series_data, do you really want to include the value in the unique key? 2) Can we not forget who created and "owns" a public query? Pwerhaps use something other than zeroing the creator to make a query public? Naturally, that would mean that the searches get run with a less-priviledged userid other than the creator. This would also help sites where no bugs are totally public but there are large groups of users with access to entire products. 3) While I understand the desire to spread the load out for generation of stats, this will tend to make the data inconsistent. Can the "phase" of statistic generation be specified rather than hard-coding it to (series_id mod 7)? 4) For testing, we will want to be able to change the basic unit from days to hours or minutes. Also, we probably will need to come up with some scripts to do mass-bug-creation so that there is a pool of bugs that can be mass-whatever'd to make the reports interesting during testing.

... also, in the query, you need to use a COUNT(DISTINCT(bugs.bug_id)) instead of a COUNT(*) when you do the searching or else you will multiple count whenever there is a JOIN in the query.

1) No. :-) > 2) Can we not forget who created and "owns" a public query? Pwerhaps use > something other than zeroing the creator to make a query public? I don't know where you got that idea; that used to be the case, but the current code subscribes user 0 to the query to make it public. Is there a stale comment somewhere? We need to remember who owns a public query, for several reasons. Firstly, they can edit it. Secondly, it needs to continue to run with the same permissions, because otherwise the data will be inconsistent. > 3) While I understand the desire to spread the load out for generation of > stats, this will tend to make the data inconsistent. By inconsistent, you mean "not all updated at the same time", right? I don't think this is a problem, because the nature of this is historical - if you want to see what's happening right now, you run the query. Anyway, I anticipate that, on all but the largest sites, most users will be allowed to create series at frequency of 1 day. > Can the "phase" of statistic > generation be specified rather than hard-coding it to (series_id mod 7)? You mean specified at creation time by the user? > 4) For testing, we will want to be able to change the basic unit from days to > hours or minutes. Or have a little patience :-) To be honest, there's nothing in the code that would work for 3 data points but not for 300. The only possible concern is performance - and there's no way we can put b.m.o. loads on without this being on b.m.o. anyway. > Also, we probably will need to come up with some scripts to > do mass-bug-creation so that there is a pool of bugs that can be > mass-whatever'd to make the reports interesting during testing. Well, Landfill has 500 bugs - we can mass-change them all over the place if we like. Perhaps a good time to remind people to disable their landfill mail, though :-) Gerv

+sub createInDatabase { ..... + + # Public queries are owned by user 0. + $self->{'creator'} = $::userid; + ;) Perhaps we should make sure the landfill instance has bugmail disabled. On the "phase" issue, the sice the site is free to permit daily stats, the issue is enough of a non-issue. You might want to consider using the minimum frequency to compute the offset rather than the actual frequency, but it is not a big deal.

And further down: > # Public series are subscribed to by userid 0. > $self->subscribe(0) if ($self->{'public'}); So yes, it's a stale comment - you can see it doesn't match the code. Fixing the comments is on the TODO list :-) > Perhaps we should make sure the landfill instance has bugmail disabled. Perhaps. But what if someone wants to use it to test bugmail patches <cough JP cough>? ;-) Gerv

OK, here's a second version (77k). The first one was missing at least one file. :-) Quite a bit of polish has gone on, with the big remaining item being conversion to the new DBI stuff. Still, this basically works, if anyone wants to play with it. The only current problem is a weird layout thing with the footer on the create chart page. Gerv

Having had a slightly nasty loss (requiring the repetition of an hour's work) recently, here's another version, in the spirit of Bugzilla-as-backup-mechanism. This is really nearly there - just the migration code to write. Gerv

This is it. Gerv

Comment on attachment 113752 [details] [diff] [review] Patch v.4 This is as close as it's going to get without some review. Joel, are you available? Gerv

Comment on attachment 113752 [details] [diff] [review] Patch v.4 bbaetz: could you review this for good DBI use, please? If you have time to review the rest, that's great too. Gerv

Comment on attachment 113752 [details] [diff] [review] Patch v.4 Dave - I figure AOL management will go gaga over this, so you might want to review it for the kudos. ;-) Gerv

This patch deals with the fallout from bug 191863. All review request still apply; it's not worth moving them. Too much spam. Gerv

Misc semi-random comments: - If there isn't any data, then I just get a broken image. chart.cgi shouldn't produce html in an error case. - chart.cgi claims to always be sending text/html (via |sub view()| in chart.cgi) - # We need to remove the GROUP BY clause, so that we get a single # number, and not a table full of 672 1's. $sql =~ s/GROUP BY.*//; That will break HAVING searches, like votes. I'm not sure what hte problem there is, either. Oh, wait, I see. You want: "SELECT COUNT(bug_id) FROM (" . $search->getSQL() ")". Pity you can't do that with mysql. - If there isn't any data/mining data, then teh 'default' series don't get created by checksetup. Maybe this bit should include the 'regerate stats' thing from that other bug. We won't be able to do that for the more generic charting, so this is probably a good place. IOW, ignore the existing data entirely. I don'tknow how effeciant thats likely to be wthout subselects, but hey.... - chart.cgi has some extra spaces at the end of the href= attribute for the image. - Why does collectstats.pl need to still create the old chart info. Can't we just drop them? - You need docs. Lots of them. - "I'm feeling Buggy" ?? Whilst fun, and all that..... - You need more than editbugs for this, I think. - + my $serieses = $dbh->selectall_hashref("SELECT series_id, query " . + "FROM series " . + "WHERE ($days_since_epoch + series_id) % frequency = 0",+ "series_id"); Extra "series_id" arg. Plus, the plural of 'series' is 'series'. Why can't the CleanupChartTables() logic happen when stuff is deleted, rather than via this cron job? + "WHERE ($days_since_epoch + series_id) % frequency = 0", If a collectstats run is missed, don't you want to run it on the next day? IOW, have a 'next_date' entry, and just adjust that when the run is complete. Does anyone have a large data/mining set, so that I can test this in more detail?

One more thing - if you have a product with html chars in it, the js sleect stuff from query.cgi doesn't work. In the script, foo&amp;bar becomes foo&amp;amp;bar, which is correct for html, but not what we want. you need to js filter, not html filter in the js, or something along those lines, and then filter before adding the option. I think. Make sure that a product called "</script>" doesn't affect stuff, though.

.. and I seem to be getting x axis labels _way_ too often, so everything runs over each other.

> - If there isn't any data, then I just get a broken image. chart.cgi shouldn't > produce html in an error case. Well, I haven't yet figured out how to get the library to produce errors as PNGs. Any ideas? But it shouldn't do this, it should just produce a blank chart. I'll look into it. > - chart.cgi claims to always be sending text/html (via |sub view()| in > chart.cgi) I don't quite understand you. Is it sending text/html for the PNGs? > That will break HAVING searches, like votes. I'm not sure what hte problem > there is, either. Oh, wait, I see. You want: "SELECT COUNT(bug_id) FROM (" . > $search->getSQL() ")". Pity you can't do that with mysql. Better ideas? :-) - If there isn't any data/mining data, then teh 'default' series don't get created by checksetup. > Maybe this bit should include the 'regerate stats' thing > from that other bug. That code scares me a bit. > so this is probably a good place. IOW, ignore the existing data entirely. I > don'tknow how effeciant thats likely to be wthout subselects, but hey.... Ignoring the existing data seems highly dangerous to me :-) > - Why does collectstats.pl need to still create the old chart info. Can't we > just drop them? That's phase 2. The two cn run in parallel meantime. > - "I'm feeling Buggy" ?? Whilst fun, and all that..... What else says both "search" and "start recording data"? > - You need more than editbugs for this, I think. You can only create weekly ones with just editbugs. I think that's a good starting point. > Extra "series_id" arg. Plus, the plural of 'series' is 'series'. Yeah, but I didn't want the two to have the same name. > Why can't the CleanupChartTables() logic happen when stuff is deleted, rather > than via this cron job? "When stuff is deleted"? > If a collectstats run is missed, don't you want to run it on the next day? Naah; missing one is not a big deal. It copes. > and I seem to be getting x axis labels _way_ too often, so everything runs > over each other. Known bug. The graphing library doesn't like non-integer values for x_label_skip. Still working on this, but it's not a major issue. Gerv

> Well, I haven't yet figured out how to get the library to produce errors as PNGs. You can use the GD textutils. Or just produce a blank graph; thats what you have. > I don't quite understand you. Is it sending text/html for the PNGs? Yes. <img> does sniffing in mozilla, but try loading the image standalong in a new window. |view| has |print "Content-Type: text/html\n\n";| in it, unconditionally. Thats linked with your debug stuff... > [collectstats regenerate] code scares me a bit. Sure, but after this is checked in, that can't be, because series are more generic. > Ignoring the existing data seems highly dangerous to me :-) Not really. The stuff in the db must be correct, sort of my definition. Subject to bug deletion, and we don't support that. > "When stuff is deleted"? When a series is unwatched, check the COUNT then, and delete it if required. > Naah; missing one is not a big deal. It copes. No, because of the mod thing, it won't run again until its next run. If you graph monthly, then it'll be another month before it goes again. Have a last_run column, and a time_between_runs column, and there you go. > Known bug. The graphing library doesn't like non-integer values for > x_label_skip. Still working on this, but it's not a major issue. I someohow got to a state where I couldn't read anything at all horizontally. But that may jsut be my lack of real data, or something.

Just finally took my first crack at playing with this. Initial notes.... 1) Collectstats returns.... [Wed Feb 12 07:08:59 2003] collectstats.pl: DBI::db=HASH(0x85ceba0)->disconnect invalidates 79 active statement handles (either destroy statement handles or cal l finish on them before disconnecting) at Bugzilla.pm line 98. This is non-fatal since I think it only happens after all is done, but it is bad form. 2) The date range probably should allow the user to subsample the stored data. IOW if I collect a stat every week, I usually want to plot it by the week, but I may want to plot it (or export it to CSV) much more sparsely once I have a few years worth of data, I may plot it monthly. This also helps the case where I always store data daily and often want to see it weekly. 3) I really am going to want to specify what group can see the data (rather than everyone/nobody) and if the query should be run with my own group permissions or some subset. If we permit a dataset to be have a group restriction, I have no problem with creating a (disabled) pseudo-user for the second part of that, but the administrator needs to be able to do this. 4) If I add a new series and want to kick-start the collection by running collectstats right away, it dies due to the duplicate entries on other queries. It should probably handle this quietly and replace the duplicates with the revised queries. 5) Making a series visible to all does not seem to make it visible to all [[ And "fine_whine" is way funnier than "I'm feeling buggy" :-) ]]

1) I'll fix. 2) is definitely an RFE, and if you want advanced features, export to CSV and work with it in Excel. :-) 3) It's not even as good as you think. You can't restrict anyone from seeing any series at the moment, if they find out its series id. I appreciate that we may need to apply some group-related stuff to this, and am open to suggestions - but I don't want to break the fundamentally open and simple model. 4) Possibly; although this means that unless there's an SQL command "insert this if it isn't there, otherwise update it", then we'd need to query for _every_ data point before inserting it, which would suck a bit. 5) Need more information. Who can't see it? 6) ("fine_whine" vs. "I'm feeling buggy") As I said, I'm open to suggestions. "Do It" seemed too weak. Gerv

3)The simplest solution I could think of would be to allow the user to select one or more groups of which the user is a member to which to restrict the dataset. 4)I belive that using REPLACE instead of INSERT does exactly that. 5)I log in as one user, create a dataset that is "public" and then log out and back in as another user. I would expect that second user to be able to choose the dataset, but I do not see it. (I used ALL/ALL for the categories)

bbaetz said: > - You need docs. Lots of them. What, you mean it's not all crystal clear how it works? :-) Seriously, do you mean more code comments, or an addition to the Bugzilla Guide? > + my $serieses = $dbh->selectall_hashref("SELECT series_id, query " . > + "FROM series " . > + "WHERE ($days_since_epoch + series_id) % frequency = > 0",+ "series_id"); > > Extra "series_id" arg. No - it's the key for the hashref. > Why can't the CleanupChartTables() logic happen when stuff is deleted, rather > than via this cron job? We want to leave open the possibility that someone goes "oops, didn't mean to unsubscribe from that" and resubscribes. If we delete all the data on the last unsubscribe, a slipped finger could lose months of info. > If a collectstats run is missed, don't you want to run it on the next day? > IOW, have a 'next_date' entry, and just adjust that when the run is complete. If it gets forgotten for a week, that would be a big backlog to clear, and might have a detrimental effect on server performance. In real life, I would say, important ones run daily, and so don't benefit from being "run again", and weekly ones are only for spotting trends. > Does anyone have a large data/mining set, so that I can test this in more > detail? b.m.o. :-) landfill might have one, if Dave's been running collectstats out of cron. > One more thing - if you have a product with html chars in it, the js sleect > stuff from query.cgi doesn't work. In the script, foo&amp;bar becomes > foo&amp;amp;bar, which is correct for html, but not what we want. > > you need to js filter, not html filter in the js, or something along those > lines, and then filter before adding the option. I think. You'll need to explain this more clearly. Is this a bug in my stuff, or a bug in search.html.tmpl's JS? Which template do you mean when you say "The JS... in query.cgi"? > Yes. <img> does sniffing in mozilla, but try loading the image standalong in > a new window. |view| has |print "Content-Type: text/html\n\n";| in it, > unconditionally. Thats linked with your debug stuff... Er... it's plot() that produces PNGs, not view(). view() produces HTML always, as you've noticed - it writes the HTML wrapper for the PNG. > Not really. The stuff in the db must be correct, sort of my definition. > Subject to bug deletion, and we don't support that. And data corruption bugs for the activities table, and we've had several of those. And Joel said: > 1) Collectstats returns.... > [Wed Feb 12 07:08:59 2003] collectstats.pl: > DBI::db=HASH(0x85ceba0)->disconnect > invalidates 79 active statement handles (either destroy statement handles or > call finish on them before disconnecting) at Bugzilla.pm line 98. I can't reproduce the error, but I've added a call to finish(). > 4) If I add a new series and want to kick-start the collection by running > collectstats right away, it dies due to the duplicate entries on other > queries. I've changed this to REPLACE instead of INSERT. Good call. > 3)The simplest solution I could think of would be to allow the user to select > one or more groups of which the user is a member to which to restrict the > dataset. I'm going to let groups be part of Phase 2. If you see me make any design decisions which would make adding groups support harder, let me know. Otherwise, admins who are worried about this can just turn off the feature. Them's the breaks. > 5)I log in as one user, create a dataset that is "public" and then log out and > back in as another user. I would expect that second user to be able to choose > the dataset, but I do not see it. (I used ALL/ALL for the categories) Another good catch; that was a bug left over from my change to do "public" queries differently. Gerv

So, who is going to take a serious long look at this? I don't want to do 12 revisions of nits and then have someone point our major architectural flaws. Dave? bbaetz? Joel? Gerv

Both types of docs :) The js thing is a bug in your code. Create a product with '&amp;' in the name, and you'll see what I mean. Then look at the generated page. JS stuff doesn't need ot be html escaped, and shouldn't be. For a product of 'foo&amp;bar', the hash key becomes 'foo&amp;amp;bar', which then doesn't match. Hmm, not sure about the plot thing. Right click on the image, then select 'view image' and you'll see what I mean. The finish isn't needed - that was a separate bug which has now been fixed. REPLACE isn't potable - only mysql supports it. You basically need to lock, delete, and insert. I'm still not sure I'm happy about the lazy deletion thing, but...

Comment on attachment 114832 [details] [diff] [review] Patch v.6 See also my previous comments >Index: checksetup.pl > > use strict; >+use lib "."; Why? >+$table{user_series_map} = >+ 'user_id mediumint not null, >+ series_id mediumint not null, >+ >+ index(user_id), >+ index(series_id), >+ unique(user_id, series_id)'; >+ You can drop the separate user_id index - an inex on (a,b) can be used to lookup data on (a) >+$table{series_categories} = >+ 'category_id smallint auto_increment primary key, >+ name varchar(64) not null, >+ >+ index(name), >+ unique(name)'; You certainly don't need both an index and a non-unique index.... >+# 2003-xx-xx Copy the old charting data into the database, and create the >+# queries that will keep it all running. When the old charting system goes >+# away, if this code ever runs, it'll just find no files and do nothing. >+my $series_exists = $dbh->selectrow_array("SELECT 1 FROM series"); LIMIT 1 >+ >+ # We can't give the Series we create a meaningful owner; that's not a big >+ # problem. But we do need to set this global, otherwise Series.pm objects. >+ $::userid = 0; >+ Err, this is a big problem. 0 is not a valid userid; please don't use it. Either give it to an admin (prompted, perhaps?), or make it NULL. The former is definately better, but if you want to handle 'ownerless' queries then NULL is best. >+ >+ # The query for statuses is different to that for resolutions. >+ $queries{$_} = $query_prod . "status=$_" foreach (@statuses); Is that bracketed like: $queries{$_} = $query_prod . ("status=$_" foreach (@statuses)); Its ceratinly not clear. >+ $queries{$_} = $query_prod . "resolution=$_" foreach (@resolutions); >+ >+ foreach my $field (@fields) { >+ # Create a Series for each field in this product >+ my $series = new Bugzilla::Series(-1, $product, $all_name, >+ $field, $::userid, 1, >+ $queries{$field}); >+ $series->createInDatabase(); >+ $seriesids{$field} = $series->{'series_id'}; >+ } >+ # We prepared this above For a long-lastings sth, call it something meaningful. >Index: chart.cgi >=================================================================== >RCS file: chart.cgi >diff -N chart.cgi >--- /dev/null 1 Jan 1970 00:00:00 -0000 >+++ chart.cgi 18 Feb 2003 23:50:32 -0000 >@@ -0,0 +1,312 @@ >+#!/usr/bonsaitools/bin/perl -wT >+# Go to buglist.cgi if we are doing a search. >+if ($action eq "search") { >+ my $params = $cgi->canonicalise_query("format", "ctype", "action"); >+ print "Location: buglist.cgi" . ($params ? "?$params" : "") . "\n\n"; >+ exit; >+} How can we end up here if we are doing a search? >+elsif ($action eq "delete") { >+ # This is an admin-only function. Usually, series get deleted automatically >+ # when everyone unsubscribes from them. >+ # @@@ complete Whats that @@@ about? >+# Check if the user is the owner of series_id or is an admin. >+sub assertCanDelete { >+ my ($series_id) = @_; >+ >+ return if UserInGroup("admin"); What about another group? Or is this not worth it? >+ my $dbh = Bugzilla->dbh; >+ my $creator = $dbh->selectrow_array("SELECT creator FROM series " . >+ "WHERE series_id = $series_id"); a) use a placeholder b) have the $creator == $::userid test in the SQL, and just check if anything was returned >Index: query.cgi >+if (($::FORM{'query_format'} || $::FORM{'format'}) eq "create-series") { >+ use Bugzilla::Chart; >+ $vars->{'category'} = Bugzilla::Chart::getVisibleSeries(); >+} require, not use - use happens always, assuming that your goal here was to reduce load time when not needed. >Index: buglist.cgi >@@ -251,6 +263,12 @@ > if ($::FORM{'cmdtype'} eq "dorem") { > if ($::FORM{'remaction'} eq "run") { > $::buffer = LookupNamedQuery($::FORM{"namedcmd"}); >+ $vars->{'title'} = "Bug List: $::FORM{'namedcmd'}"; >+ $params = new Bugzilla::CGI($::buffer); >+ $order = $params->param('order') || $order; >+ } >+ elsif ($::FORM{'remaction'} eq "runseries") { >+ $::buffer = LookupSeries($::FORM{"series_id"}); > $vars->{'title'} = "Bug List: $::FORM{'namedcmd'}"; > $params = new Bugzilla::CGI($::buffer); > $order = $params->param('order') || $order; Is this from another patch? >Index: collectstats.pl >=================================================================== >+ # We prepare the insertion into the data table, for efficiency. >+ my $sth = $dbh->prepare("REPLACE INTO series_data " . >+ "(series_id, date, value) " . >+ "VALUES (?, " . $dbh->quote($today) . ", ?)"); As mentioned, REPLACE isn't portable >+ foreach my $series_id (keys %$serieses) { >+ # We set up the user for Search.pm's permission checking - each series >+ # runs with the permissions of its creator. >+ $::vars->{'user'} = >+ new Bugzilla::User($serieses->{$series_id}->{'creator'}); >+ >+ my $cgi = new Bugzilla::CGI($serieses->{$series_id}->{'query'}); :) This is why B::S doesn't take a cgi param, but rather a 'params' object. Leave it for the moment, I guess. >+ # We need to count the returned rows. Without subselects, we can't >+ # do this directly in the SQL for all queries. So we do it by hand. Why do we need to count? In fact, what are we counting here? >+ $sth->finish(); You don't need to ->finish - it dies when it does out of scope >+ # Delete all series which no-one is subscribed to. >+ if ($series_ids) { >+ $dbh->do("DELETE FROM series WHERE series_id IN($series_ids)"); >+ >+ # Delete the unwanted data >+ $dbh->do("DELETE FROM series_data WHERE series_id IN($series_ids)"); Delete the data first, else if something happens in the middle (db server error), then you'll have dangling data. Ah, for transactions! >+ >+ # Delete empty categories or subcategories >+ # @@@ Is this right? >+# $dbh->do("DELETE FROM series_categories LEFT JOIN series AS s1 " . >+# "ON series.series_id = s1.series_id " . >+# "LEFT JOIN series AS s2 ON series.series_id = s2.series_id " . >+# "WHERE s1.series_id IS NULL AND s2.series_id IS NULL"); You can't do that. You could delete from series_categories where id IN (...), or you could on non-mysql dbs, anyway, >Index: template/en/default/reports/chart.png.tmpl >=================================================================== >+ # Workaround for the fact that set_legend won't take chart.labels directly, >+ # because chart.labels is an array reference rather than an array. >+ graph.set_legend(chart.labels.0, chart.labels.1, chart.labels.2, >+ chart.labels.3, chart.labels.4, chart.labels.5, >+ chart.labels.6, chart.labels.7, chart.labels.8, >+ chart.labels.9, chart.labels.10, chart.labels.11, >+ chart.labels.12, chart.labels.13, chart.labels.14, >+ chart.labels.15); There has to be a way arround this.... >+<script> >+[%# This structure holds details of the series the user can select from. %] >+var series = { >+[% FOREACH c = category.keys.sort %] >+ "[%+ c FILTER html %]" : { >+ [% FOREACH s = category.$c.keys.sort %] >+ "[%+ s FILTER html %]" : { >+ [% IF donames %] >+ [% FOREACH n = category.$c.$s.keys.sort %] >+ "[% n FILTER html %]": >+ [% category.$c.$s.$n %][% ", " UNLESS loop.last %] >+ [% END %] >+ [% END %] >+ }[% ", " UNLESS loop.last %] >+ [% END %] >+ }[% ", " UNLESS loop.last %] >+[% END %] >+}; This is the filtering bit I mentioned earlier which is wrong. You have to FILTER js instead (and also test what happens with a product with </script> in the name - probably .replace('</', '<" + "/'), or something similar. (I've really just skimmed over the rest of the templates) I'll look at the .pm's later

>>Index: checksetup.pl >> >> use strict; >>+use lib "."; > > Why? Because otherwise "perl -cT checksetup.pl" doesn't work. This is debugging code - I could take it out if it does any harm. Does it? >>+$table{series_categories} = >>+ 'category_id smallint auto_increment primary key, >>+ name varchar(64) not null, >>+ >>+ index(name), >>+ unique(name)'; > > You certainly don't need both an index and a non-unique index.... Perhaps someone needs to explain to me exactly what index() and unique() do :-) Is unique() a superset of index()? >>+ # We can't give the Series we create a meaningful owner; that's not a big >>+ # problem. But we do need to set this global, otherwise Series.pm objects. >>+ $::userid = 0; > > Err, this is a big problem. 0 is not a valid userid; please don't use it. > Either give it to an admin (prompted, perhaps?), or make it NULL. The former is > definately better, but if you want to handle 'ownerless' queries then NULL is > best. userid 0 means "not logged in" all over the code; it's not too much of a stretch to make that mean "no-one", is it? >>+ # The query for statuses is different to that for resolutions. >>+ $queries{$_} = $query_prod . "status=$_" foreach (@statuses); > > Is that bracketed like: > > $queries{$_} = $query_prod . ("status=$_" foreach (@statuses)); Nope. $queries{$_} = ($query_prod . "status=$_") foreach (@statuses); Above bracketing added to clarify. >>Index: chart.cgi >>=================================================================== >>+# Go to buglist.cgi if we are doing a search. >>+if ($action eq "search") { >>+ my $params = $cgi->canonicalise_query("format", "ctype", "action"); >>+ print "Location: buglist.cgi" . ($params ? "?$params" : "") . "\n\n"; >>+ exit; >>+} > > How can we end up here if we are doing a search? There's a radio button on the page for defining charts. You can either define a chart, or do a search to see what bugs would be counted for the chart. However, we can't change the CGI to submit to using JS, so we have to hardcode it to chart.cgi and redirect if necessary. >>+elsif ($action eq "delete") { >>+ # This is an admin-only function. Usually, series get deleted automatically >>+ # when everyone unsubscribes from them. >>+ # @@@ complete > > Whats that @@@ about? My way of marking that there's more work to do here. single chart deletion by admins isn't done yet. I'll take it out for the moment. >>+# Check if the user is the owner of series_id or is an admin. >>+sub assertCanDelete { >>+ my ($series_id) = @_; >>+ >>+ return if UserInGroup("admin"); > > What about another group? Or is this not worth it? The permissions model is similarly being worked out. This will do for now IMO. >>Index: buglist.cgi > >>@@ -251,6 +263,12 @@ >> if ($::FORM{'cmdtype'} eq "dorem") { >> if ($::FORM{'remaction'} eq "run") { >> $::buffer = LookupNamedQuery($::FORM{"namedcmd"}); >>+ $vars->{'title'} = "Bug List: $::FORM{'namedcmd'}"; >>+ $params = new Bugzilla::CGI($::buffer); >>+ $order = $params->param('order') || $order; >>+ } >>+ elsif ($::FORM{'remaction'} eq "runseries") { >>+ $::buffer = LookupSeries($::FORM{"series_id"}); >> $vars->{'title'} = "Bug List: $::FORM{'namedcmd'}"; >> $params = new Bugzilla::CGI($::buffer); >> $order = $params->param('order') || $order; > > Is this from another patch? Nope. This allows you to run any serieses query as a buglist. The link for this is on the "create chart" page. >>Index: collectstats.pl >>=================================================================== > >>+ # We prepare the insertion into the data table, for efficiency. >>+ my $sth = $dbh->prepare("REPLACE INTO series_data " . >>+ "(series_id, date, value) " . >>+ "VALUES (?, " . $dbh->quote($today) . ", ?)"); > > As mentioned, REPLACE isn't portable So I'll go back to INSERT. Don't run it more than once a day :-) As an aside, how much does that suck? Surely "Put this data into the table, overwriting any that's there if there is some" is a common operation? >>+ # We need to count the returned rows. Without subselects, we can't >>+ # do this directly in the SQL for all queries. So we do it by hand. > > Why do we need to count? In fact, what are we counting here? We are counting bugs. That's kinda what this patch is all about :-) If you remember, I used to do this by removing the GROUP BY clause, until you told me that this breaks HAVING searches (comment 33). >>+ >>+ # Delete empty categories or subcategories >>+ # @@@ Is this right? >>+# $dbh->do("DELETE FROM series_categories LEFT JOIN series AS s1 " . >>+# "ON series.series_id = s1.series_id " . >>+# "LEFT JOIN series AS s2 ON series.series_id = s2.series_id " . >>+# "WHERE s1.series_id IS NULL AND s2.series_id IS NULL"); > > You can't do that. You could delete from series_categories where id IN (...), > or you could on non-mysql dbs, anyway, Would you stop complaining? :-P Please tell me how I do it for all DBs. >>Index: template/en/default/reports/chart.png.tmpl >>=================================================================== > >>+ # Workaround for the fact that set_legend won't take chart.labels directly, >>+ # because chart.labels is an array reference rather than an array. >>+ graph.set_legend(chart.labels.0, chart.labels.1, chart.labels.2, >>+ chart.labels.3, chart.labels.4, chart.labels.5, >>+ chart.labels.6, chart.labels.7, chart.labels.8, >>+ chart.labels.9, chart.labels.10, chart.labels.11, >>+ chart.labels.12, chart.labels.13, chart.labels.14, >>+ chart.labels.15); > > There has to be a way arround this.... You would think. There's an open bug on it (bug 185474) and I will try and track the problem down at some point. > This is the filtering bit I mentioned earlier which is wrong. You have to > FILTER js instead (and also test what happens with a product with </script> in > the name - probably .replace('</', '<" + "/'), or something similar. I'll filter JS, but why do I need to worry about "</script>"? The values are quoted. Gerv

> Perhaps someone needs to explain to me exactly what index() and unique() do > :-) Is unique() a superset of index()? Yes. A multicolumn unique() also creates a normal index() on the first column listed as a side effect, so if you have "unique(a,b), index(b)" you also have index(a) without having to declare it. >>>+ # We can't give the Series we create a meaningful owner; that's not a > big >>>+ # problem. But we do need to set this global, otherwise Series.pm > objects. >>>+ $::userid = 0; >> >> Err, this is a big problem. 0 is not a valid userid; please don't use it. >> Either give it to an admin (prompted, perhaps?), or make it NULL. The >> former is definately better, but if you want to handle 'ownerless' >> queries then NULL is best. > > userid 0 means "not logged in" all over the code; it's not too much of a > stretch to make that mean "no-one", is it? In the code, sure, but not in the database. The second we start supporting a database that has real referential integrity, it'll start throwing SQL errors when you try to insert that, because it knows that column is a user ID, and we don't have a user with that ID. NULL works though, as long as the column is declared to allow them.

checksetup doesn't run with -T atm YEs, a unique index is an index where entries are constrained to be unique. userid =0 will shortly not mean that; Bugzilla->user being |undef| will :) the referential integrety checks we don't have yet will pick that up Don't use INSERT on its own, do |LOCK write|, |DELETE|, |INSERT|. SQL-<mumble> specifies MERGE, but neither mysql or pg support that yet. The problme with REPLACE is that if you have more than one unique index, which ones should be consdered conflicting? Its not specified that well... Oh, right, forgot about that. Yeah, mysql sucks :) The delete has to be done as a select for an id, followed by a delete of that id. Suitably locked, mind you I think that the sgml parsing of </script> happens before the js. Try it and see..

> userid =0 will shortly not mean that; Bugzilla->user being |undef| will :) OK, I'll fix that in the next round. But it could still do with another review, unless that's all there is :-) > Don't use INSERT on its own, do |LOCK write|, |DELETE|, |INSERT|. That too. > I think that the sgml parsing of </script> happens before the js. Try it and > see.. Hmm. Surely I want [% foo FILTER html FILTER js %], then, given that the values are going to get inserted into <select> widgets? Gerv

Comment on attachment 113752 [details] [diff] [review] Patch v.4 This patch is huge, and I'm too short on time to give this a proper review. If someone else gives it a code review I'll be happy to review usability though :) clearing the other review requests, too, since they're on an obsolete patch.

Comment on attachment 113791 [details] [diff] [review] Patch v.5 clearing review on obsolete patch

Comment on attachment 115983 [details] [diff] [review] Patch v.7 Dave: if you don't have time to review the whole thing, how about just reviewing the integration points with the rest of Bugzilla? With generic reporting, we reviewed it a bit, checked it in, and then basically fixed things in response to user testing, because it was a new subsystem. We could do the same here. Gerv

http://lxr.mozilla.org/mozilla/source/webtools/PLIF/PLIF/Service/TemplateToolkit.pm#304 is what I'm talking about with the js-in-html stuff, btw (html_js_filter)

bbaetz: how is that different from "FILTER js FILTER html"? Gerv

Because they're nto the same. Given: if (i > 3) we don't want if (i &nbsp; 3) since that doesn't work for js... We don't want to html escape. We need to js escape, plus have an extra escape for </ (Not sure why hixie has spaces arround the last bit in his filter, mind you.

The spaces are merely to make the regexp more readable, they don't affect the pattern matching at all.

> One more thing - if you have a product with html chars in it, the js sleect > stuff from query.cgi doesn't work. In the script, foo&amp;bar becomes > foo&amp;amp;bar, which is correct for html, but not what we want. (This is bbaetz' original comment about js-in-html.) Here's the question: when definining a product "Foo < Bar", are we expecting people to type into the "New product" box (and us to store) "Foo < Bar" or "Foo &lt; Bar"? I'd say the former. So, if that's true, a product "Foo &lt: Bar" means literally "F o o & l t ; B a r", and so my code is correct. Gerv

You haven't actually tried it, though. Create a product with &amp; in teh name, and then test.

As I mentioned to gerv a few weeks back via email, apart form thsi escaping issue, I'm happy, I think, although he does need a second reviewer, esp to look at the query deletion vs query data deletion design choices and the like.

bbaetz: I'm afraid you are going to have to spell this escaping thing out to me in words of less than one syllable. I've spent the last twenty minutes creating products and components with & and &amp; in them and it all seems to work fine (well, apart from creating components, but that's another bug.) Gerv

Here's a refreshed version, with no conflicts, and filterexceptions.pl changes for the new templates so the tests all pass. Gerv

Hmm, I just got an email dated March form this bug. (cmment #56) Gerv: Have you got an install of this up on landfill? If so, give me admin access and I'll set it up to show you what I mean.

Latest-and-greatest, for uploading to landfill. Gerv

OK - this version is now on Landfill at http://landfill.bugzilla.org/bz16009/ Click "Reports" on the front page, then click "New Charts". There's not much data in there because collectstats.pl appears not to have been running on landfill, and so there's no historical data to import. Mail me with any questions :-) Gerv

Atleast on landfill entering an invalid date to 'Date Range' causes an Internal Server Error. I used '2003-01-01' and 'Now'.

Comment on attachment 122953 [details] [diff] [review] Patch v.10 Someone please review this :-) It's a lot of code to be dragging around as a patch... Gerv

Some crazy ideas to be ignored :-) regarding http://bugzilla.mozilla.org/attachment.cgi?id=122953&action=view . RCS file: template/en/default/reports/series.html.tmpl diff -N template/en/default/reports/series.html.tmpl ... + <th>Name:</th> + <td></td> + </tr> Shouldn't that <td></td> be <th></th> for uniformity? If it's an empty table header, it should be marked using the th tag. + my @openedstatuses = ("UNCONFIRMED", "NEW", "ASSIGNED", "REOPENED"); I found three matches for this line. I know the values are hardcoded for now, but I wonder if introducing additional hardcoded strings in several places is a good idea, considering the purpose would be, even in the long term, to allow edit on the fly for those. Maybe a sub or something that will allow the initialization of @openedstatuses, or something like: @openedstatuses = getstatuses(); print "<INPUT TYPE=HIDDEN NAME=\"action\" VALUE=\"new\">\n"; + print "<INPUT TYPE=HIDDEN NAME='open_name' VALUE='All Open'>\n"; + print "<INPUT TYPE=HIDDEN NAME='closed_name' VALUE='All Closed'>\n"; print "</FORM>"; That's bad HTML but I guess until we make a template for this thing we'll have to live with it. :( Those are nitpicking/rants, of course. :P

FWIW, bug 193177 also reports bugs vs. time, but uses the bugs_activity table. The patch may be more popular than 16009 for some installations, such as pre-template versions of bugzilla. Even if the bugs_activity table has old corruption, the recent data is correct, and usually more interesting too. The HTML table contains links to bug lists, so the results are easily investigated and verified. Not only can the patch count matching bugs (a census), but it can also count transitions during intervals.

Comment on attachment 122953 [details] [diff] [review] Patch v.10 I have not tested this *extensively* but I did play around with it a bit on landfill this afternoon, and it looks sweet and seems to work well. Funny I got onto this by looking at the collectstats.pl rewrite script... we could run that on landfill and you'd have lots of data to import :-) There were a couple comments about this patch that you'll probably want to deal with (or not) prior to checkin. Patch still applies cleanly to the tip. :) With the size of this patch and the difficulty of getting thorough reviews on anything huge these days, you have my blessing if you'd like to check it in as is, and we'll pick up the pieces as people find bugs in it.

Fixed. Woohoo! Checking in checksetup.pl; /cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v <-- checksetup.pl new revision: 1.234; previous revision: 1.233 done RCS file: /cvsroot/mozilla/webtools/bugzilla/chart.cgi,v done Checking in chart.cgi; /cvsroot/mozilla/webtools/bugzilla/chart.cgi,v <-- chart.cgi initial revision: 1.1 done Checking in query.cgi; /cvsroot/mozilla/webtools/bugzilla/query.cgi,v <-- query.cgi new revision: 1.119; previous revision: 1.118 done Checking in buglist.cgi; /cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v <-- buglist.cgi new revision: 1.230; previous revision: 1.229 done Checking in collectstats.pl; /cvsroot/mozilla/webtools/bugzilla/collectstats.pl,v <-- collectstats.pl new revision: 1.31; previous revision: 1.30 done Checking in editproducts.cgi; /cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v <-- editproducts.cgi new revision: 1.38; previous revision: 1.37 done Checking in editcomponents.cgi; /cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v <-- editcomponents.cgi new revision: 1.32; previous revision: 1.31 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/chart.html.tmpl,v done Checking in template/en/default/reports/chart.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/chart.html.tmpl,v <-- chart.html.tmpl initial revision: 1.1 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/chart.png.tmpl,v done Checking in template/en/default/reports/chart.png.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/chart.png.tmpl,v <-- chart.png.tmpl initial revision: 1.1 done Checking in template/en/default/reports/menu.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/menu.html.tmpl,v <-- menu.html.tmpl new revision: 1.3; previous revision: 1.2 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/series-common.html.tmpl,v done Checking in template/en/default/reports/series-common.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/series-common.html.tmpl,v <-- series-common.html.tmpl initial revision: 1.1 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/chart.csv.tmpl,v done Checking in template/en/default/reports/chart.csv.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/chart.csv.tmpl,v <-- chart.csv.tmpl initial revision: 1.1 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/series.html.tmpl,v done Checking in template/en/default/reports/series.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/series.html.tmpl,v <-- series.html.tmpl initial revision: 1.1 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/create-chart.html.tmpl,v done Checking in template/en/default/reports/create-chart.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/create-chart.html.tmpl,v <-- create-chart.html.tmpl initial revision: 1.1 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/edit-series.html.tmpl,v done Checking in template/en/default/reports/edit-series.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/edit-series.html.tmpl,v <-- edit-series.html.tmpl initial revision: 1.1 done RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/search/search-create-series.html.tmpl,v done Checking in template/en/default/search/search-create-series.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/search/search-create-series.html.tmpl,v <-- search-create-series.html.tmpl initial revision: 1.1 done Checking in template/en/default/global/code-error.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v <-- code-error.html.tmpl new revision: 1.28; previous revision: 1.27 done Checking in template/en/default/global/user-error.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v <-- user-error.html.tmpl new revision: 1.36; previous revision: 1.35 done Checking in template/en/default/global/messages.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v <-- messages.html.tmpl new revision: 1.11; previous revision: 1.10 done RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v done Checking in Bugzilla/Series.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v <-- Series.pm initial revision: 1.1 done Checking in Bugzilla/Template.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Template.pm,v <-- Template.pm new revision: 1.7; previous revision: 1.6 done RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Chart.pm,v done Checking in Bugzilla/Chart.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Chart.pm,v <-- Chart.pm initial revision: 1.1 done Checking in template/en/default/filterexceptions.pl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v <-- filterexceptions.pl new revision: 1.4; previous revision: 1.3 done Gerv

Are you sure that you want to have this in the checked in version? + [% ELSIF message_tag == "series_already_exists" %] + [% title = "Series Already Exists" %] + A series <em>[% series.category FILTER html %] / + [%+ series.subcategory FILTER html %] / + [%+ series.name FILTER html %]</em> + already exists. If you want to create this series, you will need to give + it a different name. @@@ subscribe? ^^^^^^^^^^^^^^ My initial idea is simply to remove it or to use [%# @@@ subscribe %] if it is a kind of FIXME remark.

Comment on attachment 214035 [details] [diff] [review] docs patch about 'Bugzilla Database Tables' section, for 2.18 ... series category ... sounds wrong. ... stores series information/data (pick one) including category... whether _or not to_ publish the series||.