472872 - Add a field where people can put the URLs to Bugzilla bugs (from any Bugzilla installation)

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Max Kanat-Alexander]

This is the first step of bug 231429. We just need a field that allows you to enter the URLs of bugs in other Bugzilla installations. 

It will validate that the URL points to a valid Bugzilla bug that the server can access, because we'll want to do things with these URLs in future bugs (like retrieve their summary information and all that). I'm not quite sure what to do about requirelogin installations, though.

Comment 1

[Max Kanat-Alexander]

Attachment: WIP

Here's a work-in-progress with EXTREMELY basic functionality.

Comment 2

[Frédéric Buclin]

Why having to paste the whole URL? It would be easier to just select the target Bugzilla installation from a select field, and just add the bug ID, as they do for RedHat.

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> Why having to paste the whole URL?

  Because then nobody has to maintain a list of Bugzilla installations, and it's very easy to understand the concept of a URL.

Comment 4

[David Lawrence [:dkl]]

Couple questions:

1. Will the input field be a textarea so that multiple links can be added or is this going to be only for one link really?

2. Will the backend code be grepping the bug id and domain name from the posted link in order to create an appropriate webservices call to the server or will it just call the link as is and do some kind of html parsing?

Dave

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> Couple questions:
> 
> 1. Will the input field be a textarea so that multiple links can be added or is
> this going to be only for one link really?

  I think a textarea would take up too much space. It allows comma-separated URLs.

> 2. Will the backend code be grepping the bug id and domain name from the posted
> link in order to create an appropriate webservices call to the server or will
> it just call the link as is and do some kind of html parsing?

  It will probably be parsing the link. That will also allow us to do some rudimentary validation checking on the URL before doing a webservices call (which could be slow).

Comment 6

[David Lawrence [:dkl]]

There is a perl module called URI that has a function called uri_split() that would help with this. But looking at the source, the uri_split() function is only a single line regex so you could just pull that out and put it in Bugzilla::Util.

sub uri_split {
    my ($uri) = @_;

    my ($scheme, $auth, $path, $query, $frag) = $uri
        =~ m,(?:([^:/?#]+):)?(?://([^/?#]*))?([^?#]*)(?:\?([^#]*))?(?:#(.*))?,;

    return {
        scheme => $scheme,
        auth   => $auth,
        path   => $path,
        query  => $query,
        frag   => $frag
    };
}

or just return as a list. The you can take the pieces and form a xmlrpc.cgi call to the proper server.

Dave

Comment 7

[Frédéric Buclin]

(In reply to comment #3)
>   Because then nobody has to maintain a list of Bugzilla installations, and
> it's very easy to understand the concept of a URL.

I still prefer the concept used by RedHat. Or at least, if I paste https://bugzilla.redhat.com/show_bug.cgi?id=479725, I would like that the field displays "RedHat bug 479725" instead, to be easily human readable by other users. This means that the local Bugzilla has to recognize https://bugzilla.redhat.com/ as being RedHat's installation, but we could either have some admin UI to add the URL -> installation name relationship, or maybe have e.g. an XML file on updates.bugzilla.org with most common Bugzilla installations in it, which would be reloaded once per week or so; this way we would have a centralized file, which would require no action from the admin. If someone wants to add custom installations, he can then edit his local XML file and add them.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> (In reply to comment #3)
> >   Because then nobody has to maintain a list of Bugzilla installations, and
> > it's very easy to understand the concept of a URL.
> 
> I still prefer the concept used by RedHat. Or at least, if I paste
> https://bugzilla.redhat.com/show_bug.cgi?id=479725, I would like that the field
> displays "RedHat bug 479725" instead,

  Yeah, I was thinking of adding a feature to do that after this, where we add a "nickname" field to config.cgi and use that if it's set. We could also do something like reading the domain name and the URL path and combining them to create a nickname.

Comment 9

[Max Kanat-Alexander]

(In reply to comment #6)
> There is a perl module called URI that has a function called uri_split() that
> would help with this. But looking at the source, the uri_split() function is
> only a single line regex so you could just pull that out and put it in
> Bugzilla::Util.

  Sweet. Perhaps I will. Although I think one of our modules might already pull in the URI module--I'll have to check.

Comment 10

[Max Kanat-Alexander]

Attachment: v1

Okay, this is the most basic functionality reasonably possible for this field. We validate the URL by using the URI module. I made it a requirement--I've wanted to use this module for many other things in Bugzilla in the past, and now that we have install-module.pl I don't feel so bad about adding a new requirement.

I made it required instead of optional because I want ALL Bugzillas to have the "See Also" functionality, so that we can implement all the inter-Bugzilla features that we want and have them work on every Bugzilla in the world. Also, I want to start using some of the URI methods internally in Bugzilla to replace some custom hacks we're currently using.

The installation-nickname stuff and any further functionality (such as using AJAX to get data about a bug when you hover over it, possibly) will happen in future bugs, not in this one. This just gives us very basic functionality.

Comment 11

[David Lawrence [:dkl]]

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

>Index: process_bug.cgi
>+    if (should_set('see_also')) {
>+        my @see_also = split(',', $cgi->param('see_also'));
>+        $b->add_see_also($_) foreach @see_also;
>+    }
>+    if (should_set('remove_see_also')) {
>+        $b->remove_see_also($_) foreach $cgi->param('remove_see_also')
>+    }

Nit-pick: I know it is less to type but is generally best to use my variables
in a foreach loop for readability (PBP #71). But not a show-stopper.

>Index: Bugzilla/Bug.pm
>+    if ($uri->path !~ /show_bug.cgi$/) {
>+        ThrowUserError('bug_url_invalid', 
>+                       { url => $input, reason => 'show_bug' });
>+    }

Need to escape the '.' in the regex.

    if ($uri->path !~ /show_bug\.cgi$/) {
        ThrowUserError('bug_url_invalid', 
                       { url => $input, reason => 'show_bug' });
    }

>Index: Bugzilla/DB/Schema.pm
>+    bug_see_also => {
>+        FIELDS => [
>+            bug_id => {TYPE => 'INT3', NOTNULL => 1},
>+            value  => {TYPE => 'varchar(255)', NOTNULL => 1},
>+        ],
>+        INDEXES => [
>+            bug_see_alsobug_id_idx => {FIELDS => [qw(bug_id value)], 
>+                                       TYPE   => 'UNIQUE'},
>+        ],
>+    },

Missing space in index name:

            bug_see_also_bug_id_idx => {FIELDS => [qw(bug_id value)], 
                                       TYPE   => 'UNIQUE'},  


>Index: template/en/default/bug/field.html.tmpl
>+     [% CASE 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>
>+           <label><input type="checkbox" value="[% url FILTER html %]"
>+                         name="remove_[% field.name FILTER html %]">
>+             Remove</label>
>+         </li>
>+       [% END %]
>+       [% '</ul>' IF value.size %]
>+
>+       Add [% terms.Bug %] URLs:
>+       <input type="text" id="[% field.name FILTER html %]" 
>+              name="[% field.name FILTER html %]" size="40">

Nit: It would be nicer if the input text field was below the "Add Bug URLS:" and
that the "Add Bug URLs:" text was bold. This is how it is for the CC input. Having
the text above the input also makes the screen less wide.

>Index: template/en/default/pages/fields.html.tmpl
>+<h2><a name="see_also"></a>See Also</h2>
>+
>+<p>This allows you to refer to [% terms.bugs %] in other [% terms.Bugzilla %]
>+  installations. You can enter a URL to a [% terms.Bugzilla %] [%+ terms.bug %]
>+  in the "Add [% terms.Bug %] URLs" field to note that that [% terms.bug %]
>+  is related to this one. You can enter multiple URLs at once by separating
>+  them with a comma.</p>
>+
>+<p>You should normally use this field to refer to [% terms.bugs %] in 
>+  <em>other</em> [% terms.Bugzilla %] installations. For [% terms.bugs %]
>+  in this [% terms.Bugzilla %], it is better to use the "Depends On" and
>+  "Blocks" fields.</p>
>+
> [% INCLUDE global/footer.html.tmpl %]

Is it possible to use something other than [% terms.Bugzilla %] here since for example
our variable is set to "Red Hat Bugzilla" so this would read:

<p>You should normally use this field to refer to bugs in 
  <em>other</em> Red Hat Bugzilla installations. For bugs
  in this Red Hat Bugzilla, it is better to use the "Depends On" and
  "Blocks" fields.</p>

which is technically incorrect. So somehow we should have a variable that
refers to the a generic Bugzilla name and not the proper name of that 
particular installation. Maybe we are the only ones who ever change that 
value to a company name, which in that case this can be ignored, but I feel
it might be more widespread.

Dave

Comment 12

[David Lawrence [:dkl]]

Also I forgot to mention in my review that running a search on the "See Also" field using boolean search causes database error:

Boolean values: "See Also" "contains the string" "bugzilla.redhat.com"

Error:

[Mon Jan 19 16:40:49 2009] [error] DBD::mysql::st execute failed: Unknown column 'bugs.see_also' in 'where clause' [for Statement "SELECT bugs.bug_id, bugs.bug_severity, bugs.priority, bugs.bug_status, bugs.resolution, bugs.bug_severity, bugs.priority, bugs.op_sys, map_assigned_to.login_name, bugs.bug_status, bugs.resolution, bugs.short_desc, map_products.name FROM bugs  INNER JOIN profiles AS map_assigned_to ON (bugs.assigned_to = map_assigned_to.userid) INNER JOIN products AS map_products ON (bugs.product_id = map_products.id) LEFT JOIN priority ON (priority.value = bugs.priority) LEFT JOIN bug_severity ON (bug_severity.value = bugs.bug_severity) LEFT JOIN bug_group_map  ON bug_group_map.bug_id = bugs.bug_id  AND bug_group_map.group_id NOT IN (1,12,10,11,13,9,4,8,5,6,7,3,2)  LEFT JOIN cc ON cc.bug_id = bugs.bug_id AND cc.who = 1 WHERE (( bugs.bug_status IN ('NEW','ASSIGNED','REOPENED') )) AND ((INSTR(bugs.see_also, 'bugzilla.redhat.com') > 0)) AND bugs.creation_ts IS NOT NULL AND ((bug_group_map.group_id IS NULL)    OR (bugs.reporter_accessible = 1 AND bugs.reporter = 1)     OR (bugs.cclist_accessible = 1 AND cc.who IS NOT NULL)     OR (bugs.assigned_to = 1) ) GROUP BY bugs.bug_id ORDER BY priority.sortkey,priority.value,bug_severity.sortkey,bug_severity.value"] at /var/www/html/bugzilla/buglist.cgi line 1054\n\tModPerl::ROOT::Bugzilla::ModPerl::ResponseHandler::var_www_html_bugzilla_buglist_2ecgi::handler('Apache2::RequestRec=SCALAR(0xb9fff1a4)') called at /usr/lib/perl5/vendor_perl/5.10.0/i386-linux-thread-multi/ModPerl/RegistryCooker.pm line 204\n\teval {...} called at /usr/lib/perl5/vendor_perl/5.10.0/i386-linux-thread-multi/ModPerl/RegistryCooker.pm line 204\n\tModPerl::RegistryCooker::run('Bugzilla::ModPerl::ResponseHandler=HASH(0xb9c43d84)') called at /usr/lib/perl5/vendor_perl/5.10.0/i386-linux-thread-multi/ModPerl/RegistryCooker.pm line 170\n\tModPerl::RegistryCooker::default_handler('Bugzilla::ModPerl::ResponseHandler=HASH(0xb9c43d84)') called at /usr/lib/perl5/vendor_perl/5.10.0/i386-linux-thread-multi/ModPerl/Registry.pm line 31\n\tModPerl::Registry::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0xb9fff1a4)') called at /var/www/html/bugzilla/mod_perl.pl line 97\n\tBugzilla::ModPerl::ResponseHandler::handler('Bugzilla::ModPerl::ResponseHandler', 'Apache2::RequestRec=SCALAR(0xb9fff1a4)') called at -e line 0\n\teval {...} called at -e line 0\n

Comment 13

[Max Kanat-Alexander]

(In reply to comment #11)
> Nit-pick: I know it is less to type but is generally best to use my variables
> in a foreach loop for readability (PBP #71). But not a show-stopper.

  Only true for multi-line foreach loops.

  I'll fix everything else, thanks for catching all those! :-)

Comment 14

[Max Kanat-Alexander]

Attachment: v2

Okay, fixed all that stuff. :-)

Comment 15

[David Lawrence [:dkl]]

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

Looks good. All testing passed. r=dkl

Comment 16

[Max Kanat-Alexander]

Inter-Bugzilla, here we come! :-)

Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.416; previous revision: 1.415
done
Checking in Bugzilla/Bug.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Bug.pm,v  <--  Bug.pm
new revision: 1.274; previous revision: 1.273
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.105; previous revision: 1.104
done
Checking in Bugzilla/Field.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Field.pm,v  <--  Field.pm
new revision: 1.42; previous revision: 1.41
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.170; previous revision: 1.169
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.111; previous revision: 1.110
done
Checking in Bugzilla/Install/Requirements.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/Requirements.pm,v  <--  Requirements.pm
new revision: 1.60; previous revision: 1.59
done
Checking in skins/standard/global.css;
/cvsroot/mozilla/webtools/bugzilla/skins/standard/global.css,v  <--  global.css
new revision: 1.58; previous revision: 1.57
done
Checking in template/en/default/bug/edit.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/edit.html.tmpl,v  <--  edit.html.tmpl
new revision: 1.145; previous revision: 1.144
done
Checking in template/en/default/bug/field.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/bug/field.html.tmpl,v  <--  field.html.tmpl
new revision: 1.22; previous revision: 1.21
done
Checking in template/en/default/global/field-descs.none.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/field-descs.none.tmpl,v  <--  field-descs.none.tmpl
new revision: 1.31; previous revision: 1.30
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.272; previous revision: 1.271
done
Checking in template/en/default/pages/fields.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/pages/fields.html.tmpl,v  <--  fields.html.tmpl
new revision: 1.15; previous revision: 1.14
done

Comment 17

[Vitaly Fedrushkov]

>+    [% title = "Invalid Bug URL" %]

shouldn't that be $terms.Bug?

Comment 18

[Max Kanat-Alexander]

(In reply to comment #17)
> >+    [% title = "Invalid Bug URL" %]
> 
> shouldn't that be $terms.Bug?

  Indeed it should. Fixed:

Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.273; previous revision: 1.272
done

Comment 19

[Max Kanat-Alexander]

Added to the release notes for Bugzilla 3.4 in bug 494037.