Open
Bug 230254
Opened 21 years ago
Updated 7 years ago
Ability to edit a data set within the "new charts" feature
Categories
(Bugzilla :: Reporting/Charting, enhancement)
Tracking
()
NEW
People
(Reporter: veshi, Assigned: dylanAtHome)
References
(Blocks 1 open bug)
Details
(Whiteboard: See bug 302542 for ability to delete a data set)
Attachments
(1 file, 3 obsolete files)
7.32 KB,
patch
|
gerv
:
review-
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1) Opera 7.21 [en] Build Identifier: I'd like to see the ability to edit or delete the query that defines a data set in the "new charts" section of the reports page. Note: this is not a duplicate of Bug 226682 (and hopefully it's not a duplicate of any other bug) Reproducible: Always Steps to Reproduce: 1. 2. 3.
Comment 1•21 years ago
|
||
You can delete it - just unsubscribe from it and it'll go away. You can't edit it. That capability may come in the future, and I'll do it under this bug number. Gerv
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: ability to edit or delete a data set within the "new charts" feature → Ability to edit a data set within the "new charts" feature
Comment 2•19 years ago
|
||
I hope nobody minds, but since deleting a data and chaning the query executed by a data set are two different things, I've created bug 302542 to cover data set deletion.
Comment 3•19 years ago
|
||
I think this affects all hardware & platforms, and I think (though I'm not sure) the affected version is 2.18.
Flags: blocking2.20?
Comment 4•19 years ago
|
||
We just can't do this many changes in charting so close to a release or after we've released.
Flags: blocking2.20? → blocking2.20-
OS: Windows XP → All
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.22
Version: unspecified → 2.17.7
Comment 5•19 years ago
|
||
The reports/edit-series.html template is modified to include an "edit the query associated with this series" link to query.cgi, and is also modified to display a message when the update succeeds. query.cgi is modified to recognize that a data set query is being edited; it looks up the old query, along with the series name/category/subcategory, and passes them on to a new template. The new template is query/search-edit-series.html. It's similar to query/search-create-series.html; at first I simply tried to modify search-create-series, but the result looked pretty complicated so I made a new template instead. Finally, chart.cgi is modified to take the new query, canonicalize it, and update the appropriate Bugzilla::Series entry. This patch also modifies Bugzilla::Series so that it actually updates the query when writeToDatabase is called. This is exactly the same change made in Bugzilla::Series in attachment 191081 [details] [diff] [review] to bug 267240. Requesting review from gerv.
Attachment #191242 -
Flags: review?(gerv)
Comment 6•19 years ago
|
||
Max, can you please do some resolving on blocking2.20 here? This bug has blocking2.20- while at the same time blocks blocking2.20+ bug 247936.
Updated•19 years ago
|
Attachment #191242 -
Flags: review?(gerv)
Comment 7•19 years ago
|
||
Modification of attachment 191242 [details] [diff] [review]. During regular usage & testing, I noticed that if you were editing a data set category/subcategory/series name, the stored query would be overwritten upon submission. This happens because the 'alter' action creates a new Bugzilla::Series object by giving it CGI params, as opposed to creating a new object initialized with a series ID. Bugzilla::Series would then canonicalize the query and the correct one in the DB would be overwritten. This updated patch updates Bugzilla::Series to only canonicalize the query if a query CGI param isn't being passed in. The query param is inserted in the edit-series page as a hidden field. Is this a security risk? I don't think so, as the permissions required for editing a query should be the same for editing other data set attributes (except maybe for the public visibility). Requesting review from Gerv. Also requesting review from documentation@. BTW, since this bug is really focusing on the query, maybe a better summary would be "Ability to edit a data set query within the "new charts" feature"?
Attachment #191242 -
Attachment is obsolete: true
Attachment #191934 -
Flags: review?(gerv)
Comment 8•19 years ago
|
||
Comment on attachment 191934 [details] [diff] [review] Patch v1.1 Removing review pending some changes (making this patch compatible with the one to bug 267240 and updating it for tip).
Attachment #191934 -
Attachment is obsolete: true
Attachment #191934 -
Flags: review?(gerv)
Comment 9•19 years ago
|
||
Modification of attachment 191934 [details] [diff] [review], with some bits from attachment 191081 [details] [diff] [review] of bug 267240. For some reason, previous patches lacked the SQL needed to do the update. This patch now has that code. Again, requesting review from gerv.
Attachment #193496 -
Flags: review?(gerv)
Comment 10•19 years ago
|
||
Comment on attachment 193496 [details] [diff] [review] Patch v1.2 Thanks for the patch - sorry it's taken me so long to review it. >+ my $series = new Bugzilla::Series($series_id); >+ $series->{'query'} = $cgi->canonicalise_query('action-search', 'action', >+ 'series_id', 'submit-button', >+ 'reset-button', 'format', >+ 'ctype', 'query_format'); >+ trick_taint($series->{'query'}); >+ $series->writeToDatabase(); Are you sure canonicalise_query() makes the query safe for putting into the database? >Index: query.cgi >=================================================================== ... >+if (($cgi->param('query_format') || $cgi->param('format') || "") >+ eq "edit-series") { Nit: multi-line if statements have the bracket on the following line. >+ use Bugzilla::Series; >+ my $detainted_id = $cgi->param('series_id'); >+ detaint_natural($detainted_id) || ThrowCodeError('invalid_series_id'); >+ >+ my $series = new Bugzilla::Series($detainted_id); >+ $vars->{'series_id'} = $series->{'series_id'}; >+ $vars->{'series_category'} = $series->{'category'}; >+ $vars->{'series_subcategory'} = $series->{'subcategory'}; >+ $vars->{'series_name'} = $series->{'name'}; Why not: $vars->{'series'} = $series; and read the data fields out in the template? >Index: template/en/default/reports/edit-series.html.tmpl >=================================================================== ... >- <a href="query.cgi?[% default.query FILTER html %]">View >+ [% IF query_edit_complete %] >+ Updated search parameters saved | >+ [% ELSE %] Ick :-) Please don't replace an action link with result status information. Use a top-of-the-page message box or something like that. >Index: Bugzilla/Series.pm >=================================================================== ... >+ $self->{'query'} = $cgi->param('query') >+ || $cgi->canonicalise_query("format", "ctype", "action", >+ "category", "subcategory", "name", >+ "frequency", "public", "query_format"); > trick_taint($self->{'query'}); This is definitely a security problem - you are taking a CGI param and trick_tainting it without any checking on its contents. >+++ template/en/default/search/search-edit-series.html.tmpl Mon Aug 22 14:34:07 2005 ... >+<table> >+ <tr> >+ <td rowspan="2"><h3>Data Set Parameters: </h3></td> >+ <th style="text-align: right">Category</th> >+ <th> / </th> >+ <th style="text-align: center">Sub-Category</th> >+ <th> / </th> >+ <th style="text-align: left">Name</th> >+ </tr> >+ <tr> >+ <td style="text-align: right"> >+ [% series_category FILTER html %] >+ </td> >+ <th> / </th> >+ <td style="text-align: center"> >+ [% series_subcategory FILTER html %] >+ </td> >+ <th> / </th> >+ <td style="text-align: left"> >+ [% series_name FILTER html %] >+ </td> If these styles are necessary, and particularly if they are shared with the other, similar template, can they please come out to a style sheet? >+<p><i>Changing a query does not modify the data already in the set.</i><br> >+Once you have completed your changes, press 'Update Query' to save them.</p> Please change the word "query", which we no longer use in the Bugzilla interface (the name of the CGI 'query.cgi' excepted). The term used is "series search" or "series search parameters". This particular piece of text seems entirely unnecessary. People know web forms don't take unless you submit them. >+ >+<input type="reset" name="reset-button" value="Revert"> | >+<input type="submit" name="submit-button" value="Update Query"> "Update Series Search Parameters" Gerv
Attachment #193496 -
Flags: review?(gerv) → review-
Comment 11•19 years ago
|
||
The trunk is now frozen to prepare Bugzilla 2.22. Enhancement bugs are retargetted to 2.24.
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24
Comment 12•19 years ago
|
||
Modification of attachment 193496 [details] [diff] [review], with respect to comment 10: > >+ my $series = new Bugzilla::Series($series_id); > >+ $series->{'query'} = $cgi->canonicalise_query('action-search', 'action', > >+ 'series_id', 'submit-button', > >+ 'reset-button', 'format', > >+ 'ctype', 'query_format'); > >+ trick_taint($series->{'query'}); > >+ $series->writeToDatabase(); > > Are you sure canonicalise_query() makes the query safe for putting into the > database? It seems to be, as canonicalize_query is used to record the query when a new series is being created. > >Index: query.cgi > >=================================================================== > ... > >+if (($cgi->param('query_format') || $cgi->param('format') || "") > >+ eq "edit-series") { > > Nit: multi-line if statements have the bracket on the following line. OK. I was emulating the format of the previous block, but I'll use the new form instead. > >+ use Bugzilla::Series; > >+ my $detainted_id = $cgi->param('series_id'); > >+ detaint_natural($detainted_id) || ThrowCodeError('invalid_series_id'); > >+ > >+ my $series = new Bugzilla::Series($detainted_id); > >+ $vars->{'series_id'} = $series->{'series_id'}; > >+ $vars->{'series_category'} = $series->{'category'}; > >+ $vars->{'series_subcategory'} = $series->{'subcategory'}; > >+ $vars->{'series_name'} = $series->{'name'}; > > Why not: > $vars->{'series'} = $series; > and read the data fields out in the template? OK. > >Index: template/en/default/reports/edit-series.html.tmpl > >=================================================================== > ... > >- <a href="query.cgi?[% default.query FILTER html %]">View > >+ [% IF query_edit_complete %] > >+ Updated search parameters saved | > >+ [% ELSE %] > > Ick :-) Please don't replace an action link with result status information. Use > a top-of-the-page message box or something like that. Updated. > >Index: Bugzilla/Series.pm > >=================================================================== > ... > >+ $self->{'query'} = $cgi->param('query') > >+ || $cgi->canonicalise_query("format", "ctype", "action", > >+ "category", "subcategory", "name", > >+ "frequency", "public", "query_format"); > > trick_taint($self->{'query'}); > > This is definitely a security problem - you are taking a CGI param and > trick_tainting it without any checking on its contents. Updated. I send the query URL through a CGI object. If it dies during the parse or returns undef, I throw an error. Otherwise I canonicalize the query in the CGI object can use it. > >+++ template/en/default/search/search-edit-series.html.tmpl Mon Aug 22 14:34:07 2005 > ... > >+<table> > >+ <tr> > >+ <td rowspan="2"><h3>Data Set Parameters: </h3></td> > >+ <th style="text-align: right">Category</th> > >+ <th> / </th> > >+ <th style="text-align: center">Sub-Category</th> > >+ <th> / </th> > >+ <th style="text-align: left">Name</th> > >+ </tr> > >+ <tr> > >+ <td style="text-align: right"> > >+ [% series_category FILTER html %] > >+ </td> > >+ <th> / </th> > >+ <td style="text-align: center"> > >+ [% series_subcategory FILTER html %] > >+ </td> > >+ <th> / </th> > >+ <td style="text-align: left"> > >+ [% series_name FILTER html %] > >+ </td> > > If these styles are necessary, and particularly if they are shared with the > other, similar template, can they please come out to a style sheet? They don't seem to be used like this in any related files. At least I can pull them out to a style sheet in the document HEAD. > >+<p><i>Changing a query does not modify the data already in the set.</i><br> > >+Once you have completed your changes, press 'Update Query' to save them.</p> > > Please change the word "query", which we no longer use in the Bugzilla > interface (the name of the CGI 'query.cgi' excepted). The term used is "series > search" or "series search parameters". Updated. > This particular piece of text seems entirely unnecessary. People know web forms > don't take unless you submit them. Updated. > >+ > >+<input type="reset" name="reset-button" value="Revert"> | > >+<input type="submit" name="submit-button" value="Update Query"> > > "Update Series Search Parameters" Updated.
Assignee: gerv → karl
Attachment #193496 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #207290 -
Flags: review?(gerv)
Comment 13•18 years ago
|
||
Comment on attachment 207290 [details] [diff] [review] Patch v1.3 OK, this sucks as a review, but: - The template /search/search-edit-series.html.tmpl is missing from this patch. This is important, as it was supposed to contain several of the review changes. Can you upload a new patch with all the pieces? (I won't be able to review it for another two weeks due to being in hospital. This is the sucky part - after ages, I come along and say, "oops, this patch isn't ready.' Sorry about that.) Gerv
Attachment #207290 -
Flags: review?(gerv) → review-
Updated•18 years ago
|
QA Contact: mattyt-bugzilla → default-qa
Comment 14•18 years ago
|
||
Could the summary be changed in " Ability to edit or delete a data set within the "new charts" feature " ? ^^^ I did intend to create a bug report to have the ability to delete a data set, but, it's already in scope, reading comment 0. (In reply to comment #0) > I'd like to see the ability to edit or delete the query that defines a data set > in the "new charts" section of the reports page.
Updated•18 years ago
|
Summary: Ability to edit a data set within the "new charts" feature → Ability to edit or delete a data set within the "new charts" feature
Comment 15•18 years ago
|
||
This bug is retargetted to Bugzilla 3.2 for one of the following reasons: - it has no assignee (except the default one) - we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1) - it's not a blocker If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0. If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug. If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".
Target Milestone: Bugzilla 3.0 → Bugzilla 3.2
Comment 16•18 years ago
|
||
Until we have somebody actively working on this one again, I'm untargeted it.
Target Milestone: Bugzilla 3.2 → ---
Comment 17•17 years ago
|
||
See also bug 302542, which deals only with the delete-option.
Comment 18•17 years ago
|
||
Per comment 2, this is no longer about deletion because that part was moved to bug 302542.
Summary: Ability to edit or delete a data set within the "new charts" feature → Ability to edit a data set within the "new charts" feature
Whiteboard: See bug 302542 for ability to delete a data set
Updated•14 years ago
|
Assignee: karl.kornel → charting
Status: ASSIGNED → NEW
Updated•10 years ago
|
Assignee: charting → dylan
Updated•7 years ago
|
Assignee: dylan → dylan
You need to log in
before you can comment on or make changes to this bug.
Description
•