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

Show review summary in pull requests #5132

Merged
merged 37 commits into from Nov 22, 2018
Merged

Show review summary in pull requests #5132

merged 37 commits into from Nov 22, 2018

Conversation

kolaente
Copy link
Member

Resolves #4390.

This pr implements a review summary on the bottom of a pr as disscussed in #4390.

It currently shows every review grouped by type and user. This means if a user rejects a pr and later approves it, both are shown in the summary (with their dates). Should I keep this behaviour or show only the newest (grouped by user)?

Screenshot

image

@lafriks
Copy link
Member

lafriks commented Oct 20, 2018

I think it should show only latest grouped by user, also it should skip comment reviews

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 20, 2018
@kolaente
Copy link
Member Author

@lafriks done.

lafriks
lafriks requested changes Oct 20, 2018
View reviewed changes
models/review.go Show resolved Hide resolved
models/review.go Outdated Show resolved Hide resolved
@lafriks lafriks added the type/enhancement An improvement of existing functionality label Oct 20, 2018
@lafriks lafriks added this to the 1.7.0 milestone Oct 20, 2018
@codecov-io
Copy link

codecov-io commented Oct 21, 2018 •
edited

Codecov Report

Merging #5132 into master will increase coverage by 0.01%.
The diff coverage is 85.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5132      +/-   ##
==========================================
+ Coverage   37.42%   37.44%   +0.01%     
==========================================
  Files         312      312              
  Lines       46480    46507      +27     
==========================================
+ Hits        17396    17414      +18     
- Misses      26594    26601       +7     
- Partials     2490     2492       +2
Impacted Files Coverage Δ
models/review.go 68.12% <100%> (+4.81%) ⬆️
routers/repo/issue.go 32.15% <33.33%> (ø) ⬆️
modules/process/manager.go 76.81% <0%> (-4.35%) ⬇️
models/repo_list.go 63.29% <0%> (-1.27%) ⬇️

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 cef0f12...246626f. Read the comment docs.

jonasfranz
jonasfranz suggested changes Oct 21, 2018
View reviewed changes
models/review.go Show resolved Hide resolved
models/review.go Show resolved Hide resolved
@jonasfranz
Copy link
Member

Please add unit tests .

@jonasfranz jonasfranz added topic/ui Change the appearance of the Gitea UI type/changelog Adds the changelog for a new Gitea version labels Oct 21, 2018
@kolaente
Copy link
Member Author

@JonasFranzDEV done.

@lafriks
Copy link
Member

lafriks commented Oct 21, 2018

@JonasFranzDEV @kolaente I asked to ignore comment as it is not relevant for approval process as if I reject or approve changes and later do some comment it should still be in state that was previously that I either rejected or approved changes

@kolaente
Copy link
Member Author

@lafriks I see, I'll revert it.

@bkcsoft bkcsoft 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 Oct 31, 2018
@jonasfranz
Copy link
Member

@lafriks need your review

@kolaente
Copy link
Member Author

offtopic: why do we have lgtm/done if only @JonasFranzDEV has approved? 🤔

lafriks
lafriks reviewed Nov 1, 2018
View reviewed changes
models/review.go Outdated Show resolved Hide resolved
@lafriks lafriks mentioned this pull request Nov 21, 2018
Merged
@kolaente
Copy link
Member Author

kolaente commented Nov 22, 2018 •
edited

@lafriks mind approving the pr?

@bkcsoft bkcsoft 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 Nov 22, 2018
@lafriks
Copy link
Member

lafriks commented Nov 22, 2018

@kolaente I don't mind ;)

@lafriks lafriks merged commit 0dcf31a into go-gitea:master Nov 22, 2018
@lafriks lafriks changed the title Show review summary on pr Show review summary in pull requests Nov 22, 2018
@kolaente
Copy link
Member Author

@lafriks Thanks!

@kolaente kolaente deleted the feature/review-summary branch November 22, 2018 13:24
@lunny
Copy link
Member

lunny commented Nov 22, 2018

@kolaente have the eye icon was changed to a tick icon? :)

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
@delvh delvh removed the type/changelog Adds the changelog for a new Gitea version label Oct 7, 2023
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

@jonasfranz jonasfranz jonasfranz approved these changes

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI type/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.7.0
Development

Successfully merging this pull request may close these issues.

Show summary of reviews
7 participants
@kolaente @lafriks @codecov-io @jonasfranz @lunny @bkcsoft @delvh