API endpoints for stars#100
Conversation
Current coverage is 3.03% (diff: 0.00%)@@ master #100 diff @@ ======================================== Files 33 33 Lines 8096 8106 +10 Methods 0 0 Messages 0 0 Branches 0 0 ======================================== Hits 246 246 - Misses 7830 7840 +10 Partials 20 20
|
models/user.go Outdated
| @@ -1121,3 +1121,22 @@ func UnfollowUser(userID, followID int64) (err error){ | |||
| } | |||
| return sess.Commit() | |||
| } | |||
| func GetStarredRepos(userID int64) ([]*Repository, error){ | |||
lunny added the type/feature 
strk commented Nov 7, 2016
Unit tests would be needed for the new APIs |
bkcsoft left a comment
bkcsoft left a comment There was a problem hiding this comment.
Routers shouldn't do cross-route function-calls
routers/api/v1/repo/repo.go Outdated
| @@ -238,7 +238,7 @@ func Migrate(ctx *context.APIContext, form auth.MigrateRepoForm){ | |||
| ctx.JSON(201, repo.APIFormat(&api.Permission{true, true, true})) | |||
| } | |||
| func parseOwnerAndRepo(ctx *context.APIContext) (*models.User, *models.Repository){ | |||
| func ParseOwnerAndRepo(ctx *context.APIContext) (*models.User, *models.Repository){ | |||
There was a problem hiding this comment.
This should be moved to a middleware instead.
There was a problem hiding this comment.
Also I don't think "Parse" is the correct word for what it does either :)
There was a problem hiding this comment.
Please add comments for the function for linting
routers/api/v1/user/star.go Outdated
| "github.com/go-gitea/gitea/modules/context" | ||
| "github.com/go-gitea/gitea/models" | ||
| "github.com/go-gitea/gitea/routers/api/v1/repo" |
DblK left a comment
DblK left a comment There was a problem hiding this comment.
Additional comments for linting would be a plus.
models/user.go Outdated
| @@ -1121,3 +1121,22 @@ func UnfollowUser(userID, followID int64) (err error){ | |||
| } | |||
| return sess.Commit() | |||
| } | |||
| func GetStarredRepos(userID int64) ([]*Repository, error){ | |||
routers/api/v1/repo/repo.go Outdated
| @@ -238,7 +238,7 @@ func Migrate(ctx *context.APIContext, form auth.MigrateRepoForm){ | |||
| ctx.JSON(201, repo.APIFormat(&api.Permission{true, true, true})) | |||
| } | |||
| func parseOwnerAndRepo(ctx *context.APIContext) (*models.User, *models.Repository){ | |||
| func ParseOwnerAndRepo(ctx *context.APIContext) (*models.User, *models.Repository){ | |||
There was a problem hiding this comment.
Please add comments for the function for linting
routers/api/v1/user/star.go Outdated
| "github.com/go-gitea/gitea/routers/api/v1/repo" | ||
| ) | ||
| func getStarredRepos(userID int64) ([]*api.Repository, error){ |
There was a problem hiding this comment.
Since you are at it, please add comments for linting.
| return repos, nil | ||
| } | ||
| func GetStarredRepos(ctx *context.APIContext){ |
There was a problem hiding this comment.
Since you are at it, please add comments for linting.
| ctx.JSON(200, &repos) | ||
| } | ||
| func GetMyStarredRepos(ctx *context.APIContext){ |
There was a problem hiding this comment.
Since you are at it, please add comments for linting.
| ctx.JSON(200, &repos) | ||
| } | ||
| func IsStarring(ctx *context.APIContext){ |
There was a problem hiding this comment.
Since you are at it, please add comments for linting.
| } | ||
| } | ||
| func Star(ctx *context.APIContext){ |
There was a problem hiding this comment.
Since you are at it, please add comments for linting.
| ctx.Status(204) | ||
| } | ||
| func Unstar(ctx *context.APIContext){ |
There was a problem hiding this comment.
Since you are at it, please add comments for linting.
ethantkoenig commented Nov 10, 2016
models/user.go Outdated
| // Get the repos starred by a particular user | ||
| func GetStarredRepos(userID int64) ([]*Repository, error){ | ||
| sess := x.Where("star.uid=?", userID) | ||
| if setting.UsePostgreSQL{ |
There was a problem hiding this comment.
please remove database condition, just use
sess.Join("LEFT", "star", "`repository`.id=star.uid")is OK.
There was a problem hiding this comment.
I don't understand. repository.id is a repo id, and star.uid is a user id; why are we comparing them? I thought that repo ids and user ids were different things. It also seems that your suggestion ignores the userID variable, which doesn't seem right.
Please correct me if I'm wrong.
There was a problem hiding this comment.
sess.Join("LEFT", "star", "`repository`.id=star. repo_id")mistake. I mean we don't need if setting.UsePostgreSQL
lunny commented Nov 11, 2016
Great work! For unit test with database. I think that should be next work. |
ethantkoenig commented Nov 11, 2016
@lunny Fixed |
| sess = sess.Join("LEFT", "star", `"repository".id=star.repo_id`) | ||
| repos := make([]*Repository, 0, 10) | ||
| err := sess.Find(&repos) | ||
| if err != nil{ |
There was a problem hiding this comment.
I think it could be simpler
repos:=make([]*Repository, 0, 10) err:=x.Where("star.uid=?", userID). Join("LEFT", "star", `"repository".id=star.repo_id`). Find(&repos)
ethantkoenigforce-pushed the develop branch 3 times, most recently from cf3fd36 to 9debb0bComparemodels/user.go Outdated
| func GetStarredRepos(userID int64) ([]*Repository, error){ | ||
| repos := make([]*Repository, 0, 10) | ||
| err := x.Where("star.uid=?", userID). | ||
| Join("LEFT", "star", `"repository".id=star.repo_id`). |
There was a problem hiding this comment.
this line should be
Join("LEFT", "star", "`repository`.id=`star`.repo_id").lunny commented Nov 11, 2016
LGTM |
models/user.go Outdated
| func GetStarredRepos(userID int64) ([]*Repository, error){ | ||
| repos := make([]*Repository, 0, 10) | ||
| err := x.Where("star.uid=?", userID). | ||
| Join("LEFT", "star", "`repository`.id=`star`.repo_id"). |
There was a problem hiding this comment.
Does this quoting work for all 3 databases?
There was a problem hiding this comment.
According to @lunny's comment from earlier today, yes.
tboerger commented Nov 11, 2016
LGTM |
lunny commented Nov 12, 2016
bkcsoft left a comment
bkcsoft left a comment There was a problem hiding this comment.
Please read this: https://blog.golang.org/godoc-documenting-go-code especially this:
Notice this comment is a complete sentence that begins with the name of the element it describes. This important convention allows us to generate documentation in a variety of formats, from plain text to HTML to UNIX man pages, and makes it read better when tools truncate it for brevity, such as when they extract the first line or sentence.
modules/context/api.go Outdated
| @@ -70,3 +71,29 @@ func APIContexter() macaron.Handler{ | |||
| c.Map(ctx) | |||
| } | |||
| } | |||
| // Extracts the owner and repo from an APIContext, or issues an error status | |||
There was a problem hiding this comment.
Needs to start with ExtractOwnerAndRepo 😒
routers/api/v1/user/star.go Outdated
| return repos, nil | ||
| } | ||
| // Returns the repos that the user specified by the APIContext has starred |
routers/api/v1/user/star.go Outdated
| ctx.JSON(200, &repos) | ||
| } | ||
| // Returns the repos that the authenticated user has starred |
routers/api/v1/user/star.go Outdated
| ctx.JSON(200, &repos) | ||
| } | ||
| // Returns whether the authenticated is starring the repo |
routers/api/v1/user/star.go Outdated
| "code.gitea.io/gitea/models" | ||
| ) | ||
| // Returns the repos that the user with the specified userID has starred |
There was a problem hiding this comment.
While this function isn't exported (therefore doesn't need a lint-comment), it would be nice to follow conventions everywhere :)
modules/context/api.go Outdated
| return nil, nil | ||
| } | ||
| return owner, repo |
There was a problem hiding this comment.
Preferably, a middleware injects this into ctx.Data["Owner"], ctx.Data["Repo"] and returns nothing. Then it can be used as follows:
m.Group("/starred", context.ExtractOwnerAndRepo(), func(){m.Get("", user.GetMyStarredRepos) m.Group("/:username/:reponame", func(){m.Get("", user.IsStarring) m.Put("", user.Star) m.Delete("", user.Unstar) }) }) see context.RepoRef() for reference 🙂
There was a problem hiding this comment.
@bkcsoft Would be appropriate to use context.RepoAssignment() for populating ctx.Repo.Owner and ctx.Repo.Repository? It seems to already do what we need ExtractRepoAndOwner() to do. This way, we could get rid of ExtractRepoAndOwner() completely.
bkcsoft added issue/needs-feedback ethantkoenig commented Nov 14, 2016
@bkcsoft Unfortunately, Regardless, I've updated |
ethantkoenigforce-pushed the develop branch 2 times, most recently from 3522337 to 197b958Compare
modules/context/api.go Outdated
| @@ -8,6 +8,7 @@ import ( | |||
| "fmt" | |||
| "strings" | |||
| "code.gitea.io/gitea/models" | |||
There was a problem hiding this comment.
this line isn't gofmt. I would merge anyway, as this is minor and we can update with another PR. Probably there's more like that anyway which we can push all together.
There was a problem hiding this comment.
Good catch! I've fixed several gofmt issues (mostly spaces vs. tabs)
models/user.go Outdated
| @@ -1179,3 +1179,15 @@ func UnfollowUser(userID, followID int64) (err error){ | |||
| } | |||
| return sess.Commit() | |||
| } | |||
| // GetStarredRepos returns the repos starred by a particular user | |||
| func GetStarredRepos(userID int64) ([]*Repository, error){ | |||
There was a problem hiding this comment.
This function should return only stars of public repos by default, unless the user is the logged in user.
See #181 as example
There was a problem hiding this comment.
Fixed, thanks for the heads up!
andreynering commented Nov 17, 2016
LGTM |
andreynering added lgtm/done andreynering removed the lgtm/done 
tboerger added the lgtm/done 
9 participants
This pull request adds the following API endpoints:
GET /users/:username/starred: List repositories starred by a userGET /user/starred: List repositories starred by authenticated userGET /user/starred/:owner/:repo: If authenticated user is starring a repositoryPUT /user/starred/:owner/:repo: Star a repository (as authenticated user)DELETE /user/starred/:owner/:repo: Unstar a repository (as authenticated user)Each of these endpoints already exists in the Github API.