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

RESOLVED FIXED

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: glob, Assigned: glob)

Tracking

Production
x86
Mac OS X
Dependency tree / graph

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

5 years ago
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.
I like this.
woot! You will make roughly 100+ people happy if you do this.
(Assignee)

Comment 3

5 years ago
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.
Summary: attaching a file which just contains a github pull request should automatically redirect to it when viewing → attaching a file which just contains a github url should automatically redirect to it when viewing
(Assignee)

Comment 4

5 years ago
Created attachment 805813 [details] [diff] [review]
916906_1.patch
Attachment #805813 - Flags: review?(dkl)
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
(Assignee)

Comment 6

5 years ago
(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
(Assignee)

Comment 7

5 years ago
Created attachment 805852 [details] [diff] [review]
916906_2.patch

this uses the more restrictive regexs, so we only auto-redirect for pull requests and links to branches.
Attachment #805813 - Attachment is obsolete: true
Attachment #805813 - Flags: review?(dkl)
Attachment #805852 - Flags: review?(dkl)
CCing Dietrich so he can track this to update "Github tweaks for Bugzilla".
(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.
(Assignee)

Comment 10

5 years ago
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.
Attachment #805852 - Flags: review?(dkl)
how about setting a custom mime type instead? this would be more upstream compatible.
text/x-github-pull-request or something
(Assignee)

Comment 13

5 years ago
(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.
No longer blocks: 918625
(Assignee)

Comment 14

5 years ago
Created attachment 807648 [details] [diff] [review]
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)
Attachment #805852 - Attachment is obsolete: true
Attachment #807648 - Flags: review?(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
Attachment #807648 - Flags: review?(dkl) → review+
Any clue when this will be available on bugzilla.mozilla.org?
Flags: needinfo?(glob)
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
Flags: needinfo?(glob)
(Assignee)

Comment 19

5 years ago
Committing to: bzr+ssh://bjones%40mozilla.com@bzr.mozilla.org/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.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 20

5 years ago
this has been pushed to production.
Have you ran the migrate script yet? I'm still seeing patches with redirects.
Flags: needinfo?(glob)
(Assignee)

Comment 22

5 years ago
(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.
Flags: needinfo?(glob)
My user_profile numbers will stay so low then :(
You need to log in before you can comment on or make changes to this bug.