469193 - WebService function to create new products (Product.create)

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Doug Warner]

Looking for a way to create new products from the webservices API call to speed up creation of new products.

This is for use at Mozdev so we can help test/debug/develop the extensions as well.

Comment 1

[Julien Heyman]

Attachment: Implement the Product.create webservice

I implemented the Product.create webservice.

You can find this implementation in this patch.

Comment 2

[Max Kanat-Alexander]

Hey, thanks for the patch! See our development process for how to get it reviewed and accepted:

  http://wiki.mozilla.org/Bugzilla:Developers

Comment 3

[Julien Heyman]

Attachment: Implement the Product.create webservice v2

Generals guidelines and Style OK.
./runtests.pl is OK.
Patch with bazar on trunk OK.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 518676 [details] [diff] [review]
Implement the Product.create webservice v2

  Hey, thanks for this patch!! :-)

>=== modified file 'Bugzilla/WebService/Product.pm'

  Ah, I think you need this at the top of the function:

  Bugzilla->login(LOGIN_REQUIRED);

>+    # Get parameters
>+    my $allows_unconfirmed = $params->{allows_unconfirmed}
>+        || ThrowCodeError('param_required', { param => 'allows_unconfirmed' });
>+    my $classification = $params->{classification};
>+    my $name = $params->{name}
>+        || ThrowCodeError('param_required', { param => 'name' });
>+    my $description = $params->{description}
>+        || ThrowCodeError('param_required', { param => 'description' });
>+    my $version = $params->{version}
>+        || ThrowCodeError('param_required', { param => 'version' });
>+    my $defaultmilestone = $params->{defaultmilestone};
>+    my $isactive = 1;
>+    my $create_series = $params->{create_series}
>+        || ThrowCodeError('param_required', { param => 'create_series' });

  Hmm, I don't think you need to check these individually, you can just pass them all to create() as they are, and create() will throw an error if required fields aren't specified.

  A few other notes:

  For the WebService, all booleans start with "is", but that's tricky for "allows_unconfirmed". Can you think of any good names that would start with "is"?

  Also, we'd want it to be "default_milestone" instead.

  Once this is all done, we also need POD for the method.

  Overall, looks really good, though! :-)

Comment 5

[Julien Heyman]

Attachment: Implement the Product.create webservice v3

Ok, I took all comments in this new patch.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 521514 [details] [diff] [review]
Implement the Product.create webservice v3

  Cool! :-)

>=== modified file 'Bugzilla/WebService/Product.pm'
>+sub create {
>+    my $self = shift;
>+    my ($params) = @_;

  That can just be my ($self, $params) = @_;

>+    my $product = Bugzilla::Product->create({
>+        allows_unconfirmed => $params->{isallows_unconfirmed},

  Sorry, I wasn't clear. "is_allows_unconfirmed" probably isn't a good name, doesn't sound right.

  Perhaps "is_allowing_unconfirmed"? That sounds weird, though. Maybe "has_unconfirmed"? Maybe gerv has some ideas here.

>+        isactive           => $params->{isactive},

  This should be is_open, I think.

>+        create_series      => $params->{create_series}

>+=head1 Create Product

  Should be: Product Creation and Modification

>+Some parameters can have defaults set in Bugzilla, by the administrator. If these parameters have defaults set, you can omit them. These parameters are marked Defaulted.

  You don't have to note defaulted parameters for this function. In Bug.create, "defaulted" means "there are parameters that may or may not be set that give defaults." There's nothing like that here.

>+C<name> (string) B<Required> - The name of this product.
>
>+C<description> (string) B<Required> - Set the description of this product.

  This should be formatted like:

=over

=item C<name> 

B<Required> C<string> The name of this product. Must be unique.

=item C<description> 

B<Required> C<string> A description for this product. Allows some simple HTML.

And so on.

>+A hash containing one item, C<ids>. This is the id of the newly-filed product.

  Actually, it should just be "id", since we only created one product.

>+=item B<Errors> (none)

  That's not true. Trace down every error that Bugzilla::Product->create might normally throw.

Comment 7

[Max Kanat-Alexander]

Hey Gerv. Any ideas on an API name for allows_unconfirmed? (See above comment.)

Comment 8

[Gervase Markham [:gerv]]

If you want to stick with the rule that booleans start "is" or "has", then "has_unconfirmed" seems like the best option. If you don't care about that rule, I'd go for "allows_unconfirmed".

Gerv

Comment 9

[Julien Heyman]

> Should be: Product Creation and Modification
The function create can't update an existing product no ?

Comment 10

[Max Kanat-Alexander]

  Okay, so let's call it has_unconfirmed.

(In reply to comment #9)
> > Should be: Product Creation and Modification
> The function create can't update an existing product no ?

  Mmm, fair enough, so Product Creation would be fine. I was just planning ahead for when we add an "update" function.

Comment 11

[Julien Heyman]

> That's not true. Trace down every error that Bugzilla::Product->create might
normally throw.

And how can I have the list of this errors ?

Comment 12

[Max Kanat-Alexander]

(In reply to comment #11)
> And how can I have the list of this errors ?

  Look at all the validators in Bugzilla::Product (all the _check functions).

Comment 13

[Julien Heyman]

Attachment: Implement the Product.create webservice v4

New patch.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 522344 [details] [diff] [review]
Implement the Product.create webservice v4

  Cool! Looks better. A few things still need fixing about the POD, though:

>+B<Required> C<string> - The name of this product. Must be unique.

  You don't need the hyphen.

>+=item B<Returns>    
>+
>+The new id product.

  Ah, be more explicit. (See the "Returns" for Bug.create.)

>+=item B<Errors>
>+
>+=over
>+
>+All errors throw by Bugzilla::Product->create(). 
>+
>+=back

  Ah, that's not enough, unfortunately. See how errors are listed out for other functions, like Bug.create. You may need to add new error codes in Bugzilla::WebService::Constants (that's where error names are mapped to numeric error codes).

Comment 15

[Julien Heyman]

OK, I find these errors :
product_blank_name
product_name_too_long
product_name_already_in_use
product_name_diff_in_case
product_must_have_description
product_must_have_version
product_must_define_defaultmilestone

What number to assign to these errors? or range of number ?

Comment 16

[Max Kanat-Alexander]

Hey! Look and see if there are already product errors in Bugzilla/WebService/Constants.pm and use the same range, if so. Create new codes as appropriate for new errors. If you want more detail, ask, but I won't be able to get back to you very soon about it.

Comment 17

[Julien Heyman]

Attachment: Implement the Product.create webservice v5

New version.

My Pod syntax may be not good...

But if someone know the good syntax, said me, and I repatch it.

Comment 18

[Max Kanat-Alexander]

Comment on attachment 522977 [details] [diff] [review]
Implement the Product.create webservice v5

  Cool! A few notes:

>=== modified file 'Bugzilla/WebService/Constants.pm'
>+    # Product erros

  Okay, actually, these need to start at 700 and be located below the attachment errors, as their own section.

>@@ -198,3 +223,99 @@
>+=item C<allows_unconfirmed> 

  Fix all of the POD to use the input argument names, not the internal names.

>+=item C<isactive> 
>+
>+C<boolean> Set if new product is active.

  Actually, the right description is:

  True if the product is currently allowing bugs to be entered into it.

>+=item B<Errors>
>+
>+Z<>

  You don't need that.

>+
>+=over
>+
>+=item C< 130 (Product blank name)>

  You don't need a C<> around this or any of these errors.

>+You must specify a no blank name for this product.

  "non-blank" instead of "no blank".

>+=item C< 131 (Product name too long)>
>+
>+You didn't specify a too long name for this product.

  The name specified for this product was longer than the maximum allowed length.

>+=item C< 132 (Product name already exists)>
>+
>+You specify an already product name. The name must be unique. Change it.

  You specified the name of a product that already exists. (Product names must be globally unique in Bugzilla.)

>+=item C< 133 (Product name diff in case)>
>+
>+Name is not the same. Change it.

  Actually, this should not be its own error. You should just point this error at code 132 in Bugzilla::WebService::Constants, and don't document it here.


  Otherwise it looks pretty good! :-)

Comment 19

[Julien Heyman]

Attachment: Implement the Product.create webservice v6

Hi, 

New version ;)

Comment 20

[Max Kanat-Alexander]

Comment on attachment 523976 [details] [diff] [review]
Implement the Product.create webservice v6

>=== modified file 'Bugzilla/WebService/Constants.pm'
>+    product_name_already_in_use => 702,
>+    product_name_diff_in_case => 703,

  Those should both be error 702.

Comment 21

[Frédéric Buclin]

(In reply to comment #20)
>   Those should both be error 702.

Does it really worth a r-? This could easily be fixed on checkin.

Comment 22

[Max Kanat-Alexander]

(In reply to comment #21)
> (In reply to comment #20)
> >   Those should both be error 702.
> 
> Does it really worth a r-? This could easily be fixed on checkin.

  I agree, but I just wanted jheyman to have the opportunity to fix it himself.

Comment 23

[Julien Heyman]

Attachment: Implement the Product.create webservice v7

OK.

I merge the 2 errors in the same.

Thanks to review when you can ;)

Comment 24

[Max Kanat-Alexander]

Comment on attachment 524043 [details] [diff] [review]
Implement the Product.create webservice v7

Looks great, thanks! :-)

Comment 25

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/WebService/Constants.pm
modified Bugzilla/WebService/Product.pm
Committed revision 7773.

Comment 26

[Frédéric Buclin]

Added to relnotes in bug 713346.

Comment 27

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/qa/4.2/
added t/webservice_product_create.t
Committed revision 223.

I found several problems when testing Product.create, see bug 714446.

Comment 28

[Frédéric Buclin]

(In reply to Max Kanat-Alexander from comment #6)
>   This should be is_open, I think.

Why using is_open for Product.create while Product.get uses is_active? This is inconsistent, and I always mix both arguments. I always have to check the doc to use the correct one. This is painful.

Comment 29

[Max Kanat-Alexander]

(In reply to Frédéric Buclin from comment #28)
> Why using is_open for Product.create while Product.get uses is_active? This
> is inconsistent, and I always mix both arguments. I always have to check the
> doc to use the correct one. This is painful.

  Oh, they use different things? They should be consistent. "is_active" means whether or not the value shows up in any drop down anywhere, for most field values, right? That's why I wanted to use is_open.