WebService function to create new products (Product.create)

RESOLVED FIXED in Bugzilla 4.2

Status

()

--
enhancement
RESOLVED FIXED
10 years ago
7 years ago

People

(Reporter: doug, Assigned: julien.heyman)

Tracking

3.1.1
Bugzilla 4.2
Bug Flags:
approval +
testcase +

Details

Attachments

(1 attachment, 6 obsolete attachments)

(Reporter)

Description

10 years ago
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.

Updated

10 years ago
OS: Linux → All
Hardware: PC → All
Summary: WebService function to create new products → WebService function to create new products (Product.create)
(Assignee)

Comment 1

8 years ago
Created attachment 518054 [details] [diff] [review]
Implement the Product.create webservice

I implemented the Product.create webservice.

You can find this implementation in this patch.

Comment 2

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

  http://wiki.mozilla.org/Bugzilla:Developers
(Assignee)

Comment 3

8 years ago
Created attachment 518676 [details] [diff] [review]
Implement the Product.create webservice v2

Generals guidelines and Style OK.
./runtests.pl is OK.
Patch with bazar on trunk OK.
Attachment #518054 - Attachment is obsolete: true
Attachment #518676 - Flags: review?(mkanat)

Comment 4

8 years ago
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! :-)
Attachment #518676 - Flags: review?(mkanat) → review-

Updated

8 years ago
Assignee: webservice → jheyman
Target Milestone: --- → Bugzilla 4.2
(Assignee)

Comment 5

8 years ago
Created attachment 521514 [details] [diff] [review]
Implement the Product.create webservice v3

Ok, I took all comments in this new patch.
Attachment #518676 - Attachment is obsolete: true
Attachment #521514 - Flags: review?(mkanat)

Comment 6

8 years ago
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.
Attachment #521514 - Flags: review?(mkanat) → review-

Comment 7

8 years ago
Hey Gerv. Any ideas on an API name for allows_unconfirmed? (See above comment.)
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
(Assignee)

Comment 9

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

Comment 10

8 years ago
  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.
(Assignee)

Comment 11

8 years ago
> 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

8 years ago
(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).
(Assignee)

Comment 13

8 years ago
Created attachment 522344 [details] [diff] [review]
Implement the Product.create webservice v4

New patch.
Attachment #521514 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #522344 - Flags: review?(mkanat)

Comment 14

8 years ago
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).
Attachment #522344 - Flags: review?(mkanat) → review-
(Assignee)

Comment 15

8 years ago
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

8 years ago
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.
(Assignee)

Comment 17

8 years ago
Created attachment 522977 [details] [diff] [review]
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.
Attachment #522344 - Attachment is obsolete: true
Attachment #522977 - Flags: review?(mkanat)

Comment 18

8 years ago
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! :-)
Attachment #522977 - Flags: review?(mkanat) → review-
(Assignee)

Comment 19

8 years ago
Created attachment 523976 [details] [diff] [review]
Implement the Product.create webservice v6

Hi, 

New version ;)
Attachment #522977 - Attachment is obsolete: true
Attachment #523976 - Flags: review?(mkanat)

Comment 20

8 years ago
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.
Attachment #523976 - Flags: review?(mkanat) → review-

Comment 21

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

Does it really worth a r-? This could easily be fixed on checkin.
Status: NEW → ASSIGNED

Comment 22

8 years ago
(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.
(Assignee)

Comment 23

8 years ago
Created attachment 524043 [details] [diff] [review]
Implement the Product.create webservice v7

OK.

I merge the 2 errors in the same.

Thanks to review when you can ;)
Attachment #523976 - Attachment is obsolete: true
Attachment #524043 - Flags: review?(mkanat)

Comment 24

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

Looks great, thanks! :-)
Attachment #524043 - Flags: review?(mkanat) → review+

Updated

8 years ago
Flags: approval+

Comment 25

8 years ago
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/WebService/Constants.pm
modified Bugzilla/WebService/Product.pm
Committed revision 7773.
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: relnote
Resolution: --- → FIXED

Updated

7 years ago
Flags: testcase?(mkanat)

Comment 26

7 years ago
Added to relnotes in bug 713346.
Keywords: relnote

Updated

7 years ago
Blocks: 714446

Comment 27

7 years ago
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/qa/4.2/
added t/webservice_product_create.t
Committed revision 223.

I found several problems when testing Product.create, see bug 714446.
Flags: testcase?(mkanat) → testcase+

Comment 28

7 years ago
(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

7 years ago
(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.
You need to log in before you can comment on or make changes to this bug.