Bug 497031 - Need additional sanitycheck for incorrect needinfo flag statuses
Need additional sanitycheck for incorrect needinfo flag statuses
Status: CLOSED NEXTRELEASE
Product: Bugzilla
Classification: Community
Component: Bugzilla General (Show other bugs)
3.2
All Linux
low Severity medium (vote)
: ---
: ---
Assigned To: Tony Fu
:
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2009-04-21 23:57 EDT by David Lawrence
Modified: 2013-06-24 00:07 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2009-04-30 23:51:33 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
Add checking incorrect needinfo flag statuses to sanitycheck (4.74 KB, patch)
2009-04-22 23:52 EDT, Tony Fu
dkl: review-
nelhawar: review+
Details | Diff
Add checking incorrect needinfo flag statuses to sanitycheck (6.60 KB, patch)
2009-04-23 22:08 EDT, Tony Fu
dkl: review+
tfu: review? (nelhawar)
Details | Diff

  None (edit)
Description David Lawrence 2009-04-21 23:57:36 EDT
Description of problem:
Currently we use the needinfo flag specifically for requesting information from a specific user or anyone in general. The needinfo flag should not ever be in either the <blank> or ? status. We need a sanity check that returns a list of bugs that have needinfo statuses set to either - or +. Some people mistakenly set these manually and they may not get cleared properly.
Comment 1 Tony Fu 2009-04-22 23:52:22 EDT
Created attachment 340859 [details]
Add checking incorrect needinfo flag statuses to sanitycheck
Comment 2 Noura El hawary 2009-04-23 09:02:20 EDT
Comment on attachment 340859 [details]
Add checking incorrect needinfo flag statuses to sanitycheck

Hey Tony,

Patch looks good to me, I tested it and it works good , maybe good idea to add the check to the cron job.

Thanks,
Noura
Comment 3 David Lawrence 2009-04-23 12:06:51 EDT
Comment on attachment 340859 [details]
Add checking incorrect needinfo flag statuses to sanitycheck

>Index: sanitycheck.cgi
>===================================================================
>--- sanitycheck.cgi	(revision 1198)
>+++ sanitycheck.cgi	(working copy)
>@@ -113,6 +113,7 @@
>   control_values
>   unsent_emails
>   flag_check
>+  flag_needinfo_status_check
>   bugs_components
>   bugs_activity_flag
>   whines_obsolete_target
>@@ -962,7 +963,49 @@
>     }
>     $cgi->param('flag_check_report', 'output');
> }

Nit: add extra newline here

>+# RED HAT ENTENSION START 497031 
> ###########################################################################
>+# Check for flags "needinfo" having incorrect status
>+###########################################################################
>+
>+if ($cgi->param('flag_needinfo_status_check')) {
>+
>+    $cgi->param('output', '');
>+
>+    Status('flag_needinfo_status_check_start');
>+
>+    my $invalid_flags = $dbh->selectall_arrayref(
>+           'SELECT DISTINCT flags.status, flags.id, flags.attach_id, flags.bug_id
>+              FROM flags
>+        INNER JOIN flagtypes
>+                ON flags.type_id = flagtypes.id
>+             WHERE flagtypes.name = "needinfo"
>+               AND flags.status = "+" OR flags.status = "-"');
>+

You need to add parenthesis ( ) around flags.status = "+" OR flags.status = "-". Otherwise
you get more rows that you want. On bz-web2-test I got ~4000 rows where with the parens I
got only 88. 

>+    my @invalid_flags = @$invalid_flags;
>+
>+    if (scalar(@invalid_flags)) {
>+        if ($cgi->param('remove_invalid_needinfo_status')) {
>+            Status('flag_deletion_start');
>+            my @flag_ids = map {$_->[1]} @invalid_flags;
>+            # Silently delete these flags, with no notification to requesters/setters.
>+            $dbh->do('DELETE FROM flags WHERE id IN (' . join(',', @flag_ids) .')');
>+            Status('flag_deletion_end');

I would rather we add a log activity entry here and not just blindly delete the flags from the
flags table. We need to retain proper bug history whenever possible. You have the entry recorded
as being changed by bugzilla@redhat.com and of course send no email notification.

The rest looks fine so far. Make sure to add to cronjob script like Noura mentioned so we can get eng-systems to update it later.

Dave
Comment 4 Tony Fu 2009-04-23 22:05:39 EDT
Noura, Dave,

Thank you for the review.  I have changed my patch according to your review, please see attached file for details.


Thanks,
Tony
Comment 5 Tony Fu 2009-04-23 22:08:18 EDT
Created attachment 341047 [details]
Add checking incorrect needinfo flag statuses to sanitycheck

1. corrected a sql query error
2. added bugs_activity log
3. added flag_needinfo_status_check check to cron jobs.
Comment 6 David Lawrence 2009-04-24 11:51:45 EDT
Comment on attachment 341047 [details]
Add checking incorrect needinfo flag statuses to sanitycheck

Looks good to me Tony.
Comment 7 Tony Fu 2009-04-30 23:51:11 EDT
committed patch into svn repo.

Close this bug.

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