190222 - templatize editgroups.cgi

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Dave Miller [:justdave]]

templatize editgroups.cgi

Comment 1

[GavinS]

Just noting that Bug#207211 does part of the editgroups templatisation.

Comment 2

[Dave Miller [:justdave]]

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)

Comment 3

[Vlad Dascalu]

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.

Comment 4

[Vlad Dascalu]

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?" :-)

Comment 5

[Vlad Dascalu]

Attachment: 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.

Comment 6

[Vlad Dascalu]

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

Comment 7

[Joel Peshkin]

This should land before bug 240325

Comment 8

[Joel Peshkin]

Comment on attachment 145932 [details] [diff] [review]
Version 0.1

I know its not ready, but I'm testing a theory

Comment 9

[Vlad Dascalu]

Attachment: 0.1 (*nix version)

This has the endlines converted for the *nix platform.

Comment 10

[Vlad Dascalu]

Attachment: 0.1 (*nix, with the new files added)

Comment 11

[Vlad Dascalu]

Attachment: Version 0.11

Apparently some hunks were lost in the Unix conversion, this should fix that.

Comment 12

[Vlad Dascalu]

Attachment: Version 0.12

This should complete the *nix transition.

Comment 13

[Vlad Dascalu]

Attachment: Version 0.2

This one adds user-error and code-error templatization.

Comment 14

[Vlad Dascalu]

Attachment: Version 0.21

Forgot to add the created.html.tmpl file.

Comment 15

[Vlad Dascalu]

Attachment: Version 0.22

\" becomes "
(when moving HTML code out of .cgi in .html.tmpl files)

Comment 16

[Vlad Dascalu]

Attachment: Version 0.22

Oops, wrong file :)

Comment 17

[Vlad Dascalu]

Attachment: Version 0.23

The one I got after cleaning my tree.

Comment 18

[Vlad Dascalu]

Attachment: Version 0.3

Comment 19

[Vlad Dascalu]

Attachment: Version 0.31

Seems I ordered some lines in a weird way.

Comment 20

[Gervase Markham [:gerv]]

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

Comment 21

[Vlad Dascalu]

Attachment: Version 1 RC1

Comment 22

[Vlad Dascalu]

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

[Joel Peshkin]

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.

Comment 24

[Vlad Dascalu]

Attachment: RC2

Comment 25

[Vlad Dascalu]

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?

Comment 26

[Vlad Dascalu]

> >     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 27

[Gervase Markham [:gerv]]

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

Comment 28

[Joel Peshkin]

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

[Joel Peshkin]

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;

Comment 30

[Dave Miller [:justdave]]

If you get this in, cool, but I'm not holding 2.18 for it.

Comment 31

[Vlad Dascalu]

Attachment: 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.

Comment 32

[Vlad Dascalu]

Attachment: RC4

Replaced a string comparation done by using == with the proper "eq" operator
(discovered while testing).

Comment 33

[Joel Peshkin]

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

Comment 34

[Vlad Dascalu]

Joel said I can fix that on checkin; requesting approval.

Comment 35

[Jouni Heikniemi]

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.

Comment 36

[Vlad Dascalu]

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