Skip to content

Workaround to clean up old reviews on creating a new one#28554

Merged
6543 merged 25 commits into
go-gitea:mainfrom
6543-forks:cleanup_old-reviews-on-review
Feb 19, 2024
Merged

Workaround to clean up old reviews on creating a new one#28554
6543 merged 25 commits into
go-gitea:mainfrom
6543-forks:cleanup_old-reviews-on-review

Conversation

@6543
Copy link
Copy Markdown
Member

@6543 6543 commented Dec 20, 2023 •
edited
Loading

close #28542

blocks #28544

Sponsored by Kithara Software GmbH

@6543 6543 added this to the 1.22.0 milestone Dec 20, 2023
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 20, 2023
@6543 6543 mentioned this pull request Dec 20, 2023
Open
6 tasks
@6543 6543 requested a review from a team December 20, 2023 12:49
KN4CK3R
KN4CK3R reviewed Dec 20, 2023
View reviewed changes
Comment thread models/issues/review.go Outdated
@6543 6543 requested a review from KN4CK3R December 21, 2023 00:18
@KN4CK3R
Copy link
Copy Markdown
Member

KN4CK3R commented Dec 21, 2023

I think this fix is placed at the wrong position. CreateReview previously did just one thing: creating a review in the database. Now there is logic mixed in which should be one or more levels up the callstack.

@6543
Copy link
Copy Markdown
Member Author

6543 commented Dec 21, 2023 •
edited
Loading

@KN4CK3R well if you looking for the refactoring, that is& will be done at #28544

@KN4CK3R
Copy link
Copy Markdown
Member

KN4CK3R commented Dec 21, 2023

That PR contains the same code at the same place.

@6543
Copy link
Copy Markdown
Member Author

6543 commented Dec 21, 2023 •
edited
Loading

yes as I' working on more but I'll wait until the bugfix did get in as it will conflict a lot.

anyway just look at the rest of models/issues/review.go it is not the best.
I want to get the bugfix out soon and work on clean code afterwards (witch will not be backportable)

@jpraet
Copy link
Copy Markdown
Member

jpraet commented Dec 22, 2023

Maybe it belongs in

gitea/models/issues/review.go

Line 349 in 21229ed

func SubmitReview(ctx context.Context, doer *user_model.User, issue *Issue, reviewType ReviewType, content, commitID string, stale bool, attachmentUUIDs []string) (*Review, *Comment, error) {

where there is already similar code to remove team review requests

gitea/models/issues/review.go

Line 434 in 21229ed

// try to remove team review request if need

@6543
Copy link
Copy Markdown
Member Author

6543 commented Dec 22, 2023

😆 I that it should move an module higher up but should i start refactoring here or not ?!?

@jpraet
Copy link
Copy Markdown
Member

jpraet commented Dec 23, 2023

CreateReview is also called when creating a pending review. In that case the review requests should not yet be deleted.
So I think the logic should be moved to SubmitReview.

But I'm also not entirely convinced that the review requests should be deleted.

If I request a review from person A. Then person A does a review, and then person A's review gets dismissed (for example by the "dismiss stale approvals" branch protection setting). Don't we want the PR to revert to the state where a review is requested from person A?

@6543
Copy link
Copy Markdown
Member Author

6543 commented Dec 23, 2023

CreateReview is also called when creating a pending review. In that case the review requests should not yet be deleted.
So I think the logic should be moved to SubmitReview.

you are right pending reviews should not clear requests, will address that

If I request a review from person A. Then person A does a review, and then person A's review gets dismissed (for example by the "dismiss stale approvals" branch protection setting). Don't we want the PR to revert to the state where a review is requested from person A?

no you wont do that as it is done now (that'S exactly how we get in a corrupt state for example).
if so desired we should trigger a new review request via the "dismis stale reivew" function, and this might also be an setting as it can differ on the review-workflow teams have deployed.

and in any case you can manually re-request in this case

6543
6543 commented Dec 23, 2023
View reviewed changes
Comment thread models/issues/review.go Outdated
@6543 6543 changed the title Clean up old reviews on creating a new one Workaround to clean up old reviews on creating a new one Jan 19, 2024
@6543
Copy link
Copy Markdown
Member Author

6543 commented Jan 19, 2024

did rename the title to make it clear that this is not the final solution

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Feb 15, 2024
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Feb 19, 2024
@lafriks lafriks enabled auto-merge (squash) February 19, 2024 12:10
lunny
lunny reviewed Feb 19, 2024
View reviewed changes
Comment thread models/issues/review.go
@lafriks lafriks disabled auto-merge February 19, 2024 12:16
wxiaoguang
wxiaoguang reviewed Feb 19, 2024
View reviewed changes
Comment thread models/issues/review.go
@6543 6543 merged commit 217d71c into go-gitea:main Feb 19, 2024
@6543 6543 deleted the cleanup_old-reviews-on-review branch February 19, 2024 13:46
@6543 6543 added the backport/done All backports for this PR have been created label Feb 19, 2024
6543 added a commit to 6543-forks/gitea that referenced this pull request Feb 19, 2024
@6543
Workaround to clean up old reviews on creating a new one (go-gitea#28554
79f0729 
)

close  go-gitea#28542

blocks  go-gitea#28544

---
*Sponsored by Kithara Software GmbH*
@6543 6543 mentioned this pull request Feb 19, 2024
Merged
@6543
Copy link
Copy Markdown
Member Author

6543 commented Feb 19, 2024

-> #29264

@6543 6543 added the backport/manual No power to the bots! Create your backport yourself! label Feb 19, 2024
6543 added a commit that referenced this pull request Feb 19, 2024
@6543
Workaround to clean up old reviews on creating a new one (#28554) (#2…
c01b266 
…9264)

close  #28542
backport #28554

---
*Sponsored by Kithara Software GmbH*
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@6543 @silverwind
Workaround to clean up old reviews on creating a new one (go-gitea#28554
a94bf6a 
)

close  go-gitea#28542

blocks  go-gitea#28544

---
*Sponsored by Kithara Software GmbH*
zjjhot added a commit to zjjhot/gitea that referenced this pull request Feb 22, 2024
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
cc8215a 
* giteaofficial/main: (32 commits)
  [skip ci] Updated translations via Crowdin
  Prevent double use of `git cat-file` session. (go-gitea#29298)
  Revert go-gitea#28753 because UI broken. (go-gitea#29293)
  Fix error display when merging PRs (go-gitea#29288)
  Refactor markup rendering to accept general "protocol:" prefix (go-gitea#29276)
  Remove jQuery from the installation page (go-gitea#29284)
  Always write proc-receive hook for all git versions (go-gitea#29287)
  Do not use `ctx.Doer` when reset password (go-gitea#29289)
  Update Discord logo (go-gitea#29285)
  [skip ci] Updated translations via Crowdin
  Remove jQuery .map() and enable eslint rules for it (go-gitea#29272)
  Explained where create issue/PR template (go-gitea#29035) (go-gitea#29266)
  Remove jQuery from repo wiki creation page (go-gitea#29271)
  Do not show delete button when time tracker is disabled (go-gitea#29257)
  Left align the input labels for the link account page (go-gitea#29255)
  [skip ci] Updated translations via Crowdin
  Remove jQuery from the repo migration form (go-gitea#29229)
  Fix content size does not match error when uploading lfs file (go-gitea#29259)
  Workaround to clean up old reviews on creating a new one (go-gitea#28554)
  Deduplicate translations for contributors graph (go-gitea#29256)
  ...
@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Feb 28, 2024

Automatically locked because of our CONTRIBUTING guidelines

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Feb 28, 2024
@wxiaoguang
Copy link
Copy Markdown
Contributor

wxiaoguang commented Mar 18, 2024

It seems that deleting a pending review would cause the comment has a dangling Review field and cause 500 panic error.

-> Render failed when opening PR with conversation #29885

-> Fix template error when comment review doesn't exist #29888

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@lunny lunny lunny left review comments

@lafriks lafriks lafriks approved these changes

@wxiaoguang wxiaoguang wxiaoguang left review comments

@silverwind silverwind silverwind approved these changes

@KN4CK3R KN4CK3R Awaiting requested review from KN4CK3R

Assignees

No one assigned

Labels

backport/done All backports for this PR have been created backport/manual No power to the bots! Create your backport yourself! lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug

Projects

None yet

Milestone

1.22.0

Development

Successfully merging this pull request may close these issues.

pull review state bug

8 participants

@6543 @KN4CK3R @jpraet @silverwind @wxiaoguang @lunny @lafriks @GiteaBot