311278 - Eliminate %::proddesc

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

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.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

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.

Comment 2

[Max Kanat-Alexander]

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

Comment 3

[Frédéric Buclin]

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.

Comment 4

[Max Kanat-Alexander]

Attachment: v2

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

Comment 5

[Frédéric Buclin]

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 6

[Max Kanat-Alexander]

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 7

[Frédéric Buclin]

Comment on attachment 198754 [details] [diff] [review]
v2

bitrotten. And $user->get_enterable_products now exists

Comment 8

[Max Kanat-Alexander]

Attachment: v3 (fixed bitrot)

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 9

[Frédéric Buclin]

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

Comment 10

[Frédéric Buclin]

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