299753 - Replace old code in editproducts.cgi by routines from Product.pm

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Frédéric Buclin]

Due to the semi-templatization of editproducts.cgi, validation routines are
duplicated. In order to make the rest of the templatization easier, this part
has to be fixed first.

I'll do it as soon as bug 289580 is fixed.

Comment 1

[Frédéric Buclin]

This is the last step about editproducts.cgi. As soon as this file is completely
templatized, we can replace old code by code from Product.pm.

The bug was originally assigned to me "by default", but if timello or gbel is
interested in taking it, please let me know. I will reassign it to you with
pleasure. :)

Updating the summary to take into account the newly created Product.pm module.

Comment 2

[André Batosti]

Attachment: batosti_v1

Comment 3

[Frédéric Buclin]

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

>Index: editproducts.cgi

> # TestClassification:  just returns if the specified classification does exists
> # CheckClassification: same check, optionally  emit an error text

*ALL* these validation routines have to go away in favor of Product.pm and
Classification.pm methods. If some methods are missing in these .pm modules,
create them.

Note that this bug is still blocked by bug 190196 with which you will conflict.
Better is to wait for bug 190196 to be fixed first.

Moreover, please fix all errors returned by ./runtests.pl before submitting a
new patch.

And finally, update the INTERFACEs in templates accordingly.

Comment 4

[André Batosti]

(In reply to comment #3)
> *ALL* these validation routines have to go away in favor of Product.pm and
> Classification.pm methods. If some methods are missing in these .pm modules,
> create them.
I only replace the code of Product.pm not the code of Classification.pm

the both can be replaced?

i make that tomorrow.

Comment 5

[Frédéric Buclin]

(In reply to comment #4)
> I only replace the code of Product.pm not the code of Classification.pm
> 
> the both can be replaced?

Use all the .pm modules you need. But as I said, you will conflict with bug
190196, and especially with bug 307524 which will remove all the remaining
obsolete code from editproducts.cgi (due to templatization).

Comment 6

[André Batosti]

Attachment: batosti_v2

Comment 7

[Max Kanat-Alexander]

Comment on attachment 196682 [details] [diff] [review]
batosti_v2

>+        @products =
>+            Bugzilla::Product::get_products_by_classification($classification->id);

  Just use $classification->products.

  We need to eliminate these get_x_by_y functions entirely, in favor of just
having the object methods. These get_x_by_y functions were a bad idea (for
which I take responsibility).

>@@ -332,23 +215,25 @@ if ($action eq 'new') {
>             SqlQuote($maxvotesperbug) . "," .
>             SqlQuote($votestoconfirm) . "," .
>             SqlQuote($defaultmilestone) . "," .
>-            SqlQuote($classification_id) . ")");
>+            SqlQuote($classification->id) . ")");
>     my $product_id = $dbh->bz_last_key('products', 'id');
> 
>+    $product = new Bugzilla::Product($product_id);
>+    

  Don't you already have an $product from above??

>-                "($gid, $product_id, " . Param("useentrygroupdefault") .
>+                "($gid, $product->{'id'}, " . Param("useentrygroupdefault") .

  Always use $product->id instead of $product->{id}.

> if ($action eq 'del') {
>     
>-    if (!$product) {
>+    if (!$product_name) {
> [snip]
>-    my $classification_id = 1;
>+    my $product = new Bugzilla::Product({name => $product_name});
>+    $product || ThrowUserError('product_doesnt_exist',
>+                               {product => $product_name});


  Why not just use check_product here?

>@@ -506,20 +340,19 @@ if ($action eq 'del') {
> if ($action eq 'delete') {
>     
>-    if (!$product) {
>+    if (!$product_name) {
>         ThrowUserError('product_not_specified');
>     }
> 
>-    my $product_id = get_product_id($product);
>-    $product_id || ThrowUserError('product_doesnt_exist',
>-                                  {product => $product});
>+    my $product = new Bugzilla::Product({name => $product_name});
>+    $product || ThrowUserError('product_doesnt_exist',
>+                               {product => $product_name});

  And the same here? check_product($product_id).

>+if ($action eq 'edit' || (!$action && $product_name)) {
> [snip]
>+    my $product = new Bugzilla::Product({name => $product_name});
>+    $product || ThrowUserError('product_doesnt_exist',
>+                               {product => $product_name});

  And here?

>@@ -678,7 +429,10 @@ if ($action eq 'edit' || (!$action && $p
>+
>+    my $product = new Bugzilla::Product($product_name);
>+    $product || ThrowUserError('product_doesnt_exist',
>+                               {product => $product_name});

  And here? :-)

>-                     AND bugs.product_id = $product_id " .
>+                     AND bugs.product_id = $product->{'id'} " .

  Once again, $product->id. Everywhere you've done that, please fix it.

>-    CheckProduct($productold);
>-    my $product_id = get_product_id($productold);
>+    my $product = new Bugzilla::Product($product_id);
>+    $product || ThrowUserError('product_doesnt_exist',
>+                               {product => $product_name});

  And again. check_product.

> if ($action eq 'editgroupcontrols') {
>-    my $product_id = get_product_id($product);
>-    $product_id
>-      || ThrowUserError("invalid_product_name", { product => $product });
>+    my $product = new Bugzilla::Product({name => $product_name});
>+    $product || ThrowUserError('product_doesnt_exist',
>+                               {product => $product_name});

  And check_product, again. :-)

>     $vars->{'product'} = $product;
>-    $vars->{'classification'} = $classification;
>+    $vars->{'classification'} = $classification_name;

  I think we should also modify the template to just take a classification
object, but maybe that's another bug?

>Index: Bugzilla/Product.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v
>retrieving revision 1.9
>diff -u -p -r1.9 Product.pm
>--- Bugzilla/Product.pm	2 Sep 2005 21:12:08 -0000	1.9
>+++ Bugzilla/Product.pm	19 Sep 2005 18:33:54 -0000
>@@ -174,6 +174,7 @@ sub max_votes_per_bug { return $_[0]->{'
> sub votes_to_confirm  { return $_[0]->{'votestoconfirm'};    }
> sub default_milestone { return $_[0]->{'defaultmilestone'};  }
> sub classification_id { return $_[0]->{'classification_id'}; }
>+sub status            { return !$_[0]->{'disallownew'};      }

  Um, I disagree with adding this subroutine. People can just check against
!$product->disallow_new, right?

>Index: Bugzilla/Template.pm
>@@ -475,6 +475,20 @@ sub create {
>             # Wrap a displayed comment to the appropriate length
>             wrap_comment => \&Bugzilla::Util::wrap_comment,
> 
>+            # Switch the group_control number to text.
>+            group_control => [ sub {
>+                                   my ($group_control) = @_;
>+                                   use Switch;

  You can't use Switch in perl 5.6, and we still support perl 5.6.


  I didn't review the template changes; the patch is too big. :-)

Comment 8

[Frédéric Buclin]

Comment on attachment 196682 [details] [diff] [review]
batosti_v2

Fix mkanat's comments, with which I agree. But this patch is not too big ;)

Comment 9

[Frédéric Buclin]

bug 309749 doesn't block this bug. Both can be fixed independently.

Comment 10

[André Batosti]

(In reply to comment #7)
> >@@ -332,23 +215,25 @@ if ($action eq 'new') {
> >             SqlQuote($maxvotesperbug) . "," .
> >             SqlQuote($votestoconfirm) . "," .
> >             SqlQuote($defaultmilestone) . "," .
> >-            SqlQuote($classification_id) . ")");
> >+            SqlQuote($classification->id) . ")");
> >     my $product_id = $dbh->bz_last_key('products', 'id');
> > 
> >+    $product = new Bugzilla::Product($product_id);
> >+    
> 
>   Don't you already have an $product from above??
> 
yes, but it's is undef at this moment.

> >     $vars->{'product'} = $product;
> >-    $vars->{'classification'} = $classification;
> >+    $vars->{'classification'} = $classification_name;
> 
>   I think we should also modify the template to just take a classification
> object, but maybe that's another bug?
> 
> >Index: Bugzilla/Product.pm
> >===================================================================
> >RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v
> >retrieving revision 1.9
> >diff -u -p -r1.9 Product.pm
> >--- Bugzilla/Product.pm	2 Sep 2005 21:12:08 -0000	1.9
> >+++ Bugzilla/Product.pm	19 Sep 2005 18:33:54 -0000
> >@@ -174,6 +174,7 @@ sub max_votes_per_bug { return $_[0]->{'
> > sub votes_to_confirm  { return $_[0]->{'votestoconfirm'};    }
> > sub default_milestone { return $_[0]->{'defaultmilestone'};  }
> > sub classification_id { return $_[0]->{'classification_id'}; }
> >+sub status            { return !$_[0]->{'disallownew'};      }
> 
>   Um, I disagree with adding this subroutine. People can just check against
> !$product->disallow_new, right?
> 
it's used in a table in template with i don't know a way to make a field !field,
if you know help me :)

Comment 11

[André Batosti]

Attachment: batosti_v2_fix

Comment 12

[Frédéric Buclin]

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

>Index: editproducts.cgi

>+use Bugzilla::Product;

Bugzilla::Classification is required too.


>if ($action eq 'new') {

>+    if ($product) {

>+        if ($product->name eq $product_name) {
>             ThrowUserError("prod_name_already_in_use",
>                            {'product' => $product});
>         }

>+        if (lc($product->name) eq lc($product_name)) {
>             ThrowUserError("prod_name_diff_in_case",
>+                           {'product' => $product_name,
>+                            'existing_product' => $product}); 
>         }
>     }
> 
>     if ($version eq '') {
>         ThrowUserError("product_must_have_version",
>+                       {'product' => $product->name});
>     }

If we come here, then the product doesn't already exist and $product is
undefined. Then $product->name is invalid. Use $product_name instead.


>-            SqlQuote($classification_id) . ")");
>+            SqlQuote($classification->id) . ")");

If classifications are not in use, then $classification is undefined and
$classification->id won't work. In this case, you should fall back to 1.
Moreover, SqlQuote doesn't make sense here.


> if ($action eq 'del') {

>-    if (Param('useclassification')) {
>-        CheckClassificationProduct($classification, $product);
>-        $classification_id = get_classification_id($classification);
>-        $vars->{'classification'} = $classification;
>-    }

Why are you removing this check?


> if ($action eq 'delete') {

>     my $bug_ids = $dbh->selectcol_arrayref(q{
>         SELECT bug_id FROM bugs
>+        WHERE product_id = ?}, undef, $product->id);

This needs to be a method of Product.pm: $product->bug_ids. Look at
Component.pm for instance.


>     my $nb_bugs = scalar(@$bug_ids);

$nb_bugs is $product->bug_count.


>-if ($action eq 'edit' || (!$action && $product)) {

>-    if (Param('useclassification')) {
>-        # If a product has been given with no classification associated
>-        # with it, take this information from the DB
>-        if ($classification) {
>-            CheckClassificationProduct($classification, $product);
>-        } else {
>-            $classification =
>-                $dbh->selectrow_array("SELECT classifications.name
>-                                       FROM products, classifications
>-                                       WHERE products.name = ?
>-                                       AND classifications.id = products.classification_id",
>-                                       undef, $product);
>-        }
>-        $classification_id = get_classification_id($classification);
>-    }

Why removing this check?


>+    my $product = new Bugzilla::Product({name => $product_name});
>+    $product || ThrowUserError('product_doesnt_exist',
>+                               {product => $product_name});

This is the job of Product::check_product().


> if ($action eq 'update') {

>     my $productold          = trim($cgi->param('productold')          || '');

Nit: $productold should be renamed as $product_old_name for consistency.


>+    my $product = Bugzilla::Product::check_product($productold);

$product should be renamed as $product_old.



>Index: Bugzilla/Product.pm

>+sub status            { return !$_[0]->{'disallownew'};      }

We don't want this method IMO. It's nothing more than !$product->disallow_new.



>Index: Bugzilla/Template.pm

>+            # Switch the group_control number to text.
>+            group_control => [ sub {
>+                                   my ($group_control) = @_;
>+
>+                                   if ($group_control == CONTROLMAPSHOWN) {
>+                                       return "Shown";
>+                                   } elsif ($group_control == CONTROLMAPDEFAULT) {
>+                                       return "Default";
>+                                   } elsif ($group_control == CONTROLMAPMANDATORY) {
>+                                       return "Mandatory";
>+                                   } else { 
>+                                       return "NA";
>+                                   } 
>+                               }
>+                             ],

This is so specific to editproducts.cgi only that I think this code should
remain in this .cgi script. I don't understand why you move it here.


I didn't review templates yet, but I like your patch so far. :)
Please have a look at bug 306601 which will modify the way Product.pm and
Classification.pm work, i.e. the removal of $product->classification in favor
of $classification->products. Some coordination will be required here. ;)

Comment 13

[André Batosti]

Attachment: batosti_v3

the use of get_products_by_classification is dependent of bug 306601. i have a
patch in this bug waiting for a review.

Comment 14

[Frédéric Buclin]

Comment on attachment 197840 [details] [diff] [review]
batosti_v3

>Index: editproducts.cgi
> if ($action eq 'new') {

>-    my $classification_id = 1;
>+    my $classification;
>     if (Param('useclassification')) {
>+        $classification = 
>+            Bugzilla::Classification::check_classification($classification_name);
>         $vars->{'classification'} = $classification;
>     }
>+    my $classification_id = $classification->id || 1;

If 'useclassification' is turned off, $classification is undefined and
$classification->id returns an error.


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

Nit: you don't need $product_id anymore. Write:
$product = new Bugzilla::Product({name => $product_name}); directly.


>+                SqlQuote("Access to bugs in the $product->name product") . ", 1, NOW())");

You cannot write $product->name inside quotes.


>             my $series = new Bugzilla::Series(undef, $product, 

$product is an object. Write $product->name.


> if ($action eq 'delete') {

>-        $vars->{'nb_bugs'} = $nb_bugs;
>+        $vars->{'nb_bugs'} = $product->bug_count;

Nit: you don't need $vars->{'nb_bugs'} anymore. You can access it directly from
the templates using 'product.bug_count'.


>+if ($action eq 'edit' || (!$action && $product_name)) {

>     if (Param('useclassification')) {
>+        my $classification; 
>+        if (!$classification_name) {
>+            $classification = 
>+                new Bugzilla::Classification($product->classification_id);
>         } else {
>+            $classification = 
>+                Bugzilla::Classification::check_classification($classification_name);
>         }
>+        if ($classification->id != $product->classification_id) {
>+            ThrowUserError('classification_doesnt_exist_for_product',
>+                           { product => $product->name,
>+                             classification => $classification->name });
>+        }

This test is useful only if a classification name is given (else you already
know the classification IDs will match as you used it to build
$classification). So move this test in the ELSE block.


> if ($action eq 'updategroupcontrols') {

>-    $vars->{'classification'} = $classification;

Nit: Why removing it?


> if ($action eq 'update') {

>+    if (Param('useclassification')) {
>+        my $classification; 
>+        if (!$classification_name) {
>+            $classification = 
>+                new Bugzilla::Classification($product_old->classification_id);
>+        } else {
>+            $classification = 
>+                Bugzilla::Classification::check_classification($classification_name);
>+        }
>+        if ($classification->id != $product_old->classification_id) {
>+            ThrowUserError('classification_doesnt_exist_for_product',
>+                           { product => $product_old->name,
>+                             classification => $classification->name });
>+        }

Same comment as before; this last test should be done only when a
classification name is given.


>         unless ($description) {
>             ThrowUserError('prod_cant_delete_description',
>+                           {product => $product_old->name});
>         }

Nit: this is the kind of checks which should be done before locking tables.
Could you fix it in this patch? (No need to test whether the description has
changed; only test if a description is given).


>+    if ($product_name ne $product_old->name) {
>+        unless ($product_name) {
>             ThrowUserError('prod_cant_delete_name',
>+                           {product => $product_old->name});
>         }

Nit: Same remark here; this check should be done before locking tables.


> if ($action eq 'editgroupcontrols') {

>     $vars->{'product'} = $product;
>-    $vars->{'classification'} = $classification;

Nit: why removing $vars->{'classification'} ?



>Index: Bugzilla/Product.pm

>+sub bug_ids {

This method needs POD.



>Index: template/en/default/admin/products/confirm-delete.html.tmpl

You have to update the INTERFACE of this template. Now only a product and
classification objects are passed to the template.



>+[% UNLESS classification.description %]
>+  [% classification.description = '<span style="color: red">missing</span>' %]
> [% END %]

Better is to use it directly where it should be displayed.


>+[% UNLESS product.description %]
>+  [% product.description = '<span style="color: red">missing</span>' %]
> [% END %]

Same comment here.


>+[% IF product.disallownew %]
>+  [% product.disallownew = "closed" %]
> [% ELSE %]
>-  [% disallownew = "open" %]
>+  [% product.disallownew = "open" %]
> [% END %]

Same comment here.


>   [% IF Param('useclassification') %]
>     <tr>
>       <td>Classification:</td>
>+      <td>[% product.classification.name FILTER html %]</td>
>     </tr>

As you said, $product->classification is going to disappear very soon. Use
classification.name instead.


>+        [% IF product.milestoneurl %]
>+          <a href="[% product.milestoneurl FILTER uri %]">
>             [%- milestoneurl FILTER html %]
>           </a>

It should be 'product.milestone_url', used 3 times!


>+        [% FOREACH v = product.versions %]
>+          [% v.value FILTER html %]<br>
>         [% END %]

You should use v.name (use methods of version objects).


>+        [% FOREACH m = product.milestones %]
>+          [% m.value FILTER html %]<br>
>         [% END %]

Same comment here. Use m.name.


>       [% IF bug_count %]

Use 'product.bug_count' and fix filterexceptions.pl accordingly (remember that
I suggested to suppress this variable in editproducts.cgi).



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

[% PROCESS "admin/products/footer.html.tmpl" name = product %]

name is not used directly anymore.



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

[% IF nb_bugs %]

Use product.bug_count.



>Index: template/en/default/admin/products/edit-common.html.tmpl

> [%# INTERFACE:
>+  # classification: Bugzilla::Classifiation object; classification product is in.
>   #        product: array;  an array of product objects.
>   #%]

Nit: 'product' is not an array, but a product object.

Also, please update fields to use object methods.



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

Nit: update the INTERFACE where appropriate.


>+          [% FOREACH v = product.versions %]
>+            [% v.value FILTER html %]
>             <br>
>           [% END %]

Use v.name instead.


>+            [%- FOREACH m = product.milestones -%]
>+              [% m.value FILTER html %]
>             <br>
>             [% END %]

Use m.name.


<a href="editproducts.cgi?action=editgroupcontrols&product=
[%- product.name FILTER url_quote %]&classification=
[%- classification FILTER url_quote %]">

classification.name.



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

>-[% IF Param('useclassification') && classification %]
>+[% IF Param('useclassification') && classification.name %]

Nit: '&& classification' suffices.



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

Update the INTERFACE.


>      { 
>-       name => "status"
>+       name => "disallow_new"
>        heading => "Open For New $terms.Bugs"
>-       yesno_field => 1
>      },

I'm not sure this is the right way of doing this. I'm a bit ambivalent with
this solution.


> [% PROCESS admin/products/footer.html.tmpl
>     no_edit_other_products_link = 1
>+    no_edit_product_link = 1
>+    product = products.0
>  %]

Why doing this??



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

Update the INTERFACE.



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

>+[% filt_product = product.name FILTER html %]
>+[% filt_classification = classification.name FILTER html %]

Nit: remove these useless variables and write '{product|classification}.name
FILTER html' where appropriate (they are used only once).



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

Update the INTERFACE.

Comment 15

[André Batosti]

Attachment: batosti_v4

Comment 16

[Frédéric Buclin]

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

>Index: editproducts.cgi

>+if (!$action && !$product_name) {

>+    my $classification;
>     if (Param('useclassification')) {
>+        $classification = 
>+            Bugzilla::Classification::check_classification($classification_name);
> 
>+        @products =
>+            Bugzilla::Product::get_products_by_classification($classification->id);

Two things:

1) Product::get_products_by_classification() no longer exists, see bug 306601.
Use $classification->products instead.

2) $classification is only used in this IF block. So its declaration should be
in this block too.


>if ($action eq 'new') {

>+    my $classification;
>     my $classification_id = 1;
>     if (Param('useclassification')) {
>+        $classification = 
>+            Bugzilla::Classification::check_classification($classification_name);
>+        $classification_id = $classification->id;
>         $vars->{'classification'} = $classification;
>     }

Same comment as above: $classification is only used in the IF block.


>+    if ($product) {
> 
>         # Check for exact case sensitive match:
>+        if ($product->name eq $product_name) {
>             ThrowUserError("prod_name_already_in_use",
>                            {'product' => $product});
>         }

The error message takes a string => $product->name.


>         # Next check for a case-insensitive match:
>+        if (lc($product->name) eq lc($product_name)) {
>             ThrowUserError("prod_name_diff_in_case",
>+                           {'product' => $product_name,
>+                            'existing_product' => $product}); 
>         }

Same comment here => $product->name instead of $product.


>     if ($description eq '') {
>         ThrowUserError("product_must_have_description",
>                        {'product' => $product});
>     }

Same comment here => $product_name (and not $product->name which doesn't exist
here!).


> if ($action eq 'delete') {

>+    if ($product->bug_count) {
>         if (Param("allowbugdeletion")) {
>+            foreach my $bug_id (@$product->bug_ids) {

Write @{$product->bug_ids}.


> if ($action eq 'update') {

>+    SendSQL("SELECT value FROM milestones " .
>+            "WHERE value = " . SqlQuote($defaultmilestone) .
>+            "  AND product_id = " . $product_old->id);
>+    if (!FetchOneColumn()) {
>+        ThrowUserError('prod_must_define_defaultmilestone',

As you want to know whether the milestone is valid for this product, you should
write:

my $milestone = new Bugzilla::Milestone($product_old->id, $defaultmilestone);
if (!$milestone) { ThrowUserError(...) }

Moreover, to avoid any race condition, this test should be done while tables
are locked, but before any change is made into the DB.


>+    unless ($product_name) {
>+        ThrowUserError('prod_cant_delete_name',
>+                       {product => $product_old->name});
>+    }

Nit: this test should be done before validating votes (I see no interest in
validating votes if the product name is missing).


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

Here again, in order to avoid a race condition, this test should be done while
tables are locked, before any change in the DB.


>+    my $qp = SqlQuote($product_name);
>+    my $qpold = SqlQuote($product_old->name);

Nit: $qpold is never used.


>+    if ($product_name ne $product_old->name) {
>+        SendSQL("UPDATE products SET name=$qp WHERE id=".$product_old->id);

Nit: add whitespaces: ... WHERE id= " . $product_old->id


>-    $vars->{'name'} = $product;
>+    $vars->{'product'} = $product_old;

This is wrong! $product_old doesn't contain the new product name!! As we are
converting all templates to use objects, we should do the same with
updated.html.tmpl. To do this, the best thing to do is to build the updated
product object as soon as tables are unlocked, and pass both the old and new
objects to the template. This allows us to remove all these $vars->{'old_xxx'}
and $vars->{'new_xxx'}. In templates, you can access the old and new values
using old_product.xxx and new_product.xxx.



>Index: Bugzilla/Product.pm

>+=item C<bug_count()>
>+
>+ Description: Returns the ids of bugs that belong to the product.

The new method is bug_ids()! Moreover, write 'IDs' instead of ids.



>Index: template/en/default/admin/products/confirm-delete.html.tmpl

>+          [% product.bug_count %]

>+    [% IF product.bug_count > 1 %]
>+      are [% product.bug_count FILTER html %] [%+ terms.bugs %]

Either 'product.bug_count' is in filterexceptions.pl and you don't need to
filter it (in this case, filterexception.pl is missing in this patch), or it
isn't and in this case there are some missing 'FILTER html'.



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

>+[%# INTERFACE:
>+  # product: Bugzilla::Product object; the Product created.
>+  #
>+  # classification: Bugzilla::Classification object; If classifications
>+  #                         are enabled, then this is 
>+  #                         the currently selected classification
>+  #
>+  #%]

No need to mention 'classification'. It's not used in this template.



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

>+[%# INTERFACE:
>+  # product: Bugzilla::Product object; The product
>+  #
>+  # (classification fields available if Param('useclassification') is enabled:)
>+  #
>+  # classification: Bugzilla::Classification object; The classification 
>+  #                                                  the product is in
>+  #
>+  #%]

Same comment here; classification isn't used in this template.



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

>+  # groups_controls: an hash of group controls related to the product.
>   #%]

Nit: *a* hash


>+          [% FOREACH v = product.versions %]
>+            [% v.value FILTER html %]

Write 'v.name'.


>+            [%- FOREACH m = product.milestones -%]
>+              [% m.value FILTER html %]

Write 'm.name'.


<a href="editproducts.cgi?action=editgroupcontrols&product=
  [%- product.name FILTER url_quote %]&classification=
  [%- classification FILTER url_quote %]">
  Edit Group Access Controls:

classification is an object, so you have to write 'classification.name'.


>+        [% IF group_controls.size %]
>+          [% FOREACH g = group_controls.values %]
>             <b>[% g.name FILTER html %]:</b>&nbsp;
>             [% IF g.isactive %]
>               [% g.membercontrol FILTER html %]/

I'm not sure this still works with the rewrite of Product::group_controls().



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

> [%# INTERFACE:
>   # name: string; the name of the product

'name' no longer exists. We have a 'product' object now.


>+[% IF product.name && !no_edit_product_link %]

Nit: 'product' instead of 'product.name'.


>   Edit product <a 
>+  title="Edit Product '[% product.name FILTER html %]' 
>          [% classification_text %]"
>   href="editproducts.cgi?action=edit&amp;product=
>+        [%- product.name FILTER url_quote %]
>+        [% classification_url_part %]">

Write [%- instead of [% alone to avoid whitespaces in the URL.


>-[% IF Param('useclassification') && classification %]
>+[% IF Param('useclassification') && classification.name %]

Nit: 'classification' alone suffices.



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

Update the INTERFACE, especially about objects (instead of strings).

Update also all directories considering my comment in editproducts.cgi (old_xxx
and new_xxx replaced by old_product.xxx and new_product.xxx).



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

Update the INTERFACE.

Comment 17

[André Batosti]

Attachment: batosti_v5

Comment 18

[Frédéric Buclin]

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

Argh.... we were very close to a r+, but there are a few things left to fix.


>Index: editproducts.cgi

>+    my $milestone = new Bugzilla::Milestone($product_old->id,
>+                                            $defaultmilestone);

As we now call Milestone.pm here, we should add 'use Bugzilla::Milestone'.


>if ($action eq 'update') {
>                         WHERE product_id = ?
>                         AND bug_status = 'UNCONFIRMED'
>                         AND votes >= ?",
>-                        undef, ($product_id, $votestoconfirm));
>+                        undef, ($product_old->id, $votestoconfirm));

Nit: I know both results are the same, but I prefer $product->id than
$product_old->id here, for consistency with what we do above.



>Index: Bugzilla/Product.pm

> =back
> 
>+=item C<bug_ids()>

There must be no '=back' before a '=item'. This generates an error when running
runtests.pl:

*** ERROR: =item without previous =over at line 350 in file Bugzilla/Product.pm
not ok 79 - Bugzilla/Product.pm has incorrect POD syntax --ERROR
#     Failed test (t/011pod.t at line 56)



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

> [% PROCESS "admin/products/footer.html.tmpl" no_add_product_link = 1 %]

You should also call footer.html.tmpl with 'no_edit_product_link = 1' as you
cannot edit the product yet. This generates an incorrect URL.



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

Nit: please remove the trailing &amp; in the link below.
classification_url_part already has one:

[% bug_count_contentlink = BLOCK %]buglist.cgi?product=%%name%%&amp;
  [%- classification_url_part %][% END %]



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

> [%# INTERFACE:

>+  # old_product : Bugzilla::Product Object; old prodcut.

Nit: s/prodcut/product/


>-  # changer: string; user id of the user making the changes, used for mailing
>-  #          bug changes if necessary

'changer' is still in use. Don't remove it from the INTERFACE.


>+[% IF product.votes_to_confirm != old_product.votes_to_confirm%]

Nit: add a whitespace before %]



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

>+[% PROCESS global/header.html.tmpl %]

Nit: add 'title = title' when calling header.html.tmpl (this is common use, but
not mandatory).


I tested your patch already and it works great! Fix my comments and I think the
next review should be the final one. ;)

Comment 19

[André Batosti]

Attachment: batosti_v5_fix

Comment 20

[André Batosti]

Attachment: batosti_v5_fix_fix

Comment 21

[André Batosti]

Attachment: batosti_v5_fix_fix_fix

Comment 22

[Frédéric Buclin]

Comment on attachment 199814 [details] [diff] [review]
batosti_v5_fix_fix_fix

great work, thanks! :) r=LpSolit

Comment 23

[Frédéric Buclin]

Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.102; previous revision: 1.101
done
Checking in Bugzilla/Product.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Product.pm,v  <--  Product.pm
new revision: 1.13; previous revision: 1.12
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.57; previous revision: 1.56
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.2; previous revision: 1.1
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.2; previous revision: 1.1
done
Checking in template/en/default/admin/products/created.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/created.html.tmpl,v
 <--  created.html.tmpl
new revision: 1.2; previous revision: 1.1
done
Checking in template/en/default/admin/products/deleted.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/deleted.html.tmpl,v
 <--  deleted.html.tmpl
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/admin/products/edit-common.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/edit-common.html.tmpl,v
 <--  edit-common.html.tmpl
new revision: 1.2; previous revision: 1.1
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.4; previous revision: 1.3
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.6; previous revision: 1.5
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.2; previous revision: 1.1
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.2; previous revision: 1.1
done
Checking in template/en/default/admin/products/groupcontrol/confirm-edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/groupcontrol/confirm-edit.html.tmpl,v
 <--  confirm-edit.html.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/admin/products/groupcontrol/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/groupcontrol/edit.html.tmpl,v
 <--  edit.html.tmpl
new revision: 1.6; previous revision: 1.5
done
Checking in template/en/default/admin/products/groupcontrol/updated.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/groupcontrol/updated.html.tmpl,v
 <--  updated.html.tmpl
new revision: 1.2; previous revision: 1.1
done