Bug 427883 - XMLRPC functions getBug() and getBugSimple() to be replaced by Bug.get()
Summary: XMLRPC functions getBug() and getBugSimple() to be replaced by Bug.get()
Keywords:
Status: CLOSED NEXTRELEASE
Alias: None
Product: Bugzilla
Classification: Community
Component: WebService
Version: 3.2
Hardware: All
OS: Linux
high
medium
Target Milestone: ---
Assignee: Noura El hawary
QA Contact:
URL:
Whiteboard:
Depends On:
Blocks: RHBZ30UpgradeTracker 406131 427053
TreeView+ depends on / blocked
 
Reported: 2008-01-08 01:16 UTC by Noura El hawary
Modified: 2013-06-24 04:18 UTC (History)
0 users

Fixed In Version:
Clone Of:
Environment:
Last Closed: 2008-03-18 20:35:23 UTC
Embargoed:


Attachments (Terms of Use)
Bugzilla::WebService::Bug::get() (6.10 KB, patch)
2008-03-06 14:15 UTC, Noura El hawary
no flags Details | Diff
Bugzilla::WebService::Bug::get() (6.66 KB, patch)
2008-03-10 13:58 UTC, Noura El hawary
dkl: review-
Details | Diff
Bugzilla::WebService::Bug::get() (7.41 KB, patch)
2008-03-11 02:22 UTC, Noura El hawary
no flags Details | Diff


Links
System ID Private Priority Status Summary Last Updated
Mozilla Foundation 413770 0 None None None Never

Description Noura El hawary 2008-01-08 01:16:27 UTC
This is one of the highly used functions in rh_bugzilla_2_18 , in the upstream
they have partially similar function to it called get_bugs. the function will be
reworked to be compatible with the upstream.

in rh_bugzilla_2_18 the function is at: Bugzilla::RPC::Bug::getBug()

in the upstream partially similar one is: Bugzilla::WebService::Bug::getBug()

Comment 1 Kevin Baker 2008-01-08 14:51:38 UTC
You mean that the Red Hat Bugzilla::RPC::Bug::getBug will be replaced with the 
upstream Bugzilla::WebService::Bug::get_bugs right?

Comment 2 Noura El hawary 2008-01-11 07:36:39 UTC
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

Comment 3 Noura El hawary 2008-01-12 07:12:19 UTC
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.

Comment 4 David Lawrence 2008-01-17 21:40:42 UTC
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

Comment 5 Noura El hawary 2008-01-24 02:41:08 UTC
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

Comment 6 Noura El hawary 2008-01-24 02:44:08 UTC
*** Bug 427891 has been marked as a duplicate of this bug. ***

Comment 7 Noura El hawary 2008-03-06 14:15:44 UTC
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

Comment 8 Noura El hawary 2008-03-10 13:58:27 UTC
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 9 David Lawrence 2008-03-10 22:34:04 UTC
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

Comment 10 Noura El hawary 2008-03-11 02:22:00 UTC
Created attachment 297556 [details]
Bugzilla::WebService::Bug::get()

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

Noura

Comment 11 David Lawrence 2008-03-18 20:33:43 UTC
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 12 David Lawrence 2008-03-27 18:55:11 UTC
Comment on attachment 297556 [details]
Bugzilla::WebService::Bug::get()

Clearing review flag since this is has been postponed.


Note You need to log in before you can comment on or make changes to this bug.