307524 - Templatise the updategroupcontrols bit of editproducts (and tidy up templatisation)

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[GavinS]

1) Do the updategroupcontrols templatisation

2) remove any temporary bits we added during templatisation

3) add a ThrowUserError() for an unknown action

Comment 1

[Frédéric Buclin]

Bug 306757 has to land first as it still uses some old routines. gbel is working
on it.

Comment 2

[GavinS]

gbel, if you wanted to do this, then please do, as I haven't really started on it yet, and won't get around to 
it until late this week-end/next week... 

Comment 3

[André Batosti]

Attachment: batosti_v1

templatization of updategroupcontrols action

removed unused functions of editproducts

ThrowUserError for invalid action

Comment 4

[André Batosti]

please assign the bug to me :)

Comment 5

[GavinS]

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

André, Thank-you for doing this. Here are my comments, lpsolit will have some
as well...

-----------
-sub CheckProduct

NIT: We should remove these old ones (as you are doing), and rename
the temporary ones we had from CheckProductNew() back to
CheckProduct(). However, this is a NIT because we are soon going to to
remove all these Check* and Test* routines anyway, so I'm OK with
leaving them. If we don;t rename them, we should update the comment
though, to say the routines will be removed soon when we use
Product.pm etc.  lpsolit may disagree...


-	 print "dropped $count bugs<p>\n";
+	 my %group = (name => GroupIdToName($groupid),
+		      count => $count);

NIT: 'dropped_bug_count' or 'bug_count' maybe better names

+
+	 push @{$updated_groups{'removed_na'}}, \%group;

I think the BZ standard is to include the round brackets for calls like push()


-    print "Group control updates done<P>\n";
+    $vars->{'updated_groups'} = \%updated_groups;
+
+    $vars->{'product'} = $product;

We also need to pass in the classification variable here.

+[%# INTERFACE:
+  # updated_groups: hash of the groups updated to Mandatory or NA

Can we include the keys, and also the content (including keys) of the
contained hashes?

+  #
+  # product: string; name of the product.
+  #%]
+
+[% PROCESS global/header.html.tmpl
+  title = "Update group access controls for $product"
+%]

product should be filtered (runtests.pl does not complain, but I think
it should (there is a bug for that somewhere))

NIT: And I like the product to be highlighted somehow using single
quotes or double quotes, but others are less keen :)

+
+[% IF updated_groups.removed_na %]
+  [% FOREACH g = updated_groups.removed_na %]
+    Removing [% terms.bugs %] from NA group [% g.name FILTER html %]<p>
+    dropped [% g.count FILTER html %]&nbsp;[% terms.bugs %]<p>
+  [% END %]
+[% END %]

If I understand this correctly, we could change this to be:

"Removed 10 bugs no longer associated with group 'groupname'." or similar

Also, have a <p> at the start of the line (or use &rarr;) or a <ul>
maybe? And no need for the &nbsp, is there?;

+
+Group control updates done<p>

<p> at start of line and fullstop at end.


NIT: Can we add the standard admin CSS link. We want it on all pages
eventually...

Comment 6

[Frédéric Buclin]

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

>Index: editproducts.cgi

> # For the transition period, as this file is templatised bit by bit,
> # we need this routine, which does things properly, and will
> # eventually be the only version. (The older versions assume a

All these comments are now useless as you have removed the obsolete ones. Also,
fix comments which describe what routines do (CheckProduct is now
CheckProductNew). Either that or rename *New to *, as suggested by GavinS in
his review.


>-sub PutTrailer

Now that PutTrailer is going away, all these obsolete variables such as
$classhtml* and $localtrailer have to be removed too. Scan the whole file to
remove all occurences of these variables.


>+    my %updated_groups;
>     foreach my $groupid (@now_na) {

Better is to use an array here. One for now unrelated groups, and another one
for mandatory groups. This will simplify the code a bit.


>+        $updated_groups{'removed_na'} = [] 
>+            unless $updated_groups{'removed_na'};

Useless as you cannot have twice the same group in @now_na.


>+        my %group = (name => GroupIdToName($groupid),
>+                     count => $count);

As GavinS noted, use bug_count instead of count.


>+        push @{$updated_groups{'removed_na'}}, \%group;

Also noted by GavinS, use push(@array, $new_element) (with parens). And using
an array will simplify this a bit.


>+        $updated_groups{'added_mandatory'} = []
>+            unless $updated_groups{'added_mandatory'};

As said above, use a new array for mandatory groups.


>+        my %group = (name => GroupIdToName($groupid),
>+                     count => $count);
>+
>+        push @{$updated_groups{'added_mandatory'}}, \%group;

2x same comments as above.



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

>+[% PROCESS global/header.html.tmpl
>+  title = "Update group access controls for $product"
>+%]

I agree with GavinS, move the title in its own block and FILTER $product. Then
use 'title = title'.


>+[% IF updated_groups.removed_na %]
>+  [% FOREACH g = updated_groups.removed_na %]
>+    Removing [% terms.bugs %] from NA group [% g.name FILTER html %]<p>

"Removing from NA group" definitely means nothing! I prefer something like:
"Removing %bugs% from group %group% which no longer apply to this product".


>+    dropped [% g.count FILTER html %]&nbsp;[% terms.bugs %]<p>

Also, "%count% %bugs% removed" sounds better.


>+[% IF updated_groups.added_mandatory %]
>+  [% FOREACH g = updated_groups.added_mandatory %]
>+    Adding [% terms.bugs %] to Mandatory group [% g.name FILTER html %]<p>

I prefer something like: "Adding %bugs% to group %group% which is mandatory for
this product".


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

classification is missing.

Comment 7

[André Batosti]

Attachment: batosti_v1_fix

all requesteds fix made :)

Comment 8

[Frédéric Buclin]

Comment on attachment 196081 [details] [diff] [review]
batosti_v1_fix

>Index: editproducts.cgi

>-if (Param('useclassification')) {
>-    if ($classification) {
>-    }
>-    elsif (!$product) {

>+if (Param('useclassification') && !$product) {

Both tests are not equivalent and your test would generate a loop. The test
must be:

if (Param('useclassification')
    && !$classification
    && !$product)



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

>+[%# INTERFACE:
>+  # updated_groups: hash of the groups updated to Mandatory or NA
>+  #
>+  # product: string; name of the product.
>+  #%]

Please update the INTERFACE. Now you have @removed_na, @added_mandatory and
$classification instead of %updated_groups.


>+[% IF removed_na.size > 0 %]
>+  [% FOREACH g = removed_na %]
>+    Removing [% terms.bugs %] from group [% g.name FILTER html %] which
>+    no longer apply to this product<p>
>+    [% g.bug_count FILTER html %]&nbsp;[% terms.bugs %] removed<p>
>+  [% END %]
>+[% END %]

Nit#1: Add a <p> before the IF block
Nit#2: '[% g.name FILTER html %]' <-- quotes around the group name (or
<em></em> to make it italic)
Nit#3: "... no longer applies ..." <-- 'applies' instead of 'apply'


>+[% IF added_mandatory.size > 0 %]
>+  [% FOREACH g = added_mandatory %]
>+    Adding [% terms.bugs %] to group [% g.name FILTER html %] which is 
>+    mandatory for this product<p>
>+    [% g.bug_count FILTER html %]&nbsp;[% terms.bugs %] added<p>
>+  [% END %]
>+[% END %]

Nit: '[% g.name FILTER html %]' <-- quotes around the group name (or <em></em>
to make it italic)


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

footer.html.tmpl expects the product name to be in the 'name' variable. So you
have to add 'name = product'.

Comment 9

[Gabriel Sales de Oliveira]

Attachment: batosti_v2

that's it....

Comment 10

[Frédéric Buclin]

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

>Index: editproducts.cgi

>+if (Param('useclassification') 
>+    && !$classification
>+    && !$product) {

Nit: the bracket should be on its own line, aligned with 'if'.



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

>+[%# INTERFACE:
>+  #      removed_na: hash; with groups that were removed from the product.  
>+  # added_mandatory: hash; with groups updated to 'Mandatory' group control.  

Nit: array of hashes, not hash.


>+    [% g.bug_count FILTER html %]&nbsp;[% terms.bugs %] removed<p>
>+    [% g.bug_count FILTER html %]&nbsp;[% terms.bugs %] added<p>

Nit: instead of &nbsp;, you should write [%+ terms.bugs %].


These are nits which I can fix on checkin. The patch works fine. Now
editproducts.cgi is completely templatized. Thanks for the work! :)

Comment 11

[Frédéric Buclin]

Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.100; previous revision: 1.99
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.2; previous revision: 1.1
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/products/groupcontrol/updated.html.tmpl,v
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
initial revision: 1.1
done