302542 - Can not delete data sets (series) in New Charts

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, enhancement, P3)

Description

[A. Karl Kornel]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040804 Netscape/7.2 (ax)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.2) Gecko/20040804 Netscape/7.2 (ax)

I would like to be able to delete one or more series in New Charts.

Reproducible: Always

Steps to Reproduce:
1. Delete an already-existing product
2. Go to New Charts and attempt to delete the data sets associated with the product
Actual Results:  
I could not find a way to delete a data set

Expected Results:  
I should have been presented with a list of data sets to delete

This can cause a number of problems.  See bug 247936.

Comment 1

[A. Karl Kornel]

Attachment: Example "Delete Data Sets" page

This is what you will see when you click on the "Delete Data Sets" link in
chart.cgi.

Comment 2

[A. Karl Kornel]

Attachment: Patch v1

This is my first attempt at a patch for this bug.  Yay!

chart.cgi is modified to include two new actions: delete and delete_set.  In
the case of 'delete', the user is shown a list of all data sets that they can
delete.  The user checks boxes next to each set to be deleted, and pushes a
button that activates the next action, 'delete_set'.  In the case of
'delete_set', the list of items to delete is checked, to make sure the user
actually has permissions to delete the data sets.  Then each data set is
deleted and the initial New Charts page is displayed.

When a data set is deleted, all of the entries in series_data (that are
associated with the date set) are deleted.  Then the data set entry in series
is deleted.  Finally, the catagory/subcategory that held the data set is
checked.  If the category/subcategory is now empty, it's also deleted.

The list of data sets that can be deleted is displayed using it's own template
(a new file).  The list of filter exceptions is updated to include one
directive that can't be filtered.  There is also a new user-error and a new
code-error.

All of the new methods created for data set deletion are stored in the new
module Bugzilla::Series::Util module.  I created this module because all of the
new operations do not require the existence of a Bugzilla::Series object (in
C++ terms, they're static functions).  A POD is included.

I'm not sure who to ask for review on this one.

Comment 3

[A. Karl Kornel]

I have to remember to put the appropriate docs patches into a later attachment.

Comment 4

[A. Karl Kornel]

Attachment: Example "Confirm Data Set Deletion" page

Comment 5

[A. Karl Kornel]

Attachment: Patch v2

Modification of attachment 190909 [details] [diff] [review].  First of all, this attachment includes all
needed documentation patches.

The first big change is the introduction of an "are you sure you want to
delete?" page (see attachment 190915 [details]).	The page will list all of the series
that were selected (see attachment 190906 [details]) and will ask if the user really
wants to delete them, noting that all data will be lost.  The user is also
given the opportunity to (a) view the query from which the set gets its data,
and (b) view all data in the set in CSV (i.e. downloadable) format. 

The second big change comes in Bugzilla::Series::Util.	The delete_series
subroutine used to call can_mofify_series to ensure that the user doing the
deleting actually had permissions to do so.  This check has been removed;
chart.cgi is now responsible for checking permissions before calling
delete_series.	This will be particularly useful when the permissions needed to
delete a series varies depending on the action being performed.  delete_series
still checks to make sure that the series_id is valid.

I'm asking mkanat for review, as he's listed as a reviewer for .pm & .cgi files
and gerv seems to be unavailable.  mkanat: If you think someone else may be
better to review this, please don't hesitate to assign it to them!  I'm also
asking documentation@ to confirm that the docs patches don't break the
documentation.

Comment 6

[Max Kanat-Alexander]

We just can't make major changes to charting when we want our next version to be
the actual release version.

Comment 7

[A. Karl Kornel]

Comment on attachment 191018 [details] [diff] [review]
Patch v2

Since Gerv is supposed to be back next week, I might as well ask him for
review!

Comment 8

[Marc Schumann [:Wurblzap]]

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.

Comment 9

[Max Kanat-Alexander]

(In reply to comment #8)
> 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.

OK. See bug 247936 comment 17.

Comment 10

[Colin Ogilvie [:cso]]

Comment on attachment 191018 [details] [diff] [review]
Patch v2

>Index: using.xml
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/docs/xml/using.xml,v
>retrieving revision 1.33.2.1
>diff -u -r1.33.2.1 using.xml
>--- using.xml	12 Jul 2005 10:55:56 -0000	1.33.2.1
>+++ using.xml	30 Jul 2005 02:26:28 -0000
>@@ -1118,6 +1118,15 @@
>           and reduce the frequency of data collection to less than the default
>           seven days.
>         </para>
>+
>+        <para>
>+          If you no longer need the data set, you may remove it by 
>+          clicking on the "delete data sets" link on the Create Chart 
>+          page.  You will be presented with the list of data sets that you 
>+          may delete.  Select one or more and click on the "delete" 
>+          button.  Before deleting, you will be given the opportinuty to 
>+          download the data set in CSV form.
>+        </para>

Although it shouldn't break the documentation, there is a small typo
(opportinuty)...

Comment 11

[A. Karl Kornel]

Attachment: Patch v2.1

Modification of attachment 191018 [details] [diff] [review], taking into account comment 10 (among other
things).

It turns out that I was wrong about one thing.	I had originally thought that,
upon creation of a new product, all automagically-created data sets would have
Bugzilla (ID 0) set as the creator.  It turns out that the creator is being set
to the ID of the person who created the Product.  I'm changing it so all
automagically-created data sets have Bugzilla set as the creator.

Why do I do this?  Two reasons.  #1, I use "series.creator == 0" (among other
things) to identify automagically-created series.  Without this it becomes much
harder to identify automatically-created series.  In fact, without this it may
not be possible to guarantee a perfect ID (relying on
series/category/subcategory name on top of query content could result in false
positives/negatives).  #2, if the creator is a specific person, that (possibly
non-admin) person will also be able to delete the automagically-created data
sets at any time, which may not be a good idea.

I also added a confirmation page, so users will have one last change to cancel
their data set deletion.  They are also given the opportunity to download the
data set in CSV form for archving, and they can also take a look at the query
used to gather said data.

The typo in the docs has also been fixed.

Requesting review from Gerv and documentation@.

Comment 12

[Colin Ogilvie [:cso]]

Comment on attachment 191940 [details] [diff] [review]
Patch v2.1

Docs bit looks OK...

Comment 13

[Max Kanat-Alexander]

This bug doesn't have any active review request on it... shouldn't it?

Comment 14

[A. Karl Kornel]

(In reply to comment #13)
> This bug doesn't have any active review request on it... shouldn't it?
> 

Not if there's a big chance of bitrotting.  Gerv's unavailable for the rest of Nov. 2005.  I might have one other option, though.  If that option comes through I can post an up-to-date patch and target this for 2.22.

Comment 15

[Frédéric Buclin]

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

Comment 16

[Frédéric Buclin]

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

Comment 17

[bigstijn]

Is there a change that this will get in 3.2 ?  Target milestone is "---", by purpose ?

Comment 18

[Martin Hajduch]

this bug is really annoying, any chance of getting it fixed any time soon ?

Comment 19

[Max Kanat-Alexander]

(In reply to comment #18)
> this bug is really annoying, any chance of getting it fixed any time soon ?

  Probably not unless you fix it yourself, no. There is nobody currently assigned to working on Charts, at all.

Comment 21

[Matthias Klatt]

1.5 years later, has anything changed on this topic? This is really annoying, as my boss loves this feature. Not being able to clean up the mess I created in our charts really gives me a hard time.

Comment 22

[Max Kanat-Alexander]

(In reply to comment #21)
> 1.5 years later, has anything changed on this topic? This is really annoying,
> as my boss loves this feature. Not being able to clean up the mess I created in
> our charts really gives me a hard time.

  Yeah, I wish something had changed. Unfortunately, we still don't have anybody working on charts, though LpSolit (one of the primary developers of Bugzilla) *has* fixed a few charting issues, so perhaps this will be addressed. If you'd like to take a stab at it yourself, feel free. Our development process is described here:

  http://wiki.mozilla.org/Bugzilla:Developers

Comment 23

[Frédéric Buclin]

Yeah, I first focused on fixing some annoying bugs before starting to implement new features. I may decide to fix bug 276230 before this one; we will see.

Comment 24

[Frédéric Buclin]

Attachment: patch, v3

I add a link besides the Edit link and use tokens to prevent unwanted deletions. You need to apply my patch from bug 519040 first due to conflicts in global/messages.html.tmpl.

Comment 25

[Frédéric Buclin]

Just in case you are wondering, the patch contains one unrelated cleanup, where I replaced Bugzilla->user by $user where appropriate, and one small bug fix, where the Apache error log was reporting an undefined value when you call chart.cgi with no parameter. And yes, that's expected. :)

Comment 26

[Frédéric Buclin]

Attachment: patch, v3.1

I refactored assertCanEdit() to avoid code duplication. I also added a missing |require Bugzilla::User| to Series->creator (I cannot |use| it, due to dependency loops).

Comment 27

[Gervase Markham [:gerv]]

Comment on attachment 403047 [details] [diff] [review]
patch, v3.1

As far as I can see, you don't delete any of the data associated with the series. Is this intentional?

Gerv

Comment 28

[Gervase Markham [:gerv]]

Comment on attachment 403047 [details] [diff] [review]
patch, v3.1

r=gerv.

Gerv

Comment 29

[Frédéric Buclin]

(In reply to comment #27)
> As far as I can see, you don't delete any of the data associated with the
> series. Is this intentional?

For the record, the series_data table has a foreign key pointing to the series table, with ON DELETE CASCADE. So deleting an entry from the series table will automatically delete the corresponding entries from the series_data table. No need to delete stuff manually.

Comment 30

[Frédéric Buclin]

Checking in chart.cgi;
/cvsroot/mozilla/webtools/bugzilla/chart.cgi,v  <--  chart.cgi
new revision: 1.31; previous revision: 1.30
done
Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.20; previous revision: 1.19
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.92; previous revision: 1.91
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
new revision: 1.19; previous revision: 1.18
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/delete-series.html.tmpl,v
done
Checking in template/en/default/reports/delete-series.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/reports/delete-series.html.tmpl,v  <--  delete-series.html.tmpl
initial revision: 1.1
done

Comment 31

[Frédéric Buclin]

Attachment: 2nd part of patch v3.1

I just realized that I forgot to include reports/edit-series.html.tmpl in the diff.

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
new revision: 1.8; previous revision: 1.7
done

Comment 32

[Max Kanat-Alexander]

Added to the release notes in bug 547466.