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.
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
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
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. :)
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
Oops wrong bug report with the keyword ;)
(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.
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
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
worked 2 hours code modifications, testing and POD
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 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'.
> 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.
Created attachment 296199 [details] Bugzilla::WebService::Bug::get_activity() new patch for Bug.get_activity() with Dave's suggestions Noura
(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 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
Created attachment 296301 [details] Bugzilla::WebService::Bug::get_activity() Applied Dave's suggestions, committed patch to cvs.
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.