Bug 620827: Refactor remove see also to use remove_from_db instead.

r/a=mkanat

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
@@ -62,6 +64,13 @@ use constant SUB_CLASSES => qw(
 );
 
 ###############################
+####      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 %]
        [% '<ul class="bug_urls">' IF value.size %]
-       [% FOREACH url = value %]
+       [% FOREACH bug_url = value %]
          <li>
-           <a href="[% url FILTER html %]">[% url FILTER html %]</a>
-           <label><input type="checkbox" value="[% url FILTER html %]"
+           <a href="[% bug_url.name FILTER html %]">
+             [% bug_url.name FILTER html %]</a>
+           <label><input type="checkbox" value="[% bug_url.name FILTER html %]"
                          name="remove_[% field.name FILTER html %]">
              Remove</label>
          </li>
@@ -213,8 +214,9 @@
     [% END %]
 [% ELSIF field.type == constants.FIELD_TYPE_BUG_URLS %]
   [% '<ul class="bug_urls">' IF value.size %]
-    [% FOREACH url = value %]
-      <li><a href="[% url FILTER html %]">[% url FILTER html %]</a></li>
+    [% FOREACH bug_url = value %]
+      <li><a href="[% bug_url.name FILTER html %]">
+            [% bug_url.name FILTER html %]</a></li>
     [% END %]
   [% '</ul>' IF value.size %]
 [% ELSE %]
diff --git a/xt/lib/Bugzilla/Test/Search.pm b/xt/lib/Bugzilla/Test/Search.pm
index 857af6cb3..3465991f1 100644
--- a/xt/lib/Bugzilla/Test/Search.pm
+++ b/xt/lib/Bugzilla/Test/Search.pm
@@ -655,8 +655,8 @@ sub _create_one_bug {
         $dbh->do('UPDATE bugs SET creation_ts = ?, bug_status = ?,
                   resolution = ? WHERE bug_id = ?',
                  undef, $creation_ts, $status, $resolution, $bug->id);
-        $dbh->do('INSERT INTO bug_see_also (bug_id, value) VALUES (?,?)',
-                 undef, $bug->id, $see_also);
+        $dbh->do('INSERT INTO bug_see_also (bug_id, value, class) VALUES (?,?,?)',
+                 undef, $bug->id, $see_also, 'Bugzilla::BugUrl::Bugzilla');
 
         if ($number == 1) {
             # Bug 1 needs to start off with reporter_accessible and
diff --git a/xt/lib/Bugzilla/Test/Search/FieldTest.pm b/xt/lib/Bugzilla/Test/Search/FieldTest.pm
index 56c0a57d6..e57fd2a59 100644
--- a/xt/lib/Bugzilla/Test/Search/FieldTest.pm
+++ b/xt/lib/Bugzilla/Test/Search/FieldTest.pm
@@ -347,6 +347,9 @@ sub _field_values_for_bug {
     elsif ($field eq 'content') {
         @values = $self->_values_for($number, 'short_desc');
     }
+    elsif ($field eq 'see_also') {
+        @values = $self->_values_for($number, 'see_also', 'name');
+    }
     # Bugzilla::Bug truncates creation_ts, but we need the full value
     # from the database. This has no special value for changedfrom,
     # because it never changes.
@@ -385,7 +388,7 @@ sub _values_for {
         my $bug = $self->bug($number);
         $item = $bug->$bug_field;
     }
-    
+
     if ($item_field) {
         if ($bug_field eq 'flags' and $item_field eq 'name') {
             return (map { $_->name . $_->status } @$item);
@@ -592,4 +595,4 @@ sub _test_content_for_bug {
     }
 }
 
-1;
\ No newline at end of file
+1;