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

Tab on user profile to show starred repos #181

Closed
wants to merge 4 commits into from
Closed

Tab on user profile to show starred repos #181

wants to merge 4 commits into from

Conversation

andreynering
Copy link
Contributor

Improvements pending for future PRs:

  • Have a timestamp in star table and order by it
  • Pagination
  • Filter

@andreynering andreynering added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Nov 15, 2016
@codecov-io
Copy link

codecov-io commented Nov 15, 2016

Current coverage is 3.02% (diff: 0.00%)

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

@@            master      #181   diff @@
========================================
  Files           33        34     +1   
  Lines         8106      8119    +13   
  Methods          0         0          
  Messages         0         0          
  Branches         0         0          
========================================
  Hits           246       246          
- Misses        7840      7853    +13   
  Partials        20        20          

Powered by Codecov. Last update bd13c81...4476ee3

_, err = x.Exec("UPDATE `user` SET num_stars = num_stars - 1 WHERE id = ?", userID)
}
return err
}
Copy link
Member

Choose a reason for hiding this comment

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

I think all the SQL should be executed in one transaction.

if setting.UsePostgreSQL {
sess = sess.Join("LEFT", "star", `"user".id = star.uid`)
} else {
sess = sess.Join("LEFT", "star", "user.id = star.uid")
Copy link
Member

Choose a reason for hiding this comment

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

Just use

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

is OK.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lunny Both of your suggestions was not in code I touched. I just moved it from another file. I only wrote the last function.

Should I fix it in this PR or another one?

Copy link
Member

Choose a reason for hiding this comment

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

Both are OK.

@thibaultmeyer
Copy link
Contributor

LGTM

@lunny lunny added this to the 1.0.0 milestone Nov 17, 2016
}

// Star or unstar repository.
func StarRepo(userID, repoID int64, star bool) (err error) {
Copy link
Member

Choose a reason for hiding this comment

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

golint error

"code.gitea.io/gitea/modules/setting"
)

type Star struct {
Copy link
Member

Choose a reason for hiding this comment

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

golint error

return has
}

func (repo *Repository) GetStargazers(page int) ([]*User, error) {
Copy link
Member

Choose a reason for hiding this comment

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

golint

return users, sess.Find(&users)
}

func (u *User) GetStarredRepos(private bool) (repos []*Repository, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

golint

@andreynering
Copy link
Contributor Author

@lunny Updated

sess.Rollback()
return err
}
if _, err := x.Exec("UPDATE `repository` SET num_stars = num_stars + 1 WHERE id = ?", repoID); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

sess.

sess.Rollback()
return err
}
if _, err := x.Exec("UPDATE `user` SET num_stars = num_stars + 1 WHERE id = ?", userID); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

sess.

return nil
}

if _, err := x.Delete(&Star{0, userID, repoID}); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

sess.

sess.Rollback()
return err
}
if _, err := x.Exec("UPDATE `repository` SET num_stars = num_stars - 1 WHERE id = ?", repoID); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

sess.

sess.Rollback()
return err
}
if _, err := x.Exec("UPDATE `user` SET num_stars = num_stars - 1 WHERE id = ?", userID); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

sess.

@andreynering
Copy link
Contributor Author

@lunny You was right. Fixed now.

// StarRepo or unstar repository.
func StarRepo(userID, repoID int64, star bool) error {
sess := x.NewSession()

Copy link
Member

Choose a reason for hiding this comment

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

defer sess.Close()

This must be called and sess.Rollback will be invoked in Close if Commit will not be invoked.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lunny Updated

@tboerger tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Nov 23, 2016
@lunny lunny modified the milestones: 1.1.0, 1.0.0 Nov 24, 2016
@andreynering
Copy link
Contributor Author

Deprecated in favor of #519

@tboerger tboerger added issue/duplicate The issue has already been reported. and removed type/feature Completely new functionality. Can only be merged if feature freeze is not active. lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 16, 2017
@tboerger tboerger removed this from the 1.1.0 milestone Jan 16, 2017
@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
issue/duplicate The issue has already been reported.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants