302370 - remove the EmitFormElements routine from editproducts.cgi and templatize that code

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/20050524 Firefox/1.0 (Ubuntu package 1.0.2 MFSA2005-44)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.6) Gecko/20050524 Firefox/1.0 (Ubuntu package 1.0.2 MFSA2005-44)

This routine is used to action add and edit. We could create a common template
file for both.

Reproducible: Always

Steps to Reproduce:

Comment 1

[Gabriel Sales de Oliveira]

Attachment: v1-emit_form_elements no longer exists

Template create-edit.html.tmpl created for add and edit a product.

In the edit action, i called a product object and send it to the template.

Comment 2

[Frédéric Buclin]

Comment on attachment 191406 [details] [diff] [review]
v1-emit_form_elements no longer exists

The goal of this bug is to remove EmitFormElements from editproducts.cgi
*only*. This means:
- don't replace $product by $product_name (among others). There is no relation
with what we are doing now. We can do that later when moving the code to use
Product.pm
- adding and editing a product really need two separate templates. The "Block
for update fields" section is bigger than the rest of the file, meaning than
more than 50% of the script is useless in one case!!
This will also give us more flexibility.

Comment 3

[Frédéric Buclin]

Looking at editproducts.cgi a bit closer, here is what should be done:

1) move EmitFormElements() in its own template, let's say
admin/products/edit-common.html.tmpl.

2) create admin/products/create.html.tmpl which contains the code specific to
new products. The common template above is called using
[% PROCESS "admin/products/edit-common.html.tmpl" %]
somewhere from admin/products/create.html.tmpl.

3) create admin/products/edit.html.tmpl which contains the code specific to
existing products. The common template is called as above.

Comment 4

[Gabriel Sales de Oliveira]

Attachment: v2-A different propose

I made the templates structure a little bit different, is that good?  :-?

Comment 5

[Frédéric Buclin]

Comment on attachment 193965 [details] [diff] [review]
v2-A different propose

>Index: editproducts.cgi

>+use Bugzilla::Product;

Templatizing EmitFormElements() doesn't require to convert old code to use
Product.pm.


> if ($action eq 'add') {

>+    $vars->{'classification'} = $classification;
>+    $vars->{'action'} = 'new';
>+    $template->process("admin/products/edit-common.html.tmpl",
>+                       $vars)
>+      || ThrowTemplateError($template->error());
>     exit;
> }

This is not the right way to go. As I said in my previous comment,
admin/products/create.html.tmpl should be called from here, and this template
will call admin/products/edit-common.html.tmpl internally. $vars->{'action'} =
'new' really doesn't make sense! Reread my previous comment if required.


> if ($action eq 'new') {

>-    if ($existing_product) {
>-
>-        # Check for exact case sensitive match:
>-        if ($existing_product eq $product) {
>-            print "The product '$product' already exists. Please press\n";
>-            print "<b>Back</b> and try again.\n";
>-            PutTrailer($localtrailer);
>-            exit;
>-        }
>-
>-        # Next check for a case-insensitive match:
>-        if (lc($existing_product) eq lc($product)) {
>-            print "The new product '$product' differs from existing product ";
>-            print "'$existing_product' only in case. Please press\n";
>-            print "<b>Back</b> and try again.\n";
>-            PutTrailer($localtrailer);
>-            exit;
>-        }
>-    }
>-
>+    # Check for exact case sensitive match:
>+    if (lc($existing_product) eq lc($product)) {
>+      ThrowUserError('product_already_exists', {'product' => $existing_product});
>+    } 

There is a good reason to have two separate tests here. So please leave them
both!


>+    $vars->{'product'} = $product;
>+    $vars->{'classhtmlvar'} = $classhtmlvar;
>+    $template->process("admin/products/created.html.tmpl", $vars)
>+        || ThrowTemplateError($template->error());

'classhtmlvar' should be banned! Use the existing
admin/products/footer.html.tmpl instead (called internally by
admin/products/created.html.tmpl).


> if ($action eq 'edit' || (!$action && $product)) {
>-    $template->put_header("Edit product");
>-    CheckProduct($product);
>-    my $classification_id=1;
>-    if (Param('useclassification')) {

The goal if this bug, as already said several times, is to templatize
EmitFormElements! Moving old code to new code is definitely *not* the goal of
this patch!! Doing so decreases your chances to get a r+ due to the additional
bugs or regressions you may introduce. So please leave this code alone for now.


>+    $template->process("admin/products/edit-common.html.tmpl", $vars)
>+        || ThrowTemplateError($template->error());
>     exit;

Note that you call the same template as before for two different actions!
Re-re-read my previous comment and call admin/products/edit.html.tmpl from
here.



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

The common part of the code should be in admin/products/edit-common.html.tmpl!


>+    [% IF action == 'update' %]
>+      [% PROCESS "admin/products/edit.html.tmpl" %]
>+    [% END %]
>+    
>+    [% IF action == 'new' %]

This is the kind of tests which doesn't make sense if your templates are
correctly written. In other words, remove them.



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

>+  <a href="editproducts.cgi?action=add">add</a> a new product,
>+  <a href="editcomponents.cgi?action=add&product=[% product FILTER url_quote %][% classhtmlvar FILTER url_quote %]">

As I said, 'classhtmlvar' should die and this template should call
admin/products/footer.html.tmpl instead.



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

>+[% IF action == 'new' %]
>+  [% title = BLOCK %]Add Product[% END %]
>+[% ELSE %]
>+  [% title = BLOCK %]Edit Product[% END %]
>+[% END %]

Same comment as above about this kind of tests.


>+[% PROCESS "admin/products/create.html.tmpl" %]

You use these templates in the wrong way. admin/products/edit-common.html.tmpl
should contain the common code for both admin/products/create.html.tmpl and
admin/products/edit.html.tmpl and should be called from them. This answer your
question: no, your template structure is not good.


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

Previous comments apply here too.

Comment 6

[Gabriel Sales de Oliveira]

Attachment: v3-changes

EmitFormElements move to template and ($action edit) database queries changed.

Comment 7

[Frédéric Buclin]

Comment on attachment 194335 [details] [diff] [review]
v3-changes

>Index: editproducts.cgi

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

>     # get data of product
>+    trick_taint($product);

Move this detaint right after CheckProduct($product). This way, you don't need
to have it twice, once in the "if (Param('useclassification'))" block and a
second time here.


>+    my $product_data = $dbh->selectrow_hashref(qq{
>+            SELECT products.id, products.name, 
>+                   products.classification_id, products.description,
>+                   products.milestoneurl, products.disallownew,
>+                   products.votesperuser, products.maxvotesperbug,
>+                   products.votestoconfirm, products.defaultmilestone
>+              FROM products
>+             WHERE name = ?}, undef, $product);

Nit: As only the 'products' table is involved, you don't need to repeat it
everywhere. Remove these useless 'product.'; this will make the query lighter
and easier to read.


>+    $vars->{'product'} = $product_data;

Also, instead of using an intermediate variable, you could directly write:

$vars->{'product'} =
    $dbh->selectrow_hashref(...);

Do the same for other variables.


>+    my $components = $dbh->selectall_hashref(qq{
>+            SELECT name, id, description
>+              FROM components
>+             WHERE product_id = ?}, 'id', undef, ($product_id));
> 
>+    $vars->{'components'} = [values %$components];

selectall_hashref() is not recommended per the DBI doc itself. Easier is to use
selectall_arrayref() and {'Slice' => {}} as %argument:

$vars->{'components'} = $dbh->selectall_arrayref('SELECT name, description
						  FROM components
						  WHERE product_id = ?',
						  {'Slice' => {}},
$product_id);

This way, you get exactly the same result as what you did, but in a more
elegant and maybe more efficient way. ;)


>+    my $milestones = $dbh->selectcol_arrayref(q{
>+        SELECT value FROM milestones
>+        WHERE product_id = ?}, undef, $product_id);

Only check for milestones if Param('usetargetmilestone') is turned on.


>+    my $query = qq{SELECT
>+                       groups.id, groups.name, groups.isactive,
>+                       group_control_map.entry,
>+                       group_control_map.membercontrol,
>+                       group_control_map.othercontrol,
>+                       group_control_map.canedit
>+                  FROM groups
>+                  LEFT 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));

Same remark here as for components: use selectall_arrayref() and {'Slice' =>
{}}.


>+    my $constants = {
>+      &::CONTROLMAPNA => 'NA',
>+      &::CONTROLMAPSHOWN => 'Shown',
>+      &::CONTROLMAPDEFAULT => 'Default',
>+      &::CONTROLMAPMANDATORY => 'Mandatory'};

&:: is not required anymore as constants have been moved into Constants.pm.

>+
>+    foreach my $group (values %$groups) {
>+        $group->{'membercontrol'} =
>+            $constants->{$group->{'membercontrol'}};
>+        $group->{'othercontrol'} =
>+            $constants->{$group->{'othercontrol'}};
>     }

Hum... I'm not sure this is the right place to do it. You should move it in
templates.


>+    my $bug_count = $dbh->selectrow_array(qq{
>+            SELECT COUNT(bug_id) FROM bugs
>+            WHERE product_id = ?}, undef, $product_id);
>+    
>+    $vars->{'bug_count'} = $bug_count;

Use COUNT(*) instead; this makes the query in the DB faster. And again, don't
use this useless intermediate variable.



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

>+[%# INTERFACE:
>+  #         action: string; The page action {new|update}. 
>+  # classification: string; name of classification.
>+  #        product: Bugzilla::Product object.
>+  #     components: array; Bugzilla::Component object(s) 
>+  #                 related to product.
>+  #         groups: array; Bugzilla::Group object(s) related to
>+  #                 product.
>+  #%]

The only param which is passed to create.html.tmpl is 'classification'. Also,
be more explicit:
"classification: string; name of the classification in which the new product is
created" or something like that.


>+[% title = BLOCK %]Add Product[% END %]
>+
>+[% PROCESS global/header.html.tmpl
>+  title = title
>+  h2 = h2
>+%]

No need to mention 'h2' as it is not defined.


>+<form method="post" action="editproducts.cgi">
>+  <table border="0" cellpadding="4" cellspacing="0">
>+
>+  [% PROCESS "admin/products/edit-common.html.tmpl" %]
>+
>+  <tr>
>+    <th align="right">Version:</th>
>+    <td><input size="64" maxlength="255" name="version" 
>+              value="[% version FILTER html %]"></td>
>+  </tr>

Please respect the indentation:
<form
  <table
    [% PROCESS

    <tr>
      <th

Also, align (vertically) </td> with <td> when you need more than one line to
write it.


>+  <th align="right">Create chart datasets for this product:</th>
>+  <td><input type="checkbox" name="createseries" value="1"></td>

This needs to be enclose in <tr> </tr> tags.


>+  <input type="hidden" name="subcategory" value="-All-">
>+  <input type="hidden" name="open_name"   value="All Open">
>+  </table>

Hidden fields should be moved out of the table, but before </form>.



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

>+[%# INTERFACE:
>+  #         action: string; The page action {new|update}. 
>+  # classification: string; name of classification.
>+  #        product: Bugzilla::Product object.
>+  #     components: array; Bugzilla::Component object(s) 
>+  #                 related to product.
>+  #         groups: array; Bugzilla::Group object(s) related to
>+  #                 product.
>+  #%]

Please don't say "Bugzilla::Foo object(s)" but "an array of foo objects". Also,
only 'classification' and 'product' exist here; remove the other ones.


>+[% IF Param('useclassification') %]
>+  <tr>
>+    <td align="right"><b>Classification:</b></th>

Oops... you have a <td> </th> pair instead of <th> </th>.


>+  <td><textarea rows="4" cols="64" wrap="virtual" 
>+                name="description">[% product.description FILTER html
>+%]</textarea></td>

Let %] on the previous line.


>+[% IF Param('usetargetmilestone') -%]
>+  <tr>
>+    <th align="right">URL describing milestones for this product:</th>
>+    <td><input type="text" size="64" maxlength="255" name="milestoneurl"
>+               value="[% product.milestoneurl FILTER html %]"></td>
>+   </tr>
>+   <tr>
>+     <th align="right">Default milestone:</th>
>+     <td><input type="text" size="20" maxlength="20" name="defaultmilestone"
>+                value="[% product.defaultmilestone FILTER html %]"></td>
>+   </tr>
>+[% END %]

Incorrect indentation among <tr> </tr> tags.


>+  <th align="right">
>+    Number of votes a [% terms.bug %] in this product needs to&nbsp;

"a [% terms.bug %]" has to be "[% terms.abug %]".



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

>+[%# INTERFACE:
>+  #         action: string; The page action {new|update}. 
>+  # classification: string; name of classification.
>+  #        product: Bugzilla::Product object.
>+  #     components: array; Bugzilla::Component object(s) 
>+  #                 related to product.
>+  #         groups: array; Bugzilla::Group object(s) related to
>+  #                 product.
>+  #%]

Same comments as before. 'action' doesn't not exist.


>+[% title = BLOCK %]Edit Product[% END %]
>+
>+[% PROCESS global/header.html.tmpl
>+  title = title
>+  h2 = h2
>+%]

Again, 'h2' is not defined.


>+<form method="post" action="editproducts.cgi">
>+  <table border="0" cellpadding="4" cellspacing="0">
>+
>+  [% PROCESS "admin/products/edit-common.html.tmpl" %]
>+
>+  <tr>

Same problems as before about the indentation.


>+      [% IF components.size -%]
>+        [% FOREACH component = components %]
>+          [% UNLESS loop.first %]<br />[% END %]

Wouldn't it be easier to have <br> at the end of the FOREACH loop? This way,
you don't need this test (a final <br> is fine).


>+      [%- IF versions.size -%]
>+        [% FOREACH v = versions %]
>+          [% UNLESS loop.first %]<br />[% END %][% v FILTER none %]

Same remark here.


>+        [%- IF milestones.size -%]
>+          [%- FOREACH m = milestones -%]
>+            [% UNLESS loop.first %]<br />[% END %][% m FILTER none %]

Here again.


>+      [% IF groups.size %]
>+        [% FOREACH g = groups %]
>+          [% UNLESS loop.first %]<br />[% END %]

Here again.


>+  <input type="hidden" name="productold" value="[% product.name FILTER html
>+%]">
>+  <input type="hidden" name="descriptionold"
>+         value="[% product.description FILTER html %]">
>+  <input type="hidden" name="milestoneurlold" 
>+         value="[% product.milestoneurl FILTER html %]">
>+  <input type="hidden" name="disallownewold"
>+         value="[% product.disallownew FILTER html %]">
>+  <input type="hidden" name="votesperuserold"
>+         value="[% product.votesperuser FILTER html %]">
>+  <input type="hidden" name="maxvotesperbugold" 
>+         value="[% product.maxvotesperbug FILTER html %]">
>+  <input type="hidden" name="votestoconfirmold" 
>+         value="[% product.votestoconfirm FILTER html %]">
>+  <input type="hidden" name="defaultmilestoneold"
>+         value="[% product.defaultmilestone FILTER html %]">
>+  </table>

Move all these hidden fields out of the table, but before </form>.


I haven't applied your patch yet, so I may find other bugs. But what I have
seen so far looks very good. :)

Comment 8

[Gabriel Sales de Oliveira]

Attachment: v3-fix

Fixed version.

Comment 9

[Frédéric Buclin]

Comment on attachment 194449 [details] [diff] [review]
v3-fix

>Index: editproducts.cgi

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

>+    $vars->{'product'} = $dbh->selectrow_hashref(qq{
>+            SELECT id, name, classification_id, description,
>+                   milestoneurl, disallownew, votesperuser,
>+                   maxvotesperbug, votestoconfirm, defaultmilestone
>+              FROM products
>+             WHERE name = ?}, undef, $product);

Nit: as you already have the product ID, you could query the DB using it
instead of its name (that's the way Product.pm will be used here).


>+    $vars->{'components'} = $dbh->selectall_arrayref(qq{
>+            SELECT name, description
>+              FROM components
>+             WHERE product_id = ?}, {'Slice' => {}},$product_id); 

Please sort components by name => ORDER BY name.


>+    $vars->{'versions'} = $dbh->selectcol_arrayref(q{
>+        SELECT value FROM versions
>+        WHERE product_id = ?}, undef, $product_id);

Sort versions by value.


>+        $vars->{'milestones'} = $dbh->selectcol_arrayref(q{
>+                                    SELECT value 
>+                                      FROM milestones
>+                                     WHERE product_id = ?}, 
>+                                     undef, $product_id);

Sort milestones by sortkey and value.


>+    my $query = qq{SELECT
>+                       groups.id, groups.name, groups.isactive,
>+                       group_control_map.entry,
>+                       group_control_map.membercontrol,
>+                       group_control_map.othercontrol,
>+                       group_control_map.canedit
>+                  FROM groups
>+                  LEFT JOIN group_control_map

Should be INNER JOIN.


>+    my $constants = {
>+        (CONTROLMAPNA) => 'NA',
>+        (CONTROLMAPSHOWN) => 'Shown',
>+        (CONTROLMAPDEFAULT) => 'Default',
>+        (CONTROLMAPMANDATORY) => 'Mandatory'};

$constants is useless if you don't use it. ;)



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

>+  Back to the <a href="query.cgi">query page</a> or
>+  <a href="editproducts.cgi">edit</a> other products.
>+
>+[% PROCESS global/footer.html.tmpl %]

Instead of "Back to ....", you should insert [% PROCESS
admin/products/footer.html.tmpl %].



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

>+[%# INTERFACE:
>+  # classification: string; name of classification product is in.
>+  #        product: array;  an array of objects.
>+  #%]

An array of which objects?? please be more precise: "an array of product
objects".


>+  <th align="right">
>+    Number of votes a [% terms.abug %] in this product needs to&nbsp;
>+    automatically get out of the

"a [% terms.abug %]" will give you "a a bug" :( Moreover, remove this useless
&nbsp;



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

>+[%# INTERFACE:
>+  # classification: string; name of classification product is in.
>+  #        product: an array of objects.
>+  #     components: array; an array of object(s) related to product.
>+  #         groups: array; an array of objects related to product.
>+  #%]

Again, describe more precisely what kinds of objects we have: we have product
objects, component objects, etc... . Moreover, you forgot to mention version
objects, milestone objects as well as bug_count (integer; number of bugs in
this product).


>+    [% IF Param('usetargetmilestone') %]
>+      <tr>
>+        <th align="right" valign="top">
>+          <a href="editmilestones.cgi?product=[% product.name FILTER url_quote%]">

Nit: add a whitespace between url_quote and %]


>+            [%- FOREACH m = milestones -%]
>+              [% m FILTER none %]

I'm a bit surprised that we allow unfiltered versions and milestones, but this
was already here so don't change it (I just wanted to mention it ;) ).


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

Nit: add a whitespace between url_quote and %]


>+          [% FOREACH g = groups %]
>+            <b>[% g.name FILTER html %]:</b>&nbsp;
>+            [% g.membercontrol FILTER html %]/
>+            [% g.othercontrol FILTER html %]
>+            [% IF g.entry %], ENTRY[% END %]
>+            [% IF g.canedit %], CANEDIT[% END %]
>+            <br>
>+          [% END %]

Are you sure g.membercontrol and g.othercontrol will be correctly displayed?
Moreover, you have to test whether the group is active or not, and display
DISABLED if required.


>+      <th align="right" valign="top">[% terms.Bugs %]:</th>
>+      <td>[% bug_count FILTER html %]</td>

Nit: valign="top" is not required here.


>+  Back to the <a href="query.cgi">query page</a> or
>+  <a href="editproducts.cgi">edit</a> other products.
>+
>+[% PROCESS global/footer.html.tmpl %]

Here again, you should use admin/products/footer.html.tmpl instead of "Back
to...".


Your patch is now good enough to be tested. I will test it in a few
minutes/hours and I may add some additional comments if I find new bugs. Feel
free to update your patch meanwhile. :)

Comment 10

[GavinS]

(In reply to comment #9)
> >+            [%- FOREACH m = milestones -%]
> >+              [% m FILTER none %]
> 
> I'm a bit surprised that we allow unfiltered versions and milestones, but this
> was already here so don't change it (I just wanted to mention it ;) ).

One of the fixes we are doing during templatisation is to fix some of the places where we don't 
currently filter, but we should be, so I think that the version and milestones should be filtered.

Basically, the only field which is not filtered is the description (same for component description, 
keyword description, group description etc.)

Comment 11

[Gabriel Sales de Oliveira]

Attachment: v4-patch

New version.

Comment 12

[Gabriel Sales de Oliveira]

Attachment: v4-fix

Some nits fixed

Comment 13

[Frédéric Buclin]

Comment on attachment 194562 [details] [diff] [review]
v4-fix

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

>+  <th align="right">Description:</th>
>+  <td><textarea rows="4" cols="64" wrap="virtual" 
>+                name="description">[% product.description FILTER none %]
>+  </textarea></td>

Nit: </textarea> should be on the previous line to avoid two leading
whitespaces. I will fix that on checkin.


Your patch works fine, the code looks good. r=LpSolit

Comment 14

[Frédéric Buclin]

Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.95; previous revision: 1.94
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/create.html.tmpl,v
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
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/edit-common.html.tmpl,v
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
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/edit.html.tmpl,v
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
initial revision: 1.1
done