Bug 427883

You mean that the Red Hat Bugzilla::RPC::Bug::getBug will be replaced with the 
upstream Bugzilla::WebService::Bug::get_bugs right?

Hi Kevin,

Yes I think we can replave getBug with get_bugs , only we will have to give the
bug id in an array as we can give one id or a list ,, also with get_bugs in the
upstream this is an example of its output:

                      'summary' => 'this is atest bug',
                      'internals' => {
                                     'priority' => 'P3',
                                     'bug_id' => '2',
                                     '_multi_selects' => [],
                                     'bug_file_loc' => '',
                                     'cclist_accessible' => '1',
                                     'rep_platform' => 'All',
                                     'product_id' => '1',
                                     'creation_ts' => '2008.01.08 16:19',
                                     'assigned_to' => '2',
                                     'everconfirmed' => '1',
                                     'qa_contact' => '',
                                     'short_desc' => 'assa',
                                     'status_whiteboard' => '',
                                     'bug_severity' => 'normal',
                                     'bug_status' => 'NEW',
                                     'delta_ts' => '2008-01-08 17:25:54',
                                     'version' => 'unspecified',
                                     'reporter_id' => '1',
                                     'component_id' => '1',
                                     'reporter_accessible' => '1',
                                     'resolution' => '',
                                     'target_milestone' => '---',
                                     'alias' => '',
                                     'op_sys' => 'All'
                                   },
                      'creation_time' => '20080108T16:19:00',
                      'last_change_time' => '20080108T17:25:54',
                      'id' => '2',
                      'alias' => ''

from the upstream API POD the internal section of the return is UNSTABLE so we
can change and add the other things that we return from getBugSimple and getBug
like the keywords for example. 

Noura

I think we need to discuss if we are going to replace both getBug() and
getBugSimple() with the upstream function get_bugs() , basically our getBug()
returns more comprehensive information than our getBugSimple(), but getBug()
returns the bug details in User View format that is exactly similar to what the
user see in show_bug.cgi only that it is textual. My suggestion would be to use
the upstream get_bugs() to replace both functions and add to the return list all
the extra info that we need like keywords, groups etc, unless the User View
returned by getBug() is required by some people in redhat.

Filed bug upstream to rename Bug.get_bugs() to Bug.get() since the word bug
twice is redundant and not part of a consistent method naming scheme.

https://bugzilla.mozilla.org/show_bug.cgi?id=412850

Dave

LOC estimation:

This function exist upstream as Bugzilla::WebService::Bug::get() ,, few
modification will be done to it to enable it to return all the fields that are
needed by the caller like groups, comments, keywords etc. I have submitted a
patch for that upstream, added it to the external bugzilla links. 

LOC to update the function will be about: 20 LOC
LOC for xmlrpc test case: 200 LOC

total LOC = 220

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

Created attachment 297050 [details]
Bugzilla::WebService::Bug::get()

Hey Dave,

I re-thought about the Bug.get() function and I think the right behaviour would
be:

1- if the user doesn't specify the fields they want in return then they get the
stable bug fields returned + the internals hash , the internals hash should
have all the bug information that they can get just as how our 2.18 getBug
returns.

2- if they specify the fields they want then they get the stable bug fields
returned + the fields they specified.

I have made a comparison with what getBug returned and what Bug.get returns and
added all the extra fields in getBug to the internals hash in Bug.get

Please review the patch and let me know what you think.

Thanks,
Noura

Created attachment 297432 [details]
Bugzilla::WebService::Bug::get()

As Agreed with Dave ,, this is now how Bug.get() works:

1- if the user only passes bug ids, aliases then they get the stable bug info
plus the internals hash with the basic bug info as how they are returned by
upstream code.

2- id the user specifies the fields they want then they get the stable bug info
plus the internals hash the contains the fields they specified.

3- if the user passes the params all set to 1 , then they get the stable bug
info + internals hash with all the bug info that they can get.

Note: for all the returned info I have converted ids to names to be more user
friendly, as in the upstream code they mostly retun ids for product. component
and users.

Please review and let me know what you think.

Thanks,
Noura

Comment on attachment 297432 [details]
Bugzilla::WebService::Bug::get()

>+# REDHAT EXTENSION START 406151
>+# adding redhat custom fields to the mapping starting with cf_*
> use constant FIELD_MAP => {
>-    status      => 'bug_status',
>-    severity    => 'bug_severity',
>-    description => 'comment',
>-    summary     => 'short_desc',
>-    platform    => 'rep_platform',
>+    status              => 'bug_status',
>+    severity            => 'bug_severity',
>+    description         => 'comment',
>+    summary             => 'short_desc',
>+    platform            => 'rep_platform',
>+    comments            => 'longdescs',
>+    url                 => 'bug_file_loc',
>+    milestone_url       => 'url',
>+    qa_whiteboard       => 'cf_qa_whiteboard',
>+    devel_whiteboard    => 'cf_devel_whiteboard',
>+    internal_whiteboard => 'cf_internal_whiteboard',
>+    fixed_in            => 'cf_fixed_in',
>+    cust_facing         => 'cf_cust_facing',
> };
>+# REDHAT EXTENSION END 406151

Eventually we will need to add cf_issuetracker as well when the issuetracker
support finally lands.

>+# now the function can be called with optional hashkey fields as the following:
>+# $call = $rpc->call( 'Bug.get', { ids => [1,3]   , fields => ['status', 'groups', 'keywords', 'comments']} );
>+# the unstable internal section will be replaced with the passed in list of fields.
> sub get {
>     my ($self, $params) = @_;
>     my $ids = $params->{ids};

Old argument. I would still prefer if we can pass in a single id as well as a
list.

      my @ids = ref($params->{ids}) ? @{$params->{ids} : ($params->{ids});

      foreach my $bug_id (@ids) {

>@@ -76,6 +90,7 @@ sub get {
>     foreach my $bug_id (@$ids) {
>         ValidateBugID($bug_id);
>         my $bug = new Bugzilla::Bug($bug_id);
>+        my $b = new Bugzilla::Bug($bug_id);
> 
>         # Timetracking fields are deleted if the user doesn't belong to
>         # the corresponding group.
>@@ -92,10 +107,54 @@ sub get {
>         my %item;
>         $item{'creation_time'}    = type('dateTime')->value($creation_ts);
>         $item{'last_change_time'} = type('dateTime')->value($delta_ts);
>-        $item{'internals'}        = $bug;
>         $item{'id'}               = type('int')->value($bug->bug_id);
>         $item{'summary'}          = type('string')->value($bug->short_desc);
>+        my $internals;
>+ 
>+        # if the user has specified a list of fields that they want
>+        # to get for the bug ids they are passing then return that list 
>+        # instead of the internals section, otherwise if no fields
>+        # are specified then return the internals section
>+        if ($params->{fields}) {
>+            $item{internals} = {};
>+            $internals = $item{internals};
>+            foreach my $field ( @{ $params->{fields} } ) {
>+                my $field_name = FIELD_MAP->{$field} || $field;
>+                $internals->{$field_name} = $bug->$field_name;
>+            }
>+        }
>+        elsif ($params->{all}) {
>+            $item{internals} = $bug;
>+            $internals = $bug;

I am not entirely sure what is happening here. You are placing a copy of the
$bug object into $item{internals} and then you placing another copy of the $bug
object into $internals. But I do not see anywhere where you are
assigning $item{internals} to $internals before you push the %item hash onto
@return.

>+            $internals->{cc} = $b->cc;
>+            $internals->{dependson} = $b->dependson;
>+            $internals->{blocked} = $b->blocked;
>+            $internals->{longdescs} = $b->longdescs;
>+            $internals->{keywords} = $b->keywords;
>+            $internals->{groups} = $b->groups;
>+            $internals->{attachments} = $b->attachments;
>+            $internals->{flag_types} = $b->flag_types;
>+            $internals->{dupe_id} = $b->dup_id;
>+            $internals->{actual_time} = $b->actual_time;
>+            $internals->{milestone_url} = $b->milestoneurl;
>+        }
>+        else {
>+            $item{internals} = $bug;
>+            $internals = $bug;

Same here

>+        }
>+     
>+        # convert ids to names
>+        $internals->{qa_contact} = type('string')->value($b->qa_contact->login) 
>+                                   if ($internals->{qa_contact} && $b->qa_contact);
>+        $internals->{assigned_to} = type('string')->value($b->assigned_to->login) 
>+                                    if $internals->{assigned_to};
>+        $internals->{reporter} = type('string')->value($b->reporter->login) 
>+                                 if $internals->{reporter_id};
>+        $internals->{product} = type('string')->value($b->product) if $internals->{product_id};
>+        $internals->{component} = type('string')->value($b->component) if $internals->{component_id};
>+ 

Should you be assigning $internals to $item{internal} here so that the %item
hash will have all of the requested information?

	  $item{internals} = $internals;

When I run a test script against this patch on
bugdev.devel.redhat.com/bugzilla, I always get the basic
information in the $bug object back for the 'internals' key. It doesn't matter
if I pass in all => 1 or 
specify specific fields, I get the same data every time.

Dave

Created attachment 297556 [details]
Bugzilla::WebService::Bug::get()

Thanks for the review Dave.. Here we go another patch with your suggestions
applied. 

Noura

This feature is included now in Milestone 2. Any bugs found with this feature
should be file as a new report and set to block the 3.2 final release tracker.

Comment on attachment 297556 [details]
Bugzilla::WebService::Bug::get()

Clearing review flag since this is has been postponed.