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

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

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>

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

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