Fix "Dashboard shows deleted comments" #1995

jonasfranz · 2017-06-17T13:09:44Z

Fixes #1956
Adds a "CommentID" column to the action model (+migration). This is useful for deleting associated actions when a comment should be deleted.
Adds a link to the comment in the associated feed entry.

Signed-off-by: Jonas <info@jonasfranz.software>

Added explanation to return statement. Signed-off-by: Jonas <info@jonasfranz.software>

Added explanation to return statement + documentation. Signed-off-by: Jonas <info@jonasfranz.software>

Signed-off-by: Jonas Franz <info@jonasfranz.software>

…pants. Signed-off-by: Jonas Franz <info@jonasfranz.software>

Signed-off-by: Jonas Franz <info@jonasfranz.software>

Refreshing code base

Deleting feed entry if a comment is going to be deleted Signed-off-by: Jonas Franz <info@jonasfranz.software>

Signed-off-by: Jonas Franz <email@jfdev.de>

Added improved links to comments in feed entries. Signed-off-by: Jonas Franz <email@jfdev.de>

Added improved links to comments in feed entries. (+ gofmt) Signed-off-by: Jonas Franz <email@jfdev.de>

lunny · 2017-06-17T13:22:48Z

models/action.go

@@ -22,6 +22,7 @@ import (
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
+	"strconv"


standard lib should be at the first

Added improved links to comments in feed entries. (+ gofmt) Signed-off-by: Jonas Franz <email@jfdev.de>

cez81 · 2017-06-17T13:59:23Z

models/migrations/v35.go

@@ -0,0 +1,24 @@
+// Copyright 2017 The Gogs Authors. All rights reserved.


Gitea authors

Added improved links to comments in feed entries. (+ gofmt) Signed-off-by: Jonas Franz <email@jfdev.de>

ethantkoenig · 2017-06-17T15:50:12Z

routers/user/home.go

+			comment, err = models.GetCommentByID(act.CommentID)
+			if err != nil {
+				if models.IsErrCommentNotExist(err) {
+					continue


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

ethantkoenig · 2017-06-17T16:00:37Z

templates/user/dashboard/feeds.tmpl

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


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.

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.

Good point about backwards compatibility, I didn't think of that.

ethantkoenig · 2017-06-17T16:03:21Z

routers/user/home.go

@@ -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{}


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.

ethantkoenig · 2017-06-17T16:03:30Z

routers/user/home.go

@@ -115,6 +129,7 @@ func retrieveFeeds(ctx *context.Context, user *models.User, includePrivate, isPr
 				return
 			}
 		}
+


Please remove this blank line

Why? gofmt did not remove this line.

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

Added improved links to comments in feed entries. (+ gofmt) Signed-off-by: Jonas Franz <email@jfdev.de>

…f-by: Jonas Franz <email@jfdev.de>

lunny · 2017-06-18T13:41:20Z

LGTM

lafriks · 2017-06-18T18:23:14Z

models/action.go

+	}
+
+	return issue.HTMLURL()
+


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

bkcsoft · 2017-06-19T07:50:49Z

models/issue_comment.go

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

+	//Delete associated Action
+	if _, err := sess.Delete(&Action{


Please don't delete actions. Rather check in the dashboard if the comment exists or not and act accordingly.

…nced in actions. Signed-off-by: Jonas Franz <email@jfdev.de>

lafriks · 2017-06-22T19:48:41Z

models/action.go

+
+	if !opts.IncludeDeletedComments {
+		// commentID != 0 AND comment exist
+		sess.And("(comment_id ISNULL OR comment_id = 0 OR EXISTS(SELECT NULL FROM comment c WHERE comment_id = c.id))")


Select is invalid as there is no such ISNULL. Also selects with OR are very hard to optimize for dbms so mostly they are slow on large tables and this one is such. I would better prefer if there would be added new bool field that action has been deleted that would be updated to true on deleting comment etc

lafriks · 2017-06-22T19:49:22Z

models/action.go

+	issueIDString := a.GetIssueInfos()[0]
+	issueID, err := strconv.ParseInt(issueIDString, 10, 64)
+
+	if err != nil {


Please remove newline before if err != nil {

lafriks · 2017-06-22T19:49:31Z

models/action.go

+
+	issue, err := GetIssueByID(issueID)
+
+	if err != nil {


Please remove newline before if err != nil {

Signed-off-by: Jonas Franz <email@jfdev.de>

lafriks · 2017-06-23T20:27:23Z

models/action.go

+	Repo           *Repository `xorm:"-"`
+	CommentID      int64       `xorm:"INDEX"`
+	Comment        *Comment    `xorm:"-"`
+	CommentDeleted bool        `xorm:"default 0 not null"`


Do not use 0 and 1 but true and false just like it is used in IsPrivate. Cant this field be reused for other optypes also? So maybe is better call it IsDeleted?

lafriks · 2017-06-23T20:28:24Z

models/issue_comment.go

@@ -641,5 +643,7 @@ func DeleteComment(comment *Comment) error {
 		}
 	}

+	sess.Exec("UPDATE `action` SET comment_deleted = 1 WHERE comment_id = ?", comment.ID)


Use xorm update function

Introducing "IsDeleted" column to action. Signed-off-by: Jonas Franz <email@jfdev.de>

bkcsoft · 2017-06-25T18:19:24Z

LGTM

lafriks · 2017-06-25T18:48:19Z

Does migration updates deleted field for already deleted comments?

bkcsoft · 2017-06-25T19:32:48Z

@lafriks as discussed in Discord, we have no way of knowing since there was no link between Action and Comment before this PR :(

jonasfranz · 2017-06-25T19:37:19Z

@lafriks CommentID will be added by this migration. Before this, theire was no ability to identify the comment except of the column "content" that only contains a small snippet of text from the original comment. So no way to reconstruct the old comment at all. (besides searching based on the given snippet in "content") 😢

lafriks · 2017-06-25T19:41:45Z

@JonasFranzDEV @bkcsoft oh right, than it is fine 👍

jonasfranz and others added 17 commits May 4, 2017 17:58

Added comment's hashtag to url for mail notifications.

74774a4

Signed-off-by: Jonas <info@jonasfranz.software>

Added comment's hashtag to url for mail notifications.

f385727

Added explanation to return statement. Signed-off-by: Jonas <info@jonasfranz.software>

Added comment's hashtag to url for mail notifications.

e05eb96

Added explanation to return statement + documentation. Signed-off-by: Jonas <info@jonasfranz.software>

Added comment's hashtag to url for mail notifications.

5cb36ca

Signed-off-by: Jonas Franz <info@jonasfranz.software>

Replacing in-line link generation with HTMLURL. (+gofmt)

2dd674e

Signed-off-by: Jonas Franz <info@jonasfranz.software>

Replaced action-based model with nil-based model. (+gofmt)

716bf81

Signed-off-by: Jonas Franz <info@jonasfranz.software>

Replaced mailIssueActionToParticipants with mailIssueCommentToPartici…

edaa576

…pants. Signed-off-by: Jonas Franz <info@jonasfranz.software>

Updating comment for mailIssueCommentToParticipants

b0e22ab

Signed-off-by: Jonas Franz <info@jonasfranz.software>

Merge branch 'master' of github.com:JonasFranzDEV/gitea

fd70385

Refreshing code base

Merge branch 'master' of https://github.com/JonasFranzDEV/gitea

3c6d4fc

Added link to comment in "Dashboard"

36ae732

Deleting feed entry if a comment is going to be deleted Signed-off-by: Jonas Franz <info@jonasfranz.software>

Added migration

1826b2b

Signed-off-by: Jonas Franz <email@jfdev.de>

Added improved migration to add a CommentID column to action.

bb33f2e

Added improved links to comments in feed entries. Signed-off-by: Jonas Franz <email@jfdev.de>

Added improved migration to add a CommentID column to action.

3f1c85c

Added improved links to comments in feed entries. Signed-off-by: Jonas Franz <email@jfdev.de>

Added improved migration to add a CommentID column to action.

5791853

Added improved links to comments in feed entries. Signed-off-by: Jonas Franz <email@jfdev.de>

Added improved migration to add a CommentID column to action.

deddcda

Added improved links to comments in feed entries. (+ gofmt) Signed-off-by: Jonas Franz <email@jfdev.de>

Added improved migration to add a CommentID column to action.

70d8dd9

Added improved links to comments in feed entries. (+ gofmt) Signed-off-by: Jonas Franz <email@jfdev.de>

lunny reviewed Jun 17, 2017

View reviewed changes

Added improved migration to add a CommentID column to action.

3b99726

Added improved links to comments in feed entries. (+ gofmt) Signed-off-by: Jonas Franz <email@jfdev.de>

cez81 reviewed Jun 17, 2017

View reviewed changes

Added improved migration to add a CommentID column to action.

9e18513

Added improved links to comments in feed entries. (+ gofmt) Signed-off-by: Jonas Franz <email@jfdev.de>

lunny added this to the 1.2.0 milestone Jun 17, 2017

lunny added the type/bug label Jun 17, 2017

ethantkoenig suggested changes Jun 17, 2017

View reviewed changes

jonasfranz added 2 commits June 17, 2017 18:26

Added improved migration to add a CommentID column to action.

0e20e81

Added improved links to comments in feed entries. (+ gofmt) Signed-off-by: Jonas Franz <email@jfdev.de>

Added improved links to comments in feed entries. (+ gofmt) Signed-of…

8818bc7

…f-by: Jonas Franz <email@jfdev.de>

ethantkoenig approved these changes Jun 18, 2017

View reviewed changes

tboerger added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Jun 18, 2017

lafriks requested changes Jun 18, 2017

View reviewed changes

models/action.go Outdated

}

return issue.HTMLURL()

Copy link

Member

lafriks Jun 18, 2017

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 {

bkcsoft suggested changes Jun 19, 2017

View reviewed changes

Fixes go-gitea#1956 by filtering for deleted comments that are refere…

3e1241c

…nced in actions. Signed-off-by: Jonas Franz <email@jfdev.de>

lafriks requested changes Jun 22, 2017

View reviewed changes

Fixes go-gitea#1956 by filtering for deleted comments.

3ee3dc3

Signed-off-by: Jonas Franz <email@jfdev.de>

lafriks requested changes Jun 23, 2017

View reviewed changes

Fixes go-gitea#1956 by filtering for deleted comments.

56a64e3

Introducing "IsDeleted" column to action. Signed-off-by: Jonas Franz <email@jfdev.de>

bkcsoft approved these changes Jun 25, 2017

View reviewed changes

tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jun 25, 2017

bkcsoft merged commit 441986a into go-gitea:master Jun 25, 2017

go-gitea locked and limited conversation to collaborators Nov 23, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix "Dashboard shows deleted comments" #1995

Fix "Dashboard shows deleted comments" #1995

jonasfranz commented Jun 17, 2017

lunny Jun 17, 2017

jonasfranz Jun 17, 2017

cez81 Jun 17, 2017

jonasfranz Jun 17, 2017

ethantkoenig Jun 17, 2017

ethantkoenig Jun 17, 2017

jonasfranz Jun 17, 2017

ethantkoenig Jun 17, 2017

ethantkoenig Jun 17, 2017

ethantkoenig Jun 17, 2017 •

edited

Loading

jonasfranz Jun 17, 2017 •

edited

Loading

ethantkoenig Jun 17, 2017

lunny commented Jun 18, 2017

lafriks Jun 18, 2017

bkcsoft Jun 19, 2017

lafriks Jun 22, 2017 •

edited

Loading

lafriks Jun 22, 2017

lafriks Jun 22, 2017

lafriks Jun 23, 2017

lafriks Jun 23, 2017

bkcsoft commented Jun 25, 2017

lafriks commented Jun 25, 2017

bkcsoft commented Jun 25, 2017

jonasfranz commented Jun 25, 2017

lafriks commented Jun 25, 2017

		@@ -0,0 +1,24 @@
		// Copyright 2017 The Gogs Authors. All rights reserved.

Fix "Dashboard shows deleted comments" #1995

Fix "Dashboard shows deleted comments" #1995

Conversation

jonasfranz commented Jun 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethantkoenig Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

jonasfranz Jun 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Jun 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lafriks Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bkcsoft commented Jun 25, 2017

lafriks commented Jun 25, 2017

bkcsoft commented Jun 25, 2017

jonasfranz commented Jun 25, 2017

lafriks commented Jun 25, 2017

ethantkoenig Jun 17, 2017 •

edited

Loading

jonasfranz Jun 17, 2017 •

edited

Loading

lafriks Jun 22, 2017 •

edited

Loading