Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve issue autolinks #6273

Merged
merged 9 commits into from
Apr 12, 2019
18 changes: 9 additions & 9 deletions models/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -469,19 +469,19 @@ func (repo *Repository) mustOwnerName(e Engine) string {
return repo.OwnerName
}

// ComposeMetas composes a map of metas for rendering external issue tracker URL.
// ComposeMetas composes a map of metas for properly rendering issue links and external issue trackers.
func (repo *Repository) ComposeMetas() map[string]string {
unit, err := repo.GetUnit(UnitTypeExternalTracker)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the repo don't have external tracker, then just return nil. So why move it below?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't return nil at all anymore, since we also want the metas if using the local issue tracker (for this specific case of checking to see what repo a user is in when writing an issue number and deciding how to link it). We should always want to have the username and repo name in context if possible because if not people are just scraping the URL for many of the HTML / Markdown processing functions which isn't accurate always.

These changes are discussed in detail in this comment by @zeripath if that helps: https://github.com/go-gitea/gitea/pull/6273/files/102bcffcb6aa60c5bee1054d5c31daa483f2c0a0#discussion_r266227788

Which is where a few of these commits come from.

I tried to edit the description of the to include that information as well, sorry if it was not clear.

if err != nil {
return nil
}

if repo.ExternalMetas == nil {
repo.ExternalMetas = map[string]string{
"format": unit.ExternalTrackerConfig().ExternalTrackerFormat,
"user": repo.MustOwner().Name,
"repo": repo.Name,
"user": repo.MustOwner().Name,
"repo": repo.Name,
}
unit, err := repo.GetUnit(UnitTypeExternalTracker)
if err != nil {
return repo.ExternalMetas
}

repo.ExternalMetas["format"] = unit.ExternalTrackerConfig().ExternalTrackerFormat
switch unit.ExternalTrackerConfig().ExternalTrackerStyle {
case markup.IssueNameStyleAlphanumeric:
repo.ExternalMetas["style"] = markup.IssueNameStyleAlphanumeric
Expand Down
5 changes: 4 additions & 1 deletion models/repo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,10 @@ func TestRepo(t *testing.T) {
repo.Owner = &User{Name: "testOwner"}

repo.Units = nil
assert.Nil(t, repo.ComposeMetas())

metas := repo.ComposeMetas()
assert.Equal(t, "testRepo", metas["repo"])
assert.Equal(t, "testOwner", metas["user"])

externalTracker := RepoUnit{
Type: UnitTypeExternalTracker,
Expand Down
34 changes: 26 additions & 8 deletions modules/markup/html.go
Original file line number Diff line number Diff line change
Expand Up @@ -551,20 +551,37 @@ func shortLinkProcessorFull(ctx *postProcessCtx, node *html.Node, noLink bool) {
}

func fullIssuePatternProcessor(ctx *postProcessCtx, node *html.Node) {
if ctx.metas == nil {
return
}
m := getIssueFullPattern().FindStringSubmatchIndex(node.Data)
if m == nil {
return
}
link := node.Data[m[0]:m[1]]
id := "#" + node.Data[m[2]:m[3]]
// TODO if m[4]:m[5] is not nil, then link is to a comment,
// and we should indicate that in the text somehow
replaceContent(node, m[0], m[1], createLink(link, id))

// extract repo and org name from matched link like
// http://localhost:3000/gituser/myrepo/issues/1
linkParts := strings.Split(path.Clean(link), "/")
matchOrg := linkParts[len(linkParts)-4]
matchRepo := linkParts[len(linkParts)-3]

if matchOrg == ctx.metas["user"] && matchRepo == ctx.metas["repo"] {
// TODO if m[4]:m[5] is not nil, then link is to a comment,
// and we should indicate that in the text somehow
replaceContent(node, m[0], m[1], createLink(link, id))

} else {
orgRepoID := matchOrg + "/" + matchRepo + id
replaceContent(node, m[0], m[1], createLink(link, orgRepoID))
}
}

func issueIndexPatternProcessor(ctx *postProcessCtx, node *html.Node) {
prefix := cutoutVerbosePrefix(ctx.urlPrefix)

if ctx.metas == nil {
return
}
// default to numeric pattern, unless alphanumeric is requested.
pattern := issueNumericPattern
if ctx.metas["style"] == IssueNameStyleAlphanumeric {
Expand All @@ -575,18 +592,19 @@ func issueIndexPatternProcessor(ctx *postProcessCtx, node *html.Node) {
if match == nil {
return
}

id := node.Data[match[2]:match[3]]
var link *html.Node
if ctx.metas == nil {
link = createLink(util.URLJoin(prefix, "issues", id[1:]), id)
} else {
if _, ok := ctx.metas["format"]; ok {
// Support for external issue tracker
if ctx.metas["style"] == IssueNameStyleAlphanumeric {
ctx.metas["index"] = id
} else {
ctx.metas["index"] = id[1:]
}
link = createLink(com.Expand(ctx.metas["format"], ctx.metas), id)
} else {
link = createLink(util.URLJoin(setting.AppURL, ctx.metas["user"], ctx.metas["repo"], "issues", id[1:]), id)
}
replaceContent(node, match[2], match[3], link)
}
Expand Down
20 changes: 15 additions & 5 deletions modules/markup/html_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,12 @@ var alphanumericMetas = map[string]string{
"style": IssueNameStyleAlphanumeric,
}

// these values should match the Repo const above
var localMetas = map[string]string{
"user": "gogits",
"repo": "gogs",
}

func TestRender_IssueIndexPattern(t *testing.T) {
// numeric: render inputs without valid mentions
test := func(s string) {
Expand Down Expand Up @@ -91,7 +97,7 @@ func TestRender_IssueIndexPattern2(t *testing.T) {
links[i] = numericIssueLink(util.URLJoin(setting.AppSubURL, "issues"), index)
}
expectedNil := fmt.Sprintf(expectedFmt, links...)
testRenderIssueIndexPattern(t, s, expectedNil, nil)
testRenderIssueIndexPattern(t, s, expectedNil, &postProcessCtx{metas: localMetas})

for i, index := range indices {
links[i] = numericIssueLink("https://someurl.com/someUser/someRepo/", index)
Expand Down Expand Up @@ -171,6 +177,7 @@ func testRenderIssueIndexPattern(t *testing.T, input, expected string, ctx *post
if ctx.urlPrefix == "" {
ctx.urlPrefix = AppSubURL
}

res, err := ctx.postProcess([]byte(input))
assert.NoError(t, err)
assert.Equal(t, expected, string(res))
Expand All @@ -181,10 +188,10 @@ func TestRender_AutoLink(t *testing.T) {
setting.AppSubURL = AppSubURL

test := func(input, expected string) {
buffer, err := PostProcess([]byte(input), setting.AppSubURL, nil, false)
buffer, err := PostProcess([]byte(input), setting.AppSubURL, localMetas, false)
assert.Equal(t, err, nil)
assert.Equal(t, strings.TrimSpace(expected), strings.TrimSpace(string(buffer)))
buffer, err = PostProcess([]byte(input), setting.AppSubURL, nil, true)
buffer, err = PostProcess([]byte(input), setting.AppSubURL, localMetas, true)
assert.Equal(t, err, nil)
assert.Equal(t, strings.TrimSpace(expected), strings.TrimSpace(string(buffer)))
}
Expand Down Expand Up @@ -214,16 +221,19 @@ func TestRender_FullIssueURLs(t *testing.T) {
if ctx.urlPrefix == "" {
ctx.urlPrefix = AppSubURL
}
ctx.metas = localMetas
result, err := ctx.postProcess([]byte(input))
assert.NoError(t, err)
assert.Equal(t, expected, string(result))
}
test("Here is a link https://git.osgeo.org/gogs/postgis/postgis/pulls/6",
"Here is a link https://git.osgeo.org/gogs/postgis/postgis/pulls/6")
test("Look here http://localhost:3000/person/repo/issues/4",
`Look here <a href="http://localhost:3000/person/repo/issues/4">#4</a>`)
`Look here <a href="http://localhost:3000/person/repo/issues/4">person/repo#4</a>`)
test("http://localhost:3000/person/repo/issues/4#issuecomment-1234",
`<a href="http://localhost:3000/person/repo/issues/4#issuecomment-1234">#4</a>`)
`<a href="http://localhost:3000/person/repo/issues/4#issuecomment-1234">person/repo#4</a>`)
test("http://localhost:3000/gogits/gogs/issues/4",
`<a href="http://localhost:3000/gogits/gogs/issues/4">#4</a>`)
}

func TestRegExp_issueNumericPattern(t *testing.T) {
Expand Down
14 changes: 11 additions & 3 deletions modules/markup/markdown/markdown_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,12 @@ const AppURL = "http://localhost:3000/"
const Repo = "gogits/gogs"
const AppSubURL = AppURL + Repo + "/"

// these values should match the Repo const above
var localMetas = map[string]string{
"user": "gogits",
"repo": "gogs",
}

func TestRender_StandardLinks(t *testing.T) {
setting.AppURL = AppURL
setting.AppSubURL = AppSubURL
Expand Down Expand Up @@ -100,7 +106,8 @@ func testAnswers(baseURLContent, baseURLImages string) []string {
<p>Ideas and codes</p>

<ul>
<li>Bezier widget (by <a href="` + AppURL + `r-lyeh" rel="nofollow">@r-lyeh</a>) <a href="http://localhost:3000/ocornut/imgui/issues/786" rel="nofollow">#786</a></li>
<li>Bezier widget (by <a href="` + AppURL + `r-lyeh" rel="nofollow">@r-lyeh</a>) <a href="http://localhost:3000/ocornut/imgui/issues/786" rel="nofollow">ocornut/imgui#786</a></li>
<li>Bezier widget (by <a href="` + AppURL + `r-lyeh" rel="nofollow">@r-lyeh</a>) <a href="http://localhost:3000/gogits/gogs/issues/786" rel="nofollow">#786</a></li>
<li>Node graph editors <a href="https://github.com/ocornut/imgui/issues/306" rel="nofollow">https://github.com/ocornut/imgui/issues/306</a></li>
<li><a href="` + baseURLContent + `/memory_editor_example" rel="nofollow">Memory Editor</a></li>
<li><a href="` + baseURLContent + `/plot_var_example" rel="nofollow">Plot var helper</a></li>
Expand Down Expand Up @@ -188,6 +195,7 @@ var sameCases = []string{
Ideas and codes

- Bezier widget (by @r-lyeh) ` + AppURL + `ocornut/imgui/issues/786
- Bezier widget (by @r-lyeh) ` + AppURL + `gogits/gogs/issues/786
- Node graph editors https://github.com/ocornut/imgui/issues/306
- [[Memory Editor|memory_editor_example]]
- [[Plot var helper|plot_var_example]]`,
Expand Down Expand Up @@ -243,7 +251,7 @@ func TestTotal_RenderWiki(t *testing.T) {
answers := testAnswers(util.URLJoin(AppSubURL, "wiki/"), util.URLJoin(AppSubURL, "wiki", "raw/"))

for i := 0; i < len(sameCases); i++ {
line := RenderWiki([]byte(sameCases[i]), AppSubURL, nil)
line := RenderWiki([]byte(sameCases[i]), AppSubURL, localMetas)
assert.Equal(t, answers[i], line)
}

Expand All @@ -270,7 +278,7 @@ func TestTotal_RenderString(t *testing.T) {
answers := testAnswers(util.URLJoin(AppSubURL, "src", "master/"), util.URLJoin(AppSubURL, "raw", "master/"))

for i := 0; i < len(sameCases); i++ {
line := RenderString(sameCases[i], util.URLJoin(AppSubURL, "src", "master/"), nil)
line := RenderString(sameCases[i], util.URLJoin(AppSubURL, "src", "master/"), localMetas)
assert.Equal(t, answers[i], line)
}

Expand Down
2 changes: 2 additions & 0 deletions routers/api/v1/api.go
Original file line number Diff line number Diff line change
Expand Up @@ -584,6 +584,8 @@ func RegisterRoutes(m *macaron.Macaron) {
Patch(reqToken(), reqRepoWriter(models.UnitTypeIssues, models.UnitTypePullRequests), bind(api.EditLabelOption{}), repo.EditLabel).
Delete(reqToken(), reqRepoWriter(models.UnitTypeIssues, models.UnitTypePullRequests), repo.DeleteLabel)
})
m.Post("/markdown", bind(api.MarkdownOption{}), misc.Markdown)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an extra changes not related with the title.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I edited the description of the PR after the discussions above but didn't change the title. I can if you want.

This is the same as above where the change is discussed here: https://github.com/go-gitea/gitea/pull/6273/files/102bcffcb6aa60c5bee1054d5c31daa483f2c0a0#discussion_r266227788

Copy link
Contributor

@zeripath zeripath Apr 7, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah the issue is that in order to correctly render markdown issue links you need some context. The old /api/markdown is insufficient - if say I want to render #6273 as a link to go-gitea/gitea#6273 I need to know that I'm rendering for go-gitea/gitea. Now we could just keep the old markdown api but we'd need to add query params etc and add the code to load them - or - we just simply add new markdown render pathways that will do that automatically for us.

m.Post("/markdown/raw", misc.MarkdownRaw)
m.Group("/milestones", func() {
m.Combo("").Get(repo.ListMilestones).
Post(reqToken(), reqRepoWriter(models.UnitTypeIssues, models.UnitTypePullRequests), bind(api.CreateMilestoneOption{}), repo.CreateMilestone)
Expand Down
22 changes: 19 additions & 3 deletions routers/api/v1/misc/markdown.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,16 @@
package misc

import (
"strings"

api "code.gitea.io/sdk/gitea"

"code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/markup/markdown"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/util"

"mvdan.cc/xurls/v2"
)

// Markdown render markdown document to HTML
Expand Down Expand Up @@ -45,11 +49,23 @@ func Markdown(ctx *context.APIContext, form api.MarkdownOption) {
switch form.Mode {
case "gfm":
md := []byte(form.Text)
context := util.URLJoin(setting.AppURL, form.Context)
urlPrefix := form.Context
var meta map[string]string
if !strings.HasPrefix(setting.AppSubURL+"/", urlPrefix) {
// check if urlPrefix is already set to a URL
linkRegex, _ := xurls.StrictMatchingScheme("https?://")
m := linkRegex.FindStringIndex(urlPrefix)
if m == nil {
urlPrefix = util.URLJoin(setting.AppURL, form.Context)
}
}
if ctx.Repo != nil && ctx.Repo.Repository != nil {
meta = ctx.Repo.Repository.ComposeMetas()
}
if form.Wiki {
ctx.Write([]byte(markdown.RenderWiki(md, context, nil)))
ctx.Write([]byte(markdown.RenderWiki(md, urlPrefix, meta)))
} else {
ctx.Write(markdown.Render(md, context, nil))
ctx.Write(markdown.Render(md, urlPrefix, meta))
}
default:
ctx.Write(markdown.RenderRaw([]byte(form.Text), "", false))
Expand Down
2 changes: 1 addition & 1 deletion templates/repo/diff/box.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@
<div class="ui comment form">
<div class="ui top attached tabular menu">
<a class="active write item">{{$.i18n.Tr "write"}}</a>
<a class="preview item" data-url="{{AppSubUrl}}/api/v1/markdown" data-context="{{$.RepoLink}}">{{$.i18n.Tr "preview"}}</a>
<a class="preview item" data-url="{{$.Repository.APIURL}}/markdown" data-context="{{$.RepoLink}}">{{$.i18n.Tr "preview"}}</a>
</div>
<div class="ui bottom attached active write tab segment">
<textarea tabindex="1" name="content"></textarea>
Expand Down
2 changes: 1 addition & 1 deletion templates/repo/diff/comment_form.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
<input type="hidden" name="diff_base_cid">
<div class="ui top attached tabular menu" {{if not $.hidden}}onload="assingMenuAttributes(this)" {{end}}data-write="write" data-preview="preview">
<a class="active item" data-tab="write">{{$.root.i18n.Tr "write"}}</a>
<a class="item" data-tab="preview" data-url="{{$.root.AppSubUrl}}/api/v1/markdown" data-context="{{$.root.RepoLink}}">{{$.root.i18n.Tr "preview"}}</a>
<a class="item" data-tab="preview" data-url="{{$.root.Repository.APIURL}}/markdown" data-context="{{$.root.RepoLink}}">{{$.root.i18n.Tr "preview"}}</a>
</div>
<div class="ui bottom attached active tab segment" data-tab="write">
<div class="field">
Expand Down
4 changes: 2 additions & 2 deletions templates/repo/editor/edit.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@
<div class="ui top attached tabular menu" data-write="write" data-preview="preview" data-diff="diff">
<a class="active item" data-tab="write"><i class="octicon octicon-code"></i> {{if .IsNewFile}}{{.i18n.Tr "repo.editor.new_file"}}{{else}}{{.i18n.Tr "repo.editor.edit_file"}}{{end}}</a>
{{if not .IsNewFile}}
<a class="item" data-tab="preview" data-url="{{AppSubUrl}}/api/v1/markdown" data-context="{{.RepoLink}}/src/{{.BranchNameSubURL | EscapePound}}" data-preview-file-modes="{{.PreviewableFileModes}}"><i class="octicon octicon-eye"></i> {{.i18n.Tr "preview"}}</a>
<a class="item" data-tab="preview" data-url="{{.Repository.APIURL}}/markdown" data-context="{{.RepoLink}}/src/{{.BranchNameSubURL | EscapePound}}" data-preview-file-modes="{{.PreviewableFileModes}}"><i class="octicon octicon-eye"></i> {{.i18n.Tr "preview"}}</a>
<a class="item" data-tab="diff" data-url="{{.RepoLink}}/_preview/{{.BranchName | EscapePound}}/{{.TreePath | EscapePound}}" data-context="{{.BranchLink}}"><i class="octicon octicon-diff"></i> {{.i18n.Tr "repo.editor.preview_changes"}}</a>
{{end}}
</div>
<div class="ui bottom attached active tab segment" data-tab="write">
<textarea id="edit_area" name="content" data-id="repo-{{.Repository.Name}}-{{.TreePath}}"
data-url="{{AppSubUrl}}/api/v1/markdown"
data-url="{{.Repository.APIURL}}/markdown"
data-context="{{.RepoLink}}"
data-markdown-file-exts="{{.MarkdownFileExts}}"
data-line-wrap-extensions="{{.LineWrapExtensions}}"
Expand Down
4 changes: 2 additions & 2 deletions templates/repo/issue/comment_tab.tmpl
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
<div class="field">
<div class="ui top attached tabular menu" data-write="write" data-preview="preview">
<a class="active item" data-tab="write">{{.i18n.Tr "write"}}</a>
<a class="item" data-tab="preview" data-url="{{AppSubUrl}}/api/v1/markdown" data-context="{{.RepoLink}}">{{.i18n.Tr "preview"}}</a>
<a class="item" data-tab="preview" data-url="{{.Repository.APIURL}}/markdown" data-context="{{.RepoLink}}">{{.i18n.Tr "preview"}}</a>
</div>
<div class="ui bottom attached active tab segment" data-tab="write">
<textarea id="content" class="edit_area js-quick-submit" name="content" tabindex="4" data-id="issue-{{.RepoName}}" data-url="{{AppSubUrl}}/api/v1/markdown" data-context="{{.Repo.RepoLink}}">
<textarea id="content" class="edit_area js-quick-submit" name="content" tabindex="4" data-id="issue-{{.RepoName}}" data-url="{{.Repository.APIURL}}/markdown" data-context="{{.Repo.RepoLink}}">
{{if .BodyQuery}}{{.BodyQuery}}{{else if .IssueTemplate}}{{.IssueTemplate}}{{else if .PullRequestTemplate}}{{.PullRequestTemplate}}{{else}}{{.content}}{{end}}</textarea>
</div>
<div class="ui bottom attached tab segment markdown" data-tab="preview">
Expand Down
2 changes: 1 addition & 1 deletion templates/repo/issue/view_content.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@
<div class="ui comment form">
<div class="ui top attached tabular menu">
<a class="active write item">{{$.i18n.Tr "write"}}</a>
<a class="preview item" data-url="{{AppSubUrl}}/api/v1/markdown" data-context="{{$.RepoLink}}">{{$.i18n.Tr "preview"}}</a>
<a class="preview item" data-url="{{$.Repository.APIURL}}/markdown" data-context="{{$.RepoLink}}">{{$.i18n.Tr "preview"}}</a>
</div>
<div class="ui bottom attached active write tab segment">
<textarea tabindex="1" name="content"></textarea>
Expand Down
2 changes: 1 addition & 1 deletion templates/repo/wiki/new.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
<input name="title" value="{{.title}}" autofocus required>
</div>
<div class="field">
<textarea class="js-quick-submit" id="edit_area" name="content" data-id="wiki-{{.title}}" data-url="{{AppSubUrl}}/api/v1/markdown" data-context="{{.RepoLink}}/wiki" required>{{if .PageIsWikiEdit}}{{.content}}{{else}}{{.i18n.Tr "repo.wiki.welcome"}}{{end}}</textarea>
<textarea class="js-quick-submit" id="edit_area" name="content" data-id="wiki-{{.title}}" data-url="{{.Repository.APIURL}}/markdown" data-context="{{.RepoLink}}/wiki" required>{{if .PageIsWikiEdit}}{{.content}}{{else}}{{.i18n.Tr "repo.wiki.welcome"}}{{end}}</textarea>
</div>
<div class="field">
<input name="message" placeholder="{{.i18n.Tr "repo.wiki.default_commit_message"}}">
Expand Down