787734 - Implement create, update and remove methods for Classification API class

Status: NEW

(Bugzilla :: WebService, enhancement)

Comment 1

[Koosha KM]

Attachment: patch (Classification.create method) - v1

Comment 2

[Koosha KM]

Attachment: patch (Classification.update method - without POD) - v1

POD will follow once the code is approved.

Comment 3

[Koosha KM]

Attachment: patch (Classification.remove method - without POD) - v1

POD will follow once the code is approved.

Comment 4

[Koosha KM]

Attachment: patch (Classification.update method - without POD) - v1

Comment 5

[Koosha KM]

To apply the patches correctly: first apply create(), then update() and finally remove() methods.

Comment 6

[Frédéric Buclin]

Comment on attachment 663800 [details] [diff] [review]
patch (Classification.create method) - v1

>+    my ($self, $params) = validate(@_);

What's the point to call validate() if you don't specify @keys?


>+    defined $params->{name} ||
>+        ThrowCodeError('param_required', { function => 'Classification.create',
>+                                           param => 'name' });

Put || at the beginning of the next line, to follow our guidelines:

  defined $foo
    || $bar


>+=head1 Classification Modification

Must be "Classification Creation and Modification"


>+Creates a new classification with the information given.

"with the information given" seems pretty obvious. Use "This allows you to create a new classification in Bugzilla" to match the wording used in Product.pm and Group.pm


>+You should supply the following parameters in order to create a new classification:

"You should" sounds like you are not required to do it. Please follow the wording used in Product.pm and Group.pm, for consistency:

"Some params must be set, or an error will be thrown. These params are marked Required." with "Required" being bold.


>+=item C<name> (Required)

In Product.pm and Group.pm, "Required" is not in parens but bold. Please be consistent.


>+=item C<description> (Optional)

We don't write "Optional". We only mention "Required" when the parameter is mandatory.


>+=item C<sort_key> (Optional)

Same here.


>+=item 900 (Classification not enabled)

>+=item 304 (Authorization Failure)

Please sort error IDs as we do everywhere else.


I didn't test your patch yet but it looks good.

Comment 7

[Frédéric Buclin]

Comment on attachment 666375 [details] [diff] [review]
patch (Classification.update method - without POD) - v1

>+    my $classification = _get_classification_obj('Classification.update', $params);

_get_classification_obj() doesn't exist.


Also, this method has no POD.

Comment 8

[Frédéric Buclin]

Comment on attachment 666372 [details] [diff] [review]
patch (Classification.remove method - without POD) - v1

>+    my ($self, $params) = validate(@_);

Why calling validate() without @keys being set?


>+    my $classification = _get_classification_obj('Classification.remove', $params);

_get_classification_obj() doesn't exist.


>+    $classification->remove_from_db();

This method can throw an error if you try to delete the default classification. The POD, which is missing, must mention it.

Comment 9

[Koosha KM]

Attachment: patch (Classification.create() method + POD) - v1.1

Comment 10

[Koosha KM]

Attachment: patch (Classification.update() method + POD) - v1.1

Comment 11

[Koosha KM]

Attachment: patch (Classification.remove() method + POD) - v1.1

Comment 12

[Koosha KM]

Attachment: patch (Classification.remove() method + POD) - v1.1

Forgot to include the change made to Constants.pm.

Comment 13

[Frédéric Buclin]

Comment on attachment 673802 [details] [diff] [review]
patch (Classification.create() method + POD) - v1.1

>+    $user->in_group('editclassifications')
>+      || ThrowUserError('auth_failure', { group  => 'editcomponents',

The group in the error message is wrong. It must be editclassifications.


>+=item 900 (Classification not enabled)

An error is thrown if you try to create a classification with an existing name. It needs to be listed here and an error id must be added to Constants.pm.


Otherwise your patch looks good and works fine.

Comment 14

[Koosha KM]

These could be fixed on check in :(
That's alright though; I'll attach a new patch.

Comment 15

[Frédéric Buclin]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #14)
> These could be fixed on check in :(

Not really. One has to find the error message thrown, add it to Constants.pm, make sure the correct error code is returned and updating the patch. That's not something we can do on checkin.


> That's alright though; I'll attach a new patch.

I need the new patch for create() before reviewing the ones for update() and remove() as they depend on each other.

Comment 16

[Koosha KM]

Attachment: patch - v2

I attach the whole patch in a single file, but you may review it on a method-by-method basis.

Comment 17

[Frédéric Buclin]

Comment on attachment 688169 [details] [diff] [review]
patch - v2

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

>+sub create {

>+    defined $params->{name}
>+      || ThrowCodeError('param_required', { function => 'Classification.create',
>+                                           param => 'name' });

For consistency with other create() methods (e.g. in Product.pm and Group.pm), we shouldn't throw the generic param_required error but let validators do their job to have a more easily recognizable error code.


>+    my $classification = Bugzilla::Classification->create($params);

It can throw more errors than those you listed: classification_not_specified, classification_name_too_long, and classification_invalid_sortkey.



>+sub update {

>+    defined $params->{updates}
>+      || ThrowCodeError('param_required', { function => 'Classification.update',
>+                                            param => 'updates' });

As said in another review in another bug, please don't use this 'updates' key (and your documentation doesn't mention it either). Also, for consistency, you should take names and ids as input, instead of name and id. This will also resolve the conflict between the old and new names.


>+    my $user = Bugzilla->login(LOGIN_REQUIRED);
>+    
>+    Bugzilla->params->{'useclassification'}
>+      || ThrowUserError('auth_classification_not_enabled');
>+
>+    $user->in_group('editclassifications')
>+      || ThrowUserError('auth_failure', { group  => 'editclassifications',
>+                                          action => 'edit',
>+                                          object => 'classifications' });

As this exact same code is used for create(), update() and new(), it should be refactored into a _validate_user() method or something like that:

    my $user = _validate_user(action => add|update|remove);


>+    my $classification = _get_classification_obj('Classification.update', $params);

Use the existing params_to_objects() function instead.


>+    foreach my $field (qw(name description sortkey)) {
>+        my $key = MAPPED_RETURNS->{$field} || $field;

Why don't use call translate() here too?

Also, the list of errors thrown is incomplete.


>+sub remove {

Same comments as for update().


>+sub _get_classification_obj {

This method should die. We don't need it.


I didn't read the POD yet.

Comment 18

[Frédéric Buclin]

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.