506559 - WebService function to get information about fields (Bug.fields)

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Frank Becker]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_7; de-de) AppleWebKit/530.19.2 (KHTML, like Gecko) Version/4.0.2 Safari/530.19
Build Identifier: 

We need an function that list all fields with the following information:
id,  type, custom, name, description, enter_bug, visibility_field_id, visibility_value_id, value_field_id and the values if the type of the field is single or multi select.

Reproducible: Always

Comment 1

[Frank Becker]

Attachment: patch, V1

Comment 2

[Max Kanat-Alexander]

Comment on attachment 390731 [details] [diff] [review]
patch, V1

This should be in "Bug" and just be called "fields" and should also probably deprecate legal_values.

You definitely should also not be passing around field ids or value ids.

Comment 3

[Frank Becker]

Attachment: patch, V2

This is an updated Version of patch, V1 with the changes of comment#2

Comment 4

[Frank Becker]

Attachment: patch, V3

Sorry,

did not include the patch for the invalid bug# 506545 (bug_status details).

Comment 5

[Frank Becker]

Attachment: patch, V4

I switch two int fields to boolean.

Found this during my test while implementing the mylyn part.

Comment 6

[Frank Becker]

Attachment: patch, V5

Now for bug_status we return an hash with the two items name and required_comment under the can_change_to entry

Comment 7

[Frank Becker]

Comment on attachment 405694 [details] [diff] [review]
patch, V5

Did not know if the first reviewer was the right choice.

Comment 8

[Max Kanat-Alexander]

Comment on attachment 405694 [details] [diff] [review]
patch, V5

>Index: Bugzilla/WebService/Bug.pm
>+sub fields {
>+    my ($self) = @_;
>+    my @fields = @{Bugzilla::Field->match({obsolete => 0})};

  Just use Bugzilla->get_fields there, should be simpler.

>+        my $va;
>+        my $va_erg;

  Those are totally cryptic names, please use more descriptive names. Also, please declare them either where they are initialized or closer to where they are initialized.

>+        my $vi_field= $field->visibility_field;
>+        my $vi_field_name;
>+        if ($vi_field) {
>+            $vi_field_name = $vi_field->name;
>+        }

  You could do this much more tersely as:

  my $visibility_field_name = $field->visibility_field ? $field->visibility_field->name : undef;

>+        if ((($field->type eq FIELD_TYPE_SINGLE_SELECT or
>+             $field->type eq FIELD_TYPE_MULTI_SELECT)

  That should be $field->is_select.

  Also, you may want to just separate out the logic for the product-specific fields into a separate block.


>+        push (@fieldlist, {

  Please call "filter", so that this implements include_ and exclude_. Some callers won't want the legal values.

>+            enter_bug             => $self->type('boolean' , $field->enter_bug),

  Should probably be something more descriptive, ideally something that starts with is_.

>+            visibility_field_name => $self->type('string'  , $vi_field_name),

  Maybe just call that visibility_field.

>+            visibility_value      => $self->type('string'  , $vi_value_val),

  This should be an arrayref, as it will be an array of values in the future, even though it's only one value now.

>+sub field_values {
>+    if ($field_name eq 'version') {
>+        $result_ref = $dbh->selectall_arrayref(
>+            "SELECT versions.value, products.name FROM versions
>+            LEFT JOIN products ON versions.product_id = products.id
>+            ORDER BY value", undef);

  I can see why you did this, but might it not be simpler to have a Bugzilla::Version->get_all that pre-populates the "product" field in an intelligent way?

>+    elsif ($field_name eq 'target_milestone') {
>+        $result_ref = $dbh->selectall_arrayref(
>+            "SELECT milestones.value, milestones.sortkey, products.name
>+            FROM milestones
>+            LEFT JOIN products ON milestones.product_id = products.id
>+            ORDER BY sortkey, value", undef);

  In any case, this code needs to be unified, as all that changes here is the names of the tables, but the full set of code is duplicated.

>+        foreach my $val (@$result_ref) {
>+            my ($value, $sortkey, $product_n) =@$val;
>+                push(@result, {
>+                    value        => $self->type('string', $value),

  We need to be consistent, so this needs to be "name" instead of "value".

>+                    sortkey      => $self->type('int', $sortkey),

  I'm not sure it's a good idea to expose sortkeys--I'd prefer the server to do sorting and clients to not have to worry about it...but I'm not sure that will always work.

>+                    product_name => $self->type('string', $product_n),

  I think you can just call that "product", to be consistent.

>+    elsif ($field_name eq 'component') {

  And this could be Bugzilla::Component->get_all with a pre-populated "product" field.

>+        foreach my $val (@$result_ref) {
>+            my ($value, $product_n) =@$val;
>+                push(@result, {
>+                    name         => $self->type('string', $value),
>+                    product_name => $self->type('string', $product_n),
>+                });

  In any case, the code that creates these arrays needs to be unified, not specified once for each per-product field.

>+    elsif ($field_name eq 'bug_status') {
>+        $result_ref = $dbh->selectall_arrayref(
>+            "SELECT id FROM bug_status WHERE isactive = 1", undef);

  This one you can totally use Bugzilla::Field::Choice for, you don't need custom code.

>+            my @can_change_to_map = map {{name => $self->type('string',$_->name),
>+                required_comment => $self->type('boolean', $_->comment_required_on_change_from($locstatus)),

  comment_is_required

>+        foreach my $val (@$result_ref) {
>+            my ($value, $sortkey, $visibility_value_id) =@$val;

  Nit: Spacing around the =. (This happened in a few other places too, I think, that I didn't note.)

>+            my $visibility_value;
>+            if ($visibility_value_id) {
>+                my $field_name1;
>+                my $field_value_name;
>+                if ($va_field->name eq 'product') {
>+                    $field_name1  = 'products';
>+                    $field_value_name = 'name';
>+                }
>+                else {
>+                    $field_name1  = $va_field->name;
>+                    $field_value_name = 'value';
>+                }
>+                    $visibility_value = $dbh->selectrow_array(
>+                        "SELECT $field_value_name FROM $field_name1
>+                        WHERE id = ?", undef, $visibility_value_id);
>+            }

  This is all handled much more cleanly by using Bugzilla::Field::Choice.

>+=item B<Description>
>+
>+return all fields that are defined for a bug.

  "Get information about valid bug fields."

>+=item B<Params>
>+
>+=over
>+
>+=item none
>+
>+=back

  That can just be B<Params> (none)

  *However*, it should take an arrayref of field names that you want to get information about, and limit the returned list to that set of fields. The argument should be called "names". It can also take an "ids" argument, I suppose, though we rarely expose field ids. The return values should probably then also contain the field ids, if we accept "ids" as input.

>+=item C<1> Free Text
>+
>+=item C<2> Drop Down
>+
>+=item C<3> Multiple-Selection Box
>+
>+=item C<4> Large Text Box
>+
>+=item C<5> Date/Time

  There's also the BUG_URLS type.

  And you left out the 0 type.

>+=item C<custom>
>+
>+C<boolean>  true when we have an custom field.

  True when this is a custom field, false otherwise.

>+=item C<name>
>+
>+C<string> The name of this field.

  Mmm, be more descriptive.

>+=item C<description>
>+
>+C<string> The description of this field.

  Be more descriptive.



  Mmm, as far as the POD goes, I'll just edit it myself on checkin. However, you put the POD in the wrong place--look at the rendered POD if you need hints for what section it should go in (and within that section, it should be in alphabetical order).

Comment 9

[Frank Becker]

Attachment: patch, V6

Hope that I have include all requests from comment#8

Comment 10

[Gervase Markham [:gerv]]

FWIW, here is what the BzAPI currently exposes about fields:
https://wiki.mozilla.org/Bugzilla:REST_API:Objects:Configuration#Field

name            String (key of the hash)
description     String
type            Integer, Enumeration
is_on_bug_entry Boolean
values          Array of String
default         String

It would be good to match up the names by which the two APIs know each item of data, and the actual data presented.

Comparing mine to yours, I seem to have included the default value where you have not, and you seem to have included a boolean saying whether it's a custom field, and data about when the field is visible or invisible, and I have not.

Why do we need a separate boolean for whether the field is a custom field? a) Why does the user care, and b) surely the cf_ prefix identifies them?

Gerv

Comment 11

[Max Kanat-Alexander]

(In reply to comment #10)
> name            String (key of the hash)
> description     String
> type            Integer, Enumeration
> is_on_bug_entry Boolean
> values          Array of String

  These all look good.

> default         String

  That won't work (think about multi-selects)--instead you want to have an is_default item on the values.

Comment 12

[Max Kanat-Alexander]

Comment on attachment 413945 [details] [diff] [review]
patch, V6

>+sub fields {
>+    if (defined $params->{ids}) {
>+        my @ids = @{$params->{ids}};
>+        my $fields_by_id = Bugzilla::Field->new_from_list(\@ids);

  There's no need to create a my @ids there just to deference again in the constructor.

  Also, if an invalid id is passed, you should throw an error. (Bugzilla::Field->check({ id => $id }) is the easiest way to do that, though there are also more efficient ways.)

>+        @fields = @$fields_by_id;
>+    }
>+    elsif (defined $params->{names}) {

  This is not an elsif situation, both names and ids can be specified.

>+    foreach my $field (@fields) {
>+        my @value_field_array = $value_field             ? ($self->type('string'  , $value_field)) : undef;

  I don't understand what that line is doing. It's an array, but you're putting a single value into it? You could just do [$value_field] down below, if you wanted. However, I don't plan for value_field to ever be an array, so there's no reason for that to be an array.

>+        if (($field->is_select and $field->name ne 'product')
>+           or $field->name eq 'version'

  Nit: That "or" should be aligned with the first inner paren.

>+           or $field->name eq 'target_milestone'
>+           or $field->name eq 'component'

  Why are you only returning legal values for the product and product-specific fields? And why are you checking if product is a select field? It's always a select field.

>+             my $leagal_values  = $self->get_leagal_values({field => $field});
>+             $values = $leagal_values->{values};

  It's "legal", not "leagal". Also, WebService function names should not start with "get". For example, this one would just be "legal_values".

>+           name                  => $self->type('string'  , $field->name),
>+           description           => $self->type('string'  , $field->description),

  I think I want to call that display_name instead of "description".

  "Description" means something more like "here's a block of text that describes this field", which is something that we might actually *have* in Bugzilla for custom fields at some point.

>+           visibility_value      => $self->type('string'  , $visibility_value),

  This, however, does need to be an array.

>+           is_shown_on_create    => $self->type('boolean' , $field->enter_bug),

  I think we can call this is_on_bug_entry to be consistent with Gerv's BzAPI, though I'm not really happy with either name (they both seem too long and not quite descriptive enough).

>+sub get_leagal_values {

  This needs to accept a "product" argument, like the current "legal_values" subroutine does. Also, it needs to accept multiple field names or ids.

>+    if ($field_name eq 'version' or $field_name eq 'component') {
>+        my @list;
>+        if ($field_name eq 'version') {
>+            @list = Bugzilla::Version->get_all;
>+        }
>+        else {
>+            @list = Bugzilla::Component->get_all;
>+        }
>+        foreach my $value (@list) {
>+            push(@result, {
>+                value        => $self->type('string', $value->name),
>+                product_name => $self->type('string', $value->product->name),

  Let's just call that "product", which is what we call it elsewhere.

  Also, you're creating a security issue here--only products accessible to the current user can have their values returned. The solution is to use Bugzilla->user->get_accessible_products.

>+            });
>+        }
>+    }
>+    elsif ($field_name eq 'target_milestone') {

  There's no need to have these as separate blocks. Just always add a sortkey, but make it 0 or undef for Component and Version, for now. There's always the chance that we'll 

>+    elsif ($field_name eq 'bug_status') {



>+        my @st = Bugzilla::Status->get_all;
>+        foreach my $value (@st) {
>+            my @can_change_to = map {{
>+               name => $self->type('string',$_->name),

  Why call it "name" here and "value" elsewhere? Let's call it "name" everywhere (everywhere in this whole patch), since that's our Bugzilla standard internally.

>+               required_comment => $self->type('boolean', $_->comment_required_on_change_from($value)),

  I think that should be comment_required.

>+               sortkey       => $self->type('int'   , $value->sortkey),

  Do you think we should call this "sortkey" or sort_key?

>+        foreach my $value (@values) {
>+            my $visibility_value_field = $value->visibility_value;

  That's an inaccurate name for that variable, because visibility_value is a Field::Choice object, not a field.

>+            my $visibility_value       =  $visibility_value_field ? $visibility_value_field->name : undef;


>+                visibility_value => $self->type('string', $visibility_value),

  That should be an array.


  If you intend get_legal_values to be exposed as an API method, then it needs to have POD. If you don't intend to expose it as an API method, then its name needs to start with an underscore.

Comment 13

[Frank Becker]

Attachment: patch, V7

(In reply to comment #12)
> (From update of attachment 413945 [details] [diff] [review])
Hope that I fixed everything
> >+sub get_leagal_values {
> 
>   This needs to accept a "product" argument, like the current "legal_values"
> subroutine does. Also, it needs to accept multiple field names or ids.
This is now an internal function and I did not add support for multiple field names or ids.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 416161 [details] [diff] [review]
patch, V7

  This looks awesome now. :-) There's a few things that need to be fixed, though:

>+    if (defined $params->{ids}) {
>+        my @ids = @{$params->{ids}};
>+        foreach my $id (@ids) {
>+            my $loop_field = Bugzilla::Field->check({id => $id});
>+            push(@fields, $loop_field);
>+        }
>+    }
>+    elsif (defined $params->{names}) {

  This is not an elsif. Both names and ids should be able to be specified in one call.

>+        if (($field->is_select and $field->name ne 'product')
>+            or $field->name eq 'version'
>+            or $field->name eq 'target_milestone'
>+            or $field->name eq 'component'
>+           ) {

  Oh, the standard formatting for that would be more like:

  if (...
      or $field->name eq 'component')
  {

  That's actually from the "perlstyle" man page--it's a Perl-wide standard.

>+    my @result;
>+    if ($field_name eq 'version'
>+       or $field_name eq 'component'
>+       or $field_name eq 'target_milestone') {

  That one should be reformatted too. (And the "or"s should be forward one space.)

>+        my @list;
>+        if ($field_name eq 'version') {
>+            @list = Bugzilla::Version->get_all;
>+        }
>+        elsif($field_name eq 'component') {

  Nit: Space after the elsif.

>+        foreach my $value (@list) {
>+            my $sortkey = $field_name eq 'target_milestone' ? $value->sortkey : undef;
>+            if ($user->can_see_product($value->product->name)) {
>+                push(@result, {
>+                    name    => $self->type('string', $value->name),
>+                    sortkey => $self->type('int'   , $sortkey),
>+                    product => $self->type('string', $value->product->name),
>+                });

  Hmmmm. Actually, instead of doing this in a custom way, how about just use visibility_field on the fields, and then and visibility_value on the values? :-) That way all the fields are consistent (and we actually hope to make the architecture work that way internally eventually, too).

>+               required_comment => $self->type('boolean', $_->comment_required_on_change_from($status)),

  That should be comment_required.

>+        my @values = Bugzilla::Field::Choice->type($field)->get_all();
>+        foreach my $value (@values) {

>+                visibility_value => \@visibility_values,

  Hmmm. Maybe we should call it visibility_values? (We'd have to make the same change in fields()

>+C<boolean>  true when we have an custom field.

  "true when this is a custom field"

>+=item C<name>
>+
>+C<string> The internal name of this field.

  Add this sentence after that: This is a unique identifier for this field.

  (Or something like that.)

>+C<string> The description of this field. This is shown in the interface.

  s/interface/user interface/

>+C<string>  field only appears when the field with this name is equal value of C<visibility_value>.

  The name of a field that controls the visibility of this field in the user interface. This field only appears in the user interface when the named field is equal to one of the values in C<visibility_values>.

>+Only inclueded if it is not NULL.

  Nit: typo on included.

>+=item C<visibility_value>
>+
>+array of C<string>  the value that the C<visibility_field_name> must have to show this field.

  "C<array> of C<string>s. This field is only shown when C<visibility_field> matches one of these values."


>+Only inclueded if it is not NULL.

  Nit: included

>+C<string>  field that controls the values that appear in this field.

  The name of the field that controls whether or not 

>+Only inclueded if it is not NULL.

  Nit: included

>+=item C<values>
>+
>+This is an hash containing a single element, C<values>. This is an array of hashes, 
>+containing the following keys:

  Shouldn't it just be an array, not a hash?

>+=over 
>+
>+=item C<value>

  That's "name" now.

>+=item C<sortkey>
>+
>+C<int> that is used for sort the values.

  You'll have to note that some fields don't have one of these.

>+=item C<visibility_value>
>+
>+If this field has contol of the visibility.

  Needs a similar description to the one above for visibility_value for fields.

>+Only inclueded if it is not NULL.

  Nit: included.

>+=item C<product_name>

  That will just become visibility_value, with my suggestion.

>+=item C<is_open>
>+
>+Only included if the field is bug_status.

  Okay. But what does it mean? What type is it? :-)

>+=item B<Errors>
>+
>+Currently, this function doesn't throw any special errors
>+(other than the ones that all webservice functions can throw).

  Oh, actually, check() can throw several errors.

Comment 15

[Frank Becker]

(In reply to comment #14)
> (From update of attachment 416161 [details] [diff] [review])
>   This looks awesome now. :-) There's a few things that need to be fixed,
> though:
> 
> >+        foreach my $value (@list) {
> >+            my $sortkey = $field_name eq 'target_milestone' ? $value->sortkey : undef;
> >+            if ($user->can_see_product($value->product->name)) {
> >+                push(@result, {
> >+                    name    => $self->type('string', $value->name),
> >+                    sortkey => $self->type('int'   , $sortkey),
> >+                    product => $self->type('string', $value->product->name),
> >+                });
> 
>   Hmmmm. Actually, instead of doing this in a custom way, how about just use
> visibility_field on the fields, and then and visibility_value on the values?
> :-) That way all the fields are consistent (and we actually hope to make the
> architecture work that way internally eventually, too).

Sorry I did not see what you mean. Here I only use some attributes from Field.
Product is only for the fields 'version', 'target_milestone' and 'component' which are only valid for a special product.
> 
> 
> >+=item C<product_name>
> 
>   That will just become visibility_value, with my suggestion.
> 
> >+=item B<Errors>
> >+
> >+Currently, this function doesn't throw any special errors
> >+(other than the ones that all webservice functions can throw).
> 
>   Oh, actually, check() can throw several errors.

Should I have do define a new constant in Webservice/Constants.pm and throw an special error?
Or
Should I only list the possible errors (without an error number) ?

Hope that all other thinks are changed in an way that is OK for you.

Patch is coming soon.

Comment 16

[Frank Becker]

Attachment: patch, V8

see comment#15

Comment 17

[Max Kanat-Alexander]

(In reply to comment #15)
> >   Hmmmm. Actually, instead of doing this in a custom way, how about just use
> > visibility_field on the fields, and then and visibility_value on the values?
> > :-) That way all the fields are consistent (and we actually hope to make the
> > architecture work that way internally eventually, too).
> 
> Sorry I did not see what you mean. Here I only use some attributes from Field.
> Product is only for the fields 'version', 'target_milestone' and 'component'
> which are only valid for a special product.

  Here's what I mean:

  For the target_milestone field, you specify visibility_field => 'product'.
  Then, for each target_milestone value, you specify visibility_value as the name of the product that the target_milestone is visible in.


> Should I have do define a new constant in Webservice/Constants.pm and throw an
> special error?

  That's not necessary, they're already all defined. If there are any errors that check() can throw that are not defined, please file a bug separate from this one.

> Or
> Should I only list the possible errors (without an error number) ?

  You should only have to list the possible errors.

Comment 18

[Max Kanat-Alexander]

(In reply to comment #17)
>   For the target_milestone field, you specify visibility_field => 'product'.

  Oops, I mean, you specify "value_field" as "product".

  Do you think that you can get me an updated patch with these things also changed?

Comment 19

[Frank Becker]

(In reply to comment #18)
> (In reply to comment #17)
> >   For the target_milestone field, you specify visibility_field => 'product'.
> 
>   Oops, I mean, you specify "value_field" as "product".
> 
>   Do you think that you can get me an updated patch with these things also
> changed?

Yes, but I need until tomorrow night (German timezone).

Comment 20

[Frank Becker]

Attachment: patch, V9

updated patch V8

as requested in comment# 18

Comment 21

[Max Kanat-Alexander]

Comment on attachment 417989 [details] [diff] [review]
patch, V9

This looks basically good, though I do have to fix a few things on checkin. Mostly that @array = [something] is wrong--you should be doing @array = (something).

Comment 22

[Max Kanat-Alexander]

Thank you for all your work, Frank! :-)

Checking in Bugzilla/WebService/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/WebService/Bug.pm,v  <--  Bug.pm
new revision: 1.52; previous revision: 1.51
done

Comment 23

[Max Kanat-Alexander]

Attachment: Checked-In Version

Comment 24

[Max Kanat-Alexander]

Added to the release notes in bug 547466.