313122 - implement validations and database persistence functions for Product.pm and clean-up editproducts.cgi.

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[André Batosti]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050725 Firefox/1.0.6 (Ubuntu package 1.0.6)

We need methods and/or subroutines for validations and database persistence.
All related routines about inserts, updates, deletes and validations for those
routines have to be implemented at this bug.

Reproducible: Always

Comment 1

[André Batosti]

Attachment: batosti_v1

Comment 2

[Frédéric Buclin]

Comment on attachment 201160 [details] [diff] [review]
batosti_v1

4 out of 6 hunks FAILED -- saving rejects to file editproducts.cgi.rej

Comment 3

[André Batosti]

Attachment: batosti_v2

Comment 4

[André Batosti]

Attachment: batosti_v2_fix

Comment 5

[Frédéric Buclin]

Comment on attachment 214425 [details] [diff] [review]
batosti_v2_fix

At first glance:
1) do not lock tables in Product.pm, as discussed on IRC;

2) $::userid no longer exists;

3) do not use GroupIdToName() as well as any other routines from globals.pl;

4) Do not use Bugzilla::Classification from Product.pm, due to module dependency loops. If you need a classification object, create it outside Product.pm and pass it to the routine.

5) Do not write:
$user = Bugzilla->login(LOGIN_REQUIRED);
but:
$user = Bugzilla->user;
User rights must be assumed to be already checked at this point.

I didn't look further.

Comment 6

[André Batosti]

Attachment: batosti_v3

Comment 7

[André Batosti]

Attachment: batosti_v4

Comment 8

[Frédéric Buclin]

Comment on attachment 218902 [details] [diff] [review]
batosti_v4

>Index: editproducts.cgi

>+            use Data::Dumper;
>+            die Dumper($vars);

What's that?? Remove these lines.


>-    # First make sure the product name is valid.
>-    my $product_old = Bugzilla::Product::check_product($product_old_name);
>+    my $product = Bugzilla::Product::check_product($product_old_name);

No reason to remove this comment. It makes perfect sense here.


>-    # Then make sure the user is allowed to edit properties of this product.
>-    $user->can_see_product($product->name)
>-      || ThrowUserError('product_access_denied', {product => $product->name});

Bad! This check was introduced in bug 271596 to prevent users to edit products they are not allowed to.



>Index: Bugzilla/Product.pm

>+sub all_group_controls {

What's the difference between group_controls() and all_group_controls()?? There is no documentation for this method. POD is also missing for several other routines too.


>+sub check_classification_product
>+{

Nit: move the bracket on the previous line.


>+    my $classification = 
>+        Bugzilla::Classification::check_classification($cl);

check_classification_product() is used nowhere!!! Remove it.


>+sub check_votes {

Please do not lock tables while updating votes (in editproducts.cgi). This introduced too many regressions in the past. Maybe could you have two separate methods ->update() and ->check_votes(), so that ->check_votes() can be called outside the locking.



>Index: template/en/default/admin/products/updated.html.tmpl

>-[% IF product.name != old_product.name %]
>-  <p>
>-  Updated product name from '[% old_product.name FILTER html %]' to 
>-  <a href="editproducts.cgi?action=edit&amp;product=

>+  [% IF field == 'name' %]
>+    <p>
>+    Updated product name to 

I see no reason to change the way this template works, i.e. showing both old and new data. At least, this has nothing to do in this bug.


>+[% FOREACH field = product.changed_fields %]

I'm not sure a FOREACH loop is appropriate here. Better is to use the old way, i.e. passing an old product object and a new product object.


This was a very quick and partial review. I will probably have more to say in an updated patch. ;)

Comment 9

[Frédéric Buclin]

One more thing. Names of internal routines from Product.pm should begin with an underscore.

Comment 10

[André Batosti]

Attachment: batosti_v5

Comment 11

[Frédéric Buclin]

Comment on attachment 223773 [details] [diff] [review]
batosti_v5

>Index: editproducts.cgi

>+    $new_product{'maxvotesperbug'} = defined($cgi->param('maxvotesperbug')) ?
>+    $cgi->param('maxvotesperbug') : 10000;

Fix the indentation. Move $cgi->foo 2 or 4 places to the right. Else it's confusing.


>+    # Insert default charting queries for this product.
>+    # If they aren't using charting, this won't do any harm.
> 
>     if ($cgi->param('createseries')) {
>         # Insert default charting queries for this product.

No need to repeat the same comment twice.

More important: this part about series must be moved into Product.pm as well. Else someone using Product::create_product() would have no series for this product. Only collect data here, then pass everything to create_product().


>+    $product->remove_from_db;

Nit: I'm a bit ambivalent with this. remove_from_db() doesn't do any check to make sure you are allowed to delete this product. This includes making sure you are allowed to delete bugs being in this product. On the other hand, Bug::remove_from_db() only makes sure you are allowed to view the bug you try to delete, but doesn't check Param("allowbugdeletion"). We could also say that it's the problem of the caller to make sure he is doing things correctly. Both solutions are fine for now probably. We could change this behavior in a separate bug if required.


>+        if (($f =~ /^membercontrol_(\d+)$/) ||
>+            ($f =~ /^othercontrol_(\d+)$/) ||
>+            ($f =~ /^entry_(\d+)$/) ||
>+            ($f =~ /^canedit_(\d+)$/)) {
>+        my $id = $1;

Indentation! Moreover, for very long IF condition, the bracket must be moved on a newline. This is much clearer.


>+            $group_control{'id'} = $id;
>+            $group_control{'membercontrol'} = 
>+                $cgi->param('membercontrol_' . $id);
>+            $group_control{'othercontrol'} = 
>+                $cgi->param('othercontrol_' . $id);
>+            $group_control{'entry'} = 
>+                $cgi->param('entry_' . $id) || 0;
>+            $group_control{'canedit'} = 
>+                $cgi->param('canedit_' . $id) || 0;
>+            $group_controls{$id} = \%group_control;

Nit: Probably having $group_controls{$id}->{'foo'} = $bar would be better than pushing a ref to a hash into another hash.


>+        if ((scalar(@{$product->confirm_na(\@now_na)})) || 
>+            (scalar(@{$product->confirm_mandatory(\@now_mandatory)}))) 
>         {
>             $vars->{'product'} = $product;
>+            $vars->{'na_groups'} = $product->confirm_na(\@now_na);
>+            $vars->{'mandatory_groups'} =
>+                $product->confirm_mandatory(\@now_mandatory);

Each call to $product->confirm_foo() generates a new SQL call. Either store the returned value of $product->confirm_foo() into a variable here, or add a cache in confirm_foo() to avoid extra and useless SQL calls.


>+    my ($removed_na, $added_mandatory) = 
>+        $product->update_group_controls(\%group_controls);
> 
>+    $vars->{'removed_na'} = $removed_na;
>+    $vars->{'added_mandatory'} = $added_mandatory;

Nit: You could as well write:
($vars->{'removed_na'}, $vars->{'added_mandatory'} =
    $product->update_group_controls(\%group_controls);


>     unless ($product_name) {
>         ThrowUserError('product_cant_delete_name',
>                        {product => $product_old->name});
>+        my $test_product = new Bugzilla::Product({ name => $product_name });
>+        if ((lc($product->name) ne lc($product_name)) && $test_product) {
>+            ThrowUserError('product_name_already_in_use',
>+                           {product => $test_product->name});
>+        }
>     }

If the $product_name exists, you will never enter this block. So moving this check here is wrong.

Moreover, all this part of the code has to do is to collect new values and to pass them to Product.pm which will do the required checks itself, as it did when creating a new product. Else this bug has no sense.



>Index: Bugzilla/Product.pm

>+use vars qw(@legal_bug_status @legal_resolution);

So ugly! globals.pl has been removed anyway. Look at Field.pm now.


>+sub all_group_controls {

I really don't like this name, and I don't understand what its goal is. Where is the POD for this function?


>+        foreach my $group (keys(%{$self->{all_group_controls}})) {
>+            $self->{all_group_controls}->{$group}->{'group'} = 
>+                new Bugzilla::Group($group);

Couldn't it be easier?


>+sub remove_from_db {
>+    my $self = shift;
>+
>+    my $user = Bugzilla->user;

$user is never used in this routine.


>+sub update {

Validation routines must be called from here.


>+        if (($field eq 'name') ||
>+            ($field eq 'milestoneurl') ||
>+            ($field eq 'description') ||
>+            ($field eq 'defaultmilestone')) {
>+            trick_taint($new_value);

All values must be detainted anyway. So this test is useless. Call trick_taint() in all cases.


>+        $query .= qq{$field = ?, };
>+        push (@values, $new_value);
>+    }
>+    if (scalar(@{$self->{'changed_fields'}})) {
>+        $query =~ s/, $/ /;

All this is much clearer using map { ... } @foo


>+        $query .= qq{WHERE id = ?};
>+        $dbh->do($query, undef, @values, $self->id);
>+        $self->_check_votes if $check_votes;

No! As I said, do not lock tables while updating votes. Two main reasons:
1. It's prone to regressions.
2. Emails will be sent while tables are locked.


>+sub confirm_na {

Please find a better name for this routine.


>+sub confirm_mandatory {

Same remark here.


>+sub create_product {

>+    $new_product->{'votesperuser'} ||= 0;
>+    detaint_natural($new_product->{'votesperuser'});
>+    $new_product->{'maxvotesperbug'} = 10000 if !defined $new_product->{'maxvotesperbug'};
>+    detaint_natural($new_product->{'maxvotesperbug'});
>+    $new_product->{'votestoconfirm'} ||= 0;
>+    detaint_natural($new_product->{'votestoconfirm'});
>+    $new_product->{'defaultmilestone'} ||= "---";
>+    trick_taint($new_product->{'defaultmilestone'});

You already made sure that these values were defined. Do it here only maybe and not in editproducts.cgi.


>+    $dbh->do(q{INSERT INTO products (name,
>+                                    description,
>+                                    milestoneurl,
>+                                    disallownew,
>+                                    votesperuser,
>+                                    maxvotesperbug,
>+                                    votestoconfirm,
>+                                    defaultmilestone,
>+                                    classification_id)
>+              VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?)}, undef,
>+              $new_product->{'name'},
>+              $new_product->{'description'},
>+              $new_product->{'milestoneurl'},
>+              $new_product->{'disallownew'},
>+              $new_product->{'votesperuser'},
>+              $new_product->{'maxvotesperbug'},
>+              $new_product->{'votestoconfirm'},
>+              $new_product->{'defaultmilestone'},
>+              $new_product->{'classification_id'});

Have these values been detainted??

I haven't looked at the code in Product.pm in great details, but only for editproducts.cgi.

Comment 12

[André Batosti]

Attachment: batosti_v6

Comment 13

[Frédéric Buclin]

Comment on attachment 229167 [details] [diff] [review]
batosti_v6

You should now use methods from Object.pm to create and update products. Sorry for the delay, but it makes more sense to use this centralized way to do this.

Comment 14

[André Batosti]

Attachment: batosti_v7

now with Bugzilla::Object.

Comment 15

[Frédéric Buclin]

Comment on attachment 238528 [details] [diff] [review]
batosti_v7

bitrotten (due to security bugs, probably)

Comment 16

[Frédéric Buclin]

We are in "soft freeze" mode to prepare 3.0 RC1.

Comment 17

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set "blocking3.2" tp "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Comment 18

[Frédéric Buclin]

Attachment: patch, v8

This patch implements ->create, ->update and ->remove_from_db. I tested it and it works fine, passes runtests.pl and sanitycheck.cgi doesn't report any inconsistency. The removal of |use Bugzilla::User| in Series.pm is required to avoid dependency loops, and is useless anyway (thanks to the code cleanup we did in Bugzilla 2.20 or 2.22, I don't remember).

I didn't include changes for "edit group access control" as this would definitely make the patch too large. I plan to do this remaining part in a separate bug.

Comment 19

[Max Kanat-Alexander]

Comment on attachment 331376 [details] [diff] [review]
patch, v8

>Index: Bugzilla/Product.pm
>+use constant UPDATE_VALIDATORS => {
>+};

  That doesn't need to be defined if it's empty.

>+sub update {

  update() needs a transaction that starts before SUPER::update().

>+        if ($self->max_votes_per_bug < $self->votes_per_user) {
>+            my $votes = $dbh->selectall_arrayref(
>+                        'SELECT votes.who, votes.bug_id
>+                           FROM votes
>+                     INNER JOIN bugs
>+                             ON bugs.bug_id = votes.bug_id

  Nit: INNER JOIN and ON should not be aligned with FROM.

>+                          WHERE bugs.product_id = ?
>+                            AND votes.vote_count > ?',

  Nit: AND should be aligned with bugs.product_id, not WHERE.

  (Same nits for the rest of the SQL in this function.)

>+                # If some votes are removed, RemoveVotes() returns a list
>+                # of messages to send to voters.
>+                my $msgs = RemoveVotes($id, $who, 'votes_too_many_per_bug');
>+                foreach my $msg (@$msgs) {
>+                    MessageToMTA($msg);

  You should only send these messages once the entire transaction finishes. Otherwise, if there's some error during update() (particularly an error with message sending) some messages could be sent over and over.

  (Also true for the MessageToMTA that's lower down.)

>+sub _check_default_milestone {
>+    my ($invocant, $milestone) = @_;
>+
>+    # Do nothing if target milestones are not in use.
>+    unless (Bugzilla->params->{'usetargetmilestone'}) {
>+        return (ref $invocant) ? $invocant->default_milestone : undef;
>+    }

  Wait...if usetargetmilestone is off, shouldn't we still set a default milestone of '---', for when people turn on usetargetmilestone?

  By the way, the fact that we use the string '---' all over the code makes me think we should put it in a constant called UNSET.

>+sub _check_votes_per_bug {
>+    return _check_votes(@_, 10000);

  That number should be in a constant.


  And otherwise it's all awesome! :-)

Comment 20

[Frédéric Buclin]

(In reply to comment #19)
>   Wait...if usetargetmilestone is off, shouldn't we still set a default
> milestone of '---', for when people turn on usetargetmilestone?

'---' is the default in the DB schema, so undef will automatically writes '---' in the DB. So that's fine.


>   By the way, the fact that we use the string '---' all over the code makes me
> think we should put it in a constant called UNSET.

Maybe in a separate bug.

Comment 21

[Frédéric Buclin]

(In reply to comment #19)
>   update() needs a transaction that starts before SUPER::update().

The transaction starts before the first $product->set_foo, i.e. outside update() itself (as done for Bug::update()). Do we really want another transaction inside update()?


>   You should only send these messages once the entire transaction finishes.
> Otherwise, if there's some error during update() (particularly an error with
> message sending) some messages could be sent over and over.

OK. The reason I left it here is because at that point, we passed all checks successfully, so I didn't expect any other error to occur at that point. Should we let the caller send them, or should we do it at the end of update()? Probably the latter, I think.


> >+    return _check_votes(@_, 10000);
> 
>   That number should be in a constant.

Maybe in another bug. :)


>   And otherwise it's all awesome! :-)

Thanks! :)

Comment 22

[Max Kanat-Alexander]

(In reply to comment #21)
> The transaction starts before the first $product->set_foo, i.e. outside
> update() itself (as done for Bug::update()). Do we really want another
> transaction inside update()?

  Oh...well, yeah, it couldn't hurt to have another one inside.

> OK. The reason I left it here is because at that point, we passed all checks
> successfully, so I didn't expect any other error to occur at that point. 

  Yeah, but the message sending itself could error out, and I don't want that to interfere with the rest of the update.

> Should
> we let the caller send them, or should we do it at the end of update()?
> Probably the latter, I think.

  Yeah, end of update sounds better.

> > >+    return _check_votes(@_, 10000);
> > 
> >   That number should be in a constant.
> 
> Maybe in another bug. :)

  Awww. It's just one number in one place, though, isn't it?

Comment 23

[Frédéric Buclin]

(In reply to comment #22)
>   Awww. It's just one number in one place, though, isn't it?

No. This number is also hardcoded in Schema.pm. Really another bug. :)

Comment 24

[Max Kanat-Alexander]

(In reply to comment #23)
> No. This number is also hardcoded in Schema.pm. Really another bug. :)

  Okay. :-)

Comment 25

[Frédéric Buclin]

Attachment: patch, v9

Comment 26

[Max Kanat-Alexander]

Comment on attachment 331763 [details] [diff] [review]
patch, v9

>+    # We have to stop the transaction before committing changes, in case
>+    # something goes wrong with email sending. That's unfortunate!
>     $dbh->bz_commit_transaction();

  There shouldn't even be a transaction here if you're not actually changing the database inside of it. That's inconsistent with the rest of Bugzilla.

  Otherwise this looks fine. Please remove that transaction on checkin.

Comment 27

[Frédéric Buclin]

I did the fix on checkin. Thanks for the review. :)

Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.143; previous revision: 1.142
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.413; previous revision: 1.412
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.249; previous revision: 1.248
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.95; previous revision: 1.94
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.27; previous revision: 1.26
done
Checking in Bugzilla/Series.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Series.pm,v  <--  Series.pm
new revision: 1.17; previous revision: 1.16
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v  <--  filterexceptions.pl
new revision: 1.114; previous revision: 1.113
done
Checking in template/en/default/admin/products/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
new revision: 1.10; previous revision: 1.9
done
Checking in template/en/default/admin/products/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/admin/products/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.13; previous revision: 1.12
done
Checking in template/en/default/admin/products/footer.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/footer.html.tmpl,v  <--  footer.html.tmpl
new revision: 1.12; previous revision: 1.11
done
Checking in template/en/default/admin/products/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/list.html.tmpl,v  <--  list.html.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/admin/products/updated.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/updated.html.tmpl,v  <--  updated.html.tmpl
new revision: 1.8; previous revision: 1.7
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.74; previous revision: 1.73
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.253; previous revision: 1.252
done

Comment 28

[Frédéric Buclin]

There is no POD about e.g. create. It's not obvious that the version field is mandatory to create a new product.