460742 - The type should be mandatory to create a custom field

Status: RESOLVED FIXED

(Bugzilla :: Administration, task)

Description

[Frédéric Buclin]

Currently, you can create custom fields having no defined type, and they are inserted into the DB as FIELD_TYPE_UNKNOWN, which is pretty useless. IMO, this type doesn't make sense and we should require the field type to be always defined on creation.

What was the logic to implement this type in the original implementation of custom fields (bug 287325)?

Comment 1

[Max Kanat-Alexander]

It's the type that standard fields have, if I'm not mistaken. Standard fields are also created with create().

Comment 2

[Scott Saad]

Was looking into this and have not been able to reproduce with latest 3.3.3+. The create template for custom_fields appears to filter FIELD_TYPE_UNKNOWN out.

Comment 3

[Max Kanat-Alexander]

Yeah, you can't do it from the UI, but you can do it when calling create() directly.

Comment 4

[Nitish Bezzala]

Attachment: added validation for FIELD_TYPE_UNKOWN

Comment 5

[Frédéric Buclin]

Comment on attachment 379833 [details] [diff] [review]
added validation for FIELD_TYPE_UNKOWN

>-    (detaint_natural($type) && $type <= FIELD_TYPE_BUG_URLS)
>+    (detaint_natural($type) && $type <= FIELD_TYPE_BUG_URLS
>+                            && $type != FIELD_TYPE_UNKNOWN)


'type' must be listed in REQUIRED_CREATE_FIELDS, else this field is left optional. Also, this change in _check_type() is incorrect. What I was asking for is that all fields have an explicit type. Hardcoded fields specified in DEFAULT_FIELDS have no explicit type and so populate_field_definitions() should set it before calling Bugzilla::Field->create(). This way, we are sure that the type of all fields (hardcoded and custom ones) is as expected.

Comment 6

[Frédéric Buclin]

To make my previous comment clearer, as Bugzilla::Field->create() is also used by hardcoded fields, we cannot ban FIELD_TYPE_UNKNOWN in _check_type(). But forcing the caller to set the 'type' attribute will prevent to forget to set it and to have custom fields with an unwanted FIELD_TYPE_UNKNOWN type. If the caller really wants to set the type as FIELD_TYPE_UNKNOWN, we shouldn't prevent it, I think.

Comment 7

[Nitish Bezzala]

Attachment: made type a required field

Don't know how to test this as the UI doesn't allow me to leave the type empty.

Comment 8

[Max Kanat-Alexander]

Type should only be mandatory to create a *custom* field. It should not be in REQUIRED_CREATE_FIELDS.

Comment 9

[Max Kanat-Alexander]

You can do this without putting the burden on the caller, by modifying run_create_validators.

Comment 10

[Nitish Bezzala]

Attachment: modified run_create_validators

Using ThrowCodeError because the user can't not select a type.

Comment 11

[Max Kanat-Alexander]

Comment on attachment 380358 [details] [diff] [review]
modified run_create_validators

>Index: Bugzilla/Field.pm
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v
>retrieving revision 1.43
>diff -u -r1.43 Field.pm
>--- Bugzilla/Field.pm	8 Feb 2009 19:42:21 -0000	1.43
>+++ Bugzilla/Field.pm	29 May 2009 04:26:40 -0000
>@@ -836,6 +836,9 @@
>                                      $params->{visibility_field_id});
> 
>     my $type = $params->{type} || 0;
>+    ($params->{custom} && $type)
>+      || ThrowCodeError('invalid_customfield_type', { type => $type });

  I think you mean !$type, right? also, invalid_customfield_type will just be confusing in this case, because it doesn't say that you missed a type, it just says that you picked an invalid one.

Comment 12

[Nitish Bezzala]

Attachment: added type not available message

Comment 13

[Frédéric Buclin]

Comment on attachment 386347 [details] [diff] [review]
added type not available message

You cannot r+ your own patches. Asking mkanat for review.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 386347 [details] [diff] [review]
added type not available message

>Index: Bugzilla/Field.pm
>+# If it is a custom field, type should not be empty. 
>+# Not sure why we have to say && $type instead of && !$type, 
>+# but it works this way.
>+    ($params->{custom} && $type)
>+        || ThrowCodeError('customfield_type_not_available');

  Okay, what? You didn't indent the comment? And you didn't fix the logic here? Do you even read my review comments?

>Index: template/en/default/global/code-error.html.tmpl
>+  [% ELSIF error == "customfield_type_not_available" %]
>+    [% title = "Field Type not available" %]
>+    The type is mandatory to create a custom field.

  "not available" isn't quite gramatically sensible. Also, the error should be something more like, "You must specify a type when creating a custom field."

  Also, did you not notice the comment in this file that says that all items are in alphabetical order and should be kept that way? Also, this should be a "field_" error, because those already exist (did you even check?) and not customfield_.

Comment 15

[Max Kanat-Alexander]

Oh wait, nevermind my comment about $type. Your logic is right, but it's confusing. Instead, make it:

if ($params->{custom} && !$type) {
    throw error
}

Comment 16

[Nitish Bezzala]

Attachment: Changed to "Field type unaivalable"

Comment 17

[Max Kanat-Alexander]

Comment on attachment 408581 [details] [diff] [review]
Changed to "Field type unaivalable"

This looks fine, but the word "unavailable" should be changed to "not specified" on checkin.

Comment 18

[Frédéric Buclin]

I did the fixes on checkin.

Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.46; previous revision: 1.45
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.120; previous revision: 1.119
done