526184 - Allow groups to be specified in email_in.pl and the WebService Bug.create

Status: RESOLVED FIXED

(Bugzilla :: Incoming Email, enhancement)

Description

[Max Kanat-Alexander]

Basically, all we need to do to allow this is change how we pass groups around in post_bug and process_bug. We should just have a single field called "groups" that has multiple values, and that contains the *names* of groups instead of strange and cryptic id numbers with "bit-\d+" field names.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Okay, here we go! This also involves a pretty nice simplification of the group-checking code for filing bugs.

I plan to use group_is_valid in more places in the future, and groups_available may be used in more places as well.

I didn't adjust the process_bug code, so email_in won't be able to adjust groups very easily, but at least it will be able to create them, and this was all I needed for the WebService.

Comment 2

[Max Kanat-Alexander]

Attachment: Actual v1

Oops, that was the wrong patch. This one's right.

Comment 3

[Frédéric Buclin]

Comment on attachment 409911 [details] [diff] [review]
Actual v1

>=== modified file 'Bugzilla/Bug.pm'

> sub _check_groups {

>     my $user = Bugzilla->user;
>     my $controls = $product->group_controls;

These two variables are no longer used.


>+        my $group = new Bugzilla::Group({ name => $name })
>+            # Don't expose the existence or non-existence of groups.
>+            or next;

Put the comment before my $group = ... . Having "or next;" alone is more than confusing.



>=== modified file 'Bugzilla/Product.pm'

> use Bugzilla::Mailer;
> use Bugzilla::Series;
> 
>+use Scalar::Util qw(blessed);

Nit: we use to list non-Bugzilla modules before Bugzilla ones. Would be good to do the same here, for consistency.


>+sub groups_available {

We don't need nor want separate SQL queries in this method. You should call $self->group_controls, which already contains all the data we need, and use the same logic as originally used in Bugzilla::Bug::_check_groups().


>+sub groups_mandatory {

group_mandatory_for() is still used in Bugzilla::Bug::set_product().


>+=item C<group_is_valid>

>+Tells you whether or not the currently-logged-in user can set a group
>+on a bug (whether or not they match the MemberControl/OtherControl
>+settings for a group in this product).

This description is confusing. A user cannot decide to not set a mandatory group; he has no control on mandatory groups.

Comment 4

[Reed Loden [:reed]]

This needs to be relnote'd in big, bold print, as it could cause people's custom create forms to not place security bugs in the right group(s) anymore.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #3)
> > use Bugzilla::Mailer;
> > use Bugzilla::Series;
> > 
> >+use Scalar::Util qw(blessed);
> 
> Nit: we use to list non-Bugzilla modules before Bugzilla ones. Would be good to
> do the same here, for consistency.

  I think we list non-Bugzilla modules after Bugzilla modules now, more often.

  I'll get fixes for other stuff soon.

Comment 6

[Max Kanat-Alexander]

Attachment: v2

Okay, so I did everything except for changing groups_available.

I actually thought about what you said a lot, and wrestled with it a bit in my mind. The reasons that I decided to stay with having SQL in groups_available are:

* group_controls returns a hash, not an arrayref, so there's no standard ordering. This would cause group controls to switch around positions on the page every time you loaded it, unless I re-created the Bugzilla::Group objects using new_from_list, which seemed like a waste.

* I like the SQL in groups_available--it's very straightforward, simple, readable, and does just one thing, whereas group_controls is used all over the place to do many things and so grabs more data than we need (which doesn't *really* matter, for this point I'm making) and also is a bit more complicated (which is a little important).

* I actually want to try to move away from using the group_controls accessor in as many places as possible, in favor of focusing on groups_available and groups_mandatory instead. I think that group_controls will still be used in admin pages, perhaps, but I think that groups_available and groups_mandatory are generally easier to use.


Also, you'll notice that I created groups_in_sql, and I started to use it in a few places other than just the places touched by this patch normally. This doesn't address the fact that we use "NOT IN" with groups in a few places without a sql_in call. That's something we should address in another bug, because we have to talk about (or test) whether we should just be doing NOT ( sql_in ) or whether we should be creating a sql_not_in function.

Comment 7

[Frédéric Buclin]

This is a major code change, and we are feature frozen for 3.6. I would like to focus on stability and QA rather than reviewing and taking such a patch for 3.6 so late in the game. Max, is that fine with you to retarget it to 3.8?

Comment 8

[Max Kanat-Alexander]

Yeah, that's fine.

Comment 9

[Frédéric Buclin]

Comment on attachment 418118 [details] [diff] [review]
v2

>Index: Bugzilla/Bug.pm

>+    foreach my $name (@$group_names) {
>+       # We don't want to expose the existence or non-existence of groups,
>+       # so instead of doing check(), we just do "next" on an invalid group.
>+        my $group = new Bugzilla::Group({ name => $name })
>+            or next;

Fix the indentation of the comment.


>-    foreach my $id (keys %$controls) {
>-        next unless $controls->{$id}->{'group'}->is_active;

This line explicitly skips inactive groups...


>+    $add_groups{$_->id} = $_ foreach @{ $product->groups_mandatory };

... but $product->groups_mandatory doesn't. Maybe should we skip them from here till bug 526189 is completely fixed.



>Index: Bugzilla/Product.pm

>+sub groups_available {

>+    my %other_groups = @{ $dbh->selectcol_arrayref(
>+        "SELECT group_id, othercontrol
>+           FROM group_control_map
>+                INNER JOIN groups ON group_control_map.group_id = groups.id
>+          WHERE isbuggroup = 1 AND isactive = 1 AND product_id = ?
>+                AND othercontrol = $shown OR othercontrol = $default", 

Missing parens around |othercontrol = $shown OR othercontrol = $default|. This makes enter_bug.cgi and show_bug.cgi display default groups from other products, and process_bug.cgi then refuses to remove them despite they are illegal for this product.


>+Tells you what groups are set to Default or Shown for the 
>+currently-logged-in user (taking into account both OtherControl and
>+ MemberControl). Returns an arrayref of L<Bugzilla::Group> objects with

Nit: extra whitespace at the beginning of the line.



>Index: Bugzilla/User.pm

>         my $glist = $self->groups_as_string;
>         $sth = $dbh->prepare("SELECT DISTINCT grantor_id
>                                  FROM group_group_map
>-                                WHERE member_id IN($glist)
>+                                WHERE " . $self->groups_in_sql('member_id') . "
>                                   AND grant_type=" . GROUP_VISIBLE);

$glist is no longer used.



>Index: Bugzilla/WebService/Bug.pm

>+=item The C<groups> argument was added in Bugzilla B<3.6>.

s/3.6/3.8/



>Index: template/en/default/bug/create/create.html.tmpl

>+        <input type="checkbox" id="groups_[% group.id FILTER html %]"
>+               name="groups" value="[% group.name FILTER html %]"
>+               [% ' checked="checked"' IF default.groups.contains(group.name)
>+                                          OR group.is_default %]>
>+          <label for="groups_[% group.id FILTER html %]">

<label> should be vertically aligned with <input>.


Otherwise looks good, even if my testing is incomplete due to the missing parens above.

Comment 10

[Max Kanat-Alexander]

Attachment: v3

Okay, this patch also moves the "default groups" stuff into Bugzilla::Bug->_check_groups, so that it works in both the WebService and email_in.

Comment 11

[Max Kanat-Alexander]

  Oh, and I didn't fix the skipping-mandatory-groups thing if they're not active...I think maybe we should fix that first? Or I could just do it in this patch, I suppose.

Comment 12

[Frédéric Buclin]

Comment on attachment 428123 [details] [diff] [review]
v3

Seems to work fine. I also tested email_in.pl but not the WS. r=LpSolit

Comment 13

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/                       
modified email_in.pl
modified enter_bug.cgi
modified post_bug.cgi
modified Bugzilla/Bug.pm
modified Bugzilla/Product.pm
modified Bugzilla/User.pm
modified Bugzilla/WebService/Bug.pm
modified template/en/default/filterexceptions.pl
modified template/en/default/bug/create/create.html.tmpl
Committed revision 7026.

Comment 14

[Max Kanat-Alexander]

Added to the release notes in bug 604256.