Closed Bug 24496 Opened 21 years ago Closed 17 years ago

Forbid resolving fixed when there're unresolved dependencies.


(Bugzilla :: Creating/Changing Bugs, enhancement, P2)




Bugzilla 2.18


(Reporter: CodeMachine, Assigned: andreas.hoefler)



(Whiteboard: docs info in comment 40)


(3 files, 8 obsolete files)

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
Priority: P3 → P2
Hmm, not a bad idea.  And maybe this would help reduce some of the abuse of
dependencies that I see... is the new owner of Bugzilla and Bonsai.  (For details,
see my posting in netscape.public.mozilla.webtools,
news:// .)
Assignee: terry → tara
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.)
QA Contact: matty
Whiteboard: 3.4
moving to real milestones...
Target Milestone: --- → Future
Severity: minor → enhancement
Moving to new Bugzilla product ...
Assignee: tara → myk
Component: Bugzilla → Creating/Changing Bugs
Product: Webtools → Bugzilla
Version: other → unspecified
Whiteboard: 3.4 → 3.4 [relations:depend] [flow:status,resolution] consistency
*** Bug 158653 has been marked as a duplicate of this bug. ***
Attached patch patch to add dependency check (obsolete) — Splinter Review
Adds a parameter to
Adds checks for unresolved dependencies in 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
Attachment #131668 - Attachment description: Adds parameter and checks to forbid resolving bugs with unresolved dependencies → patch to add dependency check
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! :-)

>> # 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

>>         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
>>   [% 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 :)
Attachment #131668 - Flags: review-
Attached patch patch to add dependency check V2 (obsolete) — Splinter Review
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.
Attachment #131668 - Attachment is obsolete: true
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.
Attachment #131673 - Flags: review+
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>.
Assignee: myk → andreas.hoefler
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
Attachment #131673 - Attachment is obsolete: true
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.
Attachment #131677 - Flags: review+
Flags: approval?
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).
Flags: approval? → approval-
Comment on attachment 131677 [details] [diff] [review]
patch to add dependency check V2.1

r- (see comment 18)
Attachment #131677 - Flags: review-
Depends on: 219605
Anyone care to provide some better wording for the param?
Target Milestone: Future → Bugzilla 2.18
'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 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.
Attachment #131677 - Attachment is obsolete: true
Attached patch patch for multiple bugs V 1.1 (obsolete) — Splinter Review
If from changing multiple bugs only one was blocked, the Bug ID was not
Attachment #131941 - Attachment is obsolete: true
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.
Attached patch patch for multiple bugs V 1.2 (obsolete) — Splinter Review
Check is now only done, if resolution should become 'FIXED'.
Attachment #131942 - Attachment is obsolete: true
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


>+#  - 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"?
Attached patch patch for multiple bugs V 1.3 (obsolete) — Splinter Review
Optical and comment fixes
Attachment #131945 - Attachment is obsolete: true
Comment on attachment 131954 [details] [diff] [review]
patch for multiple bugs V 1.3

This patch doesn't apply...
patching file
Hunk #1 succeeded at 1169 (offset 30 lines).
patching file
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:
Attachment #131954 - Flags: review?(justdave) → review-
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. :)


Anyone have ideas for a better name, or should we just go with that?
Attachment #131954 - Attachment is obsolete: true
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 ;)
Flags: approval?
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.
Flags: approval? → approval+

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


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;
/cvsroot/mozilla/webtools/bugzilla/,v  <--
new revision: 1.126; previous revision: 1.125
Checking in;
/cvsroot/mozilla/webtools/bugzilla/,v  <--
new revision: 1.258; previous revision: 1.257
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.201; previous revision: 1.200
Checking in template/en/default/global/user-error.html.tmpl;
 <--  user-error.html.tmpl
new revision: 1.51; previous revision: 1.50
Closed: 17 years ago
Resolution: --- → FIXED
This needs coverage in the documentation...  both in section 3.1 and somewhere
in section 5.
Flags: documentation?
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 
Resolution: FIXED → ---
Checking in template/en/default/global/user-error.html.tmpl;
 <--  user-error.html.tmpl
new revision: 1.52; previous revision: 1.51
Closed: 17 years ago17 years ago
Resolution: --- → FIXED
Flags: documentation2.18?
Whiteboard: 3.4 [relations:depend] [flow:status,resolution] consistency → docs info in comment 40
Attached patch add the new param to the docs (obsolete) — Splinter Review
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
Attachment #163172 - Flags: review?(documentation)
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.
Attachment #163172 - Flags: review?(documentation) → review+
I can't checkin, so here are the nits fixed, if that is easier...
Attachment #163172 - Attachment is obsolete: true
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.
Attachment #166789 - Flags: review+
(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  <-- 
new revision: 1.43; previous revision: 1.42

Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <-- 
new revision:; previous revision:
Flags: documentation?
Flags: documentation2.18?
Flags: documentation2.18+
Flags: documentation+
QA Contact: matty_is_a_geek → default-qa
You need to log in before you can comment on or make changes to this bug.