Bug 427885 - replace XMLRPC function getBugActivity() with Bug.get_activity()
replace XMLRPC function getBugActivity() with Bug.get_activity()
Status: CLOSED NEXTRELEASE
Product: Bugzilla
Classification: Community
Component: WebService (Show other bugs)
3.2
All Linux
high Severity medium (vote)
: ---
: ---
Assigned To: Noura El hawary
:
Depends On:
Blocks: RHBZ30UpgradeTracker 406131 427053
  Show dependency treegraph
 
Reported: 2008-01-07 20:27 EST by Noura El hawary
Modified: 2013-06-24 00:17 EDT (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-03-18 16:41:46 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
get_bug_activity xmlrpc function for upstream bugzilla (866 bytes, text/plain)
2008-01-15 01:14 EST, Noura El hawary
no flags Details
v2 for function Bugzilla::WebService::Bug::get_activity() + POD (3.40 KB, patch)
2008-02-26 22:34 EST, Noura El hawary
no flags Details | Diff
Bugzilla::WebService::Bug::get_activity() (3.36 KB, patch)
2008-02-28 08:16 EST, Noura El hawary
dkl: review-
Details | Diff
Bugzilla::WebService::Bug::get_activity() (3.41 KB, patch)
2008-02-28 23:04 EST, Noura El hawary
no flags Details | Diff


External Trackers
Tracker ID Priority Status Summary Last Updated
Mozilla Foundation 413770 None None None Never

  None (edit)
Description Noura El hawary 2008-01-07 20:27:49 EST
highly used xmlrpc function in rh_bugzilla_2_18 ,, it is in 

Bugzilla::RPC::Bug::getBugActivity()

there is no similar function to it in the upstream.
Comment 1 Noura El hawary 2008-01-15 01:14:35 EST
Created attachment 291686 [details]
get_bug_activity xmlrpc function for upstream bugzilla

This patch has xmlrpc function get_bug_activity converted to match with the
upstream code , this patch works on the upstream bugzilla 3.2
Comment 2 Noura El hawary 2008-01-15 01:18:16 EST
so basically in our rh_bugzilla_2_18 xmlrpc function used to call function
GetBugActivity() in CGI.pl but as CGI.pl doesn't exist in bugzilla 3.2 ,, they
have this function GetBugActivity() located in the related module which is
Bugzilla::Bug so I called that function to do the work for us from the
WebService module Bug.pm
Comment 3 Kevin Baker 2008-01-15 16:54:27 EST
brainstorming the interface:

Is there any reason why this routine could not be folded into get_bugs? I'm 
wondering if getBugActivity, getBug, getBugSimple all can be done via 
get_bugs. Rather than creating a new routine for every little bit of data you 
might need. 

Instead couldn't the get_bug interface for getBugActivity look like

get_bugs( ids => [1, 2,...n], activity => 1 );

And for getBugSimple

get_bugs( ids => [1, 2,...n], columns => 'simple' );
# columns => 'all' is default

And for people who want to tinker

get_bugs( ids => [1, 2,...n], columns => [qw/id status product/] );

Or, getting fancy. Give me the standard response PLUS an extra col

get_bugs( ids => [1,2..n], columns => ['+flags'] )

And the flip, remove a few columns

get_bugs( ids => [1,2..n], columns => [qw/-component -product -verion/] )

But that is just dreaming I guess. :)
Comment 4 David Lawrence 2008-01-15 17:08:27 EST
Sounds good to me on the surface. Except for the rare case that the bug is huge
due to a lot of comments/attachments/etc. and all they wanted was the activity
log. But if in practice it still came back quickly enough I suppose people
wouldn't mind.

I do like the column idea as well.

Dave
Comment 5 David Lawrence 2008-01-15 17:11:36 EST
Oops wrong bug report with the keyword ;)
Comment 6 Kevin Baker 2008-01-15 17:25:28 EST
(In reply to comment #4)
> Sounds good to me on the surface. Except for the rare case that the bug is 
huge
> due to a lot of comments/attachments/etc. and all they wanted was the 
activity
> log. But if in practice it still came back quickly enough I suppose people
> wouldn't mind.

Yeh, depends on what the user wants. Perhaps I just want the basic bug info 
and the activity. Or basic bug info and the comments.

At any rate all my lovely interface design needs to pass thru the upstream 
filter. See what they think.


Comment 7 Noura El hawary 2008-01-23 21:49:25 EST
I started a discussion with the upstream, on weather to include this
functionality in Bug.get() or as a separate function Bug.get_activity() I
submitted a patch for the second case. and will see how the discussion goes.
upstream link in external bugzilla links section
Comment 8 Noura El hawary 2008-02-26 22:34:39 EST
Created attachment 296019 [details]
v2 for function Bugzilla::WebService::Bug::get_activity() + POD

this is new patch for function Bug.get_activity() for now i will make it a
sperate function, not sure what will happen later.

Noura
Comment 9 Noura El hawary 2008-02-26 22:37:05 EST
worked 2 hours code modifications, testing and POD
Comment 10 David Lawrence 2008-02-27 12:27:07 EST
Comment on attachment 296019 [details]
v2 for function Bugzilla::WebService::Bug::get_activity() + POD

>+sub get_activity {
>+    my ($self, $params) = @_;    
>+
>+    my $ids = $params->{ids};
>+    defined $ids || ThrowCodeError('param_required', { param => 'ids' });

I mentioned this in other reviews since I think it is important that we be
flexible in the params that we accept.
So maybe a good idea here would be to do:

      my @ids = ref($params->{ids}) ? @{$params->{ids}} : ($params->{id});
      @ids || ThrowCodeError('param_required', { param => 'ids' });

>+    my %item;
>+    my @return;
>+    foreach my $bug_id (@$ids) {
>+        ValidateBugID($bug_id);
>+        $item{id} = $bug_id;
>+
>+        ( $item{operations}, $item{incomplete_data} ) = Bugzilla::Bug::GetBugActivity($bug_id);    
>+
>+        my $bug = new Bugzilla::Bug($bug_id);
>+        if (Bugzilla->params->{'usebugaliases'}) {
>+            $item{'alias'} = type('string')->value($bug->alias);
>+        }
>+        else {
>+            # For API reasons, we always want the value to appear, we just
>+            # don't want it to have a value if aliases are turned off.
>+            $item{'alias'} = undef;
>+        }

Did upstream ask for the alias to be returned? We do not currently do this in
our API so is this needed?
They already know the bugids when they asked for the bug activity data and they
can also pass in the alias as the ids param if they do not know the bug id. I
don't mind if it stays in I just wonder if we really need it. It would keep
this function very simple to leave that part out.

>+
>+        push(@return, \%item);
>+    }
>+
>+    return { bugs => \@return };  
>+} 
> 
> sub create {
>     my ($self, $params) = @_;
>@@ -367,7 +399,83 @@ You do not have access to the bug_id you
> 
> =back
> 
>+=item C<get_activity> B<EXPERIMENTAL>
>+
>+=over
>+
>+=item B<Description>
>+
>+Gets activity about particular bugs in the database.
>+
>+=item B<Params>
>+
>+=over
>+
>+=item C<ids>
>+
>+An array of numbers and strings.
>+
>+If an element in the array is entirely numeric, it represents a bug_id
>+from the Bugzilla database to fetch. If it contains any non-numeric 
>+characters, it is considered to be a bug alias instead, and the bug with 
>+that alias will be loaded. 
>+
>+Note that it's possible for aliases to be disabled in Bugzilla, in which
>+case you will be told that you have specified an invalid bug_id if you
>+try to specify an alias. (It will be error 100.)
>+
>+=back
>+
>+=item B<Returns>
>+
>+A hash containing a single element, C<bugs>. This is an array of hashes. 
>+Each hash contains the following items:
>+
>+=over
> 
>+=item id
>+
>+C<int> The numeric bug_id of this bug.
>+
>+=item alias
>+
>+C<string> The alias of this bug. If there is no alias or aliases are 
>+disabled in this Bugzilla, this will be an empty string.
>+
>+=item operations
>+
>+C<array> An array of hashes with each hash containing C<when>, which is a 
>+string that holds the date the bug activity/change happened, C<who> which is
>+a string that holds the login name for the person who performed the change,
>+and C<changes> which is an array of hashes that holds the change details.
>+
>+=item incomplete_data
>+
>+C<boolean> Is set to 1 to indicate that there is an old Bugzilla data 
>+corruption bug, otherwise 0.
>+
>+=back
>+
>+=item B<Errors>
>+
>+=over
>+
>+=item 100 (Invalid Bug Alias)
>+
>+If you specified an alias and either: (a) the Bugzilla you're querying
>+doesn't support aliases or (b) there is no bug with that alias.
>+
>+=item 101 (Invalid Bug ID)
>+
>+The bug_id you specified doesn't exist in the database.
>+
>+=item 102 (Access Denied)
>+
>+You do not have access to the bug_id you specified.
>+
>+=back
>+
>+=back
> 
> =item C<create> B<EXPERIMENTAL>
>
Comment 11 David Lawrence 2008-02-27 15:25:41 EST
Comment on attachment 296019 [details]
v2 for function Bugzilla::WebService::Bug::get_activity() + POD

I apologize for the previous comment, forgot to remove the pod doc part from
the comment.

>+        my $bug = new Bugzilla::Bug($bug_id);
>+        if (Bugzilla->params->{'usebugaliases'}) {
>+            $item{'alias'} = type('string')->value($bug->alias);
>+        }
>+        else {
>+            # For API reasons, we always want the value to appear, we just
>+            # don't want it to have a value if aliases are turned off.
>+            $item{'alias'} = undef;

Nit-Pick: Remove the single quotes from around 'alias'.
Comment 12 Noura El hawary 2008-02-28 08:10:03 EST
> I mentioned this in other reviews since I think it is important that we be
> flexible in the params that we accept.
> So maybe a good idea here would be to do:
> 
>       my @ids = ref($params->{ids}) ? @{$params->{ids}} : ($params->{id});
>       @ids || ThrowCodeError('param_required', { param => 'ids' });
> 
adding this to new patch.


> 
> Did upstream ask for the alias to be returned? We do not currently do this in
> our API so is this needed?
> They already know the bugids when they asked for the bug activity data and they
> can also pass in the alias as the ids param if they do not know the bug id. I
> don't mind if it stays in I just wonder if we really need it. It would keep
> this function very simple to leave that part out.
> 

I think that it is proper to return alias as it is returned from Bug.get() as
well, also another reason is that if the user passed more than one bug aliases
then if they get bugs activities returned they need the bug alias to know which
information belongs to which alias they passed. 
Comment 13 Noura El hawary 2008-02-28 08:16:06 EST
Created attachment 296199 [details]
Bugzilla::WebService::Bug::get_activity()

new patch for Bug.get_activity() with Dave's suggestions

Noura
Comment 14 David Lawrence 2008-02-28 11:34:44 EST
(In reply to comment #12)
> I think that it is proper to return alias as it is returned from Bug.get() as
> well, 

We return all bug data with Bug.get() right? so naturally alias would be part of
that.

> also another reason is that if the user passed more than one bug aliases
> then if they get bugs activities returned they need the bug alias to know which
> information belongs to which alias they passed. 

Good point. So with that I am fine with it being left in.
Comment 15 David Lawrence 2008-02-28 11:40:30 EST
Comment on attachment 296199 [details]
Bugzilla::WebService::Bug::get_activity()

>+sub get_activity {
>+    my ($self, $params) = @_;    
>+
>+    my @ids = ref($params->{ids}) ? @{$params->{ids}} : ($params->{ids});
>+    @ids || ThrowCodeError('param_required', { param => 'ids' });
>+
>+    my %item;
>+    my @return;
>+    foreach my $bug_id (@ids) {

my %item should be declared inside of the foreach loop since it needs to be
cleaned out for
each bug that we are getting the activity for.

>+        ValidateBugID($bug_id);
>+        $item{id} = $bug_id;
>+
>+        ( $item{operations}, $item{incomplete_data} ) = Bugzilla::Bug::GetBugActivity($bug_id);    
>+
>+        my $bug = new Bugzilla::Bug($bug_id);
>+        if (Bugzilla->params->{'usebugaliases'}) {
>+            $item{alias} = type('string')->value($bug->alias);
>+        }
>+        else {
>+            # For API reasons, we always want the value to appear, we just
>+            # don't want it to have a value if aliases are turned off.
>+            $item{alias} = undef;
>+        }
>+
>+        push(@return, \%item);
>+    }
>+
>+    return { bugs => \@return };  
>+} 

Fix the above and r=dkl
Comment 16 Noura El hawary 2008-02-28 23:04:20 EST
Created attachment 296301 [details]
Bugzilla::WebService::Bug::get_activity()

Applied Dave's suggestions, committed patch to cvs.
Comment 17 David Lawrence 2008-03-18 16:33:39 EDT
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 18 David Lawrence 2008-03-18 16:41:46 EDT
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.

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