Forbid resolving fixed when there're unresolved dependencies.

RESOLVED FIXED in Bugzilla 2.18

Status

()

enhancement
P2
normal
RESOLVED FIXED
20 years ago
7 years ago

People

(Reporter: CodeMachine, Assigned: andreas.hoefler)

Tracking

unspecified
Bugzilla 2.18
Dependency tree / graph
Bug Flags:
approval +
documentation +
documentation2.18 +

Details

(Whiteboard: docs info in comment 40)

Attachments

(3 attachments, 8 obsolete attachments)

Reporter

Description

20 years ago
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.

Updated

20 years ago
Status: NEW → ASSIGNED
Priority: P3 → P2

Comment 1

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

Comment 2

20 years ago
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 .)
Assignee: terry → tara
Status: ASSIGNED → NEW

Comment 3

19 years ago
Oooh.  Yes, this is the right thing to do.
Status: NEW → ASSIGNED

Comment 4

19 years ago
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.)
Reporter

Updated

19 years ago
QA Contact: matty
Whiteboard: 3.4

Comment 5

19 years ago
moving to real milestones...
Target Milestone: --- → Future
Severity: minor → enhancement
Reporter

Comment 6

18 years ago
Moving to new Bugzilla product ...
Assignee: tara → myk
Status: ASSIGNED → NEW
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. ***
Assignee

Comment 8

16 years ago
Posted patch patch to add dependency check (obsolete) — Splinter Review
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
Assignee

Comment 9

16 years ago
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 10

16 years ago
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

16 years ago
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-
Assignee

Comment 12

16 years ago
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.
Assignee

Updated

16 years ago
Attachment #131668 - Attachment is obsolete: true

Comment 13

16 years ago
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+

Comment 14

16 years ago
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
Assignee

Comment 15

16 years ago
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.

Assignee

Comment 16

16 years ago
minor text changes
Attachment #131673 - Attachment is obsolete: true

Comment 17

16 years ago
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+

Updated

16 years ago
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-

Updated

16 years ago
Depends on: 219605
Anyone care to provide some better wording for the param?
Target Milestone: Future → Bugzilla 2.18
No longer depends on: 219605
Assignee

Comment 21

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

Comment 22

16 years ago
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."
Assignee

Comment 23

16 years ago
Yeah, I knew it:

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

Tried "Change multiple bugs at once".
Assignee

Comment 24

16 years ago
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...
Assignee

Comment 25

16 years ago
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
Assignee

Comment 26

16 years ago
Posted 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
visible.
Attachment #131941 - Attachment is obsolete: true

Comment 27

16 years ago
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.
Assignee

Comment 28

16 years ago
Posted 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 29

16 years ago
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"?
Assignee

Comment 30

16 years ago
Posted 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 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:
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. :)

"noresolveonopenblockers"

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?

Comment 35

16 years ago
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+
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
Status: NEW → RESOLVED
Closed: 16 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 
# --ERROR
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
Flags: documentation2.18?
Whiteboard: 3.4 [relations:depend] [flow:status,resolution] consistency → docs info in comment 40

Comment 44

15 years ago
Posted 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
comment.

Updated

15 years ago
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+

Comment 46

15 years ago
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?

Comment 49

15 years ago
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

Updated

15 years ago
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.