Eliminate %::proddesc

RESOLVED FIXED in Bugzilla 3.0



13 years ago
12 years ago


(Reporter: Max Kanat-Alexander, Assigned: Max Kanat-Alexander)


Bugzilla 3.0
Dependency tree / graph
Bug Flags:
approval +



(1 attachment, 2 obsolete attachments)

4.88 KB, patch
Frédéric Buclin
: review+
Details | Diff | Splinter Review


13 years ago
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

13 years ago
Created attachment 198644 [details] [diff] [review]

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

I think that eliminating these versioncache variables is top priority, and we
can go back and fix up the architecture later.
Attachment #198644 - Flags: review?(LpSolit)

Comment 2

13 years ago
Oh, also, working on this bug exposed bug 311281 -- but note that that's a
different bug that we can fix somewhere else.
Target Milestone: --- → Bugzilla 2.22


13 years ago
Blocks: 311422

Comment 3

13 years ago
Comment on attachment 198644 [details] [diff] [review]

>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.
Attachment #198644 - Flags: review?(LpSolit) → review-

Comment 4

13 years ago
Created attachment 198754 [details] [diff] [review]

OK, thanks for the good review. :-) I've adressed all the nits.
Attachment #198644 - Attachment is obsolete: true
Attachment #198754 - Flags: review?(LpSolit)

Comment 5

13 years ago
Comment on attachment 198754 [details] [diff] [review]

>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
Attachment #198754 - Flags: review?(LpSolit) → review-


13 years ago
Depends on: 306325

Comment 6

13 years ago
Comment on attachment 198754 [details] [diff] [review]

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.
Attachment #198754 - Flags: review- → review?(LpSolit)

Comment 7

13 years ago
Comment on attachment 198754 [details] [diff] [review]

bitrotten. And $user->get_enterable_products now exists
Attachment #198754 - Flags: review?(LpSolit) → review-

Comment 8

13 years ago
Created attachment 202753 [details] [diff] [review]
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.)
Attachment #198754 - Attachment is obsolete: true
Attachment #202753 - Flags: review?(LpSolit)


13 years ago
Target Milestone: Bugzilla 2.22 → Bugzilla 2.24

Comment 9

13 years ago
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
Attachment #202753 - Flags: review?(LpSolit) → review+


13 years ago
Flags: approval?
Flags: approval? → approval+

Comment 10

13 years ago
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
Checking in enter_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/enter_bug.cgi,v  <--  enter_bug.cgi
new revision: 1.127; previous revision: 1.126
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.349; previous revision: 1.348
Last Resolved: 13 years ago
Resolution: --- → FIXED


12 years ago
Blocks: 311572
You need to log in before you can comment on or make changes to this bug.