Closed Bug 242318 Opened 21 years ago Closed 19 years ago

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

Categories

(Bugzilla :: Creating/Changing Bugs, defect)

2.17.7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 2.20

People

(Reporter: justdave, Assigned: LpSolit)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Assignee: myk → LpSolit
Blocks: 174988
Status: NEW → ASSIGNED
Target Milestone: --- → Bugzilla 2.20
No longer blocks: 174988
Depends on: 174988
Blocks: 174988
No longer depends on: 174988
Attached patch kiko_v1: trivial and untested (obsolete) — Splinter Review
If you verify this works, land it :-)
Attachment #188453 - Flags: review?(LpSolit)
Attached patch patch, v1 (obsolete) — Splinter Review
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.
Attachment #188453 - Attachment is obsolete: true
Attachment #188465 - Flags: review?
Attachment #188453 - Flags: review?(LpSolit)
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.
Attachment #188465 - Flags: review? → review-
(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.
(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.
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.
(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?
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.
Attached patch patch, v2 (obsolete) — Splinter Review
the routine now requires 3 arguments and uses nested lists.
Attachment #188465 - Attachment is obsolete: true
Attachment #188473 - Flags: review?(kiko)
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
Attachment #188473 - Flags: review?(kiko) → review-
(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".
(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.
Attached patch patch, v3Splinter Review
"nits" fixed. :-p
Attachment #188473 - Attachment is obsolete: true
Attachment #188484 - Flags: review?(kiko)
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.
Attachment #188484 - Flags: review?(kiko) → review+
(In reply to comment #14) > Remember this review the next time you have a bad day. No comment... Thanks anyway (for the review)!
Flags: approval?
Flags: approval? → approval+
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
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Blocks: 271023
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: