Open
Bug 385282
Opened 18 years ago
Updated 10 years ago
Ability to get information about Components in the WebService
Categories
(Bugzilla :: WebService, enhancement)
Tracking
()
NEW
People
(Reporter: robert, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 5 obsolete files)
6.55 KB,
patch
|
mkanat
:
review-
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.3) Gecko/20070310 Iceweasel/2.0.0.3 (Debian-2.0.0.3-1)
Build Identifier: 3.0
Currently, the get_products XMLRPC call has no easy way to then fetch the matching components.
This patch adds a field containing the components for the product.
Reproducible: Always
Steps to Reproduce:
1. Use XMLRPC
2. Call the function
3. Examine the result :)
Actual Results:
I got basic information on the products
Expected Results:
I would like information about the components too.
This can possibly be made optional if the network bandwidth is considered too high.
![]() |
Reporter | |
Comment 1•18 years ago
|
||
![]() |
||
Comment 2•18 years ago
|
||
Comment on attachment 269158 [details] [diff] [review]
Patch for enabling the behaviour described
This would have to be way more specified than this, and the POD would also have to be updated.
Also, instead of this, I'd much rather do a get_components function. Don't we have that?
Attachment #269158 -
Flags: review-
![]() |
Reporter | |
Comment 3•18 years ago
|
||
Not that I could see. I'm quite happy for there to be a get_components() call instead.
![]() |
||
Comment 4•18 years ago
|
||
The versions are missing too.
Please add this as well:
versions => $_->versions()
![]() |
||
Comment 5•18 years ago
|
||
if we are to add function get_components() shall it accept only a single product id and return list of components along with their details for this product , or it can accept a list of product ids? also in the return result I think it would be a good idea to return in the component details along with the (name, description, initialowner and initialqacontact) the initial cc list I think we can use for that the function Bugzilla::Component::initial_cc() , and we will be converting the user ids to login names to be more specific. Please let me know what you think of my comments , I would like to work on this function. And one last thing the right place for this function would be the module Bugzilla/WebService/Product.pm ?
Thanks,
Noura
![]() |
||
Comment 6•18 years ago
|
||
Actually regarding where the get_components() function should be placed, we have in our redhat bugzilla xmlrpc interface functions that add components and edit components and enable getting component information by component id as well as by product id, we are willing to contribute all those functions to the upstream, so maybe it would be a good idea to consider adding a new WebService module called Component.pm!
Thanks,
Noura
![]() |
||
Comment 7•18 years ago
|
||
Yes, we probably should have Bugzilla::WebService::Component, and the function should be called Component.get.
(We should also rename Product.get_products to Product.get and Bugs.get_bugs to Bugs.get, but keeping backwards compatibility for API purposes.)
It should take a single named argument: { ids => [1,2,3,4...] } and should return data much like get_products does.
![]() |
||
Comment 8•18 years ago
|
||
As discussed earlier this is a patch for new WebService module Component.pm ,, added to it function Component.get() , besides passing the components ids to get the component information I also enabled passing the combination of component and product name to get component details as I think it is more user friendly, as lots of users might not know the component ids. also I didn't add POD yet and wasn't sure if this function will require any specific authentication.
Please review when you can and let me know what you think.
Thanks,
Noura
Attachment #298407 -
Flags: review?(mkanat)
![]() |
||
Updated•18 years ago
|
Assignee: webservice → nelhawar
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Ever confirmed: true
Target Milestone: --- → Bugzilla 3.2
![]() |
||
Comment 9•18 years ago
|
||
Hi,, Can you please advise me on the permission that will be needed in this function, so I can fix the attached patch accordingly.
Thanks,
Noura
![]() |
||
Comment 10•18 years ago
|
||
You shouldn't be able to get any component information for products that aren't accessible to you, that's the only security restriction.
![]() |
||
Comment 11•18 years ago
|
||
Comment on attachment 298407 [details] [diff] [review]
v1 for new WebService module Component.pm with function get()
>+++ Bugzilla/WebService/Component.pm 22 Jan 2008 04:42:28 -0000
>@@ -0,0 +1,76 @@
>+# -*- Mode: perl; indent-tabs-mode: nil -*-
> [snip]
Your license block is missing an Original Author section.
>+ if $params->{comp_prod};
We need a better name than comp_prod.
Also, you can't give away the existence of products that aren't accessible to the user, which Bugzilla::Product->check will do.
>+ 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 ),
>+ product_name => type('string')->value( $_->product->name ),
Don't include product_name, for now.
>+ default_assignee =>
>+ type('string')->value( $_->default_assignee->{login_name} ),
>+ default_qa_contact =>
>+ type('string')->value( $_->default_qa_contact->{login_name} ),
>+ initial_cc => [ map $_->{login_name}, @{ $_->initial_cc } ],
Make that default_cc instead of initial_cc. Also, I wonder, should these be user ids instead of login names?
Attachment #298407 -
Flags: review?(mkanat) → review-
![]() |
||
Comment 12•18 years ago
|
||
Hey Max,, Thanks for the review I am attaching a patch with your suggested fixes, also please see my comments inline:
(In reply to comment #11)
> (From update of attachment 298407 [details] [diff] [review])
> >+++ Bugzilla/WebService/Component.pm 22 Jan 2008 04:42:28 -0000
> >@@ -0,0 +1,76 @@
> >+# -*- Mode: perl; indent-tabs-mode: nil -*-
> > [snip]
>
> Your license block is missing an Original Author section.
>
added to the new patch
> >+ if $params->{comp_prod};
>
> We need a better name than comp_prod.
>
how about products_and_components
> Also, you can't give away the existence of products that aren't accessible to
> the user, which Bugzilla::Product->check will do.
>
added fix to the patch that will get Bugzilla->user->get_accessible_products
> >+ 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 ),
> >+ product_name => type('string')->value( $_->product->name ),
>
> Don't include product_name, for now.
deleted product_name, however if the product is accessible isn't it ok to see the product name? or is there another reason why you don't want the product name returned?
>
> >+ default_assignee =>
> >+ type('string')->value( $_->default_assignee->{login_name} ),
> >+ default_qa_contact =>
> >+ type('string')->value( $_->default_qa_contact->{login_name} ),
> >+ initial_cc => [ map $_->{login_name}, @{ $_->initial_cc } ],
>
> Make that default_cc instead of initial_cc. Also, I wonder, should these be
> user ids instead of login names?
>
I think login names is better returned as its more user friendly .. I replaced login_name with email to be similar to what is in template/en/default/reports/components.html.tmpl
so basically the user in describecomponents.cgi can get user emails and also product name I guess that Component.get should return the same. what do you think?
Thanks,
Noura
![]() |
||
Comment 13•18 years ago
|
||
attached new version for Bugzilla::WebService::Component::get() that has the fixes in Max's review also I added POD for the module.
Please review when you can?
Thanks,
Noura
Attachment #298407 -
Attachment is obsolete: true
Attachment #300031 -
Flags: review?(mkanat)
Comment 14•18 years ago
|
||
(In reply to comment #12)
> > >+ default_assignee =>
> > >+ type('string')->value( $_->default_assignee->{login_name} ),
> > >+ default_qa_contact =>
> > >+ type('string')->value( $_->default_qa_contact->{login_name} ),
> > >+ initial_cc => [ map $_->{login_name}, @{ $_->initial_cc } ],
> >
> > Make that default_cc instead of initial_cc. Also, I wonder, should these be
> > user ids instead of login names?
> >
>
> I think login names is better returned as its more user friendly .. I replaced
> login_name with email to be similar to what is in
> template/en/default/reports/components.html.tmpl
>
> so basically the user in describecomponents.cgi can get user emails and also
> product name I guess that Component.get should return the same. what do you
> think?
I also feel that logins should be returned as opposed to user ids only. user
ids are not very useful to third party applications that need to use the data
that is returned. Otherwise they would need to execute User.get() for each id
in order to map to a human readable login. I am not opposed to returning both
id and login if needed in a list of hashes.
Dave
Comment 15•18 years ago
|
||
Comment on attachment 300031 [details] [diff] [review]
v2 for new WebService Module Component.pm with function get()
>+# function to return component information when passed either,
>+# component ids or component and product names or both togther.
>+# exmaple of how it will be called:
>+# $call = $rpc->call( 'Component.get',
>+# {ids => [1,2], products_and_components => [{name => 'testingComponent',product => 'TestProduct'},
>+# {name => 'testComponent22',product => 'TestProduct'}
>+# ]
>+# }
>+# );
Instead of 'name' can we call it 'component' to make it more obvious which value you need to provide
for that key?
Instead:
>+# $call = $rpc->call( 'Component.get',
>+# {ids => [1,2], products_and_components => [{ component => 'testingComponent', product => 'TestProduct' },
>+# { component => 'testComponent22', product => 'TestProduct' }
>+# ]
>+# }
>+# );
![]() |
||
Comment 16•18 years ago
|
||
> Instead of 'name' can we call it 'component' to make it more obvious which
> value you need to provide
> for that key?
>
It sounds good to me. and this way it would make more sense having the key called products_and_components
![]() |
||
Comment 17•18 years ago
|
||
Comment on attachment 300031 [details] [diff] [review]
v2 for new WebService Module Component.pm with function get()
>+ # to get the component objects for product/component combination
>+ # first obtain the product object from the passed product name
>+ my @obj_by_comp_prod = map Bugzilla::Component->check(
>+ { product => Bugzilla::Product->check( { name => $_->{product} } ),
You're still doing it! This will expose the existence of secret products. Look at what Bugzilla->user->can_enter_product does.
>+ name => $_->{name}
>+ @{ $params->{products_and_components} }
Let's just call it "names" instead.
>+ my @components_objs = @obj_by_comp_prod;
Why are you copying this array around?
>+ default_assignee =>
>+ type('string')->value( $_->default_assignee->email ),
>+ default_qa_contact =>
>+ type('string')->value( $_->default_qa_contact->email ),
>+ default_cc => [ map $_->email, @{ $_->initial_cc } ],
No, use login, not email, because that's what you can pass to User.get.
We generally shouldn't return anything that you can't pass to some other function to get more info about it. That's a good rule of thumb for "what do we return?"
>+This part of the Bugzilla API allows you to list accessible components and
It doesn't allow you to "list" anything.
>+=head2 List Components
"List" is the wrong word.
>+Returns a list of accessible components information for components ids
>+or component and product names passd to it.
That sentence doesn't make any sense. :-)
>+=item B<Params>
>+
>+A hash containing either C<ids>, that is an array of components ids,
>+or C<products_and_components>, that is an array of hashes, each hash has
>+product name and component name.
Why not both?
Attachment #300031 -
Flags: review?(mkanat) → review-
![]() |
||
Comment 18•18 years ago
|
||
Hi Max,,
Sorry but I am still a bit confused with the design of this function,, basically we only want to return components information for components that belongs to accessible products by the user.
in your review you commenting about :
my @obj_by_comp_prod = map Bugzilla::Component->check(
>+ { product => Bugzilla::Product->check( { name => $_->{product} } ),
that I shouldn't use Bugzilla::Product->check() caze it will not check for product accessibility, but I am only using that to get the component objects for the passed component/product names in the params->{names} , **as also** we have to get component objects for passed component ids without checking for the product accessibility,, so my point is that as we are getting the component objects for params->{ids} without checking product accessibility then we will do the same for params->{names} and then after we get all the component objects we can filter them based on their product_id and return to the user only the accessible ones.
by:
for my $comp_obj (@components_objs){
if ( grep ( $_ eq $comp_obj->product_id, @accessible_product_ids ) ){
push @accessible_components, $comp_obj;
}
}
or even we can use here the Bugzilla->user->can_enter_product instead as you are suggesting so can get all the different error messages. so I mean we do the accessibility check after we get all the component objects as we will have to be doing it anyways for the component objects that we got by components ids. I hope I am making sense. and maybe I am still confused and not understanding your point ??!!
Thanks,
Noura
![]() |
||
Comment 19•18 years ago
|
||
Okay, I'm realizing that I was unclear, you're right. In fact, it's because I forgot to say something! :-)
If you ask for an invalid name or an invalid id, you should get an error. *However*, the error has to say "either that product doesn't exist or you can't access it". It can't just say "that product doesn't exist."
Summary: Patch to return component information when products are queried in the webservice → Ability to get information about Components in the WebService
Version: unspecified → 3.1.2
![]() |
||
Comment 20•18 years ago
|
||
Hi Max,
I have made few changes to reflect your last comment. so basically this is the new design:
1- get all the components objects for the passed component ids
2- get all the components objects for the passed component/product names using the function Object::check to validate component and product names.
3- loop through those objects to check if the user can enter bugs to the products of those components, if one violation found then an error will be thrown to tell the user that they have no access to that product then the function will exit.
4- if all passed and user has access to the products of all component ids and product/component names that they passed then return component infos for everything they passed.
also fixed the POD to make more sense, and changed to params->{names} and its keys' names as you and Dave suggested.
Thanks,
Noura
Attachment #300031 -
Attachment is obsolete: true
Attachment #301222 -
Flags: review?(mkanat)
![]() |
||
Comment 21•18 years ago
|
||
Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.
Target Milestone: Bugzilla 3.2 → Bugzilla 4.0
![]() |
||
Updated•18 years ago
|
Flags: blocking3.2?
![]() |
||
Comment 22•18 years ago
|
||
IMO, not blocking 3.2 as we have enough regressions/sec bugs to fix these days and I don't want to have another new method to track before 3.2.
![]() |
||
Comment 23•18 years ago
|
||
Okay. In that case, this is going into 4.0.
Flags: blocking3.2? → blocking3.2-
![]() |
||
Comment 24•18 years ago
|
||
Hi, This is a new patch for the function Component.get(). I was coding a function to update component then found it useful here to use separate function to return the accessible component objects from passed ids and names so we don't repeat code as how was suggested by Max for the User module. I have done this for the component module. soon will submit a patch for Component.update() that will reuse the code.
Please review when you can.
Thanks,
Noura
Attachment #301222 -
Attachment is obsolete: true
Attachment #305455 -
Flags: review?(mkanat)
Attachment #301222 -
Flags: review?(mkanat)
![]() |
||
Updated•18 years ago
|
Attachment #305455 -
Flags: review?(LpSolit)
![]() |
||
Updated•18 years ago
|
Attachment #269158 -
Attachment is obsolete: true
![]() |
||
Comment 25•18 years ago
|
||
Comment on attachment 305455 [details] [diff] [review]
v4 for Bugzilla::WebService::Component::get()
>Index: Bugzilla/WebService/Component.pm
>+# Software distributed under the License is distributed on an "AS
>+# IS" basis, WITHOUT WARRANTY OF ANY KIND, either express or
>+# implied. See the License for the specific language governing
>+# rights and limitations under the License.
>+#
>+# The Original Code is the Bugzilla Bug Tracking System.
You must include an Original Developer section.
>+sub get {
>+ my ( $self, $params ) = @_;
Nit: We don't usually add extra spaces around the parameters like that.
>+ default_cc => [ map $_->{login_name}, @{ $_->initial_cc } ],
Whoa, don't *ever* use direct hash access on objects. That should be $_->login.
Also, make sure that this returns an array of strings, always. (I'm pretty sure it will, just confirm that for me, verbally, here.)
>+# subroutine to hold repeated code between Component.get and
>+# Component.update, this function returns the unique accessible
>+# Component objects to the user from list of component ids and
>+# Component/product names.
>+sub get_accessible_components {
Add a _ in front of the name of this so that it's not a public function.
We can create another function, in a separate bug, for listing a product's components.
>+ # to get the component objects for product/component combination
>+ # first obtain the product object from the passed product name
>+ my @component_objs = map Bugzilla::Component->check(
>+ { product => Bugzilla::Product->check( { name => $_->{product} } ),
Unfortunately you can't do this this way. Here's the problem:
When somebody asks to access a component in a product they can't access, we have to tell them "Either that product doesn't exist or you don't have access to it." We can't tell them "That component doesn't exist" because then they could use that to confirm the existence of a product name. Product names can be highly confidential in some Bugzillas, so this type of protection is very important.
Probably what you need to do is to override Bugzilla::Product->check in Bugzilla::Product and add another parameter that specifies what kind of error we throw. For example, in the admin interfaces we still want to say "That Product doesn't exist", because admins get to know everything. But users should get a different error, more like the one I said above.
>+ my %unique_components;
>+ map { $unique_components{$_->id} = $_ } @component_objs;
You're not really supposed to use map like this. That is, it shouldn't be *modifying* something in that block. Either use it more like I said in the User.get bug, or convert this to a single-line foreach statement.
>+ for my $comp_obj (@component_objs) {
>+ if (Bugzilla->user->can_enter_product($comp_obj->product->name, THROW_ERROR)){
>+ push @accessible_components, $comp_obj;
>+ }
can_enter_product is not the correct check. Please see what WebService::Product::get_accessible_products does.
>+Returns information about requested components that only belongs to
>+enterable products by the user.
Returns information about L<Bugzilla::Component|Components> in accessible L<Bugzilla::Product|Products>.
(I might have that L syntax backwards.)
>+=item B<Params>
>+
>+A hash containing either C<ids>, that is an array of components ids,
>+or C<names>, that is an array of hashes, each hash has
>+product name and component name, or both together.
Write this out more like we did for User.get.
>+=item id
>+
>+C<int> The id of the compnent.
"The unique integer ID that Bugzilla uses to identify this component. Even if the name of the component changes, this ID will stay the same."
>+C<array> A list of all the login names in the default cc list of the component.
"The default CC list for this component. An array of login names."
Attachment #305455 -
Flags: review?(mkanat)
Attachment #305455 -
Flags: review?(LpSolit)
Attachment #305455 -
Flags: review-
![]() |
||
Comment 26•18 years ago
|
||
new patch for function Component.get(). I have made the fixes suggested in the previous patch review, with regarding to returning different errors in case of passing invalid component for some product I have handled that as suggested so:
if the normal user is requesting an info about component for a product they can not access weather the component name they passed was valid or not they get an error saying "Either the product 'product_name' does not exist or you don't have access to it" so this way they will not know if the product is there or not.
but if the user has access to the product then they get to know wether the component they passed was valid or not.
Please review when you can.
Noura
Attachment #305455 -
Attachment is obsolete: true
Attachment #310728 -
Flags: review?(mkanat)
![]() |
||
Comment 27•17 years ago
|
||
Comment on attachment 310728 [details] [diff] [review]
Bugzilla::WebService::Component::get()
Okay, sorry that it took me a TREMENDOUSLY long time to get to this!
>+++ Bugzilla/WebService/Component.pm 2008-03-20 20:06:37.000000000 +1000
Hmm...do we want this, or do we just want to put things in Product...?
>+# The Initial Developer of the Original Code is Netscape Communications
No, untrue. Please fix that.
>+# function to return component information when passed either,
>+# component ids or component and product names or both togther.
>+# exmaple of how it will be called:
> [snip]
The API looks good. We should use validate() on the ids and names arguments, now that we have validate().
>+sub get {
> [snip]
>+ map {
>+ { id => type('int')->value($_->id),
>+ name => type('string')->value($_->name),
>+ description => type('string')->value($_->description),
>+ product_id => type('int')->value($_->product_id),
Let's give them the product name instead of the ID. I think it was a mistake to return ids from the Product functions. :-(
Might want to use filter() here now that we have it, to implement include_fields and exclude_fields.
The return API looks good, otherwise.
>+sub _get_accessible_components {
> [snip]
>+ foreach my $item (@{$params->{names}})
>+ {
Nit: Brace goes on the same line, there.
We should throw an error if people try to access a component they can't see. Perhaps Bugzilla::Component->check should just be modified to behave that way. However, remember not to say it doesn't exist, but just to say that it might not exist or that it might be invisible.
>+ if ($accessible_products{$item->{product}}){
Unfortunately that will be case-sensitive while Bugzilla is case-insensitive. To solve this you could lc() things, though unfortunately the DB's lowercasing and Perl's lowercasing are not the same (the DB's is better, usually, I believe).
>+=item C<get> B<UNSTABLE>
Note that standard POD formatting for the API docs has changed since you posted this patch--look at the current stuff.
>+=item C<ids> (array) - An array of integers, representing components ids.
component ids
>+=item C<names> (array) - An array of hashes, each hash has product name
>+and component name.
You'll need to be clearer than that.
Attachment #310728 -
Flags: review?(mkanat) → review-
![]() |
||
Updated•14 years ago
|
Assignee: nelhawar → webservice
![]() |
||
Comment 28•14 years ago
|
||
Now that Product.get returns all data about components (bug 655229), do we still need or want a separate Component.get method? I'm not that sure.
Depends on: 655229
Comment 29•14 years ago
|
||
(In reply to Frédéric Buclin from comment #28)
> Now that Product.get returns all data about components (bug 655229), do we
> still need or want a separate Component.get method? I'm not that sure.
It wouldn't hurt I suppose to have a Component.get inside of Component.pm. It would just return the data related to one component providing the user provides a valid product/component value. We need to think about filling out the missing functions in the WebServices code such as Version.pm, Milestone.pm. Each having a corresponding get(), create(), update() and remove() method for doing administrative tasks over WebServices.
I can start creating individual bugs for each module and/or method if you think we should go this route.
dkl
![]() |
||
Comment 30•14 years ago
|
||
Instead of having an individual Component.pm, Milestone.pm, and Version.pm, I was planning to just put everything relating to those into Product.pm. But that might not be the right way to go; it's sort of based on what we see from real user needs.
Comment 31•14 years ago
|
||
(In reply to Max Kanat-Alexander from comment #30)
> Instead of having an individual Component.pm, Milestone.pm, and Version.pm,
> I was planning to just put everything relating to those into Product.pm. But
> that might not be the right way to go; it's sort of based on what we see
> from real user needs.
I feel like we should keep the WebService methods individually as simple as we can. Product.pm should have a method Product.update that only allows for editing product meta data similar to
how editproducts.cgi works now. And then have separate Component.update, Version.update, Milestone.update that only operate on the individual items. We do this now with editcomponents.cgi, editversions.cgi, editmilestones.cgi, etc. If we do this all in Product.update the code will be more complex than it needs to be. The separate methods will be able to operate on either ids or product/component combinations.
dkl
![]() |
||
Comment 32•14 years ago
|
||
(In reply to David Lawrence [:dkl] from comment #31)
> If we do this all in Product.update the code will
> be more complex than it needs to be.
But you could have Product.create_milestone, Product.update_milestone, etc... I have no strong opinion on this topic yet.
![]() |
||
Comment 33•14 years ago
|
||
Okay. I'm all for simplicity. We may also want to consider whether or not there should be some more generic methods for creating field values. Of course, the ones that have special fields, like Component, would need special handling. But Version and Milestone are just generic product-specific fields, pretty much, and could possibly be handled by something like FieldValue.create({ product =>, name =>, sort_key => }).
Comment 34•14 years ago
|
||
(In reply to Frédéric Buclin from comment #32)
> (In reply to David Lawrence [:dkl] from comment #31)
> > If we do this all in Product.update the code will
> > be more complex than it needs to be.
>
> But you could have Product.create_milestone, Product.update_milestone,
> etc... I have no strong opinion on this topic yet.
I would personally like to see everything broken out into their own logical modules
instead of having a large amount of new "utility" methods polluting the Product module.
I understand that components/versions/milestones are all connected to some "product" but when I think of creating a component, I would automatically think Component.create (similar to Bug.create) instead of Product.create_component. You would just pass in the product name to Component.create similar to how we do product=SomeProduct for editcomponents.cgi.
(In reply to Max Kanat-Alexander from comment #33)
> Okay. I'm all for simplicity. We may also want to consider whether or not
> there should be some more generic methods for creating field values. Of
> course, the ones that have special fields, like Component, would need
> special handling. But Version and Milestone are just generic
> product-specific fields, pretty much, and could possibly be handled by
> something like FieldValue.create({ product =>, name =>, sort_key => }).
I see that side of the argument too and we will definitely need have some sort of Field.create module for creating/editing the other various types of fields. But to me it makes more sense to have {Component,Version,Milestone}.create just call their relevant Bugzilla::Field->create functions in the separate methods. It just make the API more understandable to me. Also works well in relation to REST as the paths will map to the relevant modules
/rest/component (POST) => Component.create
/rest/component/id (PUT) => Component.update
/rest/component/id (DELETE) => Component.remove
and so on for the other modules, /rest/version and /rest/milestone.
dkl
![]() |
||
Comment 35•14 years ago
|
||
At least Components seem like they're worth having their own module, although then when we have Field.create, people may be confused why they can't add Components through it. Still, I'm with you for at least Component.create/update.
![]() |
||
Comment 36•14 years ago
|
||
Field.create makes me think about custom fields, not product-specific fields. And you don't create a field when talking about milestones or versions or even components. You are adding a value to the list of milestones/versions/components of the product. Field.create would be a bad name in this case.
Comment 37•14 years ago
|
||
(In reply to Frédéric Buclin from comment #36)
> Field.create makes me think about custom fields, not product-specific
> fields. And you don't create a field when talking about milestones or
> versions or even components. You are adding a value to the list of
> milestones/versions/components of the product. Field.create would be a bad
> name in this case.
Agree. Having a Field.create to create a new custom field would be great for us as we get tasked to do that quite often. Having {Component,Version,Milestone}.create would make me think adding a new value, not creating an entirely new field.
dkl
![]() |
||
Comment 38•14 years ago
|
||
Okay. FieldValue.create would be better then.
![]() |
||
Comment 39•13 years ago
|
||
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.
Target Milestone: Bugzilla 4.4 → Bugzilla 5.0
![]() |
||
Updated•11 years ago
|
Target Milestone: Bugzilla 5.0 → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•