99205 - Allow mass-editing of dependencies

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement, P1)

Description

[Gagan]

Need a way to group select a bunch of bugs and create a dependency (meta) bug
from them. We probably need to extend this as a field in the options for mass
changes.

Comment 1

[Matthew Tuck [:CodeMachine]]

Sounds like bug #42101.

Should not provide any more support than that - should be encouraging use of
keywords over trackers, that's what they're for - and so should be fixing
keywords system.

Comment 2

[Asa Dotzler [:asa]]

Keywords and keyword queries are a hack that doesn't work for almost everything
I want. Dependencty and dependency queries are almost exactly what I need with
only a few minor changes. I believe the path to making tracking bugs work for my
needs is much shorter than the path to making keywords work for my needs. 

There are about 4 bugs (see bug 99207 blockers) fixes for which would make
tracking bugs work wonderfully for me. Bug 83058 is nearly fixed and bug 99209
is already possible possible but a bit inconvenient. This bug (which is really
probably bug 42101 as you suggest) and the ability to reference a bug with an
alias are the only other changes that need fixing in order to make metabugs
server all of my needs. 

Comment 3

[Matthew Tuck [:CodeMachine]]

I'm not going to argue the old keyword vs. tracker issue here again, all that
can be said has been, I'm just going to shut up and get around to fixing
keywords so trackers can go away.

Comment 4

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

Re-targeting to Bugzilla 2.18 milestone, since I plan to work on these bugs
during that timeframe if not sooner.

Comment 5

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

Attachment: patch v1: implements feature

This patch implements mass changes of dependencies with options for adding and
removing dependencies from multiple bugs simultaneously as well as making the
dependency list equal an exact list for a set of bugs.	UI implementations are
included for both the current buglist.cgi and the new one being developed in
bug 103778, and process_bug.cgi has been hacked to handle the new dependency
capabilities.

Comment 6

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

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

Comment 7

[Jouni Heikniemi]

Comment on attachment 63822 [details] [diff] [review]
patch v1: implements feature

Bitrotten pre-templatization stuff -> needs-work.

Comment 8

[Dave Miller [:justdave]]

enhancements without a current patch are being pushed to 2.20

Comment 9

[Dave Miller [:justdave]]

Bugzilla 2.20 feature set is now frozen as of 15 Sept 2004.  Anything flagged
enhancement that hasn't already landed is being pushed out.  If this bug is
otherwise ready to land, we'll handle it on a case-by-case basis, please set the
blocking2.20 flag to '?' if you think it qualifies.

Comment 10

[Max Kanat-Alexander]

Wow, I can't believe we don't already allow this.

Comment 14

[David Lawrence [:dkl]]

We have functionality for this in our current version of Bugzilla 3.2. I can attach a patch of how we are doing it for review.

Comment 15

[David Lawrence [:dkl]]

Attachment: Patch to add multi edit for dependencies (v1)

Attaching a patch that we currently use at bugzilla.redhat.com that allows dependencies to be edited from the multiple bug edit form. Currently we only allow adding and deleting as some users mistakenly wiped out entire dependency lists when using make exact and most did not think it necessary. Could be added
back if needed.

Dave

Comment 16

[David Lawrence [:dkl]]

Attachment: Patch to add multi edit for dependencies (v2)

New patch that removed redhat specific comments.

Dave

Comment 17

[Max Kanat-Alexander]

Comment on attachment 338936 [details] [diff] [review]
Patch to add multi edit for dependencies (v2)

I'm concerned that it would be possible to create dependency loop with this code. You're modifying many bugs at once, and they don't get committed until the end, so a loop could exist when committed that never existed during the checks.

I can't actually create a scenario on paper where this could happen, but do you have any more solid reassurance than just my drawings about it?

Comment 18

[David Lawrence [:dkl]]

(In reply to comment #17)
> (From update of attachment 338936 [details] [diff] [review])
> I'm concerned that it would be possible to create dependency loop with this
> code. You're modifying many bugs at once, and they don't get committed until
> the end, so a loop could exist when committed that never existed during the
> checks.
> 
> I can't actually create a scenario on paper where this could happen, but do you
> have any more solid reassurance than just my drawings about it?

The test cases that I have tried have all thrown proper dependency loop errors as soon as it hits one in the list so all changes would be rolled back.

We have had this in place for quite a few years, previously with 2.18, and have no reported cases of dependency loop issues.

Dave

Comment 19

[Max Kanat-Alexander]

Comment on attachment 338936 [details] [diff] [review]
Patch to add multi edit for dependencies (v2)

  Okay, I think this is OK, then, but:

>+    if (defined $cgi->param('dependson') 

  You should call should_set('dependson') I believe, there.

>+        && $cgi->param('dependson_action') =~ /^(add|remove)$/) {

  Nit: opening braces of multi-line if statements need to line up with the 'i' in "if".

>+        foreach my $bug (@bug_objects) {
>+            my %seen_deps;
>+            map { $seen_deps{$_} = 1 } @{$bug->dependson};
>+            foreach my $id (split(/[,\s+]+/, $cgi->param('dependson'))) {

  You definitely don't need the + twice in that regex, and you might not need either +, actually.

>+                $seen_deps{$id} = 1 if $cgi->param('dependson_action') eq 'add';
>+                delete $seen_deps{$id} if $cgi->param('dependson_action') eq 'remove';

  Nit: Just make this an if/else that defaults to add.

>+            $bug->set_dependencies(join(' ', keys %seen_deps), join(' ', @{$bug->blocked})); 

  Nit: You could also make set_dependencies accept arrays in addition to strings--I would be happier with that.

>+    if (defined $cgi->param('blocked') 

 You could avoid code duplication if you used a "foreach my $type (qw(blocked dependson)) {" above instead of having a separate block for "blocked".

 This is also fairly important as loops may not be reported correctly if you don't set both blocked and dependson at the same time. (I think they'll still be caught, but the error message might be confusing.)

Comment 20

[David Lawrence [:dkl]]

Attachment: Patch to add multi edit for dependencies (v3)

Updated patch for mass edit dependencies.

1. Use should_set() for dependson and blocked.
2. Fixed regex typo
3. Updated Bugzilla::Bug::_check_dependencies to take either strings or array
references.
4. Less code duplication.

Let me know what you think.
Dave

Comment 21

[Max Kanat-Alexander]

Comment on attachment 354326 [details] [diff] [review]
Patch to add multi edit for dependencies (v3)

>+                my @set_args = $type eq 'dependson' 
>+                    ? ([ keys %temp_deps ], $bug->blocked) 
>+                    : ($bug->dependson, [ keys %temp_deps ]);
>+                $bug->set_dependencies(@set_args); 

 Ah, you misunderstood, slightly, one of my comments. The qw(blocked dependson) bit should go inside the foreach bug loop, and then you should set both dependson and blocked at the same time, in one set_dependencies.


>Index: Bugzilla/Bug.pm
[snip]
>-        my @bug_ids = split(/[\s,]+/, $deps_in{$type});
>+        my @bug_ids = ref($deps_in{$type}) =~ /ARRAY/ 

  Just do ref, no need to check if it's an array.

  Also, you might want to do this higher up in the code? Hmm, maybe not, I dunnow.

Comment 22

[David Lawrence [:dkl]]

Attachment: Patch to add multi edit for dependencies (v4)

Another variation based on mkanat's suggestions. Also added a conditional around the whole thing so we don't call set_dependencies on all bugs if not needed. Removed the check for ref =~ /ARRAY/ in Bugzilla::Bug::_check_dependencies.

Maybe am getting close ;)

Dave

Comment 23

[Max Kanat-Alexander]

Comment on attachment 354335 [details] [diff] [review]
Patch to add multi edit for dependencies (v4)

Yeah, this looks good to me. :-)

Comment 24

[David Lawrence [:dkl]]

tip:
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.415; previous revision: 1.414
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.271; previous revision: 1.270
done
Checking in template/en/default/list/edit-multiple.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/list/edit-multiple.html.tmpl,v  <--  edit-multiple.html.tmpl
new revision: 1.54; previous revision: 1.53
done

Comment 25

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.4 in bug 494037.