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.

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

Merged
merged 24 commits into from
Jun 25, 2017
Merged
Show file tree
Hide file tree
Changes from 19 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
74774a4
Added comment's hashtag to url for mail notifications.
jonasfranz May 4, 2017
f385727
Added comment's hashtag to url for mail notifications.
jonasfranz May 4, 2017
e05eb96
Added comment's hashtag to url for mail notifications.
jonasfranz May 4, 2017
5cb36ca
Added comment's hashtag to url for mail notifications.
jonasfranz May 4, 2017
2dd674e
Replacing in-line link generation with HTMLURL. (+gofmt)
jonasfranz May 4, 2017
716bf81
Replaced action-based model with nil-based model. (+gofmt)
jonasfranz May 4, 2017
edaa576
Replaced mailIssueActionToParticipants with mailIssueCommentToPartici…
jonasfranz May 4, 2017
b0e22ab
Updating comment for mailIssueCommentToParticipants
jonasfranz May 4, 2017
fd70385
Merge branch 'master' of github.com:JonasFranzDEV/gitea
jonasfranz Jun 13, 2017
3c6d4fc
Merge branch 'master' of https://github.com/JonasFranzDEV/gitea
jonasfranz Jun 15, 2017
36ae732
Added link to comment in "Dashboard"
jonasfranz Jun 15, 2017
1826b2b
Added migration
jonasfranz Jun 17, 2017
bb33f2e
Added improved migration to add a CommentID column to action.
jonasfranz Jun 17, 2017
3f1c85c
Added improved migration to add a CommentID column to action.
jonasfranz Jun 17, 2017
5791853
Added improved migration to add a CommentID column to action.
jonasfranz Jun 17, 2017
deddcda
Added improved migration to add a CommentID column to action.
jonasfranz Jun 17, 2017
70d8dd9
Added improved migration to add a CommentID column to action.
jonasfranz Jun 17, 2017
3b99726
Added improved migration to add a CommentID column to action.
jonasfranz Jun 17, 2017
9e18513
Added improved migration to add a CommentID column to action.
jonasfranz Jun 17, 2017
0e20e81
Added improved migration to add a CommentID column to action.
jonasfranz Jun 17, 2017
8818bc7
Added improved links to comments in feed entries. (+ gofmt) Signed-o…
jonasfranz Jun 18, 2017
3e1241c
Fixes #1956 by filtering for deleted comments that are referenced in …
jonasfranz Jun 22, 2017
3ee3dc3
Fixes #1956 by filtering for deleted comments.
jonasfranz Jun 23, 2017
56a64e3
Fixes #1956 by filtering for deleted comments.
jonasfranz Jun 25, 2017
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
35 changes: 35 additions & 0 deletions models/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"fmt"
"path"
"regexp"
"strconv"
"strings"
"time"
"unicode"
Expand Down Expand Up @@ -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"`
Expand Down Expand Up @@ -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 {
Copy link
Member

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 {

return "#"
}

issue, err := GetIssueByID(issueID)

if 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.

Please remove newline before if err != nil {

return "#"
}

return issue.HTMLURL()

Copy link
Member

Choose a reason for hiding this comment

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

Please remove unneeded newline after last return and before both if err != nil {

}

// GetBranch returns the action's repository branch.
func (a *Action) GetBranch() string {
return a.RefName
Expand Down
9 changes: 9 additions & 0 deletions models/issue_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
}

Expand Down Expand Up @@ -641,5 +643,12 @@ func DeleteComment(comment *Comment) error {
}
}

//Delete associated Action
if _, err := sess.Delete(&Action{
Copy link
Member

Choose a reason for hiding this comment

The 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()
}
2 changes: 2 additions & 0 deletions models/migrations/migrations.go
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,8 @@ var migrations = []Migration{
NewMigration("remove columns from action", removeActionColumns),
// v34 -> v35
NewMigration("give all units to owner teams", giveAllUnitsToOwnerTeams),
// v35 -> v36
NewMigration("adds comment to an action", addCommentIDToAction),
}

// Migrate database to current version
Expand Down
24 changes: 24 additions & 0 deletions models/migrations/v35.go
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
}
15 changes: 15 additions & 0 deletions routers/user/home.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{}
Copy link
Member

Choose a reason for hiding this comment

The 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) CommentID, so I don't see any point in caching comments.

repoCache := map[int64]*models.Repository{}
for _, act := range actions {
// Cache results to reduce queries.
Expand Down Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

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

This continue will prevent the action's repo owner from being loaded.

}
ctx.Handle(500, "GetCommentByID", err)
return
}
}
act.Comment = comment

repoOwner, ok := userCache[repo.OwnerID]
if !ok {
repoOwner, err = models.GetUserByID(repo.OwnerID)
Expand All @@ -115,6 +129,7 @@ func retrieveFeeds(ctx *context.Context, user *models.User, includePrivate, isPr
return
}
}

Copy link
Member

@ethantkoenig ethantkoenig Jun 17, 2017

Choose a reason for hiding this comment

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

Please remove this blank line

Copy link
Member Author

@jonasfranz jonasfranz Jun 17, 2017

Choose a reason for hiding this comment

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

Why? gofmt did not remove this line.

Copy link
Member

Choose a reason for hiding this comment

The 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
Expand Down
4 changes: 2 additions & 2 deletions templates/user/dashboard/feeds.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Member

Choose a reason for hiding this comment

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

I dislike using .GetCommentLink for actions that don't have anything to do with comments. I know that .GetCommentLink will return the link to the issue in this case, but it seems sloppy.

IMO, the best solution here is to add IssueID and Issue fields to the Action struct. This would allow us to just use .Issue.HTMLURL here, and .Comment.HTMLURL in line 66. It would also allow us to get rid of .GetIssueInfos, which to me seems like a hack.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The 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.
I think that adding these links does not need to be holed off to another PR.

Copy link
Member

Choose a reason for hiding this comment

The 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>
Expand Down