Prevent NPE on commenting on lines with invalidated comments

[zeripath]

#12239 reports an NPE when viewing the diff page of a PR when comments are made on a line that has previously had comments invalidated on it.

@mrsdizzie discovered the mechanism and reason this occurs.

• This PR fixes the above NPE by setting a comment without Review to Comment Type.
• It prevents a review being assigned to 0 by only checking for a review if we are replying to a previous review.
• Finally it adds a section into the comments template to show these reviewless comments.

Fix #12239

Signed-off-by: Andrew Thornton [email protected]

[mrsdizzie]

I think the problem with replyReviewID != 0 is that it will match for all real replies which do have replyReviewID set (it is only 0 for an initial review comment) and so then all comments on a review will have the 'Review' Tag displayed and have their own review entry in the database. The tag part is mostly cosmetic but not sure how it might effect other code for the database to consider every comment a separate full review.

Shouldn't it be better to just fix models.ReviewExists which seems to have broken logic now that we have code to invalidate reviews?

Another small issue is that the resolve conversation option for the 'reviewless' comments only shows on the main discussion but not the files view. I think thats OK though it is easy enough to dismiss these.

[codecov-commenter]

Codecov Report

Merging #12548 into master will increase coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master   #12548      +/-   ##
==========================================
+ Coverage   43.48%   43.50%   +0.01%     
==========================================
  Files         642      642              
  Lines       71040    71040              
==========================================
+ Hits        30893    30904      +11     
+ Misses      35136    35127       -9     
+ Partials     5011     5009       -2     

Impacted Files	Coverage Δ
services/pull/review.go	47.74% <0.00%> (ø)
modules/indexer/stats/queue.go	52.94% <0.00%> (-23.53%)	⬇️
modules/indexer/stats/db.go	43.47% <0.00%> (-8.70%)	⬇️
modules/queue/workerpool.go	58.77% <0.00%> (-1.23%)	⬇️
models/error.go	35.32% <0.00%> (+0.51%)	⬆️
modules/queue/unique_queue_disk_channel.go	55.38% <0.00%> (+1.53%)	⬆️
modules/process/manager.go	75.00% <0.00%> (+2.50%)	⬆️
services/pull/patch.go	68.96% <0.00%> (+2.58%)	⬆️
services/pull/temp_repo.go	29.78% <0.00%> (+3.19%)	⬆️
services/pull/check.go	53.07% <0.00%> (+5.38%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c6943cc...e7d014b. Read the comment docs.

[zeripath]

OK I've figured out how to create a migration for these and have a different fix which is 1.13 only in #12549

@mrsdizzie When you're replying to a comment you actually pass in the review which you're replying to.

[zeripath]

We should use these commits as the basis of a backport. Therefore I'll close this and if #12549 is merged the backported should grab these commits and rebase them on 1.12 for the backport.