Bug 406261 - 3.16: Bugzilla/SanityCheck.pm
3.16: Bugzilla/SanityCheck.pm
Status: CLOSED NEXTRELEASE
Product: Bugzilla
Classification: Community
Component: Bugzilla General (Show other bugs)
3.2
All Linux
high Severity medium (vote)
: ---
: ---
Assigned To: Noura El hawary
:
Depends On:
Blocks: 406201
  Show dependency treegraph
 
Reported: 2007-11-30 11:58 EST by David Lawrence
Modified: 2013-06-24 00:15 EDT (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-05-22 02:36:56 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)
patch to enable specific sanitychecks and diff checks in sanitycheck.cgi and sanitycheck.pl (35.00 KB, patch)
2008-04-10 02:19 EDT, Noura El hawary
no flags Details | Diff
patch to enable specific sanitychecks and diff checks in sanitycheck.cgi and sanitycheck.pl (35.00 KB, patch)
2008-04-10 02:20 EDT, Noura El hawary
dkl: review-
nelhawar: review? (kbaker)
Details | Diff
sanitycheck patch (38.42 KB, patch)
2008-04-13 23:40 EDT, Noura El hawary
dkl: review-
Details | Diff
Looks good Noura. Go ahead and check in. Dave (38.82 KB, patch)
2008-04-14 09:16 EDT, Noura El hawary
dkl: review+
Details | Diff
sanitycheck patch (40.47 KB, patch)
2008-04-15 02:49 EDT, Noura El hawary
dkl: review+
Details | Diff


External Trackers
Tracker ID Priority Status Summary Last Updated
Mozilla Foundation 427936 None None None Never

  None (edit)
Description David Lawrence 2007-11-30 11:58:25 EST
Description:
The Bugzilla/SanityCheck.pm module contains the code that was originally in sanitycheck.cgi. The module was created so that different front ends could be wrapped around it such as a command line utility or web UI.

Function Requirements:
Bugzilla/SanityCheck.pm sanitycheck.pl
Comment 1 Noura El hawary 2007-12-18 08:25:34 EST
Looking at the module Bugzilla/SanityCheck.pm in rh_bugzilla_2_18 and
sanitycheck.cgi in Bugzilla_3 , The following is noted:

1- Bugzilla_3 sanitycheck.cgi includes code that checks for user permissions to
perform the sanitychecks, code to perform the sanity checking for the following:
(referential cross checks, double field referential cross checks, login checks,
vote/keyword cache checks, flags being in incorrect products and components,
General bug checks, Control Values, Unsent mail) ,, and then code to fix the
errors found by the sanity checking.

2- rh_bugzilla_2_18 Bugzilla/SanityCheck.pm includes code that performs sanity
checking only ,no code for fixing and no code for checking user permissions, as
this module is used by the bugzilla team to sanity check the database. The
checks included are : ('foreign_keys',  'profile_loginnames', 'keyword_cache',
'general_bug_check','date_check','control_values',
'unsent_emails','bugs_components') .

3- So basically the checks performed by our rh_bugzilla_2_18
Bugzilla/SanityCheck.pm are similar to the ones in Bugzilla_3 sanitycheck.cgi
only one check we are missing which is:
flags being in incorrect products and components

which is of total 40 lines of code (calculated by running cloc on that code)

Also some work will be needed to convert the current SanityCheck.pm code to the
coding standards, and few lines of code will be added in sanitycheck.pl to call
that new check that will be added So I would expect in total it would be around
60 lines of code + the effort for the conversion of code to comply with coding
standards. 

Comment 2 Kevin Baker 2007-12-18 08:42:39 EST
1. What about LOC for unit tests?

2. Also, this sounds like we would be trying to get a patch accepted upstream. 
There is a risk then that the upstream won't accept it. What is your plan B in 
this case? How will you code it then?
Comment 3 Noura El hawary 2007-12-18 09:07:23 EST
(In reply to comment #2)
> 1. What about LOC for unit tests?
> 
Basically for unit test I think sanitycheck.pl will do the job for us ,, as we
can only test that extra check by giving option for this check in the command
line to sanitycheck.pl


> 2. Also, this sounds like we would be trying to get a patch accepted upstream. 
> There is a risk then that the upstream won't accept it. What is your plan B in 
> this case? How will you code it then?

I am not sure if the upstream would want to replace sanitycheck.cgi with
SanityCheck.pm ,, we have done that replacement when someone ran sanitycheck.cgi
and produced a lot of annoying unnecessary emails to lots of people in redhat. I
think they might accept SanityCheck.pm as an entire new module ,, but this mean
that they will have replicated code,, hmm so my plan would be to move all the
functionality from sanitycheck.cgi to Bugzilla/SanityCheck.pm that includes all
the code for the checks and for the fixing. Then we would code sanitycheck.cgi
to call those functions in SanityCheck.pm so it can provide an interface for the
users through the web UI and we should keep all the permissions restrictions for
running those checks in sanitycheck.cgi ,, and we will have sanitycheck.pl to do
similar to what it is doing now in our rh_bugzilla_2_18 which is similar to what
it is doing in the upstream version only we would convert it to use
SanityCheck.pm instead of using sanitycheck.cgi   

Comment 4 Kevin Baker 2007-12-29 13:26:36 EST
(In reply to comment #3)
> (In reply to comment #2)
> > 1. What about LOC for unit tests?
> > 
> Basically for unit test I think sanitycheck.pl will do the job for us ,, as 
we
> can only test that extra check by giving option for this check in the 
command
> line to sanitycheck.pl

That is not a test. A unit test should test the functions in the 
SanityCheck.pm module.

> > 2. Also, this sounds like we would be trying to get a patch accepted 
upstream. 
> > There is a risk then that the upstream won't accept it. What is your plan 
B in 
> > this case? How will you code it then?
> 
> I am not sure if the upstream would want to replace sanitycheck.cgi with
> SanityCheck.pm ,, we have done that replacement when someone ran 
sanitycheck.cgi
> and produced a lot of annoying unnecessary emails to lots of people in 
redhat. I
> think they might accept SanityCheck.pm as an entire new module ,, but this 
mean
> that they will have replicated code,, hmm so my plan would be to move all 
the
> functionality from sanitycheck.cgi to Bugzilla/SanityCheck.pm that includes 
all
> the code for the checks and for the fixing. Then we would code 
sanitycheck.cgi
> to call those functions in SanityCheck.pm so it can provide an interface for 
the
> users through the web UI and we should keep all the permissions restrictions 
for
> running those checks in sanitycheck.cgi ,, and we will have sanitycheck.pl 
to do
> similar to what it is doing now in our rh_bugzilla_2_18 which is similar to 
what
> it is doing in the upstream version only we would convert it to use
> SanityCheck.pm instead of using sanitycheck.cgi   

Talk with the upstream about how best to do this. The only way to know is to 
ask. ;)
Comment 5 Noura El hawary 2008-04-10 02:19:40 EDT
Created attachment 301931 [details]
patch to enable specific sanitychecks and diff checks in sanitycheck.cgi and sanitycheck.pl

HI,,

Attached is a patch to sanitycheck.cgi and sanitycheck.pl ,, it will enable the
following:
1- user can run specific sanitycheck reports from the webui by passing cgi
params like for example:
https://bugdev.devel.redhat.com/bugzilla/sanitycheck.cgi?foreign_keys=1&unsent_emails=1


2- if user didn specify any check cgi params then all checks will run.

4- user also can use commandline script sanitycheck.pl to run specific checks
same way as how it worked in RHBZ2.18 for example:
perl sanitycheck.pl --profile_loginnames --unsent_emails --login 
'bugzilla-owner@redhat.com'

5- user can generate diff reports by running sanitycheck.pl with option
--diff_report

6- if user wants to get full sanitycheck reports not only the errors then they
can use --verbose option

- Please note that there is no need now for Bugzilla/SanityCheck.pm to be
borted from 2.18 to 3.2 and sanitycheck.cgi and sanitycheck.pl do its work
nicely.

- I haven't put REDHAT tags except for the redhat database fields and table in
the cross checks , as I was thiking we might contribute this patch to upstream,
but I would think maybe better to give me your opinions first before i submit
it upstream.

-please note the patch looks abit too long but that is becuase there is a lot
of identation changes resulted from adding if statements around
Comment 6 Noura El hawary 2008-04-10 02:20:15 EDT
Created attachment 301932 [details]
patch to enable specific sanitychecks and diff checks in sanitycheck.cgi and sanitycheck.pl

HI,,

Attached is a patch to sanitycheck.cgi and sanitycheck.pl ,, it will enable the
following:
1- user can run specific sanitycheck reports from the webui by passing cgi
params like for example:
https://bugdev.devel.redhat.com/bugzilla/sanitycheck.cgi?foreign_keys=1&unsent_emails=1


2- if user didn specify any check cgi params then all checks will run.

4- user also can use commandline script sanitycheck.pl to run specific checks
same way as how it worked in RHBZ2.18 for example:
perl sanitycheck.pl --profile_loginnames --unsent_emails --login 
'bugzilla-owner@redhat.com'

5- user can generate diff reports by running sanitycheck.pl with option
--diff_report

6- if user wants to get full sanitycheck reports not only the errors then they
can use --verbose option

- Please note that there is no need now for Bugzilla/SanityCheck.pm to be
borted from 2.18 to 3.2 and sanitycheck.cgi and sanitycheck.pl do its work
nicely.

- I haven't put REDHAT tags except for the redhat database fields and table in
the cross checks , as I was thiking we might contribute this patch to upstream,
but I would think maybe better to give me your opinions first before i submit
it upstream.

-please note the patch looks abit too long but that is becuase there is a lot
of identation changes resulted from adding if statements around the different
check sections in sanitycheck.cgi.

please review when you can.

Thanks,
Noura
Comment 7 Noura El hawary 2008-04-10 02:25:06 EDT
worked 12 hours
Comment 8 Noura El hawary 2008-04-10 02:30:52 EDT
the patch is applied to bugdev.devel.redhat.com , so you can test it there.
Comment 9 Noura El hawary 2008-04-10 03:01:18 EDT
please note that there are 2 checks that we used to have in 2.18 that are not
required anymore for 3.2 which are:

1- date_check ,, as that check was used for groups.last_changed and
profiles.refreshed_when those fields don't exist anymore.

2- bugs_components,, for some bugs were found by Tony where the component ids in
some bugs were inconsistent with the actual component ids of the bug's product,
running that check produced no errors at all so I am guess that Tony has fixed
them all , so need for that check. Tony can you please confirm.

Noura
Comment 10 David Lawrence 2008-04-11 11:43:35 EDT
Comment on attachment 301932 [details]
patch to enable specific sanitychecks and diff checks in sanitycheck.cgi and sanitycheck.pl

>Index: sanitycheck.cgi
>===================================================================

Looks fine to me.

>Index: sanitycheck.pl
>===================================================================
>RCS file: /cvs/qa/rh_bugzilla_3/sanitycheck.pl,v
>retrieving revision 1.2
>diff -p -u -r1.2 sanitycheck.pl
>--- sanitycheck.pl	31 Jan 2008 17:48:09 -0000	1.2
>+++ sanitycheck.pl	10 Apr 2008 06:07:50 -0000
>@@ -18,6 +18,7 @@
> # Frédéric Buclin. All Rights Reserved.
> #
> # Contributor(s): Frédéric Buclin <LpSolit@gmail.com>
>+#                 Noura Elhawary <nelhawar@redhat.com>
> 
> use strict;
> 
>@@ -32,49 +33,157 @@ use Bugzilla::Mailer;
> use Getopt::Long;
> use Pod::Usage;
> 
>-my $verbose = 0; # Return all comments if true, else errors only.
>+use English qw( -no_match_vars );
>+use Text::Diff;

You will need to add this to Bugzilla/Install/Requirements.pm so that
checksetup.pl will complain if Text::Diff is not installed. Otherwise you
should code this to make it an optional feature.

my $do_text_diff = 1;
eval "use Text::Diff";
$@ && $do_text_diff = 0;

>+my $verbose = 0;    # Return all comments if true, else errors only.
> my $login = '';  # Login name of the user which is used to call sanitycheck.cgi.
>-my $help = 0;    # Has user asked for help on this script?
>+my $help  = 0;   # Has user asked for help on this script?
>

Nit: Line variables up with $version = 0;

> 
>-pod2usage({-verbose => 1, -exitval => 1}) if $help;
>+GetOptions( \%options, 'verbose', 'login=s', 'help|h|?', @OptionDefs );
>+
>+pod2usage( { -verbose => 1, -exitval => 1 } ) if $help;

Nit: Upstream does not like the extra whitespace near the parens ( and ) even
though perltidy 
does. So we should remove them for quicker review.

> Bugzilla->usage_mode(USAGE_MODE_CMDLINE);
> 
> # Be sure a login name if given.
> $login || ThrowUserError('invalid_username');
> 
>-my $user = new Bugzilla::User({ name => $login })
>-  || ThrowUserError('invalid_username', { name => $login });
>+my $user = new Bugzilla::User( { name => $login } )
>+  || ThrowUserError( 'invalid_username', { name => $login } );
>

Nit: same comment about extra whitespace. Also definitely do not change any
lines that are not necessary for program proper function or are not additional
lines we add. This will keep the patch size smaller and insure quicker review
from upstream.

>-my $cgi = Bugzilla->cgi;
>+my $cgi      = Bugzilla->cgi;
> my $template = Bugzilla->template;
> 

Same comment as above.

> # Authenticate using this user account.
> Bugzilla->set_user($user);
> 
>-# Pass this param to sanitycheck.cgi.
>-$cgi->param('verbose', $verbose);
>+# build check list
>+my @checks;
>+for my $check (@Checks) {
>+    if ( $options{$check} ) {
>+        $cgi->param( $check, 1 );
>+        push @checks, $check;
>+    }
>+}
>+
>+# if no checks given on command line then assume all checks
>+if ( @checks == 0 ) {    
>+    push @checks, @Checks;
>+    $cgi->param( $_, 1 ) foreach @checks;
>+}
> 
>+# run checks and report
>+my $msg_body = '';    # accumulate check reports here
>+
>+# Pass this param to sanitycheck.cgi.
>+$cgi->param( 'verbose', $verbose );
> require 'sanitycheck.cgi';
> 
>-# Now it's time to send an email to the user if there is something to notify.
>-if ($cgi->param('output')) {
>+for my $check (@checks) {
>+
>+    my $out_rpt = $check . "_report";
>+
>+   # Now it's time to send an email to the user if there is something to notify.
>     my $message;
>     my $vars = {};
>-    $vars->{'addressee'} = $user->email;
>-    $vars->{'output'} = $cgi->param('output');
>+    $vars->{'addressee'}   = $user->email;
>+    $vars->{'output'}      = $cgi->param($out_rpt);
>     $vars->{'error_found'} = $cgi->param('error_found') ? 1 : 0;
> 
>-    $template->process('email/sanitycheck.txt.tmpl', $vars, \$message)
>-      || ThrowTemplateError($template->error());
>+    $template->process( 'email/sanitycheck.txt.tmpl', $vars, \$message )
>+      || ThrowTemplateError( $template->error() );
>+

No white space changes here.

>+    # full_report if given then report in case of FAIL or PASS
>+    # otherwise only report in case of FAIL
>+    if ( $options{diff_report} ) {
>+        $msg_body .= create_diff_report( $check, $message );
>+    }

Add the $do_text_diff check here:

    if ($options{diff_report} && $do_text_diff) {
	$msg_body .= create_diff_report( $check, $message );
    }

>+    else {
>+        $msg_body .= create_report( $check, $message );
>+    }
>+}


The rest looks fine to me.

Dave
Comment 11 David Lawrence 2008-04-11 12:48:38 EDT
Also please make sure you pass back in the enabled checks when clicking on any
of the lines in the sanitycheck.cgi report such as "Click here to delete invalid
flags", "Click here to rebuild the keyword cache", etc. Otherwise when you click
on any of those links, it will try to run all checks on the second pass. You
will probably have to do this in the templates as well as sanitycheck.cgi.

Here is some patch code to illustrate what I mean:

Index: sanitycheck.cgi
===================================================================
RCS file: /cvs/qa/rh_bugzilla_3/sanitycheck.cgi,v
retrieving revision 1.2
diff -u -r1.2 sanitycheck.cgi
--- sanitycheck.cgi 2 Apr 2008 18:48:27 -0000   1.2
+++ sanitycheck.cgi 11 Apr 2008 16:46:18 -0000
@@ -49,6 +50,9 @@
     my $cgi = Bugzilla->cgi;
     return if (!$alert && Bugzilla->usage_mode == USAGE_MODE_CMDLINE &&
!$cgi->param('verbose'));

+    # Pass in current query url as well
+    $vars->{query} = $cgi->canonicalise_query();
+
     if (Bugzilla->usage_mode == USAGE_MODE_CMDLINE) {
         my $output = $cgi->param('output') || '';
         my $linebreak = $alert ? "\nALERT: " : "\n";

Index: template/en/default/admin/sanitycheck/messages.html.tmpl
===================================================================
RCS file:
/cvs/qa/rh_bugzilla_3/template/en/default/admin/sanitycheck/messages.html.tmpl,v
retrieving revision 1.3
diff -u -r1.3 messages.html.tmpl
--- template/en/default/admin/sanitycheck/messages.html.tmpl    27 Feb 2008
20:50:44 -0000  1.3
+++ template/en/default/admin/sanitycheck/messages.html.tmpl    11 Apr 2008
16:46:18 -0000
@@ -34,7 +34,7 @@
     [% errortext FILTER html %]: [% INCLUDE bug_list badbugs = badbugs %]

   [% ELSIF san_tag == "bug_check_repair" %]
-    <a href="sanitycheck.cgi?[% param FILTER url_quote %]=1">[% text FILTER
html %]</a>.
+    <a href="sanitycheck.cgi?[% param FILTER url_quote %]=1&[% query %]">[%
text FILTER html %]</a>.

   [% ELSIF san_tag == "bug_check_creation_date" %]
     Checking for [% terms.bugs %] with no creation date (which makes them
invisible).

Index: template/en/default/filterexceptions.pl
===================================================================
RCS file: /cvs/qa/rh_bugzilla_3/template/en/default/filterexceptions.pl,v
retrieving revision 1.4
diff -u -r1.4 filterexceptions.pl
--- template/en/default/filterexceptions.pl 31 Mar 2008 18:35:32 -0000  1.4
+++ template/en/default/filterexceptions.pl 11 Apr 2008 16:46:18 -0000
@@ -557,6 +557,10 @@
   'new_status.id',
 ],

+'admin/sanitycheck/messages.html.tmpl' => [
+  'query',
+],
+
 'account/login.html.tmpl' => [
   'target',
 ],
Comment 12 Noura El hawary 2008-04-13 23:40:42 EDT
Created attachment 302299 [details]
sanitycheck patch 

Thanks for the review Dave. Here is another patch with all your suggestions
applied.

Noura
Comment 13 Noura El hawary 2008-04-13 23:42:21 EDT
worked one hour
Comment 14 David Lawrence 2008-04-14 00:21:07 EDT
Comment on attachment 302299 [details]
sanitycheck patch 


The recent check in of changes upstream submitted by you causes conflicts with
sanitycheck.cgi when applying your patch (upstream bug 427936). I have merged
the upstream sanitycheck.cgi changes with our tree recently so update your
checkout and generate a new patch for review.

>+if ($cgi->param('foreign_keys')) {
>+    $cgi->param('output' , '');
>+  
>+    CrossCheck('classifications', 'id',
>+               ['products', 'classification_id']);
>+
>+    CrossCheck("keyworddefs", "id",
>+               ["keywords", "keywordid"]);
>+
>+    CrossCheck("fielddefs", "id",
>+               ["bugs_activity", "fieldid"],
>+               ['profiles_activity', 'fieldid']);
>+
>+    CrossCheck("flagtypes", "id",
>+               ["flags", "type_id"],
>+               ["flagexclusions", "type_id"],
>+               ["flaginclusions", "type_id"]);

>Index: sanitycheck.pl
>===================================================================
>-my $result = GetOptions('verbose'  => \$verbose,
>-                        'login=s'  => \$login,
>-                        'help|h|?' => \$help);
>+use English qw( -no_match_vars );
>+use Text::Diff;
> 
>-pod2usage({-verbose => 1, -exitval => 1}) if $help;
>+my $do_text_diff = 1;
>+eval "use Text::Diff";
>+if ($@){
>+    warn $@;
>+    $do_text_diff = 0;
>+}

In sanitycheck.pl you are still doing 'use Text::Diff;' on line 37 even though
you are eval'ing 'use Text::Diff;' on line 40. Please remove the first one.

Dave
Comment 15 Noura El hawary 2008-04-14 09:16:48 EDT
Created attachment 302332 [details]
Looks good Noura. Go ahead and check in.

Dave

Hi Dave,, I fixed the patch so it deosn conflict with sanitycheck.cgi and also
made a fix for the use of Text::Diff 

Thanks,
Noura
Comment 16 David Lawrence 2008-04-14 12:32:50 EDT
Comment on attachment 302332 [details]
Looks good Noura. Go ahead and check in.

Dave

>Index: sanitycheck.pl
>===================================================================
>RCS file: /cvs/qa/rh_bugzilla_3/sanitycheck.pl,v
>retrieving revision 1.2
>diff -p -u -r1.2 sanitycheck.pl
>--- sanitycheck.pl	31 Jan 2008 17:48:09 -0000	1.2
>+++ sanitycheck.pl	14 Apr 2008 13:12:58 -0000
>@@ -18,6 +18,7 @@
> # Frédéric Buclin. All Rights Reserved.
> #
> # Contributor(s): Frédéric Buclin <LpSolit@gmail.com>
>+#                 Noura Elhawary <nelhawar@redhat.com>
> 
> use strict;
> 
>@@ -32,15 +33,41 @@ use Bugzilla::Mailer;
> use Getopt::Long;
> use Pod::Usage;
> 
>-my $verbose = 0; # Return all comments if true, else errors only.
>-my $login = '';  # Login name of the user which is used to call sanitycheck.cgi.
>-my $help = 0;    # Has user asked for help on this script?
>-
>-my $result = GetOptions('verbose'  => \$verbose,
>-                        'login=s'  => \$login,
>-                        'help|h|?' => \$help);
>+use English qw( -no_match_vars );
> 
>-pod2usage({-verbose => 1, -exitval => 1}) if $help;
>+my $do_text_diff = 1;
>+eval {use Text::Diff};
>+if ($@){
>+    warn $@;
>+    $do_text_diff = 0;
>+}
>+
>+my $verbose = 0;    # Return all comments if true, else errors only.
>+my $login   = '';   # Login name of the user which is used to call sanitycheck.cgi.
>+my $help    = 0;    # Has user asked for help on this script?
>+
>+my %options = ('verbose' => \$verbose, 'login' => \$login, 'help' => \$help);
>+
>+my @OptionDefs = (
>+    'foreign_keys',   'profile_loginnames',
>+    'keyword_cache',  'general_bug_check',
>+    'control_values', 'unsent_emails',
>+    'flag_check',     'diff_report');
>+
>+# check options
>+my @Checks = qw(
>+  foreign_keys
>+  profile_loginnames
>+  keyword_cache
>+  general_bug_check
>+  control_values
>+  unsent_emails
>+  flag_check
>+);
>+
>+GetOptions(\%options, 'verbose', 'login=s', 'help|h|?', @OptionDefs);
>+
>+pod2usage({ -verbose => 1, -exitval => 1 }) if $help;
> 
> Bugzilla->usage_mode(USAGE_MODE_CMDLINE);
> 
>@@ -56,25 +83,113 @@ my $template = Bugzilla->template;
> # Authenticate using this user account.
> Bugzilla->set_user($user);
> 
>+# build check list
>+my @checks;
>+for my $check (@Checks) {
>+    if ( $options{$check} ) {
>+        $cgi->param( $check, 1 );
>+        push @checks, $check;
>+    }
>+}
>+
>+# if no checks given on command line then assume all checks
>+if (@checks == 0) {    
>+    push @checks, @Checks;
>+    $cgi->param($_, 1) foreach @checks;
>+}
>+
>+# run checks and report
>+my $msg_body = '';    # accumulate check reports here
>+
> # Pass this param to sanitycheck.cgi.
> $cgi->param('verbose', $verbose);
> 
> require 'sanitycheck.cgi';
> 
>-# Now it's time to send an email to the user if there is something to notify.
>-if ($cgi->param('output')) {
>+for my $check (@checks) {
>+
>+    my $out_rpt = $check . "_report";
>+
>+   # Now it's time to send an email to the user if there is something to notify.
>     my $message;
>     my $vars = {};
>     $vars->{'addressee'} = $user->email;
>-    $vars->{'output'} = $cgi->param('output');
>+    $vars->{'output'} = $cgi->param($out_rpt);
>     $vars->{'error_found'} = $cgi->param('error_found') ? 1 : 0;
> 
>     $template->process('email/sanitycheck.txt.tmpl', $vars, \$message)
>       || ThrowTemplateError($template->error());
> 
>-    MessageToMTA($message);
>+    # full_report if given then report in case of FAIL or PASS
>+    # otherwise only report in case of FAIL
>+    if ($options{diff_report} && $do_text_diff) {
>+        $msg_body .= create_diff_report($check, $message);
>+    }
>+    else {
>+        $msg_body .= create_report($check, $message);
>+    }
>+}
>+
>+if (Bugzilla->params->{mail_delivery_method} eq 'None') {
>+    print "$msg_body\n";
>+}
>+else {
>+    MessageToMTA($msg_body);
>+}
>+
>+# returns string report
>+sub create_report {
>+    my ($check, $rpt) = @_;
>+
>+    my $check_rpt =
>+        "Check: $check\n===================================="
>+        . "===============================\n$rpt\n";
>+
>+    return $check_rpt;
>+}
>+
>+# returns empty string '' if no difference, otherwise a string representing the diff report
>+sub create_diff_report {
>+    my ($check, $rpt) = @_;
>+
>+    my $report = create_report($check, $rpt);
>+
>+    my $baseline_fn = $check . ".orig";
>+
>+    $baseline_fn = "data/sanitycheck/$baseline_fn";
>+
>+    if (not -e $baseline_fn) {
>+        create_report_file($baseline_fn, $report);
>+        return '';
>+    }
>+
>+    # write the current report to a file to diff it against
>+    # the baseline report
>+    my $rpt_file = 'rpt_tmp_file';
>+    create_report_file($rpt_file, $report);
>+
>+    my $diff = diff $baseline_fn, $rpt_file;
>+
>+    if ($diff) {
>+        return "Difference Check: $check\n"
>+          . "===================================================================\n"
>+          . "$diff\n\n";
>+    }
>+
>+    unlink $rpt_file;
>+    return '';
> }
> 
>+sub create_report_file {
>+
>+    my ($file_name, $report) = @_;
>+    open my $FILE, ">", $file_name
>+      or die "Couldn't open '$file_name' : $OS_ERROR";
>+    print {$FILE} $report
>+      or die "Couldn't write to '$file_name': $OS_ERROR";
>+    close $FILE or die "Couldn't close '$file_name': $OS_ERROR";
>+
>+}
> 
> __END__
> 
>@@ -86,6 +201,8 @@ sanitycheck.pl - Perl script to perform 
> 
>  ./sanitycheck.pl [--help]
>  ./sanitycheck.pl [--verbose] --login <user@domain.com>
>+ ./sanitycheck.pl [check_name] --login <user@domain.com>
>+ ./sanitycheck.pl [check_name] [--diff_report] --login <user@domain.com>
> 
> =head1 OPTIONS
> 
>@@ -107,10 +224,70 @@ This should be passed the email address 
> running the Sanity Check process, a user with the editcomponents priv. This
> user will receive an email with the results of the script run.
> 
>+=item B<--diff_report>
>+
>+Generates only diff report for checks by comparing freshly generated check report 
>+with stored report for that check.
>+
>+=item B<check_name>
>+
>+This can be any one of the following checknames or a combination of them:
>+
>+=over
>+
>+=item B<--foreign_keys>
>+
>+Performs referential integrity checks (cross and double cross).
>+
>+=item B<--profile_loginname>
>+
>+Performs login email checks for format and lowercase.
>+
>+=item B<--keyword_cache>
>+
>+Performs vote/keyword cache checks.
>+
>+=item B<--general_bug_check>
>+
>+Performs general bug checks ex: resolutions, statuses etc.
>+
>+=item B<--control_values>
>+
>+Performs group control values are validation check.
>+
>+=item B<--unsent_emails>       
>+
>+Performs checks for bugs that have changes but no mail sent for at least half an hour.
>+
>+=item B<--flag_check>
>+
>+Check for flags being in incorrect products and components.
>+
>+=back 
>+
> =back
> 
> =head1 DESCRIPTION
> 
> This script provides a way of running a 'Sanity Check' on the database
> via either a CLI or cron. It is equivalent to calling sanitycheck.cgi
>-via a web broswer.
>+via a web broswer with an extra feature for generating diff sanitycheck
>+reports. If the user specified the checks they want to perform then only
>+those checks will be performed, otherwise all checks will be performed.
>+
>+The generated check reports can be of three types:
>+
>+"full reports"
>+    that include errors as well as successful checks, to get
>+    those the --verbose option must be set.
>+
>+"fail reports"
>+    that include only errors in the reports, to get those 
>+    the --verbose must be ignored.
>+
>+"diff reports"
>+    to produce diff check reports. freshly generated reports
>+    are diffed against original ones kept in the data/sanitycheck
>+    dir and the result is the desired diff reports. to get those
>+    the --diff_report option must be set.
>+
>Index: sanitycheck.cgi
>===================================================================
>RCS file: /cvs/qa/rh_bugzilla_3/sanitycheck.cgi,v
>retrieving revision 1.3
>diff -p -u -r1.3 sanitycheck.cgi
>--- sanitycheck.cgi	12 Apr 2008 05:01:32 -0000	1.3
>+++ sanitycheck.cgi	14 Apr 2008 13:13:11 -0000
>@@ -23,6 +23,7 @@
> #                 Max Kanat-Alexander <mkanat@bugzilla.org>
> #                 Marc Schumann <wurblzap@gmail.com>
> #                 Frédéric Buclin <LpSolit@gmail.com>
>+#                 Noura Elhawary <nelhawar@redhat.com>
> 
> use strict;
> 
>@@ -49,6 +50,9 @@ sub Status {
>     my $cgi = Bugzilla->cgi;
>     return if (!$alert && Bugzilla->usage_mode == USAGE_MODE_CMDLINE && !$cgi->param('verbose'));
> 
>+    # Pass in current query url as well
>+    $vars->{query} = $cgi->canonicalise_query();
>+
>     if (Bugzilla->usage_mode == USAGE_MODE_CMDLINE) {
>         my $output = $cgi->param('output') || '';
>         my $linebreak = $alert ? "\nALERT: " : "\n";
>@@ -96,6 +100,28 @@ unless (Bugzilla->usage_mode == USAGE_MO
>       || ThrowTemplateError($template->error());
> }
> 
>+# check if any cgi params were set for specific checks
>+# if not then all checks should run
>+my $check_all = 1;
>+
>+my @check_params = qw(
>+  foreign_keys
>+  profile_loginnames
>+  keyword_cache
>+  general_bug_check
>+  control_values
>+  unsent_emails
>+  flag_check
>+);
>+for my $chk (@check_params) {
>+    if ($cgi->param($chk)){
>+        $check_all = 0;
>+    }
>+}
>+if ($check_all) {
>+    $cgi->param($_, 1) foreach @check_params;
>+}
>+
> ###########################################################################
> # Users with 'editkeywords' privs only can only check keywords.
> ###########################################################################
>@@ -390,133 +416,168 @@ sub CrossCheck {
>     }
> }
> 
>-CrossCheck('classifications', 'id',
>-           ['products', 'classification_id']);
>+if ($cgi->param('foreign_keys')) {
>+    $cgi->param('output' , '');
>+  
>+    CrossCheck('classifications', 'id',
>+               ['products', 'classification_id']);
>+
>+    CrossCheck("keyworddefs", "id",
>+               ["keywords", "keywordid"]);
>+
>+    CrossCheck("fielddefs", "id",
>+               ["bugs_activity", "fieldid"],
>+               ['profiles_activity', 'fieldid']);
>+
>+    CrossCheck("flagtypes", "id",
>+               ["flags", "type_id"],
>+               ["flagexclusions", "type_id"],
>+               ["flaginclusions", "type_id"]);
>+
>+    CrossCheck("bugs", "bug_id",
>+               ["bugs_activity", "bug_id"],
>+               ["bug_group_map", "bug_id"],
>+               ["attachments", "bug_id"],
>+               ["cc", "bug_id"],
>+               ["longdescs", "bug_id"],
>+               ["dependencies", "blocked"],
>+               ["dependencies", "dependson"],
>+               ['flags', 'bug_id'],
>+               ["votes", "bug_id"],
>+               ["keywords", "bug_id"],
>+               ["duplicates", "dupe_of", "dupe"],
>+               ["duplicates", "dupe", "dupe_of"],
>+               # REDHAT EXTENSION START 406261 
>+               ["bz_it_map", "bz_id", "it_id"],
>+               ["ext_bz_bug_map", "bug_id", "ext_bz_id"]);
>+               # REDHAT EXTENSION END 406261
>+             
>+    CrossCheck("groups", "id",
>+               ["bug_group_map", "group_id"],
>+               ['category_group_map', 'group_id'],
>+               ["group_group_map", "grantor_id"],
>+               ["group_group_map", "member_id"],
>+               ["group_control_map", "group_id"],
>+               ["namedquery_group_map", "group_id"],
>+               ["user_group_map", "group_id"],
>+               ["flagtypes", "grant_group_id"],
>+               ["flagtypes", "request_group_id"],
>+               # REDHAT EXTENSION START 406261
>+               ["flagtypes", "view_group_id"]);
>+               # REDHAT EXTENSION END 406261
>+
>+    CrossCheck("namedqueries", "id",
>+               ["namedqueries_link_in_footer", "namedquery_id"],
>+               ["namedquery_group_map", "namedquery_id"],
>+              );
>+
>+    CrossCheck("profiles", "userid",
>+               ['profiles_activity', 'userid'],
>+               ['profiles_activity', 'who'],
>+               ['email_setting', 'user_id'],
>+               ['profile_setting', 'user_id'],
>+               ["bugs", "reporter", "bug_id"],
>+               ["bugs", "assigned_to", "bug_id"],
>+               ["bugs", "qa_contact", "bug_id"],
>+               ["attachments", "submitter_id", "bug_id"],
>+               ['flags', 'setter_id', 'bug_id'],
>+               ['flags', 'requestee_id', 'bug_id'],
>+               ["bugs_activity", "who", "bug_id"],
>+               ["cc", "who", "bug_id"],
>+               ['quips', 'userid'],
>+               ["votes", "who", "bug_id"],
>+               ["longdescs", "who", "bug_id"],
>+               ["logincookies", "userid"],
>+               ["namedqueries", "userid"],
>+               ["namedqueries_link_in_footer", "user_id"],
>+               ['series', 'creator', 'series_id'],
>+               ["watch", "watcher"],
>+               ["watch", "watched"],
>+               ['whine_events', 'owner_userid'],
>+               ["tokens", "userid"],
>+               ["user_group_map", "user_id"],
>+               ["components", "initialowner", "name"],
>+               ["components", "initialqacontact", "name"],
>+               ["component_cc", "user_id"]);
>+
>+    CrossCheck("products", "id",
>+               ["bugs", "product_id", "bug_id"],
>+               ["components", "product_id", "name"],
>+               ["milestones", "product_id", "value"],
>+               ["versions", "product_id", "value"],
>+               ["group_control_map", "product_id"],
>+               ["flaginclusions", "product_id", "type_id"],
>+               ["flagexclusions", "product_id", "type_id"]);
>+
>+    CrossCheck("components", "id",
>+               ["component_cc", "component_id"],
>+               ["bugs", "component_id"],
>+               ["flagexclusions", "component_id", "type_id"],
>+               ["flaginclusions", "component_id", "type_id"]);
>+
>+    # Check the former enum types -mkanat@bugzilla.org
>+    CrossCheck("bug_status", "value",
>+               ["bugs", "bug_status", "bug_id"]);
>+
>+    CrossCheck("resolution", "value",
>+               ["bugs", "resolution", "bug_id"]);
>+
>+    CrossCheck("bug_severity", "value",
>+               ["bugs", "bug_severity", "bug_id"]);
>+
>+    CrossCheck("op_sys", "value",
>+               ["bugs", "op_sys", "bug_id"]);
>+
>+    CrossCheck("priority", "value",
>+               ["bugs", "priority", "bug_id"]);
>+
>+    CrossCheck("rep_platform", "value",
>+               ["bugs", "rep_platform", "bug_id"]);
>+
>+    CrossCheck('series', 'series_id',
>+               ['series_data', 'series_id']);
>+
>+    CrossCheck('series_categories', 'id',
>+               ['series', 'category'],
>+               ["category_group_map", "category_id"],
>+               ["series", "subcategory", "name"]);
>+
>+    # REDHAT EXTENSION START 406261    
>+    CrossCheck("external_bugzilla", "id",
>+               ["ext_bz_bug_map", "ext_bz_id", "bug_id"]);
>+    # REDHAT EXTENSION END 406261
>+
>+    CrossCheck('whine_events', 'id',
>+               ['whine_queries', 'eventid'],
>+               ['whine_schedules', 'eventid']);
>+
>+    CrossCheck('attachments', 'attach_id',
>+               ['attach_data', 'id'],
>+               ['bugs_activity', 'attach_id']);
>+
>+    CrossCheck('bug_status', 'id',
>+               ['status_workflow', 'old_status'],
>+               ['status_workflow', 'new_status']);
>+
>+    DoubleCrossCheck('attachments', 'bug_id', 'attach_id',
>+                     ['flags', 'bug_id', 'attach_id'],
>+                     ['bugs_activity', 'bug_id', 'attach_id']);
>+
>+    DoubleCrossCheck("components", "product_id", "id",
>+                     ["bugs", "product_id", "component_id", "bug_id"],
>+                     ['flagexclusions', 'product_id', 'component_id'],
>+                     ['flaginclusions', 'product_id', 'component_id']);
>+
>+    DoubleCrossCheck("versions", "product_id", "value",
>+                     ["bugs", "product_id", "version", "bug_id"]);
>+
>+    DoubleCrossCheck("milestones", "product_id", "value",
>+                     ["bugs", "product_id", "target_milestone", "bug_id"],
>+                     ["products", "id", "defaultmilestone", "name"]);
> 
>-CrossCheck("keyworddefs", "id",
>-           ["keywords", "keywordid"]);
>-
>-CrossCheck("fielddefs", "id",
>-           ["bugs_activity", "fieldid"],
>-           ['profiles_activity', 'fieldid']);
>-
>-CrossCheck("flagtypes", "id",
>-           ["flags", "type_id"],
>-           ["flagexclusions", "type_id"],
>-           ["flaginclusions", "type_id"]);
>-
>-CrossCheck("bugs", "bug_id",
>-           ["bugs_activity", "bug_id"],
>-           ["bug_group_map", "bug_id"],
>-           ["attachments", "bug_id"],
>-           ["cc", "bug_id"],
>-           ["longdescs", "bug_id"],
>-           ["dependencies", "blocked"],
>-           ["dependencies", "dependson"],
>-           ['flags', 'bug_id'],
>-           ["votes", "bug_id"],
>-           ["keywords", "bug_id"],
>-           ["duplicates", "dupe_of", "dupe"],
>-           ["duplicates", "dupe", "dupe_of"]);
>-
>-CrossCheck("groups", "id",
>-           ["bug_group_map", "group_id"],
>-           ['category_group_map', 'group_id'],
>-           ["group_group_map", "grantor_id"],
>-           ["group_group_map", "member_id"],
>-           ["group_control_map", "group_id"],
>-           ["namedquery_group_map", "group_id"],
>-           ["user_group_map", "group_id"],
>-           ["flagtypes", "grant_group_id"],
>-           ["flagtypes", "request_group_id"]);
>-
>-CrossCheck("namedqueries", "id",
>-           ["namedqueries_link_in_footer", "namedquery_id"],
>-           ["namedquery_group_map", "namedquery_id"],
>-          );
>-
>-CrossCheck("profiles", "userid",
>-           ['profiles_activity', 'userid'],
>-           ['profiles_activity', 'who'],
>-           ['email_setting', 'user_id'],
>-           ['profile_setting', 'user_id'],
>-           ["bugs", "reporter", "bug_id"],
>-           ["bugs", "assigned_to", "bug_id"],
>-           ["bugs", "qa_contact", "bug_id"],
>-           ["attachments", "submitter_id", "bug_id"],
>-           ['flags', 'setter_id', 'bug_id'],
>-           ['flags', 'requestee_id', 'bug_id'],
>-           ["bugs_activity", "who", "bug_id"],
>-           ["cc", "who", "bug_id"],
>-           ['quips', 'userid'],
>-           ["votes", "who", "bug_id"],
>-           ["longdescs", "who", "bug_id"],
>-           ["logincookies", "userid"],
>-           ["namedqueries", "userid"],
>-           ["namedqueries_link_in_footer", "user_id"],
>-           ['series', 'creator', 'series_id'],
>-           ["watch", "watcher"],
>-           ["watch", "watched"],
>-           ['whine_events', 'owner_userid'],
>-           ["tokens", "userid"],
>-           ["user_group_map", "user_id"],
>-           ["components", "initialowner", "name"],
>-           ["components", "initialqacontact", "name"],
>-           ["component_cc", "user_id"]);
>-
>-CrossCheck("products", "id",
>-           ["bugs", "product_id", "bug_id"],
>-           ["components", "product_id", "name"],
>-           ["milestones", "product_id", "value"],
>-           ["versions", "product_id", "value"],
>-           ["group_control_map", "product_id"],
>-           ["flaginclusions", "product_id", "type_id"],
>-           ["flagexclusions", "product_id", "type_id"]);
>-
>-CrossCheck("components", "id",
>-           ["component_cc", "component_id"],
>-           ["flagexclusions", "component_id", "type_id"],
>-           ["flaginclusions", "component_id", "type_id"]);
>-
>-# Check the former enum types -mkanat@bugzilla.org
>-CrossCheck("bug_status", "value",
>-            ["bugs", "bug_status", "bug_id"]);
>-
>-CrossCheck("resolution", "value",
>-            ["bugs", "resolution", "bug_id"]);
>-
>-CrossCheck("bug_severity", "value",
>-            ["bugs", "bug_severity", "bug_id"]);
>-
>-CrossCheck("op_sys", "value",
>-            ["bugs", "op_sys", "bug_id"]);
>-
>-CrossCheck("priority", "value",
>-            ["bugs", "priority", "bug_id"]);
>-
>-CrossCheck("rep_platform", "value",
>-            ["bugs", "rep_platform", "bug_id"]);
>-
>-CrossCheck('series', 'series_id',
>-           ['series_data', 'series_id']);
>-
>-CrossCheck('series_categories', 'id',
>-           ['series', 'category'],
>-           ["category_group_map", "category_id"],
>-           ["series", "subcategory"]);
>-
>-CrossCheck('whine_events', 'id',
>-           ['whine_queries', 'eventid'],
>-           ['whine_schedules', 'eventid']);
>-
>-CrossCheck('attachments', 'attach_id',
>-           ['attach_data', 'id'],
>-           ['bugs_activity', 'attach_id']);
>-
>-CrossCheck('bug_status', 'id',
>-           ['status_workflow', 'old_status'],
>-           ['status_workflow', 'new_status']);
>+    $cgi->param('foreign_keys_report', $cgi->param('output'));
> 
>+}
> ###########################################################################
> # Perform double field referential (cross) checks
> ###########################################################################
>@@ -575,41 +636,37 @@ sub DoubleCrossCheck {
>     }
> }
> 
>-DoubleCrossCheck('attachments', 'bug_id', 'attach_id',
>-                 ['flags', 'bug_id', 'attach_id'],
>-                 ['bugs_activity', 'bug_id', 'attach_id']);
>-
>-DoubleCrossCheck("components", "product_id", "id",
>-                 ["bugs", "product_id", "component_id", "bug_id"],
>-                 ['flagexclusions', 'product_id', 'component_id'],
>-                 ['flaginclusions', 'product_id', 'component_id']);
>-
>-DoubleCrossCheck("versions", "product_id", "value",
>-                 ["bugs", "product_id", "version", "bug_id"]);
>- 
>-DoubleCrossCheck("milestones", "product_id", "value",
>-                 ["bugs", "product_id", "target_milestone", "bug_id"],
>-                 ["products", "id", "defaultmilestone", "name"]);
>-
> ###########################################################################
> # Perform login checks
> ###########################################################################
> 
>-Status('profile_login_start');
>+if ($cgi->param('profile_loginnames')) {
> 
>-my $sth = $dbh->prepare(q{SELECT userid, login_name FROM profiles});
>-$sth->execute;
>+    $cgi->param('output', '');
> 
>-while (my ($id, $email) = $sth->fetchrow_array) {
>-    validate_email_syntax($email)
>-      || Status('profile_login_alert', {id => $id, email => $email}, 'alert');
>-}
>+    Status('profile_login_start');
>+
>+    my $sth = $dbh->prepare(q{SELECT userid, login_name FROM profiles});
>+    $sth->execute;
>+
>+    while (my ($id, $email) = $sth->fetchrow_array) {
>+        validate_email_syntax($email)
>+            || Status('profile_login_alert', {id => $id, email => $email}, 'alert');
>+    }
> 
>+    $cgi->param('profile_loginnames_report', $cgi->param('output'));
>+}
> ###########################################################################
> # Perform vote/keyword cache checks
> ###########################################################################
> 
>-check_votes_or_keywords();
>+if ($cgi->param('keyword_cache')) {
>+    $cgi->param('output', '');
>+
>+    check_votes_or_keywords();
>+
>+    $cgi->param('keyword_cache_report', $cgi->param('output'));
>+}
> 
> sub check_votes_or_keywords {
>     my $check = shift || 'all';
>@@ -786,52 +843,57 @@ sub _check_keywords {
> # Check for flags being in incorrect products and components
> ###########################################################################
> 
>-Status('flag_check_start');
>+if ($cgi->param('flag_check')) {
> 
>-my $invalid_flags = $dbh->selectall_arrayref(
>-       'SELECT DISTINCT flags.id, flags.bug_id, flags.attach_id
>-          FROM flags
>-    INNER JOIN bugs
>-            ON flags.bug_id = bugs.bug_id
>-     LEFT JOIN flaginclusions AS i
>-            ON flags.type_id = i.type_id
>-           AND (bugs.product_id = i.product_id OR i.product_id IS NULL)
>-           AND (bugs.component_id = i.component_id OR i.component_id IS NULL)
>-         WHERE i.type_id IS NULL');
>-
>-my @invalid_flags = @$invalid_flags;
>-
>-$invalid_flags = $dbh->selectall_arrayref(
>-       'SELECT DISTINCT flags.id, flags.bug_id, flags.attach_id
>-          FROM flags
>-    INNER JOIN bugs
>-            ON flags.bug_id = bugs.bug_id
>-    INNER JOIN flagexclusions AS e
>-            ON flags.type_id = e.type_id
>-         WHERE (bugs.product_id = e.product_id OR e.product_id IS NULL)
>-           AND (bugs.component_id = e.component_id OR e.component_id IS NULL)');
>-
>-push(@invalid_flags, @$invalid_flags);
>-
>-if (scalar(@invalid_flags)) {
>-    if ($cgi->param('remove_invalid_flags')) {
>-        Status('flag_deletion_start');
>-        my @flag_ids = map {$_->[0]} @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');
>-    }
>-    else {
>-        foreach my $flag (@$invalid_flags) {
>-            my ($flag_id, $bug_id, $attach_id) = @$flag;
>-            Status('flag_alert',
>-                   {flag_id => $flag_id, attach_id => $attach_id, bug_id => $bug_id},
>-                   'alert');
>+    $cgi->param('output', '');
>+
>+    Status('flag_check_start');
>+
>+    my $invalid_flags = $dbh->selectall_arrayref(
>+           'SELECT DISTINCT flags.id, flags.bug_id, flags.attach_id
>+              FROM flags
>+        INNER JOIN bugs
>+                ON flags.bug_id = bugs.bug_id
>+         LEFT JOIN flaginclusions AS i
>+                ON flags.type_id = i.type_id
>+               AND (bugs.product_id = i.product_id OR i.product_id IS NULL)
>+               AND (bugs.component_id = i.component_id OR i.component_id IS NULL)
>+             WHERE i.type_id IS NULL');
>+
>+    my @invalid_flags = @$invalid_flags;
>+
>+    $invalid_flags = $dbh->selectall_arrayref(
>+           'SELECT DISTINCT flags.id, flags.bug_id, flags.attach_id
>+              FROM flags
>+        INNER JOIN bugs
>+                ON flags.bug_id = bugs.bug_id
>+        INNER JOIN flagexclusions AS e
>+                ON flags.type_id = e.type_id
>+             WHERE (bugs.product_id = e.product_id OR e.product_id IS NULL)
>+               AND (bugs.component_id = e.component_id OR e.component_id IS NULL)');
>+
>+    push(@invalid_flags, @$invalid_flags);
>+
>+    if (scalar(@invalid_flags)) {
>+        if ($cgi->param('remove_invalid_flags')) {
>+            Status('flag_deletion_start');
>+            my @flag_ids = map {$_->[0]} @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');
>+        }
>+        else {
>+            foreach my $flag (@$invalid_flags) {
>+                my ($flag_id, $bug_id, $attach_id) = @$flag;
>+                Status('flag_alert',
>+                    {flag_id => $flag_id, attach_id => $attach_id, bug_id => $bug_id},
>+                    'alert');
>+            }
>+            Status('flag_fix');
>         }
>-        Status('flag_fix');
>     }
>+    $cgi->param('flag_check_report', 'output');
> }
>-
> ###########################################################################
> # General bug checks
> ###########################################################################
>@@ -857,113 +919,134 @@ sub BugCheck {
>     }
> }
> 
>-Status('bug_check_creation_date');
>-
>-BugCheck("bugs WHERE creation_ts IS NULL", 'bug_check_creation_date_error_text',
>-         'repair_creation_date', 'bug_check_creation_date_repair_text');
>+if ($cgi->param('general_bug_check')) {
>+  
>+    $cgi->param('output', '');
>+    
>+    Status('bug_check_creation_date');
> 
>-Status('bug_check_res_dupl');
>+    BugCheck("bugs WHERE creation_ts IS NULL", 'bug_check_creation_date_error_text',
>+             'repair_creation_date', 'bug_check_creation_date_repair_text');
> 
>-BugCheck("bugs INNER JOIN duplicates ON bugs.bug_id = duplicates.dupe " .
>-         "WHERE bugs.resolution != 'DUPLICATE'", 'bug_check_res_dupl_error_text');
>+    Status('bug_check_res_dupl');
> 
>-BugCheck("bugs LEFT JOIN duplicates ON bugs.bug_id = duplicates.dupe WHERE " .
>-         "bugs.resolution = 'DUPLICATE' AND " .
>-         "duplicates.dupe IS NULL", 'bug_check_res_dupl_error_text2');
>+    BugCheck("bugs INNER JOIN duplicates ON bugs.bug_id = duplicates.dupe " .
>+             "WHERE bugs.resolution != 'DUPLICATE'", 'bug_check_res_dupl_error_text');
> 
>-Status('bug_check_status_res');
>+    BugCheck("bugs LEFT JOIN duplicates ON bugs.bug_id = duplicates.dupe WHERE " .
>+             "bugs.resolution = 'DUPLICATE' AND " .
>+             "duplicates.dupe IS NULL", 'bug_check_res_dupl_error_text2');
> 
>-my @open_states = map($dbh->quote($_), BUG_STATE_OPEN);
>-my $open_states = join(', ', @open_states);
>+    Status('bug_check_status_res');
> 
>-BugCheck("bugs WHERE bug_status IN ($open_states) AND resolution != ''",
>-         'bug_check_status_res_error_text');
>-BugCheck("bugs WHERE bug_status NOT IN ($open_states) AND resolution = ''",
>-         'bug_check_status_res_error_text2');
>+    my @open_states = map($dbh->quote($_), BUG_STATE_OPEN);
>+    my $open_states = join(', ', @open_states);
> 
>-Status('bug_check_status_everconfirmed');
>+    BugCheck("bugs WHERE bug_status IN ($open_states) AND resolution != ''",
>+             'bug_check_status_res_error_text');
>+    BugCheck("bugs WHERE bug_status NOT IN ($open_states) AND resolution = ''",
>+             'bug_check_status_res_error_text2');
> 
>-BugCheck("bugs WHERE bug_status = 'UNCONFIRMED' AND everconfirmed = 1",
>-         'bug_check_status_everconfirmed_error_text');
>+    Status('bug_check_status_everconfirmed');
> 
>-my @confirmed_open_states = grep {$_ ne 'UNCONFIRMED'} BUG_STATE_OPEN;
>-my $confirmed_open_states = join(', ', map {$dbh->quote($_)} @confirmed_open_states);
>+    BugCheck("bugs WHERE bug_status = 'UNCONFIRMED' AND everconfirmed = 1",
>+             'bug_check_status_everconfirmed_error_text');
> 
>-BugCheck("bugs WHERE bug_status IN ($confirmed_open_states) AND everconfirmed = 0",
>-         'bug_check_status_everconfirmed_error_text2');
>+    my @confirmed_open_states = grep {$_ ne 'UNCONFIRMED'} BUG_STATE_OPEN;
>+    my $confirmed_open_states = join(', ', map {$dbh->quote($_)} @confirmed_open_states);
> 
>-Status('bug_check_votes_everconfirmed');
>+    BugCheck("bugs WHERE bug_status IN ($confirmed_open_states) AND everconfirmed = 0",
>+             'bug_check_status_everconfirmed_error_text2');
> 
>-BugCheck("bugs INNER JOIN products ON bugs.product_id = products.id " .
>-         "WHERE everconfirmed = 0 AND votestoconfirm <= votes",
>-         'bug_check_votes_everconfirmed_error_text');
>+    Status('bug_check_votes_everconfirmed');
> 
>+    BugCheck("bugs INNER JOIN products ON bugs.product_id = products.id " .
>+             "WHERE everconfirmed = 0 AND votestoconfirm <= votes",
>+             'bug_check_votes_everconfirmed_error_text');
>+    
>+    $cgi->param('general_bug_check_report', $cgi->param('output'));
>+}
> ###########################################################################
> # Control Values
> ###########################################################################
> 
>-# Checks for values that are invalid OR
>-# not among the 9 valid combinations
>-Status('bug_check_control_values');
>-my $groups = join(", ", (CONTROLMAPNA, CONTROLMAPSHOWN, CONTROLMAPDEFAULT,
>-CONTROLMAPMANDATORY));
>-my $query = qq{
>-     SELECT COUNT(product_id) 
>-       FROM group_control_map 
>-      WHERE membercontrol NOT IN( $groups )
>-         OR othercontrol NOT IN( $groups )
>-         OR ((membercontrol != othercontrol)
>-             AND (membercontrol != } . CONTROLMAPSHOWN . q{)
>-             AND ((membercontrol != } . CONTROLMAPDEFAULT . q{)
>-                  OR (othercontrol = } . CONTROLMAPSHOWN . q{)))};
>-
>-my $entries = $dbh->selectrow_array($query);
>-Status('bug_check_control_values_alert', {entries => $entries}, 'alert') if $entries;
>-
>-Status('bug_check_control_values_violation');
>-BugCheck("bugs
>-         INNER JOIN bug_group_map
>-            ON bugs.bug_id = bug_group_map.bug_id
>-          LEFT JOIN group_control_map
>-            ON bugs.product_id = group_control_map.product_id
>-           AND bug_group_map.group_id = group_control_map.group_id
>-         WHERE ((group_control_map.membercontrol = " . CONTROLMAPNA . ")
>-         OR (group_control_map.membercontrol IS NULL))",
>-         'bug_check_control_values_error_text',
>-         'createmissinggroupcontrolmapentries',
>-         'bug_check_control_values_repair_text');
>-
>-BugCheck("bugs
>-         INNER JOIN group_control_map
>-            ON bugs.product_id = group_control_map.product_id
>-         INNER JOIN groups
>-            ON group_control_map.group_id = groups.id
>-          LEFT JOIN bug_group_map
>-            ON bugs.bug_id = bug_group_map.bug_id
>-           AND group_control_map.group_id = bug_group_map.group_id
>-         WHERE group_control_map.membercontrol = " . CONTROLMAPMANDATORY . "
>-           AND bug_group_map.group_id IS NULL
>-           AND groups.isactive != 0",
>-         'bug_check_control_values_error_text2');
>+if ($cgi->param('control_values')) {
>+
>+    $cgi->param('output', '');
>+
>+    # Checks for values that are invalid OR
>+    # not among the 9 valid combinations
>+    Status('bug_check_control_values');
>+    my $groups = join(", ", (CONTROLMAPNA, CONTROLMAPSHOWN, CONTROLMAPDEFAULT,
>+    CONTROLMAPMANDATORY));
>+    my $query = qq{
>+         SELECT COUNT(product_id) 
>+           FROM group_control_map 
>+          WHERE membercontrol NOT IN( $groups )
>+             OR othercontrol NOT IN( $groups )
>+             OR ((membercontrol != othercontrol)
>+                 AND (membercontrol != } . CONTROLMAPSHOWN . q{)
>+                 AND ((membercontrol != } . CONTROLMAPDEFAULT . q{)
>+                      OR (othercontrol = } . CONTROLMAPSHOWN . q{)))};
>+
>+    my $entries = $dbh->selectrow_array($query);
>+    Status('bug_check_control_values_alert', {entries => $entries}, 'alert') if $entries;
>+
>+    Status('bug_check_control_values_violation');
>+    BugCheck("bugs
>+             INNER JOIN bug_group_map
>+                ON bugs.bug_id = bug_group_map.bug_id
>+              LEFT JOIN group_control_map
>+                ON bugs.product_id = group_control_map.product_id
>+               AND bug_group_map.group_id = group_control_map.group_id
>+             WHERE ((group_control_map.membercontrol = " . CONTROLMAPNA . ")
>+             OR (group_control_map.membercontrol IS NULL))",
>+             'bug_check_control_values_error_text',
>+             'createmissinggroupcontrolmapentries',
>+             'bug_check_control_values_repair_text');
>+
>+    BugCheck("bugs
>+             INNER JOIN group_control_map
>+                ON bugs.product_id = group_control_map.product_id
>+             INNER JOIN groups
>+                ON group_control_map.group_id = groups.id
>+              LEFT JOIN bug_group_map
>+                ON bugs.bug_id = bug_group_map.bug_id
>+               AND group_control_map.group_id = bug_group_map.group_id
>+             WHERE group_control_map.membercontrol = " . CONTROLMAPMANDATORY . "
>+               AND bug_group_map.group_id IS NULL
>+               AND groups.isactive != 0",
>+             'bug_check_control_values_error_text2');
>+
>+    $cgi->param('control_values_report', $cgi->param('output'));
>+}
> 
> ###########################################################################
> # Unsent mail
> ###########################################################################
> 
>-Status('unsent_bugmail_check');
>+if ($cgi->param('unsent_emails')) {
>+
>+    $cgi->param('output' , '');
>+
>+    Status('unsent_bugmail_check');
> 
>-my $time = $dbh->sql_interval(30, 'MINUTE');
>-my $badbugs = $dbh->selectcol_arrayref(qq{
>-                    SELECT bug_id 
>-                      FROM bugs 
>-                     WHERE (lastdiffed IS NULL OR lastdiffed < delta_ts)
>-                       AND delta_ts < now() - $time
>-                  ORDER BY bug_id});
>+    my $time = $dbh->sql_interval(30, 'MINUTE');
>+    my $badbugs = $dbh->selectcol_arrayref(qq{
>+                        SELECT bug_id 
>+                          FROM bugs 
>+                         WHERE (lastdiffed IS NULL OR lastdiffed < delta_ts)
>+                           AND delta_ts < now() - $time
>+                      ORDER BY bug_id});
>+
>+
>+    if (scalar(@$badbugs > 0)) {
>+        Status('unsent_bugmail_alert', {badbugs => $badbugs}, 'alert');
>+        Status('unsent_bugmail_fix');
>+    }
> 
>+    $cgi->param('unsent_emails_report', $cgi->param('output'));
> 
>-if (scalar(@$badbugs > 0)) {
>-    Status('unsent_bugmail_alert', {badbugs => $badbugs}, 'alert');
>-    Status('unsent_bugmail_fix');
> }
> 
> ###########################################################################
>Index: template/en/default/filterexceptions.pl
>===================================================================
>RCS file: /cvs/qa/rh_bugzilla_3/template/en/default/filterexceptions.pl,v
>retrieving revision 1.4
>diff -p -u -r1.4 filterexceptions.pl
>--- template/en/default/filterexceptions.pl	31 Mar 2008 18:35:32 -0000	1.4
>+++ template/en/default/filterexceptions.pl	14 Apr 2008 13:13:22 -0000
>@@ -557,6 +557,10 @@
>   'new_status.id',
> ],
> 
>+'admin/sanitycheck/messages.html.tmpl' => [
>+  'query',
>+],
>+
> 'account/login.html.tmpl' => [
>   'target', 
> ],
>Index: template/en/default/admin/sanitycheck/messages.html.tmpl
>===================================================================
>RCS file: /cvs/qa/rh_bugzilla_3/template/en/default/admin/sanitycheck/messages.html.tmpl,v
>retrieving revision 1.3
>diff -p -u -r1.3 messages.html.tmpl
>--- template/en/default/admin/sanitycheck/messages.html.tmpl	27 Feb 2008 20:50:44 -0000	1.3
>+++ template/en/default/admin/sanitycheck/messages.html.tmpl	14 Apr 2008 13:13:38 -0000
>@@ -34,7 +34,8 @@
>     [% errortext FILTER html %]: [% INCLUDE bug_list badbugs = badbugs %]
> 
>   [% ELSIF san_tag == "bug_check_repair" %]
>-    <a href="sanitycheck.cgi?[% param FILTER url_quote %]=1">[% text FILTER html %]</a>.
>+    <a href="sanitycheck.cgi?[% param FILTER url_quote %]=1&[% query %]">[%
>+text FILTER html %]</a>.
> 
>   [% ELSIF san_tag == "bug_check_creation_date" %]
>     Checking for [% terms.bugs %] with no creation date (which makes them invisible).
>@@ -127,11 +128,11 @@
>     [% END %]
> 
>   [% ELSIF san_tag == "cross_check_attachment_has_references" %]
>-    <a href="sanitycheck.cgi?remove_invalid_attach_references=1">Remove
>+    <a href="sanitycheck.cgi?remove_invalid_attach_references=1&[% query %]">Remove
>     invalid references to non existent attachments.</a>
> 
>   [% ELSIF san_tag == "cross_check_bug_has_references" %]
>-    <a href="sanitycheck.cgi?remove_invalid_bug_references=1">Remove
>+    <a href="sanitycheck.cgi?remove_invalid_bug_references=1&[% query %]">Remove
>     invalid references to non existent [% terms.bugs %].</a>
> 
>   [% ELSIF san_tag == "double_cross_check_to" %]
>@@ -171,7 +172,7 @@
>     [%+ PROCESS bug_link bug_id = bug_id %].
> 
>   [% ELSIF san_tag == "flag_fix" %]
>-    <a href="sanitycheck.cgi?remove_invalid_flags=1">Click
>+    <a href="sanitycheck.cgi?remove_invalid_flags=1&[% query %]">Click
>     here to delete invalid flags</a>
> 
>   [% ELSIF san_tag == "group_control_map_entries_creation" %]
>@@ -221,7 +222,7 @@
>     Keyword cache fixed.
> 
>   [% ELSIF san_tag == "keyword_cache_rebuild" %]
>-    <a href="sanitycheck.cgi?rebuildkeywordcache=1">Click here to
>+    <a href="sanitycheck.cgi?rebuildkeywordcache=1&[% query %]">Click here to
>     rebuild the keyword cache</a>.
> 
>   [% ELSIF san_tag == "profile_login_start" %]
>@@ -252,7 +253,7 @@
>     half an hour: [% INCLUDE bug_list badbugs = badbugs %]
> 
>   [% ELSIF san_tag == "unsent_bugmail_fix" %]
>-    <a href="sanitycheck.cgi?rescanallBugMail=1">Send these mails</a>.
>+    <a href="sanitycheck.cgi?rescanallBugMail=1&[% query %]">Send these mails</a>.
> 
>   [% ELSIF san_tag == "vote_cache_rebuild_start" %]
>     OK, now rebuilding vote cache.
>@@ -261,7 +262,7 @@
>     Vote cache has been rebuilt.
> 
>   [% ELSIF san_tag == "vote_cache_rebuild_fix" %]
>-    <a href="sanitycheck.cgi?rebuildvotecache=1">Click here to
>+    <a href="sanitycheck.cgi?rebuildvotecache=1&[% query %]">Click here to
>     rebuild the vote cache</a>
> 
>   [% ELSIF san_tag == "vote_cache_alert" %]
Comment 17 Noura El hawary 2008-04-15 02:49:29 EDT
Created attachment 302410 [details]
sanitycheck patch

as agreed in the meeting ,, I added the extra check for bugs_components,
attaching new patch that has it.

Noura
Comment 18 David Lawrence 2008-04-15 12:17:19 EDT
Comment on attachment 302410 [details]
sanitycheck patch

Looks good Noura. Testing worked well. Please check in.

Dave
Comment 19 Noura El hawary 2008-04-16 00:49:23 EDT
(In reply to comment #18)
> (From update of attachment 302410 [details] [edit])
> Looks good Noura. Testing worked well. Please check in.
> 
> Dave
> 

Thanks Dave.. Committed to cvs.

Noura
Comment 20 Kevin Baker 2008-04-16 17:11:56 EDT
Looks ok. The only piece of trivia I can add is to use HERE docs instead of 
appending strings together

For example, instead of this

+sub create_report {
+    my ($check, $rpt) = @_;
+
+    my $check_rpt =
+        "Check: $check\n===================================="
+        . "===============================\n$rpt\n";
+

Do this

    my $check_rpt = <<CHECK_RPT
Check: $check
===================================================================
$rpt
CHECK_RPT

IMHO it's easier to look at that in the code and visualise how it will look on 
the final output. YMMV

Not asking you to fix it now, just for next time.
Comment 21 Kevin Baker 2008-04-16 17:13:09 EDT
I cleaned up the perldoc in sanitycheck.pl. The only question I have is what 
is the meaning of "more output" with the --verbose flag? I didn't see a good 
explanation of what it might be. Not a big deal
Comment 22 Noura El hawary 2008-04-17 21:53:01 EDT
(In reply to comment #21)
> I cleaned up the perldoc in sanitycheck.pl. 
Thanks Kevin.

>The only question I have is what 
> is the meaning of "more output" with the --verbose flag? I didn't see a good 
> explanation of what it might be. Not a big deal
> 
I have added clearer explanation to the documentation. so basically if we don't
have the --verbose option then only checks that produced errors will be in the
output but with the option then the output will include also the comments for
the checks that passed along with the ones that produced errors.

Noura


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