From a43231dd4ccef2b02fa0434217b637a6d1638c97 Mon Sep 17 00:00:00 2001 From: "mkanat%bugzilla.org" <> Date: Fri, 3 Oct 2008 06:40:15 +0000 Subject: Bug 456922: Now that Bugzilla::Field::Choice is complete, clean up editvalues.cgi and error messages Patch By Max Kanat-Alexander r=bbaetz, a=mkanat --- Bugzilla/Field/Choice.pm | 11 +- editvalues.cgi | 210 +++++---------------- .../admin/fieldvalues/confirm-delete.html.tmpl | 83 ++++---- .../en/default/admin/fieldvalues/create.html.tmpl | 13 +- .../en/default/admin/fieldvalues/edit.html.tmpl | 32 ++-- .../en/default/admin/fieldvalues/footer.html.tmpl | 9 +- .../en/default/admin/fieldvalues/list.html.tmpl | 31 ++- template/en/default/global/messages.html.tmpl | 16 +- template/en/default/global/user-error.html.tmpl | 27 +-- 9 files changed, 151 insertions(+), 281 deletions(-) diff --git a/Bugzilla/Field/Choice.pm b/Bugzilla/Field/Choice.pm index a5c5fe6b1..dbdfea1a3 100644 --- a/Bugzilla/Field/Choice.pm +++ b/Bugzilla/Field/Choice.pm @@ -174,18 +174,17 @@ sub remove_from_db { my $self = shift; if ($self->is_default) { ThrowUserError('fieldvalue_is_default', - { field => $self->field, value => $self->name, + { field => $self->field, value => $self, param_name => $self->DEFAULT_MAP->{$self->field->name}, }); } if ($self->is_static) { ThrowUserError('fieldvalue_not_deletable', - { field => $self->field, value => $self->name }); + { field => $self->field, value => $self }); } if ($self->bug_count) { ThrowUserError("fieldvalue_still_has_bugs", - { field => $self->field, value => $self->name, - count => $self->bug_count }); + { field => $self->field, value => $self }); } $self->SUPER::remove_from_db(); } @@ -272,7 +271,7 @@ sub _check_value { && $invocant->is_static) { ThrowUserError('fieldvalue_not_editable', - { field => $field, old_value => $invocant->name }); + { field => $field, old_value => $invocant }); } ThrowUserError('fieldvalue_undefined') if !defined $value || $value eq ""; @@ -282,7 +281,7 @@ sub _check_value { my $exists = $invocant->type($field)->new({ name => $value }); if ($exists && (!blessed($invocant) || $invocant->id != $exists->id)) { ThrowUserError('fieldvalue_already_exists', - { field => $field, value => $value }); + { field => $field, value => $exists }); } return $value; diff --git a/editvalues.cgi b/editvalues.cgi index 2992a5f3d..85d4c9761 100755 --- a/editvalues.cgi +++ b/editvalues.cgi @@ -25,79 +25,33 @@ use Bugzilla; use Bugzilla::Util; use Bugzilla::Error; use Bugzilla::Constants; -use Bugzilla::Config qw(:admin); use Bugzilla::Token; use Bugzilla::Field; -use Bugzilla::Bug; -use Bugzilla::Status qw(SPECIAL_STATUS_WORKFLOW_ACTIONS); +use Bugzilla::Field::Choice; # List of different tables that contain the changeable field values # (the old "enums.") Keep them in alphabetical order by their # English name from field-descs.html.tmpl. # Format: Array of valid field names. -our @valid_fields = ('op_sys', 'rep_platform', 'priority', 'bug_severity', +my @valid_fields = ('op_sys', 'rep_platform', 'priority', 'bug_severity', 'bug_status', 'resolution'); # Add custom select fields. -my @custom_fields = Bugzilla->get_fields({custom => 1, - type => FIELD_TYPE_SINGLE_SELECT}); -push(@custom_fields, Bugzilla->get_fields({custom => 1, - type => FIELD_TYPE_MULTI_SELECT})); - +my @custom_fields = Bugzilla->get_fields( + {custom => 1, type => [FIELD_TYPE_SINGLE_SELECT, FIELD_TYPE_MULTI_SELECT]}); push(@valid_fields, map { $_->name } @custom_fields); -###################################################################### -# Subroutines -###################################################################### - -# Returns whether or not the specified table exists in the @tables array. -sub FieldExists { - my ($field) = @_; - - return lsearch(\@valid_fields, $field) >= 0; -} - -# Same as FieldExists, but emits and error and dies if it fails. -sub FieldMustExist { - my ($field)= @_; - - $field || - ThrowUserError('fieldname_not_specified'); - - # Is it a valid field to be editing? - FieldExists($field) || - ThrowUserError('fieldname_invalid', {'field' => $field}); - - return new Bugzilla::Field({name => $field}); -} - -# Returns if the specified value exists for the field specified. -sub ValueExists { - my ($field, $value) = @_; - # Value is safe because it's being passed only to a SELECT - # statement via a placeholder. - trick_taint($value); +############### +# Subroutines # +############### - my $dbh = Bugzilla->dbh; - my $value_count = - $dbh->selectrow_array("SELECT COUNT(*) FROM $field " - . " WHERE value = ?", undef, $value); - - return $value_count; -} - -# Same check as ValueExists, emits an error text and dies if it fails. -sub ValueMustExist { - my ($field, $value)= @_; - - # Values may not be empty (it's very difficult to deal - # with empty values in the admin interface). - trim($value) || ThrowUserError('fieldvalue_not_specified'); - - # Does it exist in the DB? - ValueExists($field, $value) || - ThrowUserError('fieldvalue_doesnt_exist', {'value' => $value, - 'field' => $field}); +sub display_field_values { + my $vars = shift; + my $template = Bugzilla->template; + $vars->{'values'} = $vars->{'field'}->legal_values; + $template->process("admin/fieldvalues/list.html.tmpl", $vars) + || ThrowTemplateError($template->error()); + exit; } ###################################################################### @@ -110,7 +64,7 @@ Bugzilla->login(LOGIN_REQUIRED); my $dbh = Bugzilla->dbh; my $cgi = Bugzilla->cgi; my $template = Bugzilla->template; -local our $vars = {}; +my $vars = {}; # Replace this entry by separate entries in templates when # the documentation about legal values becomes bigger. @@ -126,134 +80,79 @@ Bugzilla->user->in_group('admin') || # # often-used variables # -my $field = trim($cgi->param('field') || ''); -my $value = trim($cgi->param('value') || ''); -my $sortkey = trim($cgi->param('sortkey') || '0'); -my $action = trim($cgi->param('action') || ''); -my $token = $cgi->param('token'); - -# Gives the name of the parameter associated with the field -# and representing its default value. -local our %defaults; -$defaults{'op_sys'} = 'defaultopsys'; -$defaults{'rep_platform'} = 'defaultplatform'; -$defaults{'priority'} = 'defaultpriority'; -$defaults{'bug_severity'} = 'defaultseverity'; - -# Alternatively, a list of non-editable values can be specified. -# In this case, only the sortkey can be altered. -local our %static; -$static{'bug_status'} = ['UNCONFIRMED', Bugzilla->params->{'duplicate_or_move_bug_status'}]; -$static{'resolution'} = ['', 'FIXED', 'MOVED', 'DUPLICATE']; -$static{$_->name} = ['---'] foreach (@custom_fields); +my $action = trim($cgi->param('action') || ''); +my $token = $cgi->param('token'); # # field = '' -> Show nice list of fields # -unless ($field) { +if (!$cgi->param('field')) { # Convert @valid_fields into the format that select-field wants. - my @field_list = (); - foreach my $field_name (@valid_fields) { - push(@field_list, {name => $field_name}); - } + my @field_list = map({ name => $_ }, @valid_fields); $vars->{'fields'} = \@field_list; - $template->process("admin/fieldvalues/select-field.html.tmpl", - $vars) + $template->process("admin/fieldvalues/select-field.html.tmpl", $vars) || ThrowTemplateError($template->error()); exit; } -# At this point, the field is defined. -my $field_obj = FieldMustExist($field); -$vars->{'field'} = $field_obj; -trick_taint($field); - -sub display_field_values { - my $template = Bugzilla->template; - my $field = $vars->{'field'}->name; - - $vars->{'values'} = $vars->{'field'}->legal_values; - $vars->{'default'} = Bugzilla->params->{$defaults{$field}} if defined $defaults{$field}; - $vars->{'static'} = $static{$field} if exists $static{$field}; - - $template->process("admin/fieldvalues/list.html.tmpl", $vars) - || ThrowTemplateError($template->error()); - exit; +# At this point, the field must be defined. +my $field = Bugzilla::Field->check($cgi->param('field')); +if (!grep($_ eq $field->name, @valid_fields)) { + ThrowUserError('fieldname_invalid', { field => $field }); } +$vars->{'field'} = $field; # # action='' -> Show nice list of values. # -display_field_values() unless $action; +display_field_values($vars) unless $action; # # action='add' -> show form for adding new field value. # (next action will be 'new') # if ($action eq 'add') { - $vars->{'value'} = $value; $vars->{'token'} = issue_session_token('add_field_value'); - $template->process("admin/fieldvalues/create.html.tmpl", - $vars) + $template->process("admin/fieldvalues/create.html.tmpl", $vars) || ThrowTemplateError($template->error()); - exit; } - # # action='new' -> add field value entered in the 'action=add' screen # if ($action eq 'new') { check_token_data($token, 'add_field_value'); - my $created_value = Bugzilla::Field::Choice->type($field_obj)->create( - { value => $value, sortkey => $sortkey, + my $created_value = Bugzilla::Field::Choice->type($field)->create( + { value => scalar $cgi->param('value'), + sortkey => scalar $cgi->param('sortkey'), is_open => scalar $cgi->param('is_open') }); delete_token($token); $vars->{'message'} = 'field_value_created'; - $vars->{'value'} = $created_value->name; - display_field_values(); + $vars->{'value'} = $created_value; + display_field_values($vars); } +# After this, we always have a value +my $value = Bugzilla::Field::Choice->type($field)->check($cgi->param('value')); +$vars->{'value'} = $value; # # action='del' -> ask if user really wants to delete # (next action would be 'delete') # if ($action eq 'del') { - ValueMustExist($field, $value); - trick_taint($value); - - # See if any bugs are still using this value. - if ($field_obj->type != FIELD_TYPE_MULTI_SELECT) { - $vars->{'bug_count'} = - $dbh->selectrow_array("SELECT COUNT(*) FROM bugs WHERE $field = ?", - undef, $value); - } - else { - $vars->{'bug_count'} = - $dbh->selectrow_array("SELECT COUNT(*) FROM bug_$field WHERE value = ?", - undef, $value); - } - - $vars->{'value_count'} = - $dbh->selectrow_array("SELECT COUNT(*) FROM $field"); - - $vars->{'value'} = $value; - $vars->{'param_name'} = $defaults{$field}; - # If the value cannot be deleted, throw an error. - if (lsearch($static{$field}, $value) >= 0) { + if ($value->is_static) { ThrowUserError('fieldvalue_not_deletable', $vars); } $vars->{'token'} = issue_session_token('delete_field_value'); - $template->process("admin/fieldvalues/confirm-delete.html.tmpl", - $vars) + $template->process("admin/fieldvalues/confirm-delete.html.tmpl", $vars) || ThrowTemplateError($template->error()); exit; @@ -265,15 +164,11 @@ if ($action eq 'del') { # if ($action eq 'delete') { check_token_data($token, 'delete_field_value'); - my $value_obj = Bugzilla::Field::Choice->type($field)->check($value); - $vars->{'value'} = $value_obj->name; - $value_obj->remove_from_db(); - + $value->remove_from_db(); delete_token($token); - $vars->{'message'} = 'field_value_deleted'; $vars->{'no_edit_link'} = 1; - display_field_values(); + display_field_values($vars); } @@ -282,20 +177,7 @@ if ($action eq 'delete') { # (next action would be 'update') # if ($action eq 'edit') { - ValueMustExist($field, $value); - trick_taint($value); - - $vars->{'sortkey'} = $dbh->selectrow_array( - "SELECT sortkey FROM $field WHERE value = ?", undef, $value) || 0; - - $vars->{'value'} = $value; - $vars->{'is_static'} = (lsearch($static{$field}, $value) >= 0) ? 1 : 0; $vars->{'token'} = issue_session_token('edit_field_value'); - if ($field eq 'bug_status') { - $vars->{'is_open'} = $dbh->selectrow_array('SELECT is_open FROM bug_status - WHERE value = ?', undef, $value); - } - $template->process("admin/fieldvalues/edit.html.tmpl", $vars) || ThrowTemplateError($template->error()); @@ -308,17 +190,13 @@ if ($action eq 'edit') { # if ($action eq 'update') { check_token_data($token, 'edit_field_value'); - $vars->{'value'} = $cgi->param('valueold'); - my $value_obj = Bugzilla::Field::Choice->type($field_obj) - ->check($cgi->param('valueold')); - $value_obj->set_name($value); - $value_obj->set_sortkey($sortkey); - $vars->{'changes'} = $value_obj->update(); + $vars->{'value_old'} = $value->name; + $value->set_name($cgi->param('value_new')); + $value->set_sortkey($cgi->param('sortkey')); + $vars->{'changes'} = $value->update(); delete_token($token); - - $vars->{'value_obj'} = $value_obj; $vars->{'message'} = 'field_value_updated'; - display_field_values(); + display_field_values($vars); } diff --git a/template/en/default/admin/fieldvalues/confirm-delete.html.tmpl b/template/en/default/admin/fieldvalues/confirm-delete.html.tmpl index d18e03a53..12be0e8e4 100644 --- a/template/en/default/admin/fieldvalues/confirm-delete.html.tmpl +++ b/template/en/default/admin/fieldvalues/confirm-delete.html.tmpl @@ -14,18 +14,14 @@ #%] [%# INTERFACE: - # value: string; The field value being deleted. - # bug_count: number; The number of bugs that have this field value. - # value_count: number; The number of values left for this field, including - # this value. + # value: Bugzilla::Field::Choice; The field value being deleted. + # value_count: number; The number of values available for this field. # field: object; the field the value is being deleted from. - # param_name: string; The name of the parameter (defaultxxx) associated - # with the field. #%] [% title = BLOCK %] - Delete Value '[% value FILTER html %]' from the '[% field.description FILTER html %]' - ([% field.name FILTER html %]) field + Delete Value '[% value.name FILTER html %]' from the + '[% field.description FILTER html %]' ([% field.name FILTER html %]) field [% END %] [% PROCESS global/header.html.tmpl @@ -44,15 +40,18 @@ Field Value: - [% value FILTER html %] + [% value.name FILTER html %] [% terms.Bugs %]: -[% IF bug_count %] - [% bug_count FILTER html %] +[% IF value.bug_count %] + + [%- value.bug_count FILTER html %] [% ELSE %] None [% END %] @@ -62,44 +61,52 @@

Confirmation

-[% IF (param_name.defined && Param(param_name) == value) || bug_count || (value_count == 1) %] +[% IF value.is_default || value.bug_count || (value_count == 1) %] -

Sorry, but the '[% value FILTER html %]' value cannot be deleted - from the '[% field.description FILTER html %]' field for the following reason(s):

+

Sorry, but the '[% value.name FILTER html %]' value cannot be deleted + from the '[% field.description FILTER html %]' field for the following + reason(s):

@@ -111,7 +118,7 @@ - + diff --git a/template/en/default/admin/fieldvalues/create.html.tmpl b/template/en/default/admin/fieldvalues/create.html.tmpl index fe906bfe3..9d318cd1d 100644 --- a/template/en/default/admin/fieldvalues/create.html.tmpl +++ b/template/en/default/admin/fieldvalues/create.html.tmpl @@ -26,26 +26,27 @@ %]

- This page allows you to add a new value for the '[% field.description FILTER html %]' field. + This page allows you to add a new value for the + '[% field.description FILTER html %]' field.

- + - + [% IF field.name == "bug_status" %]
- +
diff --git a/template/en/default/admin/fieldvalues/edit.html.tmpl b/template/en/default/admin/fieldvalues/edit.html.tmpl index 98b480ba8..29f881ec8 100644 --- a/template/en/default/admin/fieldvalues/edit.html.tmpl +++ b/template/en/default/admin/fieldvalues/edit.html.tmpl @@ -14,16 +14,15 @@ #%] [%# INTERFACE: - # value: string; The field value we are editing. - # sortkey: number; Sortkey of the field value we are editing. - # field: object; The field this value belongs to. + # value: Bugzilla::Field::Choice; The field value we are editing. + # field: Bugzilla::Field; The field this value belongs to. #%] [% PROCESS global/variables.none.tmpl %] [% title = BLOCK %] - Edit Value '[% value FILTER html %]' for the '[% field.description FILTER html %]' - ([% field.name FILTER html %]) field + Edit Value '[% value.name FILTER html %]' for the + '[% field.description FILTER html %]' ([% field.name FILTER html %]) field [% END %] [% PROCESS global/header.html.tmpl title = title @@ -33,32 +32,33 @@ - + - + [% IF field.name == "bug_status" %] - + [% END %]
- [% IF is_static %] - - [% value FILTER html %] + [% IF value.is_static %] + + [%- value.name FILTER html %] [% ELSE %] - + [% END %]
[% IF is_open %]Open[% ELSE %]Closed[% END %][% IF value.is_open %]Open[% ELSE %]Closed[% END %]
- - + diff --git a/template/en/default/admin/fieldvalues/footer.html.tmpl b/template/en/default/admin/fieldvalues/footer.html.tmpl index dcb6dbc8d..288612d4c 100644 --- a/template/en/default/admin/fieldvalues/footer.html.tmpl +++ b/template/en/default/admin/fieldvalues/footer.html.tmpl @@ -35,13 +35,14 @@ [%- field.name FILTER url_quote %]">Add a value. [% END %] -[% IF value && !no_edit_link %] +[% IF value.defined && !no_edit_link %] Edit value - '[% value FILTER html %]'. + [%- field.name FILTER url_quote %]&value= + [%- value.name FILTER url_quote %]"> + '[% value.name FILTER html %]'. [% END %] [% UNLESS no_edit_other_link %] diff --git a/template/en/default/admin/fieldvalues/list.html.tmpl b/template/en/default/admin/fieldvalues/list.html.tmpl index d14bbc26d..976b58ae7 100644 --- a/template/en/default/admin/fieldvalues/list.html.tmpl +++ b/template/en/default/admin/fieldvalues/list.html.tmpl @@ -58,26 +58,13 @@ } ] %] -[% IF default.defined %] - [% overrides.action = [ { - match_value => "$default" - match_field => 'name' - override_content => 1 - content => "(Default value)" - override_contentlink => 1 - contentlink => undef - } ] - %] -[% END %] -[% IF static.size %] - [% UNLESS overrides.action.size %] - [% overrides.action = [] %] - [% END %] +[% SET overrides.action = [] %] +[% FOREACH check_value = values %] - [% FOREACH static_value = static %] + [% IF check_value.is_static %] [% overrides.action.push({ - match_value => "$static_value" + match_value => check_value.name match_field => 'name' override_content => 1 content => "(Non-deletable value)" @@ -85,7 +72,17 @@ contentlink => undef }) %] + [% ELSIF check_value.is_default %] + [% overrides.action.push({ + match_value => check_value.name + match_field => 'name' + override_content => 1 + content => "(Default value)" + override_contentlink => 1 + contentlink => undef }) + %] [% END %] + [% END %] [% PROCESS admin/table.html.tmpl diff --git a/template/en/default/global/messages.html.tmpl b/template/en/default/global/messages.html.tmpl index 82a71a5a2..d7de4fbc7 100644 --- a/template/en/default/global/messages.html.tmpl +++ b/template/en/default/global/messages.html.tmpl @@ -298,31 +298,31 @@ [% ELSIF message_tag == "field_value_created" %] [% title = "New Field Value Created" %] - The value [% value FILTER html %] has been added as a valid choice - for the [% field.description FILTER html %] + The value [% value.name FILTER html %] has been added as a + valid choice for the [% field.description FILTER html %] ([% field.name FILTER html %]) field. [% IF field.name == "bug_status" %] - You should now visit the status workflow page - to include your new [% terms.bug %] status. + You should now visit the status workflow + page to include your new [% terms.bug %] status. [% END %] [% ELSIF message_tag == "field_value_deleted" %] [% title = "Field Value Deleted" %] - The value [% value FILTER html %] of the + The value [% value.name FILTER html %] of the [% field.description FILTER html %] ([% field.name FILTER html %]) field has been deleted. [% ELSIF message_tag == "field_value_updated" %] [% title = "Field Value Updated" %] [% IF changes.keys.size %] - The [% value FILTER html %] value of the + The [% value_old FILTER html %] value of the [% field.description FILTER html %] ([% field.name FILTER html %]) field has been changed:
    [% IF changes.value %]
  • Field value updated to [% changes.value.1 FILTER html %]. - [% IF value_obj.is_default %] + [% IF value.is_default %] (Note that this value is the default for this field. All references to the default value will now point to this new value.) [% END %] @@ -334,7 +334,7 @@ [% END %]
[% ELSE %] - No changes made to the field value [% value FILTER html %]. + No changes made to the field value [% value_old FILTER html %]. [% END %] [% ELSIF message_tag == "flag_cleared" %] diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index c1fc9ae0d..c7b83e334 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/template/en/default/global/user-error.html.tmpl @@ -422,26 +422,17 @@ [% ELSIF error == "fieldname_invalid" %] [% title = "Specified Field Does Not Exist" %] - The field '[% field FILTER html %]' does not exist or + The field '[% field.name FILTER html %]' does not exist or cannot be edited with this interface. - [% ELSIF error == "fieldname_not_specified" %] - [% title = "Field Name Not Specified" %] - No field name specified when trying to edit field values. - [% ELSIF error == "fieldvalue_already_exists" %] [% title = "Field Value Already Exists" %] - The value '[% value FILTER html %]' already exists for the + The value '[% value.name FILTER html %]' already exists for the [%+ field.description FILTER html %] field. - [% ELSIF error == "fieldvalue_doesnt_exist" %] - [% title = "Specified Field Value Does Not Exist" %] - The value '[% value FILTER html %]' does not exist for - the '[% field FILTER html %]' field. - [% ELSIF error == "fieldvalue_is_default" %] [% title = "Specified Field Value Is Default" %] - '[% value FILTER html %]' is the default value for + '[% value.name FILTER html %]' is the default value for the '[% field.description FILTER html %]' field and cannot be deleted. [% IF user.in_group('tweakparams') %] You have to