Fix ignored errors when checking if organization, team member #3177

ethantkoenig · 2017-12-13T05:59:21Z

Add checks for previously-ignored errors in various org functions.
Get(..) -> Exist() micro-optimization
Remove unnecessary checks in repo.SettingsPost(..)

codecov-io · 2017-12-13T16:12:02Z

Codecov Report

Merging #3177 into master will decrease coverage by 0.05%.
The diff coverage is 23.95%.

@@            Coverage Diff            @@
##           master   #3177      +/-   ##
=========================================
- Coverage   34.95%   34.9%   -0.06%     
=========================================
  Files         277     277              
  Lines       40014   40108      +94     
=========================================
+ Hits        13987   13998      +11     
- Misses      23988   24061      +73     
- Partials     2039    2049      +10

Impacted Files	Coverage Δ
routers/api/v1/repo/fork.go	`0% <0%> (ø)`	⬆️
routers/api/v1/api.go	`75.24% <0%> (-1.11%)`	⬇️
routers/api/v1/org/team.go	`4.67% <0%> (-0.06%)`	⬇️
routers/api/v1/org/member.go	`0% <0%> (ø)`	⬆️
modules/context/org.go	`0.86% <0%> (-0.05%)`	⬇️
models/user.go	`39.76% <0%> (-0.44%)`	⬇️
routers/repo/setting.go	`4.72% <0%> (+0.15%)`	⬆️
models/repo.go	`43.38% <10%> (-0.02%)`	⬇️
routers/api/v1/repo/repo.go	`38.46% <16.12%> (-1.64%)`	⬇️
routers/repo/repo.go	`22.44% <20%> (-0.01%)`	⬇️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5294821...e75a2fd. Read the comment docs.

lunny · 2017-12-14T14:49:46Z

models/org.go

@@ -401,8 +414,9 @@ func ChangeOrgUserStatus(orgID, uid int64, public bool) error {

 // AddOrgUser adds new user to given organization.
 func AddOrgUser(orgID, uid int64) error {


This function should be removed since we have dropped the UI to add user to org.

But user is still added to org when adding it to team

That should be in a transaction but not this function

I will address this in a separate PR

lunny · 2017-12-14T14:54:37Z

models/org.go

-func IsOrganizationOwner(orgID, uid int64) bool {
-	has, _ := x.
+func IsOrganizationOwner(orgID, uid int64) (bool, error) {
+	return x.
 		Where("is_owner=?", true).
 		And("uid=?", uid).
 		And("org_id=?", orgID).
 		Get(new(OrgUser))


Get -> Exist will has better performance.
Exist(new(OrgUser)) -> Table("org_user").Exist() will has better performance and less memory usage.

lunny · 2017-12-14T14:54:58Z

models/org.go

-func IsOrganizationMember(orgID, uid int64) bool {
-	has, _ := x.
+func IsOrganizationMember(orgID, uid int64) (bool, error) {
+	return x.
 		Where("uid=?", uid).
 		And("org_id=?", orgID).
 		Get(new(OrgUser))


lunny · 2017-12-14T14:55:06Z

models/org.go

-func IsPublicMembership(orgID, uid int64) bool {
-	has, _ := x.
+func IsPublicMembership(orgID, uid int64) (bool, error) {
+	return x.
 		Where("uid=?", uid).
 		And("org_id=?", orgID).
 		And("is_public=?", true).
 		Get(new(OrgUser))


lunny · 2017-12-14T14:55:27Z

models/org_team.go

-func isTeamMember(e Engine, orgID, teamID, userID int64) bool {
-	has, _ := e.
+func isTeamMember(e Engine, orgID, teamID, userID int64) (bool, error) {
+	return e.
 		Where("org_id=?", orgID).
 		And("team_id=?", teamID).
 		And("uid=?", userID).
 		Get(new(TeamUser))


lunny · 2017-12-14T14:57:42Z

All Get -> Exist changes will not block this PR. If this PR didn't do that, I will send another PR to do that.

ethantkoenig · 2017-12-18T17:51:58Z

@lafriks @lunny Friendly ping

lafriks · 2017-12-18T19:26:13Z

Please use more descriptive titles but otherwise LGTM

lunny · 2017-12-19T01:38:09Z

models/org.go

-	return IsOrganizationOwner(org.ID, uid)
+	isOwner, err := IsOrganizationOwner(org.ID, uid)
+	if err != nil {
+		log.Error(4, "IsOwnedBy: %v", err)


And I don't know if the log will affect the SSH output? Maybe change the function's signature to IsOwnedBy(uid int64) (bool, error) or maybe just ignore them.

Oh, I had thought this function was used in templates (and thus did not want to change the signature), but it looks like it isn't. Will update signature.

Also, I don't see how this specific call to log.Error is different than any call to log.Error currently in the codebase, and therefore I don't see why this particular log.Error raises concerns about SSH output that other log.Error calls do not.

lunny · 2017-12-19T01:39:05Z

@ethantkoenig a new issue to be considered that if the log will affect SSH console output.

ethantkoenig · 2017-12-19T04:39:30Z

routers/repo/setting.go

@@ -234,13 +234,6 @@ func SettingsPost(ctx *context.Context, form auth.RepoSettingForm) {
 			return
 		}

-		if ctx.Repo.Owner.IsOrganization() {


These can be removed, because we have already checked ctx.Repo.IsOwner().

lunny · 2017-12-20T03:21:27Z

routers/api/v1/org/member.go

 			ctx.Status(404)
+			return
 		}


why not use only one return on line 149?

Because you may fall through to the redirect (lines 151-153)

lunny · 2017-12-21T05:59:27Z

LGTM

lafriks added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Dec 13, 2017

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 13, 2017

lunny requested changes Dec 14, 2017

View reviewed changes

ethantkoenig force-pushed the ignored_errors branch from f10cbf9 to f540076 Compare December 14, 2017 20:39

lafriks changed the title ~~Fix ignored errors~~ Fix ignored errors when checking if organization, team member Dec 18, 2017

tboerger added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Dec 18, 2017

lunny requested changes Dec 19, 2017

View reviewed changes

lunny added this to the 1.4.0 milestone Dec 19, 2017

Fix ignored errors

6b1948e

ethantkoenig force-pushed the ignored_errors branch from f540076 to 6b1948e Compare December 19, 2017 04:31

ethantkoenig commented Dec 19, 2017

View reviewed changes

lunny reviewed Dec 20, 2017

View reviewed changes

lunny approved these changes Dec 21, 2017

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Dec 21, 2017

Merge branch 'master' into ignored_errors

e75a2fd

lafriks merged commit 515cdaa into go-gitea:master Dec 21, 2017

ethantkoenig deleted the ignored_errors branch February 21, 2018 05:56

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ignored errors when checking if organization, team member #3177

Fix ignored errors when checking if organization, team member #3177

ethantkoenig commented Dec 13, 2017 •

edited

codecov-io commented Dec 13, 2017 •

edited

lunny Dec 14, 2017

lafriks Dec 14, 2017

lunny Dec 14, 2017

lafriks Dec 14, 2017

ethantkoenig Dec 14, 2017

lunny Dec 14, 2017

ethantkoenig Dec 14, 2017

lunny Dec 14, 2017

ethantkoenig Dec 14, 2017

lunny Dec 14, 2017

ethantkoenig Dec 14, 2017

lunny Dec 14, 2017

ethantkoenig Dec 14, 2017

lunny commented Dec 14, 2017

ethantkoenig commented Dec 18, 2017

lafriks commented Dec 18, 2017

lunny Dec 19, 2017

ethantkoenig Dec 19, 2017

ethantkoenig Dec 19, 2017 •

edited

lunny commented Dec 19, 2017 •

edited

ethantkoenig Dec 19, 2017

lunny Dec 20, 2017

ethantkoenig Dec 20, 2017

lunny commented Dec 21, 2017

		@@ -401,8 +414,9 @@ func ChangeOrgUserStatus(orgID, uid int64, public bool) error {

		// AddOrgUser adds new user to given organization.
		func AddOrgUser(orgID, uid int64) error {

Fix ignored errors when checking if organization, team member #3177

Fix ignored errors when checking if organization, team member #3177

Conversation

ethantkoenig commented Dec 13, 2017 • edited

codecov-io commented Dec 13, 2017 • edited

Codecov Report

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

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

lunny commented Dec 14, 2017

ethantkoenig commented Dec 18, 2017

lafriks commented Dec 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig Dec 19, 2017 • edited

Choose a reason for hiding this comment

lunny commented Dec 19, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Dec 21, 2017

ethantkoenig commented Dec 13, 2017 •

edited

codecov-io commented Dec 13, 2017 •

edited

ethantkoenig Dec 19, 2017 •

edited

lunny commented Dec 19, 2017 •

edited