277454 - Bugs in inactivated groups don't show padlocks on the buglist

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, defect)

Description

[Dave Miller [:justdave]]

Last night I posted a blog entry asking for help fixing the op_sys field on a
bunch of corrupted bugs on bugzilla.mozilla.org.  This morning, people were
mentioning that they appeared to be all done, but when I ran the query myself, I
still got 15 of them.  We couldn't figure out why nobody else could see them --
they didn't have padlock icons on them.  Then I opened one of the bugs and
looked at it, and it was in the Netscape Confidential group, which was disabled
a long time ago to prevent people from placing new bugs in that group.

The code at fault appears to be the code that tests whether the bug is hidden by
a n implied or manual group setting.  Disabled groups show up as neither, and
wind up without a lock.  I would think they should be assumed to be manual.

Comment 1

[Marc Schumann [:Wurblzap]]

Attachment: Patch

Comment 2

[Christian Reis]

I don't like changing this bit of code very much; I wonder if the right thing
would be to have an explicit clause for the deactivated group case and then
explicitly set it to "manual" with a comment explaining why.

Comment 3

[Marc Schumann [:Wurblzap]]

I have no qualms at all touching this piece of code :)

Actually, I think the real cause of this bug is a semi-corrupt database. I
suspect the bugs in question show up in sanity check as "Have groups not
permitted for their products".

Comment 4

[Dave Miller [:justdave]]

OK, the conversion code in checksetup.pl for when the group_control_map table
was introduced appears to be at fault here.  A record for group_control_map was
never created for this group, which was already disabled before the
group_control_map mechanism was introduced.

See http://lxr.mozilla.org/bugzilla/source/checksetup.pl#3300

Comment 5

[Frédéric Buclin]

Comment on attachment 178455 [details] [diff] [review]
Patch

Your patch is correct and makes the code easier to read... but justdave is
right, the correct fix is to remove 'AND isactive != 0' from this query in
checksetup.pl, line 3301:

    $sth = $dbh->prepare("SELECT groups.id, products.id, groups.name, " .
			 "products.name FROM groups, products " .
			 "WHERE isbuggroup != 0 AND isactive != 0");

Moreover, checksetup.pl should fix corrupted DBs. For that, it has to look at
groups associated with bugs in bug_group_map which are in a product which is
not associated with that group in group_control_map. If such invalid
group-product combinations are found, it should add it to group_control_map
with CONTROLMAPSHOWN as a default in order to keep these bugs private. If the
group has the same name as a product, then some additional checks may be
required in order to set the correct values in group_control_map.

Comment 6

[Frédéric Buclin]

I guess 2.18 is affected too and this bug produces data corruption for disabled
groups.

Comment 7

[Dave Miller [:justdave]]

It was discussed on IRC that it's probably not safe to just have checksetup.pl
fix this, because there's no way to tell at this point if the missed conversion
is the reason for the corruption.  There appears to be a test for this in
sanitycheck already on the tip, probably that test should get backported to 2.18
and make sure the repair options offer allowing those groups as an option.

Comment 8

[Marc Schumann [:Wurblzap]]

Attachment: HEAD patch 2.1

o Makes buglist.cgi display padlocks even for semi-broken bugs
o Remedies checksetup.pl
o Fixes sanitycheck.cgi to complain about semi-broken bugs, offering to set up
  SHOWN/NA group_control_map entries

Comment 9

[Marc Schumann [:Wurblzap]]

Attachment: HEAD patch 2.1.1

Style changes only, as per discussion on IRC.

Comment 10

[Frédéric Buclin]

Comment on attachment 186585 [details] [diff] [review]
HEAD patch 2.1.1

>+        qq{INSERT INTO group_control_map (
>+                       group_id, product_id, entry,
>+                       membercontrol, othercontrol, canedit
>+                      )
>+               VALUES (
>+                       ?, ?, 0,
>+                       $shown, $na, 0
>+                      )});

( Nit: /me does not like parens alone on a line... sigh )


>+    PutFooter();
>+    exit;
>+}

Nit: do you really want to stop sanitycheck at this level? If you want to
submit a new patch with these two lines removed, feel free to carry forward my
r+.

This patch needs to be backported to 2.18.

r=LpSolit

Comment 11

[Marc Schumann [:Wurblzap]]

Attachment: HEAD Patch 2.2

Continues sanitycheck after repairing, and writes a nice finishing status
summary.

Comment 12

[Marc Schumann [:Wurblzap]]

Attachment: Branch patch 2.2

Comment 13

[Frédéric Buclin]

Comment on attachment 186814 [details] [diff] [review]
HEAD Patch 2.2

>+if (defined $cgi->param('createmissinggroupcontrolmapentries')) {
>+    my $na    = CONTROLMAPNA;
>+    my $shown = CONTROLMAPSHOWN;

Usually, we inform the user that something is being fixed, such as:

Status("OK, now fixing missing entries in group_control_map.");


I think you should do the same here. Add a convenient message and carry forward
my r+.

r=LpSolit

Comment 14

[Marc Schumann [:Wurblzap]]

Attachment: HEAD Patch 2.3

o Added a starting Status() call
o Tweaked the update message
o Tweaked the repair link text

Comment 15

[Marc Schumann [:Wurblzap]]

Attachment: Branch patch 2.3

Comment 16

[Frédéric Buclin]

Comment on attachment 186933 [details] [diff] [review]
HEAD Patch 2.3

r=LpSolit

Comment 17

[Frédéric Buclin]

Comment on attachment 186934 [details] [diff] [review]
Branch patch 2.3

>+                WHERE COALESCE(gcm.membercontrol, $na) = $na
>+          } . $dbh->sql_group_by('bugs.product_id, bgm.group_id'));

$dbh->sql_group_by() does not exist in 2.18. Use GROUP BY instead.

Else the patch works fine.

Comment 18

[Marc Schumann [:Wurblzap]]

Attachment: Branch patch 2.3.1

Uses GROUP BY.

Comment 19

[Frédéric Buclin]

Comment on attachment 187007 [details] [diff] [review]
Branch patch 2.3.1

r=LpSolit

Comment 20

[Frédéric Buclin]

Tip:

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.296; previous revision: 1.295
done
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.408; previous revision: 1.407
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.95; previous revision: 1.94
done


2.18.1:

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.289.2.32; previous revision: 1.289.2.31
done
Checking in sanitycheck.cgi;
/cvsroot/mozilla/webtools/bugzilla/sanitycheck.cgi,v  <--  sanitycheck.cgi
new revision: 1.72.2.3; previous revision: 1.72.2.2
done