679547 - Add the ability to create a new bug as a sighting of another bug

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Robert Webb]

We'll need schema changes to allow us to flag a bug as a sighting of another bug.

We'll need support methods in the Bug module that will allow us to create a new bug as a sighting of another bug.

We'll need support in the WebService that exposes these new creation options and support methods.

Comment 1

[Robert Webb]

Attachment: proposed patch v1

Created a master_bug_id column on the bugs table with a foreign key restraint to the bugs.bug_id column. Implemented a validator to make sure you cannot create a sighting of a sighting or mark a master bug a sighting of another master bug. Added a method that will return all of the sightings associated with a bug; currently this is an unsorted result, however, in the future we may want to do some default sorting (like on bug id). Tested these methods using the WebService.

Comment 2

[Max Kanat-Alexander]

Comment on attachment 555207 [details] [diff] [review]
proposed patch v1

Review of attachment 555207 [details] [diff] [review]:
-----------------------------------------------------------------

Looking pretty good, overall. In particular, the schema looks good.

You also have to update Bugzilla::Install::DB though. http://www.bugzilla.org/docs/tip/en/html/api/checksetup.html#Modifying_the_Database

And a few comments on the code:

::: Bugzilla/Bug.pm
@@ +1730,5 @@
> +    $bug_id || ThrowCodeError('undefined_field', { field => 'bug_id' });
> +
> +    my $bug = Bugzilla::Bug->check({ id => $bug_id });
> +
> +    # If the master_bug_id is unchanged, we have nothing to check.

It can't possibly change, so you don't need this code for right now.

@@ +1736,5 @@
> +        if (ref $invocant && $invocant->master_bug_id &&
> +            $invocant->master_bug_id == $bug_id);
> +
> +    # Make sure the user can view the bug and edit the product
> +    $bug->check_is_visible;

check() did that for you.

@@ +1737,5 @@
> +            $invocant->master_bug_id == $bug_id);
> +
> +    # Make sure the user can view the bug and edit the product
> +    $bug->check_is_visible;
> +    Bugzilla->user->can_edit_product($bug->{product_id});

Actually, that is just a boolean check. Look at other places where that check is done.

Also, we really should have a check_for_edit in Bugzilla::Bug. Could you file a separate bug for writing that if we don't have one already?

@@ +1739,5 @@
> +    # Make sure the user can view the bug and edit the product
> +    $bug->check_is_visible;
> +    Bugzilla->user->can_edit_product($bug->{product_id});
> +
> +    # Make sure this bug isn't a master bug.

You're creating a new bug and haven't added set_master_bug_id, so you don't need this code.

@@ +1745,5 @@
> +        ThrowUserError("sighting_of_sighting_not_allowed");
> +    }
> +
> +    # Make sure the master bug isn't already a sighting.
> +    if (defined $bug->master_bug()) {

No () after a method that just represents a property of the object.

@@ +3712,5 @@
>  
> +#
> +# Bug Sightings related accessors
> +
> +sub master_bug {

I actually try to keep this in alphabetical order, believe it or not. :-) Look up and you'll find most of the accessors are alphabetical.

@@ +3716,5 @@
> +sub master_bug {
> +    my ($self) = @_;
> +    return $self->{master_bug} if exists $self->{master_bug};
> +    return undef if $self->{error} || !defined $self->{master_bug_id};
> +    $self->{master_bug} = Bugzilla::Bug->check({id => $self->{master_bug_id}});

Oh, you *definitely* don't want to call check() here; that will throw an error when there is no value! :-) You just want new.

In general, a simpler pattern is:

my ($self) = @_;
return undef if ($self->{error} || !defined($self->{master_bug_id});
$self->{master_bug} ||= new Bugzilla::Bug($self->{master_bug_id});
return $self->{master_bug};

It eliminates one line (and we write a lot of these accessors, so one line eliminated adds up to a lot of easier reading. :-) )

@@ +3725,5 @@
> +    my ($self) = @_;
> +    return [] if $self->{error};
> +    if (!defined $self->{sightings}) {
> +        $self->{sightings} = Bugzilla::Bug->match({master_bug_id => $self->id});
> +        Bugzilla::Bug->preload($self->{sightings});

Why are you calling preload? In general this should not be done inside of module code, it should only be done in performance-critical places where it becomes obvious that it's needed.

::: Bugzilla/WebService/Bug.pm
@@ +821,4 @@
>          id               => $self->type('int', $bug->bug_id),
>          is_confirmed     => $self->type('boolean', $bug->everconfirmed),
>          last_change_time => $self->type('dateTime', $bug->delta_ts),
> +        master_bug_id    => $self->type('int', $bug->master_bug_id),

Ah, you'll want to document this and add it to the History section.

@@ +884,5 @@
>          $item{'see_also'} = \@see_also;
>      }
> +    if (filter_wants $params, 'sightings') {
> +        my @blocks = map { $self->type('int', $_->id) } @{ $bug->sightings };
> +        $item{'sightings'} = \@blocks;

I'm wondering if we shouldn't call this sighting_ids instead, in case we want to add a full "sightings" item that includes all the bug data, in the future.

 Also, don't call the variable @blocks.

Comment 3

[Max Kanat-Alexander]

Oh, and by the way, you are in fact getting a sorted list out of match(), because of LIST_ORDER. (See _do_list_select in Bugzilla::Object for details.)

Comment 4

[Robert Webb]

Attachment: proposed patch v2

Attaching new revision of the patch with the required changes made.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 557675 [details] [diff] [review]
proposed patch v2

Review of attachment 557675 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/Bug.pm
@@ +1736,5 @@
>  
> +sub _check_master_bug_id {
> +    my ($invocant, $bug_id) = @_;
> +    $bug_id = trim($bug_id);
> +    $bug_id || ThrowCodeError('undefined_field', { field => 'bug_id' });

I think check will do that for you, so you don't need to do it manually.

@@ +1741,5 @@
> +
> +    my $bug = Bugzilla::Bug->check_for_edit({ id => $bug_id });
> +
> +    # Make sure the master bug isn't already a sighting.
> +    if (defined $bug->master_bug) {

Just check master_bug_id, that's faster.

@@ +1745,5 @@
> +    if (defined $bug->master_bug) {
> +        ThrowUserError("sighting_of_sighting_not_allowed");
> +    }
> +
> +    return $bug_id;

Ah, no, you should return $bug->id. We always want to return exactly what the DB gave us as validated, just in case.

@@ +3415,4 @@
>      return \@comments;
>  }
>  
> +sub master_bug {

And now I think we don't need this method, anymore.

@@ +3416,5 @@
>  }
>  
> +sub master_bug {
> +    my ($self) = @_;
> +    return undef if $self->{error} || !defined $self->{master_bug_id};

If this method stays, it's probably best to put parens around that if condition. Also, it's definitely wrong--as you have it now, this will always return undef! :-)

::: Bugzilla/DB/Schema.pm
@@ +296,5 @@
>              deadline            => {TYPE => 'DATETIME'},
>              alias               => {TYPE => 'varchar(20)'},
> +            master_bug_id       => {TYPE => 'INT3',
> +                                    REFERENCES => {TABLE  => 'bugs',
> +                                                   COLUMN => 'bug_id'}},

My only concern is SQLite--would you make sure that this doesn't die somehow on SQLite? It's pretty easy to set up a SQLite Bugzilla, just change the $db_driver in localconfig to "sqlite" and make sure you have DBD::SQLite installed according to checksetup.

::: Bugzilla/Install/DB.pm
@@ +658,5 @@
>  
> +    # 2011-08-29 [e-mail] - Bug 679547
> +    $dbh->bz_add_column('bugs', 'master_bug_id',
> +                        {TYPE => 'INT3', REFERENCES => {TABLE  => 'bugs',
> +                                                        COLUMN => 'bug_id'}});

Ah, don't add the REFERENCES in bz_add_column. There's other magic that handles that for you automatically. :-)

::: Bugzilla/WebService/Bug.pm
@@ +868,4 @@
>                         @{ $bug->see_also };
>          $item{'see_also'} = \@see_also;
>      }
> +    if (filter_wants $params, 'sighting_ids') {

You know, actually, I've changed my mind. We should make this consistent with "blocks" and "depends_on", and just call it "sightings".

@@ +1698,5 @@
>  C<dateTime> When the bug was last changed.
>  
> +=item C<master_bug_id>
> +
> +C<int> The unique numeric id of the master bug associated with this bug.

"of the master bug that this bug is a sighting of. C<undef> if this bug is not a sighting."

@@ +1745,5 @@
>  C<string> The current severity of the bug.
>  
> +=item C<sightings>
> +
> +C<array> of C<int>s Contains the bug ids of all of the bug's sightings.

C<array> of C<int>s The numeric ids of all this bug's sightings.

@@ +1895,4 @@
>  C<target_milestone>, C<update_token>, C<url>, C<version>, C<whiteboard>,
>  and all custom fields.
>  
> +=item In Bugzilla B<5.0> C<sighting_ids> and C<master_bug_id> were added to

And that will need to be switched back to C<sightings>.

Comment 6

[Robert Webb]

Attachment: proposed patch v3

Made the suggested changes as well as tested the schema changes for SQLite on landfill. All runs fine.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 558874 [details] [diff] [review]
proposed patch v3

Beautiful!

Comment 8

[Max Kanat-Alexander]

Approved for SIGHTINGS BRANCH.

Comment 9

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/sightings/                   
modified Bugzilla/Bug.pm
modified Bugzilla/Field.pm                                                     
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Install/DB.pm
modified Bugzilla/WebService/Bug.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 7957.

Comment 10

[Frédéric Buclin]

The sightings branch has never been merged with upstream. Not sure what the resolution/milestone for this bug should be, but certainly not 5.0.