Bug 434941 - New WebService Module Component.pm
New WebService Module Component.pm
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-02-26 08:49 EST by Noura El hawary
Modified: 2013-06-24 00:18 EDT (History)
0 users

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-05-22 02:40:37 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:


Attachments (Terms of Use)
new WebService module Component.pm (18.19 KB, patch)
2008-02-26 08:52 EST, Noura El hawary
no flags Details | Diff
WeService module Component.pm (20.30 KB, patch)
2008-02-29 10:52 EST, Noura El hawary
dkl: review-
Details | Diff
new WebService module Component.pm + POD (21.53 KB, patch)
2008-03-05 04:12 EST, Noura El hawary
dkl: review+
Details | Diff

  None (edit)
Description Noura El hawary 2008-02-26 08:49:13 EST
new WebService module Component.pm currently has 3 functions update, create, and
get this but is created to integrate all those functions and have them in one
patch instead of 3, easier for testing and review instead of having 3 different
patches.
Comment 1 Noura El hawary 2008-02-26 08:52:13 EST
Created attachment 295907 [details]
new WebService module Component.pm 

Hi Dave ,,

Here is the Component.pm module with the three functions get, update and create
. please review when you can.

Thanks,
Noura
Comment 2 David Lawrence 2008-02-26 15:04:12 EST
Comment on attachment 295907 [details]
new WebService module Component.pm 

>+sub get {
>+    my ( $self, $params ) = @_;
>+
>+    my $accessible_components = get_accessible_components($params);    
>+
>+    my @components =
>+        map {
>+        {   id           => type('int')->value( $_->id ),
>+            name         => type('string')->value( $_->name ),
>+            description  => type('string')->value( $_->description ),
>+            product_id   => type('int')->value( $_->product_id ),

We should also pass back the product name if possible:

	      product_name => type('string')->value( $_->product->name ),

>+            default_assignee =>
>+                type('string')->value( $_->default_assignee->{login_name} ),
>+            default_qa_contact =>
>+                type('string')->value( $_->default_qa_contact->{login_name} ),
>+            default_cc => [ map $_->{login_name}, @{ $_->initial_cc } ],
>+
>+        }
>+        } @$accessible_components;
>+
>+    return { components => \@components };
>+}
Comment 3 David Lawrence 2008-02-26 15:13:02 EST
Comment on attachment 295907 [details]
new WebService module Component.pm 


>+            default_assignee =>
>+                type('string')->value( $_->default_assignee->{login_name} ),
>+            default_qa_contact =>
>+                type('string')->value( $_->default_qa_contact->{login_name} ),
>+            default_cc => [ map $_->{login_name}, @{ $_->initial_cc } ],

Also, since we are not requiring login for Component.get() I think we should
add to the beginning 

Bugzilla->login(LOGIN_OPTIONAL);

And then when populating the default_assignee. default_qa_contact. and
default_cc, we should use the real_name instead of login_name if the user did
not login. This will help with some against spam farming. 

Also I personally would like the return keys to be initialowner,
initialqacontact, and initialcclist if possible. Was it that the upstream
requested the default_* labels or were you copying the set methods in the
Component.pm module? The reason I would prefer it the other way is 1) our
current API has them so less headache switching, the editcomponents.cgi UI uses
initial* instead of default_* for form variables, etc. and 3) I like to match
the actual database column names unless the column is an integer and needs to
mapped to a real name.

What do you think?

Dave
Comment 4 David Lawrence 2008-02-26 15:21:01 EST
> Also, since we are not requiring login for Component.get() I think we should
> add to the beginning 
> 
> Bugzilla->login(LOGIN_OPTIONAL);

Disregard this comment, as I forgot that in xmlrpc.cgi/WebService.pm the user is
always given the option to login so this statement would not be required.

Dave
Comment 5 David Lawrence 2008-02-26 15:38:38 EST
Comment on attachment 295907 [details]
new WebService module Component.pm 

>+sub update {
>+    my ($self, $params) = @_;
>+
>+    my $user = Bugzilla->login(LOGIN_REQUIRED);
>+
>+    $user->in_group('editcomponents')
>+        || scalar(@{$user->get_products_by_permission('editcomponents')})
>+        || ThrowUserError("auth_failure", {group  => "editcomponents",
>+                                           action => "edit",
>+                                           object => "components"});
>+
>+    my $accessible_components = get_accessible_components($params);
>+
>+    # if the user tries to change component name for several
>+    # components of the same product then throw an error
>+    my %unique_product_comps;
>+    foreach my $comp (@$accessible_components){
>+        if($unique_product_comps{$comp->product_id} && (grep {$_ =~ /name/ } keys %{$params->{update}})){
>+            ThrowUserError("multiple_components_update_not_allowed");
>+        }
>+        else {
>+            $unique_product_comps{$comp->product_id} = 1;
>+        }
>+    }
>+
>+    my %mapped_returns = (
>+        initialowner => 'default_assignee',
>+        initialqacontact => 'default_qa_contact',
>+        cc_list => 'default_cc',
>+    );
>+
>+    my %changes;
>+
>+    foreach my $component_obj (@$accessible_components){
>+        foreach my $field (keys %{$params->{update}}) {
>+            my $method = "set_" . $field;
>+            if ($field eq "default_cc"){
>+                $method = "set_cc_list";
>+            }
>+            $component_obj->$method($params->{update}{$field});
>+        }
>+    }
>+    # map the field names in the returned changes hash to the 
>+    # right field name for API consistency and convert 
>+    # the user ids in the changes hash to login names    
>+    foreach my $component_obj (@$accessible_components){
>+        my $returned_changes = $component_obj->update();
>+        foreach my $changed_field (keys %{$returned_changes}){
>+            my $mapped_field = $mapped_returns{$changed_field};
>+            $mapped_field ||= $changed_field;

can be shortened to:

	      my $mapped_field = $mapped_returns{$changed_field}
		  || $changed_field;


>+            if ($mapped_field eq 'default_assignee' || $mapped_field eq 'default_qa_contact'){
>+                $changes{$component_obj->id}{$mapped_field} = 
>+                     [ map $_->{login_name}, @{Bugzilla::User->new_from_list($returned_changes->{$changed_field})} ] 
>+            }
>+            elsif ($mapped_field eq 'default_cc'){
>+                my $cc_list = $returned_changes->{$changed_field};
>+                my @old_cclist = split( /[,\s]+/, $cc_list->[0] );
>+                my @new_cclist = split( /[,\s]+/, $cc_list->[1] );
>+                
>+                my $old_cclist = join( ', ', ( map $_->{login_name}, @{Bugzilla::User->new_from_list(\@old_cclist)}) );
>+                my $new_cclist = join( ', ', ( map $_->{login_name}, @{Bugzilla::User->new_from_list(\@new_cclist)}) );
>+
>+                $changes{$component_obj->id}{$mapped_field} = [$old_cclist, $new_cclist];
>+
>+            }
>+            else {
>+                $changes{$component_obj->id}{$mapped_field} = $returned_changes->{$changed_field};
>+            }
>+        }
>+    }
>+
>+    return { components_updates => \%changes };
>+
>+}

Is this maybe not necessary to return the changeset in this way? I guess what I
trying to say is that we could just simply return a list of return values from
$component_obj->update() and keep it simpler that way. Is there any information
that $component_obj->update() returns that would be useful? For example the
calling user knows what the new and old values are most of the time, they just
need some indication that a change occurred instead of no change.

Dave

Dave
Comment 6 Noura El hawary 2008-02-27 21:45:18 EST
Hi Dave ,, 

In reply to all of the above comments : 

1- comment#2 ,,passing back product_name from Component.get , I personally also
prefer that , and this how was my initial patch returning but then Max Kanat
asked me to leave it out for now, but i guess we can include now in our version
and see what will happen in the future.

2- comment#3, in using real_name instead of login_name if the user did
not login , basically in the subroutine get_accessible_components() , only
accessible components to the logged in user are returned bu checking:
Bugzilla->user->can_enter_product($comp_obj->product->name, THROW_ERROR) so i
guess this means that the user must be logged in and can access the product to
see the component details!!,, do you think we should have here 2 cases then one
if they are logged-in and another if they are not.

3- comment#3, replacing the default_* resturned values with actual field
database name like initialowner, I had them initially set th default_* except
for the cclist based on the function's accessors names and set_ methods and then
Max also asked me to hace cc_list as default_cc , then I had to do the mapping
for other Component API functions , to make the API consistent basically the
mapping is only needed for the returned changes hash from $component->update

4- comment#5, basically in the returned changes hash from update it will have
the old value and new value, I guess upstream prefers to get this changes hash
returned from the WebService function but with the field names inside the
changes hash changed to names that keeps it consistent with other fields
returned from the other API functions, I know it is causing alot of extra coding.


Basically I think if we change the default_* to the database column names as you
suggested this will save us the mapping of the changes hash because it has the
database column names, So i guess it is a good idea.

Noura
Comment 7 David Lawrence 2008-02-27 23:23:14 EST
(In reply to comment #6)
> Hi Dave ,, 
> 
> In reply to all of the above comments : 
> 
> 1- comment#2 ,,passing back product_name from Component.get , I personally also
> prefer that , and this how was my initial patch returning but then Max Kanat
> asked me to leave it out for now, but i guess we can include now in our version
> and see what will happen in the future.

Agreed.

> 2- comment#3, in using real_name instead of login_name if the user did
> not login , basically in the subroutine get_accessible_components() , only
> accessible components to the logged in user are returned bu checking:
> Bugzilla->user->can_enter_product($comp_obj->product->name, THROW_ERROR) so i
> guess this means that the user must be logged in and can access the product to
> see the component details!!,, do you think we should have here 2 cases then one
> if they are logged-in and another if they are not.

A user should be able to retrieve components lists for *public* products if they
are not logged in. We do this already with describecomponents.cgi. So the
get_accessible_components() should handle that. But like we do in
describecomponents.cgi, the login_name should be hidden if the user is not
logged in. You would just return the realname in that case.

> 3- comment#3, replacing the default_* resturned values with actual field
> database name like initialowner, I had them initially set th default_* except
> for the cclist based on the function's accessors names and set_ methods and then
> Max also asked me to hace cc_list as default_cc , then I had to do the mapping
> for other Component API functions , to make the API consistent basically the
> mapping is only needed for the returned changes hash from $component->update

Ok, if upstream will accept the changes faster by having them named default_*
then I can live with them. I was trying more for consistency with the web UI
form variable names. 

> 4- comment#5, basically in the returned changes hash from update it will have
> the old value and new value, I guess upstream prefers to get this changes hash
> returned from the WebService function but with the field names inside the
> changes hash changed to names that keeps it consistent with other fields
> returned from the other API functions, I know it is causing alot of extra coding.

Ok, same as above. If upstream prefers them named that way then that is the best
path to take.
 
> Basically I think if we change the default_* to the database column names as you
> suggested this will save us the mapping of the changes hash because it has the
> database column names, So i guess it is a good idea.

Let's leave it the way you have it submitted upstream for now and see where it goes.

Dave

Comment 8 Noura El hawary 2008-02-29 10:52:39 EST
Created attachment 296371 [details]
WeService module Component.pm

Hi Dave,

I have done the modifications you have suggested. also with Component.get and
Component.update I allowed the passing of single user id as well as the array
of ids. changed the POD accordingly .  Please review when you can.

Thanks,
Noura
Comment 9 David Lawrence 2008-03-04 10:22:10 EST
Comment on attachment 296371 [details]
WeService module Component.pm


>+    # map the field names in the returned changes hash to the 
>+    # right field name for API consistency and convert 
>+    # the user ids in the changes hash to login names    
>+    foreach my $component_obj (@$accessible_components){
>+        my $returned_changes = $component_obj->update();
>+        foreach my $changed_field (keys %{$returned_changes}){
>+	    my $mapped_field = $mapped_returns{$changed_field}
>+		               || $changed_field;

Make sure to fix your spacing (4 spaces, no tabs) and also leave a single space
before the
curly bracket in conditional blocks, for example:

      foreach my $component_obj (@$accessible_components) {


>+sub get_accessible_components {
>+    my $params = shift;
>+
>+    my @ids = ref($params->{ids}) ? @{$params->{ids}} : ($params->{ids});
>+    my $obj_by_ids;
>+    $obj_by_ids = Bugzilla::Component->new_from_list(\@ids) if $params->{ids};

Combine to single line and also check @ids not $params->{ids}:

      my $obj_by_ids = Bugzilla::Component->new_from_list(\@ids) if @ids;

>+
>+    my @component_objs;
>+
>+    # to get the component objects for product/component combination
>+    # first obtain the product object from the passed product name
>+    @component_objs = map Bugzilla::Component->check(

You can combine @component_objs in one line also:

      my @component_objs = map Bugzilla::Component->check(

>+        {   product => Bugzilla::Product->check( { name => $_->{product} } ),
>+            name    => $_->{component}
>+        }
>+        ),
>+        @{$params->{names}}
>+        if $params->{names};

Also check for ref($params->{names}) the same way you do @ids although you will
specifically need to check for ARRAY since each element of the ARRAY can be a
reference to a HASH.
This way a user can pass just a single HASH:

      my @names = ref($params->{names}) =~ /ARRAY/ ? @{$params->{names}} :
($params->{names});

      my @component_objs = map Bugzilla::Component->check(
	  {   product => Bugzilla::Product->check( { name => $_->{product} } ),
	      name    => $_->{component}
	  }
      ), @names if @names;


>+    my %unique_components;
>+    map { $unique_components{$_->id} = $_ } @component_objs;
>+    @component_objs = values %unique_components;
>+

This can be combined as well

      my %unique_components = map { $_->id => $_ } @component_objs;

>+    # if user is not logged in then return the public product components
>+    # this can only happen with Component.get as login is required
>+    # for Component.update
>+    if (!Bugzilla->user->id){
>+        my @public_products = @{Bugzilla->user->get_enterable_products};

Call this @viewable_products since if the user is logged in
get_enterable_products() will
also return private products that the user can see so using the word public is
not accurate.

	  my @viewable_products = @{Bugzilla->user->get_enterable_products};

>+        my %products;
>+        map { $products{$_->id} = $_ } @public_products;

Combine together:

	  my %products = map { $_->id => $_ } @public_products;


>+Bugzilla::WebService::Component - The Component API
>+
>+=head1 DESCRIPTION
>+
>+This part of the Bugzilla API allows you to list accessible components and
>+get information about them.
>+

This part of the Bugzilla API allows you to list accessible components, get
information about them,
and also to edit the attributes of a component.

>+
>+=head2 Get Components
>+

>+=item B<Returns>    
>+
>+Logged-in users will get information about all accessible product
>+components that they passed. Non-Logged-in users will get information
>+only about public product components that they passed with hidden 
>+login names.
>+
>+The return value will be A hash containing one item, C<components>, 
>+that is an array of hashes. Each hash describes a component,
>+and has the following items:
>+
>+=over
>+
>+=item product_id
>+
>+C<int> The id of the product that the component belongs to.
>+

=item product_name

C<string> The name of the product that the component belongs to.
Comment 10 Noura El hawary 2008-03-05 04:07:35 EST
> 
> Combine to single line and also check @ids not $params->{ids}:
> 
>       my $obj_by_ids = Bugzilla::Component->new_from_list(\@ids) if @ids;
> 
Hey Dave,, Thanks for the review. just regarding using if @ids instead of
if $params->{ids} is causing errors so I left it as it.

Noura
Comment 11 Noura El hawary 2008-03-05 04:12:20 EST
Created attachment 296864 [details]
new WebService module Component.pm + POD

Hey Dave,, Here is a new patch for Component.pm i included your suggestions and
also the upstream's . Please review when you can.

Thanks,
Noura
Comment 12 David Lawrence 2008-03-05 17:36:51 EST
Comment on attachment 296864 [details]
new WebService module Component.pm + POD

Looks good Noura, check it in unless someone else objects.

r=dkl
Comment 13 Noura El hawary 2008-03-06 01:25:34 EST
Thanks for the review now Dave, it is all committed to cvs now. 

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