349369 - Allow unused custom fields to be deleted from editfields.cgi

Status: RESOLVED FIXED

(Bugzilla :: Administration, task, P4)

Description

[Frédéric Buclin]

A low priority bug. But probably something that an admin may wish at some point. You know, the one who creates 10 custom fields because he is sure he needs them all, and finally realizes that 8 of them are useless. There should be a clear warning on the confirmation page that he should rather mark the field as obsolete instead of deleting it.

Comment 1

[Max Kanat-Alexander]

I think this is a pretty low priority. I'd just say that obsolete fields shouldn't show up if they're not set. (Which we don't do currently--currently, obsolete fields just disappear entirely from the UI.)

Comment 2

[Alex Eiser]

Attachment: Patch for deleting fields

Here is a sample patch which provides this functionality. It will only allow you to delete fields if their is no entries in bugs_activity and all the content is blank. It also requires the field to be obsolete.

Comment 3

[Max Kanat-Alexander]

Comment on attachment 296094 [details] [diff] [review]
Patch for deleting fields

Put the delete button in its own form that submits to a different action.

Also, it should be $field->remove_from_db, which is what we call the other similar methods.

The changes to Bugzilla::Field seem to be missing from this patch.

Comment 4

[Alex Eiser]

Attachment: Updated Delete Filed patch

Added new patch. Forgot to include the actual field delete code.

Comment 5

[Max Kanat-Alexander]

Comment on attachment 296098 [details] [diff] [review]
Updated Delete Filed patch

>Index: Bugzilla/Field.pm
>+sub remove_from_db {
>+    my $OKtoDelete = 1;

  No CamelCaps in Bugzilla. (See "man perlstyle")

>+    if (! $self->custom) {

  Nit: No need for a space after the "!"

>+    # Check to see if bug activity table has records (should be fast with index)
>+    $dbh->bz_lock_tables('bugs_activity READ') ;

  We don't use table locks anymore, we use transactions.

>+    if ($self->type == FIELD_TYPE_MULTI_SELECT) {
>+        $dbh->bz_lock_tables("bug_$name READ") ;
>+        $bugs_query = "SELECT COUNT(bug_id) FROM bug_$name WHERE value IS NOT NULL 

  You can just do COUNT(*). It's equally fast or faster.

>+    my $get_bugs_count = $dbh->prepare($bugs_query);
>+    $get_bugs_count->execute();
>+    $v = $get_bugs_count->fetchrow_array || 0;

  Why are you using variable names like $v? Use some better name.

>+    ## Once we reach here, we should be OK to delete. Check the flag to be sure.
>+    if ($OKtoDelete) {

  If we're not OK to delete, we should have just thrown an error. There's no need to keep track of that var, that I can see.

  That's just a quick review, not a complete one.

Comment 6

[Alex Eiser]

Attachment: Updated Delete Field Patch v3

Address Max's comments.

Comment 7

[Frédéric Buclin]

Comment on attachment 296102 [details] [diff] [review]
Updated Delete Field Patch v3

>Index: editfields.cgi

>+    # Calling deleteField, will check if field can be delete.

What is deleteField?? You mean remove_from_db()? Also remove the comma and write "... can be deleted".


>+    $vars->{'message'} = 'custom_field_delete';

_deleted



>Index: Bugzilla/DB.pm

>+sub bz_drop_field_tables {

>+    if ($type == FIELD_TYPE_MULTI_SELECT) {
>+        $self->_bz_drop_table('bug_' . $field->name);

_bz_drop_table() doesn't exist.



>Index: Bugzilla/Field.pm

>+sub remove_from_db {
>+    if (! $self->custom) {

Nit: here and below, remove the whitespace between ! and $self.


>+        ThrowUserError('delete_field_not_custom',{'name' => $self->name });

We usually start the error tag with the area being covered, here it's an error about custom fields, so we should write "custom_field_invalid". This way, all error messages about custom fields are grouped together when sorted alphabetically in user-error.html.tmpl.


>+        ThrowUserError('delete_field_not_obsolete',{'name' => $self->name });

Same here: custom_field_not_obsolete.


>+    my $get_activity_count = $dbh->prepare("SELECT COUNT(*) FROM bugs_activity
>+                                      WHERE fieldid = ?");
>+    $get_activity_count->execute($self->id);
>+    my $activity_count_result = $get_activity_count->fetchrow_array || 0;

Use $dbh->selectrow_array(); it's much more readable. And remove || 0; that's useless.


>+    if ($activity_count_result > 0) {
>+        ThrowUserError('delete_field_has_activity',{'name' => $self->name });

Remove "> 0" so that we don't care if $activity_count_result is undefined. Also, the error message should be custom_field_has_activity.


>+        # Ignore blank dates. This is the MYSQL blank date value, I am not sure of
>+        # PSQL or ORACLE having a different values.

Nit: s/MYSQL/MySQL/, s/PSQL/PostgreSQL/, s/ORACLE/Oracle/.
And remove the last sentence about not being sure if it works or not. You should make this comment in a comment in the bug, not in the patch. We will know before committing this patch if it works or not.


>+    my $get_bugs_count = $dbh->prepare($bugs_query);
>+    $get_bugs_count->execute();
>+    my $bugs_count_result = $get_bugs_count->fetchrow_array || 0;

As above, use $dbh->selectrow_array() and remove || 0.


>+    if ($bugs_count_result > 0) {
>+        ThrowUserError('delete_field_has_contents',{'name' => $self->name });

Remove "> 0" and use custom_field_has_data or something like that.


>+    $dbh->do('delete from fielddefs where id = ?', undef, $self->id);

Use capital letters for DELETE FROM WHERE.


>+    if ($type == FIELD_TYPE_MULTI_SELECT) {
>+        $dbh->bz_drop_column("bug_$name", $name);
>+    }

This doesn't make sense to me. You want to drop the whole table, isn't it?



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

>+    Please contact [% Param("maintainer") %] before trying to delete this field.

What is the maintainer supposed to do? He cannot do more than you can. So please remove this sentence.

Comment 8

[Alex Eiser]

Attachment: Updated Patch V4

Responds to comments from LpSolit.

Fixes error with multselect fields not being dropped correctly.

Comment 9

[Frédéric Buclin]

Comment on attachment 296726 [details] [diff] [review]
Updated Patch V4

Is it intentional to have no confirmation page before the deletion?

Comment 10

[Alex Eiser]

It was more or less intentional. Since the delete will fail if their is any data.. I did not feel that a confirmation page was required.

Comment 11

[Alex Eiser]

I had envisioned that a future patch would display a confirmation page IFF the field had content.

Comment 12

[Frédéric Buclin]

Comment on attachment 296726 [details] [diff] [review]
Updated Patch V4

>Index: editfields.cgi

>+elsif ($action eq 'delete') {
>+    check_token_data($token, 'edit_field');

The fact that we use the same token to delete custom fields bothers me a bit. But as you come here from the same page, you haven't choice. Having a confirmation page first would fix the problem.


>+    # Calling remove_from_db, will check if field can be delete.

Nit: useless comma and delete -> deleted (unless I speak english very badly).


>+    # if the field can not be deleted, it will throw user error.

s/if/If/, and it will throw *an* error (we don't care code or user error).



>Index: Bugzilla/DB.pm

>+sub bz_drop_field_tables {

Nit: should come after bz_add_field_tables.


>+    my ($self, $field) = @_;
>+    my $type = $field->type;
>+    if ($type == FIELD_TYPE_MULTI_SELECT) {

Nit: no need to define $type. You can type: $field->type == FIELD_TYPE_... directly.



>Index: Bugzilla/Field.pm

>+sub remove_from_db {

>+    my $name = $self->name;
>+
>+    if (!$self->custom) {
>+        ThrowUserError('customfield_invalid',{'name' => $self->name });
>+    }

Either define $name and use it everywhere instead of $self->name, or remove it.
Also, it's impossible to come here AFAIK, even if you hack the URL as editfields.cgi will throw an error earlier. So if we come here anyway, this means that there is an error in the code itself, and is not triggered by the user himself. So this should be ThrowCodeError().


>+    if (!$self->obsolete) {
>+        ThrowUserError('customfield_not_obsolete',{'name' => $self->name });
>+    }

Here too, use $name instead of $self->name (or remove $name above).


>+    my $activity_count_result = $dbh->selectrow_array("SELECT COUNT(*) FROM bugs_activity
>+                                      WHERE fieldid = ?", undef, $self->id);

Nit: the variable name could be shorter: $has_activity.


>+    if ($self->type == FIELD_TYPE_MULTI_SELECT) {
>+        $bugs_query = "SELECT COUNT(*) FROM bug_$name WHERE value IS NOT NULL 
>+                                AND value != '' AND value != '---'";

Why looking for '' and '---' in multi-select fields. By default, bugs are not listed here. If they are, then a user selected a value and should be caught. So the whole WHERE block is useless.


>+    my $bugs_count_result = $dbh->selectrow_array($bugs_query);

Nit: maybe $has_bugs.


>+    if ($bugs_count_result) {
>+        ThrowUserError('delete_field_has_contents',{'name' => $self->name });

The error tag should begin with customfield_.


>+    ## Once we reach here, we should be OK to delete.
>+    $dbh->do('DELETE FROM fielddefs WHERE id = ?', undef, $self->id);

Nit: from a logical point of view, I would delete this entry after having deleted other tables. But I guess this doesn't matter.



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

>+  <form>
>+    <input type="hidden" name="action" value="delete">

<form> needs to know where to go. It has missing attributes. Maybe should it be a link pointing to the confirmation page, to address the token issue.



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

>+  [% ELSIF error == "customfield_invalid" %]
>+    [% title = "Deleting Custom Field" %]

The title doesn't match what is happening. Should be "Not a Custom Field". Also, it should be moved into code-error.html.tmpl for the reason given above.


>+  [% ELSIF error == "customfield_not_obsolete" %]
>+    [% title = "Deleting Custom Field" %]
>+    The custom field '[% name FILTER html %]' is not in the obsolete state. 
>+    Please obsolete a custom field before attempting to delete it.

Here too, the title is inaccurate. Should be "Custom Field Not Obsolete".

Fix other titles as well.

Comment 13

[Alex Eiser]

Attachment: Delete Fields Patch V5

Corrected comments by LpSoit.

 Left delete from fielddefs ahead of delete columns.
If the field is a single select.. and the delete from bugs finishes correctly, and the drop field value tables fails... if the field is in the fielddefs table still the system will be un usable. Deleting the value from the fielddefs table will leave the system in a useable state. IMO.

Comment 14

[Alex Eiser]

Attachment: Delete Fields Patch V6

Forgot to merge from head.

Comment 15

[Frédéric Buclin]

Comment on attachment 300532 [details] [diff] [review]
Delete Fields Patch V6

>Index: editfields.cgi

>+elsif ($action eq 'delete_req') {

For consistency with other admin pages, this action should be named 'del'.


>+    check_token_data($token, 'edit_field');

No token should be required to display this page.


>+    # Validate fields.

Nit: field (as you only check only one).



>Index: Bugzilla/Field.pm

>+sub remove_from_db {

>+    if (!$self->custom) {
>+        ThrowCodeError('illegal_field',{'name' => $name });
>+    }

Where is the corresponding error in code-error.html.tmpl? This file is not appended to the patch.

You no longer check that the field is obsolete. Bring back this check.


>+        ThrowUserError('customfield_has_activity',{'name' => $name });

Nit: add a whitespace before {.


>+        ThrowUserError('customfield_has_contents',{'name' => $name });

Nit: same here.



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

>-  # Contributor(s): Frédéric Buclin <[e-mail]>
>+  # Contributor(s): Fr�d�ric Buclin <[e-mail]>

Argh, please leave my first name alone. :) You should use a text editor which use the UTF-8 encoding.


>+  <form id="delete_field_req" action="editfields.cgi" method="GET">

Now thinking about it, we don't need a button for it. A link is enough, and we do it in all other admin pages.

Also, a column should be added to the list of custom fields allowing me to delete obsolete fields.


>+  Remove this custom field from the database. <br>
>+  This action will only be sucessfull if the custom field contains no data and has never been used in a [% terms.bug %].<br>

Rather than displaying this message, you should only display the link/button if the field is obsolete. "Click here but this won't work" is not very useful.



>+++ template/en/default/admin/custom_fields/delete.html.tmpl	2008-01-30 15:52:58.000000000 -0800

All other admin folders have a confirm-delete.html.tmpl file for this confirmation page. Let's be consistent.


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

Do not use field-descs.non.tmpl when we don't need to. It does extra work which we don't want here. Call variables.none.tmpl instead.


>+<form id="delete_field" action="editfields.cgi" method="GET">

Here too, I don't think a button is useful. You can have a link instead.

Comment 16

[Alex Eiser]

Attachment: Delete Fields Patch V7

Comments:

> >+sub remove_from_db {
> 
> >+    if (!$self->custom) {
> >+        ThrowCodeError('illegal_field',{'name' => $name });
> >+    }
> 
> Where is the corresponding error in code-error.html.tmpl? This file is not
> appended to the patch.

Illegal_field was an existing code error

> 
> >+<form id="delete_field" action="editfields.cgi" method="GET">
> 
> Here too, I don't think a button is useful. You can have a link instead.
> 

I switched it to a link... but since this is an action I personally think it should be a button.

Comment 17

[Frédéric Buclin]

Comment on attachment 301045 [details] [diff] [review]
Delete Fields Patch V7

>Index: editfields.cgi

>+    $template->process('admin/custom_fields/confirm-delete.html.tmpl', $vars)

Missing || ThrowTemplateError($template->error());



>Index: Bugzilla/Field.pm

>+    if (!$self->custom) {
>+        ThrowCodeError('illegal_field', {'name' => $name });
>+    }

illegal_field is not the correct error tag to use. This one is used when validating a field value, not when validating a field name.

Also, add POD for remove_from_db().



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

>+[% columns.push({

Simply append it to the column list. No need to push() it separately.



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

>+  [% ELSIF error == "customfield_has_activity" %]
>+    [% title = "Custom Field Has Activity" %]
>+    The custom field '[% name FILTER html %]' has recorded activity. 
>+
>+  [% ELSIF error == "customfield_has_contents" %]
>+    [% title = "Custom Field Has Contents" %]
>+    [% terms.Bugs %] have non empty values for the custom field '[% name FILTER html %]'.

For both messages, specify that the field cannot be deleted.



>+++ template/en/default/admin/custom_fields/confirm-delete.html.tmpl	2008-02-02 

>+[%# INTERFACE:
>+  # none

Wrong, you pass field and token, which need to be described here.


Your patch looks much better. I will test the updated one.

Comment 18

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set "blocking3.2" tp "?", and either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

Comment 19

[Frédéric Buclin]

The patch is mostly ready, I simply ask to fix a few things.

Comment 20

[Alex Eiser]

Attachment: Delete Fields V8

>+    if (!$self->custom) {
>+        ThrowCodeError('illegal_field', {'name' => $name });
>+    }

illegal_field is not the correct error tag to use. This one is used when
validating a field value, not when validating a field name.


I decided to use field_not_custom since the actual error is that the user did not pass in a custom field

Comment 21

[Frédéric Buclin]

Comment on attachment 301587 [details] [diff] [review]
Delete Fields V8

>Index: Bugzilla/Field.pm

>+Attempts to remove the passed in field from the databaes. 

database*


>+Deleting fields are only successfuly if the field is obsolete and

is* only successful*



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

>-[%# The contents of this file are subject to the Mozilla Public
>+[%# The contents of this file are subject to the Mozilla Public

No idea what changed here. I will remove this on checkin.


>+  This action will only be sucessfull if the custom field contains no data and has never been used in a [% terms.bug %].<br>

Line too long + successful*



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

>+  [% ELSIF error == "customfield_not_obsolete" %]
>+    [% title = "Deleting Custom Field" %]

The title should reflect what's going on: Custom Field Not Obsolete.


>+    The custom field '[% name FILTER html %]' is not in the obsolete state. 

I would rather write: "... is not obsolete."


>+  [% ELSIF error == "customfield_has_activity" %]
>+    [% title = "Custom Field Has Activity" %]
>+    Cannot delete custom field '[% name FILTER html %]' because it has recorded activity. 

I would rather write: "The custom field bla bla cannot be deleted because..."


>+  [% ELSIF error == "customfield_has_contents" %]
>+    [% title = "Custom Field Has Contents" %]
>+    Cannot delete custom field '[% name FILTER html %]' because at least one [% terms.Bugs %] 

Same here: "The custom field blabla cannot be deleted because..." + terms.bug*



>+++ template/en/default/admin/custom_fields/confirm-delete.html.tmpl	2008-02-05 

>+  Are you sure you want to remove this custom field from the database?<br>

Remind the user which field we are talking about.


>+  This action will only be sucessfull if the custom field is obsolete, 

successful*


>+  contains no data, and has never been used in a [% terms.bug FILTER html %].<br>

What means "contains no data"? Should be removed.


I will fix all these comments and attach the patch here. r=LpSolit

Comment 22

[Frédéric Buclin]

Attachment: checked in patch, v9

Here is the patch I'm checking in, addressing my comments above.

Comment 23

[Frédéric Buclin]

I found two additional wording errors which I fixed on checkin (one being "a [% terms.bug %]" which must be [% terms.abug %]).

Checking in editfields.cgi;
/cvsroot/mozilla/webtools/bugzilla/editfields.cgi,v  <--  editfields.cgi
new revision: 1.9; previous revision: 1.8
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.110; previous revision: 1.109
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.30; previous revision: 1.29
done
Checking in docs/xml/administration.xml;
/cvsroot/mozilla/webtools/bugzilla/docs/xml/administration.xml,v  <--  administration.xml
new revision: 1.83; previous revision: 1.82
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
new revision: 1.9; previous revision: 1.8
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
new revision: 1.7; previous revision: 1.6
done
Checking in template/en/default/global/messages.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/messages.html.tmpl,v  <--  messages.html.tmpl
new revision: 1.71; previous revision: 1.70
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.244; previous revision: 1.243
done
Checking in template/en/default/admin/custom_fields/confirm-delete.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/custom_fields/confirm-delete.html.tmpl,v  <--  confirm-delete.html.tmpl
initial revision: 1.1
done

Comment 24

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.2 in a patch on bug 432331.