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 6 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
24 changes: 24 additions & 0 deletions models/issue_comment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ type Comment struct {
AssigneeID int64
RemovedAssignee bool
Assignee *User `xorm:"-"`
ResolveDoerID int64
ResolveDoer *User `xorm:"-"`
OldTitle string
NewTitle string
OldRef string
Expand Down Expand Up @@ -418,6 +420,23 @@ func (c *Comment) LoadAssigneeUser() error {
return nil
}

// LoadResolveDoer if comment.Type is CommentTypeCode and ResolveDoerID not zero, then load resolveDoer
func (c *Comment) LoadResolveDoer() (err error) {
if c.ResolveDoerID == 0 || c.Type != CommentTypeCode {
return nil
}
c.ResolveDoer, err = getUserByID(x, c.ResolveDoerID)
if err != nil {
return
a1012112796 marked this conversation as resolved.
Show resolved Hide resolved
}
return
}

// IsResolved check if an code comment is resolved
func (c *Comment) IsResolved() bool {
return c.ResolveDoerID != 0 && c.Type == CommentTypeCode
}

// LoadDepIssueDetails loads Dependent Issue Details
func (c *Comment) LoadDepIssueDetails() (err error) {
if c.DependentIssueID <= 0 || c.DependentIssue != nil {
Expand Down Expand Up @@ -941,7 +960,12 @@ func fetchCodeCommentsByReview(e Engine, issue *Issue, currentUser *User, review
if err := e.In("id", ids).Find(&reviews); err != nil {
return nil, err
}

for _, comment := range comments {
if err := comment.LoadResolveDoer(); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

When the user deleted, but the comment should be still loaded.

Copy link
Member Author

@a1012112796 a1012112796 Apr 14, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

yes, But It willn't be error if user is deleted now, it will load an default user to replace it, but the name to show will be not right , that's the thing what I think now. maybe shold save user name instead of user id, then it willn't be a problem , do you think so ? Thanks

Copy link
Member

Choose a reason for hiding this comment

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

@lunny If understand what you mean, that's taken care of in LoadResolveDoer:

gitea/models/issue_comment.go

Lines 430 to 437 in a2fa3ee

c.ResolveDoer, err = getUserByID(x, c.ResolveDoerID)
if err != nil {
if IsErrUserNotExist(err) {
c.ResolveDoerID = -1
c.ResolveDoer = NewGhostUser()
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
64 changes: 64 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 @@ -594,3 +595,66 @@ 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.ResolveDoerID != 0 {
return nil
}

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

if _, err = x.Exec("UPDATE `comment` SET resolve_doer_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 , offfcial reviewer and poster can do it
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 {
if issue.Repo == nil {
err = issue.LoadRepo()
a1012112796 marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return false, err
}
}

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 @@ -1065,6 +1065,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
10 changes: 10 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,11 @@ func ViewIssue(ctx *context.Context) {
ctx.ServerError("Review.LoadCodeComments", err)
return
}

if err = comment.LoadResolveDoer(); err != nil {
ctx.ServerError("LoadResolveDoer", err)
return
}
}
}

Expand Down Expand Up @@ -1033,6 +1038,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 @@ -636,6 +636,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
50 changes: 50 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,56 @@ 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
}

var permResult bool
if permResult, err = models.CanMarkConversation(issue, ctx.User); err != nil {
ctx.ServerError("CanMarkConversation", err)
return
}
if !permResult {
ctx.ServerError("UpdateResolveConversation",
a1012112796 marked this conversation as resolved.
Show resolved Hide resolved
fmt.Errorf("doer can't permission [usser_id: %d, issue_id: %d]",
ctx.User.ID, issue.ID))
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
50 changes: 48 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,78 @@
{{end}}
</tr>
{{if gt (len $line.Comments) 0}}
{{$resolved := (index $line.Comments 0).IsResolved}}
{{$resolveDoer := (index $line.Comments 0).ResolveDoer}}
<tr class="add-code-comment">
<td class="lines-num"></td>
<td class="lines-type-marker"></td>
<td class="add-comment-left">
{{if and $resolved (eq $line.GetCommentSide "previous")}}
<div class="ui top attached header">
<span class="ui grey text left"><b>{{$resolveDoer.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-unfold" 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>
</div>
{{end}}
{{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 $resolved}}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 not $resolved}}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 $resolved}}
{{$.i18n.Tr "repo.issues.review.un_resolve_conversation"}}
{{else}}
{{$.i18n.Tr "repo.issues.review.resolve_conversation"}}
{{end}}
</button>
{{end}}
</div>
{{end}}
</td>
<td class="lines-num"></td>
<td class="lines-type-marker"></td>
<td class="add-comment-right">
{{if and $resolved (eq $line.GetCommentSide "proposed")}}
<div class="ui top attached header">
<span class="ui grey text left"><b>{{$resolveDoer.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-unfold" 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>
</div>
{{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 $resolved}}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 not $resolved}}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 $resolved}}
{{$.i18n.Tr "repo.issues.review.un_resolve_conversation"}}
{{else}}
{{$.i18n.Tr "repo.issues.review.resolve_conversation"}}
{{end}}
</button>
{{end}}
</div>
{{end}}
</td>
Expand Down
26 changes: 25 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,41 @@
<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}}
{{$resolved := (index $line.Comments 0).IsResolved}}
{{$resolveDoer := (index $line.Comments 0).ResolveDoer}}
<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">
{{if $resolved}}
<div class = "ui attached header">
<span class="ui grey text left"><b>{{$resolveDoer.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-unfold" 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>
</div>
{{end}}
<div id="code-comments-{{(index $line.Comments 0).ID}}" class="field comment-code-cloud {{if $resolved}}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 not $resolved}}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 $resolved}}
{{$.root.i18n.Tr "repo.issues.review.un_resolve_conversation"}}
{{else}}
{{$.root.i18n.Tr "repo.issues.review.resolve_conversation"}}
{{end}}
</button>
{{end}}
</div>
</td>
</tr>
Expand Down
Loading