From 30787e48f219b23701f660ba0b99b326ab82e997 Mon Sep 17 00:00:00 2001
From: Antoine GIRARD <sapk@users.noreply.github.com>
Date: Thu, 6 Jul 2017 15:30:19 +0200
Subject: [PATCH] Improve org error handling (#2117)

* Improve ErrOrgNotExist type
Return new error type
Use good error check
Use new method to check error
Update tests

* Fix unchanged method name report
---
 models/error.go             | 16 ++++++++++++++++
 models/org.go               |  6 ++----
 models/org_team.go          |  2 +-
 models/org_test.go          |  4 ++--
 routers/api/v1/api.go       |  6 +++---
 routers/api/v1/repo/fork.go |  2 +-
 routers/api/v1/repo/repo.go |  2 +-
 7 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/models/error.go b/models/error.go
index 404939c58..74551ad8f 100644
--- a/models/error.go
+++ b/models/error.go
@@ -448,6 +448,22 @@ func (err ErrAccessTokenEmpty) Error() string {
 // \_______  /__|  \___  (____  /___|  /__/_____ \(____  /__| |__|\____/|___|  /
 //         \/     /_____/     \/     \/         \/     \/                    \/
 
+// ErrOrgNotExist represents a "OrgNotExist" kind of error.
+type ErrOrgNotExist struct {
+	ID   int64
+	Name string
+}
+
+// IsErrOrgNotExist checks if an error is a ErrOrgNotExist.
+func IsErrOrgNotExist(err error) bool {
+	_, ok := err.(ErrOrgNotExist)
+	return ok
+}
+
+func (err ErrOrgNotExist) Error() string {
+	return fmt.Sprintf("org does not exist [id: %d, name: %s]", err.ID, err.Name)
+}
+
 // ErrLastOrgOwner represents a "LastOrgOwner" kind of error.
 type ErrLastOrgOwner struct {
 	UID int64
diff --git a/models/org.go b/models/org.go
index b4cbabcb5..d43f15f9a 100644
--- a/models/org.go
+++ b/models/org.go
@@ -16,8 +16,6 @@ import (
 )
 
 var (
-	// ErrOrgNotExist organization does not exist
-	ErrOrgNotExist = errors.New("Organization does not exist")
 	// ErrTeamNotExist team does not exist
 	ErrTeamNotExist = errors.New("Team does not exist")
 )
@@ -180,7 +178,7 @@ func CreateOrganization(org, owner *User) (err error) {
 // GetOrgByName returns organization by given name.
 func GetOrgByName(name string) (*User, error) {
 	if len(name) == 0 {
-		return nil, ErrOrgNotExist
+		return nil, ErrOrgNotExist{0, name}
 	}
 	u := &User{
 		LowerName: strings.ToLower(name),
@@ -190,7 +188,7 @@ func GetOrgByName(name string) (*User, error) {
 	if err != nil {
 		return nil, err
 	} else if !has {
-		return nil, ErrOrgNotExist
+		return nil, ErrOrgNotExist{0, name}
 	}
 	return u, nil
 }
diff --git a/models/org_team.go b/models/org_team.go
index a7a1ba85d..8508a176c 100644
--- a/models/org_team.go
+++ b/models/org_team.go
@@ -230,7 +230,7 @@ func NewTeam(t *Team) (err error) {
 	if err != nil {
 		return err
 	} else if !has {
-		return ErrOrgNotExist
+		return ErrOrgNotExist{t.OrgID, ""}
 	}
 
 	t.LowerName = strings.ToLower(t.Name)
diff --git a/models/org_test.go b/models/org_test.go
index 95698a58e..801088c2e 100644
--- a/models/org_test.go
+++ b/models/org_test.go
@@ -222,10 +222,10 @@ func TestGetOrgByName(t *testing.T) {
 	assert.Equal(t, "user3", org.Name)
 
 	org, err = GetOrgByName("user2") // user2 is an individual
-	assert.Equal(t, ErrOrgNotExist, err)
+	assert.True(t, IsErrOrgNotExist(err))
 
 	org, err = GetOrgByName("") // corner case
-	assert.Equal(t, ErrOrgNotExist, err)
+	assert.True(t, IsErrOrgNotExist(err))
 }
 
 func TestCountOrganizations(t *testing.T) {
diff --git a/routers/api/v1/api.go b/routers/api/v1/api.go
index 56bb3b0f3..14a8d5985 100644
--- a/routers/api/v1/api.go
+++ b/routers/api/v1/api.go
@@ -208,12 +208,12 @@ func orgAssignment(args ...bool) macaron.Handler {
 
 		var err error
 		if assignOrg {
-			ctx.Org.Organization, err = models.GetUserByName(ctx.Params(":orgname"))
+			ctx.Org.Organization, err = models.GetOrgByName(ctx.Params(":orgname"))
 			if err != nil {
-				if models.IsErrUserNotExist(err) {
+				if models.IsErrOrgNotExist(err) {
 					ctx.Status(404)
 				} else {
-					ctx.Error(500, "GetUserByName", err)
+					ctx.Error(500, "GetOrgByName", err)
 				}
 				return
 			}
diff --git a/routers/api/v1/repo/fork.go b/routers/api/v1/repo/fork.go
index c743aec30..44b79a6fe 100644
--- a/routers/api/v1/repo/fork.go
+++ b/routers/api/v1/repo/fork.go
@@ -59,7 +59,7 @@ func CreateFork(ctx *context.APIContext, form api.CreateForkOption) {
 	} else {
 		org, err := models.GetOrgByName(*form.Organization)
 		if err != nil {
-			if err == models.ErrOrgNotExist {
+			if models.IsErrOrgNotExist(err) {
 				ctx.Error(422, "", err)
 			} else {
 				ctx.Error(500, "GetOrgByName", err)
diff --git a/routers/api/v1/repo/repo.go b/routers/api/v1/repo/repo.go
index e0b693a4e..7fb828ddb 100644
--- a/routers/api/v1/repo/repo.go
+++ b/routers/api/v1/repo/repo.go
@@ -156,7 +156,7 @@ func CreateOrgRepo(ctx *context.APIContext, opt api.CreateRepoOption) {
 
 	org, err := models.GetOrgByName(ctx.Params(":org"))
 	if err != nil {
-		if models.IsErrUserNotExist(err) {
+		if models.IsErrOrgNotExist(err) {
 			ctx.Error(422, "", err)
 		} else {
 			ctx.Error(500, "GetOrgByName", err)