aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorMax Kanat-Alexander <mkanat@bugzilla.org>2010-12-13 12:54:20 -0800
committerMax Kanat-Alexander <mkanat@bugzilla.org>2010-12-13 12:54:20 -0800
commitc93887f249fa25405aad68c56995cdcd2efc1e91 (patch)
tree600cca1f4b7966bfd947f18e1d880aac44f796a8
parent9fe88ea66a28168e940bf02038d4055a5e00a4ee (diff)
downloadbugs-c93887f249fa25405aad68c56995cdcd2efc1e91.tar
bugs-c93887f249fa25405aad68c56995cdcd2efc1e91.tar.gz
bugs-c93887f249fa25405aad68c56995cdcd2efc1e91.tar.bz2
bugs-c93887f249fa25405aad68c56995cdcd2efc1e91.tar.xz
bugs-c93887f249fa25405aad68c56995cdcd2efc1e91.zip
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
-rw-r--r--Bugzilla/Bug.pm56
-rw-r--r--Bugzilla/Comment.pm22
-rw-r--r--Bugzilla/Object.pm3
-rw-r--r--Bugzilla/WebService/Bug.pm122
-rw-r--r--Bugzilla/WebService/Constants.pm19
-rw-r--r--Bugzilla/WebService/Server.pm3
-rw-r--r--template/en/default/global/user-error.html.tmpl8
7 files changed, 192 insertions, 41 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;
diff --git a/Bugzilla/Comment.pm b/Bugzilla/Comment.pm
index 7b9e257df..f3628ddb1 100644
--- a/Bugzilla/Comment.pm
+++ b/Bugzilla/Comment.pm
@@ -77,7 +77,7 @@ use constant VALIDATORS => {
use constant VALIDATOR_DEPENDENCIES => {
extra_data => ['type'],
bug_id => ['who'],
- work_time => ['who'],
+ work_time => ['who', 'bug_id'],
isprivate => ['who'],
};
@@ -180,6 +180,17 @@ sub set_extra_data { $_[0]->set('extra_data', $_[1]); }
# Validators #
##############
+sub run_create_validators {
+ my $self = shift;
+ my $params = $self->SUPER::run_create_validators(@_);
+ # Sometimes this run_create_validators is called with parameters that
+ # skip bug_id validation, so it might not exist in the resulting hash.
+ if (defined $params->{bug_id}) {
+ $params->{bug_id} = $params->{bug_id}->id;
+ }
+ return $params;
+}
+
sub _check_extra_data {
my ($invocant, $extra_data, undef, $params) = @_;
my $type = blessed($invocant) ? $invocant->type : $params->{type};
@@ -246,7 +257,7 @@ sub _check_bug_id {
$bug->check_can_change_field('longdesc', 0, 1, \$privs)
|| ThrowUserError('illegal_change',
{ field => 'longdesc', privs => $privs });
- return $bug->id;
+ return $bug;
}
sub _check_who {
@@ -276,7 +287,12 @@ sub _check_work_time {
# Call down to Bugzilla::Object, letting it know negative
# values are ok
- return $invocant->check_time( $value_in, $field, $params, 1);
+ my $time = $invocant->check_time($value_in, $field, $params, 1);
+ my $privs;
+ $params->{bug_id}->check_can_change_field('work_time', 0, $time, \$privs)
+ || ThrowUserError('illegal_change',
+ { field => 'work_time', privs => $privs });
+ return $time;
}
sub _check_thetext {
diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm
index 281143889..6c5289453 100644
--- a/Bugzilla/Object.pm
+++ b/Bugzilla/Object.pm
@@ -544,9 +544,6 @@ sub check_time {
: 0;
$current ||= 0;
- # Don't let the user set the value if they aren't a timetracker
- return $current unless Bugzilla->user->is_timetracker;
-
# Get the new value or zero if it isn't defined
$value = trim($value) || 0;
diff --git a/Bugzilla/WebService/Bug.pm b/Bugzilla/WebService/Bug.pm
index 0160eff1d..14c09246e 100644
--- a/Bugzilla/WebService/Bug.pm
+++ b/Bugzilla/WebService/Bug.pm
@@ -48,7 +48,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 => {
@@ -470,7 +469,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' });
@@ -539,6 +541,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])
@@ -891,7 +898,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
@@ -2495,7 +2514,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.
@@ -2647,9 +2666,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>
@@ -2898,21 +2916,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>
@@ -3010,7 +3098,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.
diff --git a/Bugzilla/WebService/Constants.pm b/Bugzilla/WebService/Constants.pm
index 266ec3828..e2c0a0f52 100644
--- a/Bugzilla/WebService/Constants.pm
+++ b/Bugzilla/WebService/Constants.pm
@@ -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,
diff --git a/Bugzilla/WebService/Server.pm b/Bugzilla/WebService/Server.pm
index f4b3600d4..4e0315219 100644
--- a/Bugzilla/WebService/Server.pm
+++ b/Bugzilla/WebService/Server.pm
@@ -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
}
diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl
index 410636c60..b76106f24 100644
--- a/template/en/default/global/user-error.html.tmpl
+++ b/template/en/default/global/user-error.html.tmpl
@@ -707,10 +707,10 @@
[% 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" %]