Closed Bug 247936 Opened 16 years ago Closed 12 years ago
Creating a product/component which already existed crashes when adding series
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
OS: Linux → All
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.18
Version: unspecified → 2.18
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
same patch but with trailing whitespaces removed.
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
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.
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.
gerv, any progress?
Nope, sorry - still snowed under :-( I know I suck. Gerv
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.
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.
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.
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).
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
QA Contact: mattyt-bugzilla → default-qa
Whiteboard: patch awaiting review
Target Milestone: Bugzilla 2.20 → Bugzilla 2.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. ***
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...
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
Target Milestone: --- → Bugzilla 3.0
(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: 18.104.22.168; 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: 22.214.171.124; previous revision: 1.14 done
Status: NEW → RESOLVED
Closed: 12 years ago
Component: Bugzilla-General → Administration
Resolution: --- → FIXED
Summary: Delete Product does not clean up Series table → Creating a product/component which already existed crashes when adding series
You need to log in before you can comment on or make changes to this bug.