Skip to content

Notification structure refactor #22266

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
Closed
wants to merge 25 commits into from
Closed

Notification structure refactor #22266

wants to merge 25 commits into from

Conversation

lunny
Copy link
Member

@lunny lunny commented Dec 29, 2022 •
edited
Loading

Follow #22256 , once #22256 merged, we can also merge base into the parent folder. The problem is some of the interface function arguments will depend on models package which I think could be changed step by step.

  • Merge modules/notification/base into modules/notification
  • Rename modules/notification -> services/notify
  • Rename the notifier interface to remove the Notify prefix
  • Move all Notify implementations to other services packages.

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Dec 29, 2022
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 29, 2022
@KN4CK3R
Copy link
Member

KN4CK3R commented Dec 29, 2022

The Notifier interface can't be kept in modules because it depends on models and that's forbidden.

@lunny lunny added this to the 1.19.0 milestone Jan 1, 2023
@lunny
Copy link
Member Author

lunny commented Jan 1, 2023

The Notifier interface can't be kept in modules because it depends on models and that's forbidden.

Done.

@delvh
Copy link
Member

delvh commented Jan 1, 2023

What I just noticed:
All public functions in this package start with Notify, and the package is called notify as well now, so function calls are always notify.NotifyX. In addition, we don't use the notifier objects directly, we simply call the corresponding notification method.
So, should we perhaps simply drop the (now unnecessary) Notify- prefix?
This would for example shorten notify.NotifyPackageDelete(apictx, apictx.Doer, pd) to notify.PackageDelete(apictx, apictx.Doer, pd).
This doesn't even need to be done in this PR, I can post a subsequent PR once this one is merged.

@jolheiser
Copy link
Member

So, should we perhaps simply drop the (now unnecessary) Notify- prefix?

Yes, this would reduce stutter and make it easier to read. 👍
But I also agree that a follow-up PR is fine for that.

@delvh delvh added the skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. label Jan 1, 2023
delvh
delvh reviewed Jan 1, 2023
View reviewed changes
@lunny lunny changed the title Move notification implementations out of modules Notification structure refactor Jan 2, 2023
@lunny
Copy link
Member Author

lunny commented Jan 2, 2023

@delvh @jolheiser both done.

delvh
delvh approved these changes Jan 2, 2023
View reviewed changes
@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 Jan 2, 2023
@delvh
Copy link
Member

delvh commented May 1, 2023

Yes, unfortunately we haven't been able to find out why the CI keeps failing.
My guess: The queue/notifications haven't been initialized.

@wxiaoguang wxiaoguang mentioned this pull request May 3, 2023
Merged
@wxiaoguang
Copy link
Contributor

I guess it would cause conflicts with New webhook trigger for receiving Pull Request review requests #24481

@delvh delvh removed this from the 1.20.0 milestone Jun 7, 2023
@lunny
Copy link
Member Author

lunny commented Jul 2, 2023

Closed as too many conflicts to handle.

@lunny lunny closed this Jul 2, 2023
@lunny lunny deleted the lunny/move_notification_impl_services branch July 2, 2023 04:34
This was referenced Sep 5, 2023
Merged
Merged
Merged
Merged
Merged
lunny added a commit that referenced this pull request Sep 5, 2023
@lunny
Move mail notification logic to service layer (#26905)
a66d883 
Extract from #22266
lunny added a commit that referenced this pull request Sep 5, 2023
@lunny
Move indexer notification to service layer (#26906)
aa678b8 
Extract from #22266
silverwind pushed a commit that referenced this pull request Sep 5, 2023
@lunny @GiteaBot
Remove duplicated notify mail configuration on tests (#26912)
dc2dd76 
Extract from #22266

Co-authored-by: Giteabot <[email protected]>
lunny added a commit that referenced this pull request Sep 5, 2023
@lunny
Move ui notification to service layer (#26907)
6c73c0d 
Extract from #22266
lunny added a commit that referenced this pull request Sep 5, 2023
@lunny
Move feed notification service layer (#26908)
f064d71 
Extract from #22266
@lunny lunny mentioned this pull request Sep 5, 2023
Merged
techknowlogick pushed a commit that referenced this pull request Sep 5, 2023
@lunny
Move notification interface to services layer (#26915)
540bf9f 
Extract from #22266
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Sep 30, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wolfogre wolfogre wolfogre approved these changes

@delvh delvh delvh 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. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@lunny @KN4CK3R @delvh @jolheiser @wxiaoguang @wolfogre @yardenshoham @GiteaBot