Skip to content

Commit 03ba12a

Browse files
zeripathzeripath
authored
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 <[email protected]> * Add hack around to show the broken comments Signed-off-by: Andrew Thornton <[email protected]>
1 parent 24ed1b5 commit 03ba12a

File tree

4 files changed

+125
-3
lines changed

4 files changed

+125
-3
lines changed

‎services/pull/review.go

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ func CreateCodeComment(doer *models.User, gitRepo *git.Repository, issue *models
2929
// - Comments that are part of a review
3030
// - Comments that reply to an existing review
3131

32-
if !isReview {
32+
if !isReview && replyReviewID != 0 {
3333
// It's not part of a review; maybe a reply to a review comment or a single comment.
3434
// Check if there are reviews for that line already; if there are, this is a reply
3535
if existsReview, err = models.ReviewExists(issue, treePath, line); err != nil {

‎templates/repo/diff/box.tmpl

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -149,7 +149,10 @@
149149
{{if gt (len $line.Comments) 0}}
150150
{{$resolved := (index $line.Comments 0).IsResolved}}
151151
{{$resolveDoer := (index $line.Comments 0).ResolveDoer}}
152-
{{$isNotPending := (not (eq (index $line.Comments 0).Review.Type 0))}}
152+
{{$isNotPending := false}}
153+
{{if (index $line.Comments 0).Review}}
154+
{{$isNotPending = (not (eq (index $line.Comments 0).Review.Type 0))}}
155+
{{end}}
153156
<tr class="add-code-comment">
154157
<td class="lines-num"></td>
155158
<td class="lines-type-marker"></td>

‎templates/repo/diff/section_unified.tmpl

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,10 @@
2525
{{if gt (len $line.Comments) 0}}
2626
{{$resolved := (index $line.Comments 0).IsResolved}}
2727
{{$resolveDoer := (index $line.Comments 0).ResolveDoer}}
28-
{{$isNotPending := (not (eq (index $line.Comments 0).Review.Type 0))}}
28+
{{$isNotPending := false}}
29+
{{if (index $line.Comments 0).Review}}
30+
{{$isNotPending = (not (eq (index $line.Comments 0).Review.Type 0))}}
31+
{{end}}
2932
<tr>
3033
<td colspan="2" class="lines-num"></td>
3134
<td class="add-comment-left add-comment-right" colspan="2">

‎templates/repo/issue/view_content/comments.tmpl

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -366,6 +366,122 @@
366366
</div>
367367
{{end}}
368368
</div>
369+
{{else if and (eq .Type 21) (eq .ReviewID 0)}}
370+
<div class="timeline-item-group">
371+
<div class="timeline-item event" id="{{.HashTag}}">
372+
{{if .OriginalAuthor }}
373+
{{else}}
374+
<a class="timeline-avatar"{{if gt .Poster.ID 0}} href="{{.Poster.HomeLink}}"{{end}}>
375+
<img src="{{.Poster.RelAvatarLink}}">
376+
</a>
377+
{{end}}
378+
<span class="badge grey">{{svg "octicon-comment" 16}}</span>
379+
<span class="text grey">
380+
{{if .OriginalAuthor }}
381+
<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>
382+
{{else}}
383+
<a class="author"{{if gt .Poster.ID 0}} href="{{.Poster.HomeLink}}"{{end}}>{{.Poster.GetDisplayName}}</a>
384+
{{end}}
385+
{{$.i18n.Tr "repo.issues.review.comment" $createdStr | Safe}}
386+
</span>
387+
</div>
388+
<div class="timeline-item event">
389+
{{$filename := .TreePath}}
390+
{{$line := .Line}}
391+
<div class="ui segments">
392+
<div class="ui segment">
393+
{{$invalid := .Invalidated}}
394+
{{$resolved := .IsResolved}}
395+
{{$ignore := .LoadResolveDoer}}
396+
{{$resolveDoer := .ResolveDoer}}
397+
{{$isNotPending := true}}
398+
{{if or $invalid $resolved}}
399+
<button id="show-outdated-{{.ID}}" data-comment="{{.ID}}" class="ui compact right labeled button show-outdated">
400+
{{svg "octicon-unfold" 16}}
401+
{{if $invalid }}
402+
{{$.i18n.Tr "repo.issues.review.show_outdated"}}
403+
{{else}}
404+
{{$.i18n.Tr "repo.issues.review.show_resolved"}}
405+
{{end}}
406+
</button>
407+
<button id="hide-outdated-{{.ID}}" data-comment="{{.ID}}" class="hide ui compact right labeled button hide-outdated">
408+
{{svg "octicon-fold" 16}}
409+
{{if $invalid}}
410+
{{$.i18n.Tr "repo.issues.review.hide_outdated"}}
411+
{{else}}
412+
{{$.i18n.Tr "repo.issues.review.hide_resolved"}}
413+
{{end}}
414+
</button>
415+
{{end}}
416+
<a href="{{.CodeCommentURL}}" class="file-comment">{{$filename}}</a>
417+
</div>
418+
{{$diff := (CommentMustAsDiff .)}}
419+
{{if $diff}}
420+
{{$file := (index $diff.Files 0)}}
421+
<div id="code-preview-{{.ID}}" class="ui table segment{{if or $invalid $resolved}} hide{{end}}">
422+
<div class="diff-file-box diff-box file-content {{TabSizeClass $.Editorconfig $file.Name}}">
423+
<div class="file-body file-code code-view code-diff code-diff-unified">
424+
<table>
425+
<tbody>
426+
{{template "repo/diff/section_unified" dict "file" $file "root" $}}
427+
</tbody>
428+
</table>
429+
</div>
430+
</div>
431+
</div>
432+
{{end}}
433+
<div id="code-comments-{{.ID}}" class="ui segment{{if or $invalid $resolved}} hide{{end}}">
434+
<div class="ui comments">
435+
{{ $createdSubStr:= TimeSinceUnix .CreatedUnix $.Lang }}
436+
<div class="comment" id="{{.HashTag}}">
437+
{{if not .OriginalAuthor }}
438+
<a class="avatar">
439+
<img src="{{.Poster.RelAvatarLink}}">
440+
</a>
441+
{{end}}
442+
<div class="content">
443+
<div class="code-comment-content">
444+
<span class="text grey">
445+
{{if .OriginalAuthor }}
446+
<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>
447+
{{else}}
448+
<a class="author"{{if gt .Poster.ID 0}} href="{{.Poster.HomeLink}}"{{end}}>{{.Poster.GetDisplayName}}</a>
449+
{{end}}
450+
{{$.i18n.Tr "repo.issues.commented_at" .HashTag $createdSubStr | Safe}}
451+
<div class="text">
452+
<div class="render-content markdown">
453+
{{if .RenderedContent}}
454+
{{.RenderedContent|Str2html}}
455+
{{else}}
456+
<span class="no-content">{{$.i18n.Tr "repo.issues.no_content"}}</span>
457+
{{end}}
458+
</div>
459+
<div class="raw-content hide">{{.Content}}</div>
460+
</div>
461+
</span>
462+
</div>
463+
</div>
464+
</div>
465+
</div>
466+
{{template "repo/diff/comment_form_datahandler" dict "hidden" true "reply" .ReviewID "root" $ "comment" .}}
467+
468+
{{if and $.CanMarkConversation $isNotPending}}
469+
<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" >
470+
{{if $resolved}}
471+
{{$.i18n.Tr "repo.issues.review.un_resolve_conversation"}}
472+
{{else}}
473+
{{$.i18n.Tr "repo.issues.review.resolve_conversation"}}
474+
{{end}}
475+
</button>
476+
{{end}}
477+
478+
{{if $resolved}}
479+
<span class="ui grey text"><b>{{$resolveDoer.Name}}</b> {{$.i18n.Tr "repo.issues.review.resolved_by"}}</span>
480+
{{end}}
481+
</div>
482+
</div>
483+
</div>
484+
</div>
369485
{{else if eq .Type 22}}
370486
<div class="timeline-item-group">
371487
<div class="timeline-item event" id="{{.HashTag}}">

0 commit comments

Comments
 (0)