This is another versioncache variable that's got to go. We can use get_enterable_products and the various Bugzilla::Product methods to handle this one.

You'll notice that I don't do any re-architecture -- that can be done in another bug. In this bug, I just purely eliminate the %proddesc versioncache variable. I think that eliminating these versioncache variables is top priority, and we can go back and fix up the architecture later.

Oh, also, working on this bug exposed bug 311281 -- but note that that's a different bug that we can fix somewhere else.

Comment on attachment 198644 [details] [diff] [review] v1 >Index: describecomponents.cgi >+my $user = Bugzilla->user; Nit: You could define $user earlier in the code: my $user = Bugzilla->login(); >+ if (!@products) { > ThrowUserError("no_products"); > } Nit: I prefer 'if (scalar(@products) == 0)' or 'unless (scalar(@products))'. >+ # XXX - For backwards-compatibility with old template >+ # interfaces, we now create a proddesc hash. This can go away >+ # once we update the templates. I opened bug 311422 for that. :) >Index: enter_bug.cgi >+ # XXX - This loop should work in some more sensible, efficient way. I agree. We should open a bug as soon as possible. The actual way is ugly. ;) And this would allow us to get rid of IsInClassification(). >Index: globals.pl >- $zz = %main::proddesc; Nit: while you are on it, could you remove the unused definition '$zz = @main::default_column_list' too? SendSQL("SELECT name, description, votesperuser, disallownew$mpart " . "FROM products ORDER BY name"); while (@line = FetchSQLData()) { > my ($p, $d, $votesperuser, $dis, $u) = (@line); >- $::proddesc{$p} = $d; Here is the reason of my r-: description ($d) is not used anymore and should be removed from the SQL query.

OK, thanks for the good review. :-) I've adressed all the nits.

Comment on attachment 198754 [details] [diff] [review] v2 >Index: describecomponents.cgi >+ # Products which the user is allowed to see. >+ my @products = @{$user->get_selectable_products()}; Oops, I missed that in my previous review. You have to write: my @products = @{$user->get_enterable_products()}; which is defined in bug 306325. So my patch in bug 306325 has to be checked in first.

Comment on attachment 198754 [details] [diff] [review] v2 No, actually what I do here is the same thing that was done before. As I said in bug 311281, changing that to get_enterable_products is a different bug.

Comment on attachment 198754 [details] [diff] [review] v2 bitrotten. And $user->get_enterable_products now exists

OK, I've fixed the bitrot. See how this one does, it's basically the same as the old patch, although I do use get_enterable_products now. (Although I really think describecomponents should work on selectable, since it's called from both enter_bug and query.cgi.)

Comment on attachment 202753 [details] [diff] [review] v3 (fixed bitrot) This looks good and works correctly. But I agree, a cleanup will be required as soon as all these global variables are gone. Especially those files in this patch which are suboptimal. But we have time till we freeze for 2.24, so r=LpSolit

Max, I did the checkin myself to avoid to bitrot this one with other checkins. Checking in describecomponents.cgi; /cvsroot/mozilla/webtools/bugzilla/describecomponents.cgi,v <-- describecomponents.cgi new revision: 1.34; previous revision: 1.33 done Checking in enter_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v <-- enter_bug.cgi new revision: 1.127; previous revision: 1.126 done Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.349; previous revision: 1.348 done