Bommels05/Mozilla
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

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

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


git-svn-id: svn://10.0.0.236/trunk@261653 18797224-902f-48f8-a5cc-f745e15eee43
This commit is contained in:
mkanat%bugzilla.org
2010-12-13 21:02:10 +00:00
parent d71c7c42e5
commit 1caec144d2
8 changed files with 193 additions and 42 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
mozilla/webtools/bugzilla/.bzrrev
View File

@@ -1 +1 @@
7619
7620

56
mozilla/webtools/bugzilla/Bugzilla/Bug.pm
View File

@@ -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;

22
mozilla/webtools/bugzilla/Bugzilla/Comment.pm
View File

@@ -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 {

3
mozilla/webtools/bugzilla/Bugzilla/Object.pm
View File

@@ -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;

122
mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm
View File

@@ -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,22 +2916,92 @@ 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.
=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.
=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
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 B<History>
=over
@@ -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.

19
mozilla/webtools/bugzilla/Bugzilla/WebService/Constants.pm
View 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,

3
mozilla/webtools/bugzilla/Bugzilla/WebService/Server.pm
View 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
}

8
mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl
View File

@@ -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" %]