templatize editgroups.cgi

RESOLVED FIXED in Bugzilla 2.20

Status

()

Bugzilla
Administration
--
enhancement
RESOLVED FIXED
15 years ago
5 years ago

People

(Reporter: justdave, Assigned: Vlad Dascalu)

Tracking

2.17.3
Bugzilla 2.20
Dependency tree / graph
Bug Flags:
approval +

Details

Attachments

(1 attachment, 15 obsolete attachments)

RC4
64.32 KB, patch
Joel Peshkin
: review+
Details | Diff | Splinter Review
templatize editgroups.cgi
Severity: normal → enhancement
Target Milestone: --- → Bugzilla 2.18

Updated

15 years ago
Blocks: 181589

Comment 1

15 years ago
Just noting that Bug#207211 does part of the editgroups templatisation.
Enhancements which don't currently have patches on them which are targetted at
2.18 are being retargetted to 2.20 because we're about to freeze for 2.18. 
Consideration will be taken for moving items back to 2.18 on a case-by-case
basis (but is unlikely for enhancements)
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20

Updated

14 years ago
No longer blocks: 181589
(Assignee)

Updated

14 years ago
Assignee: justdave → vlad
(Assignee)

Updated

14 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

14 years ago
We already have some features in progress for 2.18. Is this too late to make 
the 2.18 train? I guess so but one can only dream... hrmm... I mean ask... :-)

I'd like this one for 2.18 because the HTML inside editgroups.cgi is really 
really ugly and here and there it could be quite invalid per W3C 
specifications. Maintaining and supporting such code in the long-lived BZ 2.18 
branch is going to be a pain.

In addition, a lot of localization efforts will be started based on 2.18 and 
I'd like to offer to them the templates for editgroups.cgi as well.

In addition, having the templates ready makes words like bugs customizable, and 
the whole review of code implied by FILTER directives and stuff is done 
automatically by the testing suite.

In addition, if this makes it into 2.18, admins will have more freedom to 
customize templates according to their personal style.
Target Milestone: Bugzilla 2.20 → Bugzilla 2.18
(Assignee)

Comment 4

14 years ago
By the way, I expect to have this ready in a couple of days. The review process 
could add to it and make it up to one week, because I expect the usual "oh, 
since you're at it, let's modify this and that, shall we?" :-)
(Assignee)

Comment 5

14 years ago
Created attachment 145932 [details] [diff] [review]
Version 0.1

This is far from ready, but it applies cleanly and it keeps the functionability
of the script. There might be some taint issues that I haven't fully managed to
tickle yet, but it's a nice preview for those interested. Not ready for review.
(Assignee)

Comment 6

14 years ago
CCing the template guru guy who will probably have to review the final version 
anyway :)

Comment 7

14 years ago
This should land before bug 240325
Blocks: 240325

Comment 8

14 years ago
Comment on attachment 145932 [details] [diff] [review]
Version 0.1

I know its not ready, but I'm testing a theory
Attachment #145932 - Flags: review-

Updated

14 years ago
Attachment #145932 - Flags: review-
(Assignee)

Comment 9

14 years ago
Created attachment 146005 [details] [diff] [review]
0.1 (*nix version)

This has the endlines converted for the *nix platform.
Attachment #145932 - Attachment is obsolete: true
(Assignee)

Comment 10

14 years ago
Created attachment 146006 [details] [diff] [review]
0.1 (*nix, with the new files added)
Attachment #146005 - Attachment is obsolete: true
(Assignee)

Comment 11

14 years ago
Created attachment 146007 [details] [diff] [review]
Version 0.11

Apparently some hunks were lost in the Unix conversion, this should fix that.
Attachment #146006 - Attachment is obsolete: true
(Assignee)

Comment 12

14 years ago
Created attachment 146008 [details] [diff] [review]
Version 0.12

This should complete the *nix transition.
Attachment #146007 - Attachment is obsolete: true
(Assignee)

Comment 13

14 years ago
Created attachment 146048 [details] [diff] [review]
Version 0.2

This one adds user-error and code-error templatization.
Attachment #146008 - Attachment is obsolete: true
(Assignee)

Comment 14

14 years ago
Created attachment 146049 [details] [diff] [review]
Version 0.21

Forgot to add the created.html.tmpl file.
Attachment #146048 - Attachment is obsolete: true
(Assignee)

Comment 15

14 years ago
Created attachment 146050 [details] [diff] [review]
Version 0.22

\" becomes "
(when moving HTML code out of .cgi in .html.tmpl files)
Attachment #146049 - Attachment is obsolete: true
(Assignee)

Comment 16

14 years ago
Created attachment 146051 [details] [diff] [review]
Version 0.22

Oops, wrong file :)
Attachment #146050 - Attachment is obsolete: true
(Assignee)

Comment 17

14 years ago
Created attachment 146054 [details] [diff] [review]
Version 0.23

The one I got after cleaning my tree.
Attachment #146051 - Attachment is obsolete: true

Updated

14 years ago
No longer blocks: 240325
(Assignee)

Comment 18

14 years ago
Created attachment 146129 [details] [diff] [review]
Version 0.3
Attachment #146054 - Attachment is obsolete: true
(Assignee)

Comment 19

14 years ago
Created attachment 146130 [details] [diff] [review]
Version 0.31

Seems I ordered some lines in a weird way.
Attachment #146129 - Attachment is obsolete: true
Vlad: may I recommend Patch Maker (http://www.gerv.net/software/patch-maker/)?
It helps prevent most of the errors in creating patches you seem to have made on
this bug ;-)

Gerv
(Assignee)

Comment 21

14 years ago
Created attachment 146170 [details] [diff] [review]
Version 1 RC1
(Assignee)

Updated

14 years ago
Attachment #146130 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #146170 - Flags: review?(gerv)
(Assignee)

Updated

14 years ago
Attachment #146170 - Flags: review?(bugreport)
(Assignee)

Updated

14 years ago
Attachment #146170 - Flags: review?(kiko)
(Assignee)

Comment 22

14 years ago
Gerv: thanks for the tip! Like any other tool it has a learning curve but I hope
I can find the time to learn it and make myself more productive.

Comment 23

14 years ago
Comment on attachment 146170 [details] [diff] [review]
Version 1 RC1



> 
>     SendSQL("SELECT id, name, description, userregexp, isactive, isbuggroup
>              FROM groups WHERE id=" . SqlQuote($gid));
>     my ($group_id, $name, $description, $rexp, $isactive, $isbuggroup) 
>         = FetchSQLData();

bleah! (yeah, I know you didn't add this... let's do a cleanup after this lands
and migrate away from SendSQL, use placeholders, and not sqlquote numbers)



>+[%# INTERFACE:
>+  # action: integer. Can be 1, 2 or 3, depending on the action
>+  #         performed:
>+  #         1 - remove_explicit_members
>+  #         2 - remove_explicit_members_regexp
>+  #         3 - no conversion, just save the changes
>+  # changes: boolean int. Is 1 if changes occured.
>+  # gid: integer. The ID of the group.
>+  # name: the name of the product where removal is performed.
>+  # regexp: the regexp according to which the update is performed.
>+  #%]

I'll defer to Gerv on this, but I think I led you astray on IRC.  If we are
passing something like "action", we should use a mnemonic rather than an
integer.

You need to make sure you can pass runtests.sh.  It is complaining about a
number of unfiltered outputs in the templates.	The group names certainly need
FILTER html.  The groups ids just need that because runtests doesn't know that
we already are certain it is a number. Just filter it anyway.  
Fix the runtests errors, Check with Gerv on the interface, fix the trivial
bitrot in the code error template, and this will be ready to go.
Attachment #146170 - Flags: review?(kiko)
Attachment #146170 - Flags: review?(gerv)
Attachment #146170 - Flags: review?(bugreport)
Attachment #146170 - Flags: review-
(Assignee)

Comment 24

14 years ago
Created attachment 146201 [details] [diff] [review]
RC2
Attachment #146170 - Attachment is obsolete: true
(Assignee)

Comment 25

14 years ago
Comment on attachment 146201 [details] [diff] [review]
RC2

This passes the testing suite. Gerv, can you take a look?

Also:

1) Do you want group id FILTER html or do you want it unfiltered (it's a
number) and added to filterexception.pl?

2) in admin/groups/change.html.tmpl, it's ok to number-code the action value
(with comments in the INTERFACE section, like it's done now) in order to
prevent its accidental translation by newbie translators without Perl/HTML
knowledge, or should we have text-strings?
Attachment #146201 - Flags: review?(gerv)
(Assignee)

Comment 26

14 years ago
> >     SendSQL("SELECT id, name, description, userregexp, isactive, isbuggroup
> >              FROM groups WHERE id=" . SqlQuote($gid));
> bleah! (yeah, I know you didn't add this... let's do a cleanup after this lands
> and migrate away from SendSQL, use placeholders, and not sqlquote numbers)

Now that I think about it, we detaint_natural the $gid, so we can throw away the
SqlQuote on it. I did that in the "RC2" version.
Comment on attachment 146201 [details] [diff] [review]
RC2

>     SendSQL("SELECT id,name,description,userregexp,isactive,isbuggroup " .
>             "FROM groups " .
>             "ORDER BY isbuggroup, name");

I echo the "bletch" here. This should be fixed, either now or in a subsequent
patch. Doing it now means that all the testing you are doing tests it all at
once ;-)

Can the SQL code for reading groups from the DB be centralised a bit more? It's
hard to tell when reviewing a patch like this.

> if ($action eq 'changeform') {

The usual way of doing the $action stuff is to default it when you get it out
of the form variable, and then have a big dispatch statement, rather than have
an "unless ($action) ..." block. This is cleaner, because every action has a
name, and $action is always defined.

>+    my $name = trim($cgi->param('name') || '');
>+    my $desc = trim($cgi->param('desc') || '');
>+    my $regexp = trim($cgi->param('regexp') || '');
>     # convert an undefined value in the inactive field to zero
>     # (this occurs when the inactive checkbox is not checked
>     # and the browser does not send the field to the server)
>-    my $isactive = $::FORM{isactive} || 0;
>+    my $isactive = $cgi->param('isactive') || 0;
>+
>+    ThrowUserError("empty_group_name") unless $name;
>+    ThrowUserError("empty_group_description") unless $desc;

If you are going to do this, you don't need to default $name and $desc to ''.

>+    my $gid = trim($cgi->param('group') || '');
>+    ThrowUserError("group_not_specified") unless ($gid);

Same comment here.

>+    ThrowUserError("invalid_group_ID") unless FetchOneColumn();

Nit: we tend to use lowercase, even for "id".

>     if (!FetchOneColumn()) {} else {

Huh? :-)

>+    my $hasproduct = 0;
>     SendSQL("SELECT name FROM products WHERE name=" . SqlQuote($name));
>     if (MoreSQLData()) {
>+        $hasproduct = 1;
>+    }

The usual idiom for this (and there's several here in a row) is:

$hasproduct = MoreSQLData() ? 1 : 0;


>+    if (!$cantdelete) {
>+        SendSQL("DELETE FROM user_group_map WHERE group_id = $gid");
>+        SendSQL("DELETE FROM group_group_map WHERE grantor_id = $gid");
>+        SendSQL("DELETE FROM bug_group_map WHERE group_id = $gid");
>+        SendSQL("DELETE FROM group_control_map WHERE group_id = $gid");
>+        SendSQL("DELETE FROM groups WHERE id = $gid");

Is it possible to do DELETE FROM user_group_map, group_group_map, .. WHERE
group_id = $gid?

>+    my $action;
>+
>+    if ($cgi->param('remove_explicit_members')) {
>+        $action = 1;
>+    } elsif ($cgi->param('remove_explicit_members_regexp')) {
>+        $action = 2;
>+    } else {
>+        $action = 3;
>     }

I'd definitely go for names here. No-one will localise them by mistake. These
are basically magic numbers, which is bad.

I'm out of time for reviewing the templates (it's my bedtime ;-), so if someone
else can do that, that would be great. They look fine and, given that this is
post-2.18 work (right?) they'll get a good long testing period.

Gerv
Attachment #146201 - Flags: review?(gerv)
Attachment #146201 - Flags: review?
Attachment #146201 - Flags: review+

Comment 28

14 years ago
I have a bunch of changes to the SQL that will pick up the cleanup for this.
I'll re-review and retest this when I get a chance.

This is not a high risk change.  The processing of the changes IS done by the
cgi, not the templates.  I do not think it needs to be barred from 2.18

Comment 29

14 years ago
Comment on attachment 146201 [details] [diff] [review]
RC2


Editing the group checkboxes throws a taint error.  
     my $v = substr($b, 7);
leaves $v tainted.  It's a terrible way to do that anyway.  Try changing it to 
$b =~ /^oldgrp-(\d+)$/;
$v = $1;
Attachment #146201 - Flags: review? → review-
If you get this in, cool, but I'm not holding 2.18 for it.
Target Milestone: Bugzilla 2.18 → Bugzilla 2.20

Updated

14 years ago
No longer blocks: 240325
(Assignee)

Comment 31

14 years ago
Created attachment 152851 [details] [diff] [review]
RC3

Since we are now branched, the CVS tip hosts the 2.19 devel cycle and this is
ready to go.

I've updated the patch to eliminate the bitrotten, and I've eliminated the
taint error reported in comment #29 using the approach suggested by joel.
Attachment #146201 - Attachment is obsolete: true
(Assignee)

Comment 32

14 years ago
Created attachment 152852 [details] [diff] [review]
RC4

Replaced a string comparation done by using == with the proper "eq" operator
(discovered while testing).
Attachment #152851 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #152852 - Flags: review?(bugreport)

Updated

14 years ago
Blocks: 250703

Comment 33

14 years ago
Comment on attachment 152852 [details] [diff] [review]
RC4

fix the validation problem at...
101: <p>
102: Users become members of this group in one of three ways:
103: <ul>
104: <li> by being explicity included when the user is edited.
105: <li> by matching the user regexp above.
106: <li> by being a member of one of the groups included in this group
107: by checking the boxes below.
108: </ul>
109: </p>
110: 
(by moving the /p so it doesn't span the list item)
and r=joel
Attachment #152852 - Flags: review?(bugreport) → review+
(Assignee)

Comment 34

14 years ago
Joel said I can fix that on checkin; requesting approval.
Flags: approval?
Flags: approval? → approval+

Comment 35

14 years ago
All admin-page templatization bugs: Note that just-fixed bug 244265 implemented
a basic generic structure for writing out those typical
product/milestone/whatever selection tables that exist on most of the admin
pages. If you templatize one of those, please use the generic version. If it
doesn't have all the features you need, let's discuss expanding it with new
features.
(Assignee)

Comment 36

14 years ago
Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.39; previous revision: 1.38
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/ch
ange.html.tmpl,v
done
Checking in template/en/default/admin/groups/change.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/change.html.
tmpl,v  <--  change.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/admin/groups/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/create.html.
tmpl,v  <--  create.html.tmpl
new revision: 1.4; previous revision: 1.3
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/cr
eated.html.tmpl,v
done
Checking in template/en/default/admin/groups/created.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/created.html
.tmpl,v  <--  created.html.tmpl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/de
lete.html.tmpl,v
done
Checking in template/en/default/admin/groups/delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/delete.html.
tmpl,v  <--  delete.html.tmpl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/de
leted.html.tmpl,v
done
Checking in template/en/default/admin/groups/deleted.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/deleted.html
.tmpl,v  <--  deleted.html.tmpl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/ed
it.html.tmpl,v
done
Checking in template/en/default/admin/groups/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/edit.html.tm
pl,v  <--  edit.html.tmpl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/li
st.html.tmpl,v
done
Checking in template/en/default/admin/groups/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/list.html.tm
pl,v  <--  list.html.tmpl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/re
move.html.tmpl,v
done
Checking in template/en/default/admin/groups/remove.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/groups/remove.html.
tmpl,v  <--  remove.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tm
pl,v  <--  code-error.html.tmpl
new revision: 1.40; previous revision: 1.39
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tm
pl,v  <--  user-error.html.tmpl
new revision: 1.63; previous revision: 1.62
done
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.