Last Comment Bug 180879 - Implement privs for bug flags modification
: Implement privs for bug flags modification
Status: RESOLVED FIXED
: selenium
Product: Bugzilla
Classification: Server Software
Component: Creating/Changing Bugs (show other bugs)
: 2.17.1
: All All
: -- enhancement (vote)
: Bugzilla 2.20
Assigned To: Frédéric Buclin
: default-qa
Mentors:
: 227573 (view as bug list)
Depends on:
Blocks: 275610 275611 rt-clean-up 275608 288603
  Show dependency treegraph
 
Reported: 2002-11-19 03:45 PST by Bradley Baetz (:bbaetz)
Modified: 2012-12-18 20:46 PST (History)
10 users (show)
justdave: approval+
justdave: blocking2.20-
justdave: blocking2.18-
LpSolit: documentation+
LpSolit: documentation2.22+
LpSolit: documentation2.20+
LpSolit: testcase+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
restrict bug flag settings (4.01 KB, patch)
2004-09-22 12:17 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
restrict bug flag settings, v2 (3.71 KB, patch)
2004-09-22 16:50 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
Untested patch, work in progress (9.07 KB, patch)
2004-09-23 19:08 PDT, Frédéric Buclin
no flags Details | Diff | Splinter Review
flag restrictions using groups (11.51 KB, patch)
2004-10-02 17:22 PDT, Frédéric Buclin
bugreport: review-
Details | Diff | Splinter Review
flag restrictions using groups, v2 (21.15 KB, patch)
2004-10-12 17:55 PDT, Frédéric Buclin
myk: review-
Details | Diff | Splinter Review
validation now in Flag*.pm (15.78 KB, patch)
2004-10-24 05:16 PDT, Frédéric Buclin
myk: review+
bugreport: review-
Details | Diff | Splinter Review
validation now in Flag*.pm, v2 (16.51 KB, patch)
2004-11-05 12:52 PST, Frédéric Buclin
bugreport: review-
Details | Diff | Splinter Review
validation now in Flag*.pm, v2.1 (16.52 KB, patch)
2004-11-20 11:16 PST, Frédéric Buclin
bugreport: review+
Details | Diff | Splinter Review
docs patch about 'Bugzilla Database Tables' section, for 2.18 (982 bytes, patch)
2006-03-04 15:55 PST, victory <never@receive.bug.mails.i.hate.spammer>
LpSolit: review-
Details | Diff | Splinter Review
documentation patch, v1 (7.17 KB, patch)
2006-11-20 14:32 PST, Frédéric Buclin
myk: review-
Details | Diff | Splinter Review
documentation patch, v2 (7.22 KB, patch)
2006-11-21 02:01 PST, Frédéric Buclin
myk: review+
Details | Diff | Splinter Review

Description Bradley Baetz (:bbaetz) 2002-11-19 03:45:58 PST
This was discussed on irc, but not filed.

People w/o editbugs can only edit attachment flags on patches they attached.
That got inherited from the old attach status stuff.

However, there isn't a corresponding restriction on bug flags. My suggestion is:

- require editbugs to +/- a bug flag
- assignee can ?/cancel a bug flag

Should the reporter be able to make requests? What about the qa contact? I'm
leaning towards saying no, and we can always revisit this.

Its definately not useful for stuff like approval, which is what bmo will be
using it for, but I can imagine it being useful in other contexts. OTOH, such
sites can edit the sub in process_bug.
Comment 1 Asa Dotzler [:asa] 2002-12-05 13:51:01 PST
At bmo we are likely to have cases where it makes sense for the reporter to be
able to set flags on his bugs.  A reporter without edit bugs may use a bug flag
like "blocking1.3a" to "nominate" a bug for a particular milestone. I think that
a reporter should be able to make this changes to his own bugs. 

Here's my position:

Edit bugs should be required for setting flags. With editbugs a user should be
able to set "?", "+", and "-" on any bug. 

A reporter without editbugs should be able to set and unset "?" in bugs he filed
 but not set the "+" or "-".

Assignee and QA contact should have editbugs so there isn't any problem there.
They should be allowed to set any flags if they have editbugs.

Comment 2 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-11-08 20:50:18 PST
We seem to be having problems with this a lot at bmo now.  Might be nice to
sneak this in before bmo upgrades.

There's some cases where it would be useful to assign a group who is allowed to
+/- a flag rather than just assuming editbugs (although editbugs could certainly
be a default).  For example, drivers get to set the blocking flags for
Mozilla-related stuff, and there's in theory three people who are allowed to set
approval on Bugzilla stuff...
Comment 3 Dave Miller [:justdave] (justdave@bugzilla.org) 2003-12-05 08:12:29 PST
*** Bug 227573 has been marked as a duplicate of this bug. ***
Comment 4 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-04-16 21:21:39 PDT
This would still be nice to have for 2.18, but I'm not going to hold for it.  I
was planning to do this myself, but I'm probably not going to have time for it
in the near future.
Comment 5 Frédéric Buclin 2004-08-05 07:51:02 PDT
(In reply to comment #1)
> Here's my position:
> 
> Edit bugs should be required for setting flags. With editbugs a user should be
> able to set "?", "+", and "-" on any bug. 
> 
> A reporter without editbugs should be able to set and unset "?" in bugs he filed
>  but not set the "+" or "-".

Why not something like:

1. Reporter and users with canconfirm privs are allowed to set "?".
2. Reporter can unset "?" only if he set "?" himself.
3. Users with canconfirm privs can unset "?" only if they set "?" themselves or
the reporter did, but not if it was set by a user with editbugs privs, by the
assignee or by the QA contact.
4. Users with editbugs privs can set or unset "?", "-" and "+" flags
independently of who set a flag previously.
5. The Assignee and QA contact have the same rights (about flags) as users with
canconfirm privs.

erik, you said you were interested in taking this bug? ;-)
Comment 6 Erik Stambaugh 2004-08-05 10:59:09 PDT
(In reply to comment #5)

> erik, you said you were interested in taking this bug? ;-)
 
I have quite a queue right now.  If I run out of things to fix and this one
still needs someone, I will probably try it, but that could be several months at
least.  I don't want to assign it to myself and discourage anyone that wants to
fix this right now.
Comment 7 Frédéric Buclin 2004-09-22 12:17:55 PDT
Created attachment 159778 [details] [diff] [review]
restrict bug flag settings

This patch restricts bug flag modifications as per comment 5. Do not care about
the param name 'usereporterrestrictions'. It is because I use the same param as
in bug 90619. This is a minor consideration.

I hesitate to restrict the "?" status to the reporter, users with canedit and
canconfirm privs, as well as the owner and the assignee through
'usereporterrestrictions' and restrict as given by my patch by default (am I
clear?? ;-)).

justdave, erik, gerv, what do you think about this?
Comment 8 Frédéric Buclin 2004-09-22 16:50:40 PDT
Created attachment 159798 [details] [diff] [review]
restrict bug flag settings, v2

Flag settings now do not depend on any parameter.

In brief: users with editbugs privs can set/clear any flags ("+", "-" and "?").
QA contact and the owner (assignee) can clear the "?" flag independently of the
user who set that flag, but they cannot set/modify the "+" and the "-" flags
(except if they have editbugs privs, which is considered first). Any user
(including the owner and QA contact) can clear its own flags or set the "?"
flag if not yet defined ("X").

That seems a good compromise and also seems to be the minimum we may ask
Bugzilla to do.
Comment 9 Frédéric Buclin 2004-09-22 16:52:42 PDT
Comment on attachment 159798 [details] [diff] [review]
restrict bug flag settings, v2

justdave, please have a look! It looks like quite reasonable.
Comment 10 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-09-22 23:07:30 PDT
hmm, and all the comments we've since made on IRC in discussing this never made
it back to it back to the bug :(

What's been discussed on IRC is that since different flags are used for
different things, it would be much more useful if the privileges could be set on
each flag.  This would necessitate placing two groups (possibly 3?) on each flag
in the editflagtypes.cgi when editing a flag.  One group would define who is
allowed to request that flag (?), and one would define who is allowed to grant
or deny that flag (-/+).  The possible third group would define who is able to
even see the flag, although that one may make queries darn difficult to enforce,
so I'm not too concerned about it.
Comment 11 Frédéric Buclin 2004-09-23 19:08:43 PDT
Created attachment 159926 [details] [diff] [review]
Untested patch, work in progress

In reply to justdave's comment (also on IRC), I wrote a new patch using groups.
Note that this work is still in progress and this patch has not been tested
yet! I attach it here to have comments as soon as possible.

justdave, is that what you wanted?
Comment 12 Frédéric Buclin 2004-10-02 17:22:10 PDT
Created attachment 160881 [details] [diff] [review]
flag restrictions using groups

This one is a well tested patch used to restrict flag settings by using groups.
Comment 13 Frédéric Buclin 2004-10-02 17:24:28 PDT
Comment on attachment 160881 [details] [diff] [review]
flag restrictions using groups

justdave, please review!
Comment 14 Frédéric Buclin 2004-10-02 17:26:18 PDT
Comment on attachment 159798 [details] [diff] [review]
restrict bug flag settings, v2

removing review for this patch as specs have changed!
Comment 15 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-10-02 17:45:12 PDT
ok, this actually is an enhancement, and we're frozen for 2.20 (likely
unfreezing within a week or two when we branch).  I'll pounce on this as soon as
we branch.
Comment 16 Joel Peshkin 2004-10-02 18:18:21 PDT
Comment on attachment 160881 [details] [diff] [review]
flag restrictions using groups

Groups are numbers, not varchars.  You need to either store a single group id
for each action for each flag or have a flag_group_map that has a row for each
group in which the user must be a member to do that particular operation on
that particular flag.
Comment 17 Frédéric Buclin 2004-10-12 17:55:52 PDT
Created attachment 161919 [details] [diff] [review]
flag restrictions using groups, v2

Following my discussion with Joel Peshkin on IRC, some modifications have been
made:

1) Store group IDs instead of group names in the DB (to prevent problems when
renaming groups);
2) Add the 2 columns 'grant_id' and 'request_id' when upgrading Bugzilla;
3) Warn the user if some flag restrictions depend on groups which are going to
be deleted
4) Attachments now also depend on groups for flag settings.

I think this patch is now complete and can be reviewed! :)
Comment 18 Frédéric Buclin 2004-10-12 18:04:12 PDT
Comment on attachment 161919 [details] [diff] [review]
flag restrictions using groups, v2

justdave, again asking for review (sorry for bugspam).

This patch should now be complete!
Comment 19 Frédéric Buclin 2004-10-20 12:40:05 PDT
Comment on attachment 161919 [details] [diff] [review]
flag restrictions using groups, v2

Anybody else? :)
Comment 20 Myk Melez [:myk] [@mykmelez] 2004-10-21 01:59:09 PDT
Comment on attachment 161919 [details] [diff] [review]
flag restrictions using groups, v2

This code looks pretty good.  There are a couple issues with it, and I have
some style and clarity nits (i.e. optional, but recommended fixes), but overall
it's well on its way.


>  grant_id            MEDIUMINT     NOT NULL  DEFAULT 0 , 
>  request_id          MEDIUMINT     NOT NULL  DEFAULT 0

The "no group" value should be NULL rather than zero.


># Check if the user is allowed to set/modify flags

Shouldn't this be part of Flag->validate and FlagType->validate?


>    my $isqacontact = (($whoid eq $qacontact) && Param('useqacontact'));

Nit: check for Param('useqacontact') first.


>    SendSQL("SELECT type_id, setter_id, status FROM flags WHERE id = $id");
...
>    SendSQL("SELECT name, grant_id FROM flagtypes WHERE id = $type_id");

To reduce code duplication it would make sense to create a Flag "object" rather
than accessing the flags and flagtypes table directly, even if you only need a
subset of the data that the Flag object provides.


> my $cangrant = GroupIdToName($grant_id);

Nit: this is named similarly to isowner and isqacontact, so it looks like it
behaves the same way (i.e. a boolean value defining whether the user can grant
a flag, just as isowner defines whether the user is the owner); but in fact
it's different, since it contains the name of the group that can grant a flag
and is converted to the corresponding boolean value via UserInGroup($cangrant).
 The naming here should be consistent so that this variable is called f.e.
grant_group or $cangrant gets assigned to
UserInGroup(GroupIdToName($grant_id)).	The same is true for $canrequest.


>      name of the group allowed to grant/deny flags of this type
>      (do not specify any group to include all users)

"the group allowed to grant/deny flags of this type (to allow all users to
grant/deny these flags, leave this empty)"


>      if requestable, name of the group allowed to request flags of
>      this type (do not specify any group to include all users)

"if flags of this type are requestable, the group allowed to request them
(to allow all users to request these flags, leave this empty)"


>    <input type="text" name="grant_id" value="[% type.grant_id FILTER html %]" size="50" maxlength="255">
>    <input type="text" name="request_id" value="[% type.request_id FILTER html %]" size="50" maxlength="255">

Nit: seems like these controls would be better as drop-down menus, but I guess
we don't do that elsewhere either, so that's probably the stuff of another bug.


>  [% IF hasflags %]
>    <p><b>This group is used to restrict flag settings. You cannot delete
>    this group while flags use it.</b>
>    <br><a href="editflagtypes.cgi?action=list&group=[% gid FILTER html %]">Show
>    me which flags</a>
 - <input type="checkbox" name="removeflags">Remove all
>    flag dependencies to this group for me.</p>
>    <p><b>NOTE:</b> Deleting this group will remove all the corresponding
>    user restrictions related to these flags.</p>
>  [% END %]

"This group restricts who can make changes to flags of certain types.  Show me
which types. [] Remove all flag types from this group for me."

Also, I'm not sure the "NOTE:" makes sense here.  Of the other three
relationships between groups and other objects (users, bugs, and products),
only bugs has a note, and that's because removing bugs from a group is
sufficiently dangerous to warrant extra caution, which is not the case with
flags, which are publicly visible even when not modifiable.  Also, the note is
vague, since it doesn't say what user restrictions are being removed, leaving
the user wondering what flag restrictions we refer to.	We should either
specify exactly what will happen when the user checks that box or not warn the
user (in this case probably the latter).


>  [% ELSIF error == "flag_update_denied" %]
>    [% title = "Flag Modification Denied" %]
>    You tried to [% IF status == "+" %] grant [% ELSIF status == "-" %] deny
>    [% ELSIF status == "X" %] clear [% ELSE %] modify [% END %] the 
>    [% IF type == "b" %][% terms.bug %][% ELSE %] attachment [% END %] flag
>    <code>[% name FILTER html %]</code>. Only a sufficiently empowered user
>    [% IF status == "X" %] or the user who set the actual status
>    <code>[% name FILTER html %][% old_status %]</code>[% END %] is allowed
>    to change this [% IF type == "b" %][% terms.bug %][% ELSE %] attachment
>    [% END %] flag to that status.

You don't account for requests.  Also users never modify both bug and
attachment flags simultaneously, so it's not necessary to specify whether the
flag was a bug or attachment flag.  Also, elsewhere in the code we refer to the
flags by flag name only rather than calling them flags, so we should do so here
as well, i.e.:

"You tried to grant/deny/clear/request <flag name>.  Only a sufficiently
empowered user |or the user who set <flag name> in the first place| can make
this change."


>    The group [% name FILTER html %] does not exist. Please specifiy

specifiy -> specify


Otherwise it looks good.  I'm not a groups expert, so I may be missing problems
with the group code interaction.  This should thus be also reviewed by someone
with groups knowledge, but that review can be focused on that portion of the
code, since I've already done a general review here.
Comment 21 Frédéric Buclin 2004-10-24 04:55:34 PDT
Comment on attachment 161919 [details] [diff] [review]
flag restrictions using groups, v2

Much better patch coming soon... :)
Comment 22 Frédéric Buclin 2004-10-24 05:16:28 PDT
Created attachment 163201 [details] [diff] [review]
validation now in Flag*.pm

This new patch takes into account all remarks from myk. Validation is now made
in Flag*.pm which prevents code duplication.

I remove the ability for the owner and qa contact to clear the "?" flags. The
reasons are:

1) Users in edibugs group which are not in the (flag) grant group could
assigned the bug to themselves so that they can clear requests. I don't want
that! If they should be able to do so, administrators should put them in the
grant group.

2) Users able to request flags are in the (flag) request group. That means they
ARE allowed to request and I don't see why the owner or the qa contact who are
NOT in the grant group should be able to clear their requests. This is the job
of users in the grant group to grant/deny (or clear) flags.

Now you understand why I removed this part from my previous patch! ;)

It is important to note that due to the inclusion/exclusion lists associated
with flag types, and the fact that several flag types may have the same name,
users may be in the grant/request groups for some flag types and products but
not in others. This allows a better control on who can do what. (such a feature
does not exist for the editbugs, i.e on a per product basis. But this is
another story ;)).
Comment 23 Frédéric Buclin 2004-10-24 05:18:50 PDT
Comment on attachment 163201 [details] [diff] [review]
validation now in Flag*.pm

myk, all your comments have been included. Could you please review?
Comment 24 Frédéric Buclin 2004-10-24 05:21:42 PDT
Comment on attachment 163201 [details] [diff] [review]
validation now in Flag*.pm

also asking bugreport to review my patch about the groups part, as suggested by
myk.
Comment 25 Myk Melez [:myk] [@mykmelez] 2004-10-27 17:28:49 PDT
Comment on attachment 163201 [details] [diff] [review]
validation now in Flag*.pm

Hmm, this looks good.  My only nit is that grant_id and request_id would
probably make more sense as grant_group_id and request_group_id, since although
these names are unwieldy they follow the common Bugzilla practice of naming
foreign keys after the tables to which they're related.

r=myk
Comment 26 Joel Peshkin 2004-10-31 17:09:37 PST
Comment on attachment 163201 [details] [diff] [review]
validation now in Flag*.pm


With a few exceptions, this looks good (still need to test it, of course)...

>+        # - The flag is unchanged
>+        next if ($status eq $flag->{status});
>+
>+        # - User can clear flags set by itself
>+        next if (($status eq "X") && ($::userid eq $flag->{setter}));
>+
>+        # - User in the $grant_id group can set/clear flags,
>+        #   including "+" and "-"
>+        next if (!$flag->{type}->{grant_id} || 
>+                 &::UserInGroup(&::GroupIdToName($flag->{type}->{grant_id})));
>+


get rid of the globals ($::userid) should be replaced by looking up $user =
Bugzilla->user and then using $user->id for the userid and checking for a group
is done using Bugzilla->user->groups->{name} [ or $user->in_group(name) ] to
check on group membership.  Essentially, any time you see a "::" in your patch,
you are putting something in that we are trying to eradicate.
Comment 27 Christian Reis 2004-11-02 16:18:30 PST
(In reply to comment #26)
> &::UserInGroup(&::GroupIdToName($flag->{type}->{grant_id})));

> Essentially, any time you see a "::" in your patch,
> you are putting something in that we are trying to eradicate.

Except that GroupIdToName doesn't have an alternative that I see, so you can go
on using it.
Comment 28 Joel Peshkin 2004-11-02 16:56:54 PST
OK, fair enough.

Later, let's add a mechanism to confirm group membership by group id in User.pm
Comment 29 Frédéric Buclin 2004-11-05 12:52:42 PST
Created attachment 164780 [details] [diff] [review]
validation now in Flag*.pm, v2

Including myk's and bugreport's comments:
grant_id -> grant_group_id
request_id -> request_group_id
$::userid -> Bugzilla->user->id
&::UserInGroup -> Bugzilla->user->in_group()

+ add a comment when editing flag types that the request group has no effect if
the grant group is not defined (for obvious reasons).
Comment 30 Frédéric Buclin 2004-11-05 12:55:18 PST
Comment on attachment 164780 [details] [diff] [review]
validation now in Flag*.pm, v2

few modifications compared to the previous one.
Comment 31 Joel Peshkin 2004-11-19 23:28:21 PST
Comment on attachment 164780 [details] [diff] [review]
validation now in Flag*.pm, v2

This is failing runtests (filter violations).  I'm also getting some runtime
errors on my test install, but that may well be unrelated.
Comment 32 Frédéric Buclin 2004-11-20 11:16:59 PST
Created attachment 166593 [details] [diff] [review]
validation now in Flag*.pm, v2.1

Thanks to joel who noticed that I forgot to filter old_status in an error
message.
[% old_status FILTER html %] is now correct and runtests.sh does not complain
anymore.
Comment 33 Dave Miller [:justdave] (justdave@bugzilla.org) 2004-11-22 18:15:15 PST
This is actually a pretty high-impact high-gain patch, and since we haven't
actually branched yet, let's go ahead and throw this into 2.20.
Comment 34 Vlad Dascalu 2004-11-23 14:41:58 PST
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.313; previous revision: 1.312
done
Checking in editflagtypes.cgi;
/cvsroot/mozilla/webtools/bugzilla/editflagtypes.cgi,v  <--  editflagtypes.cgi
new revision: 1.10; previous revision: 1.9
done
Checking in editgroups.cgi;
/cvsroot/mozilla/webtools/bugzilla/editgroups.cgi,v  <--  editgroups.cgi
new revision: 1.43; previous revision: 1.42
done
Checking in Bugzilla/Flag.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Flag.pm,v  <--  Flag.pm
new revision: 1.23; previous revision: 1.22
done
Checking in Bugzilla/FlagType.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/FlagType.pm,v  <--  FlagType.pm
new revision: 1.10; previous revision: 1.9
done
Checking in template/en/default/admin/flag-type/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/flag-type/edit.html.tmpl,v
 <--  edit.html.tmpl
new revision: 1.8; previous revision: 1.7
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
new revision: 1.3; previous revision: 1.2
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v
 <--  user-error.html.tmpl
new revision: 1.75; previous revision: 1.74
done
Comment 35 victory <never@receive.bug.mails.i.hate.spammer> 2006-03-04 15:55:17 PST
Created attachment 214033 [details] [diff] [review]
docs patch about 'Bugzilla Database Tables' section, for 2.18
Comment 36 Frédéric Buclin 2006-03-06 16:26:57 PST
Comment on attachment 214033 [details] [diff] [review]
docs patch about 'Bugzilla Database Tables' section, for 2.18

this has nothing to do in this bug IMO. Moreover, you mention tables which do not appear in the figure above it.

This bug was about the grant_group_id and request_group_id fields of the flagtypes table.
Comment 37 Frédéric Buclin 2006-11-20 14:32:30 PST
Created attachment 246081 [details] [diff] [review]
documentation patch, v1
Comment 38 Myk Melez [:myk] [@mykmelez] 2006-11-20 16:34:06 PST
Comment on attachment 246081 [details] [diff] [review]
documentation patch, v1

>Index: docs/xml/administration.xml
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v
>retrieving revision 1.62
>diff -3 -p -u -r1.62 administration.xml
>--- docs/xml/administration.xml	13 Nov 2006 23:32:28 -0000	1.62
>+++ docs/xml/administration.xml	20 Nov 2006 22:30:48 -0000
>@@ -880,9 +880,9 @@
>    <section id="flag-askto">
>      <title>Using flag requests</title>
>      <para>
>-       If a flag has been defined as 'requestable', 
>-       users are allowed to set the flag's status to <quote>?</quote>.
>-       This status indicates that someone (aka <quote>the requester</quote> is asking
>+       If a flag has been defined as 'requestable', and a user has enough privileges
>+       to request it (see below), the user can set the flag's status to <quote>?</quote>.
>+       This status indicates that someone (aka <quote>the requester</quote>) is asking
>        for someone else to set the flag to either <quote>+</quote> or <quote>-</quote>.

Nits:

aka -> a.k.a.
for someone else -> someone else

(I realize these are both in the current version.)



>+           <listitem>
>+             <para>
>+	       Requests are listed in the <quote>Request Queue</quote> which
>+	       is accessible from the <quote>My Requests</quote> (if you are
>+	       logged in) or <quote>Requests</quote> (if you are logged out)
>+	       link visible in the footer of all pages.
>+             </para>
>+           </listitem>

<quote>Request Queue</quote> which -> <quote>Request Queue</quote>, which

Nit: <quote>My Requests</quote> (if you are logged in) or <quote>Requests</quote> (if you are logged out) link -> <quote>My Requests</quote> link (if you are logged in) or <quote>Requests</quote> link (if you are logged out)

>          Bug flags are used to set a status on the bug itself. You can 
>-         see Bug Flags in the <quote>Show Bug</quote> screen 
>-         (<filename>editbug.cgi</filename>).
>+         see Bug Flags in the <quote>Show Bug</quote> and <quote>Requests</quote>
>+	 screen, as above.

screen -> screens

What does "as above" mean?  Perhaps you mean "as described above"?



>-            The name may consist of any valid Unicode character. 
>+            The name may consist of any valid Unicode character, except commas
>+            and spaces.

Nit: consist of -> contain
Nit: character -> characters
Nit: , except -> except ?


>-            This describes the flag in more detail. At present, this doesn't
>-            show up anywhere helpful; ideally, it would be nice to have
>-            it show up as a tooltip. This field 
>-            can be as long as you like, and can contain any character you want.
>+            This describes the flag in more detail. This description is visible
>+            in a tooltip when hovering over a flag either in the "Show Bug" or
>+            "Edit Attachment" pages. This field  can be as long as you like,
>+            and can contain any character you want.

Nit: This describes the flag in more detail. This description is visible -> The description describes the flag in more detail. It is visible

"Show Bug" or "Edit Attachment" -> <quote>Show Bug</quote> or <quote>Edit Attachment</quote> ?


>             You may select a Product without selecting a specific Component,
>             but it is illegal to select a Component without a Product, or to select a
>-            Component that does not belong to the named Product. Doing so as of
>-            this writing (2.18rc3) will raise an error... even if all your products
>-            have a component by that name.
>+            Component that does not belong to the named Product. Doing so will raise
>+            an error... even if all your products have a component by that name.

but it is illegal to select -> but you can't select

The last sentence should read: If you do so, Bugzilla will display an error message, even if all your products have a component by that name.

We should really fix this.


>-            of the same type will appear below it with <quote>addl.</quote> (short for 
>+            of the same type will appear below it with <quote>addl.</quote> (short for

Trailing space removal?


>+	<section id="flags-create-field-cclist">
>+          <title>CC List</title>
>+
>+          <para>
>+            If you want certain users to be notified every time this flag is 
>+            set to ?, -, +, or unset, add them here. This is a comma-separated 
>+            list of email addresses that need not be restricted to Bugzilla usernames..

.. -> .


>+	<section id="flags-create-grant-group">
>+	  <title>Grant Group</title>
>+	  <para>
>+	    When this field is set to some given group, only users in this group
>+	    can set the flag to <quote>+</quote> and <quote>-</quote>. These users
>+	    can also do requests as well as cancelling the flag. If this field
>+	    is left blank, all users can set or delete this flag. This feature
>+	    is very useful when you want to restrict who is allowed to decide
>+	    whether a request should be approved or rejected.

do requests -> request

cancelling -> canceling ?

Nit: only users in this group -> only users in the group

Nit: These users can also do requests as well as canceling the flag. -> This field does not affect who can request or cancel the flag.  For that, see the <quote>Request Group</quote> field below.

Nit: This feature is very useful when you want to restrict who is allowed to decide whether a request should be approved or rejected. -> This field is useful for restricting which users can approve or reject requests.


>+	<section id="flags-create-request-group">
>+	  <title>Request Group</title>
>+	  <para>
>+	    When this field is set to some given group, only users in this group
>+	    can do requests for this flag, unless the <quote>grant group</quote>
>+	    is empty, in which case this field has no effect. Combined with the
>+	    <quote>grant group</quote>, this lets you have a delimited group
>+	    of users who can do requests, which can be the same group as those
>+	    who can do approvals or not. Also, all users in the <quote>request
>+	    group</quote> can cancel any flag, independently of its status.

I think this reads better as:

When this field is set to some given group, only users in the group can request or cancel this flag.  Note that this field has no effect if the <quote>grant group</quote> field is empty.  You can set the value of this field to a different group, but both fields have to be set to a group for this field to have an effect.
Comment 39 Frédéric Buclin 2006-11-21 02:01:34 PST
Created attachment 246136 [details] [diff] [review]
documentation patch, v2

All comments and nits fixed.
Comment 40 victory <never@receive.bug.mails.i.hate.spammer> 2006-11-21 09:50:35 PST
Comment on attachment 246136 [details] [diff] [review]
documentation patch, v2

+          <title>Grant Group</title>
+            useful for restricting which users can approve or reject requests.

not grant?
Comment 41 Frédéric Buclin 2006-11-21 12:50:05 PST
tip:

Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <--  administration.xml
new revision: 1.63; previous revision: 1.62
done

2.22.1:

Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <--  administration.xml
new revision: 1.55.2.4; previous revision: 1.55.2.3
done

2.20.3:

Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <--  administration.xml
new revision: 1.50.2.7; previous revision: 1.50.2.6
done

Note You need to log in before you can comment on or make changes to this bug.