Bug 497031

Created attachment 340859 [details]
Add checking incorrect needinfo flag statuses to sanitycheck

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 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 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

Noura, Dave,

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


Thanks,
Tony

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 on attachment 341047 [details]
Add checking incorrect needinfo flag statuses to sanitycheck

Looks good to me Tony.

committed patch into svn repo.

Close this bug.