API endpoints for stars#100

Merged

andreynering merged 1 commit into

go-gitea:masterfrom

ethantkoenig:develop

Nov 17, 2016

Member

ethantkoenig commented Nov 6, 2016

This pull request adds the following API endpoints:

GET /users/:username/starred: List repositories starred by a user
GET /user/starred: List repositories starred by authenticated user
GET /user/starred/:owner/:repo: If authenticated user is starring a repository
PUT /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.

ethantkoenig mentioned this pull request

API endpoints for stars #46

Closed

ethantkoenig force-pushed the develop branch from 2c8c147 to 89fb055 Compare

November 6, 2016 23:00

codecov-io commented Nov 6, 2016 •

edited

Loading

Current coverage is 3.03% (diff: 0.00%)

Merging #100 into master will decrease coverage by <.01%

@@            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

Powered by Codecov. Last update 871c964...0834e49

lunny requested changes

View reviewed changes

models/user.go Outdated

    
            @@ -1121,3 +1121,22 @@ func UnfollowUser(userID, followID int64) (err error) {
          
              	}

              	return sess.Commit()

              }

              func GetStarredRepos(userID int64) ([]*Repository, error) {

Member

lunny Nov 7, 2016

A join query is more efficiency

Member

DblK Nov 8, 2016

Please add comments too for linting

lunny added the type/feature label

lunny added this to the 1.0.0 milestone

Member

strk commented Nov 7, 2016

Unit tests would be needed for the new APIs

bkcsoft suggested changes

View reviewed changes

Contributor

bkcsoft left a 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) {

Contributor

bkcsoft Nov 7, 2016

This should be moved to a middleware instead.

Contributor

bkcsoft Nov 7, 2016

Also I don't think "Parse" is the correct word for what it does either :)

Member

DblK Nov 8, 2016

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"

Contributor

bkcsoft Nov 7, 2016

Because of this...

DblK suggested changes

View reviewed changes

Member

DblK left a 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) {

Member

DblK Nov 8, 2016

Please add comments too for linting

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) {

Member

DblK Nov 8, 2016

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) {

Member

DblK Nov 8, 2016

Since you are at it, please add comments for linting.

routers/api/v1/user/star.go

    
              	return repos, nil

              }

              func GetStarredRepos(ctx *context.APIContext) {

Member

DblK Nov 8, 2016

Since you are at it, please add comments for linting.

routers/api/v1/user/star.go

    
              	ctx.JSON(200, &repos)

              }

              func GetMyStarredRepos(ctx *context.APIContext) {

Member

DblK Nov 8, 2016

Since you are at it, please add comments for linting.

routers/api/v1/user/star.go

    
              	ctx.JSON(200, &repos)

              }

              func IsStarring(ctx *context.APIContext) {

Member

DblK Nov 8, 2016

Since you are at it, please add comments for linting.

routers/api/v1/user/star.go

    
              	}

              }

              func Star(ctx *context.APIContext) {

Member

DblK Nov 8, 2016

Since you are at it, please add comments for linting.

routers/api/v1/user/star.go

    
              	ctx.Status(204)

              }

              func Unstar(ctx *context.APIContext) {

Member

DblK Nov 8, 2016

Since you are at it, please add comments for linting.

ethantkoenig force-pushed the develop branch from 89fb055 to 25f63a9 Compare

November 10, 2016 21:03

Member Author

ethantkoenig commented Nov 10, 2016

@lunny @bkcsoft @DblK I've added all of the requested changes. The only comment I haven't addressed is @strk's request for unit tests. I'm not sure how to mock out the database for unit tests, and I couldn't find any existing unit tests to use as examples.

lunny requested changes

View reviewed changes

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 {

Member

lunny Nov 11, 2016

please remove database condition, just use

sess.Join("LEFT", "star", "`repository`.id=star.uid")

is OK.

Member Author

ethantkoenig Nov 11, 2016

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.

Member

lunny Nov 11, 2016

sess.Join("LEFT", "star", "`repository`.id=star. repo_id")

mistake. I mean we don't need if setting.UsePostgreSQL

Member

lunny commented Nov 11, 2016

Great work! For unit test with database. I think that should be next work.

ethantkoenig force-pushed the develop branch from 25f63a9 to 399e6c1 Compare

November 11, 2016 15:25

Member Author

ethantkoenig commented Nov 11, 2016

@lunny Fixed if setting.UsePostgreSQL issue.

lunny reviewed

View reviewed changes

models/user.go

    
              	sess = sess.Join("LEFT", "star", `"repository".id=star.repo_id`)

              	repos := make([]*Repository, 0, 10)

              	err := sess.Find(&repos)

              	if err != nil {

Member

lunny Nov 11, 2016

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)

Member Author

ethantkoenig Nov 11, 2016

Fixed.

ethantkoenig force-pushed the develop branch 3 times, most recently from cf3fd36 to 9debb0b Compare

November 11, 2016 15:57

lunny reviewed

View reviewed changes

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`).

Member

lunny Nov 11, 2016

this line should be

Join("LEFT", "star", "`repository`.id=`star`.repo_id").

Member Author

ethantkoenig Nov 11, 2016

Updated

ethantkoenig force-pushed the develop branch from 9debb0b to 218454f Compare

November 11, 2016 16:03

lunny approved these changes

View reviewed changes

Member

lunny commented Nov 11, 2016

LGTM

tboerger reviewed

View reviewed changes

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").

Member

tboerger Nov 11, 2016

Does this quoting work for all 3 databases?

Member Author

ethantkoenig Nov 11, 2016

According to @lunny's comment from earlier today, yes.

Member

tboerger Nov 11, 2016

Ok

Member

lunny Nov 12, 2016

Yes

Member

tboerger commented Nov 11, 2016

LGTM

Member

lunny commented Nov 12, 2016

@bkcsoft @DblK Please review again.

bkcsoft suggested changes

View reviewed changes

Contributor

bkcsoft left a 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

Contributor

bkcsoft Nov 14, 2016

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

Contributor

bkcsoft Nov 14, 2016

Lint-comment

routers/api/v1/user/star.go Outdated

    
              	ctx.JSON(200, &repos)

              }

              // Returns the repos that the authenticated user has starred

Contributor

bkcsoft Nov 14, 2016

Lint-comment

routers/api/v1/user/star.go Outdated

    
              	ctx.JSON(200, &repos)

              }

              // Returns whether the authenticated is starring the repo

Contributor

bkcsoft Nov 14, 2016

Lint-comment

routers/api/v1/user/star.go Outdated

    
              	"code.gitea.io/gitea/models"

              )

              // Returns the repos that the user with the specified userID has starred

Contributor

bkcsoft Nov 14, 2016

While this function isn't exported (therefore doesn't need a lint-comment), it would be nice to follow conventions everywhere :)

bkcsoft reviewed

View reviewed changes

Contributor

bkcsoft left a comment

consistency for middlewares

modules/context/api.go Outdated

    
              		return nil, nil

              	}

              	return owner, repo

Contributor

bkcsoft Nov 14, 2016

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 🙂

Member Author

ethantkoenig Nov 14, 2016

@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.

Contributor

bkcsoft Nov 14, 2016

Yes, please do :)

bkcsoft added issue/needs-feedback pr/wip labels

bkcsoft modified the milestones: 1.1.0, 1.0.0

bkcsoft mentioned this pull request

[Meta] Improvements to the API #64

Closed

ethantkoenig force-pushed the develop branch from 218454f to 656f605 Compare

November 14, 2016 22:34

Member Author

ethantkoenig commented Nov 14, 2016

@bkcsoft Unfortunately, context.RepoAssignment does not seem to work; I made the change and tried running locally, but was hitting errors at the line that calls git.OpenRepository(..) (context/repo.go:233).

Regardless, I've updated ExtractRepoAndOwner() to return a macaron.Handler.

ethantkoenig force-pushed the develop branch 2 times, most recently from 3522337 to 197b958 Compare

November 14, 2016 22:44

metalmatze reviewed

View reviewed changes

modules/context/api.go Outdated

    
            @@ -8,6 +8,7 @@ import (
          
              	"fmt"

              	"strings"

                      "code.gitea.io/gitea/models"

Contributor

metalmatze Nov 15, 2016 •

edited

Loading

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.

Member Author

ethantkoenig Nov 15, 2016

Good catch! I've fixed several gofmt issues (mostly spaces vs. tabs)

ethantkoenig force-pushed the develop branch from 197b958 to f216e32 Compare

November 15, 2016 14:47

andreynering reviewed

View reviewed changes

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) {

Contributor

andreynering Nov 16, 2016

This function should return only stars of public repos by default, unless the user is the logged in user.

See #181 as example

Member Author

ethantkoenig Nov 17, 2016

Fixed, thanks for the heads up!


          API endpoints for stars

0834e49

ethantkoenig force-pushed the develop branch from f216e32 to 0834e49 Compare

November 17, 2016 03:52

Contributor

andreynering commented Nov 17, 2016

LGTM

andreynering added lgtm/done and removed issue/needs-feedback pr/wip labels

andreynering merged commit d884312 into go-gitea:master

andreynering removed the lgtm/done label

lunny modified the milestones: 1.0.0, 1.1.0

tboerger added the lgtm/done label

lunny added the kind/api label

grinat mentioned this pull request

feat: highlight issue references with : #8101

Merged

go-gitea locked and limited conversation to collaborators

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

lunny lunny approved these changes

+5 more reviewers

tboerger tboerger left review comments

metalmatze metalmatze left review comments

bkcsoft bkcsoft requested changes

andreynering andreynering left review comments

DblK DblK requested changes

Labels

lgtm/done type/feature