From fab5d3a38aadaed1e6153c0fbd820449258586b2 Mon Sep 17 00:00:00 2001 From: "mkanat%bugzilla.org" <> Date: Fri, 3 Oct 2008 06:30:38 +0000 Subject: Bug 455641: Implement Bugzilla::Field::Choice->update and have editvalues.cgi use it Patch By Max Kanat-Alexander r=bbaetz, a=mkanat --- Bugzilla/Bug.pm | 3 +- Bugzilla/Field/Choice.pm | 84 ++++++++++++++++++++++- Bugzilla/Object.pm | 13 ++++ Bugzilla/Product.pm | 4 +- Bugzilla/Status.pm | 12 +++- editusers.cgi | 6 +- editvalues.cgi | 90 ++----------------------- template/en/default/global/messages.html.tmpl | 25 ++++--- template/en/default/global/user-error.html.tmpl | 5 +- 9 files changed, 136 insertions(+), 106 deletions(-) diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index d620f222c..d3aa1eeec 100644 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -581,8 +581,7 @@ sub update { # inside this function. my $delta_ts = shift || $dbh->selectrow_array("SELECT NOW()"); - my $old_bug = $self->new($self->id); - my $changes = $self->SUPER::update(@_); + my ($changes, $old_bug) = $self->SUPER::update(@_); # Certain items in $changes have to be fixed so that they hold # a name instead of an ID. diff --git a/Bugzilla/Field/Choice.pm b/Bugzilla/Field/Choice.pm index 7705258eb..9db5c5efd 100644 --- a/Bugzilla/Field/Choice.pm +++ b/Bugzilla/Field/Choice.pm @@ -24,6 +24,7 @@ package Bugzilla::Field::Choice; use base qw(Bugzilla::Object); +use Bugzilla::Config qw(SetParam write_params); use Bugzilla::Constants; use Bugzilla::Error; use Bugzilla::Field; @@ -41,6 +42,11 @@ use constant DB_COLUMNS => qw( sortkey ); +use constant UPDATE_COLUMNS => qw( + value + sortkey +); + use constant NAME_FIELD => 'value'; use constant LIST_ORDER => 'sortkey, value'; @@ -55,6 +61,13 @@ use constant CLASS_MAP => { bug_status => 'Bugzilla::Status', }; +use constant DEFAULT_MAP => { + op_sys => 'defaultopsys', + rep_platform => 'defaultplatform', + priority => 'defaultpriority', + bug_severity => 'defaultseverity', +}; + ################# # Class Factory # ################# @@ -127,6 +140,37 @@ sub create { return $class->SUPER::create(@_); } +sub update { + my $self = shift; + my $dbh = Bugzilla->dbh; + my $fname = $self->field->name; + + $dbh->bz_start_transaction(); + + my ($changes, $old_self) = $self->SUPER::update(@_); + if (exists $changes->{value}) { + my ($old, $new) = @{ $changes->{value} }; + if ($self->field->type == FIELD_TYPE_MULTI_SELECT) { + $dbh->do("UPDATE bug_$fname SET value = ? WHERE value = ?", + undef, $new, $old); + } + else { + $dbh->do("UPDATE bugs SET $fname = ? WHERE $fname = ?", + undef, $new, $old); + } + + if ($old_self->is_default) { + my $param = $self->DEFAULT_MAP->{$self->field->name}; + SetParam($param, $self->name); + write_params(); + } + } + + $dbh->bz_commit_transaction(); + return $changes; +} + + ############# # Accessors # ############# @@ -143,6 +187,35 @@ sub field { return $cache->{"field_$class"}; } +sub is_default { + my $self = shift; + my $param_value = + Bugzilla->params->{ $self->DEFAULT_MAP->{$self->field->name} }; + return 0 if !defined $param_value; + return $self->name eq $param_value ? 1 : 0; +} + +sub is_static { + my $self = shift; + # If we need to special-case Resolution for *anything* else, it should + # get its own subclass. + if ($self->field->name eq 'resolution') { + return grep($_ eq $self->name, ('', 'FIXED', 'MOVED', 'DUPLICATE')) + ? 1 : 0; + } + elsif ($self->field->custom) { + return $self->name eq '---' ? 1 : 0; + } + return 0; +} + +############ +# Mutators # +############ + +sub set_name { $_[0]->set('value', $_[1]); } +sub set_sortkey { $_[0]->set('sortkey', $_[1]); } + ############## # Validators # ############## @@ -153,12 +226,21 @@ sub _check_value { my $field = $invocant->field; $value = trim($value); + + # Make sure people don't rename static values + if (blessed($invocant) && $value ne $invocant->name + && $invocant->is_static) + { + ThrowUserError('fieldvalue_not_editable', + { field => $field, old_value => $invocant->name }); + } + ThrowUserError('fieldvalue_undefined') if !defined $value || $value eq ""; ThrowUserError('fieldvalue_name_too_long', { value => $value }) if length($value) > MAX_FIELD_VALUE_SIZE; my $exists = $invocant->type($field)->new({ name => $value }); - if ($exists) { + if ($exists && (!blessed($invocant) || $invocant->id != $exists->id)) { ThrowUserError('fieldvalue_already_exists', { field => $field, value => $value }); } diff --git a/Bugzilla/Object.pm b/Bugzilla/Object.pm index bcd436484..cde440b95 100644 --- a/Bugzilla/Object.pm +++ b/Bugzilla/Object.pm @@ -283,6 +283,10 @@ sub update { $dbh->bz_commit_transaction(); + if (wantarray) { + return (\%changes, $old_self); + } + return \%changes; } @@ -703,6 +707,8 @@ updated, and they will only be updated if their values have changed. =item B +B + A hashref showing what changed during the update. The keys are the column names from L. If a field was not changed, it will not be in the hash at all. If the field was changed, the key will point to an arrayref. @@ -711,6 +717,13 @@ will be the new value. If there were no changes, we return a reference to an empty hash. +B + +Returns a list, where the first item is the above hashref. The second item +is the object as it was in the database before update() was called. (This +is mostly useful to subclasses of C that are implementing +C.) + =back =back diff --git a/Bugzilla/Product.pm b/Bugzilla/Product.pm index 235a06926..8fb7b7ca9 100644 --- a/Bugzilla/Product.pm +++ b/Bugzilla/Product.pm @@ -147,7 +147,7 @@ sub update { # Don't update the DB if something goes wrong below -> transaction. $dbh->bz_start_transaction(); - my $changes = $self->SUPER::update(@_); + my ($changes, $old_self) = $self->SUPER::update(@_); # We also have to fix votes. my @msgs; # Will store emails to send to voters. @@ -247,7 +247,7 @@ sub update { require Bugzilla::Bug; import Bugzilla::Bug qw(LogActivityEntry); - my $old_settings = $self->new($self->id)->group_controls; + my $old_settings = $old_self->group_controls; my $new_settings = $self->group_controls; my $timestamp = $dbh->selectrow_array('SELECT NOW()'); diff --git a/Bugzilla/Status.pm b/Bugzilla/Status.pm index 5f37974e7..565433850 100644 --- a/Bugzilla/Status.pm +++ b/Bugzilla/Status.pm @@ -77,11 +77,19 @@ sub create { ##### Accessors #### ############################### -sub name { return $_[0]->{'value'}; } -sub sortkey { return $_[0]->{'sortkey'}; } sub is_active { return $_[0]->{'isactive'}; } sub is_open { return $_[0]->{'is_open'}; } +sub is_static { + my $self = shift; + if ($self->name eq 'UNCONFIRMED' + || $self->name eq Bugzilla->params->{'duplicate_or_move_bug_status'}) + { + return 1; + } + return 0; +} + ############## # Validators # ############## diff --git a/editusers.cgi b/editusers.cgi index f133b7409..a75f42040 100755 --- a/editusers.cgi +++ b/editusers.cgi @@ -238,7 +238,7 @@ if ($action eq 'search') { # Update profiles table entry; silently skip doing this if the user # is not authorized. - my %changes; + my $changes = {}; if ($editusers) { $otherUser->set_login($cgi->param('login')); $otherUser->set_name($cgi->param('name')); @@ -246,7 +246,7 @@ if ($action eq 'search') { if $cgi->param('password'); $otherUser->set_disabledtext($cgi->param('disabledtext')); $otherUser->set_disable_mail($cgi->param('disable_mail')); - %changes = %{$otherUser->update()}; + $changes = $otherUser->update(); } # Update group settings. @@ -334,7 +334,7 @@ if ($action eq 'search') { delete_token($token); $vars->{'message'} = 'account_updated'; - $vars->{'changed_fields'} = [keys %changes]; + $vars->{'changed_fields'} = [keys %$changes]; $vars->{'groups_added_to'} = \@groupsAddedTo; $vars->{'groups_removed_from'} = \@groupsRemovedFrom; $vars->{'groups_granted_rights_to_bless'} = \@groupsGrantedRightsToBless; diff --git a/editvalues.cgi b/editvalues.cgi index 4525774e1..f6a602b55 100755 --- a/editvalues.cgi +++ b/editvalues.cgi @@ -356,91 +356,15 @@ if ($action eq 'edit') { # if ($action eq 'update') { check_token_data($token, 'edit_field_value'); - my $valueold = trim($cgi->param('valueold') || ''); - my $sortkeyold = trim($cgi->param('sortkeyold') || '0'); - - ValueMustExist($field, $valueold); - trick_taint($valueold); - - $vars->{'value'} = $value; - # If the value cannot be renamed, throw an error. - if (lsearch($static{$field}, $valueold) >= 0 && $value ne $valueold) { - $vars->{'old_value'} = $valueold; - ThrowUserError('fieldvalue_not_editable', $vars); - } - - if (length($value) > 60) { - ThrowUserError('fieldvalue_name_too_long', $vars); - } - - $dbh->bz_start_transaction(); - - # Need to store because detaint_natural() will delete this if - # invalid - my $stored_sortkey = $sortkey; - if ($sortkey != $sortkeyold) { - - if (!detaint_natural($sortkey)) { - ThrowUserError('fieldvalue_sortkey_invalid', - {'name' => $field, - 'sortkey' => $stored_sortkey}); - - } - - $dbh->do("UPDATE $field SET sortkey = ? WHERE value = ?", - undef, $sortkey, $valueold); - - $vars->{'updated_sortkey'} = 1; - $vars->{'sortkey'} = $sortkey; - } - - if ($value ne $valueold) { - - unless ($value) { - ThrowUserError('fieldvalue_undefined'); - } - if (ValueExists($field, $value)) { - ThrowUserError('fieldvalue_already_exists', $vars); - } - if ($field eq 'bug_status' - && (grep { lc($value) eq $_ } SPECIAL_STATUS_WORKFLOW_ACTIONS)) - { - $vars->{'value'} = $value; - ThrowUserError('fieldvalue_reserved_word', $vars); - } - trick_taint($value); - - if ($field_obj->type != FIELD_TYPE_MULTI_SELECT) { - $dbh->do("UPDATE bugs SET $field = ? WHERE $field = ?", - undef, $value, $valueold); - } - else { - $dbh->do("UPDATE bug_$field SET value = ? WHERE value = ?", - undef, $value, $valueold); - } - - $dbh->do("UPDATE $field SET value = ? WHERE value = ?", - undef, $value, $valueold); - - $vars->{'updated_value'} = 1; - } - - $dbh->bz_commit_transaction(); - - # If the old value was the default value for the field, - # update data/params accordingly. - # This update is done while tables are unlocked due to the - # annoying calls in Bugzilla/Config/Common.pm. - if (defined $defaults{$field} - && $value ne $valueold - && $valueold eq Bugzilla->params->{$defaults{$field}}) - { - SetParam($defaults{$field}, $value); - write_params(); - $vars->{'default_value_updated'} = 1; - } + $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(); delete_token($token); + $vars->{'value_obj'} = $value_obj; $vars->{'message'} = 'field_value_updated'; display_field_values(); } diff --git a/template/en/default/global/messages.html.tmpl b/template/en/default/global/messages.html.tmpl index eb869a776..82a71a5a2 100644 --- a/template/en/default/global/messages.html.tmpl +++ b/template/en/default/global/messages.html.tmpl @@ -314,20 +314,23 @@ [% ELSIF message_tag == "field_value_updated" %] [% title = "Field Value Updated" %] - [% IF updated_value || updated_sortkey %] - Changes to the [% value FILTER html %] value of the + [% IF changes.keys.size %] + The [% value FILTER html %] value of the [% field.description FILTER html %] - ([% field.name FILTER html %]) field have been changed: + ([% field.name FILTER html %]) field has been changed:
    - [% IF updated_value %] -
  • Field value updated to [% value FILTER html %]
    • - [% IF default_value_updated %] - (note that this value is the default for this field. All - references to the default value will now point to this new value) - [% END %] + [% IF changes.value %] +
  • Field value updated to + [% changes.value.1 FILTER html %]. + [% IF value_obj.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 %] +
    • [% END %] - [% IF updated_sortkey %] -
  • Field value sortkey updated to [% sortkey FILTER html %]
    • + [% IF changes.sortkey %] +
  • Sortkey updated to + [% changes.sortkey.1 FILTER html %].
    • [% END %]
[% ELSE %] diff --git a/template/en/default/global/user-error.html.tmpl b/template/en/default/global/user-error.html.tmpl index bcb6b6630..c306b692a 100644 --- a/template/en/default/global/user-error.html.tmpl +++ b/template/en/default/global/user-error.html.tmpl @@ -457,7 +457,8 @@ [% ELSIF error == "fieldvalue_not_editable" %] [% title = "Field Value Not Editable" %] The value '[% old_value FILTER html %]' cannot be renamed because - it plays some special role for the '[% field.description FILTER html %]' field. + it plays some special role for the '[% field.description FILTER html %]' + field. [% ELSIF error == "fieldvalue_not_deletable" %] [% title = "Field Value Not Deletable" %] @@ -470,7 +471,7 @@ [% ELSIF error == "fieldvalue_reserved_word" %] [% title = "Reserved Word Not Allowed" %] - You cannot use the '[% value FILTER html %]' value for the + You cannot use the value '[% value FILTER html %]' for the '[% field.description FILTER html %]' field. This value is used internally. Please choose another one. -- cgit v1.2.3-24-g4f1b