Bug 617477: Fix numerous consistency and behavior issues surroudning Bug.update

and Bugzilla::Bug. See https://bugzilla.mozilla.org/show_bug.cgi?id=617477#c2
for details.
r=LpSolit, a=LpSolit

1 files changed, 42 insertions, 14 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index f5b092800..9d41d5c7e 100644
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -130,7 +130,7 @@ sub VALIDATORS {
         creation_ts    => \&_check_creation_ts,
         deadline       => \&_check_deadline,
         dup_id         => \&_check_dup_id,
-        estimated_time => \&Bugzilla::Object::check_time,
+        estimated_time => \&_check_time_field,
         everconfirmed  => \&Bugzilla::Object::check_boolean,
         groups         => \&_check_groups,
         keywords       => \&_check_keywords,
@@ -138,7 +138,7 @@ sub VALIDATORS {
         priority       => \&_check_priority,
         product        => \&_check_product,
         qa_contact     => \&_check_qa_contact,
-        remaining_time => \&Bugzilla::Object::check_time,
+        remaining_time => \&_check_time_field,
         rep_platform   => \&_check_select_field,
         resolution     => \&_check_resolution,
         short_desc     => \&_check_short_desc,
@@ -1456,12 +1456,14 @@ sub _check_creation_ts {
 
 sub _check_deadline {
     my ($invocant, $date) = @_;
-    
-    # Check time-tracking permissions.
-    # deadline() returns '' instead of undef if no deadline is set.
-    my $current = ref $invocant ? ($invocant->deadline || undef) : undef;
-    return $current unless Bugzilla->user->is_timetracker;
-    
+
+    # When filing bugs, we're forgiving and just return undef if
+    # the user isn't a timetracker. When updating bugs, check_can_change_field
+    # controls permissions, so we don't want to check them here.
+    if (!ref $invocant and !Bugzilla->user->is_timetracker) {
+        return undef;
+    }
+
     # Validate entered deadline
     $date = trim($date);
     return undef if !$date;
@@ -1787,6 +1789,10 @@ sub _check_short_desc {
     if (!defined $short_desc || $short_desc eq '') {
         ThrowUserError("require_summary");
     }
+    if (length($short_desc) > MAX_FREETEXT_LENGTH) {
+        ThrowUserError('freetext_too_long', 
+                       { field => 'short_desc', text => $short_desc });
+    }
     return $short_desc;
 }
 
@@ -1876,6 +1882,20 @@ sub _check_target_milestone {
     return $object->name;
 }
 
+sub _check_time_field {
+    my ($invocant, $value, $field, $params) = @_;
+
+    # When filing bugs, we're forgiving and just return 0 if
+    # the user isn't a timetracker. When updating bugs, check_can_change_field
+    # controls permissions, so we don't want to check them here.
+    if (!ref $invocant and !Bugzilla->user->is_timetracker) {
+        return 0;
+    }
+
+    # check_time is in Bugzilla::Object.
+    return $invocant->check_time($value, $field, $params);
+}
+
 sub _check_version {
     my ($invocant, $version, undef, $params) = @_;
     $version = trim($version);
@@ -1940,11 +1960,12 @@ sub _check_datetime_field {
 sub _check_default_field { return defined $_[1] ? trim($_[1]) : ''; }
 
 sub _check_freetext_field {
-    my ($invocant, $text) = @_;
+    my ($invocant, $text, $field) = @_;
 
     $text = (defined $text) ? trim($text) : '';
     if (length($text) > MAX_FREETEXT_LENGTH) {
-        ThrowUserError('freetext_too_long', { text => $text });
+        ThrowUserError('freetext_too_long', 
+                       { field => $field, text => $text });
     }
     return $text;
 }
@@ -2221,7 +2242,6 @@ sub reset_assigned_to {
 sub set_cclist_accessible { $_[0]->set('cclist_accessible', $_[1]); }
 sub set_comment_is_private {
     my ($self, $comment_id, $isprivate) = @_;
-    return unless Bugzilla->user->is_insider;
 
     # We also allow people to pass in a hash of comment ids to update.
     if (ref $comment_id) {
@@ -2237,6 +2257,7 @@ sub set_comment_is_private {
 
     $isprivate = $isprivate ? 1 : 0;
     if ($isprivate != $comment->is_private) {
+        ThrowUserError('user_not_insider') if !Bugzilla->user->is_insider;
         $self->{comment_isprivate} ||= [];
         $comment->set_is_private($isprivate);
         push @{$self->{comment_isprivate}}, $comment;
@@ -2561,7 +2582,13 @@ sub set_bug_status {
     else {
         # We do this here so that we can make sure closed statuses have
         # resolutions.
-        my $resolution = delete $params->{resolution} || $self->resolution;
+        my $resolution = $self->resolution;
+        # We need to check "defined" to prevent people from passing
+        # a blank resolution in the WebService, which would otherwise fail
+        # silently.
+        if (defined $params->{resolution}) {
+            $resolution = delete $params->{resolution};
+        }
         $self->set_resolution($resolution, $params);
 
         # Changing between closed statuses zeros the remaining time.
@@ -3810,7 +3837,8 @@ sub check_can_change_field {
     } elsif (trim($oldvalue) eq trim($newvalue)) {
         return 1;
     # numeric fields need to be compared using ==
-    } elsif (($field eq 'estimated_time' || $field eq 'remaining_time')
+    } elsif (($field eq 'estimated_time' || $field eq 'remaining_time' 
+              || $field eq 'work_time')
              && $oldvalue == $newvalue)
     {
         return 1;
@@ -3845,7 +3873,7 @@ sub check_can_change_field {
     # $PrivilegesRequired = PRIVILEGES_REQUIRED_EMPOWERED : an empowered user.
     
     # Only users in the time-tracking group can change time-tracking fields.
-    if ( grep($_ eq $field, qw(deadline estimated_time remaining_time)) ) {
+    if ( grep($_ eq $field, TIMETRACKING_FIELDS) ) {
         if (!$user->is_timetracker) {
             $$PrivilegesRequired = PRIVILEGES_REQUIRED_EMPOWERED;
             return 0;