289580 - Templatize the 'confirm delete' bit of editproducts

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[GavinS]

Comment 1

[GavinS]

Attachment: templatize the 'confirm delete' bit v1

Comment 2

[Frédéric Buclin]

Comment on attachment 180078 [details] [diff] [review]
templatize the 'confirm delete' bit v1

where is the new template, admin/products/confirm-delete.html.tmpl  ?

Comment 3

[GavinS]

Attachment: templatize confirm delete bit of editproducts v2 (complete now!)

Comment 4

[Frédéric Buclin]

Comment on attachment 180414 [details] [diff] [review]
templatize confirm delete bit of editproducts v2 (complete now!)

>+sub CheckProductNew ($)
>+{
>+    my $prod = shift;
>+
>+    # do we have a product?
>+    unless ($prod) {
>+        ThrowUserError('product_not_specified');
>+    }
>+
>+    unless (TestProduct $prod) {
>+        ThrowUserError('product_doesnt_exist',
>+                       {product => $prod});
>+    }
>+}

Why defining new functions such as TestProduct() as we already have
get_product_id() & co defined in globals.pl? You could simplify the code using:

get_product_id($prod) || ThrowUserError();

We should really avoid code duplication and we should only use
get_(product|component|classification)_(id|name) from globals.pl instead. So I
would *really* appreciate if you could start using them from now and remove
these useless functions such as TestProduct() and TestClassification().


>+# XXX Shouldn't these SQL statements include: 'classifications.id =
>+# products.classification_id' ???

Of course they have to! Because you have to check that an existing product is
really in the existing classification it pretends to be.


I did not go further in the review because these two points are enough for a
review-.

Comment 5

[GavinS]

(In reply to comment #4)
> (From update of attachment 180414 [details] [diff] [review] [edit])
> I did not go further in the review because these two points are enough for a
> review-.
> 

Could those 2 issues be fixed in seperate bugs maybe? I know we have sneaked
fixes in during templatisation, but those 2 extra changes would need a lot more
testing.
(Note that I do have the long untouched bug#257240 for one of these issues)

Comment 6

[Tiago Mello [:timello]]

GavinS,
   Could we give more attention for bug 293524 and after that we fix this? I
already did some fixes in your patch and I attached with my patch. But I think
that can be more easy land one of them first. I don't care about the order, but
if you care, let me know please, right?

Comment 7

[Tiago Mello [:timello]]

Attachment: v3: GavinS's patch with some fixes.

Comment 8

[Frédéric Buclin]

Comment on attachment 187835 [details] [diff] [review]
v3: GavinS's patch with some fixes.

>Index: editproducts.cgi

>+    trick_taint($product);
>+    get_product_id($product) || ThrowUserError('product_doesnt_exist',
>+                                               {product => $product});

We need the product ID later in the code. You could get it from here already:

my $product_id = get_product_id($product);
$product_id || ThrowUserError('product_doesnt_exist', {product => $product});


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

Be careful! CheckClassificationProduct() has a missing join condition! You have
to fix that too!


>+    # Extract some data about the product
>+    my $query = q{SELECT classifications.description,
>+                         products.id,
>+                         products.description,
>+                         products.milestoneurl,
>+                         products.disallownew
>+                  FROM products
>+                  INNER JOIN classifications
>+                    ON products.classification_id = classifications.id
>+                  WHERE products.name = ?
>+                  AND classifications.id = ?};

This query does not make sense to me. classifications.id is used in the join
condition, so giving the product name only, or even better it's ID obtained
above (see my previous comment) suffices.


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

We have classifications, products, components, etc... involved here. So 'name'
is not the best choice. Please replace it by 'product' or 'product_name'.


>+    $vars->{'versions'} = $dbh->selectall_arrayref(q{
>+            SELECT value FROM versions
>+            WHERE product_id = ? ORDER BY value}, {'Slice' => {}},
>+            $product_id);

Only one column is returned. Choose $dbh->selectcol_arrayref().


>+        $vars->{'milestones'} = $dbh->selectall_arrayref(q{
>+            SELECT value FROM milestones
>+            WHERE product_id = ?
>+            ORDER BY sortkey, value}, {'Slice' => {}}, $product_id);

Same remark here: only one column returned.


>+    ($vars->{'bug_count'}) = $dbh->selectrow_array(q{
>+        SELECT count(bug_id), product_id FROM bugs } . 
>+        $dbh->sql_group_by('product_id') . q{ HAVING product_id = ?},
>+        undef, $product_id);
>+ 
>+    $vars->{'bug_count'} ||= 0;

Even better:
$vars->{'bug_count'} = $dbh->selectrow_array("SELECT COUNT(*) FROM bugs WHERE
product_id = ?", undef, $product_id);


@@ -775,9 +684,11 @@
>if ($action eq 'delete') {
>     CheckProduct($product);
>     my $product_id = get_product_id($product);

Do you really want to keep CheckProduct() here? I would prefer the same change
as in the previous section, that is:

ThrowUserError('product_not_specified') unless $product;
my $product_id = get_product_id($product);
$product_id || ThrowUserError('product_doesnt_exist', { product => $product });


>     $dbh->bz_lock_tables('products WRITE', 'components WRITE',
>                          'versions WRITE', 'milestones WRITE',
>                          'group_control_map WRITE',
>-                         'flaginclusions WRITE', 'flagexclusions WRITE');
>+                         'flaginclusions WRITE',
>+                         'flagexclusions WRITE');

Nit: no need to move 'flagexclusions WRITE' on a new line.


>-    $dbh->do("DELETE FROM components WHERE product_id = ?", undef, $product_id);
>-    print "Components deleted.<BR>\n";
>+    $dbh->do(q{DELETE FROM components WHERE product_id = ?},
>+             undef, $product_id);
> 
>-    $dbh->do("DELETE FROM versions WHERE product_id = ?", undef, $product_id);
>-    print "Versions deleted.<BR>\n";
>+    $vars->{'components_deleted'} = 1; 

Two things here:
1) Nit: no need to replace quotes by q{}
2) All these $vars->{} are useless as they are set to 1 in all cases (there is
no check whether something has really been removed or not). So you can remove
them completely for now. If we later want to know what has been removed
exactly, we can fix this in another bug.


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

>+[% IF disallownew %]
>+  [% disallownew = "closed" %]
>+[% ELSE %]
>+  [% disallownew = "open" %]
>+[% END %]
>+
>+<p>
>+<table border="1" cellpadding="4" cellspacing="0">

Nit: Useless <p>


>+<tr bgcolor="#6666FF">
>+  <th valign="top" align="left">Field</th>
>+  <th valign="top" align="left">Value</th>
>+</tr>

Please respect the indentation. Don't align <tr> with <table> but add 2
whitespaces instead:

<table>
  <tr>
    <td>
      ....
    </td>
    <td>
      ....
    </td>
  </tr>
</table>

Please apply this in the whole file.


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

Same remark here: respect the identation in blocks (IF, UNLESS, FOREACH):

[% IF something %]
  <tr>
    ....
  </tr>
[% END %]


>+[% IF Param('usetargetmilestone') %]
>+<tr>
>+  <td>Milestone URL:</td>
>+  <td>[% IF milestoneurl %]
>+        <a href="[% milestoneurl FILTER uri %]">
>+                 [%- milestoneurl FILTER html %]</a>
>+      [% ELSE %]
>+        none
>+      [% END %]</td>
>+</tr>

When you need more than one line to write the content of a cell, please put
<td> and </td> tags on their own line:
    <td>
      [% IF something %]
	....
      [% ELSE %]
	....
      [% END %]
    </td>

This comment applies everywhere in these templates.


>+  <td>Closed for [% terms.bugs %]:</td>
>+  <td>[% IF dissallownew %]Yes[% ELSE %]no[% END %]</td>

This won't work! You redefined 'dissallownew' at the beginning of this file.
Simply write:
<td>Closed for [% terms.bugs %]:</td>
<td>[% disallownew FILTER html %]</td>


>+  <td>[% IF components.size > 0 %]<a
>+         href="editcomponents.cgi?product=[% name FILTER url_quote %]

Nit: write "<a" on the same line than "href=".



>+    [% IF components.size %]
>+      <table>

Nit: write [% IF components.size > 0 %] to be sure. I remember a patch where
adding "> 0" fixed a problem.


>+  <td>[% terms.Bugs %]:</td>
>+  <td>[% IF bug_count %]
>+        <a title="List of [% terms.bugs %] for product '
>+             [%- name FILTER html %]'"
>+           href="buglist.cgi?product=
>+             [%- name FILTER url_quote %]
>+             [%- classification_url_part %]">[% bug_count %]</a>

Nit: we usually write "href" before "title".


>+      When you delete this
>+      product, <b><blink>ALL</blink></b> stored [% terms.bugs %] will be deleted,
>+      too.
>+      You could not even see the [% terms.bug %] history for this product anymore!

A better wording (see admin/components/confirm-delete.html.tmpl):

When you delete this product, <b><blink>ALL</blink></b> stored [% terms.bugs %]
and
their history will be deleted too.


>+    <p class="areyoureallyreallysure">
>+      Please be aware of the consequences of this before continuing.
>+    </p>

I think the administrator has understood the implications of removing a product
already. Remove this sentence.


>+  <form method="post" action="editproducts.cgi">
>+  <input type="submit" value="Yes, delete">
>+  <input type="hidden" name="action" value="delete">
>+  <input type="hidden" name="product" value="[% name FILTER html %]">
>+  <input type="hidden" name="classification" value="[% classification FILTER html %]">
>+  </form>

Nit: again, respect the identation:

<form>
  <input>
  <input>
</form>


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

>+[% IF nb_bugs %]
>+  All references to deleted [% terms.bugs %] removed.
>+[% END %]

$vars->{'nb_bugs'} has not been defined in editproducts.cgi. Fix that!


>+<p>
>+  [% IF components_deleted %]
>+    Components deleted.<br>
>+  [% END %]
>+
>+  [% IF versions_deleted %]
>+    Versions deleted.<br>
>+  [% END %]
>+
>+  [% IF milestones %]
>+    Milestones deleted.
>+  [% END %]
>+</p>
>+
>+<p>
>+  [% IF group_controls_deleted %]
>+    Group controls deleted.<br>
>+  [% END %]
>+
>+  [% IF flags_deleted %]
>+    Flag inclusions and exclusions deleted.
>+  [% END %]
>+</p>
>+
>+<p>
>+  [% IF product_deleted %]
>+    Product [% product FILTER html %] deleted.
>+  [% END %]
>+</p>

All these tests are useless. As said above, all these $vars->{} have been set
to 1, so there is no need to test anything here; we already know what the
answer is.


In spite of all these comments, the patch is already very good. :)

Comment 9

[Tiago Mello [:timello]]

Attachment: v4: fixes.

Comment 10

[Tiago Mello [:timello]]

Attachment: missing some fixes.

Comment 11

[Tiago Mello [:timello]]

Attachment: vfinal: last fixes.

Comment 12

[Frédéric Buclin]

Comment on attachment 188315 [details] [diff] [review]
vfinal: last fixes.

thanks to GavinS and timello for this patch!! :)

r=LpSolit

Comment 13

[Frédéric Buclin]

Comment on attachment 188315 [details] [diff] [review]
vfinal: last fixes.

oops, try to set an invalid classification and see the result: the header is
missing. :(

Comment 14

[Tiago Mello [:timello]]

Attachment: vx: adding CheckClassificationProductNew

Comment 15

[Frédéric Buclin]

Comment on attachment 188333 [details] [diff] [review]
vx: adding CheckClassificationProductNew

>+sub CheckClassificationProductNew ($$)

>+          ON products.classifications_id = classifications_id

ON products.classification_id = classifications.id

Submit a new patch with this line fixed and carry forward my r+. r=LpSolit

Comment 16

[Tiago Mello [:timello]]

Attachment: some fixes.

Comment 17

[Dave Miller [:justdave]]

yeah, why not...  this will make it easier to maintain for the next several
months :)

r=justdave on the filterexceptions.pl change as well (changes to that file
require 2nd review because it affects security).

Comment 18

[Frédéric Buclin]

Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.85; previous revision: 1.84
done
Checking in template/en/default/filterexceptions.pl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/filterexceptions.pl,v 
<--  filterexceptions.pl
new revision: 1.43; previous revision: 1.42
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/confirm-delete.html.tmpl,v
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
initial revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/deleted.html.tmpl,v
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
initial revision: 1.1
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.115; previous revision: 1.114
done