Closed Bug 247936 Opened 20 years ago Closed 16 years ago

Creating a product/component which already existed crashes when adding series

Categories

(Bugzilla :: Administration, task)

2.18
task
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.0

People

(Reporter: jdubrule, Assigned: LpSolit)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040404 Firefox/0.8
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6) Gecko/20040404 Firefox/0.8

Deleting a product does not remove its entries from the 'Series' table.

Reproducible: Always
Steps to Reproduce:
1. Add a product
2. Delete it
3. Try to add it again

Actual Results:  
Software error:

DBD::Oracle::db do failed: ORA-00001: unique constraint (BUGZILLA.SERIES_1_UDX)
violated (DBD ERROR: OCIStmtExecute) [for statement ``INSERT INTO series
(series_id, creator, category, subcategory, name, frequency, query) VALUES
(series_series_id_seq.nextval, ?, ?, ?, ?, ?, ?)'']) at Bugzilla/Series.pm line 185
	Bugzilla::Series::writeToDatabase('Bugzilla::Series=HASH(0x88e531c)') called at
/srv/www/cgi-bin/bugzilla/editproducts.cgi line 420



Expected Results:  
Adding new product
	  	
OK, done.
I'm seeing this as well for 2.18RC1.  This becomes a problem if you delete
a product and want to recreate the product.  
*** Bug 263161 has been marked as a duplicate of this bug. ***
Happens with components, too.
Severity: minor → normal
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking2.20+
Flags: blocking2.18.1+
OS: Linux → All
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.18
Version: unspecified → 2.18
Assignee: justdave → general
Attached patch untested patch, v1 (obsolete) — Splinter Review
this patch checks if the series already exists and updates its series_id
accordingly. Works with editproducts.cgi but may break something else. This
patch needs to be tested...
Assignee: general → LpSolit
Status: NEW → ASSIGNED
Attachment #177853 - Flags: review?(gerv)
Attachment #177853 - Flags: review?(justdave)
same patch but with trailing whitespaces removed.
Attachment #177853 - Attachment is obsolete: true
Attachment #177855 - Flags: review?(gerv)
Attachment #177855 - Flags: review?(justdave)
Attachment #177853 - Flags: review?(justdave)
Attachment #177853 - Flags: review?(gerv)
Whiteboard: patch awaiting review
I'm not sure this is the right fix - or at least, not a complete fix. For
example, if we delete a product, we should be deleting all the series data too.

Gerv
gerv, justdave said we should keep series data:

(18:15:41) LpSolit: justdave, when deleting a product, do we want to delete its
informations from series too or do we want to keep them for historical purpose?
(18:16:18) justdave: that's a really good question.
(18:16:33) justdave: in theory we should probably keep it around
(18:16:43) LpSolit: ok
(18:16:55) wicked: what about in practice? :)
(18:16:57) justdave: we probably just need to not bail if there's already
categories or subcategories existing with that product/component name
(18:17:14) justdave: i.e. check if they already exist and not do anything if
they're already there
(18:17:28) justdave: only create them if they don't already exist

Or I misunderstood what he said ;)
May I suggest running "Sanity check" too.  For example, after I deleted a group,
it reports the orphaned records as errors:
Checking references to groups.id
  [snip]
Bad value 10 found in group_group_map.grantor_id
  [snip]
Bad value 10 found in user_group_map.group_id 

As a final test of the fixes to cleanup orphaned records, should not "Sanity
check" run without reporting these errors?

I also experienced the error with the components table (which is already
reported), and note the groups table has the same type of problem.
gerv, could you please review this patch?
Hmm. But what happens if the newly-created product is in fact a different thing
from the old one which happened to have the same name? No series will get
created when perhaps they were wanted... I need to think about this some more.

I also can't be sure about the possible side-effects of this patch without
checking carefully. I will endeavour to do so over the Easter weekend.

Gerv
Attachment #177855 - Flags: review?(gerv) → review-
Assignee: LpSolit → gerv
Status: ASSIGNED → NEW
Attachment #177855 - Flags: review?(justdave)
Just for the record, I disagree with the idea of comment 7 to retain the series
data when deleting a product (or component). I think it should be deleted along
with the product. If you want to keep the data, keep the product.
(In reply to comment #11)
> Just for the record, I disagree with the idea of comment 7 to retain the series
> data when deleting a product (or component). I think it should be deleted along
> with the product. If you want to keep the data, keep the product.

  I agree also. You can always close a product to bug entry and put it behind a
mandatory group if you just want to make it "invisible."

  Keeping around data in the DB that breaks referential integrity is a bad idea.
Whiteboard: patch awaiting review → bug awaiting patch
I talked to Gerv on IRC, and he thinks that this doesn't really block 2.18.1,
but we should push it off to 2.18.2.
Flags: blocking2.18.2?
Flags: blocking2.18.2?
Flags: blocking2.18.2+
Flags: blocking2.18.1-
Flags: blocking2.18.1+
gerv, any progress?
Nope, sorry - still snowed under :-( I know I suck.

Gerv
Flags: blocking2.18.4+
Flags: blocking2.18.3-
Flags: blocking2.18.2+
I talked with LpSolit & Wurblzap about this on IRC not too long ago, and it
seems that this relies on two things:

1) A standard interface for deleting a data set, both manually through chart.cgi
and automatically when deleting a product.
2) A way to modify the query executed by a data set, accessible through
chart.cgi (and linked to when deleting a product).

Why is it so important for someone to be able to modify a query?  Wurblzap noted
that you might have a data set tracking (for example) the number of bugs over
more than one customer product.  Deleting the entire set simply because you're
deleting a single customer product would delete still-valuable data.  Even so,
it may be enough for now to simply take care of (1) and leave (2) for another time.

I've created bug 302542 to track (1).  (2) already exists as bug 230254.  By the
way, the IRC conversation can be read at <http://tinyurl.com/8l7r3>, from time
23:16 to 23:37.
Depends on: 230254, 302542
justdave - I recommend that this bug no longer block the final 2.20 release, due
to the possible complexity that it will create, and also due to the fact that
it's extremely unlikely that we will have a r+ patch on this bug any time soon.
Agreed.  We shall live with charting continuing to suck for another release.
Flags: blocking2.20-
Flags: blocking2.20+
Flags: blocking2.18.4-
Flags: blocking2.18.4+
Attached file Sample "Confirm Product Deletion" Page (obsolete) —
Here is what the Patch v1 output would look like.

This is an example of how the delete product confirmation page will look if
there are data sets explicitly mentioning the product, and the logged-in user
is a non-admin who has chartgroup and editcomponents access.

The first table shows the data sets that were automagically created.  The
second table shows the data sets created by the logged-in user, which will also
be deleted.  The paragraph directly below the second table states that there
are 1+ data sets that were created by others and can't be touched, so the
product can't be deleted.  If the other 1+ data sets were visible to all, then
they would also be displayed.

Note the CSV & query links.  Also note how the logged-in user is able to edit
his data set to keep it from deletion.
Attached patch Patch v1 (obsolete) — Splinter Review
This patch, when combined with the patches to bug 230254 (edit data set query)
and bug 302542 (delete data set), should do all that's required.

This patch only comes into play if there are one or more series that explicitly
refer to the product being deleted.  If one or more series are found that
explicitly refer to the product, the users group memberships determine the
outcome.

* If the user is an administrator, the deletion can go ahead.  The admin is
also given the option to edit the data set queries, and can also download the
data set data (in CSV form) for archiving.

* If the user is not an administrator but is in the chargroup, the user can
delete the product IF the only series that exist were automagically-created
data sets (data sets created by Bugzilla when the product was created) or
custom data sets created by the user.  The user is given the option to edit his
own custom data sets instead of deleting them.	If any of the data sets were
created by other users, then the product can not be deleted.  The user will be
able to archive data set data for all data sets, except for private data sets
created by other users.  In fact, if there are any private data sets, the user
will be told that they exist but won't be given any names.

* If the user is not an admin AND is not in the chartgroup, then the product
can only be deleted IF the only relevant data sets were automagically created
(in other words, no user-created data sets).  The user won't be able to
archive, view, or edit anything except for the data set names.

Most of the work takes place in editproducts.cgi.  Changes were also made to
the templates admin/product/confirm.delete.html and global/user-error.html.

Requesting review from Gerv.  I'll also be requesting review from
documentation@.  I'll be looking out for review responses regularly, so any
delays probably won't be coming from me!

I should note that the dependencies could probably be removed, but doing so
requires a sacrifice.  If you're willing to lose the ability to edit a data set
query, then the dependency on bug 230254 can be removed.  The dependency on bug
302542 can also be removed, but some of the code from bug 302542 would have to
be moved into this bug, and it would also mean that the only way to delete a
data set would be to delete the product associated with it (at least, until bug
302542 lands).
Attachment #191655 - Flags: review?(gerv)
gerv, could you have a look at karl's patches? Charts really need work and we
should be glad that somebody cares to do some work on them. These patches are
awaiting your reviews for almost one month. FYI, karl is often in #mozwebtools
and maybe you could discuss together.
Whiteboard: bug awaiting patch → patch awaiting review
I've reviewed several of Karl's patches today, and sent him an email about the rest.

Gerv
Now that 2.20 has been released, 2.18 should now only accept crash and security
fixes. Retargetting to 2.20
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20
Attachment #177855 - Attachment is obsolete: true
Attachment #191655 - Flags: review?(gerv)
QA Contact: mattyt-bugzilla → default-qa
Whiteboard: patch awaiting review
Target Milestone: Bugzilla 2.20 → Bugzilla 2.22
Flags: blocking2.22?
I'll leave this targetted to 2.22 for now, but it's not going to block release.  This has been broken for as long as we've had charts, so it won't break anything new by continuing to be broken.
Flags: blocking2.22? → blocking2.22-
*** Bug 336861 has been marked as a duplicate of this bug. ***
*** Bug 336861 has been marked as a duplicate of this bug. ***
*** Bug 347809 has been marked as a duplicate of this bug. ***
Target Milestone: Bugzilla 2.22 → ---
Target Milestone: --- → Bugzilla 3.2
I also get a sub-bug of this bug and I find that quite disturbing: Bugzilla requires removing people from groups before removing groups, but it will happily keep components of removed products and the components will magically re-appear if you create the Product again...
Target Milestone: Bugzilla 3.2 → ---
gerv, I would like to take my patch 1.0.1 as a workaround till this bug is fixed to stop these crashes when you create twice the same component or product. This kind of problem happens all the time when doing QA. I can file a separate bug to include this workaround so that we keep this bug open for a real fix. Is that OK for you?
You should do whatever you think is right :-)

Gerv
Comment on attachment 177855 [details] [diff] [review]
untested patch, v1.0.1

If this works, then it's fine with me. Let's just do it--it's such an edge case anyway.
Attachment #177855 - Flags: review- → review+
The patch is so old we should make sure it still works on all the modern code and branches before checkin, of course.
Assignee: gerv → LpSolit
Flags: approval3.2+
Flags: approval3.0+
Flags: approval+
Target Milestone: --- → Bugzilla 3.0
Attachment #177855 - Attachment is obsolete: false
Attachment #191651 - Attachment is obsolete: true
Attachment #191655 - Attachment is obsolete: true
(In reply to comment #34)
> The patch is so old we should make sure it still works on all the modern code
> and branches before checkin, of course.

No worry, all our QA installations, from 2.22 to 3.2, have this patch applied already. So I know it works as all our Selenium scripts are written assuming this patch is applied (else we would be unable to delete and recreate products and components with the same names).

I morphed the bug summary to reflect what we did here. I will clone the bug to decide what to do when deleting products.

tip:

Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.18; previous revision: 1.17
done

3.2rc1:

Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.16.2.1; previous revision: 1.16
done

3.0.5:

Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.14.2.1; previous revision: 1.14
done
Status: NEW → RESOLVED
Closed: 16 years ago
Component: Bugzilla-General → Administration
Keywords: relnote
Resolution: --- → FIXED
Summary: Delete Product does not clean up Series table → Creating a product/component which already existed crashes when adding series
Blocks: 451716
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.