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)

2.17.7
enhancement
Not set
normal

Tracking

()

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)

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.
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
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.
I think this affects all hardware & platforms, and I think (though I'm not sure)
the affected version is 2.18.
Flags: blocking2.20?
Blocks: 247936
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
Attached patch Patch v1 (obsolete) — Splinter Review
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)
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.
Attachment #191242 - Flags: review?(gerv)
Attached patch Patch v1.1 (obsolete) — Splinter Review
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 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)
Attached patch Patch v1.2 (obsolete) — Splinter Review
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)
Blocks: 267240
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:&nbsp;&nbsp;</h3></td>
>+    <th style="text-align: right">Category</th>
>+    <th>&nbsp;/&nbsp;</th>
>+    <th style="text-align: center">Sub-Category</th>
>+    <th>&nbsp;/&nbsp;</th>
>+    <th style="text-align: left">Name</th>
>+  </tr>
>+  <tr>
>+    <td style="text-align: right">
>+      [% series_category FILTER html %]
>+    </td>
>+    <th>&nbsp;/&nbsp;</th>
>+    <td style="text-align: center">
>+      [% series_subcategory FILTER html %]
>+    </td>
>+    <th>&nbsp;/&nbsp;</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"> &nbsp;|&nbsp;
>+<input type="submit" name="submit-button" value="Update Query">

"Update Series Search Parameters"

Gerv
Attachment #193496 - Flags: review?(gerv) → review-
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
Attached patch Patch v1.3Splinter Review
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:&nbsp;&nbsp;</h3></td>
> >+    <th style="text-align: right">Category</th>
> >+    <th>&nbsp;/&nbsp;</th>
> >+    <th style="text-align: center">Sub-Category</th>
> >+    <th>&nbsp;/&nbsp;</th>
> >+    <th style="text-align: left">Name</th>
> >+  </tr>
> >+  <tr>
> >+    <td style="text-align: right">
> >+      [% series_category FILTER html %]
> >+    </td>
> >+    <th>&nbsp;/&nbsp;</th>
> >+    <td style="text-align: center">
> >+      [% series_subcategory FILTER html %]
> >+    </td>
> >+    <th>&nbsp;/&nbsp;</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"> &nbsp;|&nbsp;
> >+<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 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-
QA Contact: mattyt-bugzilla → default-qa
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.
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
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
Until we have somebody actively working on this one again, I'm untargeted it.
Target Milestone: Bugzilla 3.2 → ---
See also bug 302542, which deals only with the delete-option.
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
Blocks: 451716
No longer blocks: 451716
Assignee: karl.kornel → charting
Status: ASSIGNED → NEW
Assignee: charting → dylan
Assignee: dylan → dylan
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: