From 03ba12aabf95ced8398c7859deff76780a4577cd Mon Sep 17 00:00:00 2001 From: zeripath <art27@cantab.net> Date: Fri, 21 Aug 2020 08:52:20 +0100 Subject: [PATCH] Prevent NPE on commenting on lines with invalidated comments (#12549) (#12550) * 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 <art27@cantab.net> * Add hack around to show the broken comments Signed-off-by: Andrew Thornton <art27@cantab.net> --- services/pull/review.go | 2 +- templates/repo/diff/box.tmpl | 5 +- templates/repo/diff/section_unified.tmpl | 5 +- .../repo/issue/view_content/comments.tmpl | 116 ++++++++++++++++++ 4 files changed, 125 insertions(+), 3 deletions(-) diff --git a/services/pull/review.go b/services/pull/review.go index 25e0ca858..5a77a4da1 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 b6d1fdf3a..1f4484fb1 100644 --- a/templates/repo/diff/box.tmpl +++ b/templates/repo/diff/box.tmpl @@ -149,7 +149,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}} <tr class="add-code-comment"> <td class="lines-num"></td> <td class="lines-type-marker"></td> diff --git a/templates/repo/diff/section_unified.tmpl b/templates/repo/diff/section_unified.tmpl index 707822e45..0a0849bad 100644 --- a/templates/repo/diff/section_unified.tmpl +++ b/templates/repo/diff/section_unified.tmpl @@ -25,7 +25,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}} <tr> <td colspan="2" class="lines-num"></td> <td class="add-comment-left add-comment-right" colspan="2"> diff --git a/templates/repo/issue/view_content/comments.tmpl b/templates/repo/issue/view_content/comments.tmpl index 0a146513a..7611e390c 100644 --- a/templates/repo/issue/view_content/comments.tmpl +++ b/templates/repo/issue/view_content/comments.tmpl @@ -366,6 +366,122 @@ </div> {{end}} </div> + {{else if and (eq .Type 21) (eq .ReviewID 0)}} + <div class="timeline-item-group"> + <div class="timeline-item event" id="{{.HashTag}}"> + {{if .OriginalAuthor }} + {{else}} + <a class="timeline-avatar"{{if gt .Poster.ID 0}} href="{{.Poster.HomeLink}}"{{end}}> + <img src="{{.Poster.RelAvatarLink}}"> + </a> + {{end}} + <span class="badge grey">{{svg "octicon-comment" 16}}</span> + <span class="text grey"> + {{if .OriginalAuthor }} + <span class="text black"><i class="fa {{MigrationIcon $.Repository.GetOriginalURLHostname}}" aria-hidden="true"></i> {{ .OriginalAuthor }}</span><span class="text grey"> {{if $.Repository.OriginalURL}}</span><span class="text migrate">({{$.i18n.Tr "repo.migrated_from" $.Repository.OriginalURL $.Repository.GetOriginalURLHostname | Safe }}){{end}}</span> + {{else}} + <a class="author"{{if gt .Poster.ID 0}} href="{{.Poster.HomeLink}}"{{end}}>{{.Poster.GetDisplayName}}</a> + {{end}} + {{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}} + </span> + </div> + <div class="timeline-item event"> + {{$filename := .TreePath}} + {{$line := .Line}} + <div class="ui segments"> + <div class="ui segment"> + {{$invalid := .Invalidated}} + {{$resolved := .IsResolved}} + {{$ignore := .LoadResolveDoer}} + {{$resolveDoer := .ResolveDoer}} + {{$isNotPending := true}} + {{if or $invalid $resolved}} + <button id="show-outdated-{{.ID}}" data-comment="{{.ID}}" class="ui compact right labeled button show-outdated"> + {{svg "octicon-unfold" 16}} + {{if $invalid }} + {{$.i18n.Tr "repo.issues.review.show_outdated"}} + {{else}} + {{$.i18n.Tr "repo.issues.review.show_resolved"}} + {{end}} + </button> + <button id="hide-outdated-{{.ID}}" data-comment="{{.ID}}" class="hide ui compact right labeled button hide-outdated"> + {{svg "octicon-fold" 16}} + {{if $invalid}} + {{$.i18n.Tr "repo.issues.review.hide_outdated"}} + {{else}} + {{$.i18n.Tr "repo.issues.review.hide_resolved"}} + {{end}} + </button> + {{end}} + <a href="{{.CodeCommentURL}}" class="file-comment">{{$filename}}</a> + </div> + {{$diff := (CommentMustAsDiff .)}} + {{if $diff}} + {{$file := (index $diff.Files 0)}} + <div id="code-preview-{{.ID}}" class="ui table segment{{if or $invalid $resolved}} hide{{end}}"> + <div class="diff-file-box diff-box file-content {{TabSizeClass $.Editorconfig $file.Name}}"> + <div class="file-body file-code code-view code-diff code-diff-unified"> + <table> + <tbody> + {{template "repo/diff/section_unified" dict "file" $file "root" $}} + </tbody> + </table> + </div> + </div> + </div> + {{end}} + <div id="code-comments-{{.ID}}" class="ui segment{{if or $invalid $resolved}} hide{{end}}"> + <div class="ui comments"> + {{ $createdSubStr:= TimeSinceUnix .CreatedUnix $.Lang }} + <div class="comment" id="{{.HashTag}}"> + {{if not .OriginalAuthor }} + <a class="avatar"> + <img src="{{.Poster.RelAvatarLink}}"> + </a> + {{end}} + <div class="content"> + <div class="code-comment-content"> + <span class="text grey"> + {{if .OriginalAuthor }} + <span class="text black"><i class="fa {{MigrationIcon $.Repository.GetOriginalURLHostname}}" aria-hidden="true"></i> {{ .OriginalAuthor }}</span><span class="text grey"> {{if $.Repository.OriginalURL}}</span><span class="text migrate">({{$.i18n.Tr "repo.migrated_from" $.Repository.OriginalURL $.Repository.GetOriginalURLHostname | Safe }}){{end}}</span> + {{else}} + <a class="author"{{if gt .Poster.ID 0}} href="{{.Poster.HomeLink}}"{{end}}>{{.Poster.GetDisplayName}}</a> + {{end}} + {{$.i18n.Tr "repo.issues.commented_at" .HashTag $createdSubStr | Safe}} + <div class="text"> + <div class="render-content markdown"> + {{if .RenderedContent}} + {{.RenderedContent|Str2html}} + {{else}} + <span class="no-content">{{$.i18n.Tr "repo.issues.no_content"}}</span> + {{end}} + </div> + <div class="raw-content hide">{{.Content}}</div> + </div> + </span> + </div> + </div> + </div> + </div> + {{template "repo/diff/comment_form_datahandler" dict "hidden" true "reply" .ReviewID "root" $ "comment" .}} + + {{if and $.CanMarkConversation $isNotPending}} + <button class="ui tiny button resolve-conversation" data-action="{{if not $resolved}}Resolve{{else}}UnResolve{{end}}" data-comment-id="{{.ID}}" data-update-url="{{$.RepoLink}}/issues/resolve_conversation" > + {{if $resolved}} + {{$.i18n.Tr "repo.issues.review.un_resolve_conversation"}} + {{else}} + {{$.i18n.Tr "repo.issues.review.resolve_conversation"}} + {{end}} + </button> + {{end}} + + {{if $resolved}} + <span class="ui grey text"><b>{{$resolveDoer.Name}}</b> {{$.i18n.Tr "repo.issues.review.resolved_by"}}</span> + {{end}} + </div> + </div> + </div> + </div> {{else if eq .Type 22}} <div class="timeline-item-group"> <div class="timeline-item event" id="{{.HashTag}}">