Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add a way to mark Conversation (code comment) resolved #11037

Merged
merged 18 commits into from
Apr 18, 2020
Merged

Add a way to mark Conversation (code comment) resolved #11037

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
b95fb6f
Add a way to mark Conversation (code comment) resolved
a1012112796 Apr 10, 2020
f962c5e
fix lint
a1012112796 Apr 10, 2020
aa3bae7
Merge branch 'master' into resolve-cov
a1012112796 Apr 12, 2020
b99c2ac
Apply suggestions from code review
a1012112796 Apr 12, 2020
b27c8ad
Merge branch 'master' into resolve-cov
a1012112796 Apr 12, 2020
fdd4b5e
change IsResolved to an function
a1012112796 Apr 12, 2020
2276cee
Apply suggestions from code review
a1012112796 Apr 14, 2020
a2fa3ee
Merge branch 'master' into resolve-cov
a1012112796 Apr 14, 2020
87672ff
Merge branch 'master' into resolve-cov
a1012112796 Apr 15, 2020
4907d7f
change button color
a1012112796 Apr 15, 2020
bbbd8b5
Merge branch 'master' into resolve-cov
a1012112796 Apr 15, 2020
982bd6f
resolve button size
a1012112796 Apr 15, 2020
3fbc628
fix code style
a1012112796 Apr 16, 2020
655e2b6
Merge branch 'master' into resolve-cov
a1012112796 Apr 16, 2020
6ecd910
Merge branch 'master' into resolve-cov
a1012112796 Apr 17, 2020
4776c11
Merge branch 'master' into resolve-cov
guillep2k Apr 18, 2020
b7b4c0a
Merge branch 'master' into resolve-cov
guillep2k Apr 18, 2020
1ae80d1
Merge branch 'master' into resolve-cov
a1012112796 Apr 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions models/issue_comment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -942,6 +942,13 @@ func fetchCodeCommentsByReview(e Engine, issue *Issue, currentUser *User, review
return nil, err
}
for _, comment := range comments {
// use assignee as Conversation doer
if comment.AssigneeID != 0 {
if err := comment.LoadAssigneeUser(); err != nil {
return nil, err
}
}

if re, ok := reviews[comment.ReviewID]; ok && re != nil {
// If the review is pending only the author can see the comments (except the review is set)
if review.ID == 0 {
Expand Down
57 changes: 57 additions & 0 deletions models/review.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
package models

import (
"fmt"
"strings"

"code.gitea.io/gitea/modules/timeutil"
Expand Down Expand Up @@ -590,3 +591,59 @@ func RemoveRewiewRequest(issue *Issue, reviewer *User, doer *User) (comment *Com

return comment, sess.Commit()
}

// MarkConversation Add or remove Conversation mark for a code comment
func MarkConversation(comment *Comment, doer *User, isResolve bool) (err error) {
if comment.Type != CommentTypeCode {
return nil
}

if isResolve {
if comment.AssigneeID != 0 {
return nil
}

if _, err = x.Exec("UPDATE `comment` SET assignee_id=? WHERE id=?", doer.ID, comment.ID); err != nil {
return err
}
} else {
if comment.AssigneeID == 0 {
return nil
}

if _, err = x.Exec("UPDATE `comment` SET assignee_id=? WHERE id=?", 0, comment.ID); err != nil {
return err
}
}

return nil
}

// CanMarkConversation Add or remove Conversation mark for a code comment permission check
// the PR writer , offfical reviewer and poster can do it
a1012112796 marked this conversation as resolved.
Show resolved Hide resolved
func CanMarkConversation(issue *Issue, doer *User) (permResult bool, err error) {
if doer == nil || issue == nil {
return false, fmt.Errorf("issue or doer is nil")
}

if doer.ID != issue.PosterID {
perm, err := GetUserRepoPermission(issue.Repo, doer)
if err != nil {
return false, err
}

permResult = perm.CanAccess(AccessModeWrite, UnitTypePullRequests)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should check if this PR's base branch is protected. If it's protected, we should check if it's official review, otherwise we should check if it's the writer of the pull request unit.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello, I think I also have done what you say, because IsOfficialReviewer will check them by step. then I'd like to do more easy check more earlly to lower average time cost , so I first check whether doer is poster of PR, if not ,then check if doer have write permission of pull request, at last check if doer is offical reviewer by IsOfficialReviewer. do ypu think so? Thanks.

gitea/models/review.go

Lines 185 to 205 in cc07b9c

// IsOfficialReviewer check if reviewer can make official reviews in issue (counts towards required approvals)
func IsOfficialReviewer(issue *Issue, reviewer *User) (bool, error) {
return isOfficialReviewer(x, issue, reviewer)
}
func isOfficialReviewer(e Engine, issue *Issue, reviewer *User) (bool, error) {
pr, err := getPullRequestByIssueID(e, issue.ID)
if err != nil {
return false, err
}
if err = pr.loadProtectedBranch(e); err != nil {
return false, err
}
if pr.ProtectedBranch == nil {
return false, nil
}
return pr.ProtectedBranch.isUserOfficialReviewer(e, reviewer)
}

if !permResult {
permResult, err = IsOfficialReviewer(issue, doer)
if err != nil {
a1012112796 marked this conversation as resolved.
Show resolved Hide resolved
return false, err
}
}

if !permResult {
return false, nil
}
}

return true, nil
}
5 changes: 5 additions & 0 deletions options/locale/locale_en-US.ini
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1064,6 +1064,11 @@ issues.review.review = Review
issues.review.reviewers = Reviewers
issues.review.show_outdated = Show outdated
issues.review.hide_outdated = Hide outdated
issues.review.show_resolved = Show resolved
issues.review.hide_resolved = Hide resolved
issues.review.resolve_conversation = Resolve conversation
issues.review.un_resolve_conversation = Unresolve conversation
issues.review.resolved_by = marked this conversation as resolved
issues.assignee.error = Not all assignees was added due to an unexpected error.

pulls.desc = Enable pull requests and code reviews.
Expand Down
13 changes: 13 additions & 0 deletions routers/repo/issue.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -990,6 +990,14 @@ func ViewIssue(ctx *context.Context) {
ctx.ServerError("Review.LoadCodeComments", err)
return
}

// use assignee as Mark Conversation doer
if comment.Type == models.CommentTypeCode && comment.AssigneeID != 0 {
if err = comment.LoadAssigneeUser(); err != nil {
ctx.ServerError("LoadAssigneeUser", err)
return
}
}
}
}

Expand Down Expand Up @@ -1033,6 +1041,11 @@ func ViewIssue(ctx *context.Context) {
ctx.ServerError("IsUserAllowedToMerge", err)
return
}

if ctx.Data["CanMarkConversation"], err = models.CanMarkConversation(issue, ctx.User); err != nil {
ctx.ServerError("CanMarkConversation", err)
return
}
}

prUnit, err := repo.GetUnit(models.UnitTypePullRequests)
Expand Down
7 changes: 7 additions & 0 deletions routers/repo/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -635,6 +635,13 @@ func ViewPullFiles(ctx *context.Context) {
return
}

if ctx.IsSigned && ctx.User != nil {
if ctx.Data["CanMarkConversation"], err = models.CanMarkConversation(issue, ctx.User); err != nil {
ctx.ServerError("CanMarkConversation", err)
return
}
}

setImageCompareContext(ctx, baseCommit, commit)
setPathsCompareContext(ctx, baseCommit, commit, headTarget)

Expand Down
38 changes: 38 additions & 0 deletions routers/repo/pull_review.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,44 @@ func CreateCodeComment(ctx *context.Context, form auth.CodeCommentForm) {
ctx.Redirect(comment.HTMLURL())
}

// UpdateResolveConversation add or remove an Conversation resolved mark
func UpdateResolveConversation(ctx *context.Context) {
a1012112796 marked this conversation as resolved.
Show resolved Hide resolved
action := ctx.Query("action")
issueID := ctx.QueryInt64("issue_id")
commentID := ctx.QueryInt64("comment_id")
a1012112796 marked this conversation as resolved.
Show resolved Hide resolved

issue, err := models.GetIssueByID(issueID)
if err != nil {
ctx.ServerError("GetIssueByID", err)
return
}

if !issue.IsPull {
return
a1012112796 marked this conversation as resolved.
Show resolved Hide resolved
}

comment, err := models.GetCommentByID(commentID)
if err != nil {
ctx.ServerError("GetCommentByID", err)
return
}

if action == "Resolve" || action == "UnResolve" {
err = models.MarkConversation(comment, ctx.User, action == "Resolve")
if err != nil {
ctx.ServerError("MarkConversation", err)
return
}
} else {
ctx.ServerError("UpdateResolveConversation", fmt.Errorf("error action : %s", action))
a1012112796 marked this conversation as resolved.
Show resolved Hide resolved
return
}

ctx.JSON(200, map[string]interface{}{
"ok": true,
})
}

// SubmitReview creates a review out of the existing pending review or creates a new one if no pending review exist
func SubmitReview(ctx *context.Context, form auth.SubmitReviewForm) {
issue := GetActionIssue(ctx)
Expand Down
1 change: 1 addition & 0 deletions routers/routes/routes.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -739,6 +739,7 @@ func RegisterRoutes(m *macaron.Macaron) {
m.Post("/assignee", reqRepoIssuesOrPullsWriter, repo.UpdateIssueAssignee)
m.Post("/request_review", reqRepoIssuesOrPullsReader, repo.UpdatePullReviewRequest)
m.Post("/status", reqRepoIssuesOrPullsWriter, repo.UpdateIssueStatus)
m.Post("/resolve_conversation", reqRepoIssuesOrPullsReader, repo.UpdateResolveConversation)
}, context.RepoMustNotBeArchived())
m.Group("/comments/:id", func() {
m.Post("", repo.UpdateCommentContent)
Expand Down
54 changes: 52 additions & 2 deletions templates/repo/diff/box.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,32 +147,82 @@
{{end}}
</tr>
{{if gt (len $line.Comments) 0}}
{{$assigneeid := (index $line.Comments 0).AssigneeID}}
{{$assignee := (index $line.Comments 0).Assignee}}
<tr class="add-code-comment">
<td class="lines-num"></td>
<td class="lines-type-marker"></td>
<td class="add-comment-left">
<div class="ui segment">
{{if and (not (eq $assigneeid 0)) (eq $line.GetCommentSide "previous")}}
<span class="ui grey text left"><b>{{$assignee.Name}}</b> {{$.i18n.Tr "repo.issues.review.resolved_by"}}</span>
<button id="show-outdated-{{(index $line.Comments 0).ID}}" data-comment="{{(index $line.Comments 0).ID}}" class="ui compact right labeled button show-outdated">
{{svg "octicon-fold" 16}}
{{$.i18n.Tr "repo.issues.review.show_resolved"}}
</button>
<button id="hide-outdated-{{(index $line.Comments 0).ID}}" data-comment="{{(index $line.Comments 0).ID}}" class="hide ui compact right labeled button hide-outdated">
{{svg "octicon-fold" 16}}
{{$.i18n.Tr "repo.issues.review.hide_resolved"}}
</button>
{{end}}
</div>
{{if eq $line.GetCommentSide "previous"}}
<div class="field comment-code-cloud">
<div id="code-comments-{{(index $line.Comments 0).ID}}" class="field comment-code-cloud {{if not (eq $assigneeid 0)}}hide{{end}}">
<div class="comment-list">
<ui class="ui comments">
{{ template "repo/diff/comments" dict "root" $ "comments" $line.Comments}}
</ui>
</div>
{{template "repo/diff/comment_form_datahandler" dict "reply" (index $line.Comments 0).ReviewID "hidden" true "root" $ "comment" (index $line.Comments 0)}}
{{if $.CanMarkConversation }}
<button class="ui green icon tiny button resolve-conversation" data-action="{{if eq $assigneeid 0}}Resolve{{else}}UnResolve{{end}}" data-issue-id="{{$.Issue.ID}}" data-comment-id="{{(index $line.Comments 0).ID}}" data-update-url="{{$.RepoLink}}/issues/resolve_conversation" >
{{if not (eq $assigneeid 0)}}
{{$.i18n.Tr "repo.issues.review.un_resolve_conversation"}}
{{else}}
{{$.i18n.Tr "repo.issues.review.resolve_conversation"}}
{{end}}
</button>
{{end}}
{{if not (eq $assigneeid 0)}}
<span class="ui grey text"><b>{{$assignee.Name}}</b> {{$.i18n.Tr "repo.issues.review.resolved_by"}}</span>
{{end}}
</div>
{{end}}
</td>
<td class="lines-num"></td>
<td class="lines-type-marker"></td>
<td class="add-comment-right">
{{if and (not (eq $assigneeid 0)) (eq $line.GetCommentSide "proposed")}}
<span class="ui grey text left"><b>{{$assignee.Name}}</b> {{$.i18n.Tr "repo.issues.review.resolved_by"}}</span>
<button id="show-outdated-{{(index $line.Comments 0).ID}}" data-comment="{{(index $line.Comments 0).ID}}" class="ui compact right labeled button show-outdated">
{{svg "octicon-fold" 16}}
{{$.i18n.Tr "repo.issues.review.show_resolved"}}
</button>
<button id="hide-outdated-{{(index $line.Comments 0).ID}}" data-comment="{{(index $line.Comments 0).ID}}" class="hide ui compact right labeled button hide-outdated">
{{svg "octicon-fold" 16}}
{{$.i18n.Tr "repo.issues.review.hide_resolved"}}
</button>
{{end}}
{{if eq $line.GetCommentSide "proposed"}}
<div class="field comment-code-cloud">
<div id="code-comments-{{(index $line.Comments 0).ID}}" class="field comment-code-cloud {{if not (eq $assigneeid 0)}}hide{{end}}">
<div class="comment-list">
<ui class="ui comments">
{{ template "repo/diff/comments" dict "root" $ "comments" $line.Comments}}
</ui>
</div>
{{template "repo/diff/comment_form_datahandler" dict "reply" (index $line.Comments 0).ReviewID "hidden" true "root" $ "comment" (index $line.Comments 0)}}
{{if $.CanMarkConversation}}
<button class="ui green icon tiny button resolve-conversation" data-action="{{if eq $assigneeid 0}}Resolve{{else}}UnResolve{{end}}" data-issue-id="{{$.Issue.ID}}" data-comment-id="{{(index $line.Comments 0).ID}}" data-update-url="{{$.RepoLink}}/issues/resolve_conversation" >
{{if not (eq $assigneeid 0)}}
{{$.i18n.Tr "repo.issues.review.un_resolve_conversation"}}
{{else}}
{{$.i18n.Tr "repo.issues.review.resolve_conversation"}}
{{end}}
</button>
{{end}}
{{if not (eq $assigneeid 0)}}
<span class="ui grey text"><b>{{$assignee.Name}}</b> {{$.i18n.Tr "repo.issues.review.resolved_by"}}</span>
{{end}}
</div>
{{end}}
</td>
Expand Down
30 changes: 29 additions & 1 deletion templates/repo/diff/section_unified.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,17 +23,45 @@
<td class="lines-code{{if (not $line.RightIdx)}} lines-code-old{{end}}">{{if and $.root.SignedUserID $line.CanComment $.root.PageIsPullFiles}}<a class="ui green button add-code-comment add-code-comment-{{if $line.RightIdx}}right{{else}}left{{end}}" data-path="{{$file.Name}}" data-side="{{if $line.RightIdx}}right{{else}}left{{end}}" data-idx="{{if $line.RightIdx}}{{$line.RightIdx}}{{else}}{{$line.LeftIdx}}{{end}}">+</a>{{end}}<span class="mono wrap{{if $highlightClass}} language-{{$highlightClass}}{{else}} nohighlight{{end}}">{{$section.GetComputedInlineDiffFor $line}}</span></td>
</tr>
{{if gt (len $line.Comments) 0}}
{{$assigneeid := (index $line.Comments 0).AssigneeID}}
{{$assignee := (index $line.Comments 0).Assignee}}
<tr>
<td colspan="2" class="lines-num"></td>
<td class="lines-type-marker"></td>
<td class="add-comment-left add-comment-right">
<div class="field comment-code-cloud">
<div class = "ui segment">
{{if not (eq $assigneeid 0)}}
<span class="ui grey text left"><b>{{$assignee.Name}}</b> {{$.root.i18n.Tr "repo.issues.review.resolved_by"}}</span>
<button id="show-outdated-{{(index $line.Comments 0).ID}}" data-comment="{{(index $line.Comments 0).ID}}" class="ui compact right labeled button show-outdated">
{{svg "octicon-fold" 16}}
{{$.root.i18n.Tr "repo.issues.review.show_resolved"}}
</button>
<button id="hide-outdated-{{(index $line.Comments 0).ID}}" data-comment="{{(index $line.Comments 0).ID}}" class="hide ui compact right labeled button hide-outdated">
{{svg "octicon-fold" 16}}
{{$.root.i18n.Tr "repo.issues.review.hide_resolved"}}
</button>
{{end}}
</div>
<div id="code-comments-{{(index $line.Comments 0).ID}}" class="field comment-code-cloud {{if not (eq $assigneeid 0)}}hide{{end}}">
<div class="comment-list">
<ui class="ui comments">
{{ template "repo/diff/comments" dict "root" $.root "comments" $line.Comments}}
</ui>
</div>
{{template "repo/diff/comment_form_datahandler" dict "hidden" true "reply" (index $line.Comments 0).ReviewID "root" $.root "comment" (index $line.Comments 0)}}
{{if $.root.CanMarkConversation}}
<button class="ui green icon tiny button resolve-conversation" data-action="{{if eq $assigneeid 0}}Resolve{{else}}UnResolve{{end}}" data-issue-id="{{$.root.Issue.ID}}" data-comment-id="{{(index $line.Comments 0).ID}}" data-update-url="{{$.root.RepoLink}}/issues/resolve_conversation" >
{{if not (eq $assigneeid 0)}}
{{$.root.i18n.Tr "repo.issues.review.un_resolve_conversation"}}
{{else}}
{{$.root.i18n.Tr "repo.issues.review.resolve_conversation"}}
{{end}}
</button>
{{end}}

{{if not (eq $assigneeid 0)}}
<span class="ui grey text"><b>{{$assignee.Name}}</b> {{$.root.i18n.Tr "repo.issues.review.resolved_by"}}</span>
{{end}}
</div>
</td>
</tr>
Expand Down
34 changes: 29 additions & 5 deletions templates/repo/issue/view_content/comments.tmpl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,22 +356,32 @@
<div class="ui segments">
<div class="ui segment">
{{$invalid := (index $comms 0).Invalidated}}
{{if $invalid}}
{{$assigneeid := (index $comms 0).AssigneeID}}
{{$assignee := (index $comms 0).Assignee}}
{{if or $invalid (not (eq $assigneeid 0))}}
<button id="show-outdated-{{(index $comms 0).ID}}" data-comment="{{(index $comms 0).ID}}" class="ui compact right labeled button show-outdated">
{{svg "octicon-fold" 16}}
{{$.i18n.Tr "repo.issues.review.show_outdated"}}
{{if $invalid }}
{{$.i18n.Tr "repo.issues.review.show_outdated"}}
{{else}}
{{$.i18n.Tr "repo.issues.review.show_resolved"}}
{{end}}
</button>
<button id="hide-outdated-{{(index $comms 0).ID}}" data-comment="{{(index $comms 0).ID}}" class="hide ui compact right labeled button hide-outdated">
{{svg "octicon-fold" 16}}
{{$.i18n.Tr "repo.issues.review.hide_outdated"}}
{{if $invalid}}
{{$.i18n.Tr "repo.issues.review.hide_outdated"}}
{{else}}
{{$.i18n.Tr "repo.issues.review.hide_resolved"}}
{{end}}
</button>
{{end}}
<a href="{{(index $comms 0).CodeCommentURL}}" class="file-comment">{{$filename}}</a>
</div>
{{$diff := (CommentMustAsDiff (index $comms 0))}}
{{if $diff}}
{{$file := (index $diff.Files 0)}}
<div id="code-preview-{{(index $comms 0).ID}}" class="ui table segment{{if $invalid}} hide{{end}}">
<div id="code-preview-{{(index $comms 0).ID}}" class="ui table segment{{if or $invalid (not (eq $assigneeid 0))}} hide{{end}}">
<div class="diff-file-box diff-box file-content {{TabSizeClass $.Editorconfig $file.Name}}">
<div class="file-body file-code code-view code-diff code-diff-unified">
<table>
Expand All @@ -383,7 +393,7 @@
</div>
</div>
{{end}}
<div id="code-comments-{{(index $comms 0).ID}}" class="ui segment{{if $invalid}} hide{{end}}">
<div id="code-comments-{{(index $comms 0).ID}}" class="ui segment{{if or $invalid (not (eq $assigneeid 0))}} hide{{end}}">
<div class="ui comments">
{{range $comms}}
{{ $createdSubStr:= TimeSinceUnix .CreatedUnix $.Lang }}
Expand Down Expand Up @@ -413,6 +423,20 @@
{{end}}
</div>
{{template "repo/diff/comment_form_datahandler" dict "hidden" true "reply" (index $comms 0).ReviewID "root" $ "comment" (index $comms 0)}}

{{if $.CanMarkConversation}}
<button class="ui green icon tiny button resolve-conversation" data-action="{{if eq $assigneeid 0}}Resolve{{else}}UnResolve{{end}}" data-issue-id="{{$.Issue.ID}}" data-comment-id="{{(index $comms 0).ID}}" data-update-url="{{$.RepoLink}}/issues/resolve_conversation" >
{{if not (eq $assigneeid 0)}}
{{$.i18n.Tr "repo.issues.review.un_resolve_conversation"}}
{{else}}
{{$.i18n.Tr "repo.issues.review.resolve_conversation"}}
{{end}}
</button>
{{end}}

{{if not (eq $assigneeid 0)}}
<span class="ui grey text"><b>{{$assignee.Name}}</b> {{$.i18n.Tr "repo.issues.review.resolved_by"}}</span>
{{end}}
</div>
</div>
{{end}}
Expand Down
Loading