Bug 406111

LOC Estimation:

Bugzilla/IssueTrackerList.pm: 448
data/params: 7
process_bug.cgi: 41
XMLRPC tests needed: 3 methods * 20 lines of code avg = 60
Module API tests needed: 24 methods * 20 lines of code avg: 480
template lines of code: 80
Bugzilla/Bug.pm: 20
Bugzilla/Search.pm: 10
Bugzilla/BugMail.pm: 15
long_list.cgi: 2
globals.pl: 12
defparams.pl: 46
colchange.cgi: 4
CGI.pl: 2
Bugzilla/DB/Schema.pm: 10

Total LOC: 1237
 

It must keep in sync Status, Priority and Severity if it doesn't already.

(In reply to comment #2)
> It must keep in sync Status, Priority and Severity if it doesn't already.

see bug 406461 for tracking this issue

Created attachment 295481 [details]
Issue tracker integration (v1)

I have completed a patch to add IssueTracker support to rh_bugzilla_3. I have
done it as a Bugzilla extension which keeps the code (for the most part)
separate from the main code base.

I have also updated IssueTrackerList.pm to send any bug_severity and priority
changes along with the normal change set to IssueTracker. IssueTracker will
need
to have support on its end to recognize when a severity or priority change has
occurred and update its report. Also IssueTracker will need to use the
appropriate update{Severity,Priority} XMLRPC call when a change occurs on it's
side as well.

Worked 12 hours

Created attachment 295565 [details]
Issue tracker integration (v2)

Some fixes as well as added in missing support for

1. Adding of Issue Tracker ids when creating a new bug
2. Sending bug changes to issuetracker.

Please review. Let me know if you have any questions as extension support is
new ground for all of us :)

Dave

Created attachment 295587 [details]
Issue tracker integration (v3)

All previous changes plus added ability to change issuetracker params using the
editparams.cgi UI.

Q1) rh_bugzilla_3/Bugzilla/Template/Plugin/Hook.pm

Did you add these changes or were they upstream? My concern is that you are 
altering their $template var which may have unforseen consequences.

Q2) post_bug.cgi

You added a hook 'get_bug_fields' which adds the issuetracker field to the 
@bug_fields array but we have also an extension here 406151 which is adding 
other custom fields. Can we merge these two?

Also, I see that custom fields is adding fields to the @bug_fields array. 
Which makes me wonder if we shouldn't be using them?

Q3) Those hooks you added to the pm files, are they likely to be accepted 
upstream? I guess they will push back and say we should be using custom 
fields.

(In reply to comment #8)
> Q1) rh_bugzilla_3/Bugzilla/Template/Plugin/Hook.pm
> 
> Did you add these changes or were they upstream? My concern is that you are 
> altering their $template var which may have unforseen consequences.
>

Bug 318205 – the path to the hook is incorrect when called from inside a block
https://bugzilla.mozilla.org/show_bug.cgi?id=318205 

> Q2) post_bug.cgi
> 
> You added a hook 'get_bug_fields' which adds the issuetracker field to the 
> @bug_fields array but we have also an extension here 406151 which is adding 
> other custom fields. Can we merge these two?
>
> Also, I see that custom fields is adding fields to the @bug_fields array. 
> Which makes me wonder if we shouldn't be using them?
> 

I thought about using custom fields for this but it would take similar amount of
work as we would still need to add hooks to various places to validate the issue
ids that are entered, send changes to issuetracker when bugs are changed that
have issue ids attached, and filter the issuetracker diffs for bugs activity
view and email notifications. Also there will need to be an import from the
standard bugzilla->issuetracker mapping tables to the custom fields tables when
we move the database. That is not that hard but with the Hooks we are able to
leave the database schema as is.

> Q3) Those hooks you added to the pm files, are they likely to be accepted 
> upstream? I guess they will push back and say we should be using custom 
> fields.

I am going to see what I can do. I tried to keep it as generic as possible and
put it in places where we could add new functionality easily in the future. So
they may take some of them.

http://www.bugzilla.org/docs/3.0/html/cust-hooks.html
" If there is no hook at the appropriate place within the Bugzilla source file
or template you want to extend, file a bug requesting one, specifying: the
source or template file for which you are requesting a hook; where in the file
you would like the hook to be placed (line number/position for latest version of
the file in CVS or description of location); the purpose of the hook; a link to
information about your extension, if any."

Hi Dave,

sounds reasonable on all points.

ACK

Created attachment 295999 [details]
Issue tracker integration (v4)

Same patch as v3 except created new code file
extensions/issuetracker/code/quote_urls.pl and added
Hook::process('quote_urls') in Bugzilla::Template::quoteUrls. quote_urls.pl
handles the linkification(sp) of the Issue Tracker links in bug comments.

Please review.

Dave

Created attachment 297255 [details]
Issue tracker integration using custom fields (v1)

I have reimplemented issue tracker id support using custom fields instead of
having separate redhat only mapping tables. I had to extend custom field
support
to allow for a freetext field for multiple integer values. I will try to get
this enhancement submitted upstream as I feel this could be useful for others.
 
Pros:

1. Allows us to use the standard backend code for adding/removing/changing
values in the fields instead of using complete separate custom code.
2. Can add other fields similar in the future as needed.

Cons: 

1. Looses support for attaching a score to each issue tracker ID although Lisa
says it may not be needed anymore anyway.

Currently I am just validating ids that are added against issuetracker to make
sure they are valid and not attached to another bug already. I have not yet
implemented the actually sending of changes to issuetracker yet.

Please take a look.
Dave

Bugzilla/DB/Schema.pm - should this also have a FK to bugs.bug_id?

I think this should be pushed upstream asap. I'm sure it will change under 
review. This functionality would be generally useful, think of the blocks & 
depends on fields.

(In reply to comment #13)
> Bugzilla/DB/Schema.pm - should this also have a FK to bugs.bug_id?

Will add.
 
> I think this should be pushed upstream asap. I'm sure it will change under 
> review. This functionality would be generally useful, think of the blocks & 
> depends on fields.

Exactly.

Already filed: https://bugzilla.mozilla.org/show_bug.cgi?id=287334

I will create a sterilized patch and submit upstream

Dave

Created attachment 298010 [details]
Issue tracker integration using custom fields (v2)

Added webservice.pl for bugzilla.addIT, bugzilla.deleteIT, and
bugzilla.issueExists. Mostly the same as v1 everywhere else. Please take a
look. Will install on bugdev.devel.redhat.com/bugzilla for function testing.

Dave

Created attachment 298220 [details]
 Issue tracker integration using custom fields (v3)

Fixed some issues with sendChanges() to Issue Tracker. This patch seems to give
similar functionality now to 2.18 finally.

Thanks
Dave

Bugzilla/DB/Schema.pm
 - should there by a CASCADE clause there too? tfu?

Bugzilla/DB.pm
 - I'm a little curious about this one. Let's talk on the phone.

Bugzilla/Field.pm
 - probably wiser to add a FILE_TYPE_LAST so you don't have to rewrite those

     detaint_natural($type) && $type < FIELD_TYPE_LAST)    
   But yeh, it's the upstream code so we're already messing on the wrong
   side of the pond.


Bugzilla/Bug.pm _check_freetext_multi_integer
 - this is my take. note the sort using <=> to correctly sort for numbers.

sub _check_freetext_multi_integer {
    my ($invocant, $value, $field) = @_;

    return [] if !$value;

    my %value;
    foreach my $v (split(/[,\s]+/, $value)) {
        my $orig = trim($v);
        detaint_natural($v)
            || ThrowUserError('field_invalid_value',
                { value => $orig, field => $field, type => 'integer' });
        $value{$v} = 1; # removes duplicates
    }

    my @values = sort { $a <=> $b } keys %value;

    # REDHAT EXTENSION START 406111
    Bugzilla::Hook::process('check_freetext_multi_integer',
        { current => $invocant->$field, field => $field, values => 
\@values });
    # REDHAT EXTENSION END 406111

    return \@values;
}

(In reply to comment #17)
> Bugzilla/DB/Schema.pm
>  - should there by a CASCADE clause there too? tfu?

Good idea. Added.

> Bugzilla/DB.pm
>  - I'm a little curious about this one. Let's talk on the phone.

We talked :)

> Bugzilla/Field.pm
>  - probably wiser to add a FILE_TYPE_LAST so you don't have to rewrite those
> 
>      detaint_natural($type) && $type < FIELD_TYPE_LAST)    
>    But yeh, it's the upstream code so we're already messing on the wrong
>    side of the pond.
>

Good idea. Added.
 
> 
> Bugzilla/Bug.pm _check_freetext_multi_integer
>  - this is my take. note the sort using <=> to correctly sort for numbers.
> 
> sub _check_freetext_multi_integer {
>     my ($invocant, $value, $field) = @_;
> 
>     return [] if !$value;
> 
>     my %value;
>     foreach my $v (split(/[,\s]+/, $value)) {
>         my $orig = trim($v);
>         detaint_natural($v)
>             || ThrowUserError('field_invalid_value',
>                 { value => $orig, field => $field, type => 'integer' });
>         $value{$v} = 1; # removes duplicates
>     }
> 
>     my @values = sort { $a <=> $b } keys %value;
> 
>     # REDHAT EXTENSION START 406111
>     Bugzilla::Hook::process('check_freetext_multi_integer',
>         { current => $invocant->$field, field => $field, values => 
> \@values });
>     # REDHAT EXTENSION END 406111
> 
>     return \@values;
> }

Added as well.