More improvements

Author: lunny

models/issues/dependency_test.go

@@ -49,7 +49,7 @@ func TestCreateIssueDependency(t *testing.T) {
 	assert.False(t, left)
 
 	// Close #2 and check again
-	_, err = issues_model.ChangeIssueStatus(db.DefaultContext, issue2, user1, true)
+	_, err = issues_model.CloseIssue(db.DefaultContext, issue2, user1)
 	assert.NoError(t, err)
 
 	left, err = issues_model.IssueNoDependenciesLeft(db.DefaultContext, issue1)

models/issues/issue_update.go

@@ -118,16 +118,28 @@ func doChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.Use
 	})
 }
 
+// CloseIssue changes issue status to closed.
+func CloseIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Comment, error) {
+	if err := issue.LoadRepo(ctx); err != nil {
+		return nil, err
+	}
+	if err := issue.LoadPoster(ctx); err != nil {
+		return nil, err
+	}
+
+	return changeIssueStatus(ctx, issue, doer, true, false)
+}
+
 // ChangeIssueStatus changes issue status to open or closed.
-func ChangeIssueStatus(ctx context.Context, issue *Issue, doer *user_model.User, isClosed bool) (*Comment, error) {
+func ReopenIssue(ctx context.Context, issue *Issue, doer *user_model.User) (*Comment, error) {
 	if err := issue.LoadRepo(ctx); err != nil {
 		return nil, err
 	}
 	if err := issue.LoadPoster(ctx); err != nil {
 		return nil, err
 	}
 
-	return changeIssueStatus(ctx, issue, doer, isClosed, false)
+	return changeIssueStatus(ctx, issue, doer, false, false)
 }
 
 // ChangeIssueTitle changes the title of this issue, as the given user.

models/issues/issue_xref_test.go

@@ -98,7 +98,7 @@ func TestXRef_ResolveCrossReferences(t *testing.T) {
 	i1 := testCreateIssue(t, 1, 2, "title1", "content1", false)
 	i2 := testCreateIssue(t, 1, 2, "title2", "content2", false)
 	i3 := testCreateIssue(t, 1, 2, "title3", "content3", false)
-	_, err := issues_model.ChangeIssueStatus(db.DefaultContext, i3, d, true)
+	_, err := issues_model.CloseIssue(db.DefaultContext, i3, d)
 	assert.NoError(t, err)
 
 	pr := testCreatePR(t, 1, 2, "titlepr", fmt.Sprintf("closes #%d", i1.Index))

routers/web/repo/issue.go

@@ -3166,7 +3166,7 @@ func NewComment(ctx *context.Context) {
 				ctx.Flash.Info(ctx.Tr("repo.pulls.open_unmerged_pull_exists", pr.Index))
 			} else {
 				closeOrReopen := form.Status == "close"
-				if closeOrReopen {
+				if closeOrReopen && !issue.IsClosed {
 					if err := issue_service.CloseIssue(ctx, issue, ctx.Doer, ""); err != nil {
 						log.Error("CloseIssue: %v", err)
 						if issues_model.IsErrDependenciesLeft(err) {
@@ -3184,7 +3184,7 @@ func NewComment(ctx *context.Context) {
 						}
 						log.Trace("Issue [%d] status changed to closed: %v", issue.ID, issue.IsClosed)
 					}
-				} else {
+				} else if !closeOrReopen && issue.IsClosed {
 					if err := issue_service.ReopenIssue(ctx, issue, ctx.Doer, ""); err != nil {
 						log.Error("ReopenIssue: %v", err)
 					}

services/issue/commit.go

@@ -188,17 +188,22 @@ func UpdateIssuesCommit(ctx context.Context, doer *user_model.User, repo *repo_m
 					continue
 				}
 			}
-			isClosed := ref.Action == references.XRefActionCloses
-			if isClosed && len(ref.TimeLog) > 0 {
-				if err := issueAddTime(ctx, refIssue, doer, c.Timestamp, ref.TimeLog); err != nil {
-					return err
+
+			closeOrReopen := ref.Action == references.XRefActionCloses
+			refIssue.Repo = refRepo
+			if closeOrReopen && !refIssue.IsClosed {
+				if len(ref.TimeLog) > 0 {
+					if err := issueAddTime(ctx, refIssue, doer, c.Timestamp, ref.TimeLog); err != nil {
+						return err
+					}
 				}
-			}
-			if isClosed && !refIssue.IsClosed {
-				refIssue.Repo = refRepo
 				if err := CloseIssue(ctx, refIssue, doer, c.Sha1); err != nil {
 					return err
 				}
+			} else if !closeOrReopen && refIssue.IsClosed {
+				if err := ReopenIssue(ctx, refIssue, doer, c.Sha1); err != nil {
+					return err
+				}
 			}
 		}
 	}

services/issue/status.go

@@ -14,7 +14,7 @@ import (
 
 // CloseIssue close and issue.
 func CloseIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string) error {
-	comment, err := issues_model.ChangeIssueStatus(ctx, issue, doer, true)
+	comment, err := issues_model.CloseIssue(ctx, issue, doer)
 	if err != nil {
 		if issues_model.IsErrDependenciesLeft(err) {
 			if err := issues_model.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil {
@@ -36,15 +36,11 @@ func CloseIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model
 // ReopenIssue reopen an issue.
 // FIXME: If some issues dependent this one are closed, should we also reopen them?
 func ReopenIssue(ctx context.Context, issue *issues_model.Issue, doer *user_model.User, commitID string) error {
-	comment, err := issues_model.ChangeIssueStatus(ctx, issue, doer, false)
+	comment, err := issues_model.ReopenIssue(ctx, issue, doer)
 	if err != nil {
 		return err
 	}
 
-	if err := issues_model.FinishIssueStopwatchIfPossible(ctx, doer, issue); err != nil {
-		return err
-	}
-
 	notify_service.IssueChangeStatus(ctx, doer, commitID, issue, comment, false)
 
 	return nil

services/pull/merge.go

@@ -242,14 +242,18 @@ func handleCloseCrossReferences(ctx context.Context, pr *issues_model.PullReques
 		if err = ref.Issue.LoadRepo(ctx); err != nil {
 			return err
 		}
-		isClosed := ref.RefAction == references.XRefActionCloses
-		if isClosed && !ref.Issue.IsClosed {
+		closeOrReopen := ref.RefAction == references.XRefActionCloses
+		if closeOrReopen && !ref.Issue.IsClosed {
 			if err = issue_service.CloseIssue(ctx, ref.Issue, doer, pr.MergedCommitID); err != nil {
 				// Allow ErrDependenciesLeft
 				if !issues_model.IsErrDependenciesLeft(err) {
 					return err
 				}
 			}
+		} else if !closeOrReopen && ref.Issue.IsClosed {
+			if err = issue_service.ReopenIssue(ctx, ref.Issue, doer, pr.MergedCommitID); err != nil {
+				return err
+			}
 		}
 	}
 	return nil