306242 - Templatize the 'update product' bit of editproducts

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[GavinS]

Comment 1

[GavinS]

Attachment: templatize product updated page, v1

Comment 2

[GavinS]

Attachment: templatize product updated page, v2

Sorry about that last patch - it was pretty much pants. So we'll quickly forget
that one and move on...

This patch fixes the comments mentioned in irc, hopefully I got them all:

1) missing filtering
2) variable typos
3) bugword fixes
4) duplicate errors
5) no need for exit after a ThrowUserError()

and changes some CheckXXX() calls to the temporary CheckXXXNew() calls

Comment 3

[Frédéric Buclin]

Comment on attachment 194707 [details] [diff] [review]
templatize product updated page, v2

>Index: editproducts.cgi

>+sub CheckProductNew

>+    unless ($prod) {
>+        ThrowUserError('product_not_specified');
>+        exit;
>+    }

Nit: 'exit' is not required. ThrowUserError() already calls it for you.


>+    unless (TestProduct $prod) {

I would prefer that you use parens when passing arguments to a function:
TestProduct($prod).


>     if (!detaint_natural($maxvotesperbug)) {
>-        print "Sorry, the max votes per bug must be an integer >= 0.";
>-        PutTrailer($localtrailer);
>-        exit;
>+        ThrowUserError('prod_max_votes_must_be_nonnegative',
>+                       {maxvotesperbug => $stored_maxvotesperbug});
>     }

Nit: 'prod_votes_per_bug_must_be_nonnegative' sounds better and is more
consistent with 'prod_votes_per_user_must_be_nonnegative'.


>+    if (Param('usetargetmilestone') &&
>+        ($milestoneurl ne $milestoneurlold)) {

Nit: our usual convention is of the form:

if (test1
    && test2
    && test3)
{
    ....
}

Note the position of '&&' as well as '{'.


>+            ThrowUserError('prod_must_define_defaultmilestone',
>+                           {product => $productold,
>+                            defaultmilestone => $defaultmilestone,
>+                            classification => $classification});

Nit: please align '=>' vertically.


>             while (MoreSQLData()) {
>                 my ($who, $id) = (FetchSQLData());
>                 push(@list, [$who, $id]);
>+                
>             }

Nit: This empty line in the 'while' loop is useless.


>+        $vars->{'confirmedbugs'} = \@updated_bugs;
>+        $vars->{'whoid'} = $whoid;

Nit: $vars->{'whoid'} isn't explicit enough. What about $vars->{'changer'} ?



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

>+  Edit <a 
>+  href="editproducts.cgi?
>+        [%- classification_url_part %]">other products
>+          [% classification_text %]</a>.

'classification_url_part' starts with an &amp;. So we get
editproducts.cgi?&classification=... :(



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

>   [% ELSIF error == "product_not_specified" %]

Nit: could we group together error messages related to products?


>+  [% ELSIF error == "prod_cant_delete_description" %]
>+    [% title = "Cannot delete product description" %]
>+    Cannot delete the description for product
>+    '[% product FILTER html %]'.

Nit: I much prefer using <em> </em> than quotes, especially when the field may
contain quotes itself. This comment applies to other places too.


>+  [% ELSIF error == "prod_must_define_defaultmilestone" %]

>+    create the milestone '[% defaultmilestone FILTER html%]'</a> before

Nit: add a space before " %] ".



>+++ template/en/default/admin/products/updated.html.tmpl

>+[%# INTERFACE:
>+  #
>+  # 'updated_XXX' variables are booleans, and are defined if the
>+  # 'XXX' field was updated during the edit just being handled.
>+  # For any fields that were updated, there will be an 'old_XXX'
>+  # string variable and a 'new_XXX' string variable.

Maybe an explanation of the form

updated_XXX : boolean; is true when the 'XXX' field has been updated.
old_XXX : ... string; old value of the field 'XXX'.
new_XXX : ... string; new value of the field 'XXX'.

would be better.


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

As said before, 'changer' sounds better.


>+  # name: string; same as product parameter, i.e. the product name, needed
>+  #       for product admin page footer compatability

As far as I can see, the 'product' parameter doesn't exist, so I would remove
this sentence.


>+  # checkvotes: boolean; were any vote related fields changed. I so, 

'I so'? or 'If so'? Also, I would reword it as "boolean; is true if vote
related fields have changed.". I don't like this pseudo-questions as a
definition.


>+[% IF updated_product %]
>+  <p>
>+  Updated product name from '[% old_product FILTER html %]' to 
>+  <a href="editproducts.cgi?action=edit&amp;product=
>+  [%- new_product FILTER url_quote %]
>+  [%- classification_url_part FILTER none %]">'[% new_product FILTER html %]'</a>.
>+[% END %]

Nit: no need to add quotes to [% new_product FILTER html %] as this one is
already linkified. I think it's already clear enough (and I prefer <em> </em>
;) ).


>+[% IF updated_description %]
>+  <p>
>+  <table>
>+    <tr>
>+      <td>Updated description to:</td>
>+      <td>[% new_description FILTER html %]</td>
>+    </tr>
>+  </table>
>+[% END %]

I don't like nested tables, especially for such minor things. Please remove it.
Also, why defining $vars->{'old_description'} in editproducts.cgi as you don't
use it here?


>+[%# It should be possible to administrate the target milestone and
>+    voting parameters, even if not enabled for general use, and so any
>+    changes should be displayed, without checking the
>+    relevant parameters. (This is a change from
>+    pre-templatisation.)
>+%]

This comment should not be here as $vars->{'updated_milestoneurl'} is
controlled by editproducts.cgi itself and you have a priori no idea what the
policy in this .cgi file is. If $vars->{'updated_milestoneurl'} is defined,
then editproducts.cgi allowed the user to edit this field, else not. Period.


>+[% IF updated_defaultmilestone %]
>+  <p>
>+  Updated default milestone from <a href="editmilestones.cgi?action=edit&amp;product=
>+  [% name FILTER url_quote %][% classification_url_part FILTER none %]&amp;milestone=
>+  [%- old_defaultmilestone FILTER url_quote %]">'
>+  [%- old_defaultmilestone FILTER html %]'</a> to
>+  <a href="editmilestones.cgi?action=edit&amp;product=
>+  [%- name FILTER url_quote %][% classification_url_part FILTER none %]&amp;milestone=
>+  [%- new_defaultmilestone FILTER url_quote %]">'[% new_defaultmilestone FILTER html %]'</a>.
>+  </p>
>+[% END %]

Nit: I'm not a big fan of all these URLs. This page is to inform the user about
changes made to the product, not to display a list of links to edit the product
further (there are links in the footer for that already).


>+      [%# This is INLUDED instead of PROCESSED to avoid variables getting
>+          overwritten, which happens otherwise %]

Nit: INCLUDED


Most of my comments are nits, but there are too many of them to grant review.
Fix what you seem reasonable and I will test your updated patch.

Comment 4

[GavinS]

Attachment: templatize product updated page, v3

(In reply to comment #3)

> >+	if (Param('usetargetmilestone') &&
> >+	    ($milestoneurl ne $milestoneurlold)) {
> 
> Nit: our usual convention is of the form:
> 
> if (test1
>     && test2
>     && test3)
> {
>     ....
> }
> 
> Note the position of '&&' as well as '{'.

I can see much of Bugzilla using && at the start of the line, but
there are very few opening curly braces on a line by themselves,
and the perl style guidelines we sort of follow don't agree, so
I'll happily change the &&'s, but I'm going to leave the '{' for
now., if that's OK

> >+  [% ELSIF error == "prod_cant_delete_description" %]
> >+	[% title = "Cannot delete product description" %]
> >+	Cannot delete the description for product
> >+	'[% product FILTER html %]'.
> 
> Nit: I much prefer using <em> </em> than quotes, especially when the field
may
> contain quotes itself. This comment applies to other places too.

I've always used single quotes in the other templatisations I've
done, so this is consistent with them, at least. What we want is
to use CSS to identify all such new values, and style them that
way. But that is another bug...


> >+[% IF updated_description %]
> >+  <p>
> >+  <table>
> >+	<tr>
> >+	  <td>Updated description to:</td>
> >+	  <td>[% new_description FILTER html %]</td>
> >+	</tr>
> >+  </table>
> >+[% END %]
> 
> I don't like nested tables, especially for such minor things. Please remove
it.
> Also, why defining $vars->{'old_description'} in editproducts.cgi as you
don't
> use it here?

It wasn't really a nested table, but I've changed it anyway. The
old_description is there in case a custom or future template
wants to use it.

Comment 5

[Frédéric Buclin]

Comment on attachment 195182 [details] [diff] [review]
templatize product updated page, v3

>Index: editproducts.cgi

>+    my @toomanyvotes_list = ();
>+
>     if ($checkvotes) {

Nit: @toomanyvotes_list doesn't need to be defined outside the "if
($checkvotes)" block.



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

>+  [% ELSIF error == "prod_must_define_defaultmilestone" %]

>+    You must <a href="editmilestones.cgi?action=add&amp;product=
>+    [%- product FILTER url_quote %]
>+    [%- classification_url_part FILTER none %]&amp;milestone=
>+    [%- defaultmilestone FILTER url_quote %]">

Nit: &milestone=foo is useless as editmilestones.cgi doesn't take this
parameter into account.


Both are nits and both can be fixed on checkin. r=LpSolit

Comment 6

[Frédéric Buclin]

Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.96; previous revision: 1.95
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.4; previous revision: 1.3
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/updated.html.tmpl,v
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
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.126; previous revision: 1.125
done