Skip to content

Sort repositories by watch status #19

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.

Sign up for GitHub

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

Closed
ghost wants to merge 4 commits into from
Closed

Sort repositories by watch status #19

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a27909a
Sort repositories by user's stars
LefsFlare Sep 21, 2016
0329f15
Sort collaborative repositories by user star
LefsFlare Oct 2, 2016
2a0aec3
Sort mirror repositories by star status
LefsFlare Oct 2, 2016
0eea5f5
Fixed compile errors
LefsFlare Oct 2, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 9 additions & 1 deletion models/access.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import (
"fmt"

"github.com/go-gitea/gitea/modules/log"
"github.com/go-gitea/gitea/modules/setting"
)

type AccessMode int
Expand Down Expand Up @@ -124,13 +125,20 @@ func (u *User) GetRepositoryAccesses() (map[*Repository]AccessMode, error) {
// GetAccessibleRepositories finds repositories which the user has access but does not own.
// If limit is smaller than 1 means returns all found results.
func (user *User) GetAccessibleRepositories(limit int) (repos []*Repository, _ error) {
sess := x.Where("owner_id !=? ", user.ID).Desc("updated_unix")
sess := x.Where("owner_id !=? ", user.ID).Desc("uid").Desc("updated_unix")
Copy link
Copy Markdown
Member

@lunny lunny Nov 3, 2016

Choose a reason for hiding this comment

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

order by uid desc will disable order by updated_unix desc.

Copy link
Copy Markdown
Author

@ghost ghost Nov 4, 2016

Choose a reason for hiding this comment

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

Will it? How do I get it to do an ORDER BY "uid" desc, "updated_unix" desc?

Copy link
Copy Markdown
Member

@lunny lunny Nov 5, 2016

Choose a reason for hiding this comment

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

I mean it will disable on logic not the generated SQL. The code is right.

Copy link
Copy Markdown
Author

@ghost ghost Nov 6, 2016

Choose a reason for hiding this comment

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

huh?

Copy link
Copy Markdown
Member

@strk strk Nov 8, 2016

Choose a reason for hiding this comment

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

@lunny this seems to be the only thing left to resolve here, how do you get multi-column sort with xorm ?

Copy link
Copy Markdown
Member

@lunny lunny Nov 8, 2016

Choose a reason for hiding this comment

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

LGTM then. That's right!

Copy link
Copy Markdown
Contributor

@bkcsoft bkcsoft Nov 8, 2016

Choose a reason for hiding this comment

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

@LefsFlarey @strk seems useless, since UID are never duplicates :)

Copy link
Copy Markdown
Member

@strk strk Nov 8, 2016

Choose a reason for hiding this comment

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

@bkcsoft good point ! Besides, how does that have to do with "watch status" sorting in the title of this PR ?

Copy link
Copy Markdown
Contributor

@thibaultmeyer thibaultmeyer Nov 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

For a better readability and to get the real line / instruction in case of error, please set one XORM instruction per line

eg :

x.Where("owner_id !=? ", user.ID).
        Desc("uid").
        Desc("updated_unix")

or

x.
        Where("owner_id !=? ", user.ID).
        Desc("uid").
        Desc("updated_unix")

if limit > 0 {
sess.Limit(limit)
repos = make([]*Repository, 0, limit)
} else {
repos = make([]*Repository, 0, 10)
}

if setting.UsePostgreSQL {
Copy link
Copy Markdown
Member

@tboerger tboerger Nov 4, 2016

Choose a reason for hiding this comment

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

I really dislike this part to check for the used DB

Copy link
Copy Markdown
Author

@ghost ghost Nov 4, 2016

Choose a reason for hiding this comment

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

It has to be done, PostgreSQL uses different syntax.

Copy link
Copy Markdown
Member

@tboerger tboerger Nov 5, 2016

Choose a reason for hiding this comment

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

But you don't need to do that if you use XORM properly

Copy link
Copy Markdown
Author

@ghost ghost Nov 6, 2016

Choose a reason for hiding this comment

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

Would you like to suggest changes? I replicated what @unknwon did previously.

Copy link
Copy Markdown
Member

@tboerger tboerger Nov 7, 2016

Choose a reason for hiding this comment

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

Just look at the XORM docs, or maybe @lunny can give you a hint since he's the XORM author ;)

Copy link
Copy Markdown
Author

@ghost ghost Nov 8, 2016

Choose a reason for hiding this comment

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

@tboerger I understand where you are coming from but this style of compensating for PostgreSQL is present throughout the existing codebase. I want to propose this be merged first before optimisations be carried out throughout.

Copy link
Copy Markdown
Contributor

@bkcsoft bkcsoft Nov 8, 2016

Choose a reason for hiding this comment

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

There's no need to keep these separate, go for the one PostgreSQL needs, since MySQL and SQLite handles that format as well.

sess = sess.Join("LEFT", "star", `"repository".id=star.repo_id AND star.uid = ?`, user.ID)
} else {
sess = sess.Join("LEFT", "star", "repository.id=star.repo_id AND star.uid = ?", user.ID)
}

return repos, sess.Join("INNER", "access", "access.user_id = ? AND access.repo_id = repository.id", user.ID).Find(&repos)
}

Expand Down
19 changes: 17 additions & 2 deletions models/repo.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1450,7 +1450,14 @@ func GetRepositoryByID(id int64) (*Repository, error) {

// GetUserRepositories returns a list of repositories of given user.
func GetUserRepositories(userID int64, private bool, page, pageSize int) ([]*Repository, error) {
sess := x.Where("owner_id = ?", userID).Desc("updated_unix")
sess := x.Where("owner_id = ?", userID).Desc("uid").Desc("updated_unix")
Copy link
Copy Markdown
Contributor

@thibaultmeyer thibaultmeyer Nov 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

For a better readability and to get the real line / instruction in case of error, please set one XORM instruction per line

eg :

x.Where("owner_id !=? ", user.ID).
        Desc("uid").
        Desc("updated_unix")

or

x.
        Where("owner_id !=? ", user.ID).
        Desc("uid").
        Desc("updated_unix")


if setting.UsePostgreSQL {
Copy link
Copy Markdown
Member

@tboerger tboerger Nov 4, 2016

Choose a reason for hiding this comment

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

I really dislike this part to check for the used DB

Copy link
Copy Markdown
Author

@ghost ghost Nov 4, 2016

Choose a reason for hiding this comment

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

It has to be done, PostgreSQL uses different syntax.

Copy link
Copy Markdown
Member

@tboerger tboerger Nov 5, 2016

Choose a reason for hiding this comment

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

But you don't need to do that if you use XORM properly

Copy link
Copy Markdown
Author

@ghost ghost Nov 6, 2016

Choose a reason for hiding this comment

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

Would you like to suggest changes? I replicated what @unknwon did previously.

Copy link
Copy Markdown
Member

@tboerger tboerger Nov 7, 2016

Choose a reason for hiding this comment

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

Just look at the XORM docs, or maybe @lunny can give you a hint since he's the XORM author ;)

Copy link
Copy Markdown
Author

@ghost ghost Nov 8, 2016

Choose a reason for hiding this comment

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

@tboerger I understand where you are coming from but this style of compensating for PostgreSQL is present throughout the existing codebase. I want to propose this be merged first before optimisations be carried out throughout.

sess = sess.Join("LEFT", "star", `"repository".id=star.repo_id AND star.uid = ?`, userID)
} else {
sess = sess.Join("LEFT", "star", "repository.id=star.repo_id AND star.uid = ?", userID)
}

if !private {
sess.And("is_private=?", false)
}
Expand All @@ -1467,7 +1474,15 @@ func GetUserRepositories(userID int64, private bool, page, pageSize int) ([]*Rep
// GetUserRepositories returns a list of mirror repositories of given user.
func GetUserMirrorRepositories(userID int64) ([]*Repository, error) {
repos := make([]*Repository, 0, 10)
return repos, x.Where("owner_id = ?", userID).And("is_mirror = ?", true).Find(&repos)
sess := x.Where("owner_id = ?", userID).And("is_mirror = ?", true).Desc("uid")
Copy link
Copy Markdown
Contributor

@thibaultmeyer thibaultmeyer Nov 13, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

For a better readability and to get the real line / instruction in case of error, please set one XORM instruction per line

eg :

sess := x.Where("owner_id = ?", userID).
        And("is_mirror = ?", true).
        Desc("uid")

or

sess := x.
        Where("owner_id = ?", userID).
        And("is_mirror = ?", true).
        Desc("uid")


if setting.UsePostgreSQL {
Copy link
Copy Markdown
Member

@tboerger tboerger Nov 4, 2016

Choose a reason for hiding this comment

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

I really dislike this part to check for the used DB

Copy link
Copy Markdown
Author

@ghost ghost Nov 4, 2016

Choose a reason for hiding this comment

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

It has to be done, PostgreSQL uses different syntax.

Copy link
Copy Markdown
Member

@tboerger tboerger Nov 5, 2016

Choose a reason for hiding this comment

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

But you don't need to do that if you use XORM properly

Copy link
Copy Markdown
Author

@ghost ghost Nov 6, 2016

Choose a reason for hiding this comment

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

Would you like to suggest changes? I replicated what @unknwon did previously.

Copy link
Copy Markdown
Member

@tboerger tboerger Nov 7, 2016

Choose a reason for hiding this comment

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

Just look at the XORM docs, or maybe @lunny can give you a hint since he's the XORM author ;)

Copy link
Copy Markdown
Author

@ghost ghost Nov 8, 2016

Choose a reason for hiding this comment

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

@tboerger I understand where you are coming from but this style of compensating for PostgreSQL is present throughout the existing codebase. I want to propose this be merged first before optimisations be carried out throughout.

sess = sess.Join("LEFT", "star", `"repository".id=star.repo_id AND star.uid = ?`, userID)
} else {
sess = sess.Join("LEFT", "star", "repository.id=star.repo_id AND star.uid = ?", userID)
}

return repos, sess.Find(&repos)
}

// GetRecentUpdatedRepositories returns the list of repositories that are recently updated.
Expand Down