User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.9) Gecko/20070209 Fedora/1.5.0.9-3.fc6 Firefox/1.5.0.9 pango-text Build Identifier: Need a WebService function to create new component for existing products. The function will be added to new WebService module Component.pm and called Bugzilla::WebSerivce::Component::create() It should recieve in the params hash the new component information, required arguments are product name, component name, description, and default assignee and optional arguments are default qa contact and default cc. Then it will return the id for the newly created components. Reproducible: Always

HI ,, Attched is patch for the function Bugzilla::WebService::Component::create() along with POD, I made it consistent with other WebService Component API functions update and get. Please review when you can. Thanks, Noura

Comment on attachment 305680 [details] [diff] [review] v1 for function Bugzilla::WebService::Component::create() + POD >Index: Bugzilla/WebService/Component.pm >+use Bugzilla::Product; >+use Bugzilla::User; Those two modules are useless. You never need them (at least not for now). >+# default_cc => ['testuserb@redhat.com, 'testuserc@redhat.com'] }); There is a missing quote after the first user, right before the comma. Everything else looks good, pass tests, and works great. r=LpSolit

Note that there is a small bitrot in xmlrpc.cgi. Easy to fix (I can do it myself on checkin).

Comment on attachment 305680 [details] [diff] [review] v1 for function Bugzilla::WebService::Component::create() + POD >Index: Bugzilla/WebService/Component.pm >+# The Original Code is the Bugzilla Bug Tracking System. This license block is missing an Original Author section. >+ my $product_name = trim($params->{product}); >+ my $component_name = trim($params->{component}); >+ my $default_assignee = trim($params->{default_assignee}); >+ my $default_qa_contact = trim($params->{default_qa_contact}); >+ my $description = trim($params->{description}); >+ my $default_cc = $params->{default_cc}; You don't need to trim those--Component->create will do that for you. >+This part of the Bugzilla API allows you to list accessible components and >+get information about them. Actually, just say, "This part of the Bugzilla API allows you to work with the Components of a Product." >+Creates new component for an existing product. "a new" >+A hash containing one item, C<id>, which is the id of the newly >+created component "Which is the integer id of the newly created component." (Added "integer" and a period.) >+=item 304 (Authorization Required) >+Logged-in user not authorized to create components. You need to add another newline between =item and the description--otherwise they will be displayed on one line in the rendered documentation. (This is true for all the errors listed.) >+=item 106 (Product Not Specified) >+Logged-in user didn't specify any product names for the component they want >+to create. You should probably be throwing param_required for that. >+=item 106 (Product Doesn't Exist) >+The specified product doesn't exist. >+=item 105 (Component Name Is Too Long) >+The name of the component is longer than 64 characters. >+ >+=item 105 (Component Requires Default Assignee) >+User didn't specify a default assignee for the new component. >+ >+=item 105 (Blank Component Description Not Allowed) >+User didn't specify a description for the new component. >+ >+=item 105 (Blank Component Name Not Allowed) >+User didn't specifiy name for the new component. >+ >+=item 105 (Component Already Exists) >+Trying to create a component which has similar name to an existing component. Those all need to be *different* error codes. The two for Component Name can both be the same, though. You should never, ever be listing the same error code twice in these listings. If you can't describe the error code as one single error, then it should be multiple error codes.

Also, the 100-200 error range is for bugs! You should have a new error range for components, if you can.

Hi , New version for the function Component.create I made the suggested modifications. created new range of error codes for the Component and hence i made a change to Bugzilla/WebService/Bug.pm that was using the old range. Please review. Thanks, Noura

Comment on attachment 321690 [details] [diff] [review] v2 for new WebService function Component.create >+++ Bugzilla/WebService/Component.pm 2008-05-20 10:50:19.000000000 +1000 >+# The Initial Developer of the Original Code is Netscape Communications >+# Corporation. Nit: the initial developer is you, not Netscape. Otherwise looks good. r=LpSolit

Requesting approval once more, to have it in our radar (till Max finds something wrong :)).

Fixed the initial developer section and updated the patch to include latest updates that were committed to cvs for the modules included in that patch so it can be applied to latest release easily with no failure. Please review when you can. Thanks, Noura

mkanat, ping? Could you review this one? We are slow at implementing new webservice methods.

(In reply to comment #10) > mkanat, ping? Could you review this one? We are slow at implementing new > webservice methods. I'm sorry, I have a lot of paid work in front of this. However, there's a chance that a client will be funding a fair bit of work on XML-RPC, in which case these reviews will become more of a priority.

Comment on attachment 326254 [details] [diff] [review] v3 for new WebService function Component.create Let's make this Product.add_component instead. Also, definitely don't change the numbers of old errors. You can add new errors, though. Otherwise, this actually looks pretty good.

Hi, Where are we with this feature? What can I implement : * Component.create ? * Product.add_component ? * Product.update with control add/remove component ? Please, could you said me how you want implented this feature.

Hmm. I think Product.add_component would be the best way to go. Then we don't create a whole new module.

Julien, are you willing to work on Product.add_component? I think Noura is no longer working on it.

In bug 385282 there has been discussion on webservice naming for these new functions. I personally prefer creating a new Component.pm module and have a method named Component.create for this type of functionality. Same for Milestone.create, Version.create, etc. instead of later adding Product.add_milestone, Product.add_version, etc. It just makes things seem more consistent to me. LpSolit, do you prefer Product.add_component or Component.create personally? Internally code looks good at quick glance although I still need to do some real testing on it. Just wanted to voice my concern early regarding the method naming before we get too far. dkl

Well, though I've just joined the community and I may not be familiar so much with design issues, I think adding Component.pm would be better, more consistent and more desirable for web service users. However, if they were going to accept this idea, they must had accepted the previous patches already.

(In reply to David Lawrence [:dkl] from comment #17) > LpSolit, do you prefer Product.add_component or Component.create personally? Let's go with Component.{create|update|remove}.

Comment on attachment 631869 [details] [diff] [review] patch Ok, based on recent comments, let's go ahead and create the new Component.pm and add this as a create() method. Once that's done I will do a thorough review then. dkl

Do I have to implement get() as well? This function can be really useful.

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #21) > Do I have to implement get() as well? This function can be really useful. Yes that would be awesome. But please do it get() as a separate bug that depends on this one. Meaning the patch on the get() bug will require the patch from this one to be committed creating the new Component.pm module first. Thanks dkl

koosha: do not remove a review set by a reviewer. If a patch has review-, leave it as is, even if you know you are going to attach a new patch. Even reviewers are not allowed to withdraw other reviewers' reviews. (In reply to David Lawrence [:dkl] from comment #22) > Yes that would be awesome. But please do it get() as a separate bug Component.get() will be implemented in bug 385282.

(In reply to Frédéric Buclin from comment #23) > koosha: do not remove a review set by a reviewer. If a patch has review-, > leave it as is, even if you know you are going to attach a new patch. Even > reviewers are not allowed to withdraw other reviewers' reviews. Sorry! Won't happen anymore. That was only because of his feedback; he didn't comment on the patch. So, I thought review- would be non-sense. Excuse me.

I created the module and the methods. Now, I would like you to comment on it. POD and some explanatory comments in the code will follow once you accept the code.

Comment on attachment 633923 [details] [diff] [review] patch - v2 - Added Component.pm - Request for comments >> $changes->{$f}->[0] = join(', ', map { $_->login } @$removed); >> $changes->{$f}->[1] = join(', ', map { $_->login } @$added); I'm not sure whether to return an array or a string of csv. Personally, I prefer an array.

dkl: could you please comment on this patch?

Comment on attachment 633923 [details] [diff] [review] patch - v2 - Added Component.pm - Request for comments Review of attachment 633923 [details] [diff] [review]: ----------------------------------------------------------------- Please add Component.get. When doing Component.get you will also need a related _component_to_hash method as well. Functionally this works for me in my testing so far. So for the next revision, we will need POD added as well to call complete. dkl ::: Bugzilla/WebService/Component.pm @@ +17,5 @@ > +use Bugzilla::User; > +use Bugzilla::WebService::Constants; > +use Bugzilla::WebService::Util qw(translate params_to_objects); > + > +use constant PARAMS => qw{ nit: qw() @@ +26,5 @@ > + default_cc > + is_open > +}; > + > +use constant PARAM2FIELD => { s/PARAM2FIELD/MAPPED_FIELDS/ similar to Product.pm @@ +33,5 @@ > + default_cc => 'initial_cc', > + is_open => 'isactive', > +}; > + > +use constant FIELD2RETURN => { s/FIELD2RETURN/MAPPED_RETURNS/ similar to Product.pm @@ +34,5 @@ > + is_open => 'isactive', > +}; > + > +use constant FIELD2RETURN => { > + initialowner => 'default_assginee', default_assignee @@ +40,5 @@ > + cc_list => 'default_cc', > + isactive => 'is_open', > +}; > + > +use constant PARAM2SETTER => { s/PARAM2SETTER/MAPPED_SETTERS/ @@ +45,5 @@ > + default_cc => 'cc_list', > + is_open => 'is_active', > +}; > + > +my $_return_component = sub { Would be more readable if you just made this a standard function such as sub _get_component_obj { my ($function, $params) = @_; ... and then later then: my $component = _get_component_obj('Component.update', $params); This of course could be reused for Component.get @@ +135,5 @@ > + > + $component->set_all($values); > + my $changes = translate($component->update(), FIELD2RETURN); > + > + for my $f (qw/default_cc default_qa_contact default_assginee/) { default_assignee ::: template/en/default/global/user-error.html.tmpl @@ +1427,5 @@ > [% ELSE %] > named '[% comp FILTER html %]'. > [% END %] > + > + [% ELSIF error == "id_unknown_component" %] Would rather it be 'component_unknown_id' as the current way seems backward.

(In reply to David Lawrence [:dkl] from comment #29) > Please add Component.get. When doing Component.get you will also need a > related _component_to_hash method as well. Component.get will be implemented on bug 385282 as stated on comment 23. Bug 385282 is dependent on this one. > @@ +26,5 @@ > > + default_cc > > + is_open > > +}; > > + > > +use constant PARAM2FIELD => { > > s/PARAM2FIELD/MAPPED_FIELDS/ similar to Product.pm > > @@ +33,5 @@ > > + default_cc => 'initial_cc', > > + is_open => 'isactive', > > +}; > > + > > +use constant FIELD2RETURN => { > > s/FIELD2RETURN/MAPPED_RETURNS/ similar to Product.pm > > @@ +34,5 @@ > > + is_open => 'isactive', > > +}; > > + > > +use constant FIELD2RETURN => { > > + initialowner => 'default_assginee', > > default_assignee > > @@ +40,5 @@ > > + cc_list => 'default_cc', > > + isactive => 'is_open', > > +}; > > + > > +use constant PARAM2SETTER => { > > s/PARAM2SETTER/MAPPED_SETTERS/ > No. Please lets not continue this ambiguous naming. I digged into Product.pm a lot until I could find out the sources and the destinations of the mappings. I believe this naming convention is much more clear.

Also, please reply my question on comment 27. Almost all users need to get login names separately. So I think an array of strings would be more desirable than a string of csv login names.

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #30) > Component.get will be implemented on bug 385282 as stated on comment 23. Bug > 385282 is dependent on this one. My mistake. I forgot about this being broken into two parts. > No. Please lets not continue this ambiguous naming. I digged into Product.pm > a lot until I could find out the sources and the destinations of the > mappings. I believe this naming convention is much more clear. I did not have the same confusion when reading Product.pm in the past. I have always never liked have the number 2 in any variable which is a more of personal preference. I was suggesting it more so to try and keep the naming consistent across all of Bugzilla WebService modules. Good or bad is up to the individual I suppose. MAPPED_FIELDS and MAPPED_RETURNS are also used in other modules other than Product.pm. (In reply to Koosha Khajeh Moogahi [:koosha] from comment #27) > >> $changes->{$f}->[0] = join(', ', map { $_->login } @$removed); > >> $changes->{$f}->[1] = join(', ', map { $_->login } @$added); > > I'm not sure whether to return an array or a string of csv. Personally, I > prefer an array. I prefer it to be CSV so as to not require client code to have to interpret the return values differently depending on which field has been updated. This way all fields have the same value types (strings). dkl

I'll file a new bug for the POD once this patch has been approved.

Comment on attachment 641942 [details] [diff] [review] patch - v1.1 Review of attachment 641942 [details] [diff] [review]: ----------------------------------------------------------------- Sorry for being a pain here. I am r- this due to my comment in https://bugzilla.mozilla.org/show_bug.cgi?id=694755#c6. Please update this patch to work for multiple components in a single call. dkl ::: Bugzilla/WebService/Component.pm @@ +128,5 @@ > + $user->in_group('editcomponents') || > + scalar @{ $user->get_products_by_permission('editcomponents') } || > + ThrowUserError("auth_failure", { group => 'editcomponents', > + action => 'edit', > + object => 'components' }); Nit: Normally the logic operator goes to the next line also need to fix indentation: $user->in_group('editcomponents') || scalar @{ $user->get_products_by_permission('editcomponents') } || ThrowUserError("auth_failure", { group => 'editcomponents', action => 'edit', object => 'components' });

As you know, clients who wants to choose a component with the use of strings must specify both the product name and the component name. So, in this case it would be different from other get() methods. We could accept two arrays; 'products' & 'components' having same indices specifying a specific component. But, I'm not a fan of this strange approach. So, multiple component retrieval is only possible though ids. What should I do?

Besides, this bug is about component creation, update and removal not retrieval.

Ok, had some time to think about this. Basically when I worked at Red Hat, we did this already with our internal code tree of Bugzilla. Based on what worked then, here it the data structure I would like to propose for Component.get, Component.update, and Component.remove. Component.get: Params: { ids => [1,2], names => [ { component => 'TestComponent', product => 'TestProduct' }, { component => 'AnotherComponent', product => 'AnotherProduct' } ] } Returns: { components => { 'TestProduct' => [ { id => 1, name => 'TestComponent', description => 'This is a test component', default_assignee => 'dkl@mozilla.com', default_qa_contact => '', default_cc => [ 'foo@example.com', ... ], }, ... ], 'AnotherProduct' => [ { id => 2, name => 'AnotherComponent', description => 'This is a another test component', default_assignee => 'dkl@mozilla.com', default_qa_contact => '', default_cc => [ 'foo@example.com', ... ], }, ... ], } } Component.update: Params: { ids => [1,2], names => [ { component => 'TestComponent', product => 'TestProduct' }, ... ], updates => { description => 'This is an updated description, default_assignee => 'bar@example.com', ... }, } Returns: { components => [ { id => 1, changes => [ ] }, ] }

(In reply to David Lawrence [:dkl] from comment #37) > Returns: > { > components => [ > { > id => 1, > changes => [ > > ] > }, > > ] > } Ugh, hit enter before I finished completing my comment. Continuing... Returns: { components => [ { id => 1, changes => [ { description => [ 'This is a test component', 'This is an updated description' ], }, { default_assignee => [ 'dkl@mozilla.com', 'bar@example.com', ], }, ... ] }, ... ] } Component.create: Params: { name => 'TestComponent2', product => 'TestProduct', description => 'Another test product component', default_assignee => 'foo@example.com', default_qa_contact => 'bar@example.com', default_cc => [ 'testusera@example.com, 'testuserb@example.com'] } } Returns: New component id Question: Should we support adding more than one component at a time? Probably not as we don't support that in other methods. Component.remove: Params: { ids => [1,2], names => [ { component => 'TestComponent', product => 'TestProduct' }, { component => 'AnotherComponent', product => 'AnotherProduct' } ] } Returns: 1 or 0 Any objections to this? dkl

I don't know what to say. But, such complex data structures require a comprehensive and exact Doc (that's OK, it just needs more work). For Component.update, I don't think it would be a good idea to update more than one component at a time. Also, the proposed data structure for it is vague. You probably mean that the 'updates' hash is going to be applied to all the mentioned components. Right? Furthermore, you removed the old keys 'added' and 'removed' in the returned data structure. Is that OK? Wouldn't that make some inconsistency? As an alternative, we could offer two ways of interaction which could be determined by the client as a param, like 'single_mode'. Some users, may prefer to interact with web service in a simpler manner. But, of course, that would require us to code more and make maintenance harder.

(In reply to David Lawrence [:dkl] from comment #38) > (In reply to David Lawrence [:dkl] from comment #37) > > Returns: > > { > > components => [ > > { > > id => 1, > > changes => [ > > > > ] > > }, > > > > ] > > } > > Ugh, hit enter before I finished completing my comment. Continuing... > > Returns: > { > components => [ > { > id => 1, > changes => [ > { > description => [ > 'This is a test component', > 'This is an updated description' > ], Actually, this would be changed to: changes => [ { description => { removed => 'This is a test component', added => 'This is an updated description' }, } ] dkl

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #40) > I don't know what to say. But, such complex data structures require a > comprehensive and exact Doc (that's OK, it just needs more work). I don't see it as being overly complex. We can copy the doc already in the Red Hat module. > For Component.update, I don't think it would be a good idea to update more > than one component at a time. Also, the proposed data structure for it is > vague. You probably mean that the 'updates' hash is going to be applied to > all the mentioned components. Right? Any values in the 'updates' hash would be applied to all components listed in either 'ids' or 'names'. I could definitely see people wanting to mass set the assignee, qa contact, or default cc for a bunch of components at once. So I think it would be nice to be able to do that. > Furthermore, you removed the old keys 'added' and 'removed' in the returned > data structure. Is that OK? Wouldn't that make some inconsistency? Yeah I commented after this one to add them back in. That was an error. > As an alternative, we could offer two ways of interaction which could be > determined by the client as a param, like 'single_mode'. Some users, may > prefer to interact with web service in a simpler manner. But, of course, > that would require us to code more and make maintenance harder. I think this would be overkill and add too much complexity to the code. People would just add one product/component to names (or id) and the changes they need to 'updates' to update one component. Seems simple enough to me. dkl

(In reply to David Lawrence [:dkl] from comment #37) > Component.update: > > Params: > { > ids => [1,2], > names => [ > { > component => 'TestComponent', > product => 'TestProduct' > }, > ... > ], > updates => { > description => 'This is an updated description, > default_assignee => 'bar@example.com', > ... > }, > } > > Returns: > { > components => [ > { > id => 1, > changes => [ > > ] > }, > > ] > } I believe that the 'updates' key in the input param should be removed as it deepens the hash unnecessarily and makes it inconsistent with Product.update. Instead, update params must be specified flatly just like Product.update.

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #43) > I believe that the 'updates' key in the input param should be removed as it > deepens the hash unnecessarily and makes it inconsistent with > Product.update. Instead, update params must be specified flatly just like > Product.update. I did that originally to isolate the changes to be made away from the keys/values used to designate which components need to be updated. Also to alleviate any possibility of key conflict in the future. For example if we had used 'name' to designate a component to update, how would be know if that wasn't really to change the name of the component to that value. We would have had to use special update prefixes instead which is also confusing such as 'set_name', 'set_id', 'set_description', etc. dkl

(In reply to David Lawrence [:dkl] from comment #41) > Actually, this would be changed to: > > changes => [ > { > description => { > removed => 'This is a test component', > added => 'This is an updated description' > }, > } > ] > > dkl [] is useless since the client must search for their desired field. We should remove the enclosing array. changes => { description => { removed => 'Old desc', added => 'New desc', }, .... This way the client can conveniently access the changed field: $changes->{changes}->{description}->{added}

component_has_bugs error is not descriptive when deleting more that one component. So, I made a minor change to it and altered the calling code accordingly.

component_has_bugs error is not descriptive when deleting more that one component. So, I made a minor change to it and altered the calling code accordingly.

Some slight modifications.

Comment on attachment 648512 [details] [diff] [review] patch - v1.2 I will let dkl review this one as he already started reviewing the previous patches, and my review queue is already long enough. Note that reviews would come much faster if you implemented only one method at once, as Noura did originally. Here, there are so many tests to do to validate all methods that a review will probably not come in the near future. Also, I would prefer to have POD at the same time as the code itself.

(In reply to Frédéric Buclin from comment #49) > Also, I would prefer to have POD at the same time as the code itself. I can't write POD for the code that its behavior is not accepted yet as I may, in fact, waste my time. Once my code is approved, I'll be happy to spend time on writing POD.

Comment on attachment 648512 [details] [diff] [review] patch - v1.2 OK, since I'm really disappointed in your review, after more than 3 months of waiting, I cancel my request and will try to break the code to smaller parts, add POD and request for review again from you and LpSolit.

Comment on attachment 648512 [details] [diff] [review] patch - v1.2 OK, since I'm really disappointed in your review, after more than 3 months of waiting, I cancel my request and will try to break the code to smaller parts, add POD and request for review again from you and LpSolit.

Comment on attachment 648512 [details] [diff] [review] patch - v1.2 Review of attachment 648512 [details] [diff] [review]: ----------------------------------------------------------------- Need to add 'use 5.10.1;' to the top of Component.pm or t/002goodperl.t complains. Overall I am happy with the API that you have in the code. So the next patch should be good to include the documentation. ::: Bugzilla/WebService/Component.pm @@ +63,5 @@ > + $user->in_group('editcomponents') > + || scalar @{ $user->get_products_by_permission('editcomponents') } > + || ThrowUserError('auth_failure', { group => 'editcomponents', > + action => 'edit', > + object => 'components' }); Essentially I would like the Bugzilla->login and editcomponents check to happen first thing as no point in checking params and throwing those errors if the user is not logged in properly or can even add a component. @@ +75,5 @@ > + if (defined $params->{$field}) { > + my $key = MAPPED_FIELDS->{$field} || $field; > + $component_params->{$key} = $params->{$field}; > + } > + } I would also like to see the default_cc param automatically converted to a list if the client passes in a string containing cc addresses (space/comma separated). We allow this for Bug.create (see Bugzilla::Bug::_check_cc) so we should also allow here as well. Otherwise you get an ugly Perl error if the client mistakenly passes a string instead of a list. @@ +96,5 @@ > + $user->in_group('editcomponents') > + || scalar @{ $user->get_products_by_permission('editcomponents') } > + || ThrowUserError('auth_failure', { group => 'editcomponents', > + action => 'edit', > + object => 'components' }); Same as above about moving to beginning. ::: Bugzilla/WebService/Constants.pm @@ +189,5 @@ > + 'Bug' => 'Bugzilla::WebService::Bug', > + 'Component' => 'Bugzilla::WebService::Component', > + 'User' => 'Bugzilla::WebService::User', > + 'Product' => 'Bugzilla::WebService::Product', > + 'Group' => 'Bugzilla::WebService::Group', This part of the patch unfortunately will no longer apply due to Classification being added now.

I spent much time on writing the POD. However, that might still contain some errors. Please read it carefully. Thanks.

Captured some mistakes in POD.

Comment on attachment 685821 [details] [diff] [review] patch - create() method + POD >=== modified file 'Bugzilla/WebService.pm' >+=item L<Bugzilla::WebService::Component> >+ > =item L<Bugzilla::WebService::Classification> The list is sorted alphabetically. "Component" comes after "Classification". >=== added file 'Bugzilla/WebService/Component.pm' >+use Bugzilla::Product; >+use Bugzilla::User; Those are not in use in this patch and should be removed. >+use constant PARAMS => qw ( We don't use such a constant anywhere else. We must keep our code consistent across all WebServices modules if we want to keep it maintainable. >+use constant MAPPED_SETTERS => { >+ default_cc => 'cc_list', >+ is_open => 'is_active', >+}; Same here. >+sub create { >+ # Check if the mandatory params are set. Ignore the optional ones. >+ for my $param (qw(name product description default_assignee)) { >+ if (not defined $params->{$param}) { >+ ThrowCodeError('param_required', { function => 'Component.create', >+ param => $param }); >+ } >+ } This check is already done by Bugzilla::Component->create(). No need to duplicate it here. Also, we don't do such checks for products and groups. No reason to do it here. This also means that instead of getting a generic 'param_required' error, we will get more useful error messages. >+ my $product_obj = $user->check_can_admin_product($params->{product}); Simply name the variable $product. >+B<Required> C<string> The name of the product that the component must be added to. Maybe make it clearer that the product must already exists, i.e. that it won't be created if missing. >+B<Required> C<string> The login name of the default assignee of the component Missing dot at the end of the sentence. >+C<boolean> 1 if you want to enable the component for bug creations. 0 otherwise. s/creations/creation/. >=== modified file 'Bugzilla/WebService/Constants.pm' > my $dispatch = { > 'Bugzilla' => 'Bugzilla::WebService::Bugzilla', > 'Bug' => 'Bugzilla::WebService::Bug', >+ 'Component' => 'Bugzilla::WebService::Component', > 'Classification' => 'Bugzilla::WebService::Classification', > 'Group' => 'Bugzilla::WebService::Group', > 'Product' => 'Bugzilla::WebService::Product', Same here, the list is sorted alphabetically.

Comment on attachment 686069 [details] [diff] [review] patch - update() method + POD >+ if (not defined $params->{updates}) { >+ ThrowCodeError('param_required', { function => 'Component.update', >+ param => 'updates' }); >+ } I have no idea what this 'updates' param is, and reading the POD didn't help. We also don't use such a parameter in any other update() WS method. I think it should just die. >+ my $component_objs = _get_component_objs('Component.update', $params); >+ >+ my $values = translate($params->{updates}, MAPPED_SETTERS); In other update() methods, we simply write |my $values = translate($params, MAPPED_FIELDS);|. I don't see why this cannot be done here. If the complexity is because this method tries to update components from several products at once, then this support should be dropped, and all components should belong to the same product (which I think we should do anyway). Also, we don't use MAPPED_SETTERS anywhere else. (You will note that I'm very picky about consistency, but we managed to be consistent with the implementation of Bugzilla::Object, there is no reason we cannot be consistent in all WebServices methods.) I didn't read further, because I guess the code will change a lot with my comments above.

Comment on attachment 685823 [details] [diff] [review] patch - remove() method + POD >+ my $component_objs = _get_component_objs('Component.remove', $params); >+ $_->remove_from_db foreach @$component_objs; I see no code which prevents deleting components you are not allowed to manage. _get_component_objs() simply makes sure components exist, but doesn't check that you are allowed to edit them if you have local editcomponents privs.

(In reply to Frédéric Buclin from comment #62) > Comment on attachment 686069 [details] [diff] [review] > patch - update() method + POD > > >+ if (not defined $params->{updates}) { > >+ ThrowCodeError('param_required', { function => 'Component.update', > >+ param => 'updates' }); > >+ } > > I have no idea what this 'updates' param is, and reading the POD didn't > help. We also don't use such a parameter in any other update() WS method. I > think it should just die. > See comment 37. dkl suggested that many people want to change more than once component at once and I changed my code accordingly. > > >+ my $component_objs = _get_component_objs('Component.update', $params); > >+ > >+ my $values = translate($params->{updates}, MAPPED_SETTERS); > > In other update() methods, we simply write |my $values = translate($params, > MAPPED_FIELDS);|. I don't see why this cannot be done here. If the > complexity is because this method tries to update components from several > products at once, then this support should be dropped, and all components > should belong to the same product (which I think we should do anyway). Also, > we don't use MAPPED_SETTERS anywhere else. (You will note that I'm very > picky about consistency, but we managed to be consistent with the > implementation of Bugzilla::Object, there is no reason we cannot be > consistent in all WebServices methods.) > > I didn't read further, because I guess the code will change a lot with my > comments above. I changed my code a lot already to address dkl's suggested data structure and wrote a full doc accordingly. Now you're saying that we should eliminate this way of behavior. What am I supposed to do? Whom do I have to listen to?

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #64) > See comment 37. dkl suggested that many people want to change more than once > component at once and I changed my code accordingly. Editing several components at once in the same product is fine. I have no objection against that. I'm currently not convinced that we need to support to edit several components across several products at once. Other update() methods already have limitations when updating several objects at once. If this brings no complexity, both for the caller and the backend code, then I have no objection to support this anyway. But consistency matters, and I want to have a similar interface for all update() methods. It's a nightmare to have one syntax per method. Just a good way to mess everything and making debugging harder. > I changed my code a lot already to address dkl's suggested data structure > and wrote a full doc accordingly. Now you're saying that we should eliminate > this way of behavior. What am I supposed to do? Whom do I have to listen to? I have no idea which "way of behavior" you are talking about, but if you are talking about POD, I never said that it shouldn't be part of the patch. POD must be part of the patch, because it permits to understand how the method is supposed to work.

Too late for 4.4. We already released 4.4rc1, and 4.4rc2 is just around the corner. We don't have a plan for a 3rd release candidate, so this bug is postponed to 5.0.

At the beginning of Component.pm there is a line: use 5.10.1; Is that version of perl really a requirement?

(In reply to Jim Walters from comment #67) > At the beginning of Component.pm there is a line: > > use 5.10.1; > > Is that version of perl really a requirement? It a *minimum* requirement, yes. This enables several new features which were not implemented in Perl 5.8.

Fair enough. Do you know which features are used in this module which require 5.10 rather than 5.8? (Asking because I need to back-port it to an older system.)

(In reply to Jim Walters from comment #69) > Fair enough. Do you know which features are used in this module which > require 5.10 rather than 5.8? (Asking because I need to back-port it to an > older system.) Maybe nothing. ./runtests.pl should complain if you back port it to an older perl and it cannot compile.

Lets try this again. Now incorporates LpSolit's comments. There are two other changes. 1) default_cc MUST be an array. 2) I've removed the documentation about is_open. It still works, but creating a non active component doesn't make sense (this option is not shown in the create component screen in the web UI). Once this patch is approved, I'll add the update RPC call, and then the delete call. -- simon

Comment on attachment 8400197 [details] [diff] [review] create v4 patch Review of attachment 8400197 [details] [diff] [review]: ----------------------------------------------------------------- Minor bit-rot. patching file Bugzilla/WebService/Constants.pm Hunk #1 succeeded at 203 with fuzz 2 (offset 12 lines). Hunk #2 FAILED at 274. 1 out of 2 hunks FAILED -- saving rejects to file Bugzilla/WebService/Constants.pm.rej Also needs a REST resource file (Bugzilla/WebService/Server/REST/Resources/Component.pm) and related REST example in the POD. Otherwise, works well and looks good. ::: Bugzilla/WebService/Component.pm @@ +129,5 @@ > +You are not authorized to create a new component. > + > +=item 1100 (Component already exists) > + > +The name that you specified for the new component is already of another component. Nit: The name that you specified for the new component already exists in the specified product. @@ +137,5 @@ > +=item B<History> > + > +=over > + > +=item Added in Bugzilla B<4.4>. s/4.4/5.0/

Comment on attachment 8470506 [details] [diff] [review] bug419568-create-v5.patch Review of attachment 8470506 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. Nits can be fixed on checkin. r=dkl ::: Bugzilla/WebService/Component.pm @@ +117,5 @@ > +=back > + > +=item B<Returns> > + > +A hash with one key: id. This will represent the ID of the newly-added nit: C<id> ::: Bugzilla/WebService/Constants.pm @@ +205,5 @@ > > + # Component errors are 1100-1200 > + component_already_exists => 1100, > + component_is_last => 1101, > + component_has_bugs => 1102, nit: line up => @@ +287,4 @@ > 'Bugzilla' => 'Bugzilla::WebService::Bugzilla', > 'Bug' => 'Bugzilla::WebService::Bug', > 'Classification' => 'Bugzilla::WebService::Classification', > + 'Component' => 'Bugzilla::WebService::Component', nit: line up =>

To ssh://gitolite3@git.mozilla.org/bugzilla/bugzilla.git 2863a66..8b98912 master -> master I also changed the bug numners from 110x to 120x because 110x was used to flag types. I'll file a new bug to handle the update and remove functions for this API. -- simon