714446 - Product.create default behavior is broken and inconsistent with POD

Status: RESOLVED FIXED

(Bugzilla :: WebService, defect)

Description

[Frédéric Buclin]

Documentation for Product.create states that error 705 (Product must define a defaut milestone) can be thrown, which is incorrect. This error can only be thrown when updating a product (in the case where the admin chooses a milestone which doesn't exist). This means that the POD for Product.update must also be fixed as it can throw this error but not Product.create.

Moreover, the current POD and/or default behavior of Product.create is confusing. The defaults in the DB is to allow UNCONFIRMED and to mark the product as accepting new bugs. The same defaults are used when creating a new product from editproducts.cgi. But when creating a product with Product.create, the behavior is exactly the opposite, i.e. UNCONFIRMED is forbidden and new bugs are not allowed unless otherwise specified. Either Product.create must make sure that these parameters have been defined before passing them to Bugzilla::Product->create (and maybe assume they are true when left undefined), or the POD must be improved to explicitly mention what the defaults are, as most admins are used to the defaults in editproducts.cgi.

Comment 1

[Frédéric Buclin]

Oh, and if classifications are enabled, Product.create incorrectly requires one:

not ok 45 - XML-RPC: Passing the name, description and version only works

#   Failed test 'XML-RPC: Passing the name, description and version only works'
#   at lib/QA/RPC.pm line 78.
# You must select/enter a classification.

If no classification is specified, it should fallback to the classification with ID = 1 (by default: Unclassified, but its name can be edited, so you must not rely on its name, but on its ID only).

Also, if you specify a classification name which doesn't exist, an error will be thrown. This error ID must be listed in the Errors section too.

Comment 2

[Max Kanat-Alexander]

To be clear, the intention for all WebServices is that the defaults be identical to the defaults in the database.

Comment 3

[Frédéric Buclin]

Attachment: patch for 4.2, v1

Comment 4

[Frédéric Buclin]

Attachment: patch for trunk, v1

Comment 5

[Max Kanat-Alexander]

Comment on attachment 591293 [details] [diff] [review]
patch for 4.2, v1

Review of attachment 591293 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/WebService/Product.pm
@@ +115,5 @@
> +        isactive           => defined $params->{is_open} ?
> +                                $params->{is_open} : 1,
> +        create_series      => defined $params->{create_series} ?
> +                                $params->{create_series} : 1
> +    };

Instead of this, args should only be passed if they are defined. That way the underlying systems pick the defaults instead of having them manually encoded here. Generally we want as little logic as possible to happen in the WebServices, and as much as possible to happen in the backend.

Perhaps there should be a _map_fields that does this properly.

Comment 6

[David Lawrence [:dkl]]

Comment on attachment 591294 [details] [diff] [review]
patch for trunk, v1

Review of attachment 591294 [details] [diff] [review]:
-----------------------------------------------------------------

r- based on mkanats comment. I agree that we should not be setting defaults in the WebService code itself and let the 
objects create() method dictate defaults or generate errors for missing params.

dkl

Comment 7

[Frédéric Buclin]

Attachment: patch for 4.2, v2

Comment 8

[Frédéric Buclin]

Attachment: Fix for webservice_product_create.t

I updated webservice_product_create.t to check is_active and has_unconfirmed.

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

Comment 9

[Frédéric Buclin]

Attachment: patch for trunk, v2

Comment 10

[David Lawrence [:dkl]]

Comment on attachment 592299 [details] [diff] [review]
patch for 4.2, v2

Review of attachment 592299 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good and works as expected. r=dkl

Comment 11

[David Lawrence [:dkl]]

Comment on attachment 592748 [details] [diff] [review]
patch for trunk, v2

Review of attachment 592748 [details] [diff] [review]:
-----------------------------------------------------------------

Along with the spelling fix, add default for default_milestone:

=item C<default_milestone> 

C<string> The default milestone for this product. Default '---'

Otherwise rest looks good and works as expected. r=dkl

::: Bugzilla/WebService/Product.pm
@@ +720,5 @@
> +=item 704 (Product must have version)
> +
> +You must specify a version for this product.
> +
> +=item 705 (Product must define a defaut milestone)

s/defaut/default/

(fix on commit)

Comment 12

[David Lawrence [:dkl]]

(In reply to David Lawrence [:dkl] from comment #10)
> Comment on attachment 592299 [details] [diff] [review]
> patch for 4.2, v2
> 
> Review of attachment 592299 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good and works as expected. r=dkl

Also add Default: '---' for default_milestone in the POD doc similar to comment 11. Fix on commit.

dkl

Comment 13

[Frédéric Buclin]

Thanks, I will commit these patches with the fixes on checkin in a few minutes.

Comment 14

[Frédéric Buclin]

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

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