Skip to content

Dismiss prior pull reviews if done via web in review dismiss #20197

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
Merged
merged 17 commits into from
Jul 19, 2022
Merged

Dismiss prior pull reviews if done via web in review dismiss #20197

merged 17 commits into from
Jul 19, 2022

Conversation

6543
Copy link
Member

@6543 6543 commented Jul 1, 2022

story:

  1. user A (repo admin) create pull to protected branch (need 1 lgtm)
  2. user B review accept
  3. user A create undesired changes ...
  4. user B rejects
  5. user A changes things
  6. user A dismiss review

before:
7. webUI show valid accepted (fallback to last review) review from user B

after:
7. webUI show no valid review from user B

@6543 6543 added the type/bug label Jul 1, 2022
@6543 6543 added this to the 1.18.0 milestone Jul 1, 2022
@6543 6543 mentioned this pull request Jul 1, 2022
Closed
@6543 6543 requested a review from lunny July 1, 2022 15:13
lunny
lunny reviewed Jul 3, 2022
View reviewed changes
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 3, 2022
Gusted
Gusted reviewed Jul 4, 2022
View reviewed changes
@6543 6543 requested a review from Gusted July 4, 2022 08:30
@6543 6543 requested a review from lunny July 4, 2022 09:21
@lunny
Copy link
Member

lunny commented Jul 4, 2022

Since we just in 1.17rc1, maybe we could have a break for that? Is false parameter for that meaningful?

@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 Jul 4, 2022
@6543
Copy link
Member Author

6543 commented Jul 4, 2022

Since we just in 1.17rc1, maybe we could have a break for that? Is false parameter for that meaningful?

breaking api should have a serious reason and has nothing to do with rc or so.
now we have the webUI case also covered via api if one like to write a own ui he can do so ...

@zeripath
Copy link
Contributor

antecessor is a somewhat technical term used rarely in English ... prior, preceding or earlier would be more usual.

@6543
Copy link
Member Author

6543 commented Jul 12, 2022

ok I'll rename to prior

@6543 6543 changed the title Dismiss antecessors pull reviews if done via web in review dismiss Dismiss prior pull reviews if done via web in review dismiss Jul 13, 2022
@codecov-commenter
Copy link

Codecov Report

❗ No coverage uploaded for pull request base (main@00d3876). Click here to learn what that means.
The diff coverage is 13.88%.

@@           Coverage Diff           @@
##             main   #20197   +/-   ##
=======================================
  Coverage        ?   46.86%           
=======================================
  Files           ?      976           
  Lines           ?   135223           
  Branches        ?        0           
=======================================
  Hits            ?    63376           
  Misses          ?    64075           
  Partials        ?     7772
Impacted Files Coverage Δ
models/issues/review.go 52.77% <0.00%> (ø)
routers/web/repo/pull_review.go 0.00% <0.00%> (ø)
services/pull/review.go 48.24% <7.14%> (ø)
routers/api/v1/repo/pull_review.go 69.76% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00d3876...3e6c94b. Read the comment docs.

@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 Jul 19, 2022
6543
6543 commented Jul 19, 2022
View reviewed changes
Copy link
Member Author

@6543 6543 left a comment

Choose a reason for hiding this comment

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

LGTM for priors ;)

@6543
Copy link
Member Author

6543 commented Jul 19, 2022

🚀

@6543 6543 merged commit c0f5111 into go-gitea:main Jul 19, 2022
@6543 6543 deleted the dismiss-antecessors-pull-reviews branch July 19, 2022 13:20
6543 added a commit to 6543-forks/gitea that referenced this pull request Jul 19, 2022
@6543
Dismiss prior pull reviews if done via web in review dismiss (go-gite…
4fd3ef0 
…a#20197)
@6543 6543 mentioned this pull request Jul 19, 2022
Merged
@6543
Copy link
Member Author

6543 commented Jul 19, 2022

-> #20407

@6543 6543 added the backport/done All backports for this PR have been created label Jul 19, 2022
6543 added a commit that referenced this pull request Jul 19, 2022
@6543
Dismiss prior pull reviews if done via web in review dismiss (#20197) (…
e930d66 
…#20407)
zjjhot added a commit to zjjhot/gitea that referenced this pull request Jul 20, 2022
@zjjhot
Merge remote-tracking branch 'giteaofficial/main'
d887752 
* giteaofficial/main:
  [skip ci] Updated translations via Crowdin
  Dismiss prior pull reviews if done via web in review dismiss (go-gitea#20197)
  Fix modified due date message (go-gitea#20388)
  Fix public org members displayed too many informations (go-gitea#20403)
  Add two factor status to admin cmd display (go-gitea#20401)
  Use tippy.js for context popup (go-gitea#20393)
vsysoev pushed a commit to IntegraSDL/gitea that referenced this pull request Aug 10, 2022
@6543
Dismiss prior pull reviews if done via web in review dismiss (go-gite…
4324da4 
…a#20197)
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lunny lunny Awaiting requested review from lunny

2 more reviewers

@zeripath zeripath zeripath approved these changes

@Gusted Gusted Gusted approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Milestone
1.18.0
Development

Successfully merging this pull request may close these issues.
6 participants
@6543 @lunny @zeripath @codecov-commenter @Gusted @GiteaBot