690879 - Webservice Bug.fields should include include keyword name and description as legal values

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Frank Becker]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_7_1) AppleWebKit/534.48.3 (KHTML, like Gecko) Version/5.1 Safari/534.48.3

Steps to reproduce:

run Bug.fields for keyword


Actual results:

values is not included in the result


Expected results:

the result should have an entry values which is an array of hashes with the name and description of all keywords

Comment 1

[Frank Becker]

Attachment: patch V1

Comment 2

[David Lawrence [:dkl]]

Comment on attachment 608455 [details] [diff] [review]
patch V1

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

Looks good but a few changes.

::: Bugzilla/WebService/Bug.pm
@@ +111,5 @@
> +        } 
> +
> +        if ($field->name eq 'keywords') {
> +             $has_values = 1;
> +             @values = @{ $self->_legal_field_values({ field => $field }) };

You can just add the conditional to the one above like so:

        if ( ($field->is_select and $field->name ne 'product')
             or grep($_ eq $field->name, PRODUCT_SPECIFIC_FIELDS)
             or $field->name eq 'keywords')
        {
             $has_values = 1;
             @values = @{ $self->_legal_field_values({ field => $field }) };
        }

@@ +210,5 @@
> +               name     => $self->type('string', $value->name),
> +               description => $self->type('string', $value->description),
> +            });
> +        }
> +    } else {

Nit-pick: Move else { down one line

   }
   else {

@@ +1145,5 @@
>  Note that for per-product fields, C<value_field> is set to C<'product'>
>  and C<visibility_values> will reflect which product(s) this value appears in.
>  
> +=item C<description>
> +C<string> The Description for the Keyword.

Add a blank line between =item and C<string> to make the doc display properly.

Also since this part of the doc should be generic and not specific to any field, change to:

C<string> The description of the value

Comment 3

[Frank Becker]

Attachment: patch V2

next version.

Points from review of patch V1 are corrected.

Comment 4

[David Lawrence [:dkl]]

Comment on attachment 608594 [details] [diff] [review]
patch V2

Looks good and works as expected r=dkl

Comment 5

[Frédéric Buclin]

Unless I miss something, the description is only available for keywords, right? This makes the API pretty inconsistent, IMO, but I could probably live with that as there are already special data for the bug status already. It must at least be clearly written in the documentation, which this patch is lacking. So holding approval till the patch is updated.

This API change is too late for 4.2 as we released 4.2 final already.

Comment 6

[Frank Becker]

Attachment: patch V3

I only change the documentation as requested in comment#5

Comment 7

[David Lawrence [:dkl]]

(In reply to Frédéric Buclin from comment #5)
> Unless I miss something, the description is only available for keywords,
> right? This makes the API pretty inconsistent, IMO, but I could probably
> live with that as there are already special data for the bug status already.
> It must at least be clearly written in the documentation, which this patch
> is lacking. So holding approval till the patch is updated.

The values data has several other inconsistent key/values so adding 'description' doesn't hurt a whole lot. The others only display for specific fields as well. Having different documentation per field returned would also add substantially to the documentation and be redundant.
 
> This API change is too late for 4.2 as we released 4.2 final already.

I assumed since Bug.history is still set as UNSTABLE, it would be fine to put such a small change in for 4.2.1.

dkl

Comment 8

[David Lawrence [:dkl]]

Comment on attachment 610486 [details] [diff] [review]
patch V3

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

Looks good. r=dkl

Comment 9

[Frédéric Buclin]

(In reply to David Lawrence [:dkl] from comment #7)
> I assumed since Bug.history is still set as UNSTABLE, it would be fine to
> put such a small change in for 4.2.1.

UNSTABLE across major releases, but should still be stable across minor releases, else it's going to be a nightmare for external tools to use our API.

Comment 10

[David Lawrence [:dkl]]

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

dkl

Comment 11

[Frédéric Buclin]

Added to the relnotes for 4.4.