Closed Bug 344875 Opened 18 years ago Closed 18 years ago

Implement a UI to manage custom fields and remove customfield.pl

Categories

(Bugzilla :: Administration, task)

2.23
task
Not set
normal

Tracking

()

RESOLVED FIXED
Bugzilla 3.0

People

(Reporter: LpSolit, Assigned: LpSolit)

References

Details

Attachments

(2 files, 1 obsolete file)

Much easier to manage custom fields using a nice UI than typing command lines, even if these commands are actually pretty trivial. ;)

I think the UI should also allow us to edit and remove custom fields.
This actually isn't necessary for 3.0, though. We don't really want to make it easy for people to add new fields.
(In reply to comment #1)
> This actually isn't necessary for 3.0, though. We don't really want to make it
> easy for people to add new fields.

huh? :) This should be. This will be one of most requested feature. Could you explain why you wouldn't want to make it easy?
(In reply to comment #2)
> huh? :) This should be. This will be one of most requested feature. Could you
> explain why you wouldn't want to make it easy?

  Because users new to Bugzilla will start off adding many, many custom fields that they do not need, and then later discover they made a mistake. The end-users will blame Bugzilla itself, and request a new system that's "less complicated."

  Usually when I'm working with Bugzilla inside a company, they start off requesting many fields that they do not need, and then eventually quite happily realize they can live better without them.
If you put something like my first paragraph above on the page itself, though, that would help.
Attached patch patch, v1 (obsolete) — Splinter Review
This patch allows you to create new custom fields using the UI (editcustomfields.cgi). Other actions are not yet implemented. I plan to do it in a separate patch.

I don't want to add a link to this script yet. We can add one later.
Attachment #232387 - Flags: review?(mkanat)
Comment on attachment 232387 [details] [diff] [review]
patch, v1

>@@ -217,6 +219,31 @@
> 
> =over
> 
>+=item C<mailhead>

  If we're going to call it in_new_bugmail for create_or_update, we might as well call it in_new_bugmail here.

>@@ -286,16 +323,15 @@
>                  undef, $params->{desc}, $in_new_bugmail, $field->id);
>     }
>     else {

  Seems like $is_obsolete and $enter_bug also ought to appear in the update code. But since the UI currently only allows you to create new fields, I suppose that's fine.

>Index: editcustomfields.cgi

  I'd rather this were just called editfields.cgi. Eventually it may control the sort order and layout of all fields, not just custom fields.

>+my $action = trim($cgi->param('action') || 'list');
>+
>+print $cgi->header();
>+
>+# List all existing custom fields.
>+if ($action eq 'list') {

  I'd rather just not have action defined. This is like having action=view for attachment.cgi.

>+    $vars->{'custom_fields'} = Bugzilla::Field->match({'custom' => 1});

  This is not the correct interface--you should be using Bugzilla->get_fields.

>+elsif ($action eq 'edit') {
>+    my $name = $cgi->param('name') || ThrowUserError('customfield_missing_name');

  You should trim the name, or (since you do it on add) clean_text it. Otherwise this will accept spaces for the name.

  Also, I don't think we really ought to let people change the names of custom fields. I think that would just sort of cause havoc. We should just let people delete old ones and make new ones.

>+    $vars->{'field'} = @$field[0];

  I think you mean $field->[0]. The way you're doing it now is pretty ambiguous.

>+  Adding a new custom field is generally not what you want as it may overload
>+  the interface. Better is to optimize your workflow with existing fields first.
>+  Now if you really want to add a new field, that's your decision.

  That's pretty good. How about this instead, though:

  Adding custom fields can make the interface of Bugzilla very complicated. Many admins who are new to Bugzilla start off adding many custom fields, and then their users complain that the interface is "too complex." Please think carefully before adding any custom fields. It may be the case that Bugzilla already does what you need, and you just haven't enabled the correct feature yet.

>+  <ul>
>+    <li>Custom field names must begin with "cf_" to distinguish them from standard
>+        fields. If you omit it, it will automatically be appended.</li>

  "If you omit "cf_" from the name, it will..."

>+    <li>Descriptions are a short string describing the field and will be used as
>+        the label for this field.</li>

  Say where the label will be used. Just like, "in the User Interface" or something like that.


>+      <td><input type="text" id="name" name="name" value="cf_" size="40" maxlength="64"></td>

  That line is longer than 80 characters.

>+      <th align="right">
>+        <label for="new_bugmail">Displayed in bugmail for new [% terms.bugs %]:</label>
>+      </th>
>+      <td><input type="checkbox" id="new_bugmail" name="new_bugmail" value="1"></td>

  Instead of having this be user-controlled, I think if a field is an enter_bug field, it should always show up in new bugmail. It's simpler that way.

>+        <label for="enter_bug">Editable on [% terms.bug %] creation:</label>

  Not "Editable," but "Can be set"

>Index: template/en/default/admin/custom_fields/edit.html.tmpl

>+[% PROCESS "global/field-descs.none.tmpl" %]

>+    <tr>
>+      <th align="right"><label for="type">Type:</label></th>
>+      <td>Free Text</td>
>+
>+      <th align="right"><label for="obsolete">Is obsolete:</label></th>
>+      <td><input type="checkbox" id="obsolete" name="obsolete" value="1"
>+                 [%- " checked" IF field.obsolete %]></td>
>+    </tr>

  I don't think people should be able to change the type of a field, either. That would get pretty complex later on. At the least, if we're going to let people do that, we should do it in a separate bug.

>Index: template/en/default/global/user-error.html.tmpl

>+  [% ELSIF error == "customfield_inexistent" %]
>+    [% title = "Unknown Custom Field" %]
>+    There is not custom field with the name '[% name FILTER html %]'.

  It's "nonexistent".

  Also, it should be "There is no" instead of "There is not"

  Shouldn't there already be messages about fields that we can just re-use?

>+  [% ELSIF error == "customfield_invalid_sortkey" %]
>+    [% title = "Invalid Sortkey for Field" %]
>+    The sortkey [% sortkey FILTER html %] for the '[% name FILTER html %]'
>+    field is not a valid positive integer.

  Might want to say "The sortkey [% blah %] that you have provided"--that's a bit clearer.

>+  [% ELSIF error == "customfield_missing_name" %]
>+    [% title = "Missing Name for Field" %]
>+    You must enter a name for this field.
>+
>   [% ELSIF error == "product_doesnt_exist" %]

  Is that really alphabetical order? C and then P?
Attachment #232387 - Flags: review?(mkanat) → review-
(In reply to comment #6)
> >+    $vars->{'custom_fields'} = Bugzilla::Field->match({'custom' => 1});
> 
>   This is not the correct interface--you should be using Bugzilla->get_fields.

Bah, this doesn't matter, really. This interface is only useful from templates. And ->match returns a ref, which is what I want, instead of an array.


> >+elsif ($action eq 'edit') {
> >+    my $name = $cgi->param('name') || ThrowUserError('customfield_missing_name');
> 
>   You should trim the name, or (since you do it on add) clean_text it.
> Otherwise this will accept spaces for the name.
> 
>   Also, I don't think we really ought to let people change the names of custom
> fields. I think that would just sort of cause havoc. We should just let people
> delete old ones and make new ones.

Hey, Max! The name of the field is not editable, but that's the unique ID of fields!! That's the reason I don't trim it or anything. If that's an invalid field name, it will be caught a few lines below. The UI doesn't make this field editable, it's only displayed so that the admin knows which field he is looking at.


> >+      <th align="right"><label for="type">Type:</label></th>
> >+      <td>Free Text</td>
> >+
>   I don't think people should be able to change the type of a field, either.
> That would get pretty complex later on. At the least, if we're going to let
> people do that, we should do it in a separate bug.

I never planned to! This data is only given as an information to the admin. As you can see, the field is not editable.


>   Shouldn't there already be messages about fields that we can just re-use?

I checked, an no. Unfortunately.


>   Is that really alphabetical order? C and then P?

Sure, P comes after C, not the opposite. :-D What I'm doing is right. This message about products has nothing to do here, but that's not my job to move it.


So from what I can see, there are only some minor nits to fix. All other comments are because you reviewed it too quickly. ;)
Status: NEW → ASSIGNED
Attached patch patch, v2Splinter Review
Addressed all valid comments, except the one about removing the "In new bugmail" checkbox. If having a field available in enter_bug.cgi means to get it in bugmail in all cases, then we have a useless column in the fielddefs table. I prefer to keep it for now; this doesn't hurt (and it's easier to remove it in a few weeks if needed than adding the logic in scripts).
Attachment #232387 - Attachment is obsolete: true
Attachment #234604 - Flags: review?(mkanat)
Comment on attachment 234604 [details] [diff] [review]
patch, v2

Yeah, that looks fine.

You're right. Let's get right of mailhead and just use enter_bug. (In another bug.) Have you already filed it?
Attachment #234604 - Flags: review?(mkanat) → review+
Flags: approval?
Flags: approval? → approval+
Removing customfield.pl;
/cvsroot/mozilla/webtools/bugzilla/customfield.pl,v  <--  customfield.pl
new revision: delete; previous revision: 1.7
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/editfields.cgi,v
done
Checking in editfields.cgi;
/cvsroot/mozilla/webtools/bugzilla/editfields.cgi,v  <--  editfields.cgi
initial revision: 1.1
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.19; previous revision: 1.18
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/custom_fields/create.html.tmpl,v
done
Checking in template/en/default/admin/custom_fields/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/custom_fields/create.html.tmpl,v  <--  create.html.tmpl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/custom_fields/edit.html.tmpl,v
done
Checking in template/en/default/admin/custom_fields/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/custom_fields/edit.html.tmpl,v  <--  edit.html.tmpl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/custom_fields/list.html.tmpl,v
done
Checking in template/en/default/admin/custom_fields/list.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/custom_fields/list.html.tmpl,v  <--  list.html.tmpl
initial revision: 1.1
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.183; previous revision: 1.182
done
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: relnote
Resolution: --- → FIXED
Summary: Implement a UI for customfield.pl → Implement a UI to manage custom fields and remove customfield.pl
Blocks: 345107
Flags: documentation?
Attachment #246493 - Flags: review?(documentation)
Comment on attachment 246493 [details] [diff] [review]
documentation patch, v1

>+      <quote>Custom Fields</quote> link in the footer of pages. The first thing
>+      they will see is the list of existing custom fields (which is empty by
>+      default, of course).

Not sure if "of course" is logical here, but I suppose it is if you don't know the history.  We had originally talked about moving some of the current "strange" fields into custom fields if they could be represented that way.  So I suppose this is fine as is, I just had to laugh at it when I read it. :)

>+              Name: the name of the field, used internally. This name MUST
>+              Description: the string which is used as a label for this custom
>+              Type: as mentioned above, only two types are implemented so far.
>+              Sortkey: this integer determines in which order custom fields
>+              Can be set on bug creation: this boolean determines whether this
>+              Displayed in bugmail for new bugs: this boolean determines
>+              Is obsolete: this boolean determines whether or not this field

It's hard to tell the name of the field apart from the text describing it.  Should there be some sort of emphasis on the field names?  What do we do in other places in the docs where we describe this stuff?  This is a nit, and more important to get this into the docs than to have it looking perfect. :)  If you come up with something that looks nice, fix it on commit.
Attachment #246493 - Flags: review?(documentation) → review+
Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <--  administration.xml
new revision: 1.68; previous revision: 1.67
done
Flags: documentation? → documentation+
Added to the release notes on bug 255155.
Keywords: relnote
The correct bug number for those release notes is actually bug 349423.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: