Can not delete data sets (series) in New Charts

RESOLVED FIXED in Bugzilla 3.6

Status

()

Bugzilla
Reporting/Charting
P3
enhancement
RESOLVED FIXED
12 years ago
6 years ago

People

(Reporter: A. Karl Kornel, Assigned: Frédéric Buclin)

Tracking

(Blocks: 2 bugs)

2.18
Bugzilla 3.6
Dependency tree / graph
Bug Flags:
approval +
blocking2.20 -

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

12 years ago
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.
(Reporter)

Updated

12 years ago
Version: unspecified → 2.18
(Reporter)

Updated

12 years ago
Blocks: 247936
(Reporter)

Updated

12 years ago
Flags: blocking2.20?
(Reporter)

Comment 1

12 years ago
Created attachment 190906 [details]
Example "Delete Data Sets" page

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

Comment 2

12 years ago
Created attachment 190909 [details] [diff] [review]
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.
(Reporter)

Updated

12 years ago
Attachment #190909 - Flags: review?
(Reporter)

Comment 3

12 years ago
I have to remember to put the appropriate docs patches into a later attachment.
Whiteboard: [patch awaiting review]
(Reporter)

Comment 4

12 years ago
Created attachment 191015 [details]
Example "Confirm Data Set Deletion" page
(Reporter)

Updated

12 years ago
Attachment #190909 - Flags: review?
(Reporter)

Comment 5

12 years ago
Created attachment 191018 [details] [diff] [review]
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.
Attachment #190909 - Attachment is obsolete: true
Attachment #191018 - Flags: review?(mkanat)
(Reporter)

Updated

12 years ago
Attachment #191018 - Flags: review?(documentation)

Comment 6

12 years ago
We just can't make major changes to charting when we want our next version to be
the actual release version.
Assignee: gerv → karl
Flags: blocking2.20? → blocking2.20-
Target Milestone: --- → Bugzilla 2.22
(Reporter)

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: Bugzilla 2.22 → ---
(Reporter)

Comment 7

12 years ago
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!
Attachment #191018 - Flags: review?(mkanat) → 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.

Comment 9

12 years ago
(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

12 years ago
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)...
Attachment #191018 - Flags: review?(documentation) → review-
(Reporter)

Updated

12 years ago
Attachment #191018 - Flags: review?(gerv)
(Reporter)

Comment 11

12 years ago
Created attachment 191940 [details] [diff] [review]
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@.
Attachment #191018 - Attachment is obsolete: true
Attachment #191940 - Flags: review?(documentation)
(Reporter)

Updated

12 years ago
Attachment #191940 - Flags: review?(gerv)

Comment 12

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

Docs bit looks OK...
Attachment #191940 - Flags: review?(documentation) → review+
(Reporter)

Updated

12 years ago
Attachment #191940 - Flags: review?(gerv)

Comment 13

12 years ago
This bug doesn't have any active review request on it... shouldn't it?
(Reporter)

Comment 14

12 years ago
(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.
Whiteboard: [patch awaiting review]
(Assignee)

Comment 15

12 years ago
*** Bug 316704 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 16

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

Comment 17

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

Comment 18

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

Comment 19

10 years ago
(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.
Assignee: karl.kornel → gerv
(Assignee)

Updated

9 years ago
Assignee: gerv → charting
(Assignee)

Updated

9 years ago
Severity: major → enhancement

Updated

9 years ago
Blocks: 386842
(Assignee)

Updated

9 years ago
Blocks: 451716

Updated

8 years ago
Whiteboard: [needs new patch]
(Assignee)

Updated

8 years ago
Duplicate of this bug: 518778

Comment 21

8 years ago
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

8 years ago
(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
(Assignee)

Comment 23

8 years ago
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.
Assignee: charting → LpSolit
Status: NEW → ASSIGNED
Whiteboard: [needs new patch]
Target Milestone: --- → Bugzilla 3.6
(Assignee)

Updated

8 years ago
Priority: -- → P3
(Assignee)

Comment 24

8 years ago
Created attachment 403038 [details] [diff] [review]
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.
Attachment #190906 - Attachment is obsolete: true
Attachment #191015 - Attachment is obsolete: true
Attachment #191940 - Attachment is obsolete: true
Attachment #403038 - Flags: review?(gerv)
(Assignee)

Comment 25

8 years ago
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. :)
(Assignee)

Comment 26

8 years ago
Created attachment 403047 [details] [diff] [review]
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).
Attachment #403038 - Attachment is obsolete: true
Attachment #403047 - Flags: review?(gerv)
Attachment #403038 - Flags: review?(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 on attachment 403047 [details] [diff] [review]
patch, v3.1

r=gerv.

Gerv
Attachment #403047 - Flags: review?(gerv) → review+
(Assignee)

Comment 29

8 years ago
(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.
Flags: approval+
(Assignee)

Comment 30

8 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: relnote
Resolution: --- → FIXED
(Assignee)

Comment 31

8 years ago
Created attachment 404575 [details] [diff] [review]
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

8 years ago
Added to the release notes in bug 547466.
Keywords: relnote
(Assignee)

Updated

7 years ago
Duplicate of this bug: 597223
(Assignee)

Updated

7 years ago
Duplicate of this bug: 597223

Updated

6 years ago
Blocks: 364249
You need to log in before you can comment on or make changes to this bug.