294160 - Step 1 (RO): Create libraries for Products, Components, Classifications, Milestones, and Versions

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Tiago Mello [:timello]]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.6) Gecko/20050405 Firefox/1.0 (Ubuntu package 1.0.2)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.6) Gecko/20050405 Firefox/1.0 (Ubuntu package 1.0.2)

Products.pm would offer an API for external manipulation of products.
Then, for instance, we will be able to use that for syncing up two instances
(bugzilla and a proprietary product).

Reproducible: Always

Steps to Reproduce:

Comment 1

[Tiago Mello [:timello]]

Attachment: v1: api draft

LpSolit, take quick look, please.

Comment 2

[Frédéric Buclin]

Comment on attachment 183588 [details] [diff] [review]
v1: api draft

>+sub FetchAllHashRef {
>+    
>+    my ($sth) = @_;
>+    my @list;
>+    while(my $row = $sth->fetchrow_hashref) {
>+        push @list, $row;
>+    }
>+    return @list;
>+}

I'm not sure we need this function. And if it appears to be really useful, it
should go into Util.pm.


>+sub get_product_by_name {
>+    
>+    my ($name) = @_;
>+    my $query = "SELECT * FROM products WHERE name = ?";
>+
>+    my $dbh = Bugzilla->dbh;
>+    my $sth = $dbh->prepare($query);
>+
>+    trick_taint($name);
>+    $sth->execute($name);
>+   
>+    my @data = FetchAllHashRef($sth);
>+    return $data[0];
>+}

I prefer something like:

sub get_product_by_name ($) {
    my ($name) = @_;

    my $dbh = Bugzilla->dbh;
    trick_taint($name);

    my $product = $dbh->selectrow_hashref("SELECT * FROM products WHERE name =
?", undef, $name);
    return %$product;
}

I did not test it, but it should work. The same comment applies to other
routines, as long as only one row is returned.
Moreover, I think get_components() should be called from get_product_by_*().
This way, you also have components directly from here. Having a
$product->{'has_components'} would be fine too.

This is a very quick review, but I'm in a hurry right now and you wanted a
review asap! :) I may change my mind...

Comment 3

[Max Kanat-Alexander]

Yeah, I was going to do this, at some point, too. I might have even already had
a bug for it, somewhere.

Comment 4

[Tiago Mello [:timello]]

Attachment: v2: keeping the patch updated.

LpSolit, we can continue the review.

Comment 5

[Frédéric Buclin]

Comment on attachment 183750 [details] [diff] [review]
v2: keeping the patch updated.

r- per our long discussion on IRC.

Comment 6

[Tiago Mello [:timello]]

Attachment: v3: separates the routines by context

Comment 7

[Christian Reis]

The general approach looks fine to me. I don't think we need to place these into
a subdirectory of Bugzilla -- Bugzilla::Product, etc, is fine by me.

Comment 8

[Frédéric Buclin]

Comment on attachment 183844 [details] [diff] [review]
v3: separates the routines by context

>+# The Initial Developer of the Original Code is Netscape Communications
>+# Corporation. Portions created by Netscape are
>+# Copyright (C) 1998 Netscape Communications Corporation. All
>+# Rights Reserved.

Is that really correct? I don't think so. Ask justdave if required.


>+    my $query = qq{SELECT classifications.*,
>+                          COUNT(classification_id) AS product_count
>+                   FROM   classifications
>+                   LEFT JOIN products
>+                        ON classifications.id = products.classification_id
>+                } . $dbh->sql_group_by('classifications.id',
>+                                       'classifications.name,
>+                                        classifications.description') .
>+                qq{ ORDER BY name};
>+
>+    my $classifications = $dbh->selectall_hashref($query, 'id', undef, ());

ORDER BY makes no sense in a hash.


>+sub get_by_product {
>+
>+    my ($product_id) = @_;
>+    
>+    my $dbh = Bugzilla->dbh;
>+    detaint_natural($product_id);
>+
>+    my $query = "SELECT * FROM components WHERE product_id = ?";
>+
>+    my $components = $dbh->selectall_hashref($query, 'id', undef,
>+                                             ($product_id));
>+    return $components;
>+}

Two things here: first, why not get_all_by_product as we want all components?
In Classification.pm we wrote get_all, so we should be consistent. Second,
would it be useful to have get_all_by_product_name and get_all_by_product_id
instead of this unique routine?


>+sub get_by_id {
>+
>+    my ($id) = @_;
>+
>+    my $dbh = Bugzilla->dbh;
>+    detaint_natural($id);
>+
>+    my $query = "SELECT * FROM components WHERE product_id = ?";
>+}

No query, no return. Very nice routine! :-p


>+sub get_by_name {
>+    
>+    my ($name) = @_;
>+
>+    my $dbh = Bugzilla->dbh;
>+    trick_taint($name);
>+}

Same remark.


>+sub get_by_name {
>+    
>+    my ($name) = @_;
>+    
>+    my $dbh = Bugzilla->dbh;
>+    trick_taint($name);
>+
>+    my $query = "SELECT * FROM products WHERE name = ?";
>+    my $product = $dbh->selectrow_hashref($query, undef, $name) || {};
>+
>+    _get_extra_fields($product) if $product->{'id'}; 
>+
>+    return %$product;
>+}

First, why || {} ? We never did it before. Second, why return %$product instead
of $product ? OK, maybe because I told you to write it like that. :) But be
consistent please and use return $product.


>+sub get_by_id {
>+
>+    my ($id) = @_;
>+    
>+    my $dbh = Bugzilla->dbh;
>+    detaint_natural($id);
>+
>+    my $query = "SELECT * FROM products WHERE id = ?";
>+    my $product = $dbh->selectrow_hashref($query, undef, $id) || {};
>+
>+    _get_extra_fields($product) if $product->{'id'}; 
>+
>+    return %$product;
>+}

same remark here.


>+sub get_group_controls {
>+
>+    my ($product_id) = @_;
>+    
>+    my $dbh = Bugzilla->dbh;
>+    detaint_natural($product_id);
>+
>+    my $query = qq{SELECT * FROM groups
>+                   INNER JOIN group_control_map
>+                         ON groups.id = group_control_map.group_id
>+                   WHERE group_control_map.product_id = ?
>+                   AND   groups.isbuggroup != 0
>+                   ORDER BY groups.name};
>+
>+    my $groups = $dbh->selectall_hashref($query, 'id', undef, ($product_id));
>+
>+    return $groups;
>+}

ORDER BY used with a hash?? Same remark as above.


>+sub _get_extra_fields {
>+    
>+    my ($product) = @_;
>+    
>+    $product->{'groups'} = get_group_controls($product->{'id'});
>+
>+    my $comp = Bugzilla::Component::get_by_product($product->{'id'});
>+    
>+    $product->{'has_components'} = 0;
>+    $product->{'has_components'} = $comp if %$comp;

$product->{'has_components'} = (defined $comp) ? 1 : 0;


>+        
>+    $product->{'classification'} =
>+        Bugzilla::Classification::get_by_id($product->{'classification_id'});
>+
>+    delete($product->{'classification_id'});

Keep it! It does not hurt and *may* be useful.


This is a very good first candidate anyhow! :)

Comment 9

[Christian Reis]

I've opened a branch for this work:

  kiko_product_admin_20050517-BRANCH (branch: 1.324.2)
  kiko_product_admin_20050517-BASE (revision: 1.324)

Tiago, could you also ensure that all the dependent bugs are marked here? I know
for instance that the templatization of editproducts.cgi should be indicated..

Comment 10

[Max Kanat-Alexander]

Comment on attachment 183844 [details] [diff] [review]
v3: separates the routines by context

>+sub get_by_name {

  Please don't have subroutines with identical names in different packages,
even if they're not exported. I would much prefer:

  classification_name_to_id
  classification_id_to_name
  get_all_classifications

  It would be different if these were objects with methods, but they're not.
Subroutines should have distinct names.

>+sub get_all {
>+
>+    my $dbh = Bugzilla->dbh;
>+    
>+    my $query = qq{SELECT classifications.*,

  Please don't use the *. Tables can change over time. When they do, we need to
grep through the sources to see where the old column was being used. This
function needs to return a very well-defined hash that doesn't depend on the
underlying table at all.

>+################################################################################
>+# Private Functions
>+################################################################################

  You don't need this section if there aren't any private functions.

>+# The Initial Developer of the Original Code is Netscape Communications
>+# Corporation. Portions created by Netscape are
>+# Copyright (C) 1998 Netscape Communications Corporation. All
>+# Rights Reserved.

  And yes, LpSolit is right about this -- this should be either Tiago or Async.

>+sub get_by_product {
>+
>+    my ($product_id) = @_;

  Nit: We usually don't have a line-break there, but put the params right after
the function.

>+    my $query = "SELECT * FROM components WHERE product_id = ?";

  In short, * should never be used. Please always specifically select the
columns, and return a well-defined hash.

  Also, all of the subroutines in all the files need POD docs that describe
exactly what they return, and exactly what their intended use is.

>+    $product->{'has_components'} = 0;
>+    $product->{'has_components'} = $comp if %$comp;

  That will die with "cannot use an undefined value as a hashref" if $comp is
undefined. Just check $comp. And you can reverse the two lines and do ||= 0 for
the second one.

Comment 11

[Max Kanat-Alexander]

And generally, please don't simultaneously move functions and also change their
functionality.

I saw no lines removed from globals.pl in the patch -- why?

Comment 12

[Max Kanat-Alexander]

Oh, and one other nit that I've noticed now that I'm looking at my email: make
the comment lines that are all hashmarks only 70 characters long. It prevents
them from wrapping in my email, at the least, and it's what I've been doing
everywhere else.

Comment 13

[Frédéric Buclin]

(In reply to comment #12)
> Oh, and one other nit that I've noticed now that I'm looking at my email: make
> the comment lines that are all hashmarks only 70 characters long. It prevents
> them from wrapping in my email

Huh? I don't think because it would prevent them from wrapping in your email
that it is a valid reason to limit these lines to 70 characters. But I agree
that lines should not be too long, typically 72 charaters. Simply because this
is a convention and not because of mkanat's mailbox? ;)

Two more comments about mkanat's review:

- Flag.pm and FlagType.pm also use the same function names. They are not
exported so this is fine for me.

- We don't want these something_name_to_id anymore.

- PODS can come later. We created a branch were we will implement things
incrementally. Note: This branch is not under your control! :-p

- Some routines are still in globals.pl because they are still in use. We will
remove them as soon as we are ready to change other .cgi files.

- I see no problem using SELECT * for now. Maybe we will change this later; not
sure.

Comment 14

[Max Kanat-Alexander]

(In reply to comment #13)
> - Flag.pm and FlagType.pm also use the same function names. They are not
> exported so this is fine for me.

  Flag and FlagType.pm aren't used in as many places as these functions will be
used.

> - We don't want these something_name_to_id anymore.

  They are going to go away anyhow, once Product, Component, and Classification
all become standard objects. You don't want to re-write all of these functions
and then have to re-write all of them again during the 2.22 development cycle.

> - PODS can come later. We created a branch were we will implement things
> incrementally. 

  I'd like to see PODs in the initial version of this that we check-in. It's too
easy to say that we'll implement them later, but very unlikely that it will
actually happen.

> Note: This branch is not under your control! :-p

  Fred, nothing is "under my control." I'm offering my review comments on how to
make good code that will make the lives of all future Bugzilla developers easier.

> - Some routines are still in globals.pl because they are still in use. We will
> remove them as soon as we are ready to change other .cgi files.

  OK, so we have these new ones that will be in use in the new files, and the
old ones will still be in use in the old files? That causes me some concern, but
I can't quite put my finger on why.

> - I see no problem using SELECT * for now. Maybe we will change this later; 
> not sure.

  I see a huge problem with it. It's absolutely not OK, for the reasons that
I've specified above. It is not future-proof, and even the case of the column
names returned can be DB-dependent. Subroutines need a well-defined, stable API,
and you can't get a well-defined API by just returning all the column names on a
table. I don't want to have to look at the DB tables every time I use a function
just to know what it's going to return.

  The goal of API design is to make things as simple as humanly possible, with
no possible ambiguity, and to do this in a way that is as unlikely to change as
possible.

Comment 15

[Christian Reis]

Keep lines under 72 chars unless impossible.

Don't use SELECT *, it's broken.

We'll move the calls in existing CGIs from globals.pl to using functions in this
file, but not all in this bug or it'll take forever. We can do a cleanup later,
I vouch someone at Async will do it if a reviewer is available to handle it.

Follow the POD requirement, all other modules have it.

The only remaining issue I have is with the naming of functions. Would we be
better off moving Product.pm to be an object module?

Comment 16

[Max Kanat-Alexander]

(In reply to comment #15)
> We can do a cleanup later,
> I vouch someone at Async will do it if a reviewer is available to handle it.

  OK, I totally agree with that. :-)

> The only remaining issue I have is with the naming of functions. Would we be
> better off moving Product.pm to be an object module?

  Yes, if you can, and you don't feel like it would cause too much other code
change. Ideally a Product, a Component, and a Classification would each be an
object class, and they'd all have similar APIs.

Comment 17

[Tiago Mello [:timello]]

Attachment: v4: chages from modules to classes

Just to see the proposed api. No need for a completely review.
Especially, take a look at Product.pm.

Comment 18

[Max Kanat-Alexander]

Comment on attachment 184046 [details] [diff] [review]
v4: chages from modules to classes

  OK, just some general API comments on Product.pm, below. I have other
comments, but I'll save them for a more detailed review.

  In general, I'm not totally comfortable with just using the database columns
as the names of the object fields, because I want the object APIs to be more
stable than the underlying database. However, I suppose that the names as they
exist are familiar, and I can't see anything directly wrong with the way it's
working.

>+my %fields = (

  I don't really understand exactly why we need this. If we do, it should
probably be a "use constant" and it can be dclone'd when needed. I don't think
we do anything similar in our other objects (Bug, User, User::Setting).

  Bugzilla::User::Setting is probably the best example of an object that we
have, at the moment, since it's the newest and went through a long review
cycle.

>+sub new {
>+    my $type = shift;
>+    my %class;

  I'm slightly confused, here -- usually we call $type "$class." And what's the
%class doing here?

>+    if ($#_ == 0) {
>+        $self->init_product($self->get_product_by_name(@_));
>+    }

  Normally we want to create objects by their ID, not by their name. Or at
least, that's the case with the other type of objects that we have. Sometimes
we give a choice, but I think that a product could be named with all digits...

>+sub init_product {

  This is usually called _init.

>+sub get_product_by_name {

  These look like private helpers for init, to me... or at least private
helpers to new(). They should start with an underscore if they're not meant to
be used outside of the package.

>+    $self->init_product($product);

  It looks to me like init_product is just doing the work of dclone.

>+sub make_product {

  I think we've been calling subroutines like this "add_" in other modules.

  Also, how are you going to get a Product object to call this function on?
Usually things like this are a subroutine, or there's some simple way to make
an object in the DB that doesn't involve knowing the internals of the object.

  Otherwise, this subroutine should probably be called "write_to_db" or
something like that, since it's not actually *making* a Product, just writing
the object to the database. And then it should also have an UPDATE function for
when the product is being changed.

>+sub update_product {
>+    my $self = shift;
>+    my ($updated_fields) = @_;

  It looks like $updated_fields isn't being used.

>+    my $sth = $dbh->prepare($query);
>+    $self = $self->detaint_fields;

  Actually replacing $self is probably not the best idea for code clarity...
maybe just have another var named $detainted_self or something?

>+sub get_all_products {
>+    my ($class_id) = @_;
>+    my $dbh = Bugzilla->dbh;
>+    detaint_natural($class_id);

  Every time you call a detainting function, you should probably throw an error
if it fails.

>+                      disallownew = 0 AS status,

  That isn't ANSI SQL, and not PostgreSQL-safe -- it needs to be a CASE WHEN
statement instead.

>+                      votesperuser,  maxvotesperbug, votestoconfirm,
>+                      COUNT(bug_id) AS bug_count
>+                 FROM products
>+                 INNER JOIN classifications
>+                   ON classifications.id = products.classification_id
>+                 LEFT JOIN bugs ON products.id = bugs.product_id
>+                 WHERE classifications.id = ? } .
>+                 $dbh->sql_group_by('products.name',
>+                                    'products.description, disallownew,
>+                                     votesperuser, maxvotesperbug,
>+                                     votestoconfirm');
>+                 
>+    my $products = $dbh->selectall_hashref($query, 'name', undef,
>+                                           ($class_id));

  This looks like you're creating a different object, which is actually a
ProductList. The only problem with doing it this way is that you won't be able
to call any functions on the returned hashes, because they won't be objects.

>+sub get_group_controls {

  This sub looks good to me, except I think it should be called on a Product
object, and cached inside that object. (And should be called group_controls.)

  For details on what I mean by "cached," look at how Bugzilla::User works. In
general, most Bugzilla objects should work that way, where instead of ever
directly calling a field value, you call a subroutine that returns that value.
That gives us more flexibility in the future. If you want to use an AUTOLOAD to
accomplish that (like Bugzilla::Bug does), instead of writing out all the
subroutines... I wouldn't object, but I also don't think it's an ideal
solution. But it can be easier to write, so I understand that.

>+sub get_product_extra_fields {
>+    my ($product) = @_;

  You have the product_id in $self when you're calling this, so you might as
well make it a method.

  Also, I think that this function shouldn't exist, and instead all of the
functions that it calls to populate fields should be functions within this .pm
itself, that are cached like I wanted for group_controls.

>+sub get_versions {

  Same for this.

>+sub get_milestones {

  And this.

>+sub make_version {

  I'd call it add_version, but that's just a nit. Do you think that this method
should also check if the specified version already exists? I think that might
make things easier for callers.

>+sub make_milestone {

  Same comments here.

>+=head1 NAME
>+
>+Bugzilla::Product - A module to deal with Bugzilla products values.

  And I'd like to see this inline, like it is for the other modules. :-)

>+sub current_timespamp {
>+    my ($self) = @_;
>+    my ($timestamp) = $self->selectrow_array('SELECT NOW()');

  I was thinking of making a similar function, actually. :-) Seems like a good
idea. You could even cache a prepared $sth for it, so that multiple calls in a
tight loop would be fast.

Comment 19

[Tiago Mello [:timello]]

Attachment: v5: changes the Product.pm api.

Take a look if is something like this. Max, please, forget the POD right now.
If you agree with this propose, I'll atach the complete one.

Comment 20

[Joel Peshkin]

Looks like a reasonable direction.

A few questions...

1) When we call update(), will it update everything including the group
controls, components, etc???    Also, should that return a list of what was
changed so that the next CGI page can list the changes made?

2) When you do get to group controls, will you be including enforment of the new
rules on the existing bugs in the update() function?

This may make the datatype returned by update() rather interesting.

Comment 21

[Tiago Mello [:timello]]

(In reply to comment #20)
> 1) When we call update(), will it update everything including the group
> controls, components, etc???    Also, should that return a list of what was
> changed so that the next CGI page can list the changes made?
  Humm, I'm afraid of that, because the cgis are different, no? I can't update 
components in the editproducts, for instance. Then we only will make calls on
product object?, and what about the others classes? I'm thinking something like:

In the editcomponents:
my $product = new Bugzilla::Product({name=>'foo'});
my $component = new Bugzilla::Component({name=>'bar', product_id => $product->id});
$component->name('BarF00');
$component->write_to_db();
....
But, does it make sense write_to_db returns a list of updated fields?
....
Maybe, actually, leave update and insert to the callsite? It will be less
complex, IMO!
....

> 2) When you do get to group controls, will you be including enforment of the new
> rules on the existing bugs in the update() function?
   Not yet but I can create a Bugzilla::Group::UpdateGroupControls(...) that
will do that. Like GroupControls that returs to me all the product group
contros. Reasonable?

> This may make the datatype returned by update() rather interesting.
  Sure, but the code that I attached uses update and insert as a private method
that is used by write_to_db.

Sorry if I didn't understand something.

Comment 22

[Christian Reis]

(In reply to comment #20)
> 1) When we call update(), will it update everything including the group
> controls, components, etc???    Also, should that return a list of what was
> changed so that the next CGI page can list the changes made?

Group controls, surely. Components is a bit trickier, because I think most of
the time you will be doing

  $component = $product->create_component()
 
or direct manipulation on $component and then $component->update().

> 2) When you do get to group controls, will you be including enforment of the new
> rules on the existing bugs in the update() function?
> 
> This may make the datatype returned by update() rather interesting.

Well, what sort of list of changes do we want -- just a list of strings?

Comment 23

[Max Kanat-Alexander]

> (In reply to comment #20)
>    Not yet but I can create a Bugzilla::Group::UpdateGroupControls(...) that
> will do that. Like GroupControls that returs to me all the product group
> contros. Reasonable?

  I think that sounds quite reasonable, but would it be overkill for this bug?
It would be nice to keep this bug as focused as possible. Otherwise the review
period will keep getting longer and longer. That might be inevitable, though,
and we have some time before we unfreeze, anyway...

(In reply to comment #22)
> Well, what sort of list of changes do we want -- just a list of strings?

  For diffs, I think the best format is a hash that looks like:

  fieldname => [$old_value, $new_value](In reply to comment #21)

Comment 24

[Tiago Mello [:timello]]

Attachment: v6: product, components, classification, version and milestone classes whitout insert, update and deletes methods.

Let's try review, land these classes and finish the discussion about api.

Comment 25

[Tiago Mello [:timello]]

So, my plan is:
1. Land the proposed api; No need for all routines, I think just the routines
that define the data structure;
2. Start to move the code from editproduct, edit*, with one bug for each edit*;
Now, we could move the insert, updates and deletes routines;
3. Start the templatizing editproduct first and then edit*, with one bug for
each edit*;

Sounds good? Who could help me on this review?

Comment 26

[Frédéric Buclin]

Why these AUTOLOAD sections?

Comment 27

[Tiago Mello [:timello]]

Attachment: v7: without AUTOLOADs

Here it is.

Comment 28

[Frédéric Buclin]

Comment on attachment 184868 [details] [diff] [review]
v7: without AUTOLOADs

per my "live" review on IRC.

Comment 29

[Tiago Mello [:timello]]

Attachment: v8: cleanup the patch.

Lets continue. :)

Comment 30

[Frédéric Buclin]

Comment on attachment 184880 [details] [diff] [review]
v8: cleanup the patch.

>+my @db_columns = qw(
>+    id          
>+    name        
>+    description 
>+);

Before I forget (this is not a full review yet; I'll try to do it later today).
Change these fields to classifcations.id, classifications.name and
classifications.description so that you can use @db_columns in SQL queries
using several tables.


>+    # Loads the classification with id or name. 
>+    if (defined $id) {
>+        $query = qq{SELECT $columns FROM classifications
>+                     WHERE id = ?};
>+        detaint_natural($id);
>+        $data = $id;

if (detaint_natural($id)) is better.


>+    foreach my $key (keys(%$classifications)) {
>+        $classifications->{$key} = 
>+            bless($classifications->{$key},
>+                  "Bugzilla::Classification");
>+    }

Don't use bless() but new() instead...

Comment 31

[Tiago Mello [:timello]]

(In reply to comment #30)
> >+    foreach my $key (keys(%$classifications)) {
> >+        $classifications->{$key} = 
> >+            bless($classifications->{$key},
> >+                  "Bugzilla::Classification");
> >+    }
> 
> Don't use bless() but new() instead...

Ok, so I'm following the User/Setting.pm, ok? With something like this:

sub new {
    my $invocant = shift;
    my $classification_id = shift;

    my $class = ref($invocant) || $invocant;

    my $self = {};
    bless($self, $class);

    my $dbh = Bugzilla->dbh;
    my $columns = join(", ", @db_columns);

    # There was only one parameter passed in.
    if (scalar @_ == 0 && defined $classification_id) {

        return $self unless (detaint_natural($classification_id));

        my $query = qq{SELECT $columns FROM classifications
                       WHERE id = ?};

        my $cl = $dbh->selectrow_hashref($query, undef,
                                         $classification_id);
        foreach my $field (keys(%$cl)) {
            $self->{$field} = $cl->{$field};
        }
    } else {
        # Values where passed in.
        $self->{'id'} = $classification_id;
        $self->{'name'} = shift;
        $self->{'description'} = shift;
    }
    return $self;
}

Comment 32

[Frédéric Buclin]

(In reply to comment #31)
> Ok, so I'm following the User/Setting.pm, ok?

No, I'm not. new() is fine actually. I see no reason to change it.

Comment 33

[Frédéric Buclin]

Comment on attachment 184880 [details] [diff] [review]
v8: cleanup the patch.

Again a live review. Sorry for all of you who would like to see the comments.
but this file is still in alpha stage. The next patch will in a beta stage and
reviews will be in the bug. ;)

Comment 34

[Tiago Mello [:timello]]

Attachment: v9: patch reviewed.

Here we go!

Comment 35

[Tiago Mello [:timello]]

Attachment: v10: patch reviewed. (again)

Sorry for the spam.

Comment 36

[Tiago Mello [:timello]]

Attachment: v11: backing to the older constructor and some code adjusts

Comment 37

[Frédéric Buclin]

Comment on attachment 185308 [details] [diff] [review]
v11: backing to the older constructor and some code adjusts

>Index: Bugzilla/Classification.pm

>+    foreach my $field (keys(%$classification)) {
>+        $self->{$field} = $classification->{$field};
>+    }

Nit: I wonder if $self->{'exists'} would be useful, assuming an invalid ID or
name is passed to the object.


>+sub product_count {
>+    my $self = shift;
>+    my $dbh = Bugzilla->dbh;
>+
>+    return unless $self->{'id'};
>+    return unless detaint_natural($self->{'id'});

No need to detaint ID as it comes from the DB. So this second line is useless.
We have here the answer to my previous question. Instead of testing
$self->{'exists'}, you have tested $self->{'id'} which is fine too.


>+        $self->{'product_count'} = $dbh->selectrow_array(qq{
>+            SELECT COUNT(classification_id) AS product_count
>+            FROM products
>+            WHERE classification_id = $self->{'id'}
>+        });

First: there is no need to name your count AS product_count. Here an array is
returned, not a hash. Second, please move $self->{'id'} out of the query
itself, using: "... WHERE classification_id = ?", undef, $self->{'id'}.


>+sub get_all_classifications {
>+    my $dbh = Bugzilla->dbh;
>+    my $query = q{SELECT classifications.id FROM classifications};
>+
>+    my $classifications = $dbh->selectall_hashref($query, 'id',
>+                                                  undef, ());

It's the first time here that you use $query. It's not useful. Moreover, as the
request use only one table, write "SELECT id FROM classifications". Also, use
selectcol_arrayref which will return an array of IDs which is exactly what we
want. I see no interest in having a hash here.


>+    foreach my $key (keys(%$classifications)) {

With my comment above, we get "foreach my $id (@ids)". Use $id, @ids which make
things clear of what they contain.



>Index: Bugzilla/Component.pm

>+use Bugzilla;
>+use Bugzilla::Util;
>+
>+my @db_columns = qw(
>+    components.id
>+    components.name
>+    components.product_id
>+    components.initialowner
>+    components.initialqacontact
>+    components.description
>+);
>+
>+my $columns = join(", ", @db_columns);
>+
>+sub new {
>+    my $invocant = shift;
>+    my $class = ref($invocant) || $invocant;
>+    my $self = {};
>+    bless($self, $class);
>+    $self->_init(@_);
>+    return $self;
>+}
>+
>+sub _init {
>+    my $self = shift;
>+    my ($id, $params) = (@_);
>+    my $dbh = Bugzilla->dbh;
>+
>+    my $component;
>+
>+    # Default param: component id.
>+    # Optional hash param can be passed in.
>+
>+    if (defined $params->{'product_id'} &&
>+        detaint_natural($params->{'product_id'}) &&
>+        defined $params->{'name'}) {

Nit: usually, we move '&&' and '||' on a new line:
if (defined ()
    && detaint_natural()
    && defined()) {


>+        $component = $dbh->selectrow_hashref(qq{
>+            SELECT $columns FROM components
>+            WHERE LOWER(name) = LOWER(?)
>+            AND   product_id = ?}, undef,
>+            ($params->{'name'}, $params->{'product_id'}));

Nit: Not sure we need LOWER, at least using MySQL. I don't know about
PostgreSQL.

>+
>+    } elsif (defined $id && detaint_natural($id)) {

Nit: As $id is the primary key for components, I would give it a higher
priority, that is, checking if $id is defined first; and if not, then check if
its name and product ID are defined.


>+sub get_all_components {
>+    my ($product_id) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    return {} unless detaint_natural($product_id);
>+
>+    my $query = qq{SELECT id FROM components
>+                   WHERE product_id = ?};

Same comments as above: $query not useful.


>+
>+    my $components = $dbh->selectall_hashref($query, 'id', undef,
>+                                             ($product_id));

Use selectcol_arrayref() instead.


>+    foreach my $key (keys(%$components)) {

Following my previous comment: foreach my $id (@ids)



>Index: Bugzilla/Group.pm

>+sub MakeProductGroup {
>+    my ($product_id, $product_name) = @_;
>+    my $product_group = $product_name;
>+    my $description = "Access to bugs in the $product_name product";
>+    my $dbh = Bugzilla->dbh;
>+
>+
>+    trick_taint($product_group);
>+    trick_taint($description);

If you detaint $product_name before assigning it to $product_group, you don't
need to detaint $description or $product_group.


>+    # Gets the group id by the group name

Nit: get the 'admin' group ID.


>+    # Given "admin" group privileges.

I don't understand this sentence.


>+    $query = "INSERT INTO group_group_map (
>+                member_id, grantor_id, grant_type
>+              ) VALUES (?, ?, ?)";
>+
>+    $dbh->do($query, undef, $admin, $gid, GROUP_MEMBERSHIP);
>+    $dbh->do($query, undef, $admin, $gid, GROUP_BLESS);
>+    $dbh->do($query, undef, $admin, $gid, GROUP_VISIBLE);

$dbh->do() is used for non-repeated statements. In your case, what you want is:
my $sth = $dbh->prepare("INSERT INTO group_group_map
			 (member_id, grantor_id, grant_type)
			 VALUES (?, ?, ?)");
$sth->execute($admin, $gid, GROUP_MEMBERSHIP);
$sth->execute($admin, $gid, GROUP_BLESS);
$sth->execute($admin, $gid, GROUP_VISIBLE);


>+
>+    detaint_natural($product_id);
>+    $dbh->do(q{INSERT INTO group_control_map (
>+                group_id, product_id, entry,
>+                membercontrol, othercontrol, canedit
>+              ) VALUES (?, ?, ?, ?, ?, ?)}, undef,
>+             ($gid, $product_id, Param('useentrygroupdefault'),
>+              CONTROLMAPDEFAULT, CONTROLMAPNA, 0));

What happens at this stage if detaint_natural($product_id) fails?
Moreover, I don't like the way you use parens. Could you write something like:
$dbh->do(q{INSERT INTO group_control_map
	   (group_id, product_id, entry,
	    membercontrol, othercontrol, canedit)
	    VALUES (?, ?, ?, ?, ? ,?)},
	    undef, ($gid, ..., CONTROLMAPNA, 0));


>+sub GroupControls {

I would prefer GetGroupControls().


>+    my ($product_id) = @_;
>+    my $dbh = Bugzilla->dbh;
>+    my $query = qq{SELECT
>+                       groups.id, groups.name, groups.description,
>+                       groups.isbuggroup, groups.last_changed,
>+                       groups.userregexp, groups.isactive,
>+                       group_control_map.entry,
>+                       group_control_map.membercontrol,
>+                       group_control_map.othercontrol,
>+                       group_control_map.canedit
>+                  FROM groups
>+                  INNER JOIN group_control_map
>+                        ON groups.id = group_control_map.group_id
>+                  WHERE group_control_map.product_id = ?
>+                  AND   groups.isbuggroup != 0
>+                  ORDER BY groups.name};
>+    my $groups = $dbh->selectall_hashref($query, 'id', undef,
>+                                         ($product_id));
>+    return $groups;
>+}

You told me on IRC that this function was taken from editproducts.cgi. Sorry, I
cannot find it! But this routine looks correct.



>Index: Bugzilla/Milestone.pm

>+sub add_milestone {
>+    my ($value, $product_id) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    return unless (defined $value && defined $product_id);
>+    return unless (detaint_natural($product_id));

Write only one 'return' combining both above.


>+    $dbh->do(q{INSERT INTO milestone (product_id, value)
>+               VALUES (?, ?)}, undef, ($product_id, $value));

First: 'milestones' with 's'. Second: you forgot the milestone sortkey.


>+sub get_all_milestones {

>+    my $query = qq{SELECT $columns FROM milestones
>+                   WHERE product_id = ?};

Useless $query.


>+    my $milestones = $dbh->selectall_hashref($query, 'value', undef,
>+                                             ($product_id));
>+    foreach my $key (keys(%$milestones)) {

Same remark as before. Use an array instead of a hash.


>+        $milestones->{$key} = new Bugzilla::Milestone(
>+            $milestones->{$key}->{'value'},
>+            $milestones->{$key}->{'product_id'}
>+        );

Nit: Intuitively, I would set the product before the value.



>Index: Bugzilla/Product.pm

>+use Bugzilla;
>+use Bugzilla::Component;
>+use Bugzilla::Classification;
>+use Bugzilla::Version;
>+use Bugzilla::Milestone;
>+
>+use Bugzilla::Util;
>+use Bugzilla::Group;
>+use Bugzilla::Error;


>+sub _init {
>+    my $self = shift;
>+    my ($id, $params) = @_;

A product is identified either by its ID or by its name. So please write ($id,
$name).


>+    trick_taint($params->{'name'}) if defined $params->{'name'};

Don't write it here. Move it to the 'if (defined $name)' block below.


>+    if (defined $params->{'name'}) {
>+
>+        $product = $dbh->selectrow_hashref(qq{
>+            SELECT $columns FROM products
>+            WHERE LOWER(name) = LOWER(?)}, undef, $params->{'name'});

This is the IF block I'm talking about above.


>+    } elsif (defined $id && detaint_natural($id)) {
>+
>+        $product = $dbh->selectrow_hashref(qq{
>+            SELECT $columns FROM products
>+            WHERE id = ?}, undef, $id);
>+    }

The product ID is the primary key. Check for it first, then for its name.


>+sub write_to_db {
>+    my $self = shift;
>+
>+    return unless $self->_validate_data();
>+
>+    # Creates a new product.
>+    if (!defined $self->{'id'}) {

How do you handle a product which has no ID? By name? If yes, are you sure this
name is unique? Or is it done by _validate_data()?


>+        $self = $self->_insert();
>+        my $version = $self->{'version'} || 'unspecified';

This is not the job of Product.pm to determine the default version. This should
go into Version.pm. A developer who wants to change this default value won't
check it in Product.pm.


>+        Bugzilla::Version::add_version($version, $self->{'id'});
>+        Bugzilla::Milestone::add_milestone($self->{'defaultmilestone'});

add_milestone() has missing parameters: in which product has it to be added?
With which sortkey?


>+sub _validate_data {
>+    my $self = shift;
>+
>+    # Checks if the fields were defined and detaints all them.
>+    my @undefined_fields;
>+    foreach my $field (@db_columns) {
>+         my ($column_table, $column_name) = split('.', $field);
>+         if (!defined $self->{$column_name} && $column_name ne 'id') {
>+            ThrowCodeError('undefined_product_fields',
>+            {fields => join(', ', @undefined_fields)});
>+         }
>+    }

This won't work. ThrowCodeError will interrupt the loop as soon as a missing
field is detected. Moreover, unchecked checkboxes are not returned by the
template so you can get undefined values not because there is an error
somewhere, but because some checkboxes were not checked.

Don't care too much about this validation routine, I will write it myself as
part of another bug I'm working on.


>+sub _insert {
>+    my $self = shift;
>+    my $dbh = Bugzilla->dbh;
>+
>+    my $query = qq{INSERT INTO products (
>+                   name, description, milestoneurl, disallownew,
>+                   votesperuser, maxvotesperbug, votestoconfirm,
>+                   defaultmilestone, classification_id
>+                 ) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)};

Nit: same comment as above about parens.


>+sub classification {
>+    my $self = shift;
>+    return {} unless ($self->{'id'});
>+
>+    if (!defined $self->{'classification'}) {
>+        $self->{'classification'} = new
>+            Bugzilla::Classification($self->{'classification_id'});
>+    }
>+    return $self->{'classification'};

Nit: I would prefer 'new' on the same line as Bugzilla::Class(...)


>+sub bug_count {
>+    my $self = shift;
>+    my $dbh = Bugzilla->dbh;
>+
>+    return unless $self->{'id'};
>+    return unless detaint_natural($self->{'id'});

No need to check if ID is valid as it is returned by 'new'. So the second line
is useless. The first one is only here to make sure the product exists.


>+        $self->{'bug_count'} = $dbh->selectrow_array(qq{
>+            SELECT COUNT(bug_id) FROM bugs
>+            WHERE product_id = $self->{'id'}
>+        });

Use the $dbh->selectrow_array("... = ?", undef, $self->{'id'}) notation.


>+sub get_all_products {
>+    my ($class_id) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    return {} unless(detaint_natural($class_id));
>+
>+    my $query = qq{SELECT products.id FROM products
>+                   WHERE classification_id = ?};
>+
>+    my $products = $dbh->selectall_hashref($query, 'id', undef,
>+                                           ($class_id));
>+    foreach my $key (keys(%$products)) {
>+        $products->{$key} = new Bugzilla::Product($key);
>+        $products->{$key}->bug_count();
>+    }
>+    return $products;
>+}

Same comments as previous get_all_* routines.



>Index: Bugzilla/Version.pm

>+sub add_version {
>+    my ($value, $product_id) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    return unless (defined $value && defined $product_id);
>+    return unless (detaint_natural($product_id));

Use only one combined return.


>+sub update_version {
>+    my $self = shift;
>+    my ($new_value) = @_;
>+    my $dbh = Bugzilla->dbh;
>+    trick_taint($new_value);
>+
>+    $dbh->bz_lock_tables('bugs WRITE', 'versions WRITE');
>+
>+    # Updating all bugs with the new version.
>+    $dbh->do(q{UPDATE bugs SET version = ? WHERE version = ?
>+               AND product_id = ?}, undef,
>+             ($new_value, $self->{'value'}, $self->{'product_id'}));
>+
>+    # Updating the version.
>+    $dbh->do(q{UPDATE versions SET value = ? WHERE value = ?
>+               AND product_id = ?}, undef,
>+             ($new_value, $self->{'value'}, $self->{'product_id'}));

/me hates these tables with no primary key. This requires to update several
tables.


>+sub get_all_versions {
>+    my ($product_id) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    return {} unless detaint_natural($product_id);
>+
>+    my $query = qq{SELECT $columns FROM versions
>+                   WHERE product_id = ?};
>+
>+    my $versions = $dbh->selectall_hashref($query, 'value', undef,
>+                                             ($product_id));
>+    foreach my $key (keys(%$versions)) {
>+        $versions->{$key} = new Bugzilla::Version(
>+            $versions->{$key}->{'value'},
>+            $versions->{$key}->{'product_id'}
>+        );
>+    }
>+    return $versions;
>+}

Again, same comments as for the other get_all_* routines.



>Index: template/en/default/global/code-error.html.tmpl

>+  [% ELSIF error == "undefined_product_fields" %]
>+    Product field(s) '[% fields FILTER html %] was(were) not defined.

The way it actually works, this sentence will always be singular.

Comment 38

[Max Kanat-Alexander]

(In reply to comment #37)
> Nit: I wonder if $self->{'exists'} would be useful, assuming an invalid ID or
> name is passed to the object.

  My experience with Bug.pm shows that new() should return undef instead of a
valid object, if the object passed in was invalid. Either that or we should have
a parameter for throwing an error if the object is invalid.

> Nit: Not sure we need LOWER, at least using MySQL. I don't know about
> PostgreSQL.

  We have a function coming up for this, called sql_istring, which prepares a
string for case-insensitive comparison. However, every time we have a string
which needs to be case-insensitive, we have to add an extra index for PostgreSQL
if we want the query to be performant. Is this the only place we do a comparison
on component.name in SQL? We should do it here the same way we do it in other
places, and then we can fix them all later if necessary.

> >+        my $version = $self->{'version'} || 'unspecified';
> 
> This is not the job of Product.pm to determine the default version. This 
> should go into Version.pm. A developer who wants to change this default value 
> won't check it in Product.pm.

  Actually, instead, I think it should be a constant in Bugzilla::Constants.

Comment 39

[Frédéric Buclin]

(In reply to comment #38)
>   My experience with Bug.pm shows that new() should return undef instead of a
> valid object, if the object passed in was invalid.

I agree.


>   Actually, instead, I think it should be a constant in Bugzilla::Constants.

Hrm.... Something like DEFAULT_VERSION ? Yes, why not. I would fully agree with
you if the user was not allowed to change the default version when creating a
new product. But in our case, he can. So this is not a constant in the sense I
imagine a constant to be. That's why I suggested Version.pm.

Comment 40

[Tiago Mello [:timello]]

Attachment: v12: patch reviewed.

Comment 41

[Frédéric Buclin]

Comment on attachment 185492 [details] [diff] [review]
v12: patch reviewed.

>Index: Bugzilla/Classification.pm

>+    } else {
>+        $self->{'error'} = 'InvalidClassificationParamenters';
>+    }

Nit: Parameters, not Paramenters.
Are these $self->{'error'} really useful? I would like mkanat's opinion.


>+sub product_count {
>+    my $self = shift;
>+    my $dbh = Bugzilla->dbh;
>+
>+    return unless $self->{'id'};

Now that you have defined $self->{'error'}, why not using it here?
return unless $self->{'error'};


>+        $self->{'product_count'} = $dbh->selectrow_array(q{
>+            SELECT COUNT(classification_id) FROM products
>+            WHERE classification_id = ?}, undef, $self->{'id'});

NIT: COUNT(*) would work too.


>+sub get_all_classifications {
>+    my $dbh = Bugzilla->dbh;
>+
>+    my $ids = $dbh->selectcol_arrayref(q{
>+        SELECT id FROM classifications
>+    });

Nit: why }) on a new line?


>+    my %classifications;
>+    foreach my $id (@{$ids}) {

Nit: you can write @$ids.


>+        %classifications->{$id} = new Bugzilla::Classification($id);
>+        %classifications->{$id}->product_count();
>+    }

Is %classifications really a hash? I'd write $classifications.



>Index: Bugzilla/Component.pm

>+    } elsif (defined $params->{'product_id'}
>+        && detaint_natural($params->{'product_id'})
>+        && defined $params->{'name'}) {
>+
>+        trick_taint($params->{'name'});
>+
>+        $component = $dbh->selectrow_hashref(qq{
>+            SELECT $columns FROM components
>+            WHERE name = ?
>+            AND   product_id = ?}, undef,
>+            ($params->{'name'}, $params->{'product_id'}));
>+
>+        $self->{'error'} = 'InvalidComponent';
>+    } else {

$self->{'error'} *UNLESS* ...! Else you will always return an error ;)


>+    my %components;
>+    foreach my $id (@{$ids}) {
>+        %components->{$id} = new Bugzilla::Component($id);
>+    }
>+    return \%components;

Same comments about the hash and @$ids.



>Index: Bugzilla/Milestone.pm

>+    if (defined $product_id && detaint_natural($product_id) &&
>+        defined $value) {

Nit: write one test per line:
if (defined()
    && detaint_natural()
    && defined ())

>+
>+        trick_taint($value);
>+        $milestone = $dbh->selectrow_hashref(qq{
>+            SELECT $columns FROM milestones
>+            WHERE value = ?
>+            AND product_id = ?}, undef, ($value, $product_id));
>+        $self->{'error'} = 'InvalidMilestone';
>+    } else {

Again, $self->{'error'} *UNLESS* ...


>+    $sortkey = DEFAULT_SORTKEY unless ($sortkey);

$sortkey ||= DEFAULT_SORTKEY;


>+
>+    $dbh->do(q{INSERT INTO milestones (product_id, value, sortkey)
>+               VALUES (?, ?, ?)}, undef, ($product_id, $value, $sortkey));

Where do you check that $sortkey is valid?


>+sub get_all_milestones {
>+    my ($product_id) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    return {} unless detaint_natural($product_id);
>+
>+    my $query = qq{SELECT $columns FROM milestones
>+                   WHERE product_id = ?};
>+
>+    my $values = $dbh->selectcol_arrayref(q{
>+        SELECT value FROM milestones
>+        WHERE product_id = ?}, undef, $product_id);

$query is not used!


>+    my %milestones;
>+    foreach my $value (@{$values}) {
>+        %milestones->{$value} = new Bugzilla::Milestone($product_id,
>+                                                        $value);
>+    }
>+    return \%milestones;

Not a hash.



>Index: Bugzilla/Product.pm

>+    # The default param is the product id, but an optional
>+    # hash can be passed in.

This comment does not apply anymore. No hash can be passed.


>+sub write_to_db {
>+    my $self = shift;
>+
>+    #my $valid_product = $self->check_product();
>+
>+    #ThrowCodeError('...', {error => $self->{'error'})
>+    #   if ($self->{'error'});

What are these comments??? For debugging?


>+    $self = new Bugzilla::Product($dbh->bz_last_key('products', 'id'));

Nit: move $dbh->bz_last_key() out of this.


>+sub _update {
>+    my $self = shift;
>+    my $dbh = Bugzilla->dbh;
>+
>+    my $query = q{UPDATE products
>+                 SET
>+                    name              = ?,
>+                    description       = ?,
>+                    milestoneurl      = ?,
>+                    disallownew       = ?,
>+                    votesperuser      = ?,
>+                    maxvotesperbug    = ?,
>+                    votestoconfirm    = ?,
>+                    defaultmilestone  = ?,
>+                    classification_id = ?
>+                 WHERE
>+                    id = ?};
>+
>+    $dbh->do($query, undef, ($self->{'name'},
>+                             $self->{'description'},
>+                             $self->{'milestoneurl'},
>+                             $self->{'disallownew'},
>+                             $self->{'votesperuser'},
>+                             $self->{'maxvotesperbug'},
>+                             $self->{'votestoconfirm'},
>+                             $self->{'defaultmilestone'},
>+                             $self->{'classification_id'}));

WHERE id = which product_id ??? You forgot a parameter.


>+sub components {
>+    my $self = shift;
>+    return {} unless $self->{'id'};

Haven't you defined $self->{'error'} ? If you don't use them, then remove them
completely.


>+    my %products;
>+    foreach my $id (@{$ids}) {
>+        %products->{$id} = new Bugzilla::Product($id);
>+        %products->{$id}->bug_count();
>+    }
>+    return \%products;

Same comments.



>Index: Bugzilla/Version.pm

>+    $value = DEFAULT_VERSION unless $value;

$value ||= DEFAULT_VERSION;


>+    my $query = qq{SELECT $columns FROM versions
>+                   WHERE product_id = ?};
>+
>+    my $values = $dbh->selectcol_arrayref(q{
>+        SELECT value FROM versions
>+        WHERE product_id = ?}, undef, $product_id);

$query is never used.


>+    my %versions;
>+    foreach my $value (@{$values}) {
>+        %versions->{$value} = new Bugzilla::Version($product_id,
>+                                                    $value);
>+    }
>+    return \%versions;

Same comment about %versions.

Comment 42

[Frédéric Buclin]

Now that you have defined $self->{'error'}, why not using it here?
return unless $self->{'error'};

I meant: return if $self->{'error'}

Comment 43

[Tiago Mello [:timello]]

Attachment: v13: patch reviewed.

mkanat Could you take a look on $self->{'error'} propose?
LpSolit, the only change out of yours suggestions was the check_sortkey routine
in the Milestone.pm. Then, the v14 will be the last for r+, right?

Comment 44

[Max Kanat-Alexander]

Yeah. Right now Bug.pm uses $self->{'error'}, and it really sucks. It causes a
lot of hidden bugs, and it also causes something silly that you have to check
inside of every subroutine. If you forget to check it even once, you have a
security bug.

Instead of doing something like setting $self->{error}, new() should return
undef when something goes wrong with the object. Or even better, the function
should throw an error when something goes wrong during creation. You could
possibly provide an argument to new to skip the error, and just have it return
undef. But if new() did most error-checking itself, it would save us a lot of
validation and de-tainting elsewhere.

Comment 45

[Tiago Mello [:timello]]

Comment on attachment 185511 [details] [diff] [review]
v13: patch reviewed.

Hey LpSolit, What else we should change on this patch?

Comment 46

[Tiago Mello [:timello]]

Attachment: v14: release candidate 1

Comment 47

[Christian Reis]

Yeah, definitely -- new() should throw an exception if an error occurs. We
should encapsulate the error instead of spreading it out to callsites.

Comment 48

[Frédéric Buclin]

Comment on attachment 185573 [details] [diff] [review]
v14: release candidate 1

>Index: Bugzilla/Classification.pm

>+sub _init {
>+    my $self = shift;
>+    my ($id, $name) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    my $classification;
>+
>+    if (defined $id
>+        && detaint_natural($id)) {

Nit: short tests can be put on a single line.


>+        $classification = $dbh->selectrow_hashref(qq{
>+            SELECT $columns FROM classifications
>+            WHERE id = ?}, undef, $id);
>+
>+        return undef unless ($classification->{'id'});
>+
>+    } elsif (defined $name) {
>+
>+        trick_taint($name);
>+        $classification = $dbh->selectrow_hashref(qq{
>+            SELECT $columns FROM classifications
>+            WHERE name = ?}, undef, $name);
>+
>+        return undef unless ($classification->{'name'});
>+
>+    } else {
>+        return undef;
>+    }

"return undef unless ($classification->{'id'});" should be moved out of the
IF-ELSE block. This way, you don't need the last ELSE and you don't need to
repeat it 3 times! And please use the primary key when possible; here 'id'
instead of 'name'. This makes more sense to me.



>Index: Bugzilla/Product.pm

>+sub write_to_db {
>+    my $self = shift;
>+
>+    # Creates a new product.
>+    if (!defined $self->{'id'}) {
>+
>+        $self = $self->_insert();

Nit: Actually, we will never meet this criteria as 'id' is always defined when
the product object is created. We will need a method to create a newly non
existent product.


>+        Bugzilla::Version::add_version($self->{'version'},
>+                                       $self->{'id'});

add_version(product_id, version) <- you wrote arguments in the wrong order.


>+sub get_all_products {
>+    my ($class_id) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    return {} unless(detaint_natural($class_id));

We should accept $class_id to be undefined when classification is not used. I'd
add:
$class_id ||= DEFAULT_CLASSIFICATION_ID; where DEFAULT_CLASSIFICATION_ID = 1.


>+    foreach my $id (@$ids) {
>+        $products->{$id} = new Bugzilla::Product($id);
>+        $products->{$id}->bug_count();
>+    }

Don't include bug_count() by default as it is very time consuming. I would add
a new param such as '$wants_bug_count' and write:
$products->{$id}->bug_count() if $wants_bug_count;


>+sub update_version {
>+    my $self = shift;
>+    my ($new_value) = @_;
>+    my $dbh = Bugzilla->dbh;
>+    trick_taint($new_value);
>+
>+    $dbh->bz_lock_tables('bugs WRITE', 'versions WRITE');
>+
>+    # Updating all bugs with the new version.
>+    $dbh->do(q{UPDATE bugs SET version = ? WHERE version = ?
>+               AND product_id = ?}, undef,
>+             ($new_value, $self->{'value'}, $self->{'product_id'}));
>+
>+    # Updating the version.
>+    $dbh->do(q{UPDATE versions SET value = ? WHERE value = ?
>+               AND product_id = ?}, undef,
>+             ($new_value, $self->{'value'}, $self->{'product_id'}));
>+
>+    $self->{'value'} = $new_value;
>+    $dbh->bz_unlock_tables();
>+}

update_version() has no return $self;.

Comment 49

[Tiago Mello [:timello]]

Attachment: v15: Release Candidate 2

Comment 50

[Frédéric Buclin]

Comment on attachment 185616 [details] [diff] [review]
v15: Release Candidate 2

looks good. r=LpSolit

Comment 51

[Max Kanat-Alexander]

(In reply to comment #48)
> >+    foreach my $id (@$ids) {
> >+        $products->{$id} = new Bugzilla::Product($id);
> >+        $products->{$id}->bug_count();
> >+    }
> 
> Don't include bug_count() by default as it is very time consuming. I would add
> a new param such as '$wants_bug_count' and write:
> $products->{$id}->bug_count() if $wants_bug_count;

  Instead of that, just don't ever instantiate the bug_count by default. Callers
can get it if they want. It's silly to just call it here and throw away the result.

Comment 52

[Max Kanat-Alexander]

Comment on attachment 185616 [details] [diff] [review]
v15: Release Candidate 2

  For all of these, all fields should be accessed as subroutines instead of as
hash values. That is, instead of:

  $product->{id}

  We should be able to do:

  $product->id

  The best way to do this is either to provide individual subroutines for each
field (which I prefer) or to write an AUTOLOAD (which is easier to maintain but
more complex).

  For all my comments below, I've only noted things the first time they happen,
but they should be fixed everywhere.

  Also, all of these modules need acceptable POD docs before they are r+.

>Index: Bugzilla/Classification.pm

>+my @db_columns = qw(
>+    classifications.id
>+    classifications.name
>+    classifications.description
>+);

  This is a constant, so should be a "use constant" variable.

>+my $columns = join(", ", @db_columns);

  I don't like having a raw normal var out here, but if this is convenient, I
suppose it's OK. I think it might have to be "our" for mod_perl.

>+sub new {

  I like this; it's very clean. :-)

>+sub _init {
>+    my $self = shift;
>+    my ($id, $name) = @_;

  Instead of this, couldn't you just take a hash input, and assume it's an ID
unless otherwise stated? That is, if the passed-in parameter is a hash, then it
either has the key ID or the key NAME (but probably not in caps). I don't like
having two separate params for this, in any case.

>+        $classification = $dbh->selectrow_hashref(qq{
>+            SELECT $columns FROM classifications

  Nit: You could also just write the WHERE condition as a separate apended
string, and not have to write the base SQL twice. It's up to you.

>+    foreach my $field (keys(%$classification)) {

  Nit: We don't usually put parens around the argument to "keys."

>+sub product_count {
>+    my $self = shift;
>+    my $dbh = Bugzilla->dbh;
>+
>+    if (!defined $self->{'product_count'}) {
>+

  I'm not sure you need the blank line here.

>+sub get_all_classifications {
>+    my $dbh = Bugzilla->dbh;

  Eventually, these "all" methods should do SQL that pulls the data for all
classifications and inits them all at once, instead of doing a SQL hit for each
one.

  In fact, the best would be to have a ClassificationList, ProductList, and
ComponentList object. I'm also going to have a UserList and a BugList, so we
might want to come up with some standard interface for things like this.

>+    my $ids = $dbh->selectcol_arrayref(q{
>+        SELECT id FROM classifications});
>+
>+    my $classifications;
>+    foreach my $id (@$ids) {
>+        $classifications->{$id} = new Bugzilla::Classification($id);
>+        $classifications->{$id}->product_count();

  There's no need to run product_count() here.

>+# The Initial Developer of the Original Code is Netscape Communications
>+# Corporation. Portions created by Netscape are
>+# Copyright (C) 1998 Netscape Communications Corporation. All
>+# Rights Reserved.

  You wrote pretty much all this code yourself, so this really isn't true.

>+        $component = $dbh->selectrow_hashref(qq{
>+            SELECT $columns FROM components
>+            WHERE name = ?
>+            AND   product_id = ?}, undef,

  Nit: The AND is a *part* of the WHERE clause, so I usually space it like
this:

  WHERE name = ?
	AND product_id = ?

>+sub get_all_components {
>+    my ($product_id) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    return {} unless detaint_natural($product_id);

  Why are you returning an empty hashref instead of an empty list or undef?

  Also, we should throw an error here if we fail detaint_natural. That's a
CodeError -- we got a bad param.

>Index: Bugzilla/Group.pm

>+sub MakeProductGroup {

  This should be make_product_group. MixedCaseNames are only used for
deprecated functions, or functions that will be going away soon, usually. (At
least, inside of libraries. Inside of CGIs, I don't care as much.)

  And it should have a prototype, ($$), since it's not a method.

>+    my ($product_id, $product_name) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    return unless(detaint_natural($product_id));

  Once again, we should ThrowCodeError here, not return.

>+    trick_taint($product_name);
>+
>+    my $product_group = $product_name;
>+    my $description = "Access to bugs in the $product_name product";

  This should somehow be a constant or a template, ideally.

>+    my $query = q{SELECT id FROM groups WHERE name = ?};

  Nit: Having $query is unnecessary.

>+    # Gets the 'admin' group id.
>+    my $admin = $dbh->selectrow_array(qq{
>+        SELECT id FROM groups WHERE name = 'admin'
>+    });

  That should probably be a constant, ADMIN_GROUP_NAME. It can be local to this
.pm.

>+sub GetGroupControls {

  Should be named_like_this and have a prototype ($).

  This desperately needs some POD docs or a comment explaining its use.


>+                  FROM groups
>+                  INNER JOIN group_control_map
>+                        ON groups.id = group_control_map.group_id

  Will every group have an entry in group_control_map? I'm just not sure if
this should be a LEFT JOIN.

>Index: Bugzilla/Milestone.pm
>+package Bugzilla::Milestone;

  Let's make this Bugzilla::Field::Milestone, so that we can re-base it on
Bugzilla::Field when we have that (which is happening soon).

>+sub check_sortkey {

  This should have a prototype ($$) since it's not a method.

>Index: Bugzilla/Product.pm
>+sub write_to_db {
>+    my $self = shift;
>+
>+    # Creates a new product.
>+    if (!defined $self->{'id'}) {
>+        $self = $self->_insert();

  How would you ever get an object without an 'id' defined?

>+        Bugzilla::Version::add_version($self->{'id'},
>+                                       $self->{'value'});

  Um... wait, $self->{value} from the product? Don't you mean DEFAULT_VERSION
or something like that?

>+        Bugzilla::Milestone::add_milestone($self->{'id'},
>+                                           $self->{'defaultmilestone'});

  If you're adding a new product, isn't the default milestone actually coming
from somewhere else that the (nonexistent) product?

>+sub _update {
> [snip]
>+                 WHERE
>+                    id = ?};

  Nit: The "id = ?" can be on the same line.

>+sub get_all_products {
>+    my ($class_id, $wants_bug_count) = @_;

  And as I mentioned above $wants_bug_count isn't necessary.

>Index: Bugzilla/Version.pm
>+package Bugzilla::Version;

  Once again, I think we should do Bugzilla::Field::Version.

>+sub update_version {

  This is a method -- why isn't it just "update?"


  In general, everything that I didn't comment on looks great. :-) These are
going to be very useful modules. :-)

Comment 53

[Tiago Mello [:timello]]

(In reply to comment #52)
> >Index: Bugzilla/Classification.pm
> >+sub get_all_classifications {
> >+    my $dbh = Bugzilla->dbh;
> 
>   Eventually, these "all" methods should do SQL that pulls the data for all
> classifications and inits them all at once, instead of doing a SQL hit for each
> one.
> 
>   In fact, the best would be to have a ClassificationList, ProductList, and
> ComponentList object. I'm also going to have a UserList and a BugList, so we
> might want to come up with some standard interface for things like this.
> 
 Anyway, no changes for now, right?

>   Also, we should throw an error here if we fail detaint_natural. That's a
> CodeError -- we got a bad param.
Like:

    my $stored_product_id = $product_id;
    unless (detaint_natural($product_id)) {
        ThrowCodeError('invalid_parameter',
                       {param => $stored_product_id,
                        routine => 'get_all_components'});
    }
...

  [% ELSIF error == "invalid_parameter" %]
    [% title = "A bad parameter" %]
    The parameter '[% param %]' passed to '[% routine %]' is not valid.
?

> 
> >Index: Bugzilla/Group.pm


> >Index: Bugzilla/Milestone.pm
> >+package Bugzilla::Milestone;
> 
>   Let's make this Bugzilla::Field::Milestone, so that we can re-base it on
> Bugzilla::Field when we have that (which is happening soon).
  We really need this feature on this patch?

> >Index: Bugzilla/Product.pm
> >+sub write_to_db {
> >+    my $self = shift;
> >+
> >+    # Creates a new product.
> >+    if (!defined $self->{'id'}) {
> >+        $self = $self->_insert();
> 
>   How would you ever get an object without an 'id' defined?
    Will be:
    my $product = Bugzilla::Product::check_product($cgi);
    $product->write_to_db();
    However, LpSolit is finishing with the check_product routine;
         

> 
> >+        Bugzilla::Version::add_version($self->{'id'},
> >+                                       $self->{'value'});
> 
>   Um... wait, $self->{value} from the product? Don't you mean DEFAULT_VERSION
> or something like that?

   Sorry, $self->{'version'} instead.
> 
> >+        Bugzilla::Milestone::add_milestone($self->{'id'},
> >+                                           $self->{'defaultmilestone'});
> 
>   If you're adding a new product, isn't the default milestone actually coming
> from somewhere else that the (nonexistent) product?
 No, at this point, $self->{'defaultmilestone'} has the checked data because
check_product did that.

Comment 54

[Tiago Mello [:timello]]

mkanat, about accessors, is it ok do somethink like:
sub id { $_[0]->{'id'}; } ?

For all attributes?

Comment 55

[Tiago Mello [:timello]]

Attachment: v16: mkanat patch review.

Almost all bugs are fixed, only the POD docs and some stuffs that I commented
aren't in the patch.

Comment 56

[Max Kanat-Alexander]

(In reply to comment #54)
> mkanat, about accessors, is it ok do somethink like:
> sub id { $_[0]->{'id'}; } ?

  Yes, that's perfect! :-) That's what I usually do. Except add the "return"
statement explicitly.

(In reply to comment #53)
>  Anyway, no changes for now, right?

  Right.

>   [% ELSIF error == "invalid_parameter" %]
>     [% title = "A bad parameter" %]
>     The parameter '[% param %]' passed to '[% routine %]' is not valid.
> ?

  I think that it should be more specific on why it's not valid. Say it's not a
natural number, and also the name of the parameter.

>   We really need this feature on this patch?

  No. You can keep it as Bugzilla::Milestone for now, but it would keep the
libraries cleaner if it was in the Field/ directory. It's up to you.

>     Will be:
>     my $product = Bugzilla::Product::check_product($cgi);
>     $product->write_to_db();
>     However, LpSolit is finishing with the check_product routine;

  OK. Seems like it should be named something better than check_product. Perhaps
create_product.

>  No, at this point, $self->{'defaultmilestone'} has the checked data because
> check_product did that.

  OK, sounds good.

  I'll probably do the review tomorrow morning or evening; tonight I'm very tired.

Comment 57

[Frédéric Buclin]

Comment on attachment 185713 [details] [diff] [review]
v16: mkanat patch review.

>Index: template/en/default/global/code-error.html.tmpl

>+  [% ELSIF error == "invalid_parameter" %]
>+    [% title = "A bad parameter was passed" %]
>+    The parameter '[% param %]' passed to '[% routine %]' is not valid.

Don't forget to filter variables, usually using "FILTER html".

Comment 58

[Tiago Mello [:timello]]

Comment on attachment 185713 [details] [diff] [review]
v16: mkanat patch review.

mkanat, forget this patch, please, I'm fixing some bugs.

Comment 59

[Tiago Mello [:timello]]

Attachment: v17: Fixes for some bugs.

Comment 60

[Max Kanat-Alexander]

Comment on attachment 185788 [details] [diff] [review]
v17: Fixes for some bugs.

>Index: Bugzilla/Classification.pm
>
>+my $columns = join(", ", DB_COLUMNS);

  Thinking about it more, I'm pretty sure this has to be "our." Otherwise it
will break in mod_perl.

>+sub _init {
> [snip]
>+    if (defined $id && detaint_natural($id)) {
>+
>+        $classification = $dbh->selectrow_hashref(qq{
>+            SELECT $columns FROM classifications
>+            WHERE id = ?}, undef, $id);
>+
>+    } elsif (defined $param->{'name'}) {
>+
>+        trick_taint($param->{'name'});
>+        $classification = $dbh->selectrow_hashref(qq{
>+            SELECT $columns FROM classifications
>+            WHERE name = ?}, undef, $param->{'name'});
>+    }
>+
>+    return undef unless (defined $classification);

  You could theoretically get here if somebody provided an improper parameter.
I'd prefer a CodeError in that case. (A final "else" perhaps.)

>+            WHERE classification_id = ?}, undef, $self->{'id'});

  Try to use the accessors internally, also. So this would be $self->id. This
would need to be changed in a few places; I'll only mention it for this one.

>Index: Bugzilla/Component.pm
>
>+sub _init {

>Index: Bugzilla/Group.pm
>
>-# ValidateGroupName checks to see if ANY of the users in the provided list
>+use constant ADMIN_GROUP_NAME => 'admin';

  I'd like to export this. I think it would be useful in checksetup.pl, also,
at the least.

>+    while($dbh->selectrow_array(q{SELECT id FROM groups WHERE name = ?},
>+                                undef, ($product_group))) {

  Nit: You could prepare that and just run execute() on it in the loop. It's
not too concerning, though, since it's not run all that frequently.

>+    my $timestamp = $dbh->selectrow_array(q{SELECT NOW()});
>+
>+    $dbh->do(q{INSERT INTO groups
>+               (name, description, isbuggroup, last_changed)
>+               VALUES (?, ?, ?, ?)}, undef,
>+             ($product_group, $description, 1, $timestamp));

  You can just put NOW() directly into the SQL. You don't need to select it
separately.

>+    # Gets the 'admin' group id.
>+    my $admin = $dbh->selectrow_array(q{
>+        SELECT id FROM groups WHERE name = '}.ADMIN_GROUP_NAME.q{'

  Nit: Spaces before and after the periods. Also, you could pass
ADMIN_GROUP_NAME in through a placeholder, which would be better.

>+    # Grants to the admin group.
>+    my $sth = $dbh->prepare(q{
>+        INSERT INTO group_group_map
>+          (member_id, grantor_id, grant_type)
>+        VALUES (?, ?, ?)
>+    });
>+
>+    $sth->execute($admin, $gid, GROUP_MEMBERSHIP);
>+    $sth->execute($admin, $gid, GROUP_BLESS);
>+    $sth->execute($admin, $gid, GROUP_VISIBLE);

  I wonder... I think we should abstract this out into a separate sub, so that
we can use it generally. It's needed in other places, also. (I can at least
think of how it would be useful in checksetup.) Either that, or we should have
an add_group sub that a lot of this is abstracted into. We already have an
AddGroup in checksetup.pl that you could base it on. That's not totally
necessary, though.

>+sub get_group_controls ($) {
>+    # Gets all group controls of a specific product.

  When/if you POD doc this, you should note that this is *not* the recommended
way to get group controls, but instead a Product object should be instantiated
and you should call ->group_controls on it.

>Index: Bugzilla/Product.pm
> [snip]
>+#_versioo -*- Mode: perl; indent-tabs-mode: nil -*-

  versioo?

>+sub _insert {
> [snip]

  Looking at this, I'm realizing that we'll have to always take the return
value of write_to_db, even when we don't need it. I think that I was wrong
about having it as one function, and we really should have a separate insert()
(or add_) function, and another update function. Unless you can think of some
code examples now where it would be a lot better to have write_to_db.

  In fact, it really looks like we should just have an add_product sub, like we
do for other things.

>+sub id                { return $_[0]->{'id'};                }
>+sub name              { return $_[0]->{'name'};              }
>+sub description       { return $_[0]->{'description'};       }
>+sub milestoneurl      { return $_[0]->{'milestoneurl'};      }
>+sub disallownew       { return $_[0]->{'disallownew'};       }
>+sub votesperuser      { return $_[0]->{'votesperuser'};      }

  For perl code, we really ought to have the underscores here. So
votes_per_user as the sub name. Same for the other ones that are
squishedtogetherwords (and if you had a hard time reading that, that explains
why they should have underscores :-) ).

>Index: Bugzilla/Version.pm
>+sub update {
>+    my $self = shift;
>+    my ($new_value) = @_;
>+    my $dbh = Bugzilla->dbh;
>+    trick_taint($new_value);
>+
>+    $dbh->bz_lock_tables('bugs WRITE', 'versions WRITE');

  When you write the POD docs for this, make sure that you mention that it
locks tables.

>+sub add_version ($$) {
>+    my ($product_id, $value) = @_;
>+    my $dbh = Bugzilla->dbh;
>+
>+    return unless (defined $product_id
>+                   && detaint_natural($product_id));
>+
>+    $value ||= DEFAULT_VERSION;
>+    trick_taint($value);

  We should validate the version here, if that's necessary at all. Perhaps just
that it's not too long, and... what else?

>+sub get_all_versions ($) {

  I think this should be get_product_versions, since that would be a more
appropriate name. I think the other get_all functions should be named
similarly. I was confused when looking at them originally and thought that they
returned *all* of the things in the database, until I looked at how they were
used.

>Index: template/en/default/global/code-error.html.tmpl
>
>+  [% ELSIF error == "invalid_parameter" %]

  This shouldn't be "invalid_parameter" anymore, since it's specific to natural
numbers.

>+    [% title = "A bad parameter" %]

  Nit: That should probably just be "Bad Parameter."

>+    The parameter '[% param FILTER html %]' passed to '[% routine FILTER html %]'
>+    is not a natural number.

  Also display the value that was passed in.


  All in all, this code is starting to look really awesome. :-)

Comment 61

[Christian Reis]

(In reply to comment #60)
>   You could theoretically get here if somebody provided an improper parameter.
> I'd prefer a CodeError in that case. (A final "else" perhaps.)

Definitely, the final else should be asserted to never occur.

> >Index: Bugzilla/Component.pm
> >
> >+sub _init {

Was there anything you meant to say here?

> >-# ValidateGroupName checks to see if ANY of the users in the provided list
> >+use constant ADMIN_GROUP_NAME => 'admin';
> 
>   I'd like to export this. I think it would be useful in checksetup.pl, also,
> at the least.

Are we sure that this doesn't exist elsewhere, /and/ that it shouldn't be placed
in Bugzilla::Constants?

> >+    # Gets the 'admin' group id.
> >+    my $admin = $dbh->selectrow_array(q{
> >+        SELECT id FROM groups WHERE name = '}.ADMIN_GROUP_NAME.q{'
> 
>   Nit: Spaces before and after the periods. Also, you could pass
> ADMIN_GROUP_NAME in through a placeholder, which would be better.

If not, it should probably be quoted, as somebody will eventually break this.

>  I think that I was wrong  about having it as one function, and we really 
> should have a separate insert() (or add_) function, and another update
> function. 

It took a long time for /that/ wisdom to shine on you :-P From the start, hiding
the fact that we're adding or updating would probably only add complexity and
confuse anyone trying to understand what the callsite was doing. I agree
wholeheartedly. 

> >+sub add_version ($$) {
> >+    my ($product_id, $value) = @_;
> >+    my $dbh = Bugzilla->dbh;
> >+
> >+    return unless (defined $product_id
> >+                   && detaint_natural($product_id));
> >+
> >+    $value ||= DEFAULT_VERSION;
> >+    trick_taint($value);
> 
>   We should validate the version here, if that's necessary at all. Perhaps just
> that it's not too long, and... what else?

Don't names have restrictions on the use of certain characters? 

> >+sub get_all_versions ($) {
> 
>   I think this should be get_product_versions, since that would be a more
> appropriate name. 

Definitely so.

> >Index: template/en/default/global/code-error.html.tmpl
> >
> >+  [% ELSIF error == "invalid_parameter" %]
> 
>   This shouldn't be "invalid_parameter" anymore, since it's specific to natural
> numbers.
> 
> >+    [% title = "A bad parameter" %]
> 
>   Nit: That should probably just be "Bad Parameter."

I think we may prefer to use "argument" to parameter as it's more meaningful.
Parameter occurs in a number of places here.

Comment 62

[Tiago Mello [:timello]]

(In reply to comment #60)
> >+sub get_group_controls ($) {
> >+    # Gets all group controls of a specific product.
> 
>   When/if you POD doc this, you should note that this is *not* the recommended
> way to get group controls, but instead a Product object should be instantiated
> and you should call ->group_controls on it.
   
   I think that we really need this function only in the Product.pm, then it can
be moved from here.

Comment 63

[Max Kanat-Alexander]

I agree with all of Kiko's points.

(In reply to comment #62)
>    I think that we really need this function only in the Product.pm, then it can
> be moved from here.

  No, I'd rather it stayed in Group.pm. I'd like to try to localize SQL that's
related to a particular thing to its library. So SQL that does things to the
Group table really does belong in Group.pm.

Comment 64

[Tiago Mello [:timello]]

(In reply to comment #60)
> >+sub add_version ($$) {
>   We should validate the version here, if that's necessary at all. Perhaps just
> that it's not too long, and... what else?
  Max, could we do it in another bug? Because, first, we need a method for all
.pm that will check everything
(check_/create_[product,component,version,milestone,classification]), second, I
don't wanna write more bugs :(.

Comment 65

[Tiago Mello [:timello]]

(In reply to comment #61)
> >  I think that I was wrong  about having it as one function, and we really 
> > should have a separate insert() (or add_) function, and another update
> > function. 
> 
> It took a long time for /that/ wisdom to shine on you :-P From the start, hiding
> the fact that we're adding or updating would probably only add complexity and
> confuse anyone trying to understand what the callsite was doing. I agree
> wholeheartedly. 
Ok, So let me try something:

Callsite:
# Checks for everything.
my $checked_fields = Bugzilla::Product::check_product($cgi);
my $product = Bugzilla::Product::add_product($checked_fields);

...

Product::check_product($) {
   #if any error then ThrowUserError('any_error');
   # Returns a required checked fields for a product;
   $checked_fields->{'checked'} = 1;
   return $checked_fields;
}

Product::add_product($) {
   my ($checked_fields) = @_;

   if (defined $checked_fields->{'checked'}) {
     Adds the product...
   }
}

How does it looks?

Comment 66

[Christian Reis]

Arram. I guess I'm okay with check_product being publically available (though it
should be called check_product_data or something, since it doesn't really take a
product), but I don't think the callsite should be expected to call it --
instead,  Product's constructor should call it before blessing the new
reference. I don't see a need for add_product -- is there any?

Comment 67

[Tiago Mello [:timello]]

Attachment: v18: fixes.

Comment 68

[Tiago Mello [:timello]]

Splitting into 2 steps:

Step 1 (Read-only): Only the api, methods/subroutines that loads the object;

Step 2 (Write/Validations): Methods/Subroutines for write to database and
validations;

Step 3 (Templatize): Moves the code from callsites and templatizes all of them.

Comment 69

[Christian Reis]

I agree with the plan of action, but it really does have 3 steps.

Comment 70

[Tiago Mello [:timello]]

So mkanat, Can I remove all DB-persistence routines from this patch?

Comment 71

[Tiago Mello [:timello]]

Attachment: v19: removing DB-persistence routines.

Comment 72

[Joel Peshkin]

Comment on attachment 186496 [details] [diff] [review]
v19: removing DB-persistence routines.

r=joel
It'll get some serious enhancing in step2 and on...

Comment 73

[Max Kanat-Alexander]

Comment on attachment 186496 [details] [diff] [review]
v19: removing DB-persistence routines.

  OK! The code all looks good, from a brief scan.

  However, this needs POD docs before it can be r+.

>+  [% ELSIF error == "invalid_numeric_argument" %]
>+    [% title = "Invalid number argument" %]
>+    The argument <code>[% argument FILTER html %]</code> that contains the value
>+    <code>[% value FILTER html %]</code> passed to
>+    <code>[% function FILTER html %]</code> is not a natural number.

  Nit: The wording on this is a little awkward. I think it could be clearer,
somehow.

Comment 74

[Tiago Mello [:timello]]

Attachment: v20: adding pod docs.

Comment 75

[Christian Reis]

Comment on attachment 187055 [details] [diff] [review]
v20: adding pod docs.

Index: Bugzilla/Classification.pm
>+Classification.pm creates a classification object, which is a hash

  represents a Classification object.

It doesn't create anything yet.

I'd replace the ", which..." with

A Classification is a higher-level grouping of Bugzilla Products.

I'd also add here examples of accessing members of the classification (id,
products, product_count).

Use a real parameter in your example, too.

>+Set of database columns. It is used by functions that do database

It is to be used.

>+=item C<new($param)>
>+
>+Class's constructor.

Just Constructor maybe?

>+The constructor is used just to load an existent classification

s/just//

>+passing a classification id or classification name using a hash.

by passing

>+=item C<_init($param)>

I don't think internal methods need POD docs -- they document only the external
interface of the module.

>+=head1 ACCESSORS

I don't think these are necessary either.

>+=item C<product_count()>
>+
>+Returns the total of product that belongs to the classification.

This isn't an accessor, but a method.

>Index: Bugzilla/Component.pm
>+    my $id = $param unless (ref $param eq 'HASH');

I don't think this is necessary -- why don't you just use $param instead of $id
below?

>+    if (defined $id && detaint_natural($id)) {

The check for defined $id is a bit odd here. Do you not want to ThrowCodeError
if ! defined $param in the beginning of this function? That way you can always
trust it to be defined.

>+################################
>+#####  Module Subroutines    ###
>+################################
>+
>+sub get_product_components ($) {

Why is this a function and not a method of Product? This makes no sense to me.

>+=head1 DESCRIPTION
>+
>+Component.pm creates a product component object, which is a hash
>+containing the Bugzilla product component information.

Same comment as above.

>+=head2 USAGE
>+
>+    use Bugzilla::Component;
>+
>+    my $component = new Bugzilla::Component($param)

Same as above, exemplify accessors. Instead of $param use an actual example.

>+=item C<@DB_COLUMNS>

Oh. If this is internal, only to be used in the module, no need for POD docs.

>+=item C<_init($param)>
>+=head2 DATABASE COLUMNS ACCESSORS
>+=head1 ACCESSORS

Internals, no need for POD. Remember that you do need to document the
individual members, though.

>+Returns all Bugzilla components that belongs to the passed product.

that belong to the supplied product.

>+It returns a hash with component id as key and Bugzilla::Component

Returns a hash ... and a Bugzilla:: 

>Index: Bugzilla/Group.pm

Same comments as above on the POD docs. These additional comments:

>+sub make_product_group ($$) {
>+    my ($product_id, $product_name) = @_;

Why isn't this a method on the Product object?

>+    trick_taint($product_name);

Evil evil evil

>+    # XXX Should be a template.

Should be /in/ a template

You need to add an XXX comment to this method telling us where the code for it
came from so we know where to clean up after we land this and start using it.

>+sub get_group_controls ($) {
>+
>+    # Gets all group controls of a specific product.
>+
>+    my ($product_id) = @_;

Same as above. Why isn't this a method?

>Index: Bugzilla/Milestone.pm

Same comment on POD.

>+sub get_product_milestones ($) {

Method on Product.

>+sub check_sortkey ($$) {
>+    my ($milestone, $sortkey) = @_;

Hmmm. Why isn't this a method on the Milestone object?

>Index: Bugzilla/Product.pm

Same comment on POD.

>+sub get_classification_products {

Method on classification (get_products()).

>Index: Bugzilla/Version.pm

Same comment on POD.

>+sub get_product_versions ($) {

Method on product. Hint: when the only argument you receive is an ID, it's
because it should have been a method :-)

>Index: template/en/default/global/code-error.html.tmpl
+    [% title = "Invalid number argument" %]

Invalid numeric argument. 

>+    The argument <code>[% argument FILTER html %] = [% value FILTER html %]</code>
>+    of <code>[% function FILTER html %]</code> is not a natural number.

s/of/supplied to/

This is great infrastructure for the future. Fix this and I'll r=kiko.

Comment 76

[Tiago Mello [:timello]]

Attachment: v21: pod docs fixes

Comment 77

[Max Kanat-Alexander]

Comment on attachment 187161 [details] [diff] [review]
v21: pod docs fixes

>+__END__

  Instead of putting the POD docs at the end of the file, please spread them
throughout the file. The top sections can go at the very top of the file, and
the function descriptions should go right above the functions.

  I don't mean to be picky, but it's just very hard to change that after it's
checked-in, and having them throughout the file is very convenient for us
developers, who are more frequently looking at the actual code than at the HTML
documentation.

>+=head1 SYNOPSIS
> [snip]

  This is great! :-)

>+    my $hash_ref = Bugzilla::Classification::get_all_classifications();

  You should recommend that people always call it this way, actually:

  Bugzilla::Classification->get_all_classifications().

  And then make sure that you deal with that in the function itself (the first
argument will be $class).

>+The constructor is used to load an existing classification by passing

  Nit: Say where it exists (in the DB).

  Also, our usual method of documentation for methods is:

  Description: The constructor is used to load an existing 
	       classification by passing a classification id 
	       or classification name using a hash.
  Params:      $param - If you pass an integer, the integer is 
			the classification_id from the database
			that we want to read in. If you pass in
			a hash with a "name" key, then the value
			of the name key is the name of a classification 
			from the DB.
  Returns:     A Bugzilla::Classification object.

  That's not a nit -- they really do need to be documented that way, if at all
possible.

  For things that have no params or return nothing, it looks like:

  Description: blah blah blah
  Params:      none
  Returns:     nothing

>+=head1 AUTHOR
>+
>+    Tiago R. Mello <[e-mail]>

  We don't usually put this in the POD docs -- just leave it in the comments at
the top of the file.

>+=head1 NAME
>+
>+Bugzilla::Component - Object for a Bugzilla product component.

  Nit: "Object for" is awkward.

>Index: Bugzilla/Group.pm

>+__END__
>+
>+=head1 NAME
> [snip]

  We're missing a SYNOPSIS section for this one.

>+=item C<ValidateGroupName($name, @users)>
>+
>+ValidateGroupName checks to see if ANY of the users in the provided list
>+of user objects can see the named group.  It returns the group id if
>+successful and undef otherwise.

  Just as a note... If that's really what this function does, it's named
terribly. :-) We really should rename it.

>+__END__
>+
>+=head1 NAME
>+
>+Bugzilla::Milestone - Object for a Bugzilla product milestone.

  Nit: "Object for" again.

>+__END__
>+
>+=head1 NAME
>+
>+Bugzilla::Product - Object for a Bugzilla product.

  And here, also.


  Otherwise, these POD docs are really nice! Way to go! :-)

Comment 78

[Tiago Mello [:timello]]

(In reply to comment #77)
> 
> >+    my $hash_ref = Bugzilla::Classification::get_all_classifications();
> 
>   You should recommend that people always call it this way, actually:
> 
>   Bugzilla::Classification->get_all_classifications().
> 
  mkanat, if I do that, I have to put something like:
  sub get_products_by_classification($id) {
      my $self = shift;
      my ($id) = @_;
  }

  Then, I can call Bugzilla::Product->get_products_by_classification($id),
otherwise, $id will have 'Bugzilla::Product' as value. What do you think? Will
we have just methods in Product.pm Component.pm...?

Comment 79

[Max Kanat-Alexander]

(In reply to comment #78)
>   Then, I can call Bugzilla::Product->get_products_by_classification($id),
> otherwise, $id will have 'Bugzilla::Product' as value. What do you think? Will
> we have just methods in Product.pm Component.pm...?

  Yeah. As long as we start calling them all that way from the beginning, it's
not a problem. We weren't EXPORTing the subroutine anyway, so it's just typing
two slightly different characters anyhow. And if we want to change the
subroutine in the future to take advantage of being a class method, we can, if
we want.

Comment 80

[Tiago Mello [:timello]]

Attachment: v22: last fixes in the pod docs.

I hope that this patch is the last one for this bug. I'm with crossed-fingers
;).

Comment 81

[Tiago Mello [:timello]]

mkanat, is this patch ok for you?

Comment 82

[Max Kanat-Alexander]

Comment on attachment 188198 [details] [diff] [review]
v22: last fixes in the pod docs.

Yes, looks fine by interdiff. :-)

Comment 83

[Max Kanat-Alexander]

I recommend holding a+ until we have code that actually *uses* this code, that
we can check in simultaneously. (This is how we normally do things like this.)

Comment 84

[Frédéric Buclin]

(In reply to comment #83)
> I recommend holding a+ until we have code that actually *uses* this code, that
> we can check in simultaneously. (This is how we normally do things like this.)

I disagree. I cannot use a code which is not checked in. Requesting approval too.

Comment 85

[Christian Reis]

Withholding checkin approval of a patch which already has review+ is only useful
to promote bitrot; can we please move on with this?

Comment 86

[Tiago Mello [:timello]]

Attachment: v23: backing to :: instead of -> and unbitrotting Group.pm.

Comment 87

[Frédéric Buclin]

kiko and I fully agree that Bugzilla::Foo->bar() makes sense only if bar() is a
method of the foo object; else Bugzilla::Foo::bar() should be used. Indeed,
$self has no meaning if no object is created.

Comment 88

[Myk Melez [:myk] [@mykmelez]]

It's good to develop an interface in conjunction with a consumer of it, but it
doesn't make sense to hold up checking in an interface until a consumer arrives.

Comment 89

[Frédéric Buclin]

RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Classification.pm,v
done
Checking in Bugzilla/Classification.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Classification.pm,v  <-- 
Classification.pm
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Component.pm,v
done
Checking in Bugzilla/Component.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Component.pm,v  <--  Component.pm
initial revision: 1.1
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.26; previous revision: 1.25
done
Checking in Bugzilla/Group.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Group.pm,v  <--  Group.pm
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Milestone.pm,v
done
Checking in Bugzilla/Milestone.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Milestone.pm,v  <--  Milestone.pm
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Version.pm,v
done
Checking in Bugzilla/Version.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Version.pm,v  <--  Version.pm
initial revision: 1.1
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v
 <--  code-error.html.tmpl
new revision: 1.54; previous revision: 1.53
done