-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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.
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
Fix "Dashboard shows deleted comments" #1995
Changes from 19 commits
74774a4
f385727
e05eb96
5cb36ca
2dd674e
716bf81
edaa576
b0e22ab
fd70385
3c6d4fc
36ae732
1826b2b
bb33f2e
3f1c85c
5791853
deddcda
70d8dd9
3b99726
9e18513
0e20e81
8818bc7
3e1241c
3ee3dc3
56a64e3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,6 +9,7 @@ import ( | |
"fmt" | ||
"path" | ||
"regexp" | ||
"strconv" | ||
"strings" | ||
"time" | ||
"unicode" | ||
|
@@ -77,6 +78,8 @@ type Action struct { | |
ActUser *User `xorm:"-"` | ||
RepoID int64 `xorm:"INDEX"` | ||
Repo *Repository `xorm:"-"` | ||
CommentID int64 `xorm:"INDEX"` | ||
Comment *Comment `xorm:"-"` | ||
RefName string | ||
IsPrivate bool `xorm:"INDEX NOT NULL DEFAULT false"` | ||
Content string `xorm:"TEXT"` | ||
|
@@ -191,6 +194,38 @@ func (a *Action) GetRepoLink() string { | |
return "/" + a.GetRepoPath() | ||
} | ||
|
||
// GetCommentLink returns link to action comment. | ||
func (a *Action) GetCommentLink() string { | ||
if a == nil { | ||
return "#" | ||
} | ||
if a.Comment == nil && a.CommentID != 0 { | ||
a.Comment, _ = GetCommentByID(a.CommentID) | ||
} | ||
if a.Comment != nil { | ||
return a.Comment.HTMLURL() | ||
} | ||
if len(a.GetIssueInfos()) == 0 { | ||
return "#" | ||
} | ||
//Return link to issue | ||
issueIDString := a.GetIssueInfos()[0] | ||
issueID, err := strconv.ParseInt(issueIDString, 10, 64) | ||
|
||
if err != nil { | ||
return "#" | ||
} | ||
|
||
issue, err := GetIssueByID(issueID) | ||
|
||
if err != nil { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove newline before |
||
return "#" | ||
} | ||
|
||
return issue.HTMLURL() | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove unneeded newline after last return and before both |
||
} | ||
|
||
// GetBranch returns the action's repository branch. | ||
func (a *Action) GetBranch() string { | ||
return a.RefName | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -334,6 +334,8 @@ func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err | |
Content: fmt.Sprintf("%d|%s", opts.Issue.Index, strings.Split(opts.Content, "\n")[0]), | ||
RepoID: opts.Repo.ID, | ||
Repo: opts.Repo, | ||
Comment: comment, | ||
CommentID: comment.ID, | ||
IsPrivate: opts.Repo.IsPrivate, | ||
} | ||
|
||
|
@@ -641,5 +643,12 @@ func DeleteComment(comment *Comment) error { | |
} | ||
} | ||
|
||
//Delete associated Action | ||
if _, err := sess.Delete(&Action{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please don't delete actions. Rather check in the dashboard if the comment exists or not and act accordingly. |
||
CommentID: comment.ID, | ||
}); err != nil { | ||
return err | ||
} | ||
|
||
return sess.Commit() | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
// Copyright 2017 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 ( | ||
"fmt" | ||
|
||
"github.com/go-xorm/xorm" | ||
) | ||
|
||
func addCommentIDToAction(x *xorm.Engine) error { | ||
// Action see models/action.go | ||
type Action struct { | ||
CommentID int64 `xorm:"INDEX"` | ||
} | ||
|
||
if err := x.Sync2(new(Action)); err != nil { | ||
return fmt.Errorf("Sync2: %v", err) | ||
} | ||
|
||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -74,6 +74,7 @@ func retrieveFeeds(ctx *context.Context, user *models.User, includePrivate, isPr | |
if ctx.User != nil { | ||
userCache[ctx.User.ID] = ctx.User | ||
} | ||
commentCache := map[int64]*models.Comment{} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As far as I can tell, there will never be two actions with the same (non-zero) |
||
repoCache := map[int64]*models.Repository{} | ||
for _, act := range actions { | ||
// Cache results to reduce queries. | ||
|
@@ -104,6 +105,19 @@ func retrieveFeeds(ctx *context.Context, user *models.User, includePrivate, isPr | |
} | ||
act.Repo = repo | ||
|
||
comment, ok := commentCache[act.CommentID] | ||
if !ok { | ||
comment, err = models.GetCommentByID(act.CommentID) | ||
if err != nil { | ||
if models.IsErrCommentNotExist(err) { | ||
continue | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This |
||
} | ||
ctx.Handle(500, "GetCommentByID", err) | ||
return | ||
} | ||
} | ||
act.Comment = comment | ||
|
||
repoOwner, ok := userCache[repo.OwnerID] | ||
if !ok { | ||
repoOwner, err = models.GetUserByID(repo.OwnerID) | ||
|
@@ -115,6 +129,7 @@ func retrieveFeeds(ctx *context.Context, user *models.User, includePrivate, isPr | |
return | ||
} | ||
} | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remove this blank line There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why? gofmt did not remove this line. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nor did gofmt add it 😄. I'm generally opposed to unnecessary whitespace changes, particularly when they are far away from a commit's actual changes; they end up making the git history more complex (i.e. looking at a file's blame). |
||
repo.Owner = repoOwner | ||
} | ||
ctx.Data["Feeds"] = actions | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -63,12 +63,12 @@ | |
{{else if eq .GetOpType 7}} | ||
<span class="text truncate issue title has-emoji">{{index .GetIssueInfos 1}}</span> | ||
{{else if eq .GetOpType 10}} | ||
<span class="text truncate issue title has-emoji">{{.GetIssueTitle}}</span> | ||
<a href="{{.GetCommentLink}}" class="text truncate issue title has-emoji">{{.GetIssueTitle}}</a> | ||
<p class="text light grey has-emoji">{{index .GetIssueInfos 1}}</p> | ||
{{else if eq .GetOpType 11}} | ||
<p class="text light grey has-emoji">{{index .GetIssueInfos 1}}</p> | ||
{{else if (or (or (eq .GetOpType 12) (eq .GetOpType 13)) (or (eq .GetOpType 14) (eq .GetOpType 15)))}} | ||
<span class="text truncate issue title has-emoji">{{.GetIssueTitle}}</span> | ||
<a href="{{.GetCommentLink}}" class="text truncate issue title has-emoji">{{.GetIssueTitle}}</a> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I dislike using IMO, the best solution here is to add Would it be possible to hold off adding these links to a separate PR? The links are unrelated to fixing the deleted-comment bug, and it seems to me that the best way to add support for the links requires some more refactoring. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that the best solution is that I leave the links in place (except the link you mentioned). The .GetCommentLink function is also needed for backward-compatibility reasons. So actions that created before the update won't have a comment_id assigned. So .Comment.HTMLURL would not work. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point about backwards compatibility, I didn't think of that. |
||
{{end}} | ||
<p class="text italic light grey">{{TimeSince .GetCreate $.i18n.Lang}}</p> | ||
</div> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove newline before
if err != nil {