Bug 242318: "blocked" field is ignored in post_bug.cgi if the "dependson" field isn't present - Patch by Frédéric Buclin <LpSolit@gmail.com> r=kiko a=justdave

3 files changed, 88 insertions, 131 deletions

diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm
index 675c2ec15..40ed06631 100755
--- a/Bugzilla/Bug.pm
+++ b/Bugzilla/Bug.pm
@@ -1037,6 +1037,78 @@ sub ValidateBugAlias {
     $_[0] = $alias;
 }
 
+# Validate and return a hash of dependencies
+sub ValidateDependencies($$$) {
+    my $fields = {};
+    $fields->{'dependson'} = shift;
+    $fields->{'blocked'} = shift;
+    my $id = shift || 0;
+
+    unless (defined($fields->{'dependson'})
+            || defined($fields->{'blocked'}))
+    {
+        return;
+    }
+
+    my $dbh = Bugzilla->dbh;
+    my %deps;
+    my %deptree;
+    foreach my $pair (["blocked", "dependson"], ["dependson", "blocked"]) {
+        my ($me, $target) = @{$pair};
+        $deptree{$target} = [];
+        $deps{$target} = [];
+        next unless $fields->{$target};
+
+        my %seen;
+        foreach my $i (split('[\s,]+', $fields->{$target})) {
+            if ($id == $i) {
+                ThrowUserError("dependency_loop_single");
+            }
+            if (!exists $seen{$i}) {
+                push(@{$deptree{$target}}, $i);
+                $seen{$i} = 1;
+            }
+        }
+        # populate $deps{$target} as first-level deps only.
+        # and find remainder of dependency tree in $deptree{$target}
+        @{$deps{$target}} = @{$deptree{$target}};
+        my @stack = @{$deps{$target}};
+        while (@stack) {
+            my $i = shift @stack;
+            my $dep_list =
+                $dbh->selectcol_arrayref("SELECT $target
+                                          FROM dependencies
+                                          WHERE $me = ?", undef, $i);
+            foreach my $t (@$dep_list) {
+                # ignore any _current_ dependencies involving this bug,
+                # as they will be overwritten with data from the form.
+                if ($t != $id && !exists $seen{$t}) {
+                    push(@{$deptree{$target}}, $t);
+                    push @stack, $t;
+                    $seen{$t} = 1;
+                }
+            }
+        }
+    }
+
+    my @deps   = @{$deptree{'dependson'}};
+    my @blocks = @{$deptree{'blocked'}};
+    my @union = ();
+    my @isect = ();
+    my %union = ();
+    my %isect = ();
+    foreach my $b (@deps, @blocks) { $union{$b}++ && $isect{$b}++ }
+    @union = keys %union;
+    @isect = keys %isect;
+    if (scalar(@isect) > 0) {
+        my $both = "";
+        foreach my $i (@isect) {
+           $both .= &::GetBugLink($i, "#" . $i) . " ";
+        }
+        ThrowUserError("dependency_loop_multi", { both => $both });
+    }
+    return %deps;
+}
 
 sub AUTOLOAD {
   use vars qw($AUTOLOAD);
diff --git a/post_bug.cgi b/post_bug.cgi
index b9d63b3fe..70030dc1a 100755
--- a/post_bug.cgi
+++ b/post_bug.cgi
@@ -264,8 +264,7 @@ if ($cgi->param('keywords') && UserInGroup("editbugs")) {
 
 # Check for valid dependency info. 
 foreach my $field ("dependson", "blocked") {
-    if (UserInGroup("editbugs") && defined($cgi->param($field)) &&
-        $cgi->param($field) ne "") {
+    if (UserInGroup("editbugs") && $cgi->param($field)) {
         my @validvalues;
         foreach my $id (split(/[\s,]+/, $cgi->param($field))) {
             next unless $id;
@@ -275,63 +274,12 @@ foreach my $field ("dependson", "blocked") {
         $cgi->param(-name => $field, -value => join(",", @validvalues));
     }
 }
-# Gather the dependecy list, and make sure there are no circular refs
+# Gather the dependency list, and make sure there are no circular refs
 my %deps;
-if (UserInGroup("editbugs") && defined($cgi->param('dependson'))) {
-    my $me = "blocked";
-    my $target = "dependson";
-    my %deptree;
-    for (1..2) {
-        $deptree{$target} = [];
-        my %seen;
-        foreach my $i (split('[\s,]+', $cgi->param($target))) {
-            if (!exists $seen{$i}) {
-                push(@{$deptree{$target}}, $i);
-                $seen{$i} = 1;
-            }
-        }
-        # populate $deps{$target} as first-level deps only.
-        # and find remainder of dependency tree in $deptree{$target}
-        @{$deps{$target}} = @{$deptree{$target}};
-        my @stack = @{$deps{$target}};
-        while (@stack) {
-            my $i = shift @stack;
-            SendSQL("SELECT $target FROM dependencies WHERE $me = " .
-                    SqlQuote($i));
-            while (MoreSQLData()) {
-                my $t = FetchOneColumn();
-                if (!exists $seen{$t}) {
-                    push(@{$deptree{$target}}, $t);
-                    push @stack, $t;
-                    $seen{$t} = 1;
-                } 
-            }
-        }
-        
-        if ($me eq 'dependson') {
-            my @deps   =  @{$deptree{'dependson'}};
-            my @blocks =  @{$deptree{'blocked'}};
-            my @union = ();
-            my @isect = ();
-            my %union = ();
-            my %isect = ();
-            foreach my $b (@deps, @blocks) { $union{$b}++ && $isect{$b}++ }
-            @union = keys %union;
-            @isect = keys %isect;
-            if (@isect > 0) {
-                my $both;
-                foreach my $i (@isect) {
-                    $both = $both . GetBugLink($i, "#" . $i) . " ";
-                }
-
-                ThrowUserError("dependency_loop_multi",
-                               { both => $both });
-            }
-        }
-        my $tmp = $me;
-        $me = $target;
-        $target = $tmp;
-    }
+if (UserInGroup("editbugs")) {
+    %deps = Bugzilla::Bug::ValidateDependencies($cgi->param('dependson'),
+                                                $cgi->param('blocked'),
+                                                undef);
 }
 
 # get current time
@@ -473,10 +421,10 @@ if (UserInGroup("editbugs")) {
                 " keywords = " . SqlQuote(join(', ', @list)) .
                 " WHERE bug_id = $id");
     }
-    if (defined $cgi->param('dependson')) {
-        my $me = "blocked";
-        my $target = "dependson";
-        for (1..2) {
+    if ($cgi->param('dependson') || $cgi->param('blocked')) {
+        foreach my $pair (["blocked", "dependson"], ["dependson", "blocked"]) {
+            my ($me, $target) = @{$pair};
+
             foreach my $i (@{$deps{$target}}) {
                 SendSQL("INSERT INTO dependencies ($me, $target) values " .
                         "($id, $i)");
@@ -484,9 +432,6 @@ if (UserInGroup("editbugs")) {
                 # Log the activity for the other bug:
                 LogActivityEntry($i, $me, "", $id, $user->id, $timestamp);
             }
-            my $tmp = $me;
-            $me = $target;
-            $target = $tmp;
         }
     }
 }
diff --git a/process_bug.cgi b/process_bug.cgi
index c90ed42d9..f2f71710d 100755
--- a/process_bug.cgi
+++ b/process_bug.cgi
@@ -142,7 +142,7 @@ ValidateComment(scalar $cgi->param('comment'));
 # is a bug alias that gets converted to its corresponding bug ID
 # during validation.
 foreach my $field ("dependson", "blocked") {
-    if (defined $cgi->param($field) && $cgi->param($field) ne "") {
+    if ($cgi->param($field)) {
         my @validvalues;
         foreach my $id (split(/[\s,]+/, $cgi->param($field))) {
             next unless $id;
@@ -1319,71 +1319,11 @@ foreach my $id (@idlist) {
           || ThrowTemplateError($template->error());
         exit;
     }
-        
-    my %deps;
-    if (defined $cgi->param('dependson')) {
-        my $me = "blocked";
-        my $target = "dependson";
-        my %deptree;
-        for (1..2) {
-            $deptree{$target} = [];
-            my %seen;
-            foreach my $i (split('[\s,]+', $cgi->param($target))) {
-                next if $i eq "";
-                
-                if ($id eq $i) {
-                    ThrowUserError("dependency_loop_single");
-                }
-                if (!exists $seen{$i}) {
-                    push(@{$deptree{$target}}, $i);
-                    $seen{$i} = 1;
-                }
-            }
-            # populate $deps{$target} as first-level deps only.
-            # and find remainder of dependency tree in $deptree{$target}
-            @{$deps{$target}} = @{$deptree{$target}};
-            my @stack = @{$deps{$target}};
-            while (@stack) {
-                my $i = shift @stack;
-                SendSQL("SELECT $target FROM dependencies WHERE $me = " .
-                        SqlQuote($i));
-                while (MoreSQLData()) {
-                    my $t = FetchOneColumn();
-                    # ignore any _current_ dependencies involving this bug,
-                    # as they will be overwritten with data from the form.
-                    if ($t != $id && !exists $seen{$t}) {
-                        push(@{$deptree{$target}}, $t);
-                        push @stack, $t;
-                        $seen{$t} = 1;
-                    }
-                }
-            }
 
-            if ($me eq 'dependson') {
-                my @deps   =  @{$deptree{'dependson'}};
-                my @blocks =  @{$deptree{'blocked'}};
-                my @union = ();
-                my @isect = ();
-                my %union = ();
-                my %isect = ();
-                foreach my $b (@deps, @blocks) { $union{$b}++ && $isect{$b}++ }
-                @union = keys %union;
-                @isect = keys %isect;
-                if (@isect > 0) {
-                    my $both;
-                    foreach my $i (@isect) {
-                       $both = $both . GetBugLink($i, "#" . $i) . " ";
-                    }
-
-                    ThrowUserError("dependency_loop_multi",
-                                   { both => $both });
-                }
-            }
-            my $tmp = $me;
-            $me = $target;
-            $target = $tmp;
-        }
-    }
+    # Gather the dependency list, and make sure there are no circular refs
+    my %deps = Bugzilla::Bug::ValidateDependencies($cgi->param('dependson'),
+                                                   $cgi->param('blocked'),
+                                                   $id);
 
     #
     # Start updating the relevant database entries
@@ -1567,7 +1507,7 @@ foreach my $id (@idlist) {
                                                   undef, $id)};
         @dependencychanged{@oldlist} = 1;
 
-        if (defined $cgi->param('dependson')) {
+        if (defined $cgi->param($target)) {
             my %snapshot;
             my @newlist = sort {$a <=> $b} @{$deps{$target}};
             @dependencychanged{@newlist} = 1;