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

Uniform an abstract notification layer for ui, mail, action, webhook and indexer #4001

Closed
wants to merge 14 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented May 21, 2018

When an event occupied, many notifications will be fired. This PR uniforms them as an abstract notification layer so that you can easily add new events or new receivers and don't change the codes of event occupied.

@lunny lunny added pr/wip This PR is not ready for review type/refactoring Existing code has been cleaned up. There should be no new functionality. labels May 21, 2018
@lunny lunny added this to the 1.x.x milestone May 21, 2018
@codecov-io
Copy link

codecov-io commented May 21, 2018

Codecov Report

Merging #4001 into master will increase coverage by 0.03%.
The diff coverage is 13.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4001      +/-   ##
==========================================
+ Coverage   20.18%   20.21%   +0.03%     
==========================================
  Files         156      156              
  Lines       31168    30837     -331     
==========================================
- Hits         6292     6235      -57     
+ Misses      23918    23658     -260     
+ Partials      958      944      -14
Impacted Files Coverage Δ
models/issue_milestone.go 58.92% <ø> (+2.14%) ⬆️
models/pull.go 18.33% <ø> (+1.16%) ⬆️
models/issue_comment.go 32.46% <0%> (+3.22%) ⬆️
models/release.go 0% <0%> (ø) ⬆️
routers/api/v1/repo/issue.go 0% <0%> (ø) ⬆️
routers/api/v1/repo/issue_comment.go 0% <0%> (ø) ⬆️
routers/repo/pull.go 0% <0%> (ø) ⬆️
routers/api/v1/repo/issue_label.go 0% <0%> (ø) ⬆️
routers/api/v1/repo/pull.go 0% <0%> (ø) ⬆️
routers/repo/repo.go 0% <0%> (ø) ⬆️
... and 11 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 c40f5d2...8b6c568. Read the comment docs.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 21, 2018
receiver notification.NotifyReceiver = &indexerReceiver{}
)

func ini() {
Copy link
Member

Choose a reason for hiding this comment

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

Think this should be init here as well

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

receiver notification.NotifyReceiver = &actionReceiver{}
)

func ini() {
Copy link
Member

Choose a reason for hiding this comment

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

Think this should be init

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@@ -8,6 +8,8 @@ import (
"fmt"
"strings"

"code.gitea.io/gitea/modules/notification"

Copy link
Member

Choose a reason for hiding this comment

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

Why is this in a separate import paragraph?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

package webhook

import (
"code.gitea.io/git"
Copy link
Member

Choose a reason for hiding this comment

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

code.gitea.io/git is an external package?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is not inside of gitea. IMO

@lunny lunny force-pushed the lunny/notification branch 3 times, most recently from 8ffa982 to 796805c Compare July 13, 2018 02:10
@lunny lunny removed this from the 1.x.x milestone Oct 25, 2018
@lunny lunny closed this Oct 25, 2018
@lunny lunny deleted the lunny/notification branch October 25, 2018 15:56
@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. pr/wip This PR is not ready for review type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants