704999 - Add support for GitHub for the 'See Also' field

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Michał Górny]

Please add support for adding github issues in the 'See Also' field. Those take URL in the form:

	https://github.com/<user>/<project>/issues/<n>

where <user> & <project> would be \S+?, and <n> would be \d+.

Comment 1

[Matt Selsky [:selsky]]

Attachment: Add support for github issues, v1

Comment 2

[Michał Górny]

Comment on attachment 578698 [details] [diff] [review]
Add support for github issues, v1

Just a side comment:

> +    # While GitHub URLs can be either HTTP or HTTPS,
> +    # always go with the HTTPS scheme, as that's the default.

That's not exactly true, I'd say. Github URLs are always https; http always redirects.

Comment 3

[Matt Selsky [:selsky]]

Attachment: Add support for github issues, v2

How's this?

Comment 4

[Tiago Mello [:timello]]

Comment on attachment 578772 [details] [diff] [review]
Add support for github issues, v2

Review of attachment 578772 [details] [diff] [review]:
-----------------------------------------------------------------

::: Bugzilla/BugUrl/GitHub.pm
@@ +50,5 @@
> +        $username = $1;
> +        $repository_name = $2;
> +        $issue_id = $3;
> +    } else {
> +        ThrowUserError('bug_url_invalid', { url => $value });

Same as GetSatisfaction, should_handle could look for the right pattern and then we let class_for throw the error.

Comment 5

[Matt Selsky [:selsky]]

I was following the examples of Debian.pm, Google.pm, Launchpad.pm, and Sourceforge.pm

Should those also have a more complete pattern in should_handle?

Comment 6

[Frédéric Buclin]

CC'ing timello so that he can reply to Matt's question in comment 5.

Comment 7

[Tiago Mello [:timello]]

(In reply to Matt Selsky [:selsky] from comment #5)
> I was following the examples of Debian.pm, Google.pm, Launchpad.pm, and
> Sourceforge.pm
> 
> Should those also have a more complete pattern in should_handle?

You know what? If class_for already does that why we are repeating code in the subclasses? Also, I think the reason => 'show_bug' only applies to Bugzillas... and we can't say if the 'id' is whether valid or not, unless per the pattern...

So I think we could:

1. Move the content of [% IF reason == 'show_bug' %] in the user-error template to an [% ELSE %] and then no need to pass 'reason' in the class_for.

2. Completely remove the reason 'id' and 'show_bug' because we are going to catch that in the pattern of the should_handle method.

I think it's enough to say that the url must be a valid url from any of the listed bug trackers.

Comment 8

[Tiago Mello [:timello]]

So, the _check_value method would only be in charge of returning the URI object with the shortest url form for the bug tracker...

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

+1 this would be fantastic for b2g and pdf.js, which span across github and bugzilla.

Comment 10

[Matt Selsky [:selsky]]

Attachment: Add support for github issues, v3

Updated should_handle as per comment 7.  Should _check_value be renamed to _canonify_url, or similar, since that's the purpose as per comment 8?

Comment 11

[Matt Selsky [:selsky]]

Attachment: Add support for github issues, v4

Don't use Bugzilla::{Util,Error} since those are only called in the parent class.

Comment 12

[Tiago Mello [:timello]]

(In reply to Matt Selsky [:selsky] from comment #10)
> Created attachment 588800 [details] [diff] [review]
> Add support for github issues, v3
> 
> Updated should_handle as per comment 7.  Should _check_value be renamed to
> _canonify_url, or similar, since that's the purpose as per comment 8?

No, _check_[something] is the default name for every validator in Bugzilla even though it is not actually 'validating' anything.

Comment 13

[Tiago Mello [:timello]]

Comment on attachment 588805 [details] [diff] [review]
Add support for github issues, v4


>=== added file 'Bugzilla/BugUrl/GitHub.pm'
+
>+sub should_handle {
>+    my ($class, $uri) = @_;
>+
>+    # GitHub issue URLs have only one form:
>+    #  https://github.com/USER_OR_TEAM_OR_ORGANIZATION_NAME/REPOSITORY_NAME/issues/111
>+    return ($uri->authority =~ /^github.com$/i
>+            and $uri->path =~ m|^/[^/]+/[^/]+/issues/\d+$|) ? 1 : 0;

Ah! This is where that GetSatisfaction code came from :)

>+}
>+
>+sub _check_value {
>+    my $class = shift;
>+
>+    my $uri = $class->SUPER::_check_value(@_);
>+
>+    # GitHub HTTP URLs simply redirect to HTTPS, so just use
>+    # the HTTPS scheme.
>+    if ($uri->scheme eq 'http') {
>+        $uri->scheme('https');
>+    }

I agree, this is doing the exactly the same as GetSatisfaction does.
Lets create a common method and use it here. Maybe as you
suggested _canonify_url and call it here.

Otherwise, it looks good.

Comment 14

[:Gavin Sharp [email: [e-mail]]]

Matt, are you still available to update the patch? We'd really like this on b.m.o.

Comment 16

[Matt Selsky [:selsky]]

Attachment: Add support for github issues, v5

Simplify the scheme forcing.

Comment 17

[Tiago Mello [:timello]]

Comment on attachment 609614 [details] [diff] [review]
Add support for github issues, v5

The patch should be changed to add GitHub in the MoreBugUrl extension.

Comment 18

[Frédéric Buclin]

I think GitHub is popular enough to be in the core code.

Comment 19

[Tiago Mello [:timello]]

Comment on attachment 609614 [details] [diff] [review]
Add support for github issues, v5

So, otherwise the patch looks good.

Comment 20

[Frédéric Buclin]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/BugUrl.pm
added Bugzilla/BugUrl/GitHub.pm
modified template/en/default/global/user-error.html.tmpl
Committed revision 8202.

Comment 21

[Frédéric Buclin]

Added to relnotes for 4.4.