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

Move some actions to notification/action #8779

Merged
merged 5 commits into from
Nov 8, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 0 additions & 43 deletions models/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -283,49 +283,6 @@ func (a *Action) GetIssueContent() string {
return issue.Content
}

func newRepoAction(e Engine, u *User, repo *Repository) (err error) {
if err = notifyWatchers(e, &Action{
ActUserID: u.ID,
ActUser: u,
OpType: ActionCreateRepo,
RepoID: repo.ID,
Repo: repo,
IsPrivate: repo.IsPrivate,
}); err != nil {
return fmt.Errorf("notify watchers '%d/%d': %v", u.ID, repo.ID, err)
}

log.Trace("action.newRepoAction: %s/%s", u.Name, repo.Name)
return err
}

// NewRepoAction adds new action for creating repository.
func NewRepoAction(u *User, repo *Repository) (err error) {
return newRepoAction(x, u, repo)
}

func renameRepoAction(e Engine, actUser *User, oldRepoName string, repo *Repository) (err error) {
Copy link
Member

Choose a reason for hiding this comment

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

if err = notifyWatchers(e, &Action{
ActUserID: actUser.ID,
ActUser: actUser,
OpType: ActionRenameRepo,
RepoID: repo.ID,
Repo: repo,
IsPrivate: repo.IsPrivate,
Content: oldRepoName,
}); err != nil {
return fmt.Errorf("notify watchers: %v", err)
}

log.Trace("action.renameRepoAction: %s/%s", actUser.Name, repo.Name)
return nil
}

// RenameRepoAction adds new action for renaming a repository.
func RenameRepoAction(actUser *User, oldRepoName string, repo *Repository) error {
Copy link
Member

Choose a reason for hiding this comment

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

return renameRepoAction(x, actUser, oldRepoName, repo)
}

// PushCommit represents a commit in a push operation.
type PushCommit struct {
Sha1 string
Expand Down
53 changes: 0 additions & 53 deletions models/action_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@ package models

import (
"path"
"strings"
"testing"

"code.gitea.io/gitea/modules/setting"
Expand All @@ -28,58 +27,6 @@ func TestAction_GetRepoLink(t *testing.T) {
assert.Equal(t, expected, action.GetRepoLink())
}

func TestNewRepoAction(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

assert.NoError(t, PrepareTestDatabase())

user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
repo := AssertExistsAndLoadBean(t, &Repository{OwnerID: user.ID}).(*Repository)
repo.Owner = user

actionBean := &Action{
OpType: ActionCreateRepo,
ActUserID: user.ID,
RepoID: repo.ID,
ActUser: user,
Repo: repo,
IsPrivate: repo.IsPrivate,
}

AssertNotExistsBean(t, actionBean)
assert.NoError(t, NewRepoAction(user, repo))
AssertExistsAndLoadBean(t, actionBean)
CheckConsistencyFor(t, &Action{})
}

func TestRenameRepoAction(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())

user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
repo := AssertExistsAndLoadBean(t, &Repository{OwnerID: user.ID}).(*Repository)
repo.Owner = user

oldRepoName := repo.Name
const newRepoName = "newRepoName"
repo.Name = newRepoName
repo.LowerName = strings.ToLower(newRepoName)

actionBean := &Action{
OpType: ActionRenameRepo,
ActUserID: user.ID,
ActUser: user,
RepoID: repo.ID,
Repo: repo,
IsPrivate: repo.IsPrivate,
Content: oldRepoName,
}
AssertNotExistsBean(t, actionBean)
assert.NoError(t, RenameRepoAction(user, oldRepoName, repo))
AssertExistsAndLoadBean(t, actionBean)

_, err := x.ID(repo.ID).Cols("name", "lower_name").Update(repo)
assert.NoError(t, err)
CheckConsistencyFor(t, &Action{})
}

func TestPushCommits_ToAPIPayloadCommits(t *testing.T) {
pushCommits := NewPushCommits()
pushCommits.Commits = []*PushCommit{
Expand Down
11 changes: 9 additions & 2 deletions models/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -1469,8 +1469,15 @@ func createRepository(e *xorm.Session, doer, u *User, repo *Repository) (err err
return fmt.Errorf("watchRepo: %v", err)
}
}
if err = newRepoAction(e, doer, repo); err != nil {
return fmt.Errorf("newRepoAction: %v", err)
if err = notifyWatchers(e, &Action{
ActUserID: doer.ID,
ActUser: doer,
OpType: ActionCreateRepo,
RepoID: repo.ID,
Repo: repo,
IsPrivate: repo.IsPrivate,
}); err != nil {
return fmt.Errorf("notify watchers '%d/%d': %v", doer.ID, repo.ID, err)
}

if err = copyDefaultWebhooksToRepo(e, repo.ID); err != nil {
Expand Down
18 changes: 17 additions & 1 deletion modules/notification/action/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ var (
_ base.Notifier = &actionNotifier{}
)

// NewNotifier create a new webhookNotifier notifier
// NewNotifier create a new actionNotifier notifier
func NewNotifier() base.Notifier {
return &actionNotifier{}
}
Expand Down Expand Up @@ -75,3 +75,19 @@ func (a *actionNotifier) NotifyNewPullRequest(pull *models.PullRequest) {
log.Error("NotifyWatchers: %v", err)
}
}

func (a *actionNotifier) NotifyRenameRepository(doer *models.User, repo *models.Repository, oldName string) {
Copy link
Member

Choose a reason for hiding this comment

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

Seems weird that this is the only action in modules/notification. I guess this is the only one called from outside models, but I wonder why the action isn't simply created by models.ChangeRepositoryName().

Copy link
Member Author

Choose a reason for hiding this comment

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

@guillep2k That will result an import recycle. The dependency cycle is services > notifications > models.

I will move more actions to this notifications on continue refactor PRs.

if err := models.NotifyWatchers(&models.Action{
ActUserID: doer.ID,
ActUser: doer,
OpType: models.ActionRenameRepo,
RepoID: repo.ID,
Repo: repo,
IsPrivate: repo.IsPrivate,
Content: oldName,
}); err != nil {
log.Error("notify watchers: %v", err)
} else {
log.Trace("action.renameRepoAction: %s/%s", doer.Name, repo.Name)
}
}
47 changes: 47 additions & 0 deletions modules/notification/action/action_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
// Copyright 2019 The Gitea Authors. All rights reserved.
// Use of this source code is governed by a MIT-style
// license that can be found in the LICENSE file.

package action

import (
"path/filepath"
"strings"
"testing"

"code.gitea.io/gitea/models"
"github.com/stretchr/testify/assert"
)

func TestMain(m *testing.M) {
models.MainTest(m, filepath.Join("..", "..", ".."))
}

func TestRenameRepoAction(t *testing.T) {
assert.NoError(t, models.PrepareTestDatabase())

user := models.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User)
repo := models.AssertExistsAndLoadBean(t, &models.Repository{OwnerID: user.ID}).(*models.Repository)
repo.Owner = user

oldRepoName := repo.Name
const newRepoName = "newRepoName"
repo.Name = newRepoName
repo.LowerName = strings.ToLower(newRepoName)

actionBean := &models.Action{
OpType: models.ActionRenameRepo,
ActUserID: user.ID,
ActUser: user,
RepoID: repo.ID,
Repo: repo,
IsPrivate: repo.IsPrivate,
Content: oldRepoName,
}
models.AssertNotExistsBean(t, actionBean)

NewNotifier().NotifyRenameRepository(user, repo, oldRepoName)

models.AssertExistsAndLoadBean(t, actionBean)
models.CheckConsistencyFor(t, &models.Action{})
}
1 change: 1 addition & 0 deletions modules/notification/base/notifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ type Notifier interface {
NotifyMigrateRepository(doer *models.User, u *models.User, repo *models.Repository)
NotifyDeleteRepository(doer *models.User, repo *models.Repository)
NotifyForkRepository(doer *models.User, oldRepo, repo *models.Repository)
NotifyRenameRepository(doer *models.User, repo *models.Repository, oldName string)

NotifyNewIssue(*models.Issue)
NotifyIssueChangeStatus(*models.User, *models.Issue, bool)
Expand Down
4 changes: 4 additions & 0 deletions modules/notification/base/null.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,10 @@ func (*NullNotifier) NotifyDeleteRepository(doer *models.User, repo *models.Repo
func (*NullNotifier) NotifyForkRepository(doer *models.User, oldRepo, repo *models.Repository) {
}

// NotifyRenameRepository places a place holder function
func (*NullNotifier) NotifyRenameRepository(doer *models.User, repo *models.Repository, oldName string) {
}

// NotifyNewRelease places a place holder function
func (*NullNotifier) NotifyNewRelease(rel *models.Release) {
}
Expand Down
7 changes: 7 additions & 0 deletions modules/notification/notification.go
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,13 @@ func NotifyForkRepository(doer *models.User, oldRepo, repo *models.Repository) {
}
}

// NotifyRenameRepository notifies repository renamed
func NotifyRenameRepository(doer *models.User, repo *models.Repository, oldName string) {
for _, notifier := range notifiers {
notifier.NotifyRenameRepository(doer, repo, oldName)
}
}

// NotifyNewRelease notifies new release to notifiers
func NotifyNewRelease(rel *models.Release) {
for _, notifier := range notifiers {
Expand Down
6 changes: 1 addition & 5 deletions routers/api/v1/repo/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -603,11 +603,7 @@ func updateBasicProperties(ctx *context.APIContext, opts api.EditRepoOption) err
return err
}

if err := models.RenameRepoAction(ctx.User, oldRepoName, repo); err != nil {
log.Error("RenameRepoAction: %v", err)
ctx.Error(http.StatusInternalServerError, "RenameRepoActions", err)
return err
}
notification.NotifyRenameRepository(ctx.User, repo, oldRepoName)

log.Trace("Repository name changed: %s/%s -> %s", ctx.Repo.Owner.Name, repo.Name, newRepoName)
}
Expand Down
5 changes: 2 additions & 3 deletions routers/repo/setting.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import (
"code.gitea.io/gitea/modules/context"
"code.gitea.io/gitea/modules/git"
"code.gitea.io/gitea/modules/log"
"code.gitea.io/gitea/modules/notification"
"code.gitea.io/gitea/modules/setting"
"code.gitea.io/gitea/modules/timeutil"
"code.gitea.io/gitea/modules/validation"
Expand Down Expand Up @@ -121,9 +122,7 @@ func SettingsPost(ctx *context.Context, form auth.RepoSettingForm) {
log.Trace("Repository basic settings updated: %s/%s", ctx.Repo.Owner.Name, repo.Name)

if isNameChanged {
if err := models.RenameRepoAction(ctx.User, oldRepoName, repo); err != nil {
log.Error("RenameRepoAction: %v", err)
}
notification.NotifyRenameRepository(ctx.User, repo, oldRepoName)
}

ctx.Flash.Success(ctx.Tr("repo.settings.update_settings_success"))
Expand Down