242318 - "blocked" field is ignored in post_bug.cgi if the "dependson" field isn't present

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Dave Miller [:justdave]]

I wasted an entire day failing to get the bugzilla-submit script to work because
of this....  :)

If I pass a blank "Dependson:" line in my bug template, then "Blocked:" takes. 
If I only pass "Blocked:" it ignores it.

post_bug.cgi appears to be only checking if "dependson" is there before doing
any dependency processing.

Comment 1

[Christian Reis]

Attachment: kiko_v1: trivial and untested

If you verify this works, land it :-)

Comment 2

[Frédéric Buclin]

Attachment: patch, v1

Fix the problem for both post_bug.cgi and process_bug.cgi. Now each field is
treated independently. The validation routine is moved into Bug.pm as this
routine does exactly the same job in both files.

Comment 3

[Christian Reis]

Comment on attachment 188465 [details] [diff] [review]
patch, v1

>Index: post_bug.cgi
> foreach my $field ("dependson", "blocked") {
>-    if (UserInGroup("editbugs") && defined($cgi->param($field)) &&
>-        $cgi->param($field) ne "") {
>+    if (UserInGroup("editbugs") && $cgi->param($field)) {

I belive checking if defined is correct because the field may have been changed
to be empty.

>+if (UserInGroup("editbugs")){
>+    %deps = Bugzilla::Bug::validate_dependencies($cgi, undef);
> }

I would rather the Bugzilla::Bug function not receive a $cgi argument, but
instead only what it needed. Why are you supplying undef as the ID? This makes
for confusing APIs. Change the function signature to be:

  Bugzilla::Bug::ValidateDependencies($bug_id, $dependson, $blocked)

instead.

>-    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) = split("/", $pair);

Can't you use a set of nested lists here to make this better? I think this is
possible by using
a list of list references -- check on #perl if you are as unsure as I am.

Similar comments apply to process_bug. This patch is clean and shouldn't be too
hard to check for regressions.

Comment 4

[Christian Reis]

(In reply to comment #3)
> (From update of attachment 188465 [details] [diff] [review] [edit])
> >Index: post_bug.cgi
[...]
> I belive checking if defined is correct because the field may have been 
> changed to be empty.

Silly me, this is not applicable to post_bug, of course. It /is/ applicable to
process_bug, however, though it appears you cater for it correctly there.

Comment 5

[Frédéric Buclin]

(In reply to comment #3)
> I belive checking if defined is correct because the field may have been changed
> to be empty.

I agree that the field may be empty (or bug 0 is given). That's why I tested
this point too. Later in the code, it checks for defined($cgi->()) and use an
empty hash, as expected. I played a lot to make sure this was correct.


> I would rather the Bugzilla::Bug function not receive a $cgi argument, but
> instead only what it needed. Why are you supplying undef as the ID? This makes
> for confusing APIs. Change the function signature to be:
> 
>   Bugzilla::Bug::ValidateDependencies($bug_id, $dependson, $blocked)

Flag.pm and FlagType.pm also receive $cgi as argument when validating flags and
this does not hurt anybody. ;) 'undef' is required in post_bug.cgi, because the
routine has to check that you don't try to mark this bug as depending or
blocking itself. As the bug does not exist yet, undef is required.


> Can't you use a set of nested lists here to make this better?

This is the "trick" used in process_bug.cgi. Aren't you the guy who told me a
few minutes ago that you wanted as many "copy paste" as possible?? :-p


> Similar comments apply to process_bug. This patch is clean and shouldn't be too
> hard to check for regressions.

I did around 10 tests, with and without editbugs privs, adding and removing
deps, doing mass-change, etc... All tests are successful.

Comment 6

[Christian Reis]

I'm pretty adamant on Bugzilla::Bug::ValidateDependencies($bug_id, $dependson,
$blocked) -- I don't want Bugzilla::Bug to have /any/ $cgi content or it makes
the file unusable by non-web executables. Flag and FlagType are either wrong or
will never be used outside of the web -- this isn't the case for Bugzilla::Bug.

Yeah, I said copy-n-paste is ideal, but that's taken out of context -- it's only
when you're doing a massive cut-n-paste from a file to a library that it's
ideal. If you're changing the functionality here, I'd suggest at least trying to
use nested lists -- duplicating the split is going the wrong way, and I'd like
to avoid it if possible.

Comment 7

[Frédéric Buclin]

(In reply to comment #6)
> I'm pretty adamant on Bugzilla::Bug::ValidateDependencies($bug_id, $dependson,
> $blocked)

I agree to change the routine to take only these two arguments.


> ideal. If you're changing the functionality here, I'd suggest at least trying to
> use nested lists -- duplicating the split is going the wrong way, and I'd like
> to avoid it if possible.

You prefer the actual solution??? I don't see what's wrong with mine. myk and I
spent a lot of time thinking about the best solution, and this one is the one we
considered as acceptable.


Would you r+ my patch if I only change the arguments the routine gets?

Comment 8

[Christian Reis]

You mean three arguments, I imagine? I understand that when post_bugging bug_id
will be undef.

I'd prefer the splitting code did this:

  foreach my $pair (["blocked", "dependson"] , ["dependson", "blocked"]) {
    my ($me, $target) = @{$pair};

This is simpler code and avoids abusing strings to do what we want with nested
lists.

Comment 9

[Frédéric Buclin]

Attachment: patch, v2

the routine now requires 3 arguments and uses nested lists.

Comment 10

[Christian Reis]

Comment on attachment 188473 [details] [diff] [review]
patch, v2

Index: Bugzilla/Bug.pm
>+# Validate and return a hash of dependencies
>+sub validate_dependencies($$$) {

Aren't functions in this file CamelCased? 

>+    my %deps;
>+    if (defined($fields->{'dependson'})
>+        || defined($fields->{'blocked'}))
>+    {

I'd rather we checked if neither was defined and return early here. You save
a whole level of indentation.

>+        my %deptree;

I'm assuming everything under here is cut-n-paste and doesn't require serious
scrutiny. Warn me if it does.

Were there differences between the relevant code in post and process_bug?

>Index: process_bug.cgi
>@@ -1567,7 +1506,7 @@
>                                                   undef, $id)};
>         @dependencychanged{@oldlist} = 1;
> 
>-        if (defined $cgi->param('dependson')) {
>+        if (defined $cgi->param($target)) {

I don't understand this change. Oh, actually, I do. Yuck, what a horrible bug.
Why was this the was it was?

r+ kiko if you fix the trivialities above

Comment 11

[Frédéric Buclin]

(In reply to comment #10)

> I'd rather we checked if neither was defined and return early here. You save
> a whole level of indentation.

I consider this as a nit and does not need to be fixed. Tell me if I'm wrong.


> Were there differences between the relevant code in post and process_bug?

Mainly copy paste, except for ThrowUserError("dependency_loop_single") which
wasn't required in post_bug.cgi.


> I don't understand this change. Oh, actually, I do. Yuck, what a horrible bug.
> Why was this the was it was?

Because the one who wrote this part assumed that if the "blocked" field exists,
then the "dependson" field exists too, which is wrong for customized templates
or hacked URLs.
The reason to check whether these fields are defined or not is because
mass-change does not allow you to change dependencies, and consequently the
absence of these fields must not be assimilated to "remove all deps".

Comment 12

[Christian Reis]

(In reply to comment #11)
> > I'd rather we checked if neither was defined and return early here. You save
> > a whole level of indentation.
> 
> I consider this as a nit and does not need to be fixed. Tell me if I'm wrong.

Well, I'd prefer to see it done -- it's always a good idea to return early from
functions. I usually indicate [nit] when I'm nitpicking; here's it's just me
suggesting good programming practice.

Thanks for your explanation, this is a nice cleanup.

Comment 13

[Frédéric Buclin]

Attachment: patch, v3

"nits" fixed. :-p

Comment 14

[Christian Reis]

Comment on attachment 188484 [details] [diff] [review]
patch, v3

Looks good. I haven't tested this but since you have and most of the code is
either cut-n-paste or mine <wink> r=kiko.

Remember this review the next time you have a bad day.

Comment 15

[Frédéric Buclin]

(In reply to comment #14)
> Remember this review the next time you have a bad day.

No comment... Thanks anyway (for the review)!

Comment 16

[Frédéric Buclin]

Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.117; previous revision: 1.116
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.261; previous revision: 1.260
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.80; previous revision: 1.79
done