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.
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 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 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
> 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 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
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
(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
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 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.
> > 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
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 on attachment 296864 [details] new WebService module Component.pm + POD Looks good Noura, check it in unless someone else objects. r=dkl
Thanks for the review now Dave, it is all committed to cvs now.