781853 - Move GenerateTree function into Bugzilla::Bug

Status: NEW

(Bugzilla :: Dependency Views, enhancement)

Description

[Albert Ting]

I think showdependencytree.cgi's GenerateTree() should be moved into Bugzilla::Bug.   Reason is that I'd like provide a recursive bug list url from the main show_bug.cgi, but that's not possible as the code is only accessible from showdependencytree.cgi.  

Any reason why this wouldn't work?

Comment 1

[Simon Green]

Attachment: v1 patch

First subroutine in Bugzilla::Bug with some actual POD documentation, but it is definitely needed. Stands out like a sore thumb, but at least it is a start.

Comment 2

[Frédéric Buclin]

Comment on attachment 704196 [details] [diff] [review]
v1 patch

>=== modified file 'Bugzilla/Bug.pm'

>+sub dependency_tree {

>+    if ($relationship ne 'dependson' && $relationship ne 'blocked') {
>+        ThrowUserError('dependency_tree_bad_relationship');

Must be ThrowCodeError() as the user cannot set this parameter itself.


>+    $args->{depth} = 0                    if not defined $args->{depth};
>+    $args->{bugs} = {$self->id => $self } if not defined $args->{bugs};
>+    $args->{id} = { }                     if not defined $args->{id};

We require Perl 5.10.1 or better, which means that you can write:

  $args->{foo} //= $bar;

which is much cleaner. Also, the last line should have 'ids' as key, not 'id'.


>+    my $depth         = ++$args->{depth};

Any reason to increase $depth and $args->{depth} here? When comparing your code with the existing one, they seem to behave differently (I admit I didn't test your patch yet, but this is a guess from what I can see).


>+    my $hide_resolved = $args->{hide_resolved} // 0;

For realdepth and maxdepth and this one, please use || 0 instead of // 0, in case it's defined but set to the empty string.


>+    # Don't do anything if this bug doesn't have any dependencies.
>+    return $args unless scalar(@dependencies);

I don't see the point to return $args as it's already altered in place.


>+        if (!$args->{bugs}{$dep_id}) {

$args->{bugs} is a hashref, so it must be $args->{bugs}->{$dep_id}.


>+        if (!$args->{bugs}{$dep_id}->{'error'}

Same here and elsewhere.


>+            # Due to AUTOLOAD in Bug.pm, we cannot add 'dependencies'
>+            # as a bug object attribute from here.

AUTOLOAD is gone since Bugzilla 4.2.


>+Gives a dependency tree of bugs that the user can see which either blocks or
>+depends on this bug. The $args hashref is modified by this method.

s/blocks/block/ and s/depends/depend/. Also, remove the last sentence. It's not the right place to talk about technical details.


>+A string, either 'dependson' or 'blocked'. The first will return bugs that
>+depending on this bug. Blocked returns bugs that bug this bug.

s/depending/depend/ and s/that bug/which block/


>+A hashref that is modified by the method. Can contain the following keys,
>+all are optional.

Replace the 2nd sentence by "It contains the following keys:"


>+The bug object with an extra key of 'dependencies.

'bugs' is not an object, but a hashref of bug objects.


>+=item ids
>+
>+A hashref of all dependent or blocked bugs with the bug id as the keys
>+and the bug object as the values.

That's not true. The bug object is not stored in this hash. Only its bugs ID.


>+=item maxdepth (read only)
>+=item hide_resolved (read only)

Remove (read only).


>+=item Returns
>+
>+This same hashref as the $args hashref in the input

It's shouldn't return anything, as $args is already updated.

Comment 3

[Simon Green]

Attachment: v2 patch

Comment 4

[Simon Green]

I'm only going to reply to the things that I didn't fix.

(In reply to Frédéric Buclin from comment #2)
> >=== modified file 'Bugzilla/Bug.pm'
> >+    my $depth         = ++$args->{depth};
> 
> Any reason to increase $depth and $args->{depth} here? When comparing your
> code with the existing one, they seem to behave differently (I admit I
> didn't test your patch yet, but this is a guess from what I can see).

The original code (from showdependencytree.cgi) passes depth as a parameter to GenerateTree and when called recursively calls $depth + 1. Given that we just call $args, I thought it would make sense to do it here.
 
> >+    # Don't do anything if this bug doesn't have any dependencies.
> >+    return $args unless scalar(@dependencies);
> 
> I don't see the point to return $args as it's already altered in place.

It allows you to do this

my $args = $bug->dependency_tree('blocks');

which is in line with what most other subs do. Modifying the sub is just something that has to be done.
 
> >+        if (!$args->{bugs}{$dep_id}) {
> 
> $args->{bugs} is a hashref, so it must be $args->{bugs}->{$dep_id}.

It doesn't have to be, but I have changed it anyway. 

> >+=item Returns
> >+
> >+This same hashref as the $args hashref in the input
> 
> It's shouldn't return anything, as $args is already updated.

See above.

Regards,
Hugo

Comment 5

[Simon Green]

Attachment: Second v2 patch

Comment 6

[Frédéric Buclin]

Comment on attachment 717819 [details] [diff] [review]
Second v2 patch

Sorry for taking so long to review your patch. Releasing Bugzilla 4.4 was my top priority these last few weeks.


>=== modified file 'Bugzilla/Bug.pm'

>+sub dependency_tree {

>+            $args->{ids}{$dep_id} = 1;

Please write $args->{ids}->{$dep_id} for consistency.


>+=item relationship
>+
>+A string, either 'dependson' or 'blocked'. The first will return bugs that
>+depend on this bug. Blocked returns bugs which block this bug.

That's exactly the opposite. :)

dependson = bugs on which this bug depends
blocked = bugs blocked by this bug


>+=item bugs
>+
>+A hashref of bug object

objects (plural)


>+=item realdepth
>+
>+The maximum deep of the dependency tree

Specify that this argument is used internally only and must not be set by the caller.


>+=item hide_resolved
>+
>+Don't return closed bugs in the results

Start the sentence with: "If set to true, ..."



>=== modified file 'showdependencytree.cgi'

>+$vars->{'dependson_ids'} = [keys(%{$dependson_info->{ids}})];

Nit: To make the code slightly lighter, you could remove parens around %{}.


>+$vars->{'blocked_ids'} = [keys(%{$blocked_info->{id}})];

They key is 'ids', not 'id'.
Nit: same comment as above about parens.


When testing your patch, I realize that if I set maxdepth > 0, the 2nd tree (bugs blocked) only returns bugs from the first level, and all other levels are skipped. I didn't check where this bug comes from.

Otherwise looks good.

Comment 7

[Simon Green]

Attachment: v3 patch

Comment 8

[Frédéric Buclin]

Comment on attachment 771935 [details] [diff] [review]
v3 patch

(I like to know who is really asking me to review his patches...)

Comment 9

[Frédéric Buclin]

Comment on attachment 771935 [details] [diff] [review]
v3 patch

Bitrot due to bug 917370.