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.

Sign up for GitHub

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

Jump to bottom

[WIP] Refactor review related code #28544

Open
wants to merge 70 commits into
base: main
Choose a base branch
from
Open

[WIP] Refactor review related code #28544

wants to merge 70 commits into from

Conversation

6543
Copy link
Member

@6543 6543 commented Dec 20, 2023 •
edited
Loading

clean up review related code

TODOs:

  • Remove ReviewID from Comment (type = CommentTypeReviewRequest) if a ReviewRequest gets deleted
  • Single func to get reviews based on options
  • Reuse query builder a lot
  • Move functions for creating reviews higher up the chain to not have to delete stuff on the basic creation model function
    • Move "state machine" into its own place and don't spread it over the codebase
    • Do proper unit-tests as it's then possible

@6543 6543 added type/bug pr/wip This PR is not ready for review labels 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
@pull-request-size pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Dec 20, 2023
@6543 6543 added this to the 1.22.0 milestone Dec 20, 2023
@github-actions github-actions bot added the modifies/api This PR adds API routes or modifies them label Dec 20, 2023
6543
6543 commented Dec 20, 2023
View reviewed changes
models/issues/review.go Outdated Show resolved Hide resolved
@6543 6543 changed the title Pull review fixfix builder Fix pull review state and refactor code Dec 20, 2023
@6543 6543 marked this pull request as ready for review December 20, 2023 02:14
@6543 6543 added backport/v1.21 This PR should be backported to Gitea 1.21 and removed pr/wip This PR is not ready for review labels Dec 20, 2023
@6543 6543 requested a review from a team December 20, 2023 02:15
@6543
Copy link
Member Author

6543 commented Dec 20, 2023

PS: the review code need more cleanup - but this pull here mostly addresses bugs

@6543 6543 mentioned this pull request Dec 20, 2023
Merged
@6543 6543 added type/refactoring Existing code has been cleaned up. There should be no new functionality. and removed backport/v1.21 This PR should be backported to Gitea 1.21 labels Dec 20, 2023
6543 added a commit that referenced this pull request Feb 13, 2024
@6543
Dont load Review if Comment is CommentTypeReviewRequest (#28551)
6fad2c8 
RequestReview get deleted on review.
So we don't have to try to load them on comments.

broken out #28544
6543 added a commit to 6543-forks/gitea that referenced this pull request Feb 13, 2024
@6543
Dont load Review if Comment is CommentTypeReviewRequest (go-gitea#28551)
703c80e 
RequestReview get deleted on review.
So we don't have to try to load them on comments.

broken out go-gitea#28544
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 15, 2024
@6543 @silverwind
Dont load Review if Comment is CommentTypeReviewRequest (go-gitea#28551)
70ecb18 
RequestReview get deleted on review.
So we don't have to try to load them on comments.

broken out go-gitea#28544
@6543 6543 removed this from the 1.22.0 milestone Feb 19, 2024
6543
6543 commented Feb 19, 2024
View reviewed changes
models/issues/review.go
@@ -85,6 +85,11 @@ const (
ReviewTypeRequest
)

// AffectReview indicate if this review type alter a pull state
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

write about branch protection

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)
217d71c 
close  #28542

blocks  #28544

---
*Sponsored by Kithara Software GmbH*
6543
6543 commented Feb 19, 2024
View reviewed changes
models/issues/pull.go
And("type = ?", ReviewTypeApprove).
And("official = ?", true).
And("dismissed = ?", false)
opt := &GetReviewOption{
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lunny suggest: How about reuse db.Find here?

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 commented Feb 19, 2024
View reviewed changes
models/issues/review.go
// make sure user review requests are cleared
if opts.Type != ReviewTypePending {
if _, err := sess.Where(reviewCond.And(builder.Eq{"type": ReviewTypeRequest})).Delete(new(Review)); err != nil {
if _, err := sess.Where(opt.toCond().And(builder.Eq{"type": ReviewTypeRequest})).Delete(new(Review)); err != nil {
return nil, err
}
}
// make sure if the created review gets dismissed no old review surface
// other types can be ignored, as they don't affect branch protection
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// other types can be ignored, as they don't affect branch protection

with the new helper it should be self explaining cc @wxiaoguang ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PS: this code might be deleted later on anyway if i'm done refactoring

silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@6543 @silverwind
Fix inperformant query on retrifing review from database. (go-gitea#2…
087ab1b 
…8552)

can we please PLEAS PLEASE only use raw SQL statements if it is relay
needed!!!

source is go-gitea#28544 (before
refactoring)
silverwind pushed a commit to silverwind/gitea that referenced this pull request Feb 20, 2024
@6543 @silverwind
Dont load Review if Comment is CommentTypeReviewRequest (go-gitea#28551)
51d756e 
RequestReview get deleted on review.
So we don't have to try to load them on comments.

broken out go-gitea#28544
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*
6543 added a commit to 6543-forks/gitea that referenced this pull request Feb 26, 2024
@6543 @earl-warren
Dont load Review if Comment is CommentTypeReviewRequest (go-gitea#28551)
e9e6c61 
RequestReview get deleted on review.
So we don't have to try to load them on comments.

broken out go-gitea#28544

(cherry picked from commit 6fad2c8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lunny lunny lunny left review comments

Assignees
No one assigned
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. modifies/api This PR adds API routes or modifies them pr/wip This PR is not ready for review size/L Denotes a PR that changes 100-499 lines, ignoring generated files. type/bug type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@6543 @lunny @GiteaBot @lafriks