It annoys me how people go about marking things fixed when there are dependencies that are unresolved. You could certainly resolve this as WONTFIX, INVALID, etc, but something can't possibly be fixed in this case. Either the person should resolve the dependencies (if they're real deps) or remove them (if not). Of course it can be legitimate for a resolved fixed bug to depend on an open bug, but this only occurs if the dependant bug is fixed before the dependee is reopened.

Hmm, not a bad idea. And maybe this would help reduce some of the abuse of dependencies that I see...

tara@tequilarista.org is the new owner of Bugzilla and Bonsai. (For details, see my posting in netscape.public.mozilla.webtools, news://news.mozilla.org/38F5D90D.F40E8C1A%40geocast.com .)

Oooh. Yes, this is the right thing to do.

Having "fixed" dependencies can be useful in cases where a workaround is written for a bug that depends on an underlying bug, because the "this bug blocks" list makes it easy to see what workarounds should be removed when the underlying bug is fixed. (I'm not sure that dependencies are used this way often, but I think they are.)

moving to real milestones...

Moving to new Bugzilla product ...

*** Bug 158653 has been marked as a duplicate of this bug. ***

Adds a parameter to defparams.pl Adds checks for unresolved dependencies in globals.pl and process_bug.cgi Adds new user error when there are unresolved dependencies

Comment on attachment 131668 [details] [diff] [review] patch to add dependency check shortened patch description

Comment on attachment 131668 [details] [diff] [review] patch to add dependency check It would be nice if you could provide a unified diff (use diff -u), as it's easier to read and apply (apart from being the standard! :-) >Index: globals.pl >> >> # CountOpenDependencies tells if the bug depends on other open bugs. This comment is a bit misleading -- it returns how many open bugs this bug depends on. I guess it would be best to change it. >> sub CountOpenDependencies { >> my ($bug_num) = @_; >> my $openCount = 0; >> detaint_natural($bug_num) || die "HasOpenDependencies() called with non-integer bug number"; Hmmm. Isn't FORM{'id'} already detainted at this point? Anyway, this is a good assertion. >> SendSQL("SELECT bugs.bug_status " . >> "FROM bugs , dependencies " . >> "WHERE blocked = $bug_num " . >> "AND bug_id = dependson"); >> >> # fetch all results and set >> while (MoreSQLData()) { >> my ($bug_state) = FetchSQLData(); >> >> if (IsOpenedState($bug_state)) { >> $openCount = $openCount+1; >> } >> } Would it be possible to avoid this check by using a different query that already returned the count of open bugs? >Index: default/global/user-error.html.tmpl >=================================================================== >RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v >retrieving revision 1.38 >diff -r1.38 user-error.html.tmpl >620a621,625 >> [% ELSIF error == "still_unresolved_bugs" %] >> [% terms.Bug %] still blocked by [% dependency_count %] other [% terms.bugs %]. I would rephrase that slightly: This bug is still blocked by N other open bugs. Looks okay apart from that.

Comment on attachment 131668 [details] [diff] [review] patch to add dependency check <a href="showdependencytree.cgi?id=[% bug_id FILTER html %]"> wrong FILTER :)

Worked in all suggestions and error-corrections (sorry, I'm a bit ill). Left in the check for bug_id being valid for security reasons.

Comment on attachment 131673 [details] [diff] [review] patch to add dependency check V2 >Index: process_bug.cgi > >+ >+ # don't checkin when still unresolved bugs Err.. don't close, or don't commit, right? (hmm, 'checkin' on your mind? ;-) >Index: template/en/default/global/user-error.html.tmpl > >+ Check open [% terms.bugs %] in <a href="showdependencytree.cgi?id=[% bug_id %]"> >+ dependency tree</a>. Nit: I would change this to the simple phrase: <a>View dependency tree</a> (less words, and the verb is linked). This are only nits; r=kiko.

Err, let me go ahead and ask if you think that the phrase This bug is still blocked by N other open bugs. doesn't convey the actual problem? If so, you can use: This bug can't be resolved as fixed: it is still blocked by N other open bugs. To display the open bugs, you can <a>view the dependency tree</a>.

My patch prevents resolving at all, independend of the resolution, if there are unresolved dependencies. My opinion: If you want to set it FIXED, there should be no open dependencies. In any other case (INVALID, etc.), there shouldn't even be any dependencies, right? If there ara, you have to rethink the connection between the bugs.

minor text changes

Comment on attachment 131677 [details] [diff] [review] patch to add dependency check V2.1 Hmmm. This means that a blocker bug can't be RESOLVED LATER either. I'm not sure if this is a good or bad thing, since I dislike LATER, but since I agree with the code and your justification, r=kiko. I'd like to know the opinion of the approver, too.

The param description (and maybe the name) needs some polish before this can go in. Otherwise I like it. But the description on the param just doesn't flow right (and it requires thought to figure out what it means).

Comment on attachment 131677 [details] [diff] [review] patch to add dependency check V2.1 r- (see comment 18)

Anyone care to provide some better wording for the param?

'Whether or not resolving a bug with unresolved dependencies bugs should be forbidden.' ?

If it has dependencies, the depencies are "bugs" by default, so you don't need to mention the word again. Also, I think that "forbidden" is a little strange. How about: "Whether or not resolving a bug with unresolved dependencies should be allowed." Or (if you want to state things more actively): "Don't allow bugs to be resolved if they have unresolved dependencies."

Yeah, I knew it: ... CountOpenDependencies() called with non-integer bug number at globals.pl line 1080. ... Tried "Change multiple bugs at once".

Well, the check for open dependencies is outside of the loop, which processes every single bug if changing multiple bugs at once. What should the desired behaviour be, if we resolve multiple bugs at once, and one of them still has open dependencies? I think, in that case we should prevent resolving any of the bugs and the user should uncheck the resolve-blocking bug. Nevertheless, I'll have to rework the check from checking only a single bug to checking a list of bugs. Which result should CountOpenDependencies give back in the new version? I think, it should return a list of the bugs with open dependencies and to each bug the number of open dependencies. Then the error-msg should give all bugs with unresolved dependencies as well as a link to the dependency-tree for each bug. I'm not very experienced with handling arrays in Perl, so this could take some more time than the last patches...

Reworked patch to check for a buglist instead of a single bug. Error message distinguishes between single and several blocked bugs.

If from changing multiple bugs only one was blocked, the Bug ID was not visible.

I'd still like to revisit comment 15 about requirements with certain resolutions. Some opinions follow: When resolving as... FIXED: Dependencies should be required to be fixed INVALID: This is debatable. I'd go for not requiring blockers to be resolved, as "INVALID" tends to mean "not a bug" and other such things. A dependency setting for a "not a bug" might well be correct, but the report is still not considered to be invalid (say, somebody makes a "<option selected> doesn't work in XHTML" bug depend on "Make attribute parsing work properly in XHTML". That could be an entirely valid dependency, yet the <option selected> bug is clearly invalid.). Requiring people to clear dependencies on this kind of situations can lead to spam. WONTFIX: Blocker resolves shouldn't be required. Wontfix doesn't mean the dependencies aren't set correctly; it just means the bug won't be fixed. WORKSFORME: No requirement should be set here either. WFM is close to wontfix in its role: if you can't reproduce a bug, you can't fix it - but this doesn't mean the blocker relations wouldn't be quite justified. If you suddenly _can_ repro the bug, then you're likely to reopen it anyway. DUPLICATE: Special logic for handling dependency transitions wrt duplicate bugs would probably be in order, but that's outside the scope of this bug. It's probably best not to require anything anyway. About WONTFIX, INVALID, WFM: We should also consider that fact that frequently bugs with those resolutions get reopened. If clearing the dependencies before resolving is required, then somebody will have to readd the dependencies after reopening the bug.

Check is now only done, if resolution should become 'FIXED'.

Comment on attachment 131945 [details] [diff] [review] patch for multiple bugs V 1.2 Some comments freshly out of the oven :-) >+# CountOpenDependencies counts the number of open dependend bugs for a "dependent" >+# - A list of bug numbers, which have to be checked "A list of bug numbers whose dependencies are to be checked" or something along those lines might be more descriptive. >+sub CountOpenDependencies { ... >+ return @dependencies; Umm, doesn't this actually return the dependency array instead of the count (which one would expect from comments and the sub name?) >+ # don't resolve as fixed while still unresolved bugs What? "unresolved blocking bugs" perhaps? >+ my @dependencies = CountOpenDependencies(@idlist); >+ if (scalar @dependencies > 0) { >+ ThrowUserError("still_unresolved_bugs", { dependencies => \@dependencies, >+ dependency_count => scalar @dependencies }); >+ } Your indentation is a bit off here (extra space after "my @...", and the line length is slightly too long for my tastes :-) >+ [% terms.Bug %]# <a href="show_bug.cgi?id=[% dependencies.0.bug_id %]">[% dependencies.0.bug_id %]</a> >+ has still [% dependencies.0.dependencies FILTER html %] unresolved dependencies. Show >+ <a href="showdependencytree.cgi?id=[% dependencies.0.bug_id %]">Dependency Tree</a>. >+ [% ELSE %] >+ There are [% dependency_count %] open [% terms.bugs %] which >+ have unresolved dependencies. Doesn't this code print out things like "1 unresolved dependencies"?

Optical and comment fixes

Comment on attachment 131954 [details] [diff] [review] patch for multiple bugs V 1.3 This patch doesn't apply... patching file defparams.pl Hunk #1 succeeded at 1169 (offset 30 lines). patching file globals.pl Hunk #1 succeeded at 1069 (offset 2 lines). patching file process_bug.cgi Hunk #1 succeeded at 891 (offset 1 line). patching file template/en/default/global/user-error.html.tmpl patch: **** malformed patch at line 136:

the line counts were goofy on the last hunk of that patch. I got it to apply by changing the header to: @@ -618,6 +618,32 @@ patching file template/en/default/global/user-error.html.tmpl Hunk #1 succeeded at 666 with fuzz 1 (offset 48 lines). Installed it on landfill and played around... It works, and this is nice and simple. :)

This is identical to attachment 131954 [details] [diff] [review] except that the corrupted patch has been fixed, and it's been brought up to the tip. I like this, and I think it's ready to go, except for one thing.... I'm not sure I like the name of the param. :) "noresolveonopenblockers" Anyone have ideas for a better name, or should we just go with that?

I'm going to ask Myk to do approval on this one on UI grounds. :) If you think the param name works, go ahead and approve it ;)

How about "requirefixedblockers" (which is not exactly correct and is a bit overgeneric, but no sane admin would probably misunderstand it)? "nofixedwithopenblockers" (or the negation "allowfixedwithopenblockers")? "requireblockageintegrity"? ;-)

I think the current name is good enough. It's not great, but neither are any of the alternatives suggested, and I can't come up with anything better at the moment. Configuration parameter names tend to be cryptic anyway in Bugzilla, since we don't use underscores to separate words. Also, the description explains the purpose of the parameter well, making it less important that the name be spot on.

requireblockersclosedonresolve? Can we start putting underscores or hyphens in parameter names? :-) Gerv

>requireblockersclosedonresolve? Also OK, possibly little better, but not better enough to go back now. >Can we start putting underscores or hyphens in parameter names? :-) We already have both underscores and hyphens in some. We should convert them all over to one style rather than just adding new ones differently. Underscores are the way to go, since they are (or could be) used in code.

Thanks Andreas! Checking in defparams.pl; /cvsroot/mozilla/webtools/bugzilla/defparams.pl,v <-- defparams.pl new revision: 1.126; previous revision: 1.125 done Checking in globals.pl; /cvsroot/mozilla/webtools/bugzilla/globals.pl,v <-- globals.pl new revision: 1.258; previous revision: 1.257 done Checking in process_bug.cgi; /cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v <-- process_bug.cgi new revision: 1.201; previous revision: 1.200 done Checking in template/en/default/global/user-error.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v <-- user-error.html.tmpl new revision: 1.51; previous revision: 1.50 done

This needs coverage in the documentation... both in section 3.1 and somewhere in section 5.

Tinderbox is burning after this checkin.... not ok 70 - (en/default) template/en/default/global/user-error.html.tmpl has unfiltered directives: # 671: dependencies.0.bug_id # 671: dependencies.0.bug_id # 678: dependencies.0.bug_id # 680: dependency_count # 684: bug.bug_id # 684: bug.bug_id # 691: bug.bug_id # --ERROR

Checking in template/en/default/global/user-error.html.tmpl; /cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v <-- user-error.html.tmpl new revision: 1.52; previous revision: 1.51 done

This adds the param to the list of params in section 3 of the docs. I couldn't really see anywhere to add this in section 5, as Dave mentioned in an earlier comment.

Comment on attachment 163172 [details] [diff] [review] add the new param to the docs Doc nits: 1) First instance of 'fixed' should be capitalized to match the last two, since they're all talking about a resolution. 2) Stuff between <para> </para> needs to be indented two more spaces to match documentation formatting. These can both be fixed up on checkin.

I can't checkin, so here are the nits fixed, if that is easier...

Comment on attachment 166789 [details] [diff] [review] add the new param to the docs v2 - nits fixed This one should be ready to apply to the tree without any nit-fixing... good job Gavin.

(In reply to comment #46) > I can't checkin, so here are the nits fixed, if that is easier... Vlad: My checkin privs are still in the works, so I can't do it myself yet either. Can you please commit this to the tree now that it has passed review?

Checking in docs/xml/administration.xml; /cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v <-- administration.xml new revision: 1.43; previous revision: 1.42 done Checking in docs/xml/administration.xml; /cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v <-- administration.xml new revision: 1.34.2.7; previous revision: 1.34.2.6 done