From f0b6242097e487198de559d39d4169a2a0a4d8e7 Mon Sep 17 00:00:00 2001 From: Tiago Mello Date: Thu, 10 Feb 2011 23:31:28 -0200 Subject: Bug 620827: Refactor remove see also to use remove_from_db instead. r/a=mkanat --- Bugzilla/Bug.pm | 81 +++++++++++++++++++------------- Bugzilla/BugUrl.pm | 27 +++++++++++ Bugzilla/BugUrl/Bugzilla/Local.pm | 54 ++++++++++++++++++--- Bugzilla/DB/Schema.pm | 1 + Bugzilla/Install/DB.pm | 26 ++++++++++ Bugzilla/Util.pm | 26 +++++++--- template/en/default/bug/field.html.tmpl | 12 +++-- xt/lib/Bugzilla/Test/Search.pm | 4 +- xt/lib/Bugzilla/Test/Search/FieldTest.pm | 7 ++- 9 files changed, 183 insertions(+), 55 deletions(-) diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index e516e4bf8..42b316516 100644 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -51,7 +51,7 @@ use Bugzilla::Status; use Bugzilla::Comment; use Bugzilla::BugUrl; -use List::MoreUtils qw(firstidx uniq); +use List::MoreUtils qw(firstidx uniq part); use List::Util qw(min max first); use Storable qw(dclone); use URI; @@ -951,28 +951,17 @@ sub update { } # See Also - my @old_see_also = @{ $old_bug->see_also }; - foreach my $field_values (@{ $self->{added_see_also} || [] }) { - my $class = delete $field_values->{class}; - $class->insert_create_data($field_values); - push @{ $self->see_also }, $field_values->{value}; - } - - delete $self->{added_see_also}; - my ($removed_see, $added_see) = - diff_arrays(\@old_see_also, $self->see_also); + my ($removed_see, $added_see) = + diff_arrays($old_bug->see_also, $self->see_also, 'name'); - if (scalar @$removed_see) { - $dbh->do('DELETE FROM bug_see_also WHERE bug_id = ? AND ' - . $dbh->sql_in('value', [('?') x @$removed_see]), - undef, $self->id, @$removed_see); - } + $_->remove_from_db foreach @$removed_see; + $_->insert_create_data($_) foreach @$added_see; # If any changes were found, record it in the activity log - if (scalar @$removed_see || scalar @$added_see) { - $changes->{see_also} = [join(', ', @$removed_see), - join(', ', @$added_see)]; + if (scalar $removed_see || scalar $added_see) { + $changes->{see_also} = [join(', ', map { $_->name } @$removed_see), + join(', ', map { $_->name } @$added_see)]; } # Log bugs_activity items @@ -2825,32 +2814,36 @@ sub remove_group { sub add_see_also { my ($self, $input) = @_; + + # This is needed by xt/search.t. + $input = $input->name if blessed($input); + $input = trim($input); my ($class, $uri) = Bugzilla::BugUrl->class_for($input); - my $params = { value => $uri, bug_id => $self }; + my $params = { value => $uri, bug_id => $self, class => $class }; $class->check_required_create_fields($params); my $field_values = $class->run_create_validators($params); $uri = $field_values->{value}; $field_values->{value} = $uri->as_string; - $field_values->{class} = $class; # If this is a link to a local bug then save the # ref bug id for sending changes email. if ($class->isa('Bugzilla::BugUrl::Bugzilla::Local')) { my $ref_bug = $field_values->{ref_bug}; - my $self_url = $class->local_uri . $self->id; + my $self_url = $class->local_uri($self->id); push @{ $self->{see_also_changes} }, $ref_bug->id - if !grep { $_ eq $self_url } @{ $ref_bug->see_also }; + if !grep { $_->name eq $self_url } @{ $ref_bug->see_also }; } # We only add the new URI if it hasn't been added yet. URIs are # case-sensitive, but most of our DBs are case-insensitive, so we do # this check case-insensitively. my $value = $uri->as_string; - if (!grep { lc($_) eq lc($value) } @{ $self->see_also }) { + + if (!grep { lc($_->name) eq lc($value) } @{ $self->see_also }) { my $privs; my $can = $self->check_can_change_field('see_also', '', $value, \$privs); if (!$can) { @@ -2859,22 +2852,39 @@ sub add_see_also { privs => $privs }); } - push @{ $self->{added_see_also} }, $field_values; + push @{ $self->{see_also} }, bless ($field_values, $class); } } sub remove_see_also { my ($self, $url) = @_; my $see_also = $self->see_also; - my @new_see_also = grep { lc($_) ne lc($url) } @$see_also; + + # This is needed by xt/search.t. + $url = $url->name if blessed($url); + + my ($removed_bug_url, $new_see_also) = + part { lc($_->name) ne lc($url) } @$see_also; + + # Since we remove also the url from the referenced bug, + # we need to notify changes for that bug too. + $removed_bug_url = $removed_bug_url->[0]; + if ($removed_bug_url->isa('Bugzilla::BugUrl::Bugzilla::Local') + and defined $removed_bug_url->ref_bug_url) + { + push @{ $self->{see_also_changes} }, + $removed_bug_url->ref_bug_url->bug_id; + } + my $privs; - my $can = $self->check_can_change_field('see_also', $see_also, \@new_see_also, \$privs); + my $can = $self->check_can_change_field('see_also', $see_also, $new_see_also, \$privs); if (!$can) { ThrowUserError('illegal_change', { field => 'see_also', oldvalue => $url, privs => $privs }); - } - $self->{see_also} = \@new_see_also; + } + + $self->{see_also} = $new_see_also || []; } sub add_tag { @@ -3353,9 +3363,16 @@ sub reporter { sub see_also { my ($self) = @_; return [] if $self->{'error'}; - $self->{'see_also'} ||= Bugzilla->dbh->selectcol_arrayref( - 'SELECT value FROM bug_see_also WHERE bug_id = ?', undef, $self->id); - return $self->{'see_also'}; + if (!defined $self->{see_also}) { + my $ids = Bugzilla->dbh->selectcol_arrayref( + 'SELECT id FROM bug_see_also WHERE bug_id = ?', + undef, $self->id); + + my $bug_urls = Bugzilla::BugUrl->new_from_list($ids); + + $self->{see_also} = $bug_urls; + } + return $self->{see_also}; } sub status { diff --git a/Bugzilla/BugUrl.pm b/Bugzilla/BugUrl.pm index 521ee8193..f4f0e410b 100644 --- a/Bugzilla/BugUrl.pm +++ b/Bugzilla/BugUrl.pm @@ -40,6 +40,7 @@ use constant DB_COLUMNS => qw( id bug_id value + class ); # This must be strings with the names of the validations, @@ -48,6 +49,7 @@ use constant DB_COLUMNS => qw( use constant VALIDATORS => { value => '_check_value', bug_id => '_check_bug_id', + class => \&_check_class, }; # This is the order we go through all of subclasses and @@ -61,6 +63,13 @@ use constant SUB_CLASSES => qw( Bugzilla::BugUrl::Debian ); +############################### +#### Accessors ###### +############################### + +sub class { return $_[0]->{class} } +sub bug_id { return $_[0]->{bug_id} } + ############################### #### Methods #### ############################### @@ -92,6 +101,18 @@ sub new { return $class->SUPER::new(@_); } +sub _do_list_select { + my $class = shift; + my $objects = $class->SUPER::_do_list_select(@_); + + foreach my $object (@$objects) { + eval "use " . $object->class; die $@ if $@; + bless $object, $object->class; + } + + return $objects +} + # This is an abstract method. It must be overridden # in every subclass. sub should_handle { @@ -115,6 +136,12 @@ sub class_for { reason => 'show_bug' }); } +sub _check_class { + my ($class, $subclass) = @_; + eval "use $subclass"; die $@ if $@; + return $subclass; +} + sub _check_bug_id { my ($class, $bug_id) = @_; diff --git a/Bugzilla/BugUrl/Bugzilla/Local.pm b/Bugzilla/BugUrl/Bugzilla/Local.pm index 69d2cc151..233acbe66 100644 --- a/Bugzilla/BugUrl/Bugzilla/Local.pm +++ b/Bugzilla/BugUrl/Bugzilla/Local.pm @@ -37,29 +37,67 @@ use constant VALIDATOR_DEPENDENCIES => { #### Methods #### ############################### +sub ref_bug_url { + my $self = shift; + + if (!exists $self->{ref_bug_url}) { + my $ref_bug_id = new URI($self->name)->query_param('id'); + my $ref_value = $self->local_uri($self->bug_id); + $self->{ref_bug_url} = + new Bugzilla::BugUrl::Bugzilla::Local({ bug_id => $ref_bug_id, + value => $ref_value }); + } + return $self->{ref_bug_url}; +} + sub insert_create_data { my ($class, $field_values) = @_; my $ref_bug = delete $field_values->{ref_bug}; - my $url = $class->local_uri . $field_values->{bug_id}; my $bug_url = $class->SUPER::insert_create_data($field_values); + my $url = $class->local_uri($bug_url->bug_id); # Check if the ref bug has already the url and then, # update the ref bug to point to the current bug. - if (!grep { $_ eq $url } @{ $ref_bug->see_also }) { - $class->SUPER::insert_create_data( - { value => $url, bug_id => $ref_bug->id } ); + if (!grep { $_->name eq $url } @{ $ref_bug->see_also }) { + $class->SUPER::insert_create_data({ value => $url, + bug_id => $ref_bug->id, + class => ref($class) || $class }); } return $bug_url; } +sub remove_from_db { + my $self = shift; + + my $dbh = Bugzilla->dbh; + my $ref_bug_url = $self->ref_bug_url; + + $dbh->bz_start_transaction(); + + # We remove the current see also first so then we + # avoid infinite loop later. + $self->SUPER::remove_from_db(); + + # We also remove the referenced bug url. + if (defined $ref_bug_url) { + my $ref_bug = Bugzilla::Bug->check($ref_bug_url->bug_id); + my $product = $ref_bug->product_obj; + if (Bugzilla->user->can_edit_product($product->id)) { + $ref_bug_url->remove_from_db(); + } + } + + $dbh->bz_commit_transaction(); +} + sub should_handle { my ($class, $uri) = @_; return $uri->as_string =~ m/^\w+$/ ? 1 : 0; - my $canonical_local = URI->new($class->_local_uri)->canonical; + my $canonical_local = URI->new($class->local_uri)->canonical; # Treating the domain case-insensitively and ignoring http(s):// return ($canonical_local->authority eq $uri->canonical->authority @@ -73,7 +111,7 @@ sub _check_value { # bug id/alias to the local Bugzilla. my $value = $uri->as_string; if ($value =~ m/^\w+$/) { - $uri = new URI($class->local_uri . $value); + $uri = new URI($class->local_uri($value)); } else { # It's not a word, then we have to check # if it's a valid Bugzilla url. @@ -99,7 +137,9 @@ sub _check_value { } sub local_uri { - return correct_urlbase() . "show_bug.cgi?id="; + my ($self, $bug_id) = @_; + $bug_id ||= ''; + return correct_urlbase() . "show_bug.cgi?id=$bug_id"; } 1; diff --git a/Bugzilla/DB/Schema.pm b/Bugzilla/DB/Schema.pm index 2e1b3f78a..106d316a5 100644 --- a/Bugzilla/DB/Schema.pm +++ b/Bugzilla/DB/Schema.pm @@ -499,6 +499,7 @@ use constant ABSTRACT_SCHEMA => { COLUMN => 'bug_id', DELETE => 'CASCADE'}}, value => {TYPE => 'varchar(255)', NOTNULL => 1}, + class => {TYPE => 'varchar(255)', NOTNULL => 1}, ], INDEXES => [ bug_see_also_bug_id_idx => {FIELDS => [qw(bug_id value)], diff --git a/Bugzilla/Install/DB.pm b/Bugzilla/Install/DB.pm index ea1f1de1c..7f32166c9 100644 --- a/Bugzilla/Install/DB.pm +++ b/Bugzilla/Install/DB.pm @@ -28,6 +28,7 @@ use Bugzilla::Install (); use Bugzilla::Install::Util qw(indicate_progress install_string); use Bugzilla::Util; use Bugzilla::Series; +use Bugzilla::BugUrl; use Date::Parse; use Date::Format; @@ -648,6 +649,8 @@ sub update_table_definitions { # 2011-01-29 LpSolit@gmail.com - Bug 616185 _migrate_user_tags(); + _populate_bug_see_also_class(); + ################################################################ # New --TABLE-- changes should go *** A B O V E *** this point # ################################################################ @@ -3542,6 +3545,29 @@ sub _migrate_user_tags { $dbh->bz_drop_column('namedqueries', 'query_type'); } +sub _populate_bug_see_also_class { + my $dbh = Bugzilla->dbh; + + return if $dbh->bz_column_info('bug_see_also', 'class'); + + $dbh->bz_add_column('bug_see_also', 'class', + {TYPE => 'varchar(64)', NOTNULL => 1}, ''); + + my $result = $dbh->selectall_arrayref( + "SELECT id, value FROM bug_see_also"); + + my $update_sth = + $dbh->prepare("UPDATE bug_see_also SET class = ? WHERE id = ?"); + + $dbh->bz_start_transaction(); + foreach my $see_also (@$result) { + my ($id, $value) = @$see_also; + my $class = Bugzilla::BugUrl->class_for($value); + $update_sth->execute($class, $id); + } + $dbh->bz_commit_transaction(); +} + 1; __END__ diff --git a/Bugzilla/Util.pm b/Bugzilla/Util.pm index f9e8d12f7..058a49af3 100644 --- a/Bugzilla/Util.pm +++ b/Bugzilla/Util.pm @@ -55,7 +55,7 @@ use Digest; use Email::Address; use List::Util qw(first); use Math::Random::Secure qw(irand); -use Scalar::Util qw(tainted); +use Scalar::Util qw(tainted blessed); use Template::Filters; use Text::Wrap; @@ -307,25 +307,37 @@ sub use_attachbase { } sub diff_arrays { - my ($old_ref, $new_ref) = @_; + my ($old_ref, $new_ref, $attrib) = @_; my @old = @$old_ref; my @new = @$new_ref; + $attrib ||= 'name'; # For each pair of (old, new) entries: + # If object arrays were passed then an attribute should be defined; # If they're equal, set them to empty. When done, @old contains entries # that were removed; @new contains ones that got added. foreach my $oldv (@old) { foreach my $newv (@new) { - next if ($newv eq ''); - if ($oldv eq $newv) { - $newv = $oldv = ''; + next if ($newv eq '' or $oldv eq ''); + if (blessed($oldv) and blessed($newv)) { + if ($oldv->$attrib eq $newv->$attrib) { + $newv = $oldv = ''; + } + } + else { + if ($oldv eq $newv) { + $newv = $oldv = '' + } } } } - my @removed = grep { $_ ne '' } @old; - my @added = grep { $_ ne '' } @new; + my @removed; + my @added; + @removed = grep { $_ ne '' } @old; + @added = grep { $_ ne '' } @new; + return (\@removed, \@added); } diff --git a/template/en/default/bug/field.html.tmpl b/template/en/default/bug/field.html.tmpl index 82998993f..900a78271 100644 --- a/template/en/default/bug/field.html.tmpl +++ b/template/en/default/bug/field.html.tmpl @@ -163,10 +163,11 @@ cols = 60 defaultcontent = value mandatory = field.is_mandatory %] [% CASE constants.FIELD_TYPE_BUG_URLS %] [% '