Eliminate %::proddesc

RESOLVED FIXED in Bugzilla 3.0

Status

()

Bugzilla
Bugzilla-General
--
enhancement
RESOLVED FIXED
13 years ago
12 years ago

People

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

Tracking

2.21
Bugzilla 3.0
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 2 obsolete attachments)

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

Description

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.
(Assignee)

Comment 1

13 years ago
Created attachment 198644 [details] [diff] [review]
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.
Attachment #198644 - Flags: review?(LpSolit)
(Assignee)

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.
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.22

Updated

13 years ago
Blocks: 311422

Comment 3

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

Comment 4

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

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

Updated

13 years ago
Depends on: 306325
(Assignee)

Comment 6

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

Comment 7

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

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

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)
(Assignee)

Updated

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+

Updated

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
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Updated

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