Ordering organizations and users by name #49

thibaultmeyer · 2016-11-03T15:40:03Z

Relative to the issue #3845, this Pull Request add modifications to ordering by name Organizations and User Accounts.

It's more user friendly and make search more easy when organizations and user accounts are ordered by name rather than last update datetime.

I added new function GetUsersByIDs to decrease the number of SQL queries by using IN predicate rather than just running 1 SQL query per users.

codecov-io · 2016-11-03T16:00:47Z

Current coverage is 2.24% (diff: 0.00%)

Merging #49 into master will increase coverage by 0.03%

@@            master       #49   diff @@
========================================
  Files           31        32     +1   
  Lines         7508      7530    +22   
  Methods          0         0          
  Messages         0         0          
  Branches         0         0          
========================================
+ Hits           166       169     +3   
- Misses        7326      7344    +18   
- Partials        16        17     +1

Powered by Codecov. Last update a0e54c0...047eefc

tboerger

Just minor revert of changes.

tboerger · 2016-11-03T19:08:54Z

templates/admin/org/list.tmpl

@@ -15,7 +15,6 @@
 					<table class="ui very basic striped table">
 						<thead>
 							<tr>
-								<th>ID</th>


Please keep that, this is an admin view and I would print there as much information as possible.

ok, but trust me, when you have more than 1029 users (we use it in corporate environment), ID is just an annoyance, take a lot of place because the website CSS theme is not fluid and is useless to do admin maintenance tasks.

are you sure you want ID come back ?

here a screen :

A proper listing is a different story, but I would like to keep this information.

@tboerger Done. "ID" column restored

tboerger · 2016-11-03T19:09:02Z

templates/admin/repo/list.tmpl

@@ -15,7 +15,6 @@
 					<table class="ui very basic striped table">
 						<thead>
 							<tr>
-								<th>ID</th>


Same as above.

tboerger · 2016-11-03T19:09:10Z

templates/admin/user/list.tmpl

@@ -18,7 +18,6 @@
 					<table class="ui very basic striped table">
 						<thead>
 							<tr>
-								<th>ID</th>


Same as above.

DblK · 2016-11-03T20:38:15Z

Can you rebase it?

thibaultmeyer · 2016-11-03T21:01:34Z

You could choose "Rebase and Merge" during the merge process : https://github.com/blog/2243-rebase-and-merge-pull-requests

bkcsoft · 2016-11-03T22:19:55Z

models/org.go

@@ -495,7 +499,7 @@ func (org *User) GetUserRepositories(userID int64, page, pageSize int) ([]*Repos
 	repos := make([]*Repository, 0, pageSize)
 	// FIXME: use XORM chain operations instead of raw SQL.
 	if err = x.Sql(fmt.Sprintf(`SELECT repository.* FROM repository


Do we really need to do raw SQL-query here?

Probably not, but this code is not part of the current pull request. It could be a part of another one (eg : rewrite and optimize all SQL queries)

Well, if you're making that line better, you could just as well make it even better 😄 Saves us another PR 👍

I prefer 1 PR = 1 problem solved. This is more simple for everyone and easily to review.

I will replace all raw queries on model files with XORM chain operations in another PR.

bkcsoft · 2016-11-03T22:20:08Z

models/org.go

@@ -507,7 +511,7 @@ func (org *User) GetUserRepositories(userID int64, page, pageSize int) ([]*Repos
 	}

 	results, err := x.Query(fmt.Sprintf(`SELECT repository.id FROM repository


Same as above

bkcsoft · 2016-11-03T22:20:15Z

models/org.go

@@ -534,7 +538,7 @@ func (org *User) GetUserMirrorRepositories(userID int64) ([]*Repository, error)

 	repos := make([]*Repository, 0, 10)
 	if err = x.Sql(fmt.Sprintf(`SELECT repository.* FROM repository


Same as above

bkcsoft · 2016-11-03T22:22:32Z

models/org.go

-	sess := x.Where("uid=?", uid)
+	sess := x.
+		Join("LEFT", "user", `"org_user".org_id="user".id`).
+		Where("uid=?", uid)


This should probably be "org_user.uid=?" no?

It seem correct, Here we want organizations ordered by name, and in GOGS, organization are just a simple user.

I meant in the Where-clause, since it's a JOIN 🙂

since the field uid only exists on the table org_user, the WHERE clause is good. But if you wan't I can add "org_user.uid=?" ?? in all cases it will working.

Better be safe than sorry, and consistent (also "org_id" is only in org_user but is explicitly listed.
BTW, I don't think you need to quote "org_user" and "user"...)

strk · 2016-11-06T09:31:22Z

models/org.go

@@ -298,12 +296,16 @@ func GetOwnedOrgsByUserIDDesc(userID int64, desc string) ([]*User, error) {
 // GetOrgUsersByUserID returns all organization-user relations by user ID.
 func GetOrgUsersByUserID(uid int64, all bool) ([]*OrgUser, error) {
 	ous := make([]*OrgUser, 0, 10)
-	sess := x.Where("uid=?", uid)
+	sess := x.
+		Join("LEFT", "user", `"org_user".org_id="user".id`).


No JOIN should be needed here, as user id is already in the OrgUser table ?

Join is needed to order by Name

andreynering · 2016-11-08T22:48:07Z

models/org.go

@@ -453,6 +455,8 @@ func (org *User) getUserTeams(e Engine, userID int64, cols ...string) ([]*Team,
 	return teams, e.Where("team_user.org_id = ?", org.ID).
 		And("team_user.uid = ?", userID).
 		Join("INNER", "team_user", "team_user.team_id = team.id").
+		Join("INNER", "user", `"user".id=team_user.uid`).


Hmm.. Ideally, we should not quote column names. Quoting is different between different databases. PostgreSQL uses ", MySQL uses `, etc. Where this is necessary, I think using this Xorm function should work.

Same for other SQLs.

XORM will use as the uniform quote and will replace it as database's self quote. So just use,

Join("INNER", "user", "`user`.id=team_user.uid").

lunny · 2016-11-09T08:39:02Z

models/org.go

 	}
+	org.Members, _ = GetUsersByIDs(ids)


but why ignore the errors?

cause in all cases GetUsersByIDs return an array.

But if get users failed, we should return error to external of function. I think this should be

org.members, err = GetUsersByIDs(ids) return err

Agreed, always return errors

lunny · 2016-11-09T08:53:39Z

models/org.go

@@ -453,6 +455,8 @@ func (org *User) getUserTeams(e Engine, userID int64, cols ...string) ([]*Team,
 	return teams, e.Where("team_user.org_id = ?", org.ID).
 		And("team_user.uid = ?", userID).
 		Join("INNER", "team_user", "team_user.team_id = team.id").
+		Join("INNER", "user", `"user".id=team_user.uid`).
+		Asc("user.name").


need quote on user too

invalid

bkcsoft · 2016-11-09T18:26:54Z

models/org.go

+		ids[i] = ou.Uid
+	}
+	org.Members, err = GetUsersByIDs(ids)
+	if err != nil {


no need, just return err...

you right. Fixed

codecov-io · 2016-11-09T21:44:12Z

Current coverage is 3.13% (diff: 0.00%)

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

@@            master       #49   diff @@
========================================
  Files           33        33          
  Lines         7823      7841    +18   
  Methods          0         0          
  Messages         0         0          
  Branches         0         0          
========================================
  Hits           246       246          
- Misses        7557      7575    +18   
  Partials        20        20

Powered by Codecov. Last update 145648a...90b4883

ID column restored on admin console

lunny · 2016-11-10T06:54:06Z

models/org.go

 	if !all {
 		// Only show public organizations
 		sess.And("is_public=?", true)
 	}
-	err := sess.Find(&ous)
+	err := sess.
+		Asc("name").


so this should be

Asc("`user`.name")`

It is the FROM field in mailer configuration that needs be used, not the USER field, which is for authentication. Closes gogs/gogs#3615

* Added badges to the README * Restructured the travis config

* Dropped codebeat, we got go reportcard * Dropped gopm file, we are using govendor * Dropped chinese readme, this should be part of the docs * Dropped bra config, not really used * Dropped packager, we will provide our own packages

…or)", replaced with custom name

Creating a notice instead

😊

- Handle XORM error - Quote columns in XORM error - Restore ID column on admin console

lunny · 2016-11-10T07:42:55Z

something wrong. @joubertredrat

thibaultmeyer · 2016-11-10T07:45:31Z

Yeah i dont know why... I have just rebase to avoid multiple commit. Git show get right commit but github just explode. I will recreate PR within few minutes

thibaultmeyer · 2016-11-10T07:57:10Z

Something goes wrong with rebase, I close this issue, delete GOGS repos and fork Gitea. I will re-create PR without few hours

joubertredrat · 2016-11-10T10:41:46Z

@lunny strange, I don't have relation with this PR and my commit listed already have PRs in #5 and #54

@0xBAADF00D for me was same, was necessary to remove gogs fork and make new fork from Gitea

tboerger added this to the 1.0.0 milestone Nov 3, 2016

tboerger added the type/enhancement An improvement of existing functionality label Nov 3, 2016

tboerger previously requested changes Nov 3, 2016

View reviewed changes

bkcsoft suggested changes Nov 3, 2016

View reviewed changes

bkcsoft reviewed Nov 3, 2016

View reviewed changes

strk reviewed Nov 6, 2016

View reviewed changes

strk modified the milestones: 1.1.0, 1.0.0 Nov 6, 2016

andreynering reviewed Nov 8, 2016

View reviewed changes

lunny requested changes Nov 9, 2016

View reviewed changes

lunny reviewed Nov 9, 2016

View reviewed changes

bkcsoft previously approved these changes Nov 9, 2016

View reviewed changes

bkcsoft suggested changes Nov 9, 2016

View reviewed changes

thibaultmeyer self-assigned this Nov 9, 2016

lunny approved these changes Nov 10, 2016

View reviewed changes

lunny reviewed Nov 10, 2016

View reviewed changes

joubertredrat and others added 8 commits November 10, 2016 08:39

Small UI fixes on full width form titles

cc1e822

Use proper url for libravatar dep

c5f3692

Fix sender of issue notifications

e683dee

It is the FROM field in mailer configuration that needs be used, not the USER field, which is for authentication. Closes gogs/gogs#3615

config lgtm to disable maintainer to LGTM himself's PR

a006984

Fix SHA regexp and commit comparisons

42883f2

Restore whitespace

e8b45fe

Change import reference to match gitea instead of gogs (#37)

ac93682

Badges and travis testing (#40)

2fbebad

* Added badges to the README * Restructured the travis config

tboerger and others added 18 commits November 10, 2016 08:39

Minor cleanups, look at the specific commits (#67)

a7943a3

* Dropped codebeat, we got go reportcard * Dropped gopm file, we are using govendor * Dropped chinese readme, this should be part of the docs * Dropped bra config, not really used * Dropped packager, we will provide our own packages

Executed go fmt for all files

126b43a

Simply made go vet happy, fixed tags

e681081

Vet complains about "should have signature MarshalJSON() ([]byte, err…

c57de62

…or)", replaced with custom name

Linter doesn't like uppercase variables

6c272ac

Made linter happy in cmd folder

82969eb

Regenerated bindata

5142f94

Added a common golang gitignore

96d7ede

Totally refactored the makefile and adjusted travis config

d66249c

Fixed "net.UnixAddr composite literal uses unkeyed fields"

2930243

Dropped outdated build scripts

5cdae21

Extended gitignore

b52c15b

Use tabs in Makefiles

747a11a

Drop indent-size from Makefile section

f16c963

Fix 500 when repo has invalid .editorconfig

c4d1c3c

Creating a notice instead

Add myself to the MAINTAINERS (#82)

bcd8d55

😊

Refactor editorconfig middleware

0d02353

Minor changes

c0dc11c

- Handle XORM error - Quote columns in XORM error - Restore ID column on admin console

thibaultmeyer closed this Nov 10, 2016

thibaultmeyer mentioned this pull request Nov 10, 2016

Ordering organizations and users by name #130

Merged

lunny removed this from the 1.1.0 milestone Nov 10, 2016

tboerger added reviewed/invalid and removed type/enhancement An improvement of existing functionality labels Nov 11, 2016

go-gitea locked and limited conversation to collaborators Nov 23, 2020

delvh added issue/not-a-bug The reported issue is the intended behavior or the problem is not inside Gitea and removed reviewed/invalid labels Oct 7, 2023

		@@ -507,7 +511,7 @@ func (org User) GetUserRepositories(userID int64, page, pageSize int) ([]Repos
		}

		results, err := x.Query(fmt.Sprintf(`SELECT repository.id FROM repository

		@@ -534,7 +538,7 @@ func (org User) GetUserMirrorRepositories(userID int64) ([]Repository, error)

		repos := make([]*Repository, 0, 10)
		if err = x.Sql(fmt.Sprintf(`SELECT repository.* FROM repository

Ordering organizations and users by name #49

Ordering organizations and users by name #49

Conversation

thibaultmeyer commented Nov 3, 2016

codecov-io commented Nov 3, 2016 • edited Loading

Current coverage is 2.24% (diff: 0.00%)

tboerger left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultmeyer Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultmeyer Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DblK commented Nov 3, 2016

thibaultmeyer commented Nov 3, 2016

Choose a reason for hiding this comment

thibaultmeyer Nov 3, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultmeyer Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thibaultmeyer Nov 4, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreynering Nov 8, 2016 • edited Loading

Choose a reason for hiding this comment

lunny Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Nov 9, 2016 • edited Loading

Current coverage is 3.13% (diff: 0.00%)

lunny Nov 10, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Nov 10, 2016

thibaultmeyer commented Nov 10, 2016

thibaultmeyer commented Nov 10, 2016 • edited Loading

joubertredrat commented Nov 10, 2016 • edited Loading

codecov-io commented Nov 3, 2016 •

edited

Loading

thibaultmeyer Nov 3, 2016 •

edited

Loading

thibaultmeyer Nov 10, 2016 •

edited

Loading

thibaultmeyer Nov 3, 2016 •

edited

Loading

thibaultmeyer Nov 9, 2016 •

edited

Loading

thibaultmeyer Nov 4, 2016 •

edited

Loading

andreynering Nov 8, 2016 •

edited

Loading

lunny Nov 9, 2016 •

edited

Loading

codecov-io commented Nov 9, 2016 •

edited

Loading

lunny Nov 10, 2016 •

edited

Loading

thibaultmeyer commented Nov 10, 2016 •

edited

Loading

joubertredrat commented Nov 10, 2016 •

edited

Loading