diff options
| -rw-r--r-- | Bugzilla/Bug.pm | 225 | ||||
| -rw-r--r-- | Bugzilla/Comment.pm | 105 | ||||
| -rw-r--r-- | Bugzilla/Object.pm | 63 | ||||
| -rwxr-xr-x | importxml.pl | 7 | ||||
| -rw-r--r-- | template/en/default/global/code-error.html.tmpl | 4 |
5 files changed, 243 insertions, 161 deletions
diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index 327e55ccc..70f3f607d 100644 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -26,6 +26,7 @@ # Frédéric Buclin <LpSolit@gmail.com> # Lance Larsh <lance.larsh@oracle.com> # Elliotte Martin <elliotte_martin@yahoo.com> +# Christian Legnitto <clegnitto@mozilla.com> package Bugzilla::Bug; @@ -125,11 +126,11 @@ sub VALIDATORS { bug_status => \&_check_bug_status, cc => \&_check_cc, comment => \&_check_comment, - commentprivacy => \&_check_commentprivacy, component => \&_check_component, + creation_ts => \&_check_creation_ts, deadline => \&_check_deadline, dup_id => \&_check_dup_id, - estimated_time => \&_check_estimated_time, + estimated_time => \&Bugzilla::Object::check_time, everconfirmed => \&Bugzilla::Object::check_boolean, groups => \&_check_groups, keywords => \&_check_keywords, @@ -137,7 +138,7 @@ sub VALIDATORS { priority => \&_check_priority, product => \&_check_product, qa_contact => \&_check_qa_contact, - remaining_time => \&_check_remaining_time, + remaining_time => \&Bugzilla::Object::check_time, rep_platform => \&_check_select_field, resolution => \&_check_resolution, short_desc => \&_check_short_desc, @@ -185,6 +186,7 @@ sub VALIDATOR_DEPENDENCIES { assigned_to => ['component'], bug_status => ['product', 'comment', 'target_milestone'], cc => ['component'], + comment => ['creation_ts'], component => ['product'], dup_id => ['bug_status', 'resolution'], groups => ['product'], @@ -246,16 +248,6 @@ sub DATE_COLUMNS { return map { $_->name } @fields; } -# This is used by add_comment to know what we validate before putting in -# the DB. -use constant UPDATE_COMMENT_COLUMNS => qw( - thetext - work_time - type - extra_data - isprivate -); - # Used in LogActivityEntry(). Gives the max length of lines in the # activity table. use constant MAX_LINE_LENGTH => 254; @@ -292,6 +284,9 @@ use constant REQUIRED_FIELD_MAP => { component_id => 'component', }; +# Creation timestamp is here because it needs to be validated +# but it can be NULL in the database (see comments in create above) +# # Target Milestone is here because it has a default that the validator # creates (product.defaultmilestone) that is different from the database # default. @@ -305,7 +300,7 @@ use constant REQUIRED_FIELD_MAP => { # # Groups are in a separate table, but must always be validated so that # mandatory groups get set on bugs. -use constant EXTRA_REQUIRED_FIELDS => qw(target_milestone cc qa_contact groups); +use constant EXTRA_REQUIRED_FIELDS => qw(creation_ts target_milestone cc qa_contact groups); ##################################################################### @@ -615,14 +610,12 @@ sub create { # These are not a fields in the bugs table, so we don't pass them to # insert_create_data. - my $cc_ids = delete $params->{cc}; - my $groups = delete $params->{groups}; - my $depends_on = delete $params->{dependson}; - my $blocked = delete $params->{blocked}; - my $keywords = delete $params->{keywords}; - my ($comment, $privacy) = ($params->{comment}, $params->{commentprivacy}); - delete $params->{comment}; - delete $params->{commentprivacy}; + my $cc_ids = delete $params->{cc}; + my $groups = delete $params->{groups}; + my $depends_on = delete $params->{dependson}; + my $blocked = delete $params->{blocked}; + my $keywords = delete $params->{keywords}; + my $creation_comment = delete $params->{comment}; # We don't want the bug to appear in the system until it's correctly # protected by groups. @@ -686,20 +679,14 @@ sub create { } } - # And insert the comment. We always insert a comment on bug creation, + # Comment #0 handling... + + # We now have a bug id so we can fill this out + $creation_comment->{'bug_id'} = $bug->id; + + # Insert the comment. We always insert a comment on bug creation, # but sometimes it's blank. - my @columns = qw(bug_id who bug_when thetext); - my @values = ($bug->bug_id, $bug->{reporter_id}, $timestamp, $comment); - # We don't include the "isprivate" column unless it was specified. - # This allows it to fall back to its database default. - if (defined $privacy) { - push(@columns, 'isprivate'); - push(@values, $privacy); - } - my $qmarks = "?," x @columns; - chop($qmarks); - $dbh->do('INSERT INTO longdescs (' . join(',', @columns) . ") - VALUES ($qmarks)", undef, @values); + Bugzilla::Comment->insert_create_data($creation_comment); Bugzilla::Hook::process('bug_end_of_create', { bug => $bug, timestamp => $timestamp, @@ -726,8 +713,6 @@ sub run_create_validators { # Callers cannot set reporter, creation_ts, or delta_ts. $params->{reporter} = $class->_check_reporter(); - $params->{creation_ts} = - Bugzilla->dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)'); $params->{delta_ts} = $params->{creation_ts}; if ($params->{estimated_time}) { @@ -903,26 +888,21 @@ sub update { # Comments foreach my $comment (@{$self->{added_comments} || []}) { - my $columns = join(',', keys %$comment); - my @values = values %$comment; - my $qmarks = join(',', ('?') x @values); - $dbh->do("INSERT INTO longdescs (bug_id, who, bug_when, $columns) - VALUES (?,?,?,$qmarks)", undef, - $self->bug_id, Bugzilla->user->id, $delta_ts, @values); + $comment = Bugzilla::Comment->insert_create_data($comment); if ($comment->{work_time}) { LogActivityEntry($self->id, "work_time", "", $comment->{work_time}, Bugzilla->user->id, $delta_ts); } } - + # Comment Privacy - foreach my $comment_id (keys %{$self->{comment_isprivate} || {}}) { - $dbh->do("UPDATE longdescs SET isprivate = ? WHERE comment_id = ?", - undef, $self->{comment_isprivate}->{$comment_id}, $comment_id); + foreach my $comment (@{$self->{comment_isprivate} || []}) { + $comment->update(); + my ($from, $to) - = $self->{comment_isprivate}->{$comment_id} ? (0, 1) : (1, 0); + = $comment->is_private ? (0, 1) : (1, 0); LogActivityEntry($self->id, "longdescs.isprivate", $from, $to, - Bugzilla->user->id, $delta_ts, $comment_id); + Bugzilla->user->id, $delta_ts, $comment->id); } # Insert the values into the multiselect value tables @@ -1417,33 +1397,43 @@ sub _check_cc { } sub _check_comment { - my ($invocant, $comment) = @_; + my ($invocant, $comment_txt, undef, $params) = @_; - $comment = '' unless defined $comment; + # Comment can be empty. We should force it to be empty if the text is undef + if (!defined $comment_txt) { + $comment_txt = ''; + } - # Remove any trailing whitespace. Leading whitespace could be - # a valid part of the comment. - $comment =~ s/\s*$//s; - $comment =~ s/\r\n?/\n/g; # Get rid of \r. + # Load up some data + my $isprivate = $params->{commentprivacy}; + my $timestamp = $params->{creation_ts}; - ThrowUserError('comment_too_long') if length($comment) > MAX_COMMENT_LENGTH; - return $comment; -} + # Create the new comment so we can check it + my $comment = { + thetext => $comment_txt, + bug_when => $timestamp, + }; -sub _check_commentprivacy { - my ($invocant, $comment_privacy) = @_; - if ($comment_privacy && !Bugzilla->user->is_insider) { - ThrowUserError('user_not_insider'); + # We don't include the "isprivate" column unless it was specified. + # This allows it to fall back to its database default. + if (defined $isprivate) { + $comment->{isprivate} = $isprivate; } - return $comment_privacy ? 1 : 0; -} -sub _check_comment_type { - my ($invocant, $type) = @_; - detaint_natural($type) - || ThrowCodeError('bad_arg', { argument => 'type', - function => caller }); - return $type; + # Don't need this anymore as it is now in the comment hash + delete $params->{commentprivacy}; + + # Validate comment. We have to do this special as a comment normally + # requires a bug to be already created. For a new bug, the first comment + # obviously can't get the bug if the bug is created after this + # (see bug 590334) + Bugzilla::Comment->check_required_create_fields($comment); + $comment = Bugzilla::Comment->run_create_validators($comment, + { skip => ['bug_id'] } + ); + + return $comment; + } sub _check_component { @@ -1456,6 +1446,10 @@ sub _check_component { return $obj; } +sub _check_creation_ts { + return Bugzilla->dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)'); +} + sub _check_deadline { my ($invocant, $date) = @_; @@ -1605,10 +1599,6 @@ sub _check_dup_id { return $dupe_of; } -sub _check_estimated_time { - return $_[0]->_check_time($_[1], 'estimated_time'); -} - sub _check_groups { my ($invocant, $group_names, undef, $params) = @_; my $product = blessed($invocant) ? $invocant->product_obj @@ -1728,10 +1718,6 @@ sub _check_qa_contact { return $id || undef; } -sub _check_remaining_time { - return $_[0]->_check_time($_[1], 'remaining_time'); -} - sub _check_reporter { my $invocant = shift; my $reporter; @@ -1886,20 +1872,6 @@ sub _check_target_milestone { return $object->name; } -sub _check_time { - my ($invocant, $time, $field) = @_; - - my $current = 0; - if (ref $invocant && $field ne 'work_time') { - $current = $invocant->$field; - } - return $current unless Bugzilla->user->is_timetracker; - - $time = trim($time) || 0; - ValidateTime($time, $field); - return $time; -} - sub _check_version { my ($invocant, $version, undef, $params) = @_; $version = trim($version); @@ -1910,10 +1882,6 @@ sub _check_version { return $object->name; } -sub _check_work_time { - return $_[0]->_check_time($_[1], 'work_time'); -} - # Custom Field Validators sub _check_field_is_mandatory { @@ -2265,8 +2233,9 @@ sub set_comment_is_private { $isprivate = $isprivate ? 1 : 0; if ($isprivate != $comment->is_private) { - $self->{comment_isprivate} ||= {}; - $self->{comment_isprivate}->{$comment_id} = $isprivate; + $self->{comment_isprivate} ||= []; + $comment->set_is_private($isprivate); + push @{$self->{comment_isprivate}}, $comment; } } sub set_component { @@ -2636,23 +2605,22 @@ sub remove_cc { sub add_comment { my ($self, $comment, $params) = @_; - $comment = $self->_check_comment($comment); - $params ||= {}; - $params->{work_time} = $self->_check_work_time($params->{work_time}); - if (exists $params->{type}) { - $params->{type} = $self->_check_comment_type($params->{type}); - } - if (exists $params->{isprivate}) { - $params->{isprivate} = - $self->_check_commentprivacy($params->{isprivate}); - } - # XXX We really should check extra_data, too. + # This makes it so we won't create new comments when there is nothing + # to add if ($comment eq '' && !($params->{type} || abs($params->{work_time}))) { return; } + # Fill out info that doesn't change and callers may not pass in + $params->{'bug_id'} = $self; + $params->{'thetext'} = $comment; + + # Validate all the entered data + Bugzilla::Comment->check_required_create_fields($params); + $params = Bugzilla::Comment->run_create_validators($params); + # If the user has explicitly set remaining_time, this will be overridden # later in set_all. But if they haven't, this keeps remaining_time # up-to-date. @@ -2660,23 +2628,9 @@ sub add_comment { $self->set_remaining_time(max($self->remaining_time - $params->{work_time}, 0)); } - # So we really want to comment. Make sure we are allowed to do so. - my $privs; - $self->check_can_change_field('longdesc', 0, 1, \$privs) - || ThrowUserError('illegal_change', { field => 'longdesc', privs => $privs }); - $self->{added_comments} ||= []; - my $add_comment = dclone($params); - $add_comment->{thetext} = $comment; - # We only want to trick_taint fields that we know about--we don't - # want to accidentally let somebody set some field that's not OK - # to set! - foreach my $field (UPDATE_COMMENT_COLUMNS) { - trick_taint($add_comment->{$field}) if defined $add_comment->{$field}; - } - - push(@{$self->{added_comments}}, $add_comment); + push(@{$self->{added_comments}}, $params); } # There was a lot of duplicate code when I wrote this as three separate @@ -3638,29 +3592,6 @@ sub EmitDependList { return $list_ref; } -sub ValidateTime { - my ($time, $field) = @_; - - # regexp verifies one or more digits, optionally followed by a period and - # zero or more digits, OR we have a period followed by one or more digits - # (allow negatives, though, so people can back out errors in time reporting) - if ($time !~ /^-?(?:\d+(?:\.\d*)?|\.\d+)$/) { - ThrowUserError("number_not_numeric", - {field => "$field", num => "$time"}); - } - - # Only the "work_time" field is allowed to contain a negative value. - if ( ($time < 0) && ($field ne "work_time") ) { - ThrowUserError("number_too_small", - {field => "$field", num => "$time", min_num => "0"}); - } - - if ($time > 99999.99) { - ThrowUserError("number_too_large", - {field => "$field", num => "$time", max_num => "99999.99"}); - } -} - # Get the activity of a bug, starting from $starttime (if given). # This routine assumes Bugzilla::Bug->check has been previously called. sub GetBugActivity { diff --git a/Bugzilla/Comment.pm b/Bugzilla/Comment.pm index b4f94e213..e8bdb4e89 100644 --- a/Bugzilla/Comment.pm +++ b/Bugzilla/Comment.pm @@ -17,6 +17,7 @@ # All rights reserved. # # Contributor(s): James Robson <arbingersys@gmail.com> +# Christian Legnitto <clegnitto@mozilla.com> use strict; @@ -50,6 +51,7 @@ use constant DB_COLUMNS => qw( ); use constant UPDATE_COLUMNS => qw( + isprivate type extra_data ); @@ -62,12 +64,21 @@ use constant ID_FIELD => 'comment_id'; use constant LIST_ORDER => 'bug_when, comment_id'; use constant VALIDATORS => { - extra_data => \&_check_extra_data, - type => \&_check_type, + bug_id => \&_check_bug_id, + who => \&_check_who, + bug_when => \&_check_bug_when, + work_time => \&_check_work_time, + thetext => \&_check_thetext, + isprivate => \&_check_isprivate, + extra_data => \&_check_extra_data, + type => \&_check_type, }; use constant VALIDATOR_DEPENDENCIES => { extra_data => ['type'], + bug_id => ['who'], + work_time => ['who'], + isprivate => ['who'], }; ######################### @@ -157,12 +168,9 @@ sub body_full { # Mutators # ############ -sub set_extra_data { $_[0]->set('extra_data', $_[1]); } - -sub set_type { - my ($self, $type) = @_; - $self->set('type', $type); -} +sub set_is_private { $_[0]->set('isprivate', $_[1]); } +sub set_type { $_[0]->set('type', $_[1]); } +sub set_extra_data { $_[0]->set('extra_data', $_[1]); } ############## # Validators # @@ -209,6 +217,87 @@ sub _check_type { return $type; } +sub _check_bug_id { + my ($invocant, $bug_id) = @_; + + ThrowCodeError('param_required', {function => 'Bugzilla::Comment->create', + param => 'bug_id'}) unless $bug_id; + + my $bug; + if (blessed $bug_id) { + # We got a bug object passed in, use it + $bug = $bug_id; + $bug->check_is_visible; + } + else { + # We got a bug id passed in, check it and get the bug object + $bug = Bugzilla::Bug->check({ id => $bug_id }); + } + + # Make sure the user can edit the product + Bugzilla->user->can_edit_product($bug->{product_id}); + + # Make sure the user can comment + my $privs; + $bug->check_can_change_field('longdesc', 0, 1, \$privs) + || ThrowUserError('illegal_change', + { field => 'longdesc', privs => $privs }); + return $bug->id; +} + +sub _check_who { + my ($invocant, $who) = @_; + Bugzilla->login(LOGIN_REQUIRED); + return Bugzilla->user->id; +} + +sub _check_bug_when { + my ($invocant, $when) = @_; + + # Make sure the timestamp is defined, default to a timestamp from the db + if (!defined $when) { + $when = Bugzilla->dbh->selectrow_array('SELECT LOCALTIMESTAMP(0)'); + } + + # Make sure the timestamp parses + if (!datetime_from($when)) { + ThrowCodeError('invalid_timestamp', { timestamp => $when }); + } + + return $when; +} + +sub _check_work_time { + my ($invocant, $value_in, $field, $params) = @_; + + # Call down to Bugzilla::Object, letting it know negative + # values are ok + return $invocant->check_time( $value_in, $field, $params, 1); +} + +sub _check_thetext { + my ($invocant, $thetext) = @_; + + ThrowCodeError('param_required',{function => 'Bugzilla::Comment->create', + param => 'thetext'}) unless defined $thetext; + + # Remove any trailing whitespace. Leading whitespace could be + # a valid part of the comment. + $thetext =~ s/\s*$//s; + $thetext =~ s/\r\n?/\n/g; # Get rid of \r. + + ThrowUserError('comment_too_long') if length($thetext) > MAX_COMMENT_LENGTH; + return $thetext; +} + +sub _check_isprivate { + my ($invocant, $isprivate) = @_; + if ($isprivate && !Bugzilla->user->is_insider) { + ThrowUserError('user_not_insider'); + } + return $isprivate ? 1 : 0; +} + sub count { my ($self) = @_; diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm index 5b8576512..281143889 100644 --- a/Bugzilla/Object.pm +++ b/Bugzilla/Object.pm @@ -30,6 +30,7 @@ use Bugzilla::Error; use Date::Parse; use List::MoreUtils qw(part); +use Scalar::Util qw(blessed); use constant NAME_FIELD => 'name'; use constant ID_FIELD => 'id'; @@ -462,13 +463,21 @@ sub check_required_create_fields { } sub run_create_validators { - my ($class, $params) = @_; + my ($class, $params, $options) = @_; my $validators = $class->_get_validators; my %field_values = %$params; + # Make a hash skiplist for easier searching later + my %skip_list = map { $_ => 1 } @{ $options->{skip} || [] }; + + # Get the sorted field names my @sorted_names = $class->_sort_by_dep(keys %field_values); - foreach my $field (@sorted_names) { + + # Remove the skipped names + my @unskipped = grep { !$skip_list{$_} } @sorted_names; + + foreach my $field (@unskipped) { my $value; if (exists $validators->{$field}) { my $validator = $validators->{$field}; @@ -527,10 +536,54 @@ sub get_all { sub check_boolean { return $_[1] ? 1 : 0 } +sub check_time { + my ($invocant, $value, $field, $params, $allow_negative) = @_; + + # If we don't have a current value default to zero + my $current = blessed($invocant) ? $invocant->{$field} + : 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; + + # Make sure the new value is well formed + _validate_time($value, $field, $allow_negative); + + return $value; +} + + ################### # General Helpers # ################### +sub _validate_time { + my ($time, $field, $allow_negative) = @_; + + # regexp verifies one or more digits, optionally followed by a period and + # zero or more digits, OR we have a period followed by one or more digits + # (allow negatives, though, so people can back out errors in time reporting) + if ($time !~ /^-?(?:\d+(?:\.\d*)?|\.\d+)$/) { + ThrowUserError("number_not_numeric", + {field => $field, num => "$time"}); + } + + # Callers can optionally allow negative times + if ( ($time < 0) && !$allow_negative ) { + ThrowUserError("number_too_small", + {field => $field, num => "$time", min_num => "0"}); + } + + if ($time > 99999.99) { + ThrowUserError("number_too_large", + {field => $field, num => "$time", max_num => "99999.99"}); + } +} + # Sorts fields according to VALIDATOR_DEPENDENCIES. This is not a # traditional topological sort, because a "dependency" does not # *have* to be in the list--it just has to be earlier than its dependent @@ -1036,7 +1089,11 @@ Description: Runs the validation of input parameters for L</create>. of their input parameters. This method is B<only> called by L</create>. -Params: The same as L</create>. +Params: C<$params> - hashref - A value to put in each database + field for this object. + C<$options> - hashref - Processing options. Currently + the only option supported is B<skip>, which can be + used to specify a list of fields to not validate. Returns: A hash, in a similar format as C<$params>, except that these are the values to be inserted into the database, diff --git a/importxml.pl b/importxml.pl index e1030b4bb..174f7bdad 100755 --- a/importxml.pl +++ b/importxml.pl @@ -70,6 +70,7 @@ use lib qw(. lib); use Bugzilla; +use Bugzilla::Object; use Bugzilla::Bug; use Bugzilla::Product; use Bugzilla::Version; @@ -763,7 +764,7 @@ sub process_bug { push( @query, "deadline" ); if ( defined $bug_fields{'estimated_time'} ) { eval { - Bugzilla::Bug::ValidateTime($bug_fields{'estimated_time'}, "e"); + Bugzilla::Object::_validate_time($bug_fields{'estimated_time'}, "e"); }; if (!$@){ push( @values, $bug_fields{'estimated_time'} ); @@ -772,7 +773,7 @@ sub process_bug { } if ( defined $bug_fields{'remaining_time'} ) { eval { - Bugzilla::Bug::ValidateTime($bug_fields{'remaining_time'}, "r"); + Bugzilla::Object::_validate_time($bug_fields{'remaining_time'}, "r"); }; if (!$@){ push( @values, $bug_fields{'remaining_time'} ); @@ -781,7 +782,7 @@ sub process_bug { } if ( defined $bug_fields{'actual_time'} ) { eval { - Bugzilla::Bug::ValidateTime($bug_fields{'actual_time'}, "a"); + Bugzilla::Object::_validate_time($bug_fields{'actual_time'}, "a"); }; if ($@){ $bug_fields{'actual_time'} = 0.0; diff --git a/template/en/default/global/code-error.html.tmpl b/template/en/default/global/code-error.html.tmpl index e507c584f..c5c01f401 100644 --- a/template/en/default/global/code-error.html.tmpl +++ b/template/en/default/global/code-error.html.tmpl @@ -236,6 +236,10 @@ The series_id [% series_id FILTER html %] is not valid. It may be that this series has been deleted. + [% ELSIF error == "invalid_timestamp" %] + The entered timestamp <code>[% timestamp FILTER html %]</code> could not + be parsed into a valid date and time. + [% ELSIF error == "invalid_webservergroup" %] There is no such group: [% group FILTER html %]. Check your $webservergroup setting in [% constants.bz_locations.localconfig FILTER html %]. |