]> git.ipfire.org Git - thirdparty/bugzilla.git/commitdiff
Bug 617477: Fix numerous consistency and behavior issues surrounding Bug.update
authorMax Kanat-Alexander <mkanat@bugzilla.org>
Mon, 13 Dec 2010 20:56:46 +0000 (12:56 -0800)
committerMax Kanat-Alexander <mkanat@bugzilla.org>
Mon, 13 Dec 2010 20:56:46 +0000 (12:56 -0800)
and Bugzilla::Bug. See https://bugzilla.mozilla.org/show_bug.cgi?id=617477#c2
for details.
r=LpSolit, a=LpSolit

Bugzilla/Bug.pm
Bugzilla/WebService/Bug.pm
Bugzilla/WebService/Constants.pm
Bugzilla/WebService/Server.pm
template/en/default/global/user-error.html.tmpl

index 18a70d0e5f1d207baef9f33d8ee1e000f8ebdc0c..ceb773aec7f8741d0d35c386c22d89cda8a83dc1 100644 (file)
@@ -1436,12 +1436,14 @@ sub _check_component {
 
 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;
@@ -1644,8 +1646,7 @@ sub _check_keywords {
     my %keywords;
     foreach my $keyword (@$keyword_array) {
         next unless $keyword;
-        my $obj = new Bugzilla::Keyword({ name => $keyword });
-        ThrowUserError("unknown_keyword", { keyword => $keyword }) if !$obj;
+        my $obj = Bugzilla::Keyword->check($keyword);
         $keywords{$obj->id} = $obj;
     }
     return [values %keywords];
@@ -1776,6 +1777,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;
 }
 
@@ -1868,11 +1873,12 @@ sub _check_target_milestone {
 sub _check_time {
     my ($invocant, $time, $field) = @_;
 
-    my $current = 0;
-    if (ref $invocant && $field ne 'work_time') {
-        $current = $invocant->$field;
+    # 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;
     }
-    return $current unless Bugzilla->user->is_timetracker;
     
     $time = trim($time) || 0;
     ValidateTime($time, $field);
@@ -1890,7 +1896,15 @@ sub _check_version {
 }
 
 sub _check_work_time {
-    return $_[0]->_check_time($_[1], 'work_time');
+    my ($self, $input) = @_;
+    my $time = $self->_check_time($input, 'work_time');
+    # Because work_time isn't set by a set_ function, we need to
+    # do check_can_change_field here manually.
+    my $privs;
+    $self->check_can_change_field('work_time', 0, $time, \$privs)
+        || ThrowUserError('illegal_change', 
+                          { field => 'work_time', privs => $privs });
+    return $time;
 }
 
 # Custom Field Validators
@@ -1947,11 +1961,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;
 }
@@ -2228,7 +2243,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) {
@@ -2244,6 +2258,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} ||= {};
         $self->{comment_isprivate}->{$comment_id} = $isprivate;
     }
@@ -2567,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.
@@ -3783,7 +3804,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;
@@ -3818,7 +3840,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;
index 53e3332556ecd4a1fa5c949d98714120d02e3fd4..ffc2e138cd7442de0894e58258a28b590155a88d 100644 (file)
@@ -47,7 +47,6 @@ use constant PRODUCT_SPECIFIC_FIELDS => qw(version target_milestone component);
 use constant DATE_FIELDS => {
     comments => ['new_since'],
     search   => ['last_change_time', 'creation_time'],
-    update   => ['deadline'],
 };
 
 use constant BASE64_FIELDS => {
@@ -464,7 +463,10 @@ sub update {
     my $user = Bugzilla->login(LOGIN_REQUIRED);
     my $dbh = Bugzilla->dbh;
 
-    $params = Bugzilla::Bug::map_fields($params, { summary => 1 });
+    # We skip certain fields because their set_ methods actually use
+    # the external names instead of the internal names.
+    $params = Bugzilla::Bug::map_fields($params, 
+        { summary => 1, platform => 1, severity => 1, url => 1 });
 
     my $ids = delete $params->{ids};
     defined $ids || ThrowCodeError('param_required', { param => 'ids' });
@@ -533,6 +535,11 @@ sub update {
         foreach my $field (keys %changes) {
             my $change = $changes{$field};
             my $api_field = $api_name{$field} || $field;
+            # We normalize undef to an empty string, so that the API
+            # stays consistent for things like Deadline that can become
+            # empty.
+            $change->[0] = '' if !defined $change->[0];
+            $change->[1] = '' if !defined $change->[1];
             $hash{changes}->{$api_field} = {
                 removed => $self->type('string', $change->[0]),
                 added   => $self->type('string', $change->[1]) 
@@ -886,7 +893,9 @@ sub _bug_to_hash {
     if (Bugzilla->user->is_timetracker) {
         $item{'estimated_time'} = $self->type('double', $bug->estimated_time);
         $item{'remaining_time'} = $self->type('double', $bug->remaining_time);
-        $item{'deadline'}       = $self->type('dateTime', $bug->deadline);
+        # No need to format $bug->deadline specially, because Bugzilla::Bug
+        # already does it for us.
+        $item{'deadline'} = $self->type('string', $bug->deadline);
     }
 
     if (Bugzilla->user->id) {
@@ -1616,7 +1625,8 @@ C<string> The login name of the person who filed this bug (the reporter).
 
 =item C<deadline>
 
-C<dateTime> The day that this bug is due to be completed.
+C<string> The day that this bug is due to be completed, in the format
+C<YYYY-MM-DD>.
 
 If you are not in the time-tracking group, this field will not be included
 in the return value.
@@ -2284,7 +2294,8 @@ A hash with one element, C<id>. This is the id of the newly-filed bug.
 
 =item 51 (Invalid Object)
 
-The component you specified is not valid for this Product.
+You specified a field value that is invalid. The error message will have
+more details.
 
 =item 103 (Invalid Alias)
 
@@ -2309,6 +2320,11 @@ you don't have permission to enter bugs in this product.
 
 You didn't specify a summary for the bug.
 
+=item 116 (Dependency Loop)
+
+You specified values in the C<blocks> or C<depends_on> fields
+that would cause a circular dependency between bugs.
+
 =item 504 (Invalid User)
 
 Either the QA Contact, Assignee, or CC lists have some invalid user
@@ -2331,6 +2347,9 @@ method.
 Before Bugzilla 4.0, you had to use the undocumented C<commentprivacy>
 argument.
 
+=item Error 116 was added in Bugzilla B<4.0>. Before that, dependency
+loop errors had a generic code of C<32000>.
+
 =back
 
 =back
@@ -2502,7 +2521,7 @@ doesn't support aliases or (b) there is no bug with that alias.
 
 The id you specified doesn't exist in the database.
 
-=item 108 (Bug Edit Denied)
+=item 109 (Bug Edit Denied)
 
 You did not have the necessary rights to edit the bug.
 
@@ -2654,9 +2673,8 @@ C<string> The Component the bug is in.
 
 =item C<deadline>
 
-C<dateTime> The Deadline field--a date specifying when the bug must
-be completed by. The time specified is ignored--only the date is
-significant.
+C<string> The Deadline field--a date specifying when the bug must
+be completed by, in the format C<YYYY-MM-DD>.
 
 =item C<dupe_of>
 
@@ -2905,21 +2923,91 @@ Here's an example of what a return value might look like:
    ]
  }
 
+Currently, some fields are not tracked in changes: C<comment>,
+C<comment_is_private>, and C<work_time>. This means that they will not
+show up in the return value even if they were successfully updated.
+This may change in a future version of Bugzilla.
+
 =item B<Errors>
 
-This function can throw all of the errors that L</get> can throw, plus:
+This function can throw all of the errors that L</get>, L</create>,
+and L</add_comment> can throw, plus:
 
 =over
 
-=item 103 (Invalid Alias)
+=item 50 (Empty Field)
 
-Either you tried to set an alias when changing multiple bugs at once,
-or the alias you specified is invalid for some reason.
+You tried to set some field to be empty, but that field cannot be empty.
+The error message will have more details.
 
-=back
+=item 52 (Input Not A Number)
+
+You tried to set a numeric field to a value that wasn't numeric.
+
+=item 54 (Number Too Large)
+
+You tried to set a numeric field to a value larger than that field can
+accept.
+
+=item 55 (Number Too Small)
+
+You tried to set a negative value in a numeric field that does not accept
+negative values.
+
+=item 56 (Bad Date/Time)
+
+You specified an invalid date or time in a date/time field (such as
+the C<deadline> field or a custom date/time field).
+
+=item 112 (See Also Invalid)
+
+You attempted to add an invalid value to the C<see_also> field.
+
+=item 115 (Permission Denied)
+
+You don't have permission to change a particular field to a particular value.
+The error message will have more detail.
 
-FIXME: Plus a whole load of other errors that we haven't documented yet,
-which we won't even know about until after we do QA for 4.0.
+=item 116 (Dependency Loop)
+
+You specified a value in the C<blocks> or C<depends_on> fields that causes
+a dependency loop.
+
+=item 117 (Invalid Comment ID)
+
+You specified a comment id in C<comment_is_private> that isn't on this bug.
+
+=item 118 (Duplicate Loop)
+
+You specified a value for C<dupe_of> that causes an infinite loop of
+duplicates.
+
+=item 119 (dupe_of Required)
+
+You changed the resolution to C<DUPLICATE> but did not specify a value
+for the C<dupe_of> field.
+
+=item 120 (Group Add/Remove Denied)
+
+You tried to add or remove a group that you don't have permission to modify
+for this bug, or you tried to add a group that isn't valid in this product.
+
+=item 121 (Resolution Required)
+
+You tried to set the C<status> field to a closed status, but you didn't
+specify a resolution.
+
+=item 122 (Resolution On Open Status)
+
+This bug has an open status, but you specified a value for the C<resolution>
+field.
+
+=item 123 (Invalid Status Transition)
+
+You tried to change from one status to another, but the status workflow
+rules don't allow that change.
+
+=back
 
 =item B<History>
 
@@ -3017,7 +3105,7 @@ This method can throw all of the errors that L</get> throws, plus:
 
 =over
 
-=item 108 (Bug Edit Denied)
+=item 109 (Bug Edit Denied)
 
 You did not have the necessary rights to edit the bug.
 
index 383f148f530c0cd5dff16843bd3e0472f67516f0..8fbc7c09794fbc20fd2358e45e0c547211cae235 100644 (file)
@@ -49,13 +49,17 @@ our @EXPORT = qw(
 use constant WS_ERROR_CODE => {
     # Generic errors (Bugzilla::Object and others) are 50-99.    
     object_not_specified        => 50,
+    reassign_to_empty           => 50,
     param_required              => 50,
     params_required             => 50,
+    undefined_field             => 50,
     object_does_not_exist       => 51,
     param_must_be_numeric       => 52,
     number_not_numeric          => 52,
     param_invalid               => 53,
     number_too_large            => 54,
+    number_too_small            => 55,
+    illegal_date                => 56,
     # Bug errors usually occupy the 100-200 range.
     improper_bug_id_field_value => 100,
     bug_id_does_not_exist       => 101,
@@ -88,6 +92,7 @@ use constant WS_ERROR_CODE => {
     comment_is_private => 110,
     comment_id_invalid => 111,
     comment_too_long => 114,
+    comment_invalid_isprivate => 117, 
     # See Also errors
     bug_url_invalid => 112,
     bug_url_too_long => 112,
@@ -96,6 +101,20 @@ use constant WS_ERROR_CODE => {
     # Note: 114 is above in the Comment-related section.
     # Bug update errors
     illegal_change => 115,
+    # Dependency errors
+    dependency_loop_single => 116,
+    dependency_loop_multi  => 116,
+    # Note: 117 is above in the Comment-related section.
+    # Dup errors
+    dupe_loop_detected => 118,
+    dupe_id_required => 119,
+    # Group errors
+    group_change_denied => 120,
+    group_invalid_restriction => 120,
+    # Status/Resolution errors
+    missing_resolution => 121,
+    resolution_not_allowed => 122,
+    illegal_bug_status_transition => 123,
 
     # Authentication errors are usually 300-400.
     invalid_username_or_password => 300,
index f4b3600d4e0d1e2b5f8a745d9762a1fe95cd1a72..4e03152196563e1c661e07b2002ef6d1a060f0d8 100644 (file)
@@ -36,6 +36,9 @@ sub datetime_format_inbound {
     my ($self, $time) = @_;
     
     my $converted = datetime_from($time, Bugzilla->local_timezone);
+    if (!defined $converted) {
+        ThrowUserError('illegal_date', { date => $time });
+    }
     $time = $converted->ymd() . ' ' . $converted->hms();
     return $time
 }
index 83f51b5553d8a16632fa737bf8461149012773f2..bd9fb0a5b1ffd5040e8b50221d9809a3abb2554f 100644 (file)
 
   [% ELSIF error == "freetext_too_long" %]
     [% title = "Text Too Long" %]
-    The text you entered is too long ([% text.length FILTER html %] characters,
-    above the maximum length allowed of [% constants.MAX_FREETEXT_LENGTH FILTER none %]
-    characters):
-    <p><em>[% text FILTER html %]</em></p>
+    The text you entered in the [% field_descs.$field FILTER html %]
+    field is too long ([% text.length FILTER html %] characters,
+    above the maximum length allowed of
+    [%+ constants.MAX_FREETEXT_LENGTH FILTER none %] characters).
 
   [% ELSIF error == "group_cannot_delete" %]
     [% title = "Cannot Delete Group" %]
       Either you mis-typed the name or that user has not yet registered
       for a [% terms.Bugzilla %] account.
     [% END %]
+    [% IF class == "Bugzilla::Keyword" %]
+      The legal keyword names are <a href="describekeywords.cgi">listed
+      here</a>.
+    [% END %]
 
   [% ELSIF error == "old_password_incorrect" %]
     [% title = "Incorrect Old Password" %]