Right now, add_see_also has a bunch of code in an if/else tree, that determines which types of URLs are valid. Instead, we should have a class heirarchy like this: Bugzilla::BugUrl Bugzilla::BugUrl::Bugzilla Bugzilla::BugUrl::Bugzilla::Local Bugzilla::BugUrl::Launchpad Bugzilla::BugUrl::Google Bugzilla::BugUrl would currently implement two methods: should_handle This takes a string and returns 1 or 0. It returns 1 if this particular module should be the one to parse the input string, and 0 otherwise. (So, we go through all of the Bugzilla::BugUrl subclasses we have available, and we pick the first one that returns 1.) Bugzilla::Object create/run_create_validators/etc. Bugzilla::Bug::add_see_also can call Bugzilla::BugUrl::${class}::check_required_create_fields and Bugzilla::BugUrl::${class}::run_create_validators, and that way they will validate the url. (this is the same as what the current code in the if/else in add_see_also does) Then, Bugzilla::Bug::update can call Bugzilla::BugUrl::${class}::insert_create_data to insert the stored BugUrl objects. (Much the same as how we update the bug_see_also table now.) Of course, as part of this, $bug->see_also will have to start returning BugUrl objects. We may also have to start storing which subclass is being used for each URL in the see_also table. This will allow the See Also system to be easily extendable, and we can add a hook to extend it, in a separate bug.

(In reply to comment #0) > Right now, add_see_also has a bunch of code in an if/else tree, that determines > which types of URLs are valid. Instead, we should have a class heirarchy like > this: > > Bugzilla::BugUrl > Bugzilla::BugUrl::Bugzilla > Bugzilla::BugUrl::Bugzilla::Local > Bugzilla::BugUrl::Launchpad > Bugzilla::BugUrl::Google > I was thinking, instead of having this hardcoded.... we could allow the admin to set what are the allowed external url by saving the regex for it... we can populate it with the options we have today... then I think only the module Bugzilla::BugUrl would handle that.

(In reply to comment #1) > I was thinking, instead of having this hardcoded.... we could allow the admin > to set what are the allowed external url by saving the regex for it... we can > populate it with the options we have today... then I think only the module > Bugzilla::BugUrl would handle that. No, that's too complex for the admin--we want to ship with a system that simply works. Also, eventually these modules are going to know, individually, how to get information from the remote bug tracker (like the status and summary of remote bugs); so they have to be modules, not just a configuration parameter.

This is really the v1, but it should be working as it currently works.

Comment on attachment 481746 [details] [diff] [review] v1, without docs This is really cool! Totally like what I was thinking. :-) A few comments, though: >=== modified file 'Bugzilla/Bug.pm' >--- Bugzilla/Bug.pm 2010-10-01 11:27:21 +0000 >+++ Bugzilla/Bug.pm 2010-10-08 03:02:25 +0000 >@@ -48,6 +48,7 @@ > use Bugzilla::Group; > use Bugzilla::Status; > use Bugzilla::Comment; >+use Bugzilla::BugUrl; > > use List::MoreUtils qw(firstidx uniq); > use List::Util qw(min max first); >@@ -944,6 +945,37 @@ > } > > # See Also >+ foreach my $url_class (@{ $self->{_bug_url_classes} }) { bug_url_classes needs to be deleted after you're done with it. >+ $class->check_required_create_fields($params); >+ my $field_values = $class->run_create_validators($params); You need to be running those during add_see_also, not during update(). update() must never throw errors. >+ $ref_bug->add_see_also($self->id); And that needs to happen in add_see_also, just like it used to, not here in update(). >+ my $class = Bugzilla::BugUrl->should_handle($input); Instead of that, let's have a Bugzilla::BugUrl->class_for() that calls should_handle in each subclass. Then you should be doing check_required_create_fields and run_create_validators on the input using the class. >=== added directory 'Bugzilla/BugUrl' >+# The Original Code is the Bugzilla Bug Tracking System. You're missing an Original Developer block here. >+package Bugzilla::BugUrl; >+ >+use strict; >+ >+use base qw(Bugzilla::Object); Nit: No spaces necessary between those lines. >+sub should_handle { This should be called class_for, and the subclass methods should be called should_handle. This class should have a should_handle method that just dies, to let authors know that they have to implement it. I think there might even be a CodeError for that already. >+sub run_create_validators { >+ my ($class, $params) = @_; >+ my $url = $params->{value}; >+ $params->{value} = $class->_validate_url($url); You should just be specifying _validate_url as the validator for {value}. You can just specify the string '_check_value' as the VALIDATOR value, then it will just look for any method named that on the particular class, instead of explicitly calling this \&_validate_url. >+sub _validate_url { This should be called _check_value, since it's a validator. Your subclasses can call it with SUPER, as a normal override. >+ my ($class, $input) = @_; >+ >+ if (!$input) { >+ ThrowCodeError('param_required', >+ { function => 'add_see_also', param => '$input' }); >+ } >+ >+ # If a bug id/alias has been taken, then treat it >+ # as a link to the local Bugzilla. >+ my $local_bug_uri = correct_urlbase() . "show_bug.cgi?id="; >+ if ($input =~ m/^\w+$/) { >+ $input = $local_bug_uri . $input; >+ } How about you just have this _check_value return the input immediately if it's just a single word, and let Bugzilla::Local override _check_value and set the input to be a local bug URI. That way extensions can modify the handling of local bug URLs if they want to. >=== added file 'Bugzilla/BugUrl/Bugzilla/Local.pm' >+sub run_create_validators { >+ my ($class, $params) = @_; >+ >+ my $validators = $class->_get_validators; You shouldn't be calling _get_validators manually. >+ my $self_bug_id = $field_values->{bug_id}; >+ my $url = $field_values->{value}; >+ my $ref_bug_id = URI->new($url)->query_param('id'); >+ >+ if ($ref_bug_id == $self_bug_id) { >+ ThrowUserError('see_also_self_reference'); >+ } This should all just be in _check_bug_id or _check_value, using VALIDATOR_DEPENDENCIES. >+sub _should_handle { >+ my ($class, $uri) = @_; >+ return ($canonical_local->authority eq $uri->canonical->authority >+ and $canonical_local->path eq $uri->canonical->path); Add a "? 1 : 0" to the end of that--it's always nice for boolean things to explicitly return 1 or 0. >+sub _check_url { >+ my ($class, $uri) = @_; >+ >+ $class->SUPER::_check_url($uri); >+ >+ my $bug_id = $uri->query_param('id'); You're assuming that $uri is an object at this point. If you want that, you should probably return a URI object from the superclass _check_value. Also, you need to explicitly use URI::QueryParam somewhere in order for the query_param method to be available.

small adjustments.

Comment on attachment 482183 [details] [diff] [review] v3, without docs. >=== modified file 'Bugzilla/Bug.pm' >+ foreach my $url (keys %{ $self->{bug_url_classes} }) { bug_url_classes should be a private accessor instead of a hash item, if you're going to be using it like this. Or it should be called {added_see_also}, if it's just going to be a list of stuff to add. >+ my $class = $self->{bug_url_classes}->{$url}; >+ my $params = { value => $url, bug_id => $self->id }; I think you need to be pushing $params into some array, instead, like {added_see_also}, during add_see_also. Perhaps you could just add a "class" parameter to $params that you can delete here. >+ next if $class->new($params); That should have been handled by add_see_also, so you can remove that line. >@@ -952,10 +965,7 @@ > . $dbh->sql_in('value', [('?') x @$removed_see]), > undef, $self->id, @$removed_see); > } For deleting, you should be using remove_from_db on existing BugUrl objects, I think. However, if you want to wait for another bug to do that, that's fine--just file the bug. >@@ -963,7 +973,7 @@ > } > > # Call update for the referenced bugs. >- $_->update() foreach @{ $self->{see_also_update} || [] }; >+ $_->update() foreach values %{ $self->{see_also_update} }; Why is see_also_update a hashref now instead of an array? It looks like in most places, you access it like an array. Also, you need a || {} there, or you're going to get crashes. >@@ -2805,171 +2815,42 @@ >+ my $class = Bugzilla::BugUrl->class_for($input) >+ or ThrowUserError('bug_url_invalid', { url => $input, >+ reason => 'show_bug' }); Hmm, how about we just have class_for throw that error right now? This is the only place we need class_for, it might as well throw the error itself. >+ my $params = { value => $input, bug_id => $self->id }; You should just pass $self for bug_id, and let the validator also accept bug objects. (See how Bugzilla::Comment does this now.) >+ my $uri = URI->new($value); >+ my $ref_id = $uri->query_param('id'); >+ my $ref_bug = Bugzilla::Bug->check($ref_id); Bugzilla::BugUrl::Bugzilla::Local should have already done that check. >+ my $product = $ref_bug->product_obj; >+ if (!Bugzilla->user->can_edit_product($product->id)) { >+ ThrowUserError("product_edit_denied", >+ { product => $product->name }); >+ } And that check, as well. >+ $ref_bug->add_see_also($self->id, 'skip recursion'); Would there be any advantage to allowing class_for to take a Bugzilla::Bug object (so we could just pass $self here)? Then we wouldn't have to validate it, we'd just have to call check_visible on it (or whatever that method is called). >+ $self->{see_also_update}->{$ref_id} = $ref_bug; Yeah, I think that this can still be an array. >+ my $privs; >+ my $can = $self->check_can_change_field('see_also', '', $value, \$privs); >+ if (!$can) { >+ ThrowUserError('illegal_change', { field => 'see_also', >+ newvalue => $value, >+ privs => $privs }); >+ } You didn't actually check if we're changing see_also. If no change was made, we don't need to check this. >+ $self->{bug_url_classes}->{$value} = $class; Yeah, just push $params into an array here, with "class" as an element of $params. >=== added directory 'Bugzilla/BugUrl' >+use constant VALIDATORS => { >+ value => '_check_value', >+ bug_id => '_check_bug_id', >+}; Add a comment here explaining why these are strings and not coderefs. >+use constant SUB_CLASSES => qw( Add a comment here explaining that the order is important and why. >+sub new { >+ my $class = shift; >+ my $param = shift; >+ >+ if (ref $param) { >+ my $bug_id = $param->{bug_id}; >+sub should_handle { >+ my ($class, $input) = @_; >+ ThrowCodeError('unknown_method', { method => 'should_handle' }); You should include $class in the "method" value. >+sub class_for { >+ my ($class, $value) = @_; >+ >+ foreach my $subclass ($class->SUB_CLASSES) { >+ eval "require $subclass"; If you're going to do a string eval, might as well be "use". >+sub _check_bug_id { >+ my ($class, $bug_id) = @_; >+ return $bug_id; I think you need to at least call Bugzilla::Bug->check if it's not a Bugzilla::Bug object. Maybe some other validation, like that the user can edit the bug, and that they can change the see_also field? You could move most of that validation out to here. (Like how Bugzilla::Comment does it now.) >+ # If a number/word has been taken, then we let s/taken/passed/ >+ # Bugzilla::Local decided whether or not it's a s/decided/decide/ >+ # link to the local Bugzilla. >+ return $value if $value =~ m/^\w+$/; That doesn't make sense--this should only be in the _check_value validator for Local, since should_handle will already have sent us there if we're just \w+. >+++ Bugzilla/BugUrl/Bugzilla/Local.pm 2010-10-11 05:37:49 +0000 Is it only this class that requires VALIDATOR_DEPENDENCIES? If so, it should only be here. >+sub _check_value { >+ my ($class, $value, undef, $params) = @_; >+ >+ my $local_bug_uri = correct_urlbase() . "show_bug.cgi?id="; >+ # At this point we are going to treat the word as a s/the/any/ >+ if ($value =~ m/^\w+$/) { >+ $value = $local_bug_uri . $value; >+ } That block is where you need to make sure that $value is a valid Bug that can be both accessed and edited by the current user. >+ my $value = $class->SUPER::_check_value($value); >+ my $uri = URI->new($value); SUPER::_check_value returns a URI object already. >+ my $self_bug_id = $params->{bug_id}; >+ my $ref_bug_id = $uri->query_param('id'); query_param('id') might be an alias, so we'll want to use the Bugzilla::Bug object here that we'll get from check() above, instead. Also, we need to resolve the alias to an id. >=== added file 'Bugzilla/BugUrl/Debian.pm' >+sub _check_url { Should be _check_value. Everything else looks great! This is a really awesome patch, such a great refactoring!! I'm really excited about it!! :-)

(In reply to comment #7) > Comment on attachment 482183 [details] [diff] [review] > v3, without docs. > > >=== modified file 'Bugzilla/Bug.pm' > >+ my $uri = URI->new($value); > >+ my $ref_id = $uri->query_param('id'); > >+ my $ref_bug = Bugzilla::Bug->check($ref_id); > > Bugzilla::BugUrl::Bugzilla::Local should have already done that check. Well, I need a ref_bug object anyway, so I thought it wouldn't hurt call ->check again.

(In reply to comment #8) > > Bugzilla::BugUrl::Bugzilla::Local should have already done that check. > > Well, I need a ref_bug object anyway, so I thought it wouldn't hurt call > ->check again. Okay. Maybe you could return a bug object in the validator, or something? Or store it in $params or as an accessor in ::Local? I'm not sure though--I'm not looking at the code right now.

(In reply to comment #7) > Comment on attachment 482183 [details] [diff] [review] > v3, without docs. > > >=== modified file 'Bugzilla/Bug.pm' > > # Call update for the referenced bugs. > >- $_->update() foreach @{ $self->{see_also_update} || [] }; > >+ $_->update() foreach values %{ $self->{see_also_update} }; > > Why is see_also_update a hashref now instead of an array? It looks like in > most places, you access it like an array. This is because I'm not checking if the local bug has been added already, if it has, then I can guarantee it will not be added again, but maybe that is not the best solution or it needs a comment there.

(In reply to comment #10) > (In reply to comment #7) > > Comment on attachment 482183 [details] [diff] [review] [details] > > v3, without docs. > > > > >=== modified file 'Bugzilla/Bug.pm' > > > # Call update for the referenced bugs. > > >- $_->update() foreach @{ $self->{see_also_update} || [] }; > > >+ $_->update() foreach values %{ $self->{see_also_update} }; > > > > Why is see_also_update a hashref now instead of an array? It looks like in > > most places, you access it like an array. > > This is because I'm not checking if the local bug has been added already, if it > has, then I can guarantee it will not be added again, but maybe that is not the > best solution or it needs a comment there. Actually, that avoids a grep in the array... maybe it not worths...

(In reply to comment #7) > Comment on attachment 482183 [details] [diff] [review] > v3, without docs. > > >=== modified file 'Bugzilla/Bug.pm' > >@@ -952,10 +965,7 @@ > > . $dbh->sql_in('value', [('?') x @$removed_see]), > > undef, $self->id, @$removed_see); > > } > > For deleting, you should be using remove_from_db on existing BugUrl objects, > I think. However, if you want to wait for another bug to do that, that's > fine--just file the bug. Soooo, lets file another bug! :D

(In reply to comment #7) > Comment on attachment 482183 [details] [diff] [review] > v3, without docs. > >+++ Bugzilla/BugUrl/Bugzilla/Local.pm 2010-10-11 05:37:49 +0000 > > >+sub _check_value { > >+ my ($class, $value, undef, $params) = @_; > > >+ my $value = $class->SUPER::_check_value($value); > >+ my $uri = URI->new($value); > > SUPER::_check_value returns a URI object already. It does not. SUPER is Bugzilla::BugUrl::Bugzilla and not Bugzilla::BugUrl

Comment on attachment 492288 [details] [diff] [review] v4 >+ # Check if the ref bug has already the url. >+ if (!grep { $uri->canonical } @{ $ref_bug->see_also }) { You need to do" $_ eq $uri->canonical" there. Otherwise you're using the URI as a regex! :-) >=== added file 'Bugzilla/BugUrl.pm' >+# This must be strings with the names of >+# the validators instead of coderefs. The >+# functions will be called by the sub classes. This must be strings with the names of the validations, instead of coderefs, because subclasses override these validators with their own. >+sub should_handle { >+ my ($class, $input) = @_; >+ ThrowCodeError('unknown_method', >+ { method => "${class}::should_handle" }); >+} Add a comment saying that this is an abstract method that must be overridden in every subclass. >+sub class_for { >+ my ($class, $value) = @_; >+ >+ foreach my $subclass ($class->SUB_CLASSES) { >+ eval "use $subclass"; Add "die $@ if $@;" after, in case there's a compilation error. >+sub _check_value { >+ # If a number/word has been passed, then we let >+ # Bugzilla::Local decide whether or not it's a >+ # link to the local Bugzilla. >+ return $value if $value =~ m/^\w+$/; Instead of this, how about you just wait until the input is a valid URI, before you call SUPER::_check_value? >=== added file 'Bugzilla/BugUrl/Bugzilla/Local.pm' >+sub _check_value { >+ my ($class, $value, undef, $params) = @_; >+ >+ my $local_bug_uri = correct_urlbase() . "show_bug.cgi?id="; Since you need that in a few places, you could make it $class->local_uri. >+ # At this point we are going to treat any word as a >+ # bug id/alias to the local Bugzilla. >+ if ($value =~ m/^\w+$/) { >+ $value = $local_bug_uri . $value; >+ } You haven't checked if $value is defined or not, yet. So you probably need to check that first. >+ $value = $class->SUPER::_check_value($value); >+ my $uri = URI->new($value); Nit: Extra spaces after $uri. Why not just always have _check_value return a URI object? URI objects stringify to their URLs, so they're actually not really different than strings, and then you wouldn't have to keep calling URI->new. (Or even better, you could have should_handle return a URI object, if you wanted.) >+ my $ref_bug_id = $uri->query_param('id'); >+ my $ref_bug = Bugzilla::Bug->check($ref_bug_id); >+ my $self_bug_id = $params->{bug_id}; I think you should do: $params->{ref_bug} = $ref_bug; here, so that you don't have to create a bug object again later. Oh, also, I just had a pretty good idea! I think you should override insert_create_data in ::Local, and it can do the updating of the ref_bug. That way you don't even have to worry about it in Bugzilla::Bug->update. >+ my $product = $ref_bug->product_obj; >+ if (!Bugzilla->user->can_edit_product($product->id)) { I think you can just call $ref_bug->product_id.

Comment on attachment 497983 [details] [diff] [review] v5 Okay, this looks great. One tiny change I'd like to see made: Instead of having class_for return a hashref, it should just: return wantarray ? ($class, $uri) : $class; And it should return an empty array if it fails.

This is a great patch, thanks so much for your work!

There are small changes other than what you asked for. The significant change for this patch is that now it's checking if the self bug url isn't already in the ref bug, then we avoid sending email for the ref bugs.

Comment on attachment 498276 [details] [diff] [review] v6 Looks fine. :-)

Committing to: bzr+ssh://timello%40gmail.com@bzr.mozilla.org/bugzilla/trunk/ modified Bugzilla/Bug.pm added Bugzilla/BugUrl added Bugzilla/BugUrl.pm added Bugzilla/BugUrl/Bugzilla added Bugzilla/BugUrl/Bugzilla.pm added Bugzilla/BugUrl/Debian.pm added Bugzilla/BugUrl/Google.pm added Bugzilla/BugUrl/Launchpad.pm added Bugzilla/BugUrl/Bugzilla/Local.pm modified Bugzilla/Install/DB.pm Committed revision 7629.

Ok. This should fix the problem with bugs activity: timello@tnt ~/www/bugzilla/trunk$ BZ_WRITE_TESTS=1 prove -f xt/ :: --add-custom-fields xt/search.t .. ok All tests successful. Files=1, Tests=71643, 62 wallclock secs (13.81 usr 1.09 sys + 34.55 cusr 2.01 csys = 51.46 CPU) Result: PASS

Comment on attachment 498938 [details] [diff] [review] v6-1 >=== modified file 'Bugzilla/Bug.pm' > # See Also >+ my @old_see_also = @{ $old_bug->see_also || [] }; Ah, you shouldn't need the [] -- see_also should return an empty array when there are no values in the database. You can fix that on checkin.

(In reply to comment #23) > Ah, you shouldn't need the [] -- see_also should return an empty array when > there are no values in the database. In that case, I don't understand how this patch fixes the problem. You pass an empty arrayref in both cases.

(In reply to comment #24) > (In reply to comment #23) > > Ah, you shouldn't need the [] -- see_also should return an empty array when > > there are no values in the database. > > In that case, I don't understand how this patch fixes the problem. You pass an > empty arrayref in both cases. The problem is that, we call $class->insert_create_data($field_values) for each new url and then both $old_bug->see_also and $self->see_also will be equal for added values.

Committing to: bzr+ssh://timello%40gmail.com@bzr.mozilla.org/bugzilla/trunk/ modified Bugzilla/Bug.pm Committed revision 7630.