From 309fd941612a8d278e4520a22a32d19f03a7b75f Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 20 Aug 2020 17:50:35 +0100 Subject: [PATCH 1/3] Prevent NPE on commenting on lines with invalidated comments Only check for a review if we are replying to a previous review. Prevent the NPE in #12239 by assuming that a comment without a Review is non-pending. Fix #12239 Signed-off-by: Andrew Thornton --- services/pull/review.go | 2 +- templates/repo/diff/box.tmpl | 5 ++++- templates/repo/diff/section_unified.tmpl | 5 ++++- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/services/pull/review.go b/services/pull/review.go index 25e0ca858a6cc..5a77a4da16842 100644 --- a/services/pull/review.go +++ b/services/pull/review.go @@ -29,7 +29,7 @@ func CreateCodeComment(doer *models.User, gitRepo *git.Repository, issue *models // - Comments that are part of a review // - Comments that reply to an existing review - if !isReview { + if !isReview && replyReviewID != 0 { // It's not part of a review; maybe a reply to a review comment or a single comment. // Check if there are reviews for that line already; if there are, this is a reply if existsReview, err = models.ReviewExists(issue, treePath, line); err != nil { diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index 7338c1ee67679..397356fe6e648 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -155,7 +155,10 @@ {{if gt (len $line.Comments) 0}} {{$resolved := (index $line.Comments 0).IsResolved}} {{$resolveDoer := (index $line.Comments 0).ResolveDoer}} - {{$isNotPending := (not (eq (index $line.Comments 0).Review.Type 0))}} + {{$isNotPending := false}} + {{if (index $line.Comments 0).Review}} + {{$isNotPending = (not (eq (index $line.Comments 0).Review.Type 0))}} + {{end}} diff --git a/templates/repo/diff/section_unified.tmpl b/templates/repo/diff/section_unified.tmpl index 5f87a4176c953..2508c01fea4d9 100644 --- a/templates/repo/diff/section_unified.tmpl +++ b/templates/repo/diff/section_unified.tmpl @@ -35,7 +35,10 @@ {{if gt (len $line.Comments) 0}} {{$resolved := (index $line.Comments 0).IsResolved}} {{$resolveDoer := (index $line.Comments 0).ResolveDoer}} - {{$isNotPending := (not (eq (index $line.Comments 0).Review.Type 0))}} + {{$isNotPending := false}} + {{if (index $line.Comments 0).Review}} + {{$isNotPending = (not (eq (index $line.Comments 0).Review.Type 0))}} + {{end}} From 80ab986b21019b316af8e5f5ac4726f443897cea Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 20 Aug 2020 20:34:42 +0100 Subject: [PATCH 2/3] Add hack around to show the broken comments Signed-off-by: Andrew Thornton --- .../repo/issue/view_content/comments.tmpl | 116 ++++++++++++++++++ 1 file changed, 116 insertions(+) diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 850c5b91577f3..13589f1063251 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -367,6 +367,122 @@ {{end}} + {{else if and (eq .Type 21) (eq .ReviewID 0)}} +
+
+ {{if .OriginalAuthor }} + {{else}} + + + + {{end}} + {{svg "octicon-comment" 16}} + + {{if .OriginalAuthor }} + {{ .OriginalAuthor }} {{if $.Repository.OriginalURL}}({{$.i18n.Tr "repo.migrated_from" $.Repository.OriginalURL $.Repository.GetOriginalURLHostname | Safe }}){{end}} + {{else}} + {{.Poster.GetDisplayName}} + {{end}} + {{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}} + +
+
+ {{$filename := .TreePath}} + {{$line := .Line}} +
+
+ {{$invalid := .Invalidated}} + {{$resolved := .IsResolved}} + {{$ignore := .LoadResolveDoer}} + {{$resolveDoer := .ResolveDoer}} + {{$isNotPending := true}} + {{if or $invalid $resolved}} + + + {{end}} + {{$filename}} +
+ {{$diff := (CommentMustAsDiff .)}} + {{if $diff}} + {{$file := (index $diff.Files 0)}} +
+
+
+ + + {{template "repo/diff/section_unified" dict "file" $file "root" $}} + +
+
+
+
+ {{end}} +
+
+ {{ $createdSubStr:= TimeSinceUnix .CreatedUnix $.Lang }} +
+ {{if not .OriginalAuthor }} + + + + {{end}} +
+
+ + {{if .OriginalAuthor }} + {{ .OriginalAuthor }} {{if $.Repository.OriginalURL}}({{$.i18n.Tr "repo.migrated_from" $.Repository.OriginalURL $.Repository.GetOriginalURLHostname | Safe }}){{end}} + {{else}} + {{.Poster.GetDisplayName}} + {{end}} + {{$.i18n.Tr "repo.issues.commented_at" .HashTag $createdSubStr | Safe}} +
+
+ {{if .RenderedContent}} + {{.RenderedContent|Str2html}} + {{else}} + {{$.i18n.Tr "repo.issues.no_content"}} + {{end}} +
+
{{.Content}}
+
+ +
+
+
+
+ {{template "repo/diff/comment_form_datahandler" dict "hidden" true "reply" .ReviewID "root" $ "comment" .}} + + {{if and $.CanMarkConversation $isNotPending}} + + {{end}} + + {{if $resolved}} + {{$resolveDoer.Name}} {{$.i18n.Tr "repo.issues.review.resolved_by"}} + {{end}} +
+
+
+
{{else if eq .Type 22}}
From 16fd2c3cf25cba90669909adbe6f3de4bc1aebdf Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Thu, 20 Aug 2020 22:20:15 +0100 Subject: [PATCH 3/3] Add migration and remove template hacks Signed-off-by: Andrew Thornton --- custom/conf/app.example.ini | 2 +- models/migrations/migrations.go | 2 + models/migrations/v147.go | 132 ++++++++++++++++++ modules/git/hook.go | 2 +- templates/repo/diff/box.tmpl | 5 +- templates/repo/diff/section_unified.tmpl | 5 +- .../repo/issue/view_content/comments.tmpl | 116 --------------- 7 files changed, 138 insertions(+), 126 deletions(-) create mode 100644 models/migrations/v147.go diff --git a/custom/conf/app.example.ini b/custom/conf/app.example.ini index bbd3b5bc3652e..f3030edd822c9 100644 --- a/custom/conf/app.example.ini +++ b/custom/conf/app.example.ini @@ -985,7 +985,7 @@ NAMES = English,简体中文,繁體中文（香港）,繁體中文（台灣）,D ; Two Factor authentication with security keys ; https://developers.yubico.com/U2F/App_ID.html ;APP_ID = http://localhost:3000/ -; Comma seperated list of trusted facets +; Comma separated list of trusted facets ;TRUSTED_FACETS = http://localhost:3000/ ; Extension mapping to highlight class diff --git a/models/migrations/migrations.go b/models/migrations/migrations.go index b9fdfbfe7efe1..721b045fdce6c 100644 --- a/models/migrations/migrations.go +++ b/models/migrations/migrations.go @@ -226,6 +226,8 @@ var migrations = []Migration{ NewMigration("Increase Language field to 50 in LanguageStats", increaseLanguageField), // v146 -> v147 NewMigration("Add projects info to repository table", addProjectsInfo), + // v147 -> v148 + NewMigration("create review for 0 review id code comments", createReviewsForCodeComments), } // GetCurrentDBVersion returns the current db version diff --git a/models/migrations/v147.go b/models/migrations/v147.go new file mode 100644 index 0000000000000..9716d6e83b893 --- /dev/null +++ b/models/migrations/v147.go @@ -0,0 +1,132 @@ +// Copyright 2020 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 ( + "code.gitea.io/gitea/modules/timeutil" + + "xorm.io/xorm" +) + +func createReviewsForCodeComments(x *xorm.Engine) error { + // Review + type Review struct { + ID int64 `xorm:"pk autoincr"` + Type int + ReviewerID int64 `xorm:"index"` + OriginalAuthor string + OriginalAuthorID int64 + IssueID int64 `xorm:"index"` + Content string `xorm:"TEXT"` + // Official is a review made by an assigned approver (counts towards approval) + Official bool `xorm:"NOT NULL DEFAULT false"` + CommitID string `xorm:"VARCHAR(40)"` + Stale bool `xorm:"NOT NULL DEFAULT false"` + + CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` + UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` + } + + const ReviewTypeComment = 2 + + // Comment represents a comment in commit and issue page. + type Comment struct { + ID int64 `xorm:"pk autoincr"` + Type int `xorm:"INDEX"` + PosterID int64 `xorm:"INDEX"` + OriginalAuthor string + OriginalAuthorID int64 + IssueID int64 `xorm:"INDEX"` + LabelID int64 + OldProjectID int64 + ProjectID int64 + OldMilestoneID int64 + MilestoneID int64 + AssigneeID int64 + RemovedAssignee bool + ResolveDoerID int64 + OldTitle string + NewTitle string + OldRef string + NewRef string + DependentIssueID int64 + + CommitID int64 + Line int64 // - previous line / + proposed line + TreePath string + Content string `xorm:"TEXT"` + + // Path represents the 4 lines of code cemented by this comment + PatchQuoted string `xorm:"TEXT patch"` + + CreatedUnix timeutil.TimeStamp `xorm:"INDEX created"` + UpdatedUnix timeutil.TimeStamp `xorm:"INDEX updated"` + + // Reference issue in commit message + CommitSHA string `xorm:"VARCHAR(40)"` + + ReviewID int64 `xorm:"index"` + Invalidated bool + + // Reference an issue or pull from another comment, issue or PR + // All information is about the origin of the reference + RefRepoID int64 `xorm:"index"` // Repo where the referencing + RefIssueID int64 `xorm:"index"` + RefCommentID int64 `xorm:"index"` // 0 if origin is Issue title or content (or PR's) + RefAction int `xorm:"SMALLINT"` // What hapens if RefIssueID resolves + RefIsPull bool + } + + if err := x.Sync2(new(Review), new(Comment)); err != nil { + return err + } + sess := x.NewSession() + defer sess.Close() + if err := sess.Begin(); err != nil { + return err + } + if err := sess.Where("review_id = 0 and type = 21").Iterate(new(Comment), func(idx int, bean interface{}) error { + comment := bean.(*Comment) + + review := &Review{ + Type: ReviewTypeComment, + ReviewerID: comment.PosterID, + IssueID: comment.IssueID, + Official: false, + CommitID: comment.CommitSHA, + Stale: comment.Invalidated, + OriginalAuthor: comment.OriginalAuthor, + OriginalAuthorID: comment.OriginalAuthorID, + CreatedUnix: comment.CreatedUnix, + UpdatedUnix: comment.CreatedUnix, + } + if _, err := sess.NoAutoTime().Insert(review); err != nil { + return err + } + + reviewComment := &Comment{ + Type: 22, + PosterID: comment.PosterID, + Content: "", + IssueID: comment.IssueID, + ReviewID: review.ID, + OriginalAuthor: comment.OriginalAuthor, + OriginalAuthorID: comment.OriginalAuthorID, + CreatedUnix: comment.CreatedUnix, + UpdatedUnix: comment.CreatedUnix, + } + if _, err := sess.NoAutoTime().Insert(reviewComment); err != nil { + return err + } + + comment.ReviewID = review.ID + _, err := sess.ID(comment.ID).Cols("review_id").NoAutoTime().Update(comment) + return err + }); err != nil { + return err + } + + return sess.Commit() +} diff --git a/modules/git/hook.go b/modules/git/hook.go index 7b75405901e7f..2b9f90efd3a50 100644 --- a/modules/git/hook.go +++ b/modules/git/hook.go @@ -125,7 +125,7 @@ const ( HookPathUpdate = "hooks/update" ) -// SetUpdateHook writes given content to update hook of the reposiotry. +// SetUpdateHook writes given content to update hook of the repository. func SetUpdateHook(repoPath, content string) (err error) { log("Setting update hook: %s", repoPath) hookPath := path.Join(repoPath, HookPathUpdate) diff --git a/templates/repo/diff/box.tmpl b/templates/repo/diff/box.tmpl index 397356fe6e648..7338c1ee67679 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -155,10 +155,7 @@ {{if gt (len $line.Comments) 0}} {{$resolved := (index $line.Comments 0).IsResolved}} {{$resolveDoer := (index $line.Comments 0).ResolveDoer}} - {{$isNotPending := false}} - {{if (index $line.Comments 0).Review}} - {{$isNotPending = (not (eq (index $line.Comments 0).Review.Type 0))}} - {{end}} + {{$isNotPending := (not (eq (index $line.Comments 0).Review.Type 0))}} diff --git a/templates/repo/diff/section_unified.tmpl b/templates/repo/diff/section_unified.tmpl index 2508c01fea4d9..5f87a4176c953 100644 --- a/templates/repo/diff/section_unified.tmpl +++ b/templates/repo/diff/section_unified.tmpl @@ -35,10 +35,7 @@ {{if gt (len $line.Comments) 0}} {{$resolved := (index $line.Comments 0).IsResolved}} {{$resolveDoer := (index $line.Comments 0).ResolveDoer}} - {{$isNotPending := false}} - {{if (index $line.Comments 0).Review}} - {{$isNotPending = (not (eq (index $line.Comments 0).Review.Type 0))}} - {{end}} + {{$isNotPending := (not (eq (index $line.Comments 0).Review.Type 0))}} diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 13589f1063251..850c5b91577f3 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -367,122 +367,6 @@
{{end}}
- {{else if and (eq .Type 21) (eq .ReviewID 0)}} -
-
- {{if .OriginalAuthor }} - {{else}} - - - - {{end}} - {{svg "octicon-comment" 16}} - - {{if .OriginalAuthor }} - {{ .OriginalAuthor }} {{if $.Repository.OriginalURL}}({{$.i18n.Tr "repo.migrated_from" $.Repository.OriginalURL $.Repository.GetOriginalURLHostname | Safe }}){{end}} - {{else}} - {{.Poster.GetDisplayName}} - {{end}} - {{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}} - -
-
- {{$filename := .TreePath}} - {{$line := .Line}} -
-
- {{$invalid := .Invalidated}} - {{$resolved := .IsResolved}} - {{$ignore := .LoadResolveDoer}} - {{$resolveDoer := .ResolveDoer}} - {{$isNotPending := true}} - {{if or $invalid $resolved}} - - - {{end}} - {{$filename}} -
- {{$diff := (CommentMustAsDiff .)}} - {{if $diff}} - {{$file := (index $diff.Files 0)}} -
-
-
- - - {{template "repo/diff/section_unified" dict "file" $file "root" $}} - -
-
-
-
- {{end}} -
-
- {{ $createdSubStr:= TimeSinceUnix .CreatedUnix $.Lang }} -
- {{if not .OriginalAuthor }} - - - - {{end}} -
-
- - {{if .OriginalAuthor }} - {{ .OriginalAuthor }} {{if $.Repository.OriginalURL}}({{$.i18n.Tr "repo.migrated_from" $.Repository.OriginalURL $.Repository.GetOriginalURLHostname | Safe }}){{end}} - {{else}} - {{.Poster.GetDisplayName}} - {{end}} - {{$.i18n.Tr "repo.issues.commented_at" .HashTag $createdSubStr | Safe}} -
-
- {{if .RenderedContent}} - {{.RenderedContent|Str2html}} - {{else}} - {{$.i18n.Tr "repo.issues.no_content"}} - {{end}} -
-
{{.Content}}
-
- -
-
-
-
- {{template "repo/diff/comment_form_datahandler" dict "hidden" true "reply" .ReviewID "root" $ "comment" .}} - - {{if and $.CanMarkConversation $isNotPending}} - - {{end}} - - {{if $resolved}} - {{$resolveDoer.Name}} {{$.i18n.Tr "repo.issues.review.resolved_by"}} - {{end}} -
-
-
-
{{else if eq .Type 22}}