Closed
Bug 916906
Opened 11 years ago
Closed 11 years ago
attaching a file which just contains a github url should automatically redirect to it when viewing
Categories
(bugzilla.mozilla.org :: General, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: glob, Assigned: glob)
References
Details
Attachments
(1 file, 2 obsolete files)
7.45 KB,
patch
|
dkl
:
review+
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
I like this.
Comment 2•11 years ago
|
||
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
Attachment #805813 -
Flags: review?(dkl)
Comment 5•11 years ago
|
||
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
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)
Comment 8•11 years ago
|
||
CCing Dietrich so he can track this to update "Github tweaks for Bugzilla".
Comment 9•11 years ago
|
||
(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•11 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)
Comment 11•11 years ago
|
||
how about setting a custom mime type instead? this would be more upstream compatible.
Comment 12•11 years ago
|
||
text/x-github-pull-request or something
Assignee | ||
Comment 13•11 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.
Assignee | ||
Comment 14•11 years ago
|
||
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 15•11 years ago
|
||
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+
Comment 16•11 years ago
|
||
\o/
Comment 17•11 years ago
|
||
Any clue when this will be available on bugzilla.mozilla.org?
Flags: needinfo?(glob)
Comment 18•11 years ago
|
||
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•11 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
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 20•11 years ago
|
||
this has been pushed to production.
Comment 21•11 years ago
|
||
Have you ran the migrate script yet? I'm still seeing patches with redirects.
Flags: needinfo?(glob)
Assignee | ||
Comment 22•11 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)
Comment 23•11 years ago
|
||
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.
Description
•