Bug 254498: Check for comment required for time validation was too late.

Patch by Tiago R. Mello <tiago@async.com.br>
r=kiko, a=justdave

1 files changed, 36 insertions, 32 deletions

diff --git a/process_bug.cgi b/process_bug.cgi
index 2810d3b39..9e47d8f98 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -100,39 +100,15 @@ foreach my $field ("estimated_time", "work_time", "remaining_time") {
     }
 }
 
-# do a match on the fields if applicable
-
-# The order of these function calls is important, as both Flag::validate
-# and FlagType::validate assume User::match_field has ensured that the values
-# in the requestee fields are legitimate user email addresses.
-&Bugzilla::User::match_field({
-    'qa_contact'                => { 'type' => 'single' },
-    'newcc'                     => { 'type' => 'multi'  },
-    'masscc'                    => { 'type' => 'multi'  },
-    'assigned_to'               => { 'type' => 'single' },
-    '^requestee(_type)?-(\d+)$' => { 'type' => 'single' },
-});
-# Validate flags, but only if the user is changing a single bug,
-# since the multi-change form doesn't include flag changes.
-if (defined $::FORM{'id'}) {
-    Bugzilla::Flag::validate(\%::FORM, $::FORM{'id'});
-    Bugzilla::FlagType::validate(\%::FORM, $::FORM{'id'});
-}
-
-# If we are duping bugs, let's also make sure that we can change 
-# the original.  This takes care of issue A on bug 96085.
-if (defined $::FORM{'dup_id'} && $::FORM{'knob'} eq "duplicate") {
-    ValidateBugID($::FORM{'dup_id'});
-
-    # Also, let's see if the reporter has authorization to see the bug
-    # to which we are duping.  If not we need to prompt.
-    DuplicateUserConfirm();
+if (UserInGroup(Param('timetrackinggroup'))) {
+    my $wk_time = $::FORM{'work_time'};
+    if ($::FORM{'comment'} =~ /^\s*$/ && $wk_time && $wk_time != 0) {
+        ThrowUserError('comment_required', undef, "abort");
+    }
 }
 
 ValidateComment($::FORM{'comment'});
 
-$::FORM{'dontchange'} = '' unless exists $::FORM{'dontchange'};
-
 # If the bug(s) being modified have dependencies, validate them
 # and rebuild the list with the validated values.  This is important
 # because there are situations where validation changes the value
@@ -151,6 +127,37 @@ foreach my $field ("dependson", "blocked") {
     }
 }
 
+# If we are duping bugs, let's also make sure that we can change 
+# the original.  This takes care of issue A on bug 96085.
+if (defined $::FORM{'dup_id'} && $::FORM{'knob'} eq "duplicate") {
+    ValidateBugID($::FORM{'dup_id'});
+
+    # Also, let's see if the reporter has authorization to see the bug
+    # to which we are duping.  If not we need to prompt.
+    DuplicateUserConfirm();
+}
+
+# do a match on the fields if applicable
+
+# The order of these function calls is important, as both Flag::validate
+# and FlagType::validate assume User::match_field has ensured that the values
+# in the requestee fields are legitimate user email addresses.
+&Bugzilla::User::match_field({
+    'qa_contact'                => { 'type' => 'single' },
+    'newcc'                     => { 'type' => 'multi'  },
+    'masscc'                    => { 'type' => 'multi'  },
+    'assigned_to'               => { 'type' => 'single' },
+    '^requestee(_type)?-(\d+)$' => { 'type' => 'single' },
+});
+# Validate flags, but only if the user is changing a single bug,
+# since the multi-change form doesn't include flag changes.
+if (defined $::FORM{'id'}) {
+    Bugzilla::Flag::validate(\%::FORM, $::FORM{'id'});
+    Bugzilla::FlagType::validate(\%::FORM, $::FORM{'id'});
+}
+
+$::FORM{'dontchange'} = '' unless exists $::FORM{'dontchange'};
+
 ######################################################################
 # End Data/Security Validation
 ######################################################################
@@ -1290,9 +1297,6 @@ foreach my $id (@idlist) {
     if (UserInGroup(Param('timetrackinggroup'))) {
         $work_time = $::FORM{'work_time'};
         if ($work_time) {
-            if (!defined $::FORM{'comment'} || $::FORM{'comment'} =~ /^\s*$/) {
-                ThrowUserError('comment_required', undef, "abort");
-            }
             # AppendComment (called below) can in theory raise an error,
             # but because we've already validated work_time here it's
             # safe to log the entry before adding the comment.