Launchpad itself

are code review comments editable?

Asked by Seth Arnold

Hello, I recently made a code review with one in-line comment.

When I saw the result, it looked lazier than I expected :) so I tried to edit my review. The little yellow edit button was visible on both:

https://code.launchpad.net/~ubuntu-security/ubuntu-cve-tracker/+git/ubuntu-cve-tracker/+merge/424433
and:
https://code.launchpad.net/~ubuntu-security/ubuntu-cve-tracker/+git/ubuntu-cve-tracker/+merge/424433/comments/1122197

but clicking on the little yellow edit indicator didn't seem to do anything.

If code reviews are supposed to be editable, then something isn't hooked up quite right.

If code reviews aren't supposed to be editable, perhaps we could remove the little yellow edit icons?

(Obligatory, I'm a NoScript on firefox user; launchpad otherwise seems to work pretty well, so I don't think it's relevant, but it's mandatory to point it out. :)

Thanks

Question information

Language:
English Edit question
Status:
Solved
For:
Launchpad itself Edit question
Assignee:
No assignee Edit question
Solved by:
Guruprasad
Solved:
Last query:
Last reply:
Revision history for this message
Bernard Stafford (bernard010) said : 		#1

https://code.launchpad.net/~ubuntu-security/ubuntu-cve-tracker/+git/ubuntu-cve-tracker/+merge/424433/comments/1122197

By: Leonidas S. Barbosa
oops, fixed. Thanks :)

Revision history for this message
Seth Arnold (seth-arnold) said : 		#2

Hello Bernard, I still can't use the yellow "edit" icon to edit my code review message; what exactly are you trying to say?

Thanks

Revision history for this message
Guruprasad (lgp171188) said : 		#3

If you try to edit a comment immediately after submitting it, I have seen that clicking the button doesn't work. But if you refresh the page and try again, it should work. This is a know issue. Can you check and let us know if this helped?

Revision history for this message
Seth Arnold (seth-arnold) said : 		#4

Hello Guruprasad, I can't edit my code review comment via either of these urls:

https://code.launchpad.net/~ubuntu-security/ubuntu-cve-tracker/+git/ubuntu-cve-tracker/+merge/424433
https://code.launchpad.net/~ubuntu-security/ubuntu-cve-tracker/+git/ubuntu-cve-tracker/+merge/424433/comments/1122197

I'm starting to think that code reviews are immutable.

Thanks

Revision history for this message
Jürgen Gmach (jugmac00) said : 		#5

@Guruprasad I think Seth refers to code review comments, and you think of MP comments.

While the latter are editable, the former are unfortunately not.

And it would be great to have them editable, too.

@Seth Would you please create a feature request?

Revision history for this message
Guruprasad (lgp171188) said (last edit ): 		#6

Jürgen, the links shared by Seth in his previous comment are for an MP comment and not an inline code review comment, which do not even have a yellow edit button. So it does look like he is talking about MP comments.

From what I found, the edit button works in the merge proposal page (I used https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/424745) once you reload the page after submitting the comment.

See https://people.canonical.com/~guruprasad/review_comment_with_edit_button.png and https://people.canonical.com/~guruprasad/review_comment_editing.png.

However, when I open the permalink for the same comment (https://code.launchpad.net/~lgp171188/launchpad/+git/launchpad/+merge/424745/comments/1123021) and click the edit button that page, clicking the edit button does nothing.

See https://people.canonical.com/~guruprasad/review_comment_edit_not_working_permalink.png.

So this looks like an issue with editing a code review comment on its permalink page.

Seth, if this matches the issue that you are seeing, can you file a new bug report?

Revision history for this message
Guruprasad (lgp171188) said : 		#7

Need confirmation from the OP about the exact details of the issue.

Revision history for this message
Seth Arnold (seth-arnold) said : 		#8

I put screenshots in the bug report, I hope they help explain which exact little yellow things I'm talking about :)

Thanks

Revision history for this message
Best Guruprasad (lgp171188) said : 		#9

Thank you for providing the screenshots in the bug report. That confirms my understanding of the issue. I will now triage the bug. Now that there is a bug for this issue, can we mark this question as solved?

Revision history for this message
Seth Arnold (seth-arnold) said : 		#10

Thanks Guruprasad, that solved my question.