Skip to content

bugzilla
bugzilla
Commit a128f843 authored by Christian Legnitto's avatar Christian Legnitto Committed by Max Kanat-Alexander
Browse files
Options

Bug 590334: Change Bug.pm to use the comment object (Bugzilla::Comment)

when creating or updating bug comment r=mkanat, a=mkanat
parent 2b8ade66
Show whitespace changes
Inline Side-by-side
Showing with 234 additions and 152 deletions
Bugzilla/Bug.pm
View file @ a128f843
......@@ -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);
#####################################################################
......@@ -620,9 +615,7 @@ sub create {
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 $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,12 +888,7 @@ 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);
......@@ -916,13 +896,13 @@ sub update {
}
# 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 {
......
Bugzilla/Comment.pm
View file @ a128f843
......@@ -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 => {
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,13 +168,10 @@ sub body_full {
# Mutators #
############
sub set_is_private { $_[0]->set('isprivate', $_[1]); }
sub set_type { $_[0]->set('type', $_[1]); }
sub set_extra_data { $_[0]->set('extra_data', $_[1]); }
sub set_type {
my ($self, $type) = @_;
$self->set('type', $type);
}
##############
# 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) = @_;
......
Bugzilla/Object.pm
View file @ a128f843
......@@ -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,
......
importxml.pl
View file @ a128f843
......@@ -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;
......
template/en/default/global/code-error.html.tmpl
View file @ a128f843
......@@ -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 %].
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment