From ed3e015a1c21061ed9f30cfc3fe3c3e83c0d2fb1 Mon Sep 17 00:00:00 2001 From: Tiago Mello Date: Mon, 20 Dec 2010 20:49:10 -0200 Subject: Bug 593539: Refactor See Also to use separate modules for each type of URL r/a=mkanat --- Bugzilla/Bug.pm | 192 ++++++++++---------------------------------------------- 1 file changed, 32 insertions(+), 160 deletions(-) (limited to 'Bugzilla/Bug.pm') diff --git a/Bugzilla/Bug.pm b/Bugzilla/Bug.pm index 9d41d5c7e..fd0111578 100644 --- a/Bugzilla/Bug.pm +++ b/Bugzilla/Bug.pm @@ -49,6 +49,7 @@ use Bugzilla::Component; 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); @@ -930,6 +931,14 @@ sub update { } # 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_bug->see_also, $self->see_also); @@ -938,19 +947,13 @@ sub update { . $dbh->sql_in('value', [('?') x @$removed_see]), undef, $self->id, @$removed_see); } - foreach my $url (@$added_see) { - $dbh->do('INSERT INTO bug_see_also (bug_id, value) VALUES (?,?)', - undef, $self->id, $url); - } + # 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)]; } - # Call update for the referenced bugs. - $_->update() foreach @{ $self->{see_also_update} || [] }; - # Log bugs_activity items # XXX Eventually, when bugs_activity is able to track the dupe_id, # this code should go below the duplicates-table-updating code below. @@ -1193,9 +1196,9 @@ sub send_changes { } # Sending emails for the referenced bugs. - foreach my $ref_bug (@{ $self->{see_also_update} || [] }) { + foreach my $ref_bug_id (uniq @{ $self->{see_also_changes} || [] }) { _send_bugmail({ forced => { changer => $user }, - id => $ref_bug->id }, $vars); + id => $ref_bug_id }, $vars); } } @@ -2787,173 +2790,42 @@ sub remove_group { } sub add_see_also { - my ($self, $input, $skip_recursion) = @_; + my ($self, $input) = @_; $input = trim($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; - } - - # We assume that the URL is an HTTP URL if there is no (something):// - # in front. - my $uri = new URI($input); - if (!$uri->scheme) { - # This works better than setting $uri->scheme('http'), because - # that creates URLs like "http:domain.com" and doesn't properly - # differentiate the path from the domain. - $uri = new URI("http://$input"); - } - elsif ($uri->scheme ne 'http' && $uri->scheme ne 'https') { - ThrowUserError('bug_url_invalid', { url => $input, reason => 'http' }); - } - - # This stops the following edge cases from being accepted: - # * show_bug.cgi?id=1 - # * /show_bug.cgi?id=1 - # * http:///show_bug.cgi?id=1 - if (!$uri->authority or $uri->path !~ m{/}) { - ThrowUserError('bug_url_invalid', - { url => $input, reason => 'path_only' }); - } - - my $result; - # Launchpad URLs - if ($uri->authority =~ /launchpad.net$/) { - # Launchpad bug URLs can look like various things: - # https://bugs.launchpad.net/ubuntu/+bug/1234 - # https://launchpad.net/bugs/1234 - # All variations end with either "/bugs/1234" or "/+bug/1234" - if ($uri->path =~ m|bugs?/(\d+)$|) { - # This is the shortest standard URL form for Launchpad bugs, - # and so we reduce all URLs to this. - $result = "https://launchpad.net/bugs/$1"; - } - else { - ThrowUserError('bug_url_invalid', - { url => $input, reason => 'id' }); - } - } - # Google Code URLs - elsif ($uri->authority =~ /^code.google.com$/i) { - # Google Code URLs only have one form: - # http(s)://code.google.com/p/PROJECT_NAME/issues/detail?id=1234 - my $project_name; - if ($uri->path =~ m|^/p/([^/]+)/issues/detail$|) { - $project_name = $1; - } else { - ThrowUserError('bug_url_invalid', - { url => $input }); - } - my $bug_id = $uri->query_param('id'); - detaint_natural($bug_id); - if (!$bug_id) { - ThrowUserError('bug_url_invalid', - { url => $input, reason => 'id' }); - } - # While Google Code URLs can be either HTTP or HTTPS, - # always go with the HTTP scheme, as that's the default. - $result = "http://code.google.com/p/" . $project_name . - "/issues/detail?id=" . $bug_id; - } - # Debian BTS URLs - elsif ($uri->authority =~ /^bugs.debian.org$/i) { - # Debian BTS URLs can look like various things: - # http://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1234 - # http://bugs.debian.org/1234 - my $bug_id; - if ($uri->path =~ m|^/(\d+)$|) { - $bug_id = $1; - } - elsif ($uri->path =~ /bugreport\.cgi$/) { - $bug_id = $uri->query_param('bug'); - detaint_natural($bug_id); - } - if (!$bug_id) { - ThrowUserError('bug_url_invalid', - { url => $input, reason => 'id' }); - } - # This is the shortest standard URL form for Debian BTS URLs, - # and so we reduce all URLs to this. - $result = "http://bugs.debian.org/" . $bug_id; - } - # Bugzilla URLs - else { - if ($uri->path !~ /show_bug\.cgi$/) { - ThrowUserError('bug_url_invalid', - { url => $input, reason => 'show_bug' }); - } - - my $bug_id = $uri->query_param('id'); - # We don't currently allow aliases, because we can't check to see - # if somebody's putting both an alias link and a numeric ID link. - # When we start validating the URL by accessing the other Bugzilla, - # we can allow aliases. - detaint_natural($bug_id); - if (!$bug_id) { - ThrowUserError('bug_url_invalid', - { url => $input, reason => 'id' }); - } + my ($class, $uri) = Bugzilla::BugUrl->class_for($input); - # Make sure that "id" is the only query parameter. - $uri->query("id=$bug_id"); - # And remove any # part if there is one. - $uri->fragment(undef); - my $uri_canonical = $uri->canonical; - $result = $uri_canonical->as_string; - - # If this is a link to a local bug (treating the domain - # case-insensitively and ignoring http(s)://), then also update - # the other bug to point at this one. - my $canonical_local = URI->new($local_bug_uri)->canonical; - if (!$skip_recursion - and $canonical_local->authority eq $uri_canonical->authority - and $canonical_local->path eq $uri_canonical->path) - { - my $ref_bug = Bugzilla::Bug->check($bug_id); - if ($ref_bug->id == $self->id) { - ThrowUserError('see_also_self_reference'); - } - - my $product = $ref_bug->product_obj; - if (!Bugzilla->user->can_edit_product($product->id)) { - ThrowUserError("product_edit_denied", - { product => $product->name }); - } - - my $ref_input = $local_bug_uri . $self->id; - if (!grep($ref_input, @{ $ref_bug->see_also })) { - $ref_bug->add_see_also($ref_input, 'skip recursion'); - push @{ $self->{see_also_update} }, $ref_bug; - } - } + my $params = { value => $uri, bug_id => $self }; + $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 (length($result) > MAX_BUG_URL_LENGTH) { - ThrowUserError('bug_url_too_long', { url => $result }); + # 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; + push @{ $self->{see_also_changes} }, $ref_bug->id + if !grep { $_ 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. - if (!grep { lc($_) eq lc($result) } @{ $self->see_also }) { + my $value = $uri->as_string; + if (!grep { lc($_) eq lc($value) } @{ $self->see_also }) { my $privs; - my $can = $self->check_can_change_field('see_also', '', $result, \$privs); + my $can = $self->check_can_change_field('see_also', '', $value, \$privs); if (!$can) { ThrowUserError('illegal_change', { field => 'see_also', - newvalue => $result, + newvalue => $value, privs => $privs }); } - push(@{ $self->see_also }, $result); + push @{ $self->{added_see_also} }, $field_values; } } -- cgit v1.2.3-24-g4f1b