Skip to content

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

models/star.go Outdated
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.

models/star.go Outdated
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
models/star.go Outdated
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

models/star.go Outdated
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

models/star.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

golint

models/star.go Outdated
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

models/star.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

sess.

models/star.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

sess.

models/star.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

sess.

models/star.go Outdated
Copy link
Member

Choose a reason for hiding this comment

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

sess.

models/star.go Outdated
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