From 3448aa4db7fd9d034436df34cdaa053a81f191c1 Mon Sep 17 00:00:00 2001 From: "mkanat%bugzilla.org" <> Date: Thu, 8 Mar 2007 05:54:01 +0000 Subject: Bug 371070: Move basic validations from process_bug.cgi into Bugzilla::Bug Patch By Max Kanat-Alexander r=LpSolit, a=LpSolit --- Bugzilla/Bug.pm | 48 ++++++++++++++++++++++++++++------------- process_bug.cgi | 66 ++++++++++++++++++++++++++++----------------------------- 2 files changed, 66 insertions(+), 48 deletions(-) diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index 8ee0d87e2..a530687fe 100755 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -495,8 +495,13 @@ sub _check_assigned_to { sub _check_bug_file_loc { my ($invocant, $url) = @_; - # If bug_file_loc is "http://", the default, use an empty value instead. - $url = '' if (!defined($url) || $url eq 'http://'); + $url = '' if !defined($url); + # On bug entry, if bug_file_loc is "http://", the default, use an + # empty value instead. However, on bug editing people can set that + # back if they *really* want to. + if (!ref $invocant && $url eq 'http://') { + $url = ''; + } return $url; } @@ -564,13 +569,16 @@ sub _check_comment { ValidateComment($comment); - if (Bugzilla->params->{"commentoncreate"} && !$comment) { - ThrowUserError("description_required"); - } + # Creation-only checks + if (!ref $invocant) { + if (Bugzilla->params->{"commentoncreate"} && !$comment) { + ThrowUserError("description_required"); + } - # On creation only, there must be a single-space comment, or - # email will be supressed. - $comment = ' ' if $comment eq '' && !ref($invocant); + # On creation only, there must be a single-space comment, or + # email will be supressed. + $comment = ' ' if $comment eq ''; + } return $comment; } @@ -699,12 +707,19 @@ sub _check_keywords { sub _check_product { my ($invocant, $name) = @_; - # Check that the product exists and that the user - # is allowed to enter bugs into this product. - Bugzilla->user->can_enter_product($name, THROW_ERROR); - # can_enter_product already does everything that check_product - # would do for us, so we don't need to use it. - my $obj = new Bugzilla::Product({ name => $name }); + my $obj; + if (ref $invocant) { + $obj = Bugzilla::Product::check_product($name); + } + else { + # Check that the product exists and that the user + # is allowed to enter bugs into this product. + Bugzilla->user->can_enter_product($name, THROW_ERROR); + # can_enter_product already does everything that check_product + # would do for us, so we don't need to use it. + $obj = new Bugzilla::Product({ name => $name }); + } + return $obj; } @@ -717,7 +732,7 @@ sub _check_op_sys { sub _check_priority { my ($invocant, $priority) = @_; - if (!Bugzilla->params->{'letsubmitterchoosepriority'}) { + if (!ref $invocant && !Bugzilla->params->{'letsubmitterchoosepriority'}) { $priority = Bugzilla->params->{'defaultpriority'}; } $priority = trim($priority); @@ -784,6 +799,9 @@ sub _check_strict_isolation { sub _check_target_milestone { my ($invocant, $product, $target) = @_; $target = trim($target); + if (ref $invocant) { + return undef if !Bugzilla->params->{'usetargetmilestone'}; + } $target = $product->default_milestone if !defined $target; check_field('target_milestone', $target, [map($_->name, @{$product->milestones})]); diff --git a/process_bug.cgi b/process_bug.cgi index 6c0384973..00ff4c759 100755 --- a/process_bug.cgi +++ b/process_bug.cgi @@ -151,10 +151,8 @@ if (defined $cgi->param('id')) { # Make sure there are bugs to process. scalar(@idlist) || ThrowUserError("no_bugs_chosen"); -# Build a bug object using $cgi->param('id') as ID. -# If there are more than one bug changed at once, the bug object will be -# empty, which doesn't matter. -my $bug = new Bugzilla::Bug(scalar $cgi->param('id')); +# Build a bug object using the first bug id, for validations. +my $bug = new Bugzilla::Bug($idlist[0]); # Make sure form param 'dontchange' is defined so it can be compared to easily. $cgi->param('dontchange','') unless defined $cgi->param('dontchange'); @@ -164,11 +162,10 @@ $cgi->param('knob', 'none') unless defined $cgi->param('knob'); # Validate all timetracking fields foreach my $field ("estimated_time", "work_time", "remaining_time") { - if (defined $cgi->param($field)) { - my $er_time = trim($cgi->param($field)); - if ($er_time ne $cgi->param('dontchange')) { - Bugzilla::Bug::ValidateTime($er_time, $field); - } + if (defined $cgi->param($field) + && $cgi->param($field) ne $cgi->param('dontchange')) + { + $cgi->param($field, $bug->_check_time($cgi->param($field), $field)); } } @@ -179,7 +176,7 @@ if (Bugzilla->user->in_group(Bugzilla->params->{'timetrackinggroup'})) { } } -ValidateComment(scalar $cgi->param('comment')); +$cgi->param('comment', $bug->_check_comment($cgi->param('comment'))); # If the bug(s) being modified have dependencies, validate them # and rebuild the list with the validated values. This is important @@ -520,30 +517,33 @@ if (defined $cgi->param('id')) { # values that have been changed instead of submitting all the new values. # (XXX those error checks need to happen too, but implementing them # is more work in the current architecture of this script...) - my $prod_obj = Bugzilla::Product::check_product($cgi->param('product')); - check_field('component', scalar $cgi->param('component'), - [map($_->name, @{$prod_obj->components})]); - check_field('version', scalar $cgi->param('version'), - [map($_->name, @{$prod_obj->versions})]); - if ( Bugzilla->params->{"usetargetmilestone"} ) { - check_field('target_milestone', scalar $cgi->param('target_milestone'), - [map($_->name, @{$prod_obj->milestones})]); - } - check_field('rep_platform', scalar $cgi->param('rep_platform')); - check_field('op_sys', scalar $cgi->param('op_sys')); - check_field('priority', scalar $cgi->param('priority')); - check_field('bug_severity', scalar $cgi->param('bug_severity')); - - # Those fields only have to exist. We don't validate their value here. - foreach my $field_name ('bug_file_loc', 'short_desc', 'longdesclength') { - defined($cgi->param($field_name)) - || ThrowCodeError('undefined_field', { field => $field_name }); - } - $cgi->param('short_desc', clean_text($cgi->param('short_desc'))); - if (trim($cgi->param('short_desc')) eq "") { - ThrowUserError("require_summary"); - } + my $prod = $bug->_check_product($cgi->param('product')); + $cgi->param('product', $prod->name); + + my $comp = $bug->_check_component($prod, + scalar $cgi->param('component')); + $cgi->param('component', $comp->name); + + $cgi->param('version', $bug->_check_version($prod, + scalar $cgi->param('version'))); + $cgi->param('target_milestone', $bug->_check_target_milestone($prod, + scalar $cgi->param('target_milestone'))); + $cgi->param('rep_platform', + $bug->_check_rep_platform($cgi->param('rep_platform'))); + $cgi->param('op_sys', + $bug->_check_op_sys($cgi->param('op_sys'))); + $cgi->param('priority', + $bug->_check_priority($cgi->param('priority'))); + $cgi->param('bug_severity', + $bug->_check_bug_severity($cgi->param('bug_severity'))); + + ThrowCodeError('undefined_field', { field => 'longdesclength' }) + if !defined $cgi->param('longdesclength'); + $cgi->param('bug_file_loc', + $bug->_check_bug_file_loc($cgi->param('bug_file_loc'))); + $cgi->param('short_desc', + $bug->_check_short_desc($cgi->param('short_desc'))); } my $action = trim($cgi->param('action') || ''); -- cgit v1.2.3-24-g4f1b