The type should be mandatory to create a custom field

RESOLVED FIXED in Bugzilla 3.6

Status

()

--
minor
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: LpSolit, Assigned: nbezzala)

Tracking

Bugzilla 3.6
Bug Flags:
approval +

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

10 years ago
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)?
It's the type that standard fields have, if I'm not mistaken. Standard fields are also created with create().
(Reporter)

Updated

10 years ago
Whiteboard: [Good Intro Bug]

Comment 2

10 years ago
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.
Yeah, you can't do it from the UI, but you can do it when calling create() directly.
(Assignee)

Comment 4

10 years ago
Created attachment 379833 [details] [diff] [review]
added validation for FIELD_TYPE_UNKOWN
Attachment #379833 - Flags: review?(LpSolit)
(Reporter)

Comment 5

10 years ago
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.
Attachment #379833 - Flags: review?(LpSolit) → review-
(Reporter)

Comment 6

10 years ago
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.
Target Milestone: --- → Bugzilla 3.6
(Assignee)

Comment 7

10 years ago
Created attachment 379908 [details] [diff] [review]
made type a required field

Don't know how to test this as the UI doesn't allow me to leave the type empty.
Attachment #379833 - Attachment is obsolete: true
Attachment #379908 - Flags: review?(LpSolit)
Type should only be mandatory to create a *custom* field. It should not be in REQUIRED_CREATE_FIELDS.
You can do this without putting the burden on the caller, by modifying run_create_validators.
(Assignee)

Comment 10

10 years ago
Created attachment 380358 [details] [diff] [review]
modified run_create_validators

Using ThrowCodeError because the user can't not select a type.
Attachment #379908 - Attachment is obsolete: true
Attachment #380358 - Flags: review?(mkanat)
Attachment #379908 - Flags: review?(LpSolit)

Updated

10 years ago
Attachment #380358 - Flags: review?(mkanat) → review-
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.
(Assignee)

Comment 12

10 years ago
Created attachment 386347 [details] [diff] [review]
added type not available message
Attachment #380358 - Attachment is obsolete: true
Attachment #386347 - Flags: review+
(Reporter)

Comment 13

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

You cannot r+ your own patches. Asking mkanat for review.
Attachment #386347 - Flags: review+ → review?(mkanat)
(Reporter)

Updated

10 years ago
Assignee: administration → nbezzala
Status: NEW → ASSIGNED

Updated

10 years ago
Attachment #386347 - Flags: review?(mkanat) → review-
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_.
Oh wait, nevermind my comment about $type. Your logic is right, but it's confusing. Instead, make it:

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

Comment 16

9 years ago
Created attachment 408581 [details] [diff] [review]
Changed to "Field type unaivalable"
Attachment #386347 - Attachment is obsolete: true
Attachment #408581 - Flags: review?(mkanat)
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.
Attachment #408581 - Flags: review?(mkanat) → review+

Updated

9 years ago
Severity: normal → minor
Flags: approval+
Whiteboard: [Good Intro Bug] → requires checkin fix
(Reporter)

Comment 18

9 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Whiteboard: requires checkin fix
You need to log in before you can comment on or make changes to this bug.