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.

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

WIP: Fix review notifications and add them to mail templates #8948

Closed
wants to merge 3 commits into from

Conversation

guillep2k
Copy link
Member

This PR fixes a panic produced while creating reviews, and adds review comments to the notification mail template.

@guillep2k guillep2k changed the title Fix review notifications and add them to mail templates WIP: Fix review notifications and add them to mail templates Nov 13, 2019
@guillep2k
Copy link
Member Author

@lunny I've marked the PR WIP for your consideration, although it's already functional.

It turns out that the review service was the wrong place to add the notifications, as they were already being added a few lines below where the comment is created.

What I'd suggest to follow the "services" model is to move most of routers/repo/pull_review.go to services/pull/review.go (which I removed because it was only calling models now). Perhaps that can be done in a future PR to keep this PR simple.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Nov 13, 2019
@codecov-io
Copy link

Codecov Report

Merging #8948 into master will increase coverage by 0.01%.
The diff coverage is 18.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8948      +/-   ##
==========================================
+ Coverage   41.23%   41.25%   +0.01%     
==========================================
  Files         547      546       -1     
  Lines       70794    70801       +7     
==========================================
+ Hits        29195    29207      +12     
+ Misses      37880    37870      -10     
- Partials     3719     3724       +5
Impacted Files Coverage Δ
models/issue_comment.go 48.07% <ø> (+0.21%) ⬆️
routers/repo/pull_review.go 0% <0%> (ø) ⬆️
modules/notification/mail/mail.go 26.56% <0%> (-2.26%) ⬇️
services/mailer/mail.go 43.88% <26.08%> (-2.42%) ⬇️
modules/migrations/migrate.go 21.22% <0%> (-1.68%) ⬇️
modules/migrations/gitea.go 8.05% <0%> (-0.64%) ⬇️
models/repo.go 46.46% <0%> (-0.05%) ⬇️
models/repo_list.go 74.07% <0%> (+0.92%) ⬆️
models/error.go 33.86% <0%> (+1.18%) ⬆️
modules/task/migrate.go 28.94% <0%> (+3.94%) ⬆️
... and 1 more

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 7b97e04...86f906a. Read the comment docs.

@guillep2k
Copy link
Member Author

Example review mail:

image

@guillep2k
Copy link
Member Author

Close in favor of #8954
I can add the template stuff later, since the work is already done. 😁

@guillep2k guillep2k closed this Nov 13, 2019
@guillep2k guillep2k deleted the fix-reviews branch November 20, 2019 13:08
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants