305451 - GetGroupsByUserId() in buglist.cgi returns bad data

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Frédéric Buclin]

Due to the checkin of bug 304583, I cannot see groups anymore when editing
several bugs at once. I think the problem comes from GetGroupsByUserId() in
buglist.cgi which has not been updated. I tried the following change:

Before my hack:

    my $groups = $dbh->selectall_arrayref(
       "SELECT DISTINCT  groups.id, name, description, isactive
                   FROM  groups
             INNER JOIN  user_group_map
                     ON  user_group_map.group_id = groups.id
                  WHERE  user_id = ?
                    AND  isbless = 0
                    AND  isbuggroup = 1
               ORDER BY  description "
               , {Slice => {}}, ($userid));

After my hack:

    my $grouplist = Bugzilla->user->groups_as_string;
    my $groups = $dbh->selectall_arrayref(
       "SELECT DISTINCT  id, name, description, isactive
                   FROM  groups
                  WHERE  id IN ($grouplist)
                    AND  isbuggroup = 1
               ORDER BY  description "
               , {Slice => {}});

Now, groups are displayed, but the wrong groups are shown. I have no idea if the
problem is specific to my hack or if there is something more serious about User.pm.

Comment 1

[Frédéric Buclin]

(In reply to comment #0)
> Now, groups are displayed, but the wrong groups are shown.

I mean that inactive groups as well as groups where group control settings are
set to NA/NA are also shown.

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> I mean that inactive groups as well as groups where group control settings are
> set to NA/NA are also shown.

I guess this part will be fixed by bug 276553?

Comment 3

[Joel Peshkin]

Acually, the hack is the correct fix for the regression.   That restores the
behavior prior to bug 304583.   The other issue is one that has existed for a
long time which is that edit multiple shows too many groups.  That is bug 134474.

Comment 4

[Joel Peshkin]

Attachment: cleanup of LpSolit's hack

The hack was the right approach.

Comment 5

[Frédéric Buclin]

Comment on attachment 193405 [details] [diff] [review]
cleanup of LpSolit's hack

>-sub GetGroupsByUserId {
>+sub GetGroups {
>     my ($userid) = @_;

$userid is not a parameter anymore. Remove this line.


>        "SELECT DISTINCT  groups.id, name, description, isactive
>                    FROM  groups
>+                  WHERE  id IN ($grouplist)

Nit: as we handle only one group, 'groups.id' could be written 'id' alone.


I wondered if this routine should not be a method of the user object (i.e.
should go into User.pm). But looking at bug 276553, I think it's fine to keep
it in this file.

Comment 6

[Frédéric Buclin]

Comment on attachment 193405 [details] [diff] [review]
cleanup of LpSolit's hack

>+sub GetGroups {
>     my ($userid) = @_;
>     my $dbh = Bugzilla->dbh;
> 
>-    return if !$userid;

I think we should avoid to call the SQL query when the user ID is null:
my $user = Bugzilla->user;
return unless $user->id.

Comment 7

[Joel Peshkin]

Attachment: fixed with review comments

Comment 8

[Frédéric Buclin]

Comment on attachment 193434 [details] [diff] [review]
fixed with review comments

>     my $groups = $dbh->selectall_arrayref(
>+       "SELECT DISTINCT  id, name, description, isactive
>                    FROM  groups
>+                  WHERE  id IN ($grouplist)
>                     AND  isbuggroup = 1
>                ORDER BY  description "

Nit: DISTINCT is no longer required. Please remove it on checkin.

r=LpSolit

Comment 9

[Dave Miller [:justdave]]

This is going to bitrot bug 276553, but since it got denied review anyway, it'll
have to be redone anyhow.

Comment 10

[Frédéric Buclin]

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.307; previous revision: 1.306
done