Do not track edits and deletions of pending review comments #8593

Closed
opened 2025-11-02 08:11:55 -06:00 by GiteaMirror · 7 comments
Owner

Originally created by @delvh on GitHub (Feb 21, 2022).

Gitea Version

1.16.1-dev

Git Version

NA

Operating System

Linux

How are you running Gitea?

NA

Database

No response

Can you reproduce the bug on the Gitea demo site?

Yes, unfortunately

Log Gist

No response

Description

Currently, any edit to a comment will be logged in its change history.
For PR Reviews, however, edits should only be logged after the review was submitted.
Edits prior to that should simply be discarded.

Scope

There are two aspects that makes this issue way worse:

  1. The webhooks are also fired, meaning that even though your review is not yet present, others will be informed about the content of what you edited
  2. This does not only apply to editing pending comments, but also to deleting pending comments.

Keep in mind

The creation change should be adapted to not show the original text,
but the text of the last edit before submitting the review.
However, this appears to already be done within models/issues/content_history.go#keepLimitedContentHistory

see https://try.gitea.io/delvh/kanban-test/pulls/16#issuecomment-113571 for example.

Screenshots

No response

Originally created by @delvh on GitHub (Feb 21, 2022). ### Gitea Version 1.16.1-dev ### Git Version NA ### Operating System Linux ### How are you running Gitea? NA ### Database _No response_ ### Can you reproduce the bug on the Gitea demo site? Yes, unfortunately ### Log Gist _No response_ ### Description Currently, any edit to a comment will be logged in its change history. For PR Reviews, however, edits should only be logged after the review was submitted. Edits prior to that should simply be discarded. #### Scope There are two aspects that makes this issue **way** worse: 1. The webhooks are also fired, meaning that even though your review is not yet present, others will be informed about the content of what you edited 2. This does not only apply to editing pending comments, but also to deleting pending comments. #### Keep in mind The creation change should be adapted to not show the original text, but the text of the last edit before submitting the review. However, this appears to already be done within `models/issues/content_history.go#keepLimitedContentHistory` see https://try.gitea.io/delvh/kanban-test/pulls/16#issuecomment-113571 for example. ### Screenshots _No response_
GiteaMirror added the issue/criticaltype/proposaltype/bug labels 2025-11-02 08:11:55 -06:00
Author
Owner

@wxiaoguang commented on GitHub (Jul 5, 2022):

There is an old issue, it seems related.

@wxiaoguang commented on GitHub (Jul 5, 2022): There is an old issue, it seems related. * https://github.com/go-gitea/gitea/issues/4388
Author
Owner

@lunny commented on GitHub (Jul 5, 2022):

duplicated

@lunny commented on GitHub (Jul 5, 2022): duplicated
Author
Owner

@wxiaoguang commented on GitHub (Jul 5, 2022):

@delvh are you working on this? Should we keep this issue open, or use the old one?

@wxiaoguang commented on GitHub (Jul 5, 2022): @delvh are you working on this? Should we keep this issue open, or use the old one?
Author
Owner

@delvh commented on GitHub (Jul 5, 2022):

We can use the other one, and so far I'm not working on it because
a) I have close to no time right now (see my sparse interactions in the last few days)
b) I haven't found the root location of this "bug" yet, whenever I attempt to follow the trail, it leads me throughout the whole application...

@delvh commented on GitHub (Jul 5, 2022): We can use the other one, and so far I'm not working on it because a) I have close to no time right now (see my sparse interactions in the last few days) b) I haven't found the root location of this "bug" yet, whenever I attempt to follow the trail, it leads me throughout the whole application...
Author
Owner

@wxiaoguang commented on GitHub (Jul 5, 2022):

Hmm ... these 2 issues are related, but not exactly the same (#4388 is about sending notification before summit, this is about saving content history before submit). I think we can keep both open, just link them for further investigation.

@wxiaoguang commented on GitHub (Jul 5, 2022): Hmm ... these 2 issues are related, but not exactly the same (#4388 is about sending notification before summit, this is about saving content history before submit). I think we can keep both open, just link them for further investigation.
Author
Owner

@madxmike commented on GitHub (Aug 11, 2023):

Hi, I ended up being nerd snipped by @jolheiser on this issue last night. Heres the rundown:

There are 3 issues occuring during a pending PR Review:

  1. Replying to a pending comment during a pending PR Review will send a webhook notification
  2. Deleting a pending comment during a pending PR Review will send a webhook notification
  3. Editing a pending comment during a pending PR review will send a webhook notification

I think its easier to start with showing what happens in the working case first:

Adding a new pending comment during a pending PR review does not send a webhook notification

  1. When you press the "Add Comment" button during a pending PR Review it will call this endpoint 88479e0dfc/routers/web/web.go (L1304)
  2. This will go through the repo layer and call pull_service.CreateCodeComment with the pendingReview parameter set to false https://github.com/go-gitea/gitea/blob/main/routers/web/repo/pull_review.go#L76
  3. This ends up in the pull review service https://github.com/go-gitea/gitea/blob/main/services/pull/review.go#L73 where there is a check if !pendingReview && existsReview. For new comments this evaluates to if !true && false. As such, it does not go down that if path which contains a call to the notification service https://github.com/go-gitea/gitea/blob/main/services/pull/review.go#L116.
  4. The code continues on and creates the code comment.

As you see above, the new comment path never ends up calling the notification service, so no notifications are sent out. These notifications are eventually sent when the PR Review is published https://github.com/go-gitea/gitea/blob/main/services/pull/review.go#L310

Replying to a pending comment during a pending PR Review will send a webhook notification

  1. When you press the "Reply" button on a Reply message during a pending PR Review it will call this endpoint 88479e0dfc/routers/web/web.go (L1304) with the "single_review" flag set to "true"
    image

  2. This will go through the repo layer and call pull_service.CreateCodeComment with the pendingReview parameter set to false https://github.com/go-gitea/gitea/blob/main/routers/web/repo/pull_review.go#L76

  3. This ends up in the pull review service https://github.com/go-gitea/gitea/blob/main/services/pull/review.go#L73 where there is a check if !pendingReview && existsReview. For reply comments this evaluates to if !false && true. As such, it goes down the if path which contains a call to the notification service https://github.com/go-gitea/gitea/blob/main/services/pull/review.go#L116.

  4. A notification is sent out

The fix here would be to do some type of check around this notification call to ensure if the Review is pending or not when this path is called.

Deleting a pending comment during a pending PR Review will send a webhook notification

  1. When you press the "Delete" button on a comment during a pending PR Review it will call this endpoint 88479e0dfc/routers/web/web.go (L1052)
  2. This ends up in the issue repo which calls issue_service.DeleteComment https://github.com/go-gitea/gitea/blob/main/routers/web/repo/issue.go#L3121
  3. issue_service.DeleteComment is a implementation that simply deletes the comment and sends a notification https://github.com/go-gitea/gitea/blob/main/services/issue/comments.go#L104
  4. A notification is sent out

The fix here would be to add a DeleteComment function to the pull_service that does much the same as the pull_service.CreateCodeComment. It needs to check if the review is pending, and if so dont sent a notification. Then add a layer in the pull review repo to handle the review case. Then create a new endpoint that goes to this repo.

Editing a pending comment during a pending PR review will send a webhook notification

  1. When you press the "Save" button on a edited comment during a pending PR Review it will call this endpoint 88479e0dfc/routers/web/web.go (L1051)
  2. This ends up in the issue repo which calls issue_service.UpdateComment https://github.com/go-gitea/gitea/blob/main/routers/web/repo/issue.go#L3065
  3. issue_service.UpdateComment is an implementation that simply updates the comment and sends a notification https://github.com/go-gitea/gitea/blob/main/services/issue/comments.go#L72
  4. A notification is sent out

The fix here would be to add a UpdateComment function to the pull_service that does much the same as the pull_service.CreateCodeComment. It needs to check if the review is pending, and if so dont sent a notification. Then add a layer in the pull review repo to handle the review case. Then create a new endpoint that goes to this repo.

@madxmike commented on GitHub (Aug 11, 2023): Hi, I ended up being nerd snipped by @jolheiser on this issue last night. Heres the rundown: There are 3 issues occuring during a pending PR Review: 1. Replying to a pending comment during a pending PR Review will send a webhook notification 2. Deleting a pending comment during a pending PR Review will send a webhook notification 3. Editing a pending comment during a pending PR review will send a webhook notification I think its easier to start with showing what happens in the working case first: **Adding a new pending comment during a pending PR review does not send a webhook notification** 1. When you press the "Add Comment" button during a pending PR Review it will call this endpoint https://github.com/go-gitea/gitea/blob/88479e0dfcbcb9ce229a72ead8c3c9c3b6bb04b0/routers/web/web.go#L1304 2. This will go through the repo layer and call `pull_service.CreateCodeComment` with the `pendingReview` parameter set to `false` https://github.com/go-gitea/gitea/blob/main/routers/web/repo/pull_review.go#L76 3. This ends up in the pull review service https://github.com/go-gitea/gitea/blob/main/services/pull/review.go#L73 where there is a check `if !pendingReview && existsReview`. For new comments this evaluates to `if !true && false`. As such, it does not go down that if path which contains a call to the notification service https://github.com/go-gitea/gitea/blob/main/services/pull/review.go#L116. 4. The code continues on and creates the code comment. As you see above, the new comment path never ends up calling the notification service, so no notifications are sent out. These notifications are eventually sent when the PR Review is published https://github.com/go-gitea/gitea/blob/main/services/pull/review.go#L310 **Replying to a pending comment during a pending PR Review will send a webhook notification** 1. When you press the "Reply" button on a Reply message during a pending PR Review it will call this endpoint https://github.com/go-gitea/gitea/blob/88479e0dfcbcb9ce229a72ead8c3c9c3b6bb04b0/routers/web/web.go#L1304 with the `"single_review"` flag set to `"true"` ![image](https://github.com/go-gitea/gitea/assets/2898444/081ec675-bd97-4061-a45d-542410d3b62e) 2. This will go through the repo layer and call `pull_service.CreateCodeComment` with the `pendingReview` parameter set to `false` https://github.com/go-gitea/gitea/blob/main/routers/web/repo/pull_review.go#L76 3. This ends up in the pull review service https://github.com/go-gitea/gitea/blob/main/services/pull/review.go#L73 where there is a check `if !pendingReview && existsReview`. For reply comments this evaluates to `if !false && true`. As such, it goes down the if path which contains a call to the notification service https://github.com/go-gitea/gitea/blob/main/services/pull/review.go#L116. 4. A notification is sent out The fix here would be to do some type of check around this notification call to ensure if the Review is pending or not when this path is called. **Deleting a pending comment during a pending PR Review will send a webhook notification** 1. When you press the "Delete" button on a comment during a pending PR Review it will call this endpoint https://github.com/go-gitea/gitea/blob/88479e0dfcbcb9ce229a72ead8c3c9c3b6bb04b0/routers/web/web.go#L1052 2. This ends up in the issue repo which calls `issue_service.DeleteComment` https://github.com/go-gitea/gitea/blob/main/routers/web/repo/issue.go#L3121 3. `issue_service.DeleteComment` is a implementation that simply deletes the comment and sends a notification https://github.com/go-gitea/gitea/blob/main/services/issue/comments.go#L104 4. A notification is sent out The fix here would be to add a `DeleteComment` function to the `pull_service` that does much the same as the `pull_service.CreateCodeComment`. It needs to check if the review is pending, and if so dont sent a notification. Then add a layer in the pull review repo to handle the review case. Then create a new endpoint that goes to this repo. **Editing a pending comment during a pending PR review will send a webhook notification** 1. When you press the "Save" button on a edited comment during a pending PR Review it will call this endpoint https://github.com/go-gitea/gitea/blob/88479e0dfcbcb9ce229a72ead8c3c9c3b6bb04b0/routers/web/web.go#L1051 2. This ends up in the issue repo which calls `issue_service.UpdateComment` https://github.com/go-gitea/gitea/blob/main/routers/web/repo/issue.go#L3065 3. `issue_service.UpdateComment` is an implementation that simply updates the comment and sends a notification https://github.com/go-gitea/gitea/blob/main/services/issue/comments.go#L72 4. A notification is sent out The fix here would be to add a `UpdateComment` function to the `pull_service` that does much the same as the `pull_service.CreateCodeComment`. It needs to check if the review is pending, and if so dont sent a notification. Then add a layer in the pull review repo to handle the review case. Then create a new endpoint that goes to this repo.
Author
Owner

@xgdgsc commented on GitHub (Jan 23, 2024):

And when you first time open a PR and assign someone. 2 notifications are sent that should be merged to one.

@xgdgsc commented on GitHub (Jan 23, 2024): And when you first time open a PR and assign someone. 2 notifications are sent that should be merged to one.
Sign in to join this conversation.
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: github-starred/gitea#8593