24496 - Forbid resolving fixed when there're unresolved dependencies.

Status: RESOLVED FIXED

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

Description

[Matthew Tuck [:CodeMachine]]

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.

Comment 1

[Terry Weissman]

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

Comment 2

[Terry Weissman]

[e-mail] 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 .)

Comment 3

[Tara Hernandez]

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

Comment 4

[Jesse Ruderman]

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.)

Comment 5

[Stephan Niemz]

moving to real milestones...

Comment 6

[Matthew Tuck [:CodeMachine]]

Moving to new Bugzilla product ...

Comment 7

[Dave Miller [:justdave]]

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

Comment 8

[Andreas Höfler]

Attachment: patch to add dependency check

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 9

[Andreas Höfler]

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

shortened patch description

Comment 10

[Christian Reis]

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 11

[timeless]

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

<a href="showdependencytree.cgi?id=[% bug_id FILTER html %]">

wrong FILTER :)

Comment 12

[Andreas Höfler]

Attachment: patch to add dependency check V2

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 13

[Christian Reis]

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.

Comment 14

[Christian Reis]

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

Comment 15

[Andreas Höfler]

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.

Comment 16

[Andreas Höfler]

Attachment: patch to add dependency check V2.1

minor text changes

Comment 17

[Christian Reis]

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.

Comment 18

[Dave Miller [:justdave]]

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 19

[Dave Miller [:justdave]]

Comment on attachment 131677 [details] [diff] [review]
patch to add dependency check V2.1

r- (see comment 18)

Comment 20

[Dave Miller [:justdave]]

Anyone care to provide some better wording for the param?

Comment 21

[Andreas Höfler]

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

Comment 22

[Jason Bassford]

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."

Comment 23

[Andreas Höfler]

Yeah, I knew it:

...
CountOpenDependencies() called with non-integer bug number at globals.pl line 1080.
...

Tried "Change multiple bugs at once".

Comment 24

[Andreas Höfler]

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

Comment 25

[Andreas Höfler]

Attachment: Reworked patch to fit for multiple bugs

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

Comment 26

[Andreas Höfler]

Attachment: patch for multiple bugs V 1.1

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

Comment 27

[Jouni Heikniemi]

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.

Comment 28

[Andreas Höfler]

Attachment: patch for multiple bugs V 1.2

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

Comment 29

[Jouni Heikniemi]

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"?

Comment 30

[Andreas Höfler]

Attachment: patch for multiple bugs V 1.3

Optical and comment fixes

Comment 31

[Dave Miller [:justdave]]

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:

Comment 32

[Dave Miller [:justdave]]

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. :)

Comment 33

[Dave Miller [:justdave]]

Attachment: patch for multiple bugs V 1.3 (un-corrupted)

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?

Comment 34

[Dave Miller [:justdave]]

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 ;)

Comment 35

[Jouni Heikniemi]

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"? ;-)

Comment 36

[Myk Melez [:myk] [@mykmelez]]

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.

Comment 37

[Gervase Markham [:gerv]]

requireblockersclosedonresolve?

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

Gerv

Comment 38

[Myk Melez [:myk] [@mykmelez]]

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

Comment 39

[Dave Miller [:justdave]]

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

Comment 40

[Dave Miller [:justdave]]

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

Comment 41

[Dave Miller [:justdave]]

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

Comment 42

[Dave Miller [:justdave]]

Attachment: Additional patch to fix burning tinderbox

Comment 43

[Dave Miller [:justdave]]

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

Comment 44

[GavinS]

Attachment: add the new param to the docs

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 45

[Shane H. W. Travis]

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.

Comment 46

[GavinS]

Attachment: add the new param to the docs v2 - nits fixed

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

Comment 47

[Shane H. W. Travis]

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.

Comment 48

[Shane H. W. Travis]

(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?

Comment 49

[Vlad Dascalu]

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