620827 - When deleting see also urls we should be using remove_from_db

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Tiago Mello [:timello]]

Attachment: v1

Now we have the capability to use remove_from_db, lets use it.

Comment 1

[Max Kanat-Alexander]

Comment on attachment 499188 [details] [diff] [review]
v1

Ah, do we really want to create objects just so we can remove them? Maybe see_also should be returning objects so we can just call remove_from_db on them. You could do that pretty easily if you did "use overload" to make the objects stringify to their URLs. (Note that "use overload" can make Perl much slower on RHEL though.)

Comment 2

[Tiago Mello [:timello]]

(In reply to comment #1)
> Comment on attachment 499188 [details] [diff] [review]
> v1
> 
> Ah, do we really want to create objects just so we can remove them? Maybe
> see_also should be returning objects so we can just call remove_from_db on
> them. You could do that pretty easily if you did "use overload" to make the
> objects stringify to their URLs. (Note that "use overload" can make Perl much
> slower on RHEL though.)

I don't like the idea of having Perl much slower on RHEL. Don't we have another possibility? What's the advantage of 'use overload' over the 'new' since we that issue with use overload?

Comment 3

[Max Kanat-Alexander]

The advantage of "use overload" is that you don't have to change all the code that uses see_also to use value.name.

Comment 4

[Tiago Mello [:timello]]

Attachment: v2

Comment 6

[Max Kanat-Alexander]

Comment on attachment 500086 [details] [diff] [review]
v2

>=== modified file 'Bugzilla/Bug.pm'
>+    foreach my $bug_url (@{ $self->{removed_see_also} || [] }) {
>+        eval "use " . ref $bug_url; die $@ if $@;
>+        $bug_url->remove_from_db;

  You don't have to do the "use" here--if you already have an object of that class, then the class is already loaded.

>+    my @removed_see = map { $_->{value} } @{ $self->{removed_see_also} };

  You should be calling accessors like $_->name, not $_->{value}.

  I really don't like having {removed_see_also} and {added_see_also}. I prefer that the accessors themselves be updated, so that calling $bug->see_also will return the current data, even before update() is called.

> sub remove_see_also {
>+    push @{ $self->{removed_see_also} }, $removed_bug_url
>+        if defined $removed_bug_url;

  As I mentioned above, I don't like removed_see_also existing.

>+    if (ref $removed_bug_url eq 'Bugzilla::BugUrl::Bugzilla::Local') {

  Never, never do "ref $something eq". Always do "$something->isa(".

>+        eval "use Bugzilla::BugUrl::Bugzilla::Local"; die $@ if $@;

    If you already have a reference to the object with that class, you don't need to load it.

>+        my $ref_see_also = $removed_bug_url->ref_see_also;
>+
>+        my $ref_bug = Bugzilla::Bug->check($ref_see_also->{bug_id});
>+        my $product = $ref_bug->product_obj;
>+        if (!Bugzilla->user->can_edit_product($product->id)) {
>+            ThrowUserError("product_edit_denied",
>+                        { product => $product->name });
>         }

  This should happen in the remove_from_db method of Bugzilla::BugUrl::Bugzilla::Local, shouldn't it? You could pass the current timestamp to remove_from_db so that the bugs_activity table is correct.

>+        push @{ $self->{removed_see_also} }, $ref_see_also;
>+        push @{ $self->{see_also_changes} }, $ref_see_also->{bug_id};

  That's much too complex. Better to just put the other-bug-changing code into insert_create_data and remove_from_db in Bugzilla::BugUrl::Bugzilla::Local.

>@@ -3236,9 +3254,17 @@
>+    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);
>+        bless $_, $_->{class} foreach @$bug_urls;

  new_from_list in Bugzilla::BugUrl should do the blessing itself. This means overriding new() and _do_list_select in Bugzilla::BugUrl.


>=== modified file 'Bugzilla/BugUrl.pm'
>     id
>     bug_id
>     value
>+    class



>+sub insert_create_data {
>+    my ($class, $field_values) = @_;
>+    $field_values->{class} = $class;

  Instead of being in insert_create_data, this should just have a validator that returns the value.

>=== modified file 'Bugzilla/BugUrl/Bugzilla/Local.pm'
>+sub ref_see_also {

  I think this should be called ref_bug_url.

>+    my $class = shift;

  That should be $self, not $class.

>+    if (!defined $class->{ref_see_also}) {

  You should check exists, instead of defined.

>+        my $ref_bug_id = new URI($class->name)->query_param('id');

  Why doesn't Bugzilla::BugUrl have a $self->uri method that just returns a URI?

>+        my $ref_value = $class->local_uri . $class->{bug_id};

  Maybe local_uri should take a bug_id argument, and then you wouldn't have to do the appending.

>+        $class->{ref_see_also} =
>+            new Bugzilla::BugUrl::Bugzilla::Local({ bug_id => $ref_bug_id,
>+                                                    value => $ref_value });

  That could theoretically be undef, remember. (Just something to keep in mind in other places.)

>@@ -46,7 +59,7 @@
>+    if (!grep { $_->{value} eq $url } @{ $ref_bug->see_also }) {

  $_->name

>=== modified file 'Bugzilla/Install/DB.pm'
>+    $dbh->bz_add_column('bug_see_also', 'class',
>+        {TYPE => 'varchar(64)', NOTNULL => 1, DEFAULT => "''"});
>+    _populate_bug_see_also();

  You should add the "class" column inside of the subroutine (which should be called _populate_bug_see_also_class), and return from the function if the "class" column already exists.

>+sub _populate_bug_see_also {
>+    eval "use Bugzilla::BugUrl"; die $@ if $@;

  That is not necessary, that should be a normal "use" at the top of Install::DB.

>=== modified file 'template/en/default/bug/field.html.tmpl'
>+           <a href="[% bug_url.value FILTER html %]">

  Use the accessor, not the hash element. So it should be bug_url.name.

>+             [% bug_url.value FILTER html %]</a>

  Same there.

>+           <label><input type="checkbox" value="[% bug_url.value FILTER html %]"

  And there.

>+    [% FOREACH bug_url = value %]
>+      <li><a href="[% bug_url.value FILTER html %]">
>+            [% bug_url.value FILTER html %]</a></li>

  And there.

>=== modified file 'xt/lib/Bugzilla/Test/Search.pm'
>-        my $see_also_remove = $bug->see_also;
>+        my @see_also_remove = map { $_->{value} } @{ $bug->see_also };

  $_->name

>         my $see_also_add = [$update_params{see_also}];
>         $update_params{see_also} = { add => $see_also_add, 
>-                                     remove => $see_also_remove };
>+                                     remove => \@see_also_remove };

  You could also make add_see_also and remove_see_also take Bugzilla::BugUrl objects.

>=== modified file 'xt/lib/Bugzilla/Test/Search/FieldTest.pm'
>+    elsif ($field eq 'see_also') {
>+        @values = $self->_values_for($number, 'see_also', 'value');

  name, not value.

>     if ($item_field) {
>         if ($bug_field eq 'flags' and $item_field eq 'name') {
>             return (map { $_->name . $_->status } @$item);
>         }

>+        elsif ($bug_field eq 'see_also' and $item_field eq 'value') {
>+            return (map { $_->{value} } @$item) if ref($item) eq 'ARRAY';
>+            return $item;
>+        }

  You don't need this special code, _get_item below already does this.

Comment 7

[Tiago Mello [:timello]]

Attachment: v3

Comment 8

[Tiago Mello [:timello]]

Attachment: v4

Comment 10

[Max Kanat-Alexander]

Comment on attachment 501229 [details] [diff] [review]
v4

  This looks *very* well designed, thank you! :-)

  There's a few things that need fixing, though:

>=== modified file 'Bugzilla/Bug.pm'
>@@ -2830,9 +2805,9 @@
>+        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 { $_->{value} eq $self_url } @{ $ref_bug->see_also };

  That should be $_->name not $_->{value}.

>     }
> 
>     # We only add the new URI if it hasn't been added yet. URIs are
>@@ -2848,21 +2823,26 @@
>                                                privs    => $privs });
>         }
> 
>-        push @{ $self->{added_see_also} }, $field_values;
>+        push @{ $self->{see_also} }, bless ($field_values, $class);


>=== modified file 'Bugzilla/BugUrl.pm'

>+sub _load_and_bless_bug_url {
>+    my ($class, $bug_url) = @_;
>+    my $subclass = $bug_url->class;
>+    eval "use $subclass"; die $@ if $@;
>+    bless ($bug_url, $subclass);

  This doesn't look like it will actually do anything. You are making a copy of $bug_url and then throwing away the blessed version when you leave the scope of this method. Instead, this should return a blessed object, and you should use "map" in _do_list_select.

  Also, don't you need to call this in new()?

>+sub _check_class {
>+    my ($class, $subclass) = @_;
>+    return $subclass;

  You should check it by attempting to load it and dying if it can't be loaded.

>=== modified file 'Bugzilla/BugUrl/Bugzilla/Local.pm'
> 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 $url = $class->local_uri($field_values->{bug_id});
>     my $bug_url = $class->SUPER::insert_create_data($field_values);

  You should get "my $url" out of $bug_url instead of out of $field_values. (That is, use the created object instead of the passed-in values.)

>+    if (!grep { $_->name eq $url } @{ $ref_bug->see_also }) {
>+        $class->SUPER::insert_create_data({ value  => $url,
>+                                            bug_id => $ref_bug->id,
>+                                            class  => ref $class });

  Ah, "ref $class" does not look like what you want, there. I think you just want "$class".

>+    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)) {
>+            ThrowUserError("product_edit_denied",
>+                        { product => $product->name });
>+        }
>+        $ref_bug_url->remove_from_db;

  Hmm, won't that create an infinite loop?

> sub local_uri {
>-    return correct_urlbase() . "show_bug.cgi?id=";
>+    my ($self, $bug_id) = @_;
>+    return correct_urlbase() . "show_bug.cgi?id=$bug_id";

  Ah, I just noticed a bug--should_handle is calling _local_uri, which doesn't exist. Also, this method here will need to accommodate the fact that $bug_uri could be undef. ($bug_uri ||= '')

>=== modified file 'Bugzilla/DB/Schema.pm'
>+            class  => {TYPE => 'varchar(64)', NOTNULL => 1},

    Might as well make it 255 characters, no? Extension class names could be very long.

>=== modified file 'Bugzilla/Install/DB.pm'
>+sub _populate_bug_see_also_class {
>+    my $dbh = Bugzilla->dbh;
>+
>+    return $dbh->bz_column_info('bug_see_also', 'class');

  That should be "return if"

>+    $dbh->bz_add_column('bug_see_also', 'class',
>+        {TYPE => 'varchar(64)', NOTNULL => 1, DEFAULT => "''"});

  It doesn't need a default--instead you just need to specify the $init_value argument as an empty string. (See the implementation of bz_add_column.)

>=== modified file 'Bugzilla/Util.pm'
>         foreach my $newv (@new) {
>-            next if ($newv eq '');
>-            if ($oldv eq $newv) {
>-                $newv = $oldv = '';
>+            if ($attrib) {
>+                if ($oldv->$attrib eq $newv->$attrib) {
>+                    $newv = $oldv = undef;
>+                }

  Why not just change this to always using undef and never using ''?

Comment 11

[Tiago Mello [:timello]]

(In reply to comment #10)
> 
> >=== modified file 'Bugzilla/BugUrl/Bugzilla/Local.pm'
> > sub insert_create_data {
> >+    if (!grep { $_->name eq $url } @{ $ref_bug->see_also }) {
> >+        $class->SUPER::insert_create_data({ value  => $url,
> >+                                            bug_id => $ref_bug->id,
> >+                                            class  => ref $class });
> 
>   Ah, "ref $class" does not look like what you want, there. I think you just
> want "$class".

Well, $class is a blessed object at this point, I can't pass it, otherwise the SUPER::insert_create_data will get an object instead of a string.

Comment 12

[Tiago Mello [:timello]]

(In reply to comment #10)
> 
> >=== modified file 'Bugzilla/BugUrl/Bugzilla/Local.pm'
> > 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 $url = $class->local_uri($field_values->{bug_id});
> >     my $bug_url = $class->SUPER::insert_create_data($field_values);
> 
>   You should get "my $url" out of $bug_url instead of out of $field_values.
> (That is, use the created object instead of the passed-in values.)

That's not true... I can't get the $url from the $bug_url because it's not the one I want... in order to insert to url in the referenced bug, I want to build the url with the local uri plus the current bug id. But I can use $bug_url->bug_id instead of $field_values->{bug_id}

Comment 13

[Max Kanat-Alexander]

(In reply to comment #11)
> Well, $class is a blessed object at this point, I can't pass it, otherwise the
> SUPER::insert_create_data will get an object instead of a string.

  Ah, normally insert_create_data is not called on a blessed object. Perhaps you should do "ref($class) || $class".

(In reply to comment #12)
> But I can use $bug_url->bug_id instead of $field_values->{bug_id}

  Oh right, yeah, do that. :-)

Comment 14

[Tiago Mello [:timello]]

(In reply to comment #10)

> >=== modified file 'Bugzilla/BugUrl/Bugzilla/Local.pm'
> 
> >+    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)) {
> >+            ThrowUserError("product_edit_denied",
> >+                        { product => $product->name });
> >+        }
> >+        $ref_bug_url->remove_from_db;
> 
>   Hmm, won't that create an infinite loop?

Nope, because we remove the see also for the current bug first, then the $ref_bug_url won't have any $ref_bug_url->ref_bug_url... but I think we may add a comment explaining that.

Comment 15

[Tiago Mello [:timello]]

Attachment: v5

Note that with this patch the xt tests will fail because we need to fix the FieldTest.pm. I did not change anything since you (mkanat) said will fix that later.

Just refresh your mind: http://pastie.org/private/bitwbjbsakffo4q7so0gq

Comment 16

[Max Kanat-Alexander]

Comment on attachment 506419 [details] [diff] [review]
v5

>=== modified file 'Bugzilla/Bug.pm'
> sub add_see_also {
>     my ($self, $input) = @_;
>+
>+    # This is needed by xt/search.t.
>+    if (blessed $input) {
>+        push @{ $self->{see_also} }, $input
>+            if !grep { $_->name eq $input->name } @{ $self->see_also };
>+        return;
>+    }

  That bypasses the magic of ::Local. You should probably do what you do in remove_see_also, instead.

>+    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;

  Ah, I'm pretty sure that $removed_bug_url is not an arrayref at that point.

>+    if ($removed_bug_url->isa('Bugzilla::BugUrl::Bugzilla::Local')
>+        and defined $removed_bug_url->ref_bug_url) {

  Nit: { goes on the next line, aligned with "i" in "if".

>+        push @{ $self->{see_also_changes} },
>+            $removed_bug_url->ref_bug_url->bug_id;

  Nit on spacing: the $ should be aligned with the @ above, there.

>-    my $can = $self->check_can_change_field('see_also', $see_also, \@new_see_also, \$privs);
>+    my $can = $self->check_can_change_field('see_also', $url, '', \$privs);

  Since you're fixing diff_arrays, I don't think you need to change this line.

>=== modified file 'Bugzilla/BugUrl.pm'
>+    my $objects = $class->SUPER::_do_list_select(@_);
>+    map {
>+        eval "use " . $_->class; die $@ if $@;
>+        bless $_, $_->class;
>+    } @$objects;

  map returns a value--if you're going to use it, you need to do something with the return value. Otherwise you should be using foreach.

>+sub _check_class {
>+    my ($class, $subclass) = @_;
>+    #eval "use $subclass"; die $@ if $@;
>+    return $subclass;

  Why is that commented out?

>=== modified file 'Bugzilla/BugUrl/Bugzilla/Local.pm'
>+sub remove_from_db {
>+    my $class = shift;

  Ah, I think you mean $self.

>+    # We remove the current see also first so then we
>+    # avoid infinite loop later.
>+    $class->SUPER::remove_from_db;

  That's an action, so it should be:

  $self->SUPER::remove_from_db();

  (with the parens)

  You need to start a transaction before this. Otherwise the below block of code could throw an error and then the database would be in an inconsistent state.

>+    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)) {
>+            ThrowUserError("product_edit_denied",
>+                        { product => $product->name });
>+        }

  Are you sure you want to prevent somebody from editing this bug just because they can't edit the other bug?

  I'm not entirely certain what to do, there.

>+        $ref_bug_url->remove_from_db;

  Should have parens on it.

>=== modified file 'Bugzilla/Install/DB.pm'
>+sub _populate_bug_see_also_class {
>+    my $result = $dbh->selectall_arrayref(
>+        "SELECT id, value FROM bug_see_also WHERE class = ''");

  They'll all have class = '', no need for the WHERE clause.

>+    foreach my $see_also (@$result) {
>+        my ($id, $value) = @$see_also;
>+        my $class = Bugzilla::BugUrl->class_for($value);
>+        $dbh->do("UPDATE bug_see_also SET class = ? WHERE id = ?", undef,
>+                 $class, $id);

  You should prepare this as an $sth outside the loop and then use $sth->execute inside of the loop.

>=== modified file 'Bugzilla/Util.pm'
> sub diff_arrays {
>+            next if (!$newv or !$oldv);

  That ignores values if they are 0.

Comment 17

[Max Kanat-Alexander]

(In reply to comment #16)
> Ah, I'm pretty sure that $removed_bug_url is not an arrayref at that point.

  Oh, nevermind, I see that it is. That's an odd syntax, though. I'd do something more like:

  $removed_bug_url = $removed_bug_url->[0];

Comment 18

[Tiago Mello [:timello]]

Attachment: v6

Comment 19

[Max Kanat-Alexander]

Comment on attachment 508183 [details] [diff] [review]
v6

  Beautiful! One thing to fix on checkin:

>+sub _populate_bug_see_also_class {
>+    
>+    foreach my $see_also (@$result) {
>+        my ($id, $value) = @$see_also;
>+        my $class = Bugzilla::BugUrl->class_for($value);
>+        $update_sth->execute($class, $id);
>+    }


  That should be wrapped in a transaction.

Comment 20

[Tiago Mello [:timello]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/                                                                                 
modified Bugzilla/Bug.pm
modified Bugzilla/BugUrl.pm
modified Bugzilla/Util.pm
modified Bugzilla/BugUrl/Bugzilla/Local.pm
modified Bugzilla/DB/Schema.pm
modified Bugzilla/Install/DB.pm
modified template/en/default/bug/field.html.tmpl
modified xt/lib/Bugzilla/Test/Search.pm
modified xt/lib/Bugzilla/Test/Search/FieldTest.pm
Committed revision 7699.

Comment 21

[Frédéric Buclin]

Comment on attachment 508183 [details] [diff] [review]
v6

>=== modified file 'Bugzilla/DB/Schema.pm'

>+            class  => {TYPE => 'varchar(255)', NOTNULL => 1},



>=== modified file 'Bugzilla/Install/DB.pm'

>+    $dbh->bz_add_column('bug_see_also', 'class',
>+        {TYPE => 'varchar(64)', NOTNULL => 1}, '');


varchar(255) vs varchar(64)!! Please fix that asap.

Comment 22

[Frédéric Buclin]

Reopening, see my previous comment!

Comment 23

[Tiago Mello [:timello]]

Attachment: v6-1

Comment 24

[David Lawrence [:dkl]]

Comment on attachment 511693 [details] [diff] [review]
v6-1

Looks good to me. r=dkl

Comment 25

[Frédéric Buclin]

Comment on attachment 511693 [details] [diff] [review]
v6-1

>     return if $dbh->bz_column_info('bug_see_also', 'class');
> 
>     $dbh->bz_add_column('bug_see_also', 'class',
>-        {TYPE => 'varchar(64)', NOTNULL => 1}, '');
>+        {TYPE => 'varchar(255)', NOTNULL => 1}, '');

This is not enough. Installations which already upgraded will still have varchar(64).

Comment 26

[Tiago Mello [:timello]]

Attachment: v6-2

Comment 27

[Frédéric Buclin]

Comment on attachment 511738 [details] [diff] [review]
v6-2

Please add a comment explaining why we call bz_alter_column() here, something like # The length was incorrectly set to 64 instead of 255. r=LpSolit with this comment added.

Comment 28

[Tiago Mello [:timello]]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/                                                                                                           
modified Bugzilla/Install/DB.pm
Committed revision 7700.