Editgroups.cgi - template for add a group page

RESOLVED FIXED in Bugzilla 2.18

Status

()

RESOLVED FIXED
15 years ago
6 years ago

People

(Reporter: goobix, Assigned: goobix)

Tracking

unspecified
Bugzilla 2.18
Bug Flags:
approval +

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

15 years ago
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.
(Assignee)

Comment 1

15 years ago
Created attachment 124270 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #124270 - Flags: review?(justdave)
(Assignee)

Comment 2

15 years ago
Created attachment 124271 [details] [diff] [review]
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.
(Assignee)

Updated

15 years ago
Attachment #124271 - Flags: review?(justdave)
(Assignee)

Comment 3

15 years ago
Created 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)

XHTML improvements, support for template for "add a group" page. (same as the
previous one, but fixed contributors in header)
Attachment #124270 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #124274 - Flags: review?(bugmail)
(Assignee)

Comment 4

15 years ago
Created attachment 124275 [details] [diff] [review]
Template for add a group page, this time with contributors fixed correctly.

To be placed in /template/en/default/admin/addgroup.html.tmpl
Attachment #124271 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #124275 - Flags: review?(justdave)
(Assignee)

Updated

15 years ago
Attachment #124270 - Flags: review?(justdave) → review-
(Assignee)

Updated

15 years ago
Attachment #124271 - Flags: review?(justdave) → review-
(Assignee)

Updated

15 years ago
Attachment #124274 - Flags: review?(bugmail) → review?(justdave)
(Assignee)

Updated

15 years ago
Attachment #124274 - Flags: review?(justdave) → review?(myk)
(Assignee)

Updated

15 years ago
Attachment #124275 - Flags: review?(justdave) → review?(myk)
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=.
Attachment #124274 - Flags: review?(myk) → review-
Comment on attachment 124275 [details] [diff] [review]
Template for add a group page, this time with contributors fixed correctly.

Removing unnecessary review request.
Attachment #124275 - Flags: review?(myk)
(Assignee)

Comment 7

15 years ago
<-- me
Assignee: justdave → jocuri
(Assignee)

Updated

15 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 8

15 years ago
Created attachment 129472 [details] [diff] [review]
New patch per myk's suggestions.
Attachment #124274 - Attachment is obsolete: true
Attachment #124275 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #129472 - Flags: review?(myk)
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.
Attachment #129472 - Flags: review?(myk) → review-
(Assignee)

Comment 10

15 years ago
Created attachment 135127 [details] [diff] [review]
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).
Attachment #129472 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #135127 - Flags: review?(myk)
(Assignee)

Comment 11

15 years ago
(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)
(Assignee)

Comment 12

15 years ago
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 :)
Attachment #135127 - Flags: review?(kiko)
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.
Attachment #135127 - Flags: review?(myk) → review+
a=myk
Flags: approval+
(Assignee)

Updated

15 years ago
Attachment #135127 - Flags: review?(kiko)
(Assignee)

Comment 15

15 years ago
Created attachment 135132 [details] [diff] [review]
Patch with nice sentence-break.
(Assignee)

Updated

15 years ago
Attachment #135132 - Attachment description: Checked in patch, for ref. purposes. → Patch with nice sentence-break.
(Assignee)

Comment 16

15 years ago
Created attachment 135136 [details] [diff] [review]
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.
Attachment #135127 - Attachment is obsolete: true
Attachment #135132 - Attachment is obsolete: true
(Assignee)

Updated

15 years ago
Attachment #135136 - Flags: review?(myk)
(Assignee)

Updated

15 years ago
Attachment #135136 - Flags: review?(kiko)
Comment on attachment 135136 [details] [diff] [review]
Patch with [% terms.bugs %] customization

Works. r=myk
Attachment #135136 - Flags: review?(myk) → review+
(Assignee)

Updated

15 years ago
Attachment #135136 - Flags: review?(kiko)
(Assignee)

Comment 18

15 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
(Assignee)

Updated

15 years ago
Target Milestone: --- → Bugzilla 2.18
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.