From dd24b9ff5a2e09630c67d2de3bfba1493c299c06 Mon Sep 17 00:00:00 2001 From: Konrad Date: Wed, 1 Nov 2017 16:16:49 +0100 Subject: [PATCH 1/7] Implemented missing create table for Issue Dependencies table \ IssueNoDependenciesLeft now returns an error when something goes wrong ; --- models/action.go | 7 ++++++- models/issue_dependency.go | 10 +++++----- models/models.go | 1 + routers/repo/issue.go | 7 ++++++- routers/repo/pull.go | 7 ++++++- 5 files changed, 24 insertions(+), 8 deletions(-) diff --git a/models/action.go b/models/action.go index f8c8c713e..66fe71b77 100644 --- a/models/action.go +++ b/models/action.go @@ -470,7 +470,12 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit) err } // Check for dependencies, if there aren't any, close it - if IssueNoDependenciesLeft(issue) { + noDeps, err := IssueNoDependenciesLeft(issue) + if err != nil { + return err + } + + if noDeps { if err = issue.ChangeStatus(doer, repo, true); err != nil { return err } diff --git a/models/issue_dependency.go b/models/issue_dependency.go index 5237803c8..ee5e1e5cc 100644 --- a/models/issue_dependency.go +++ b/models/issue_dependency.go @@ -138,17 +138,17 @@ func (IssueDependencyIssue) TableName() string { } // IssueNoDependenciesLeft checks if issue can be closed -func IssueNoDependenciesLeft(issue *Issue) bool { +func IssueNoDependenciesLeft(issue *Issue) (exists bool, err error) { - exists, err := x. + exists, err = x. Join("INNER", "issue", "issue.id = issue_dependency.dependency_id"). Where("issue_dependency.issue_id = ?", issue.ID). And("issue.is_closed = ?", "0"). Exist(&IssueDependencyIssue{}) - if err != nil { + /*if err != nil { return false - } + }*/ - return !exists + return !exists, err } diff --git a/models/models.go b/models/models.go index 853b9799e..c8133bb30 100644 --- a/models/models.go +++ b/models/models.go @@ -117,6 +117,7 @@ func init() { new(TrackedTime), new(DeletedBranch), new(RepoIndexerStatus), + new(IssueDependency), ) gonicNames := []string{"SSL", "UID"} diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 441e039dc..0f164cfa4 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -937,7 +937,12 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) { !(issue.IsPull && issue.PullRequest.HasMerged) { // Check for open dependencies - if form.Status == "close" && !models.IssueNoDependenciesLeft(issue) { + noDeps, err := models.IssueNoDependenciesLeft(issue) + if err != nil { + return + } + + if form.Status == "close" && !noDeps { if issue.IsPull { ctx.Flash.Error("You need to close all issues blocking this pull request before you can merge it!") ctx.Redirect(fmt.Sprintf("%s/pulls/%d", ctx.Repo.RepoLink, issue.Index)) diff --git a/routers/repo/pull.go b/routers/repo/pull.go index 9b0564b4c..0677e002c 100644 --- a/routers/repo/pull.go +++ b/routers/repo/pull.go @@ -455,7 +455,12 @@ func MergePullRequest(ctx *context.Context) { pr.Issue = issue pr.Issue.Repo = ctx.Repo.Repository - if !models.IssueNoDependenciesLeft(issue) { + noDeps, err := models.IssueNoDependenciesLeft(issue) + if err != nil { + return + } + + if !noDeps { ctx.Flash.Error("You need to close all issues blocking this pull request before you can merge it!") ctx.Redirect(ctx.Repo.RepoLink + "/pulls/" + com.ToStr(pr.Index)) return From 827291e188a1f9b73fa5d515ae6545808c9e71a7 Mon Sep 17 00:00:00 2001 From: Konrad Langenberg Date: Wed, 1 Nov 2017 16:24:23 +0100 Subject: [PATCH 2/7] Cleanup --- models/issue_dependency.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/models/issue_dependency.go b/models/issue_dependency.go index ee5e1e5cc..9f4120d84 100644 --- a/models/issue_dependency.go +++ b/models/issue_dependency.go @@ -146,9 +146,5 @@ func IssueNoDependenciesLeft(issue *Issue) (exists bool, err error) { And("issue.is_closed = ?", "0"). Exist(&IssueDependencyIssue{}) - /*if err != nil { - return false - }*/ - return !exists, err } From c0bc5e8321e495b7bf0c3613969c96fd1fd7fd26 Mon Sep 17 00:00:00 2001 From: Konrad Langenberg Date: Wed, 1 Nov 2017 20:38:55 +0100 Subject: [PATCH 3/7] Fixed indetion --- .../repo/issue/view_content/sidebar.tmpl | 166 +++++++++--------- 1 file changed, 83 insertions(+), 83 deletions(-) diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl index 03bcdcf7e..4f7cf6dc8 100644 --- a/templates/repo/issue/view_content/sidebar.tmpl +++ b/templates/repo/issue/view_content/sidebar.tmpl @@ -193,97 +193,97 @@ {{if .IsSigned}} {{if .IssueDependenciesEnabled}} -
+
-
- {{.i18n.Tr "repo.issues.dependency.title"}} -
- {{if .BlockedByDependencies}} - - {{.i18n.Tr "repo.issues.dependency.blocked_by_short"}}: - -
- {{range .BlockedByDependencies}} -
-
- - - - {{if .IsClosed}} -
- +
+ {{.i18n.Tr "repo.issues.dependency.title"}} +
+ {{if .BlockedByDependencies}} + + {{.i18n.Tr "repo.issues.dependency.blocked_by_short"}}: + +
+ {{range .BlockedByDependencies}} +
+
+ + + + {{if .IsClosed}} +
+ +
+ {{else}} +
+ +
+ {{end}} +
+
#{{.Index}}
+ {{.Title}} +
+ {{end}}
- {{else}} -
- -
- {{end}} -
-
#{{.Index}}
- {{.Title}} -
- {{end}} -
- {{end}} + {{end}} - {{if .BlockingDependencies}} - - {{.i18n.Tr "repo.issues.dependency.blocks_short"}}: - -
- {{range .BlockingDependencies}} -
-
- - - - {{if .IsClosed}} -
- + {{if .BlockingDependencies}} + + {{.i18n.Tr "repo.issues.dependency.blocks_short"}}: + +
+ {{range .BlockingDependencies}} +
+
+ + + + {{if .IsClosed}} +
+ +
+ {{else}} +
+ +
+ {{end}} +
+
#{{.Index}}
+ {{.Title}} +
+ {{end}}
- {{else}} -
- -
- {{end}} -
-
#{{.Index}}
- {{.Title}} -
- {{end}} -
- {{end}} + {{end}} - {{if (and (not .BlockedByDependencies) (not .BlockingDependencies))}} -

{{.i18n.Tr "repo.issues.dependency.no_dependencies"}}

- {{end}} -
-
- {{$.CsrfTokenHtml}} -
-
- - -
{{.i18n.Tr "repo.issues.dependency.add"}}
-
+ {{if (and (not .BlockedByDependencies) (not .BlockingDependencies))}} +

{{.i18n.Tr "repo.issues.dependency.no_dependencies"}}

+ {{end}} +
+ + {{$.CsrfTokenHtml}} +
+
+ + +
{{.i18n.Tr "repo.issues.dependency.add"}}
+
+
+
+
-
- +
- -
-
+
From 993a3c0b86970c56bca17115c57b752726cf2c02 Mon Sep 17 00:00:00 2001 From: Konrad Langenberg Date: Wed, 1 Nov 2017 20:42:24 +0100 Subject: [PATCH 4/7] Fixed indention --- templates/repo/issue/view_content/sidebar.tmpl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl index 4f7cf6dc8..92717c7dc 100644 --- a/templates/repo/issue/view_content/sidebar.tmpl +++ b/templates/repo/issue/view_content/sidebar.tmpl @@ -204,7 +204,7 @@ {{else}} {{.i18n.Tr "repo.issues.dependency.pr_closing_blockedby"}} {{end}}" data-inverted=""> - {{.i18n.Tr "repo.issues.dependency.blocked_by_short"}}: + {{.i18n.Tr "repo.issues.dependency.blocked_by_short"}}:
{{range .BlockedByDependencies}} @@ -236,7 +236,7 @@ {{else}} {{.i18n.Tr "repo.issues.dependency.issue_close_blocks"}} {{end}}" data-inverted=""> - {{.i18n.Tr "repo.issues.dependency.blocks_short"}}: + {{.i18n.Tr "repo.issues.dependency.blocks_short"}}:
{{range .BlockingDependencies}} From 3beecd2d134a59292ad2a2c738fe5123048bc4c1 Mon Sep 17 00:00:00 2001 From: Konrad Langenberg Date: Wed, 1 Nov 2017 21:33:42 +0100 Subject: [PATCH 5/7] Improved readability --- models/issue_dependency.go | 29 ++++++++-------------------- models/migrations/v49.go | 4 +--- models/repo.go | 8 +++----- routers/repo/issue_dependency_add.go | 2 -- 4 files changed, 12 insertions(+), 31 deletions(-) diff --git a/models/issue_dependency.go b/models/issue_dependency.go index 9f4120d84..76f056509 100644 --- a/models/issue_dependency.go +++ b/models/issue_dependency.go @@ -52,16 +52,12 @@ func CreateIssueDependency(user *User, issue, dep *Issue) (exists bool, err erro } // Add comment referencing the new dependency - _, err = createIssueDependencyComment(sess, user, issue, dep, true) - - if err != nil { + if _, err = createIssueDependencyComment(sess, user, issue, dep, true); err != nil { return exists, err } // Create a new comment for the dependent issue - _, err = createIssueDependencyComment(sess, user, dep, issue, true) - - if err != nil { + if _, err = createIssueDependencyComment(sess, user, dep, issue, true); err != nil { return exists, err } } @@ -73,8 +69,8 @@ func RemoveIssueDependency(user *User, issue *Issue, dep *Issue, depType Depende sess := x.NewSession() // Check if it exists - exists, err := issueDepExists(x, issue.ID, dep.ID) - if err != nil { + var exists bool + if exists, err = issueDepExists(x, issue.ID, dep.ID); err != nil { return err } @@ -92,22 +88,17 @@ func RemoveIssueDependency(user *User, issue *Issue, dep *Issue, depType Depende return } - _, err := x.Delete(&issueDepToDelete) - if err != nil { + if _, err := x.Delete(&issueDepToDelete); err != nil { return err } // Add comment referencing the removed dependency - _, err = createIssueDependencyComment(sess, user, issue, dep, false) - - if err != nil { + if _, err = createIssueDependencyComment(sess, user, issue, dep, false); err != nil { return err } // Create a new comment for the dependent issue - _, err = createIssueDependencyComment(sess, user, dep, issue, false) - - if err != nil { + if _, err = createIssueDependencyComment(sess, user, dep, issue, false); err != nil { return err } } @@ -119,11 +110,7 @@ func issueDepExists(e Engine, issueID int64, depID int64) (exists bool, err erro exists, err = e.Where("(issue_id = ? AND dependency_id = ?) OR (issue_id = ? AND dependency_id = ?)", issueID, depID, depID, issueID).Exist(&IssueDependency{}) - if err != nil { - return exists, err - } - - return exists, nil + return } // IssueDependencyIssue custom type for mysql join diff --git a/models/migrations/v49.go b/models/migrations/v49.go index 21ed4760b..36756d319 100644 --- a/models/migrations/v49.go +++ b/models/migrations/v49.go @@ -24,9 +24,7 @@ func addIssueDependencyTables(x *xorm.Engine) (err error) { UpdatedUnix int64 `xorm:"updated"` } - err = x.Sync(new(IssueDependency)) - - if err != nil { + if err = x.Sync(new(IssueDependency)); err != nil { return fmt.Errorf("Error creating issue_dependency_table column definition: %v", err) } diff --git a/models/repo.go b/models/repo.go index 1f8f1541e..de6256937 100644 --- a/models/repo.go +++ b/models/repo.go @@ -2457,8 +2457,7 @@ func (repo *Repository) GetUserFork(userID int64) (*Repository, error) { func (repo *Repository) getBlockedByDependencies(e Engine, issueID int64) (_ []*IssueDependencyIssue, err error) { var issueDeps []*IssueDependencyIssue - err = x.Join("INNER", "issue", "issue.id = issue_dependency.dependency_id").Where("issue_id = ?", issueID).Find(&issueDeps) - if err != nil { + if err = x.Join("INNER", "issue", "issue.id = issue_dependency.dependency_id").Where("issue_id = ?", issueID).Find(&issueDeps); err != nil { return issueDeps, err } @@ -2466,11 +2465,10 @@ func (repo *Repository) getBlockedByDependencies(e Engine, issueID int64) (_ []* } // Get Blocking Dependencies, aka all issues this issue blocks. -func (repo *Repository) getBlockingDependencies(e Engine, issueID int64) (_ []*IssueDependencyIssue, err error) { +func (repo *Repository) getBlockingDependencies(e Engine, issueID int64) ([]*IssueDependencyIssue, error) { var issueDeps []*IssueDependencyIssue - err = x.Join("INNER", "issue", "issue.id = issue_dependency.issue_id").Where("dependency_id = ?", issueID).Find(&issueDeps) - if err != nil { + if err := x.Join("INNER", "issue", "issue.id = issue_dependency.issue_id").Where("dependency_id = ?", issueID).Find(&issueDeps); err != nil { return issueDeps, err } diff --git a/routers/repo/issue_dependency_add.go b/routers/repo/issue_dependency_add.go index 394a013db..c113bdbcc 100644 --- a/routers/repo/issue_dependency_add.go +++ b/routers/repo/issue_dependency_add.go @@ -16,8 +16,6 @@ import ( // AddDependency adds new dependencies func AddDependency(c *context.Context) { - // TODO: should should an issue only have dependencies in it's own repo? - depID, err := strconv.ParseInt(c.Req.PostForm.Get("newDependency"), 10, 64) if err != nil { c.Handle(http.StatusBadRequest, "issue ID is not int", err) From 4e9e1f8d839ce54f16d5b3f93e2744465195ce4f Mon Sep 17 00:00:00 2001 From: Konrad Langenberg Date: Wed, 1 Nov 2017 21:36:06 +0100 Subject: [PATCH 6/7] Removed unused named returns --- models/issue_dependency.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/models/issue_dependency.go b/models/issue_dependency.go index 76f056509..7a475f517 100644 --- a/models/issue_dependency.go +++ b/models/issue_dependency.go @@ -125,9 +125,9 @@ func (IssueDependencyIssue) TableName() string { } // IssueNoDependenciesLeft checks if issue can be closed -func IssueNoDependenciesLeft(issue *Issue) (exists bool, err error) { +func IssueNoDependenciesLeft(issue *Issue) (bool, error) { - exists, err = x. + exists, err := x. Join("INNER", "issue", "issue.id = issue_dependency.dependency_id"). Where("issue_dependency.issue_id = ?", issue.ID). And("issue.is_closed = ?", "0"). From e364cfc20fdb9b98067f39fe2e447eabaf55f9f7 Mon Sep 17 00:00:00 2001 From: Konrad Langenberg Date: Wed, 1 Nov 2017 22:02:41 +0100 Subject: [PATCH 7/7] Improved Readability --- routers/repo/issue_dependency_remove.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/routers/repo/issue_dependency_remove.go b/routers/repo/issue_dependency_remove.go index 6b29fc5a9..475c07f48 100644 --- a/routers/repo/issue_dependency_remove.go +++ b/routers/repo/issue_dependency_remove.go @@ -50,8 +50,7 @@ func RemoveDependency(c *context.Context) { return } - err = models.RemoveIssueDependency(c.User, issue, dep, depType) - if err != nil { + if err = models.RemoveIssueDependency(c.User, issue, dep, depType); err != nil { c.Handle(http.StatusInternalServerError, "CreateOrUpdateIssueDependency", err) return }