Closed Bug 916906 Opened 7 years ago Closed 7 years ago

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

Categories

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

Production
x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: glob, Assigned: glob)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
woot! You will make roughly 100+ people happy if you do this.
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
Attached patch 916906_1.patch (obsolete) β€” β€” Splinter Review
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
(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
Attached patch 916906_2.patch (obsolete) β€” β€” Splinter Review
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.
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
(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
Attached patch 916906_3.patch β€” β€” Splinter Review
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)
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
Closed: 7 years ago
Resolution: --- → FIXED
this has been pushed to production.
Have you ran the migrate script yet? I'm still seeing patches with redirects.
Flags: needinfo?(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.
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.