From 44114b38e601c8bf44f575daef1d0e0597f37d1d Mon Sep 17 00:00:00 2001 From: Lanre Adelowo Date: Mon, 18 Feb 2019 21:55:04 +0100 Subject: [PATCH] Implement "conversation lock" for issue comments (#5073) --- custom/conf/app.ini.sample | 4 + .../doc/advanced/config-cheat-sheet.en-us.md | 3 + docs/content/doc/features/comparison.en-us.md | 2 +- models/issue.go | 4 + models/issue_comment.go | 4 + models/issue_lock.go | 51 +++++++++++ models/migrations/migrations.go | 2 + models/migrations/v80.go | 18 ++++ modules/auth/repo_form.go | 27 ++++++ modules/auth/repo_form_test.go | 25 ++++++ modules/setting/setting.go | 12 +++ options/locale/locale_en-US.ini | 19 +++++ routers/api/v1/repo/issue_comment.go | 6 ++ routers/repo/issue.go | 25 ++++++ routers/repo/issue_lock.go | 71 ++++++++++++++++ routers/routes/routes.go | 11 ++- templates/repo/issue/view_content.tmpl | 34 +++++++- .../repo/issue/view_content/comments.tmpl | 36 +++++++- .../repo/issue/view_content/sidebar.tmpl | 85 +++++++++++++++++++ 19 files changed, 435 insertions(+), 4 deletions(-) create mode 100644 models/issue_lock.go create mode 100644 models/migrations/v80.go create mode 100644 routers/repo/issue_lock.go diff --git a/custom/conf/app.ini.sample b/custom/conf/app.ini.sample index 8c5925298c..9b1712b025 100644 --- a/custom/conf/app.ini.sample +++ b/custom/conf/app.ini.sample @@ -69,6 +69,10 @@ MAX_FILES = 5 ; List of prefixes used in Pull Request title to mark them as Work In Progress WORK_IN_PROGRESS_PREFIXES=WIP:,[WIP] +[repository.issue] +; List of reasons why a Pull Request or Issue can be locked +LOCK_REASONS=Too heated,Off-topic,Resolved,Spam + [ui] ; Number of repositories that are displayed on one explore page EXPLORE_PAGING_NUM = 20 diff --git a/docs/content/doc/advanced/config-cheat-sheet.en-us.md b/docs/content/doc/advanced/config-cheat-sheet.en-us.md index 5321c4de9b..b7708084e9 100644 --- a/docs/content/doc/advanced/config-cheat-sheet.en-us.md +++ b/docs/content/doc/advanced/config-cheat-sheet.en-us.md @@ -71,6 +71,9 @@ Values containing `#` or `;` must be quoted using `` ` `` or `"""`. - `WORK_IN_PROGRESS_PREFIXES`: **WIP:,\[WIP\]**: List of prefixes used in Pull Request title to mark them as Work In Progress +### Repository - Issue (`repository.issue`) +- `LOCK_REASONS`: **Too heated,Off-topic,Resolved,Spam**: A list of reasons why a Pull Request or Issue can be locked + ## UI (`ui`) - `EXPLORE_PAGING_NUM`: **20**: Number of repositories that are shown in one explore page. diff --git a/docs/content/doc/features/comparison.en-us.md b/docs/content/doc/features/comparison.en-us.md index c9682b506f..1808828d8a 100644 --- a/docs/content/doc/features/comparison.en-us.md +++ b/docs/content/doc/features/comparison.en-us.md @@ -81,7 +81,7 @@ _Symbols used in table:_ | Related issues | ✘ | ✘ | ⁄ | ✘ | ✓ | ✘ | ✘ | | Confidential issues | ✘ | ✘ | ✘ | ✓ | ✓ | ✘ | ✘ | | Comment reactions | ✓ | ✘ | ✓ | ✓ | ✓ | ✘ | ✘ | -| Lock Discussion | ✘ | ✘ | ✓ | ✓ | ✓ | ✘ | ✘ | +| Lock Discussion | ✓ | ✘ | ✓ | ✓ | ✓ | ✘ | ✘ | | Batch issue handling | ✓ | ✘ | ✓ | ✓ | ✓ | ✘ | ✘ | | Issue Boards | ✘ | ✘ | ✘ | ✓ | ✓ | ✘ | ✘ | | Create new branches from issues | ✘ | ✘ | ✘ | ✓ | ✓ | ✘ | ✘ | diff --git a/models/issue.go b/models/issue.go index 1421b28da2..8ce8658fee 100644 --- a/models/issue.go +++ b/models/issue.go @@ -57,6 +57,10 @@ type Issue struct { Reactions ReactionList `xorm:"-"` TotalTrackedTime int64 `xorm:"-"` Assignees []*User `xorm:"-"` + + // IsLocked limits commenting abilities to users on an issue + // with write access + IsLocked bool `xorm:"NOT NULL DEFAULT false"` } var ( diff --git a/models/issue_comment.go b/models/issue_comment.go index 05756c6cf2..1b02918cb7 100644 --- a/models/issue_comment.go +++ b/models/issue_comment.go @@ -80,6 +80,10 @@ const ( CommentTypeCode // Reviews a pull request by giving general feedback CommentTypeReview + // Lock an issue, giving only collaborators access + CommentTypeLock + // Unlocks a previously locked issue + CommentTypeUnlock ) // CommentTag defines comment tag type diff --git a/models/issue_lock.go b/models/issue_lock.go new file mode 100644 index 0000000000..5a2d996b64 --- /dev/null +++ b/models/issue_lock.go @@ -0,0 +1,51 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package models + +// IssueLockOptions defines options for locking and/or unlocking an issue/PR +type IssueLockOptions struct { + Doer *User + Issue *Issue + Reason string +} + +// LockIssue locks an issue. This would limit commenting abilities to +// users with write access to the repo +func LockIssue(opts *IssueLockOptions) error { + return updateIssueLock(opts, true) +} + +// UnlockIssue unlocks a previously locked issue. +func UnlockIssue(opts *IssueLockOptions) error { + return updateIssueLock(opts, false) +} + +func updateIssueLock(opts *IssueLockOptions, lock bool) error { + if opts.Issue.IsLocked == lock { + return nil + } + + opts.Issue.IsLocked = lock + + var commentType CommentType + if opts.Issue.IsLocked { + commentType = CommentTypeLock + } else { + commentType = CommentTypeUnlock + } + + if err := UpdateIssueCols(opts.Issue, "is_locked"); err != nil { + return err + } + + _, err := CreateComment(&CreateCommentOptions{ + Doer: opts.Doer, + Issue: opts.Issue, + Repo: opts.Issue.Repo, + Type: commentType, + Content: opts.Reason, + }) + return err +} diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index d6e7f31e46..652abd122a 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -213,6 +213,8 @@ var migrations = []Migration{ NewMigration("rename repo is_bare to repo is_empty", renameRepoIsBareToIsEmpty), // v79 -> v80 NewMigration("add can close issues via commit in any branch", addCanCloseIssuesViaCommitInAnyBranch), + // v80 -> v81 + NewMigration("add is locked to issues", addIsLockedToIssues), } // Migrate database to current version diff --git a/models/migrations/v80.go b/models/migrations/v80.go new file mode 100644 index 0000000000..8cd2ac80a8 --- /dev/null +++ b/models/migrations/v80.go @@ -0,0 +1,18 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package migrations + +import "github.com/go-xorm/xorm" + +func addIsLockedToIssues(x *xorm.Engine) error { + // Issue see models/issue.go + type Issue struct { + ID int64 `xorm:"pk autoincr"` + IsLocked bool `xorm:"NOT NULL DEFAULT false"` + } + + return x.Sync2(new(Issue)) + +} diff --git a/modules/auth/repo_form.go b/modules/auth/repo_form.go index 1a67f2b884..0a97b08c71 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -10,6 +10,7 @@ import ( "strings" "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/routers/utils" "github.com/Unknwon/com" @@ -308,6 +309,32 @@ func (f *ReactionForm) Validate(ctx *macaron.Context, errs binding.Errors) bindi return validate(errs, ctx.Data, f, ctx.Locale) } +// IssueLockForm form for locking an issue +type IssueLockForm struct { + Reason string `binding:"Required"` +} + +// Validate validates the fields +func (i *IssueLockForm) Validate(ctx *macaron.Context, errs binding.Errors) binding.Errors { + return validate(errs, ctx.Data, i, ctx.Locale) +} + +// HasValidReason checks to make sure that the reason submitted in +// the form matches any of the values in the config +func (i IssueLockForm) HasValidReason() bool { + if strings.TrimSpace(i.Reason) == "" { + return true + } + + for _, v := range setting.Repository.Issue.LockReasons { + if v == i.Reason { + return true + } + } + + return false +} + // _____ .__.__ __ // / \ |__| | ____ _______/ |_ ____ ____ ____ // / \ / \| | | _/ __ \ / ___/\ __\/ _ \ / \_/ __ \ diff --git a/modules/auth/repo_form_test.go b/modules/auth/repo_form_test.go index f6223d6c8a..a3369b006e 100644 --- a/modules/auth/repo_form_test.go +++ b/modules/auth/repo_form_test.go @@ -7,6 +7,7 @@ package auth import ( "testing" + "code.gitea.io/gitea/modules/setting" "github.com/stretchr/testify/assert" ) @@ -39,3 +40,27 @@ func TestSubmitReviewForm_IsEmpty(t *testing.T) { assert.Equal(t, v.expected, v.form.HasEmptyContent()) } } + +func TestIssueLock_HasValidReason(t *testing.T) { + + // Init settings + _ = setting.Repository + + cases := []struct { + form IssueLockForm + expected bool + }{ + {IssueLockForm{""}, true}, // an empty reason is accepted + {IssueLockForm{"Off-topic"}, true}, + {IssueLockForm{"Too heated"}, true}, + {IssueLockForm{"Spam"}, true}, + {IssueLockForm{"Resolved"}, true}, + + {IssueLockForm{"ZZZZ"}, false}, + {IssueLockForm{"I want to lock this issue"}, false}, + } + + for _, v := range cases { + assert.Equal(t, v.expected, v.form.HasValidReason()) + } +} diff --git a/modules/setting/setting.go b/modules/setting/setting.go index d3b45ec29d..5f65570540 100644 --- a/modules/setting/setting.go +++ b/modules/setting/setting.go @@ -227,6 +227,11 @@ var ( PullRequest struct { WorkInProgressPrefixes []string } `ini:"repository.pull-request"` + + // Issue Setting + Issue struct { + LockReasons []string + } `ini:"repository.issue"` }{ AnsiCharset: "", ForcePrivate: false, @@ -279,6 +284,13 @@ var ( }{ WorkInProgressPrefixes: []string{"WIP:", "[WIP]"}, }, + + // Issue settings + Issue: struct { + LockReasons []string + }{ + LockReasons: strings.Split("Too heated,Off-topic,Spam,Resolved", ","), + }, } RepoRootPath string ScriptType = "bash" diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index 9e51278edd..c5a62cb488 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -780,6 +780,25 @@ issues.attachment.open_tab = `Click to see "%s" in a new tab` issues.attachment.download = `Click to download "%s"` issues.subscribe = Subscribe issues.unsubscribe = Unsubscribe +issues.lock = Lock conversation +issues.unlock = Unlock conversation +issues.lock.unknown_reason = Cannot lock an issue with an unknown reason. +issues.lock_duplicate = An issue cannot be locked twice. +issues.unlock_error = Cannot unlock an issue that is not locked. +issues.lock_with_reason = "locked as %s and limited conversation to collaborators %s" +issues.lock_no_reason = "locked and limited conversation to collaborators %s" +issues.unlock_comment = "unlocked this conversation %s" +issues.lock_confirm = Lock +issues.unlock_confirm = Unlock +issues.lock.notice_1 = - Other users can’t add new comments to this issue. +issues.lock.notice_2 = - You and other collaborators with access to this repository can still leave comments that others can see. +issues.lock.notice_3 = - You can always unlock this issue again in the future. +issues.unlock.notice_1 = - Everyone would be able to comment on this issue once more. +issues.unlock.notice_2 = - You can always lock this issue again in the future. +issues.lock.reason = Reason for locking +issues.lock.title = Lock conversation on this issue. +issues.unlock.title = Unlock conversation on this issue. +issues.comment_on_locked = You cannot comment on a locked issue. issues.tracker = Time Tracker issues.start_tracking_short = Start issues.start_tracking = Start Time Tracking diff --git a/routers/api/v1/repo/issue_comment.go b/routers/api/v1/repo/issue_comment.go index 720513f007..3e6f04eb7a 100644 --- a/routers/api/v1/repo/issue_comment.go +++ b/routers/api/v1/repo/issue_comment.go @@ -5,6 +5,7 @@ package repo import ( + "errors" "time" "code.gitea.io/gitea/models" @@ -169,6 +170,11 @@ func CreateIssueComment(ctx *context.APIContext, form api.CreateIssueCommentOpti return } + if issue.IsLocked && !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.User.IsAdmin { + ctx.Error(403, "CreateIssueComment", errors.New(ctx.Tr("repo.issues.comment_on_locked"))) + return + } + comment, err := models.CreateIssueComment(ctx.User, ctx.Repo.Repository, issue, form.Body, nil) if err != nil { ctx.Error(500, "CreateIssueComment", err) diff --git a/routers/repo/issue.go b/routers/repo/issue.go index 9767d11136..6783d279b5 100644 --- a/routers/repo/issue.go +++ b/routers/repo/issue.go @@ -57,6 +57,23 @@ var ( } ) +// MustAllowUserComment checks to make sure if an issue is locked. +// If locked and user has permissions to write to the repository, +// then the comment is allowed, else it is blocked +func MustAllowUserComment(ctx *context.Context) { + + issue := GetActionIssue(ctx) + if ctx.Written() { + return + } + + if issue.IsLocked && !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.User.IsAdmin { + ctx.Flash.Error(ctx.Tr("repo.issues.comment_on_locked")) + ctx.Redirect(issue.HTMLURL()) + return + } +} + // MustEnableIssues check if repository enable internal issues func MustEnableIssues(ctx *context.Context) { if !ctx.Repo.CanRead(models.UnitTypeIssues) && @@ -898,6 +915,9 @@ func ViewIssue(ctx *context.Context) { ctx.Data["SignInLink"] = setting.AppSubURL + "/user/login?redirect_to=" + ctx.Data["Link"].(string) ctx.Data["IsIssuePoster"] = ctx.IsSigned && issue.IsPoster(ctx.User.ID) ctx.Data["IsIssueWriter"] = ctx.Repo.CanWriteIssuesOrPulls(issue.IsPull) + ctx.Data["IsRepoAdmin"] = ctx.IsSigned && (ctx.Repo.IsAdmin() || ctx.User.IsAdmin) + ctx.Data["IsRepoIssuesWriter"] = ctx.IsSigned && (ctx.Repo.CanWrite(models.UnitTypeIssues) || ctx.User.IsAdmin) + ctx.Data["LockReasons"] = setting.Repository.Issue.LockReasons ctx.HTML(200, tplIssueView) } @@ -1118,6 +1138,11 @@ func NewComment(ctx *context.Context, form auth.CreateCommentForm) { if !ctx.IsSigned || (ctx.User.ID != issue.PosterID && !ctx.Repo.CanReadIssuesOrPulls(issue.IsPull)) { ctx.Error(403) + } + + if issue.IsLocked && !ctx.Repo.CanWrite(models.UnitTypeIssues) && !ctx.User.IsAdmin { + ctx.Flash.Error(ctx.Tr("repo.issues.comment_on_locked")) + ctx.Redirect(issue.HTMLURL(), http.StatusSeeOther) return } diff --git a/routers/repo/issue_lock.go b/routers/repo/issue_lock.go new file mode 100644 index 0000000000..fa87588319 --- /dev/null +++ b/routers/repo/issue_lock.go @@ -0,0 +1,71 @@ +// Copyright 2019 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package repo + +import ( + "net/http" + + "code.gitea.io/gitea/models" + "code.gitea.io/gitea/modules/auth" + "code.gitea.io/gitea/modules/context" +) + +// LockIssue locks an issue. This would limit commenting abilities to +// users with write access to the repo. +func LockIssue(ctx *context.Context, form auth.IssueLockForm) { + + issue := GetActionIssue(ctx) + if ctx.Written() { + return + } + + if issue.IsLocked { + ctx.Flash.Error(ctx.Tr("repo.issues.lock_duplicate")) + ctx.Redirect(issue.HTMLURL()) + return + } + + if !form.HasValidReason() { + ctx.Flash.Error(ctx.Tr("repo.issues.lock.unknown_reason")) + ctx.Redirect(issue.HTMLURL()) + return + } + + if err := models.LockIssue(&models.IssueLockOptions{ + Doer: ctx.User, + Issue: issue, + Reason: form.Reason, + }); err != nil { + ctx.ServerError("LockIssue", err) + return + } + + ctx.Redirect(issue.HTMLURL(), http.StatusSeeOther) +} + +// UnlockIssue unlocks a previously locked issue. +func UnlockIssue(ctx *context.Context) { + + issue := GetActionIssue(ctx) + if ctx.Written() { + return + } + + if !issue.IsLocked { + ctx.Flash.Error(ctx.Tr("repo.issues.unlock_error")) + ctx.Redirect(issue.HTMLURL()) + return + } + + if err := models.UnlockIssue(&models.IssueLockOptions{ + Doer: ctx.User, + Issue: issue, + }); err != nil { + ctx.ServerError("UnlockIssue", err) + return + } + + ctx.Redirect(issue.HTMLURL(), http.StatusSeeOther) +} diff --git a/routers/routes/routes.go b/routers/routes/routes.go index 8ab7ff9bea..b73b030a51 100644 --- a/routers/routes/routes.go +++ b/routers/routes/routes.go @@ -432,6 +432,13 @@ func RegisterRoutes(m *macaron.Macaron) { reqRepoIssuesOrPullsWriter := context.RequireRepoWriterOr(models.UnitTypeIssues, models.UnitTypePullRequests) reqRepoIssuesOrPullsReader := context.RequireRepoReaderOr(models.UnitTypeIssues, models.UnitTypePullRequests) + reqRepoIssueWriter := func(ctx *context.Context) { + if !ctx.Repo.CanWrite(models.UnitTypeIssues) { + ctx.Error(403) + return + } + } + // ***** START: Organization ***** m.Group("/org", func() { m.Group("", func() { @@ -574,7 +581,7 @@ func RegisterRoutes(m *macaron.Macaron) { m.Post("/add", repo.AddDependency) m.Post("/delete", repo.RemoveDependency) }) - m.Combo("/comments").Post(bindIgnErr(auth.CreateCommentForm{}), repo.NewComment) + m.Combo("/comments").Post(repo.MustAllowUserComment, bindIgnErr(auth.CreateCommentForm{}), repo.NewComment) m.Group("/times", func() { m.Post("/add", bindIgnErr(auth.AddTimeManuallyForm{}), repo.AddTimeManually) m.Group("/stopwatch", func() { @@ -583,6 +590,8 @@ func RegisterRoutes(m *macaron.Macaron) { }) }) m.Post("/reactions/:action", bindIgnErr(auth.ReactionForm{}), repo.ChangeIssueReaction) + m.Post("/lock", reqRepoIssueWriter, bindIgnErr(auth.IssueLockForm{}), repo.LockIssue) + m.Post("/unlock", reqRepoIssueWriter, repo.UnlockIssue) }, context.RepoMustNotBeArchived()) m.Post("/labels", reqRepoIssuesOrPullsWriter, repo.UpdateIssueLabel) diff --git a/templates/repo/issue/view_content.tmpl b/templates/repo/issue/view_content.tmpl index 2b13768d54..7445dcce86 100644 --- a/templates/repo/issue/view_content.tmpl +++ b/templates/repo/issue/view_content.tmpl @@ -69,7 +69,38 @@ {{if and .Issue.IsPull (not $.Repository.IsArchived)}} {{ template "repo/issue/view_content/pull". }} {{end}} - + {{if .IsSigned}} + {{ if or .IsRepoAdmin .IsRepoIssuesWriter (or (not .Issue.IsLocked)) }} +
+ + + +
+
+ {{template "repo/issue/comment_tab" .}} + {{.CsrfTokenHtml}} + +
+ {{if and (or .IsIssueWriter .IsIssuePoster) (not .DisableStatusChange)}} + {{if .Issue.IsClosed}} +
+ {{.i18n.Tr "repo.issues.reopen_issue"}} +
+ {{else}} +
+ {{.i18n.Tr "repo.issues.close_issue"}} +
+ {{end}} + {{end}} + +
+
+
+
+ {{ end }} + {{else}} {{if .Repository.IsArchived}}
{{if .Issue.IsPull}} @@ -114,6 +145,7 @@
{{end}} {{end}} + {{end}} diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 6fe09221c3..ac7e9a2332 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -2,7 +2,11 @@ {{range .Issue.Comments}} {{ $createdStr:= TimeSinceUnix .CreatedUnix $.Lang }} - + {{if eq .Type 0}}
@@ -355,5 +359,35 @@ {{end}} {{end}}
+ {{else if eq .Type 23}} +
+ + + + + + {{ if .Content }} + {{.Poster.Name}} + {{$.i18n.Tr "repo.issues.lock_with_reason" .Content $createdStr | Safe}} + + {{ else }} + {{.Poster.Name}} + {{$.i18n.Tr "repo.issues.lock_no_reason" $createdStr | Safe}} + + {{ end }} +
+ {{else if eq .Type 24}} +
+ + + + + + {{.Poster.Name}} + {{$.i18n.Tr "repo.issues.unlock_comment" $createdStr | Safe}} + +
{{end}} {{end}} diff --git a/templates/repo/issue/view_content/sidebar.tmpl b/templates/repo/issue/view_content/sidebar.tmpl index 8cb01fe00e..47bf67f903 100644 --- a/templates/repo/issue/view_content/sidebar.tmpl +++ b/templates/repo/issue/view_content/sidebar.tmpl @@ -335,6 +335,91 @@ {{end}} + + {{ if .IsRepoAdmin }} +
+
+
+ + +
+
+ + + + {{ end }} + {{if and .CanCreateIssueDependencies (not .Repository.IsArchived)}}