916906 - attaching a file which just contains a github url should automatically redirect to it when viewing

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect)

Description

[:glob ✱]

attaching a file which just contains a github pull request should automatically redirect to it when viewing.  that will greatly help teams using pull requests to track development.

i'm thinking of adding a content-type for pull requests, that when you click on the attachment to view it, instead redirects you to that url's location.

Comment 1

[John Ford [:jhford] CET/CEST Berlin Time]

I like this.

Comment 2

[James Lal [:lightsofapollo]]

woot! You will make roughly 100+ people happy if you do this.

Comment 3

[:glob ✱]

i've played around with this a bit, looking at existing attachments and exploring ui options for making this "just work".  my experiments with adding a content type or a checkbox to the create attachment form resulted in a rather clumsy experience.

the simplest thing i have come up with is to look at the attachment payload for small text/plain attachments, and redirect if it's a github url.  this covers links to pull requests as well as branches.

Comment 4

[:glob ✱]

Attachment: 916906_1.patch

Comment 5

[Reed Loden [:reed]]

Comment on attachment 805813 [details] [diff] [review]
916906_1.patch

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

::: extensions/BMO/Extension.pm
@@ +715,5 @@
> +
> +    # check the attachment content
> +    my $data = trim($attachment->data);
> +    return if $data =~ /\s/;
> +    return unless $data =~ m#^https://github\.com/#i;

Not a fan of this... Can we restrict this to PRs only? Seems like this could be possibly used to maliciously redirect a user somewhere, especially since it could be *any* page on github.com

Comment 6

[:glob ✱]

(In reply to Reed Loden [:reed] from comment #5)
> Not a fan of this... Can we restrict this to PRs only? Seems like this could
> be possibly used to maliciously redirect a user somewhere, especially since
> it could be *any* page on github.com

prior to taking this approach i looked at what is being attached, and there's a fair amount of links to github branches as well as pull requests.

as best as i can tell the regex i use excludes all user-generated content -- you can only link to the github ui itself.

how about:

     m#^https://github.com/[^/]+/[^/]+/pull/\d+$#i
  || m#^https://github.com/[^/]+/[^/]+/tree/[^/]+$#i

Comment 7

[:glob ✱]

Attachment: 916906_2.patch

this uses the more restrictive regexs, so we only auto-redirect for pull requests and links to branches.

Comment 8

[Anthony Ricaud (:rik)]

CCing Dietrich so he can track this to update "Github tweaks for Bugzilla".

Comment 9

[Dietrich Ayala (:dietrich)]

(In reply to Byron Jones ‹:glob› from comment #6)
> (In reply to Reed Loden [:reed] from comment #5)
> > Not a fan of this... Can we restrict this to PRs only? Seems like this could
> > be possibly used to maliciously redirect a user somewhere, especially since
> > it could be *any* page on github.com
> 
> prior to taking this approach i looked at what is being attached, and
> there's a fair amount of links to github branches as well as pull requests.

"review this branch" is kind of not awesome for reviewers. i'd be happy with just getting support for PRs for now.

Comment 10

[:glob ✱]

Comment on attachment 805852 [details] [diff] [review]
916906_2.patch

ok, PRs only it is :)

since writing this patch i had a different idea for the implementation - instead of inspecting each attachment when viewing, add a field to the attachments table indicating if it's a github url, and set that at attachment time.  this will be more efficient, and is an enabler for other features and metrics.

Comment 11

[Dave Miller [:justdave]]

how about setting a custom mime type instead? this would be more upstream compatible.

Comment 12

[Dave Miller [:justdave]]

text/x-github-pull-request or something

Comment 13

[:glob ✱]

(In reply to Dave Miller [:justdave] from comment #11)
> how about setting a custom mime type instead? this would be more upstream
> compatible.  text/x-github-pull-request or something.

i like the cut of your jib.

Comment 14

[:glob ✱]

Attachment: 916906_3.patch

this patch:
- automatically changes the content-type of text/plain attachments which contain
  only a github pull-request url to text/x-github-pull-request (when the
  attachment is created)
- redirects to that url when viewing attachments with the appropriate content-type
- includes a script to migrate the content-type of existing attachments
- avoids redirecting to github in the attachment details iframe (this bit is a
  straight edit as adding a hook there was messy and not worth the gain imho)

Comment 15

[David Lawrence [:dkl]]

Comment on attachment 807648 [details] [diff] [review]
916906_3.patch

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

Works well and looks good. r=dkl

Comment 16

[John Ford [:jhford] CET/CEST Berlin Time]

\o/

Comment 17

[James Lal [:lightsofapollo]]

Any clue when this will be available on bugzilla.mozilla.org?

Comment 18

[David Lawrence [:dkl]]

Should be this weeks normal code push to prod. We didn't want to do too much changes before this past weekends tracking flag migration. Nothing else should be holding it up.

dkl

Comment 19

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified attachment.cgi
modified extensions/BMO/Extension.pm
added extensions/BMO/bin
added extensions/BMO/bin/migrate-github-pull-requests.pl
modified extensions/BMO/lib/Data.pm
modified template/en/default/attachment/edit.html.tmpl
Committed revision 9068.

note to self: need to run extensions/BMO/bin/migrate-github-pull-requests.pl after this is pushed to production.

Comment 20

[:glob ✱]

this has been pushed to production.

Comment 21

[Anthony Ricaud (:rik)]

Have you ran the migrate script yet? I'm still seeing patches with redirects.

Comment 22

[:glob ✱]

(In reply to Anthony Ricaud (:rik) from comment #21)
> Have you ran the migrate script yet? I'm still seeing patches with redirects.

yes, the migration script has been run.

only attachment which match the criteria have been updated (ie. text/plain with a github url).  any other attachment, including html doing their own redirects, will remain untouched as to fix those would require altering the attachment payload.  it isn't worth the risk to do that for this feature.

Comment 23

[Anthony Ricaud (:rik)]

My user_profile numbers will stay so low then :(