300231 - Bugzilla::User needs a way of returning only selectable classification objects

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

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.

Comment 1

[Gabriel Sales de Oliveira]

Attachment: Classification Subroutine

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 =

Comment 2

[Gabriel Sales de Oliveira]

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 3

[Max Kanat-Alexander]

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?

Comment 4

[Max Kanat-Alexander]

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

Comment 5

[Max Kanat-Alexander]

That is, (just to be clear) an *array* of Classification objects. :-)

(Obviously perl doesn't have a "Set" type. :-) )

Comment 6

[Gabriel Sales de Oliveira]

Attachment: Return array

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 7

[Max Kanat-Alexander]

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

Comment 8

[Gabriel Sales de Oliveira]

Attachment: v3-get_selectabe_classifications

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

Comment 9

[Max Kanat-Alexander]

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.

Comment 10

[Gabriel Sales de Oliveira]

Attachment: v4-patch

The User object now is passed by the call site.

Are good the pod doc's this way?

Comment 11

[Max Kanat-Alexander]

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.

Comment 12

[Gabriel Sales de Oliveira]

Attachment: v5-subroutine changed to User.pm

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.

Comment 13

[Tiago Mello [:timello]]

(In reply to comment #12)

> The warning at runtests.pl is about Bugzilla.pm.

See Bug 303413

Comment 14

[Max Kanat-Alexander]

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.

Comment 15

[Gabriel Sales de Oliveira]

Attachment: v6- bugzilla::user method

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

Comment 16

[Max Kanat-Alexander]

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};

Comment 17

[Gabriel Sales de Oliveira]

Attachment: v7-return right

I changed the code as yo said, thanks :)

Comment 18

[Max Kanat-Alexander]

Comment on attachment 192123 [details] [diff] [review]
v7-return right

Great! Thanks, Gabriel. :-)

Comment 19

[Max Kanat-Alexander]

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

Comment 20

[Max Kanat-Alexander]

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 21

[Max Kanat-Alexander]

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. :-(

Comment 22

[Gabriel Sales de Oliveira]

Attachment: v8- put a return statement

Sorry by my mistake :(

Comment 23

[Max Kanat-Alexander]

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. :-)

Comment 24

[Gabriel Sales de Oliveira]

Attachment: v8-fix

As you wish :)

Comment 25

[Max Kanat-Alexander]

Comment on attachment 192986 [details] [diff] [review]
v8-fix

Looks good to me.

Comment 26

[Frédéric Buclin]

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