Skip to content

Commit 2276cee

Browse files
a1012112796guillep2k
a1012112796
and
guillep2k
committed
Apply suggestions from code review
* change return error for permisson check * add default message for deleted user * get issue message from comment * add migration for ``ResolveDoerID`` column another change: * block mark pending review as resolved because it's not necessary Co-authored-by: guillep2k <[email protected]>
1 parent fdd4b5e commit 2276cee

File tree

9 files changed

+54
-30
lines changed

9 files changed

+54
-30
lines changed

‎models/issue_comment.go

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -427,7 +427,11 @@ func (c *Comment) LoadResolveDoer() (err error) {
427427
}
428428
c.ResolveDoer, err = getUserByID(x, c.ResolveDoerID)
429429
if err != nil {
430-
return
430+
if IsErrUserNotExist(err) {
431+
c.ResolveDoerID = -1
432+
c.ResolveDoer = NewGhostUser()
433+
err = nil
434+
}
431435
}
432436
return
433437
}

‎models/migrations/migrations.go

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -204,6 +204,8 @@ var migrations = []Migration{
204204
NewMigration("Refix merge base for merged pull requests", refixMergeBase),
205205
// v135 -> 136
206206
NewMigration("Add OrgID column to Labels table", addOrgIDLabelColumn),
207+
// v136 -> 137
208+
NewMigration("Add ResolveDoerID to Comment table", addResolveDoerIDCommentColumn),
207209
}
208210

209211
// GetCurrentDBVersion returns the current db version

‎models/migrations/v136.go

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
// Copyright 2020 The Gitea Authors. All rights reserved.
2+
// Use of this source code is governed by a MIT-style
3+
// license that can be found in the LICENSE file.
4+
5+
package migrations
6+
7+
import (
8+
"fmt"
9+
10+
"xorm.io/xorm"
11+
)
12+
13+
func addResolveDoerIDCommentColumn(x *xorm.Engine) error {
14+
type Comment struct {
15+
ResolveDoerID int64
16+
}
17+
18+
if err := x.Sync2(new(Comment)); err != nil {
19+
return fmt.Errorf("Sync2: %v", err)
20+
}
21+
return nil
22+
}

‎models/review.go

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -631,11 +631,9 @@ func CanMarkConversation(issue *Issue, doer *User) (permResult bool, err error)
631631
}
632632

633633
if doer.ID != issue.PosterID {
634-
if issue.Repo == nil {
635-
err = issue.LoadRepo()
636-
if err != nil {
637-
return false, err
638-
}
634+
err = issue.LoadRepo()
635+
if err != nil {
636+
return false, err
639637
}
640638

641639
perm, err := GetUserRepoPermission(issue.Repo, doer)

‎routers/repo/pull_review.go

Lines changed: 11 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -64,34 +64,31 @@ func CreateCodeComment(ctx *context.Context, form auth.CodeCommentForm) {
6464
// UpdateResolveConversation add or remove an Conversation resolved mark
6565
func UpdateResolveConversation(ctx *context.Context) {
6666
action := ctx.Query("action")
67-
issueID := ctx.QueryInt64("issue_id")
6867
commentID := ctx.QueryInt64("comment_id")
6968

70-
issue, err := models.GetIssueByID(issueID)
69+
comment, err := models.GetCommentByID(commentID)
7170
if err != nil {
7271
ctx.ServerError("GetIssueByID", err)
7372
return
7473
}
7574

75+
if err = comment.LoadIssue(); err != nil {
76+
ctx.ServerError("comment.LoadIssue", err)
77+
return
78+
}
79+
7680
var permResult bool
77-
if permResult, err = models.CanMarkConversation(issue, ctx.User); err != nil {
81+
if permResult, err = models.CanMarkConversation(comment.Issue, ctx.User); err != nil {
7882
ctx.ServerError("CanMarkConversation", err)
7983
return
8084
}
8185
if !permResult {
82-
ctx.ServerError("UpdateResolveConversation",
83-
fmt.Errorf("doer can't permission [usser_id: %d, issue_id: %d]",
84-
ctx.User.ID, issue.ID))
85-
return
86-
}
87-
88-
if !issue.IsPull {
86+
ctx.Error(403)
8987
return
9088
}
9189

92-
comment, err := models.GetCommentByID(commentID)
93-
if err != nil {
94-
ctx.ServerError("GetCommentByID", err)
90+
if !comment.Issue.IsPull {
91+
ctx.Error(400)
9592
return
9693
}
9794

@@ -102,7 +99,7 @@ func UpdateResolveConversation(ctx *context.Context) {
10299
return
103100
}
104101
} else {
105-
ctx.ServerError("UpdateResolveConversation", fmt.Errorf("error action : %s", action))
102+
ctx.Error(400)
106103
return
107104
}
108105

‎templates/repo/diff/box.tmpl

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -149,6 +149,7 @@
149149
{{if gt (len $line.Comments) 0}}
150150
{{$resolved := (index $line.Comments 0).IsResolved}}
151151
{{$resolveDoer := (index $line.Comments 0).ResolveDoer}}
152+
{{$isNotPending := (not (eq (index $line.Comments 0).Review.Type 0))}}
152153
<tr class="add-code-comment">
153154
<td class="lines-num"></td>
154155
<td class="lines-type-marker"></td>
@@ -174,8 +175,8 @@
174175
</ui>
175176
</div>
176177
{{template "repo/diff/comment_form_datahandler" dict "reply" (index $line.Comments 0).ReviewID "hidden" true "root" $ "comment" (index $line.Comments 0)}}
177-
{{if $.CanMarkConversation }}
178-
<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" >
178+
{{if and $.CanMarkConversation $isNotPending}}
179+
<button class="ui green icon tiny button resolve-conversation" data-action="{{if not $resolved}}Resolve{{else}}UnResolve{{end}}" data-comment-id="{{(index $line.Comments 0).ID}}" data-update-url="{{$.RepoLink}}/issues/resolve_conversation" >
179180
{{if $resolved}}
180181
{{$.i18n.Tr "repo.issues.review.un_resolve_conversation"}}
181182
{{else}}
@@ -210,8 +211,8 @@
210211
</ui>
211212
</div>
212213
{{template "repo/diff/comment_form_datahandler" dict "reply" (index $line.Comments 0).ReviewID "hidden" true "root" $ "comment" (index $line.Comments 0)}}
213-
{{if $.CanMarkConversation}}
214-
<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" >
214+
{{if and $.CanMarkConversation $isNotPending}}
215+
<button class="ui green icon tiny button resolve-conversation" data-action="{{if not $resolved}}Resolve{{else}}UnResolve{{end}}" data-comment-id="{{(index $line.Comments 0).ID}}" data-update-url="{{$.RepoLink}}/issues/resolve_conversation" >
215216
{{if $resolved}}
216217
{{$.i18n.Tr "repo.issues.review.un_resolve_conversation"}}
217218
{{else}}

‎templates/repo/diff/section_unified.tmpl

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
{{if gt (len $line.Comments) 0}}
2626
{{$resolved := (index $line.Comments 0).IsResolved}}
2727
{{$resolveDoer := (index $line.Comments 0).ResolveDoer}}
28+
{{$isNotPending := (not (eq (index $line.Comments 0).Review.Type 0))}}
2829
<tr>
2930
<td colspan="2" class="lines-num"></td>
3031
<td class="lines-type-marker"></td>
@@ -49,8 +50,8 @@
4950
</ui>
5051
</div>
5152
{{template "repo/diff/comment_form_datahandler" dict "hidden" true "reply" (index $line.Comments 0).ReviewID "root" $.root "comment" (index $line.Comments 0)}}
52-
{{if $.root.CanMarkConversation}}
53-
<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" >
53+
{{if and $.root.CanMarkConversation $isNotPending}}
54+
<button class="ui green icon tiny button resolve-conversation" data-action="{{if not $resolved}}Resolve{{else}}UnResolve{{end}}" data-comment-id="{{(index $line.Comments 0).ID}}" data-update-url="{{$.root.RepoLink}}/issues/resolve_conversation" >
5455
{{if $resolved}}
5556
{{$.root.i18n.Tr "repo.issues.review.un_resolve_conversation"}}
5657
{{else}}

‎templates/repo/issue/view_content/comments.tmpl

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -379,6 +379,7 @@
379379
{{$invalid := (index $comms 0).Invalidated}}
380380
{{$resolved := (index $comms 0).IsResolved}}
381381
{{$resolveDoer := (index $comms 0).ResolveDoer}}
382+
{{$isNotPending := (not (eq (index $comms 0).Review.Type 0))}}
382383
{{if or $invalid $resolved}}
383384
<button id="show-outdated-{{(index $comms 0).ID}}" data-comment="{{(index $comms 0).ID}}" class="ui compact right labeled button show-outdated">
384385
{{svg "octicon-unfold" 16}}
@@ -445,8 +446,8 @@
445446
</div>
446447
{{template "repo/diff/comment_form_datahandler" dict "hidden" true "reply" (index $comms 0).ReviewID "root" $ "comment" (index $comms 0)}}
447448

448-
{{if $.CanMarkConversation}}
449-
<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 $comms 0).ID}}" data-update-url="{{$.RepoLink}}/issues/resolve_conversation" >
449+
{{if and $.CanMarkConversation $isNotPending}}
450+
<button class="ui green icon tiny button resolve-conversation" data-action="{{if not $resolved}}Resolve{{else}}UnResolve{{end}}" data-comment-id="{{(index $comms 0).ID}}" data-update-url="{{$.RepoLink}}/issues/resolve_conversation" >
450451
{{if $resolved}}
451452
{{$.i18n.Tr "repo.issues.review.un_resolve_conversation"}}
452453
{{else}}

‎web_src/js/index.js

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2570,13 +2570,11 @@ $(document).ready(async () => {
25702570
e.preventDefault();
25712571
const id = $(this).data('comment-id');
25722572
const action = $(this).data('action');
2573-
const issue_id = $(this).data('issue-id');
25742573
const url = $(this).data('update-url');
25752574

25762575
$.post(url, {
25772576
_csrf: csrf,
25782577
action,
2579-
issue_id,
25802578
comment_id: id,
25812579
}).then(reload);
25822580
});

0 commit comments

Comments
 (0)