267240 - Renaming products and components does not update charts

Status: ASSIGNED

(Bugzilla :: Reporting/Charting, defect)

Description

[Stuart Donaldson]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040616
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7) Gecko/20040616

Renaming a product does not update the new reporting system, and leaves stale
data in the series table.

Reproducible: Always
Steps to Reproduce:
1.Rename a product
2.Select reports / New charts


Actual Results:  
The new chart categories does not contain the new product.
Running the report as specified in the series will find no bugs, because all of
the queries are of the form product=OldProductName&... and are looking for the
old product name.

Expected Results:  
a) Category name should be changed in series_category to match the new product.
b) Series table should be updated, changing the queries to search the new product.

Comment 1

[Dave Miller [:justdave]]

Reassigning bugs that I'm not actively working on to the default component owner
in order to try to make some sanity out of my personal buglist.  This doesn't
mean the bug isn't being dealt with, just that I'm not the one doing it.  If you
are dealing with this bug, please assign it to yourself.

Comment 2

[Marc Schumann [:Wurblzap]]

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

Comment 3

[Marc Schumann [:Wurblzap]]

File-based charting is affected, too.

If I rename a product, reports.cgi doesn't see its data/mining file any more.

I'd say the file names in data/mining should be based on the product IDs, but we
could rename the file in editproducts.cgi when modifying the product name, too.

Comment 4

[A. Karl Kornel]

Attachment: Patch v1

The series table is searched.  If a series includes "product=*****" in the
query, it is returned.	A subroutine is used to confirm the existence of the
old product name and change it.  Bugzilla::Series is used to get & update
series data.  The appropriate file in data/mining is also renamed.

The category/subcategory names are not changed, as I can't be completely sure
that a given category/subcategory names refers to the product.	A message is
printed that warns the user to check & change the names if needed.

There is some messiness with Bugzilla::Series and locks.  The writeToDatabase
method takes locks of series and series_categories.  When the subroutine ends
all outstanding locks are released, including those held by editproducts; the
series-updating code goes after the product name change.  DBI also wanted a
lock on series before it would allow me to get a list of series_ids that may
need to be changed.  Finally, the lock release at the very end is changed so
that it will not die if there are no tables to unlock.

I'm requesting review from two people, to cover the changes made to
Bugzilla::Series and to editproducts.cgi.

One warning:  The change to Bugzilla::Series, to include updates to the query
along with other updates, is probably going to be made in the patch to bug
230254.  The change should be exactly the same, but there may be some sort of
conflict because of it.

Comment 5

[A. Karl Kornel]

Attachment: Patch v1.1

Fixed 2 compilation errors.

Comment 6

[A. Karl Kornel]

I just realized that a similar problem may happen when a component name is
changed.  A different bug?

Comment 7

[A. Karl Kornel]

Comment on attachment 191081 [details] [diff] [review]
Patch v1.1

Marking review- per comments from LpSolit on IRC.  Revised patch to come soon.

Comment 8

[Frédéric Buclin]

Comment on attachment 191081 [details] [diff] [review]
Patch v1.1

karl, you are not allowed to deny review to your own patches. Mark the patch as
obsolete and remove the request for review only.

Comment 9

[A. Karl Kornel]

A different bug should be created to cover the changes needed to Series.pm and
the CGIs that call Bugzilla::Series::writeToDatabase, as changes will have to be
made in several places (not just here) and it would probably be best if they
were made at the same time.

Comment 10

[A. Karl Kornel]

This won't be able to be checked in until bug 230254 lands, since bug 230254 is
actually focusing on providing data set query editing functionality.

Comment 11

[A. Karl Kornel]

Attachment: Patch v1.2

Modification of attachment 191081 [details] [diff] [review].

First of all, the actual changes to Series.pm (to update the query) were placed
into the patch for bug 230254, so that bug must land first before this one
does.

Second, as per the comments from LpSolit, the series renaming code was moved
into its own block (because of locking problems).  The DBI code used was also
changed.

Requesting review from Gerv & LpSolit.

Comment 12

[Marc Schumann [:Wurblzap]]

Comment on attachment 193504 [details] [diff] [review]
Patch v1.2

So this uses a locking bracket around the first part of the updates, and
individual locking brackets around each series update. It probably works doing
the ($product ne $productold) check a second time, but it's a hack imho. Why
not get your fix in after bug 305522?

Comment 13

[Frédéric Buclin]

Comment on attachment 193504 [details] [diff] [review]
Patch v1.2

>Index: editproducts.cgi

>+        print 'Updated product name.  <A HREF="chart.cgi">New Charts</A> 
>+               data set category/subcategory names must be updated 
>+               manually';
>+        print "<BR>\n";

editproducts.cgi has been templatized. You have to use templates now.

Comment 14

[A. Karl Kornel]

Attachment: Patch v1.3

Modification of attachment 193504 [details] [diff] [review] with respect to comment 13:

><<<snip>>>
> editproducts.cgi has been templatized. You have to use templates now.
> 

Well I guess it's not that surprising that the patch is finally bitrotted. 
Updated.  Also did a bit of reformatting, and I updated the appropriate
template (also added a missing </p> tag).

Comment 15

[Frédéric Buclin]

Comment on attachment 197134 [details] [diff] [review]
Patch v1.3

bitrotten. Moreover, as this patch is related to series and I'm really not
familiar with them, please only ask gerv for the updated patch. ;)

Comment 16

[Marc Schumann [:Wurblzap]]

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

Comment 17

[Gervase Markham [:gerv]]

I don't think the method in this patch can work; it updates the Series search but not the name of the category. So if I have a bunch of serieses in product Foo, then rename Foo to Bar, all the searches now search product Bar, but they are still in category Foo. That's really confusing.

Gerv

Comment 18

[Frédéric Buclin]

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".

Comment 19

[carl]

I attempted the following workaround in SQL:

update series set query=replace(query, 'product=oldname', 'product=newname');

But for the days after the product was renamed, the queries were looking for the old product name, so the data is zeroes. The data for these days is lost.

After updating the series table, I ran collectstats.pl --regenerate, and today's data is non-zero.

Comment 20

[Alex Pavloff]

A cheezy way to regenerate all the default series data:

1. Delete old mining data
2. Empty tables series, series_categories, series_data
3. Run collectstats.pl --regenerate
4. Run checksetup.pl

Not sure what this will do with custom series.

Note that if you're on Windows, 380939 will bite.

Comment 21

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?". Then, either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

This particular bug has not been touched in over eight months, and thus is being retargeted to "---" instead of "Bugzilla 4.0". If you believe this is a mistake, feel free to retarget it to Bugzilla 4.0.

Comment 22

[Kent Rogers [:mrbball]]

(In reply to comment #6)
> I just realized that a similar problem may happen when a component name is
> changed.  A different bug?

Did a different bug ever get opened for this problem?  I could not find
one.

If not, is there a recommended way of "repairing" the new charts tables
after a component rename?

As far as I can tell, the following will work as long as the component
name is unique in series_categories and there are no other components
with the same name(s):

UPDATE series_categories SET name = 'NEW_NAME' WHERE name = 'OLD_NAME';
UPDATE series SET query = '...component=NEW_NAME' WHERE subcategory = (SELECT id FROM series_categories WHERE name = 'NEW_NAME') and name = 'All Open';
UPDATE series SET query = '...component=NEW_NAME' WHERE subcategory = (SELECT id FROM series_categories WHERE name = 'NEW_NAME') and name = 'All Closed';

If the names are not unique, obviously more criteria would need to be
used in the SQL statements.

Does anyone see problems with this logic?

Comment 24

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-267240-v1.patch

This patch handles both category names and the query arguments. It attempts to only rename queries that are automatically created.

Comment 25

[:glob ✱]

Comment on attachment 8511930 [details] [diff] [review]
bug-267240-v1.patch

Review of attachment 8511930 [details] [diff] [review]:
-----------------------------------------------------------------

given gerv's familiarity with the charting code, he would be a better reviewer for this patch.

Comment 26

[Gervase Markham [:gerv]]

Comment on attachment 8511930 [details] [diff] [review]
bug-267240-v1.patch

Review of attachment 8511930 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for working on this! But I got errors:

Insecure dependency in parameter 7 of DBI::db=HASH(0xb382d24)->do method call while running with -T switch at Bugzilla/Component.pm line 296.

Similarly, when trying to rename a Product:

Insecure dependency in parameter 7 of DBI::db=HASH(0xa1795bc)->do method call while running with -T switch at Bugzilla/Product.pm line 488.

Gerv

Comment 27

[Dylan Hardison [:dylan] (he/him)]

That is very strange, I didn't encounter those errors, although obviously we need to detaint those params. What version of perl do you have installed?

Comment 28

[Gervase Markham [:gerv]]

gerv@hare:/usr/src/bedrock$ perl -v

This is perl 5, version 18, subversion 2 (v5.18.2) built for i686-linux-gnu-thread-multi-64int
(with 41 registered patches, see perl -V for more detail)

Ubuntu 14.04 LTS.

Gerv