Improve issue autolinks #6273

mrsdizzie · 2019-03-08T03:01:17Z

Update autolinks to match what github does here:

Issue in same repo: #1
Issue in different repo: org/repo#1

As per discussion below and commits by @zeripath this also introduces a change where we ComposeMetas always returns a username and repo (previously it only returned if using an external issue tracker) and we run ComposeMetas in the Markdown API if in a repo context.

This gives a much more reliable source of user/repo information and eliminates the need for URL parsing and guessing (which is not reliable).

Updated testing for all of this new behavior.

codecov-io · 2019-03-08T03:12:57Z

Codecov Report

Merging #6273 into master will increase coverage by 0.01%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           master    #6273      +/-   ##
==========================================
+ Coverage   40.45%   40.46%   +0.01%     
==========================================
  Files         405      405              
  Lines       54320    54350      +30     
==========================================
+ Hits        21976    21994      +18     
- Misses      29322    29332      +10     
- Partials     3022     3024       +2

Impacted Files	Coverage Δ
modules/markup/html.go	`80.88% <100%> (-2.04%)`	⬇️
models/repo.go	`47.36% <100%> (ø)`	⬆️
routers/api/v1/api.go	`72.4% <100%> (+0.1%)`	⬆️
routers/api/v1/misc/markdown.go	`81.08% <71.42%> (-1.46%)`	⬇️
models/repo_list.go	`66.84% <0%> (-1.06%)`	⬇️
routers/repo/view.go	`42.07% <0%> (+0.99%)`	⬆️
models/unit.go	`14.28% <0%> (+14.28%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3186ef5...2df42df. Read the comment docs.

modules/markup/markdown/markdown_test.go

Update autolinks to match what github does here: Issue in same repo: #1 Issue in different repo: org/repo#1 Fixes go-gitea#6264

Using setting.AppURL here is a more reliable way of parsing the current URL and what other functions in this file seem to use.

Now that we include the user and repo name inside context metas, update various code and tests for this new logic

mrsdizzie · 2019-03-28T19:17:47Z

Updated this PR per above comments. One question: While this slightly modifies some API code, it doesn't actually change anything that is documented so I'm not sure if any documentation needs to be updated/changed.

lunny · 2019-03-29T01:59:54Z

models/repo.go

 func (repo *Repository) ComposeMetas() map[string]string {
-	unit, err := repo.GetUnit(UnitTypeExternalTracker)


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

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.

lunny · 2019-03-29T02:02:34Z

routers/api/v1/api.go

@@ -573,6 +573,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)


This is an extra changes not related with the title.

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

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.

zeripath · 2019-04-11T22:12:17Z

@lunny needs your review

Since go-gitea#6273 was merged, we now have access to proper context metas always. Update SHA generated links to use these instead of urlPrefix. Update tests as well. Fixes go-gitea#4536.

Since #6273 was merged, we now have access to proper context metas always. Update SHA generated links to use these instead of urlPrefix. Update tests as well. Fixes #4536.

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 8, 2019

lunny reviewed Mar 8, 2019

View reviewed changes

modules/markup/markdown/markdown_test.go Show resolved Hide resolved

lafriks added the type/enhancement An improvement of existing functionality label Mar 9, 2019

lafriks added this to the 1.9.0 milestone Mar 9, 2019

mrsdizzie and others added 5 commits March 27, 2019 19:12

Improve issue autolinks

d731520

Update autolinks to match what github does here: Issue in same repo: #1 Issue in different repo: org/repo#1 Fixes go-gitea#6264

Use setting.AppURL when parsing URL

bb5338d

Using setting.AppURL here is a more reliable way of parsing the current URL and what other functions in this file seem to use.

Make ComposeMetas always return a valid context

8f4dd55

Add per repository markdown renderers for better context

aaab9df

Update for use of context metas

102bcff

Now that we include the user and repo name inside context metas, update various code and tests for this new logic

mrsdizzie force-pushed the improve-issue-links branch from d5d0f20 to 102bcff Compare March 28, 2019 18:27

lunny requested changes Mar 29, 2019

View reviewed changes

zeripath approved these changes Apr 7, 2019

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 7, 2019

Merge branch 'master' into improve-issue-links

449f412

lafriks approved these changes Apr 7, 2019

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 7, 2019

Merge branch 'master' into improve-issue-links

bee0d92

Merge branch 'master' into improve-issue-links

d17915b

lunny approved these changes Apr 12, 2019

View reviewed changes

Merge branch 'master' into improve-issue-links

2df42df

lafriks merged commit 3ff0a12 into go-gitea:master Apr 12, 2019

mrsdizzie mentioned this pull request Apr 16, 2019

Use ctx.metas for SHA hash links #6645

Merged

zeripath pushed a commit that referenced this pull request Apr 16, 2019

Use ctx.metas for SHA hash links (#6645)

1bce189

Since #6273 was merged, we now have access to proper context metas always. Update SHA generated links to use these instead of urlPrefix. Update tests as well. Fixes #4536.

stani0502 mentioned this pull request Apr 17, 2019

Feature Request: Cross-link commits mentioning other repos #6664

Closed

7 tasks

go-gitea locked and limited conversation to collaborators Nov 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve issue autolinks #6273

Improve issue autolinks #6273

mrsdizzie commented Mar 8, 2019 •

edited

Loading

codecov-io commented Mar 8, 2019 •

edited

Loading

mrsdizzie commented Mar 28, 2019

lunny Mar 29, 2019

mrsdizzie Mar 29, 2019

lunny Mar 29, 2019

mrsdizzie Mar 29, 2019

zeripath Apr 7, 2019 •

edited

Loading

zeripath commented Apr 11, 2019

		func (repo *Repository) ComposeMetas() map[string]string {
		unit, err := repo.GetUnit(UnitTypeExternalTracker)

Improve issue autolinks #6273

Improve issue autolinks #6273

Conversation

mrsdizzie commented Mar 8, 2019 • edited Loading

codecov-io commented Mar 8, 2019 • edited Loading

Codecov Report

mrsdizzie commented Mar 28, 2019

lunny Mar 29, 2019

Choose a reason for hiding this comment

mrsdizzie Mar 29, 2019

Choose a reason for hiding this comment

lunny Mar 29, 2019

Choose a reason for hiding this comment

mrsdizzie Mar 29, 2019

Choose a reason for hiding this comment

zeripath Apr 7, 2019 • edited Loading

Choose a reason for hiding this comment

zeripath commented Apr 11, 2019

mrsdizzie commented Mar 8, 2019 •

edited

Loading

codecov-io commented Mar 8, 2019 •

edited

Loading

zeripath Apr 7, 2019 •

edited

Loading