Bug 348477: Move simple validations from post_bug.cgi to Bugzilla::Bug

Patch By Max Kanat-Alexander <mkanat@bugzilla.org> r=bkor, a=myk

2 files changed, 133 insertions, 58 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index b89b0cf37..08677967a 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -217,6 +217,116 @@ sub remove_from_db {
     return $self;
 }
 
+#####################################################################
+# Validators
+#####################################################################
+
+sub _check_alias {
+   my ($alias) = @_;
+   $alias = trim($alias);
+   ValidateBugAlias($alias) if (defined $alias && $alias ne '');
+   return $alias;
+}
+
+sub _check_assigned_to {
+    my ($name, $component) = @_;
+    my $user = Bugzilla->user;
+
+    $name = trim($name);
+    # Default assignee is the component owner.
+    my $id;
+    if (!$user->in_group("editbugs") || !$name) {
+        $id = $component->default_assignee->id;
+    } else {
+        $id = login_to_id($name, THROW_ERROR);
+    }
+    return $id;
+}
+
+sub _check_bug_file_loc {
+    my ($url) = @_;
+    if (!defined $url) {
+        ThrowCodeError('undefined_field', { field => 'bug_file_loc' });
+    }
+    # If bug_file_loc is "http://", the default, use an empty value instead.
+    $url = '' if $url eq 'http://';
+    return $url;
+}
+
+sub _check_comment {
+    my ($comment) = @_;
+
+    if (!defined $comment) {
+        ThrowCodeError('undefined_field', { field => 'comment' });
+    }
+
+    # 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.
+
+    ValidateComment($comment);
+
+    if (Bugzilla->params->{"commentoncreate"} && !$comment) {
+        ThrowUserError("description_required");
+    }
+
+    return $comment;
+}
+
+sub _check_component {
+    my ($name, $product) = @_;
+    $name = trim($name);
+    $name || ThrowUserError("require_component");
+    my $obj = Bugzilla::Component::check_component($product, $name);
+    # XXX Right now, post_bug needs this to return an object. However,
+    # when we move to Bugzilla::Bug->create, this should just return
+    # what it was passed.
+    return $obj;
+}
+
+sub _check_product {
+    my ($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 });
+    # XXX Right now, post_bug needs this to return an object. However,
+    # when we move to Bugzilla::Bug->create, this should just return
+    # what it was passed.
+    return $obj;
+}
+
+sub _check_short_desc {
+    my ($short_desc) = @_;
+    # Set the parameter to itself, but cleaned up
+    $short_desc = clean_text($short_desc) if $short_desc;
+
+    if (!defined $short_desc || $short_desc eq '') {
+        ThrowUserError("require_summary");
+    }
+    return $short_desc;
+}
+
+sub _check_qa_contact {
+    my ($name, $component) = @_;
+    my $user = Bugzilla->user;
+
+    $name = trim($name);
+
+    my $id;
+    if (!$user->in_group("editbugs") || !$name) {
+        # We want to insert NULL into the database if we get a 0.
+        $id = $component->default_qa_contact->id || undef;
+    } else {
+        $id = login_to_id($name, THROW_ERROR);
+    }
+
+    return $id;
+}
+
 
 #####################################################################
 # Class Accessors
diff --git a/post_bug.cgi b/post_bug.cgi
index d8f2150e9..7ceeffc3a 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -113,13 +113,9 @@ my $format = $template->get_format("bug/create/comment",
 $template->process($format->{'template'}, $vars, \$comment)
   || ThrowTemplateError($template->error());
 
-ValidateComment($comment);
-
 # Check that the product exists and that the user
 # is allowed to enter bugs into this product.
-$user->can_enter_product(scalar $cgi->param('product'), 1);
-
-my $product = Bugzilla::Product::check_product(scalar $cgi->param('product'));
+my $product = Bugzilla::Bug::_check_product($cgi->param('product'));
 
 # Set cookies
 if (defined $cgi->param('product')) {
@@ -143,35 +139,23 @@ if (defined $cgi->param('maketemplate')) {
 umask 0;
 
 # Some sanity checking
-$cgi->param('component') || ThrowUserError("require_component");
-my $component = Bugzilla::Component::check_component($product, scalar $cgi->param('component'));
-
-# Set the parameter to itself, but cleaned up
-$cgi->param('short_desc', clean_text($cgi->param('short_desc')));
+my $component = 
+    Bugzilla::Bug::_check_component(scalar $cgi->param('component'), $product);
+$cgi->param('short_desc', 
+            Bugzilla::Bug::_check_short_desc($cgi->param('short_desc')));
 
-if (!defined $cgi->param('short_desc')
-    || $cgi->param('short_desc') eq "") {
-    ThrowUserError("require_summary");
-}
-
-# Check that if required a description has been provided
 # This has to go somewhere after 'maketemplate' 
-#  or it breaks bookmarks with no comments.
-if (Bugzilla->params->{"commentoncreate"} && !trim($cgi->param('comment'))) {
-    ThrowUserError("description_required");
-}
-
-# If bug_file_loc is "http://", the default, use an empty value instead.
-$cgi->param('bug_file_loc', '') if $cgi->param('bug_file_loc') eq 'http://';
+# or it breaks bookmarks with no comments. 
+$comment = Bugzilla::Bug::_check_comment($cgi->param('comment'));
+# If comment is all whitespace, it'll be null at this point. That's
+# OK except for the fact that it causes e-mail to be suppressed.
+$comment = $comment ? $comment : " ";
 
-
-# Default assignee is the component owner.
-if (!UserInGroup("editbugs") || $cgi->param('assigned_to') eq "") {
-    $cgi->param(-name => 'assigned_to', -value => $component->default_assignee->id);
-} else {
-    $cgi->param(-name => 'assigned_to',
-                -value => login_to_id(trim($cgi->param('assigned_to')), THROW_ERROR));
-}
+$cgi->param('bug_file_loc', 
+            Bugzilla::Bug::_check_bug_file_loc($cgi->param('bug_file_loc')));
+$cgi->param('assigned_to', 
+            Bugzilla::Bug::_check_assigned_to(scalar $cgi->param('assigned_to'),
+                                              $component));
 
 
 my @enter_bug_field_names = map {$_->name} Bugzilla->get_fields({ custom => 1,
@@ -184,26 +168,18 @@ my @bug_fields = ("version", "rep_platform",
                   @enter_bug_field_names);
 
 if (Bugzilla->params->{"usebugaliases"}) {
-   my $alias = trim($cgi->param('alias') || "");
-   if ($alias ne "") {
-       ValidateBugAlias($alias);
-       $cgi->param('alias', $alias);
-       push (@bug_fields,"alias");
-   }
+    my $alias = Bugzilla::Bug::_check_alias($cgi->param('alias'));
+    if ($alias) {
+        $cgi->param('alias', $alias);
+        push(@bug_fields, "alias");
+    }
 }
 
-# Retrieve the default QA contact if the field is empty
 if (Bugzilla->params->{"useqacontact"}) {
-    my $qa_contact;
-    if (!UserInGroup("editbugs") || !defined $cgi->param('qa_contact')
-        || trim($cgi->param('qa_contact')) eq "") {
-        $qa_contact = $component->default_qa_contact->id;
-    } else {
-        $qa_contact = login_to_id(trim($cgi->param('qa_contact')), THROW_ERROR);
-    }
-
+    my $qa_contact = Bugzilla::Bug::_check_qa_contact(
+        scalar $cgi->param('qa_contact'), $component);
     if ($qa_contact) {
-        $cgi->param(-name => 'qa_contact', -value => $qa_contact);
+        $cgi->param('qa_contact', $qa_contact);
         push(@bug_fields, "qa_contact");
     }
 }
@@ -245,11 +221,6 @@ check_field('version',      scalar $cgi->param('version'),
 check_field('target_milestone', scalar $cgi->param('target_milestone'),
             [map($_->name, @{$product->milestones})]);
 
-foreach my $field_name ('assigned_to', 'bug_file_loc', 'comment') {
-    defined($cgi->param($field_name))
-      || ThrowCodeError('undefined_field', { field => $field_name });
-}
-
 my $everconfirmed = ($cgi->param('bug_status') eq 'UNCONFIRMED') ? 0 : 1;
 $cgi->param(-name => 'everconfirmed', -value => $everconfirmed);
 
@@ -364,12 +335,6 @@ my $query = qq{INSERT INTO bugs ($sql_used_fields, reporter, delta_ts,
                                  estimated_time, remaining_time, deadline)
                VALUES ($sql_placeholders ?, ?, ?, ?, ?)};
 
-$comment =~ s/\r\n?/\n/g;     # Get rid of \r.
-$comment = trim($comment);
-# If comment is all whitespace, it'll be null at this point. That's
-# OK except for the fact that it causes e-mail to be suppressed.
-$comment = $comment ? $comment : " ";
-
 push (@fields_values, $user->id);
 push (@fields_values, $timestamp);