207211 - Editgroups.cgi - template for add a group page

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Vlad Dascalu]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3.1) Gecko/20030428
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.3.1) Gecko/20030428

Offers partial template inside editgroups.cgi (to be more specific, only for add
a group page).

Patches comming up.

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Vlad Dascalu]

Attachment: Added a template for "Add a group" page, and improved overall XHTML compatibility.

Added a template for "Add a group" page, and improved overall XHTML
compatibility.

Comment 2

[Vlad Dascalu]

Attachment: Template for "Add a group" page. To be placed in /template/en/default/admin/addgroup.html.tmpl

Template for "Add a group" page. To be placed in
/template/en/default/admin/addgroup.html.tmpl . Works together with the patch
already submitted.

Comment 3

[Vlad Dascalu]

Attachment: XHTML improvements, support for template for "add a group" page. (same as the previous one, but fixed contributors in header)

HTML improvements, support for template for "add a group" page. (same as the
previous one, but fixed contributors in header)

Comment 4

[Vlad Dascalu]

Attachment: Template for add a group page, this time with contributors fixed correctly.

To be placed in /template/en/default/admin/addgroup.html.tmpl

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 124274 [details] [diff] [review]
XHTML improvements, support for template for "add a group" page. (same as the previous one, but fixed contributors in header)

>+    $vars->{'checked'} = "";
>+    if (Param("makeproductgroups")) {
>+	    $vars->{'checked'} = " checked";
>+    }

This variable needs to be named descriptively.	"checked" doesn't say anything
about what is being checked or why.  Even better, access the parameter directly
from within the template with [% IF Param("makeproductgroups") %] checked[% END
%].

> 
>-    PutTrailer("<a href=editgroups.cgi>Back to the group list</a>");
>+    $template->process("admin/addgroup.html.tmpl", $vars)
>+      || ThrowTemplateError($template->error());

Use a hyphen to distinguish multiple words in template names, i.e.:
add-group.html.tmpl.

I'm not convinced all these changes are worth it given that we'll just
templatize that stuff anyway, but here it is, you've done the work, and it's
not rotted.  Fix those two issues and you'll get my r=.

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 124275 [details] [diff] [review]
Template for add a group page, this time with contributors fixed correctly.

Removing unnecessary review request.

Comment 7

[Vlad Dascalu]

<-- me

Comment 8

[Vlad Dascalu]

Attachment: New patch per myk's suggestions.

Comment 9

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 129472 [details] [diff] [review]
New patch per myk's suggestions.

Sorry about the delay in reviewing.

>Index: editgroups.cgi

>-    PutTrailer("<a href=editgroups.cgi>Back to the group list</a>");

There's a trivial conflict on this line that needs to be fixed; the newer
version of editgroups.cgi only has "group list" linkified.


>+++ template/en/default/admin/add-group.html.tmpl...

>+  <input type="checkbox" name="insertnew" value="1"[% IF Param("makeproductgroups") %] checked[% END %]>

Nit: break into two lines (probably before "[% IF...").


>+<p><a href=\"./\">Back to the Main Bugs Page</a>

The quotes here shouldn't be escaped anymore.

Otherwise it looks good.

Comment 10

[Vlad Dascalu]

Attachment: New version per myk's comments

I applied your suggestions and I did some line trimming from 80+ chars/line to
around 70 chars / line (in only 2 paragraphs).

Comment 11

[Vlad Dascalu]

(I also did some capitalization fixes that were recommended by kiko in another
patch in order to have a unified look in the edit*.cgis regarding the footer links)

Comment 12

[Vlad Dascalu]

Comment on attachment 135127 [details] [diff] [review]
New version per myk's comments

kiko: considering myk is 3 months behind and stuff, and he already reviewed
this, could you give the new version a spin and make sure that it fixes only
what I just said, and that it addresses myk's concerns?

Maybe we manage to get this checked in this year after all :)

Comment 13

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 135127 [details] [diff] [review]
New version per myk's comments

>+++ template/en/default/admin/add-group.html.tmpl...

>+<p><b>Name</b> is what is used with the UserInGroup() function in any
>+customized cgi files you write that use a given group.  It can also be
>+used by people submitting bugs by email to limit a bug to a certain set
>+of groups.  It may not contain any spaces.</p>

Nit: keeping phrases together makes line-wrapped sentences easier to read,
i.e.:

<p><b>Name</b> is what is used with the UserInGroup() function in any
customized cgi files you write that use a given group.	It can also be used
by people submitting bugs by email to limit a bug to a certain set of groups.
It may not contain any spaces.</p>

Otherwise fine, r=myk.	Second review optional.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

a=myk

Comment 15

[Vlad Dascalu]

Attachment: Patch with nice sentence-break.

Comment 16

[Vlad Dascalu]

Attachment: Patch with [% terms.bugs %] customization

By running the runtests.pl suite prior to checkin I realised we would probably
put tinderbox on fire due to recent customization of "bugs" & the like. This
new version should fix this.

Comment 17

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 135136 [details] [diff] [review]
Patch with [% terms.bugs %] customization

Works. r=myk

Comment 18

[Vlad Dascalu]

Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.29; previous revision: 1.28
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/add-group.html.tmpl,v
done
Checking in template/en/default/admin/add-group.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/add-group.html.tmpl,v
 <--  add-group.html.tmpl
initial revision: 1.1
done