Right now I'm trying to eliminate %::classification from Bugzilla, a versioncache varible, that needs to go away for mod_perl support. However, here's some sample code from enter_bug.cgi: foreach my $c (GetSelectableClassifications()) { $classdesc{$c} = $::classdesc{$c}; $classifications{$c} = $::classifications{$c}; } The problem is that GetSelectableClassifications returns a bunch of names, and Bugzilla::Classification works on id, mostly. I'm still going to re-write this code to use Bugzilla::Classification, but it would be easier if Bugzilla::Classification itself could just return me a list of Selectable Classification objects.

That subroutine, get_selectable_classifications return a hash of hashes with the classification object and his products. it could be called this way: my $accessible_classifications =

restarting. That subroutine, get_selectable_classifications return a hash of hashes with the classification object and his products. it could be called this way: my $accessible_classifications = Bugzilla::Classification::get_selectable_classifications($userid); And the Hash could be treated at the template to show the classification names. That's a good solution?

Comment on attachment 190589 [details] [diff] [review] Classification Subroutine >+sub products { This is unrelated to this patch... >@@ -113,10 +125,35 @@ sub get_all_classifications () { > my $classifications; > foreach my $id (@$ids) { > $classifications->{$id} = new Bugzilla::Classification($id); >+ $classifications->{$id}->products(); And that code should *never* be there, under any circumstances. It's pointless to call an accessor without returning it to some variable. >+sub get_selectable_classifications ($) { >+ my ($user_id) = @_; Just have it take a User object. >+ my $user = new Bugzilla::User($user_id); >+ my $products = $user->get_selectable_products(1); That "1" should be something more clear. Either a string, or a constant. >+ foreach my $class_id (keys %$classifications) { >+ my $class_products = >+ $classifications->{$class_id}->products(); >+ my $class = $classifications->{$class_id}; >+ >+ foreach my $key (keys %$class_products) { >+ if ($products->{$key}) { >+ $selectable_classifications->{$class_id} = $class; >+ } >+ } >+ } Instead of this loop-within-a-loop, wouldn't it just be easier to create product objects for each product returned by get_selectable_products, and check its classification_id, and then create Classification objects for those?

Oh, also, it should just return a set of Classification objects.

That is, (just to be clear) an *array* of Classification objects. :-) (Obviously perl doesn't have a "Set" type. :-) )

I fix the code with your idea ;) , it is a lot more clean instead. I used a hash to prevente duplicate entrys cause products could be at the same classification. Then return the hash values. Is that ok?

Comment on attachment 190613 [details] [diff] [review] Return array Everything looks good, except that BY_ID is too short a name for a constant, and the constant should live in Product.pm, so it's associated with the function using it. Also, this new method needs POD docs in Classification.pm, and the new constant will need POD docs (and the POD docs will need to change for get_selectable_products, to note the use of the constant).

Is that what you mean? Maybe i didn't understand you very well :( .

Comment on attachment 191475 [details] [diff] [review] v3-get_selectabe_classifications Yes, that's basically what I meant. :-) Looking at this now, though, I realized that the functions should take User objects, not a user_id. Also, the Description section on the POD docs needs to be slightly more detailed -- explain what "select" means more.

The User object now is passed by the call site. Are good the pod doc's this way?

Comment on attachment 191510 [details] [diff] [review] v4-patch OK, yeah, the POD docs look good. Use "entering" instead of enter, though. However, looking at this now, I realize that get_selectable_products_by_user isn't needed at all, and that get_selectable_classifications should be a method of the User object, not a subroutine of Classification like we're making it.

I changed the subroutine as a method at User.pm. I think that is the better place. The method returns is right? The warning at runtests.pl is about Bugzilla.pm.

(In reply to comment #12) > The warning at runtests.pl is about Bugzilla.pm. See Bug 303413

Comment on attachment 191601 [details] [diff] [review] v5-subroutine changed to User.pm >@@ -57,6 +58,7 @@ use base qw(Exporter); > use constant USER_MATCH_MULTIPLE => -1; > use constant USER_MATCH_FAILED => 0; > use constant USER_MATCH_SUCCESS => 1; >+use constant GET_PRODUCTS_BY_ID => 1; Nit: This constant should not be grouped with those constants. >+ $self->{selectable_classifications} = >+ [values %$selectable_classifications]; >+ return values %$selectable_classifications; You should actually be returning $self->{selectable_classifications}, and you shouldn't be running the rest of the code if {selectable_classifications} already exists.

I move the constant to a separate place and the method just return $self->{selectable_classifications}.

Comment on attachment 191982 [details] [diff] [review] v6- bugzilla::user method You need: sub get_selectable_classifications ($) { my ($self) = @_; return $self->{selectable_classifications} if defined $self->{selectable_classifications};

I changed the code as yo said, thanks :)

Comment on attachment 192123 [details] [diff] [review] v7-return right Great! Thanks, Gabriel. :-)

Checking in Bugzilla/User.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v <-- User.pm new revision: 1.70; previous revision: 1.69 done

Unfortunately, this triggers bug 303413, so I had to back it out. not ok 3 - Bugzilla.pm --WARNING # Failed test (t/001compile.t at line 89) [Tue Aug 9 23:34:56 2005] Bugzilla.pm: Constant subroutine Bugzilla::SHUTDOWNHTML_EXEMPT redefined at /usr/lib/perl5/5.8.0/constant.pm line 108. Bugzilla.pm syntax OK

Comment on attachment 192123 [details] [diff] [review] v7-return right >+ $selectable_classifications->{$product->classification_id} = >+ $product->classification; >+ } >+ $self->{selectable_classifications} = >+ [values %$selectable_classifications]; >+} ...I don't know how I missed this, but this function is entirely missing a return statement. :-(

Sorry by my mistake :(

Comment on attachment 192983 [details] [diff] [review] v8- put a return statement >+ $self->{selectable_classifications} = >+ [values %$selectable_classifications]; >+ return values %$selectable_classifications; >+} Just return $self->{selectable_classifications}, please. :-)

As you wish :)

Comment on attachment 192986 [details] [diff] [review] v8-fix Looks good to me.

Checking in Bugzilla/User.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v <-- User.pm new revision: 1.76; previous revision: 1.75 done