Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Visibility of PR code review comments #5100

Closed
2 of 7 tasks
danwilliams opened this issue Oct 17, 2018 · 6 comments
Closed
2 of 7 tasks

Visibility of PR code review comments #5100

danwilliams opened this issue Oct 17, 2018 · 6 comments
Labels
issue/stale type/enhancement An improvement of existing functionality

Comments

@danwilliams
Copy link

danwilliams commented Oct 17, 2018

  • Gitea version (or commit ref): master build from 16 October 2016 (downloaded binary), version 34695f4
  • Git version: 2.11.0
  • Operating system: Debian Stretch
  • Database (use [x]):
    • PostgreSQL
    • MySQL
    • MSSQL
    • SQLite
  • Can you reproduce the bug at https://try.gitea.io:
    • Yes (provide example URL)
    • No
    • Not relevant
  • Log gist:

Description

In using the new code review comments brought in with Gitea 1.6.0 (not released yet, hence using master), the following problems have been experienced:

  1. When a user responds to a reviewer's code review comment, the response is not shown in the comment history/flow on the PR overview. In order to see the response, the file in question has to be clicked.
  2. When an area of code is updated, the original comment is still shown on the PR overview, but against the original code. The changed code is not shown in the PR overview. The file in question needs to be clicked in order to see the latest changes.
  3. When an area of code is updated, clicking to see the file in question shows the response comments against that piece of code, but the original reviewer's comment is no longer shown.

This leads to a situation where it is not possible to see the review comments, response, and updates in the same place, meaning that completing the post-update re-review process is rather fiddly and painful.

Hopefully this is a bug and not intentional behaviour!

I have searched the issues list and not found any issues on the same subject - apologies if I missed one somewhere.

@kolaente
Copy link
Member

We hotfixed some thing which fixed another thing related to comments not appering yesterday (#5096), could you update and re-verify?

@jonasfranz
Copy link
Member

  1. When a user responds to a reviewer's code review comment, the response is not shown in the comment history/flow on the PR overview. In order to see the response, the file in question has to be clicked.

We already have a feature request for this, since this feature requires a conversation mechanism: #4389

  1. When an area of code is updated, the original comment is still shown on the PR overview, but against the original code. The changed code is not shown in the PR overview. The file in question needs to be clicked in order to see the latest changes.

This is the expected behavior. Github also handles code updates like this, because the context of the comment should be saved. Is this code change marked as outdated? Can you provide an example url at try.gitea.io ?

  1. When an area of code is updated, clicking to see the file in question shows the response comments against that piece of code, but the original reviewer's comment is no longer shown.

Can you show me an example for that? I do not really understand what you're meaning.

@jonasfranz jonasfranz added the type/enhancement An improvement of existing functionality label Oct 18, 2018
@jonasfranz
Copy link
Member

I've sent a PR for 1.: #5104

@danwilliams
Copy link
Author

danwilliams commented Oct 18, 2018

Thanks guys, for the prompt and thorough response 👍

Hotfix: I updated to the current master version, 8db3bdc, but I am not sure I notice any difference with comment behaviour.

  1. That is unfortunate... but, it's brilliant that there is now already a PR! I have looked at Add comment replies #5104 and it does seem like it will address this concern.

    What's kinda strange is that, at present, if the reviewer comments more than once then the comments are seen. However, when someone else responds, the comment is not seen. From reading the PR, it seems maybe this is to do with the specific line of code... but in any case, from the UI point of view, if you click "reply" to a comment, you expect the see the comment listed in the "conversation" as a reply. Fingers crossed that Add comment replies #5104 will indeed correct that.

  2. This is perhaps not totally clear - not just to you in reading it, but to me in writing it. It's hard to understand what is going on. Essentially, a review comment is added... some change is made... a reply comment is added. The reply comment does not show up under the original review comment (and neither does the change). The reply comment instead shows up in the file... this seems wrong (see point 3). Now, the original comment is indeed marked as "outdated" - which is great - but as there is no continuation of the narrative (due to point 1) then it becomes confusing.

    I agree that the context of the original comment is important. So, the original line of code needs to be shown as part of that PR review in order to display those comments (as you say, like Github). But, because the reply is "lost" and does not appear against this - but instead, against the file only (i.e. "outside" of the thread/context of the review), it is necessary to click to see that file and then try to locate the comment (you have no idea if there is a reply or not, basically).

    It appears that what might be happening is that the reply mechanism is putting the reply comment against the updated line of code, or in the context of that later commit somehow, rather than linking it to the context of the original review comment? This would not be so bad if there were a link from the original section of code to the new, updated code - which is where the reply comment can then be found - but this isn't present. There's no way to see the updated code, and so, no way to see the reply.

    I hope that makes sense!

    So, it's not that it is wrong to show the original code in the context of the review comment (I agree with this behaviour), but rather, that in the absence of a link to the changed code it is not possible to find the later comments.

    Also, my apologies that this problem was not described clearly first time round - my points were meant to form part of a problematic narrative, rather than the whole of point 2 being a problem in isolation.

  3. Hopefully I have described enough above for this to make sense now... the problem is that because the reply comment lands on the file, and not against the review, you have to go to the file to see the reply - but then, you cannot see the original comment, as that was linked to the original context, which may have changed.

I will try to put together an example to link you to for all of this, if the further details above do not make sense. But, effectively, I believe resolving point 1 may well resolve what are mostly knock-on effects in points 2 & 3. It would also be cool to be able to link from the PR to the new, changed code context - not even Github does that - but that's an enhancement, not a bugfix.

@stale
Copy link

stale bot commented Jan 6, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 weeks. Thank you for your contributions.

@stale stale bot added the issue/stale label Jan 6, 2019
@stale
Copy link

stale bot commented Feb 22, 2019

This issue has been automatically closed because of inactivity. You can re-open it if needed.

@stale stale bot closed this as completed Feb 22, 2019
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/stale type/enhancement An improvement of existing functionality
Projects
None yet
Development

No branches or pull requests

3 participants