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

feat: expose url field on issue api. #982

Merged
merged 5 commits into from
Mar 3, 2017
Merged

feat: expose url field on issue api. #982

merged 5 commits into from
Mar 3, 2017

Conversation

appleboy
Copy link
Member

fix #402

@lunny lunny added this to the 1.2.0 milestone Feb 20, 2017
@lunny lunny added modifies/api This PR adds API routes or modifies them type/enhancement An improvement of existing functionality labels Feb 20, 2017
@appleboy
Copy link
Member Author

This would be better include in 1.1.0 release.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 20, 2017
@lunny
Copy link
Member

lunny commented Feb 20, 2017

Let's decided it before release

@appleboy
Copy link
Member Author

@lunny OK

@lunny
Copy link
Member

lunny commented Feb 26, 2017

trusted LGTM

@tboerger tboerger 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 Feb 26, 2017
@lunny
Copy link
Member

lunny commented Feb 26, 2017

@rymai @bkcsoft could you confirm this?

models/issue.go Outdated
@@ -246,6 +246,7 @@ func (issue *Issue) APIFormat() *api.Issue {

apiIssue := &api.Issue{
ID: issue.ID,
URL: issue.HTMLURL(),
Copy link

Choose a reason for hiding this comment

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

I'm not sure about that. From GitHub's API it seems that it should be the API URL to get the issue, not the HTML URL: https://developer.github.com/v3/issues/#get-a-single-issue

"url": "https://api.github.com/repos/octocat/Hello-World/issues/1347",

Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't we define the API URL for Gitea? cc @lunny

Copy link
Member

Choose a reason for hiding this comment

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

Indeed it is the API-URL :) and that function doesn't exist, so we need a issue.APIURL() as well (should be fairly simple)

Copy link
Member

Choose a reason for hiding this comment

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

@appleboy It seems there is not.

Copy link
Member Author

Choose a reason for hiding this comment

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

But we didn't have API URL in app.ini, we only have ROOT_URL config now.

Copy link
Member

Choose a reason for hiding this comment

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

We don't support a separate api url yet, so just take the regular url and append the api path

Copy link
Member

Choose a reason for hiding this comment

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

func (i *Issue) APIURL() string {
  return path.Join(settings.AppURL, "/api/v1/repos/", issue.Repo.FullName(), "issues", issue.ID)
}

Copy link
Member

Choose a reason for hiding this comment

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

Preferably there should be 2 new functions:

func (repo *Repo) APIURL() string {
  return path.Join(settings.AppURL, "/api/v1/repos/", repo.FullName())
}

func (issue *Issue) APIURL() string {
  return path.Join(issue.Repo.APIURL(), "issues", issue.ID)
}

Copy link
Member

Choose a reason for hiding this comment

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

@bkcsoft maybe you can send a PR and we can close this one.

Copy link
Member Author

Choose a reason for hiding this comment

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

I will update this PR.

@lunny
Copy link
Member

lunny commented Feb 27, 2017

build failed.

@bkcsoft
Copy link
Member

bkcsoft commented Feb 27, 2017

LGTM when the build succeeds :)

@tboerger tboerger 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 Feb 27, 2017
@bkcsoft
Copy link
Member

bkcsoft commented Feb 27, 2017

models/issue.go:205: undefined: path in path.Join
models/issue.go:205: issue.Repo.APIURL undefined (type *Repository has no field or method APIURL)
models/repo.go:268: undefined: Repo
models/repo.go:269: undefined: settings in settings.AppURL
make: *** [Makefile:71: test] Error 1

Need to import "path"

models/repo.go Outdated
@@ -264,6 +264,11 @@ func (repo *Repository) HTMLURL() string {
return setting.AppURL + repo.FullName()
}

// APIURL returns the repository API URL
func (repo *Repo) APIURL() string {
return path.Join(settings.AppURL, "/api/v1/repos/", repo.FullName())
Copy link
Member

Choose a reason for hiding this comment

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

Does appurl include the sub path of gitea is not running on the top folder?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch. path.Join will convert double slash to single slash.

@appleboy
Copy link
Member Author

I will fix errors. Please wait.

@appleboy
Copy link
Member Author

@tboerger @bkcsoft Done. Please review again.

@appleboy
Copy link
Member Author

Add unit testing.

models/repo.go Outdated
@@ -264,6 +264,11 @@ func (repo *Repository) HTMLURL() string {
return setting.AppURL + repo.FullName()
}

// APIURL returns the repository API URL
func (repo *Repository) APIURL() string {
return strings.TrimRight(setting.AppURL, "/") + "/" + path.Join("api/v1/repos", repo.FullName())
Copy link
Member Author

Choose a reason for hiding this comment

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

Support setting.AppURL include end slash or not

Copy link
Member

Choose a reason for hiding this comment

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

No need for that, path.Join does that automagically 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

@bkcsoft path.Join will replace // with /

setting.AppURL = "https://try.gitea.io/"
path.Join(setting.AppURL, "api/v1/repos", repo.FullName())
# output: https:/try.gitea.io/api/v1/repos/user01/test

see https://play.golang.org/p/-0CJvOEZ5N

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, path.Join destroys absolute urls.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see how that is relevant, it's supposed to be an absolute URL, not a relative one.

Works just fine IMO: https://play.golang.org/p/Rc734RIuJu

Copy link
Member Author

Choose a reason for hiding this comment

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

@bkcsoft Please see the result from your url

https:/try.gitea.io/api/v1/repos/user01/repo01

Wrong: https:/
Correct: https://

@tboerger
Copy link
Member

LGTM

models/issue.go Outdated
@@ -200,6 +201,11 @@ func (issue *Issue) GetIsRead(userID int64) error {
return nil
}

// APIURL returns the absolute APIURL to this issue.
func (issue *Issue) APIURL() string {
return strings.TrimRight(issue.Repo.APIURL(), "/") + "/" + path.Join("issues", fmt.Sprint(issue.ID))
Copy link
Member

Choose a reason for hiding this comment

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

Unnecessary, path.Join trims extra slashes

Copy link
Member Author

Choose a reason for hiding this comment

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

@lunny
Copy link
Member

lunny commented Feb 28, 2017

conflicted.

@appleboy
Copy link
Member Author

@lunny fix conflicts and wait for build status.

@tboerger
Copy link
Member

tboerger commented Mar 1, 2017

LGTM

@lunny lunny modified the milestones: 1.1.0, 1.2.0 Mar 2, 2017
@lunny
Copy link
Member

lunny commented Mar 2, 2017

@appleboy maybe you should rebase and push --force.

@appleboy
Copy link
Member Author

appleboy commented Mar 2, 2017

@lunny I always use git pull --rebase origin master on target branch.

@@ -125,3 +125,10 @@ func TestForkRepository(t *testing.T) {
assert.Error(t, err)
assert.True(t, IsErrRepoAlreadyExist(err))
}

func TestRepoAPIURL(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

lint?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nothing output lint errors after execute make lint command.

Copy link
Member

Choose a reason for hiding this comment

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

Linting doesn't apply to *_test.go 🙂

@@ -10,7 +10,7 @@ import (
"github.com/stretchr/testify/assert"
)

func TestIssue_ReplaceLabels(t *testing.T) {
func TestIssueReplaceLabels(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

Why was this test renamed?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

@ethantkoenig ethantkoenig Mar 3, 2017

Choose a reason for hiding this comment

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

I thought that the naming conventions for tests were the same as the convetions for testable examples (https://blog.golang.org/examples)? Was I mistaken?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ethantkoenig revert.

@appleboy
Copy link
Member Author

appleboy commented Mar 3, 2017

@lunny @bkcsoft remove trim from API URL.

@lunny
Copy link
Member

lunny commented Mar 3, 2017

need @ethantkoenig confirmation.

@lunny lunny merged commit d76d67d into go-gitea:master Mar 3, 2017
@appleboy appleboy deleted the api branch March 4, 2017 06:31
@rymai
Copy link

rymai commented Mar 5, 2017

Thanks @appleboy! ❤️

@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/api This PR adds API routes or modifies them type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[API] Issue and pull-request should expose url
6 participants