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.

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

Show review summary in pull requests #5132

Merged
merged 37 commits into from Nov 22, 2018

Conversation

6 participants
@kolaente
Copy link
Member

kolaente commented Oct 20, 2018

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

kolaente added some commits Oct 20, 2018

@lafriks

This comment has been minimized.

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 label Oct 20, 2018

kolaente added some commits Oct 20, 2018

@kolaente

This comment has been minimized.

Copy link
Member Author

kolaente commented Oct 20, 2018

@lafriks done.

Show resolved Hide resolved models/review.go
Show resolved Hide resolved models/review.go Outdated

@lafriks lafriks added this to the 1.7.0 milestone Oct 20, 2018

kolaente added some commits Oct 21, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 21, 2018

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.

Show resolved Hide resolved models/review.go
Show resolved Hide resolved models/review.go
@jonasfranz

This comment has been minimized.

Copy link
Member

jonasfranz commented Oct 21, 2018

Please add unit tests .

kolaente added some commits Oct 21, 2018

@kolaente

This comment has been minimized.

Copy link
Member Author

kolaente commented Oct 21, 2018

@JonasFranzDEV done.

@lafriks

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member Author

kolaente commented Oct 21, 2018

@lafriks I see, I'll revert it.

kolaente added some commits Oct 28, 2018

@bkcsoft bkcsoft added lgtm/need 1 and removed lgtm/need 2 labels Oct 31, 2018

@jonasfranz

This comment has been minimized.

Copy link
Member

jonasfranz commented Oct 31, 2018

@lafriks need your review

@kolaente

This comment has been minimized.

Copy link
Member Author

kolaente commented Oct 31, 2018

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

Show resolved Hide resolved models/review.go Outdated

kolaente added some commits Nov 5, 2018

@kolaente

This comment has been minimized.

Copy link
Member Author

kolaente commented Nov 22, 2018

@lafriks mind approving the pr?

@bkcsoft bkcsoft added lgtm/done and removed lgtm/need 1 labels Nov 22, 2018

@lafriks

This comment has been minimized.

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

2 checks passed

approvals/lgtm this commit looks good
continuous-integration/drone/pr the build was successful
Details

@lafriks lafriks changed the title Show review summary on pr Show review summary in pull requests Nov 22, 2018

@kolaente

This comment has been minimized.

Copy link
Member Author

kolaente commented Nov 22, 2018

@lafriks Thanks!

@kolaente kolaente deleted the kolaente:feature/review-summary branch Nov 22, 2018

@lunny

This comment has been minimized.

Copy link
Member

lunny commented Nov 22, 2018

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

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