API: support '/orgs/:org/repos' #2047

awwalker · 2017-06-24T05:57:41Z

This is in favor of #1691 in order to fix #494.

Sorry for opening a new PR, but this new approach seems to address all of the concerns brought up in #494. I also added an integration test, but was unsure of how to create a test organization in the integrations directory. If this PR is approved of over #494 I'll close that one and we can focus on this one. Otherwise I'll go back to fixing that one and close this one.

appleboy · 2017-06-24T06:35:32Z

build fail.

ethantkoenig · 2017-06-24T15:32:54Z

routers/api/v1/user/repo.go

+	//       200: RepositoryList
+	//       500: error
+
+	org := GetOrgByParams(ctx)


Not necessary, can just use ctx.Org (which is populated by the orgAssignment(true) handler)

awwalker · 2017-06-24T20:33:24Z

I think build failure is due to the added test case. I need to make an org inside the integrations test somehow but wasn't sure of how to do that. Any guidance would be helpful

ethantkoenig · 2017-06-24T20:41:31Z

integrations/api_repo_test.go

+func TestAPIOrgReposNotLogin(t *testing.T) {
+	assert.NoError(t, models.LoadFixtures())
+
+	req := NewRequest(t, "GET", "/api/v1/orgs/org1/repos")


@awwalker You need to be authenticated for this API endpoint; that's why the test is failing. Use the loginUser(..) helper function, and take a look at other API integrations test (a lot of them already use loginUser(..)).

lafriks · 2017-06-24T23:43:18Z

routers/api/v1/user/repo.go

+	//       200: RepositoryList
+	//       500: error
+
+	listUserRepos(ctx, ctx.Org)


ctx.Org.Organization must be used

lunny · 2017-06-28T15:24:54Z

@awwalker See https://github.com/go-gitea/gitea/blob/master/models/fixtures/user.yml, user3, user6 and user7 are organizations and there is no user named org1. So you can use /api/v1/orgs/user3/repos and login with user2. And it should has 1 repo repo3.

lafriks · 2017-07-05T07:03:06Z

integrations/api_repo_test.go

+	req := NewRequest(t, "GET", "/api/v1/orgs/user3/repos")
+	session := loginUser(t, "user2")
+	resp := session.MakeRequest(t, req)
+	assert.EqualValues(t, http.StatusOK, resp.HeaderCode)


Please check also if it returns correct repository count

sapk · 2017-07-06T09:02:32Z

@awwalker Can you exec make generate-swagger to update the public/swagger.v1.json and commit it. It was remove from build (go generate) to limit extra work for Drone (#1791).

sapk

A little corner case to check and a little simplification. For the rest LGTM.

sapk · 2017-07-06T09:23:21Z

routers/api/v1/user/repo.go

+	//       200: RepositoryList
+	//       500: error
+
+	listUserRepos(ctx, ctx.Org.Organization)


I have made a PR #2117 to enforce that ctx.Org.Organization is an org. So this detail could be ignore and the check up under /users/{username}/repo could be make in another PR.

Awesome thanks

sapk · 2017-07-06T09:25:19Z

routers/api/v1/api.go

@@ -455,6 +455,9 @@ func RegisterRoutes(m *macaron.Macaron) {
 				m.Combo("/:username").Get(org.IsMember).
 					Delete(reqToken(), reqOrgOwnership(), org.DeleteMember)
 			})
+			m.Group("/repos", func() {


Why a group for only one route ? Just do m.Get("/repos", user.ListOrgRepos).

Just as a side note during review context we should use a combo and migrate https://github.com/go-gitea/gitea/blob/master/routers/api/v1/api.go#L335 that does not currently follow the normal path. Still, this will maybe break few things so I will wait for the advice of other before doing that. Maybe in an other PR.

ethantkoenig · 2017-07-06T16:53:16Z

integrations/api_repo_test.go

@@ -10,6 +10,7 @@ import (

 	"code.gitea.io/gitea/models"

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


Should be next to other code.gitea.io imports (see the styleguide)

Hm this was gofmt's choice because I aliased it. I'll edit it for the styleguide though

ethantkoenig

@awwalker The new GET /orgs/:org/repos endpoint has many of the same bugs that the GET /users/:username/repos has (see #2125)

awwalker · 2017-07-09T22:12:35Z

This should pass drone once #2125 is in

ethantkoenig · 2017-07-11T01:53:32Z

@awwalker #2125 has been merged, so you'll need to rebase this on top of master

awwalker · 2017-07-11T16:40:41Z

@ethantkoenig rebased and ready for review

bkcsoft · 2017-07-12T01:35:38Z

@awwalker Conflicts again

awwalker · 2017-07-12T20:56:19Z

@bkcsoft better....for now!

bkcsoft · 2017-07-13T08:33:13Z

LGTM

sapk · 2017-07-13T09:33:13Z

LGTM

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jun 24, 2017

appleboy added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jun 24, 2017

appleboy added this to the 1.2.0 milestone Jun 24, 2017

ethantkoenig suggested changes Jun 24, 2017

View reviewed changes

lafriks reviewed Jun 24, 2017

View reviewed changes

lunny modified the milestones: 1.3.0, 1.2.0 Jul 1, 2017

lafriks reviewed Jul 5, 2017

View reviewed changes

sapk reviewed Jul 6, 2017

View reviewed changes

ethantkoenig suggested changes Jul 6, 2017

View reviewed changes

ethantkoenig suggested changes Jul 7, 2017

View reviewed changes

awwalker force-pushed the org branch from d10f4fb to 60b16c6 Compare July 9, 2017 22:11

awwalker force-pushed the org branch from 60b16c6 to a4095d6 Compare July 11, 2017 16:21

awwalker added 8 commits July 12, 2017 13:29

API: support '/orgs/:org/repos'

ec83c9c

use ctx.Org instead of OrgFromParams

f471e48

login for test and use ctx.Org.Organization

bf8a451

use proper org/user in test

b7a1dfd

validate correct num of repos returned

f4c8365

update swagger.json

b351b38

format imports/revise route

5a6522b

reformat integration test

a49fdee

remove print statements

a0c2d77

awwalker force-pushed the org branch from 7837184 to a0c2d77 Compare July 12, 2017 20:55

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 Jul 13, 2017

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 Jul 13, 2017

bkcsoft merged commit 6a3c037 into go-gitea:master Jul 13, 2017

lunny mentioned this pull request Jul 13, 2017

API: support '/orgs/:org/repos' #1691

Closed

lunny added the modifies/api This PR adds API routes or modifies them label Jul 13, 2017

lunny modified the milestones: 1.2.0, 1.3.0 Jul 13, 2017

sapk mentioned this pull request Feb 25, 2018

API support for listing tags and fetching tag details #2738

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API: support '/orgs/:org/repos' #2047

API: support '/orgs/:org/repos' #2047

awwalker commented Jun 24, 2017

appleboy commented Jun 24, 2017

ethantkoenig Jun 24, 2017

awwalker commented Jun 24, 2017

ethantkoenig Jun 24, 2017

lafriks Jun 24, 2017

lunny commented Jun 28, 2017

lafriks Jul 5, 2017

awwalker Jul 6, 2017

sapk commented Jul 6, 2017

sapk left a comment

sapk Jul 6, 2017 •

edited

Loading

sapk Jul 6, 2017

awwalker Jul 6, 2017

sapk Jul 6, 2017

sapk Jul 6, 2017

ethantkoenig Jul 6, 2017

awwalker Jul 6, 2017

ethantkoenig left a comment

awwalker commented Jul 9, 2017

ethantkoenig commented Jul 11, 2017

awwalker commented Jul 11, 2017

bkcsoft commented Jul 12, 2017

awwalker commented Jul 12, 2017

bkcsoft commented Jul 13, 2017

sapk commented Jul 13, 2017

		@@ -10,6 +10,7 @@ import (

		"code.gitea.io/gitea/models"

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

API: support '/orgs/:org/repos' #2047

API: support '/orgs/:org/repos' #2047

Conversation

awwalker commented Jun 24, 2017

appleboy commented Jun 24, 2017

Choose a reason for hiding this comment

awwalker commented Jun 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Jun 28, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sapk commented Jul 6, 2017

sapk left a comment

Choose a reason for hiding this comment

sapk Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig left a comment

Choose a reason for hiding this comment

awwalker commented Jul 9, 2017

ethantkoenig commented Jul 11, 2017

awwalker commented Jul 11, 2017

bkcsoft commented Jul 12, 2017

awwalker commented Jul 12, 2017

bkcsoft commented Jul 13, 2017

sapk commented Jul 13, 2017

sapk Jul 6, 2017 •

edited

Loading