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 transfer repository and rename repository on a service package and start action notification #8573

Merged
merged 11 commits into from
Nov 15, 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
1 change: 0 additions & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,6 @@ require (
github.com/prometheus/client_golang v1.1.0
github.com/prometheus/procfs v0.0.4 // indirect
github.com/remyoudompheng/bigfft v0.0.0-20190321074620-2f0d2b0e0001 // indirect
github.com/russross/blackfriday v2.0.0+incompatible // indirect
github.com/russross/blackfriday/v2 v2.0.1
github.com/saintfish/chardet v0.0.0-20120816061221-3af4cd4741ca // indirect
github.com/satori/go.uuid v1.2.0
Expand Down
45 changes: 0 additions & 45 deletions go.sum

Large diffs are not rendered by default.

46 changes: 0 additions & 46 deletions models/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -522,52 +522,6 @@ func UpdateIssuesCommit(doer *User, repo *Repository, commits []*PushCommit, bra
return nil
}

func transferRepoAction(e Engine, doer, oldOwner *User, repo *Repository) (err error) {
if err = notifyWatchers(e, &Action{
ActUserID: doer.ID,
ActUser: doer,
OpType: ActionTransferRepo,
RepoID: repo.ID,
Repo: repo,
IsPrivate: repo.IsPrivate,
Content: path.Join(oldOwner.Name, repo.Name),
}); err != nil {
return fmt.Errorf("notifyWatchers: %v", err)
}

// Remove watch for organization.
if oldOwner.IsOrganization() {
if err = watchRepo(e, oldOwner.ID, repo.ID, false); err != nil {
return fmt.Errorf("watchRepo [false]: %v", err)
}
}

return nil
}

// TransferRepoAction adds new action for transferring repository,
// the Owner field of repository is assumed to be new owner.
func TransferRepoAction(doer, oldOwner *User, repo *Repository) error {
return transferRepoAction(x, doer, oldOwner, repo)
}

func mergePullRequestAction(e Engine, doer *User, repo *Repository, issue *Issue) error {
return notifyWatchers(e, &Action{
ActUserID: doer.ID,
ActUser: doer,
OpType: ActionMergePullRequest,
Content: fmt.Sprintf("%d|%s", issue.Index, issue.Title),
RepoID: repo.ID,
Repo: repo,
IsPrivate: repo.IsPrivate,
})
}

// MergePullRequestAction adds new action for merging pull request.
func MergePullRequestAction(actUser *User, repo *Repository, pull *Issue) error {
return mergePullRequestAction(x, actUser, repo, pull)
}

// GetFeedsOptions options for retrieving feeds
type GetFeedsOptions struct {
RequestedUser *User
Expand Down
48 changes: 0 additions & 48 deletions models/action_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -332,54 +332,6 @@ func TestUpdateIssuesCommit_AnotherRepoNoPermission(t *testing.T) {
CheckConsistencyFor(t, &Action{})
}

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

user2 := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
user4 := AssertExistsAndLoadBean(t, &User{ID: 4}).(*User)
repo := AssertExistsAndLoadBean(t, &Repository{ID: 1, OwnerID: user2.ID}).(*Repository)

repo.OwnerID = user4.ID
repo.Owner = user4

actionBean := &Action{
OpType: ActionTransferRepo,
ActUserID: user2.ID,
ActUser: user2,
RepoID: repo.ID,
Repo: repo,
IsPrivate: repo.IsPrivate,
}
AssertNotExistsBean(t, actionBean)
assert.NoError(t, TransferRepoAction(user2, user2, repo))
AssertExistsAndLoadBean(t, actionBean)

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

func TestMergePullRequestAction(t *testing.T) {
assert.NoError(t, PrepareTestDatabase())
user := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
repo := AssertExistsAndLoadBean(t, &Repository{ID: 1, OwnerID: user.ID}).(*Repository)
repo.Owner = user
issue := AssertExistsAndLoadBean(t, &Issue{ID: 3, RepoID: repo.ID}).(*Issue)

actionBean := &Action{
OpType: ActionMergePullRequest,
ActUserID: user.ID,
ActUser: user,
RepoID: repo.ID,
Repo: repo,
IsPrivate: repo.IsPrivate,
}
AssertNotExistsBean(t, actionBean)
assert.NoError(t, MergePullRequestAction(user, repo, issue))
AssertExistsAndLoadBean(t, actionBean)
CheckConsistencyFor(t, &Action{})
}

func TestGetFeeds(t *testing.T) {
// test with an individual user
assert.NoError(t, PrepareTestDatabase())
Expand Down
31 changes: 17 additions & 14 deletions models/repo.go
Original file line number Diff line number Diff line change
Expand Up @@ -1792,8 +1792,13 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) error

if err = watchRepo(sess, doer.ID, repo.ID, true); err != nil {
return fmt.Errorf("watchRepo: %v", err)
} else if err = transferRepoAction(sess, doer, owner, repo); err != nil {
return fmt.Errorf("transferRepoAction: %v", err)
}

// Remove watch for organization.
if owner.IsOrganization() {
if err = watchRepo(sess, owner.ID, repo.ID, false); err != nil {
return fmt.Errorf("watchRepo [false]: %v", err)
Copy link
Member

Choose a reason for hiding this comment

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

@lunny should we use log.Error from code.gitea.io/gitea/modules/log in models/* too?

Copy link
Member Author

Choose a reason for hiding this comment

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

Finally we should use a standard log library to do that. Maybe code.gitea.io/log . Here we just return the error.

Copy link
Contributor

Choose a reason for hiding this comment

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

You can use modules/log in models - it's used in lots of places already.

}
}

// Rename remote repository to new path and delete local copy.
Expand Down Expand Up @@ -1824,23 +1829,21 @@ func TransferOwnership(doer *User, newOwnerName string, repo *Repository) error
}

// ChangeRepositoryName changes all corresponding setting from old repository name to new one.
func ChangeRepositoryName(u *User, oldRepoName, newRepoName string) (err error) {
oldRepoName = strings.ToLower(oldRepoName)
func ChangeRepositoryName(doer *User, repo *Repository, newRepoName string) (err error) {
newRepoName = strings.ToLower(newRepoName)
if err = IsUsableRepoName(newRepoName); err != nil {
return err
}

has, err := IsRepositoryExist(u, newRepoName)
if err != nil {
return fmt.Errorf("IsRepositoryExist: %v", err)
} else if has {
return ErrRepoAlreadyExist{u.Name, newRepoName}
if err := repo.GetOwner(); err != nil {
return err
}

repo, err := GetRepositoryByName(u.ID, oldRepoName)
has, err := IsRepositoryExist(repo.Owner, newRepoName)
if err != nil {
return fmt.Errorf("GetRepositoryByName: %v", err)
return fmt.Errorf("IsRepositoryExist: %v", err)
} else if has {
return ErrRepoAlreadyExist{repo.Owner.Name, newRepoName}
}

// Change repository directory name. We must lock the local copy of the
Expand All @@ -1849,14 +1852,14 @@ func ChangeRepositoryName(u *User, oldRepoName, newRepoName string) (err error)
repoWorkingPool.CheckIn(com.ToStr(repo.ID))
defer repoWorkingPool.CheckOut(com.ToStr(repo.ID))

newRepoPath := RepoPath(u.Name, newRepoName)
newRepoPath := RepoPath(repo.Owner.Name, newRepoName)
if err = os.Rename(repo.RepoPath(), newRepoPath); err != nil {
return fmt.Errorf("rename repository directory: %v", err)
}

wikiPath := repo.WikiPath()
if com.IsExist(wikiPath) {
if err = os.Rename(wikiPath, WikiPath(u.Name, newRepoName)); err != nil {
if err = os.Rename(wikiPath, WikiPath(repo.Owner.Name, newRepoName)); err != nil {
return fmt.Errorf("rename repository wiki: %v", err)
}
}
Expand All @@ -1868,7 +1871,7 @@ func ChangeRepositoryName(u *User, oldRepoName, newRepoName string) (err error)
}

// If there was previously a redirect at this location, remove it.
if err = deleteRepoRedirect(sess, u.ID, newRepoName); err != nil {
if err = deleteRepoRedirect(sess, repo.OwnerID, newRepoName); err != nil {
return fmt.Errorf("delete repo redirect: %v", err)
}

Expand Down
24 changes: 0 additions & 24 deletions models/repo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import (
"code.gitea.io/gitea/modules/markup"

"github.com/stretchr/testify/assert"
"github.com/unknwon/com"
)

func TestRepo(t *testing.T) {
Expand Down Expand Up @@ -142,29 +141,6 @@ func TestRepoAPIURL(t *testing.T) {
assert.Equal(t, "https://try.gitea.io/api/v1/repos/user12/repo10", repo.APIURL())
}

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

doer := AssertExistsAndLoadBean(t, &User{ID: 2}).(*User)
repo := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
repo.Owner = AssertExistsAndLoadBean(t, &User{ID: repo.OwnerID}).(*User)
assert.NoError(t, TransferOwnership(doer, "user2", repo))

transferredRepo := AssertExistsAndLoadBean(t, &Repository{ID: 3}).(*Repository)
assert.EqualValues(t, 2, transferredRepo.OwnerID)

assert.False(t, com.IsExist(RepoPath("user3", "repo3")))
assert.True(t, com.IsExist(RepoPath("user2", "repo3")))
AssertExistsAndLoadBean(t, &Action{
OpType: ActionTransferRepo,
ActUserID: 2,
RepoID: 3,
Content: "user3/repo3",
})

CheckConsistencyFor(t, &Repository{}, &User{}, &Team{})
}

func TestUploadAvatar(t *testing.T) {

// Generate image
Expand Down
25 changes: 20 additions & 5 deletions modules/notification/action/action.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ package action

import (
"fmt"
"path"
"strings"

"code.gitea.io/gitea/models"
Expand Down Expand Up @@ -77,19 +78,33 @@ func (a *actionNotifier) NotifyNewPullRequest(pull *models.PullRequest) {
}
}

func (a *actionNotifier) NotifyRenameRepository(doer *models.User, repo *models.Repository, oldName string) {
func (a *actionNotifier) NotifyRenameRepository(doer *models.User, repo *models.Repository, oldRepoName string) {
log.Trace("action.ChangeRepositoryName: %s/%s", doer.Name, repo.Name)

if err := models.NotifyWatchers(&models.Action{
ActUserID: doer.ID,
ActUser: doer,
OpType: models.ActionRenameRepo,
RepoID: repo.ID,
Repo: repo,
IsPrivate: repo.IsPrivate,
Content: oldName,
Content: oldRepoName,
}); err != nil {
log.Error("notify watchers: %v", err)
} else {
log.Trace("action.renameRepoAction: %s/%s", doer.Name, repo.Name)
log.Error("NotifyWatchers: %v", err)
}
}

func (a *actionNotifier) NotifyTransferRepository(doer *models.User, repo *models.Repository, oldOwnerName string) {
if err := models.NotifyWatchers(&models.Action{
ActUserID: doer.ID,
ActUser: doer,
OpType: models.ActionTransferRepo,
RepoID: repo.ID,
Repo: repo,
IsPrivate: repo.IsPrivate,
Content: path.Join(oldOwnerName, repo.Name),
}); err != nil {
log.Error("NotifyWatchers: %v", err)
}
}

Expand Down
3 changes: 2 additions & 1 deletion modules/notification/base/notifier.go
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,8 @@ 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)
NotifyRenameRepository(doer *models.User, repo *models.Repository, oldRepoName string)
NotifyTransferRepository(doer *models.User, repo *models.Repository, oldOwnerName string)

NotifyNewIssue(*models.Issue)
NotifyIssueChangeStatus(*models.User, *models.Issue, bool)
Expand Down
28 changes: 16 additions & 12 deletions modules/notification/base/null.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,18 +58,6 @@ func (*NullNotifier) NotifyUpdateComment(doer *models.User, c *models.Comment, o
func (*NullNotifier) NotifyDeleteComment(doer *models.User, c *models.Comment) {
}

// NotifyDeleteRepository places a place holder function
func (*NullNotifier) NotifyDeleteRepository(doer *models.User, repo *models.Repository) {
Copy link
Member

@6543 6543 Nov 6, 2019

Choose a reason for hiding this comment

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

why do you remove this?

./services/repository/repository.go:
notification.NotifyDeleteRepository(doer, repo)
./modules/notification/base/notifier.go:
NotifyDeleteRepository(doer *models.User, repo *models.Repository)
./modules/notification/indexer/indexer.go:
func (r *indexerNotifier) NotifyDeleteRepository(doer *models.User, repo *models.Repository) {
./modules/notification/webhook/webhook.go:
func (m *webhookNotifier) NotifyDeleteRepository(doer *models.User, repo *models.Repository) {
./modules/notification/notification.go:
// NotifyDeleteRepository notifies delete repository to notifiers func NotifyDeleteRepository(doer *models.User, repo *models.Repository) { notifier.NotifyDeleteRepository(doer, repo)

its still used ?!?

Copy link
Member Author

Choose a reason for hiding this comment

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

}

// NotifyForkRepository places a place holder function
func (*NullNotifier) NotifyForkRepository(doer *models.User, oldRepo, repo *models.Repository) {
Copy link
Member

Choose a reason for hiding this comment

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

same here

./services/repository/repository.go:
notification.NotifyForkRepository(doer, oldRepo, repo)
./modules/notification/base/notifier.go:
NotifyForkRepository(doer *models.User, oldRepo, repo *models.Repository)
./modules/notification/webhook/webhook.go:
func (m *webhookNotifier) NotifyForkRepository(doer *models.User, oldRepo, repo *models.Repository) {
./modules/notification/notification.go:
// NotifyForkRepository notifies fork repository to notifiers func NotifyForkRepository(doer *models.User, oldRepo, repo *models.Repository) { notifier.NotifyForkRepository(doer, oldRepo, repo)

Copy link
Member Author

Choose a reason for hiding this comment

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

As above.

}

// 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 Expand Up @@ -111,6 +99,14 @@ func (*NullNotifier) NotifyIssueChangeLabels(doer *models.User, issue *models.Is
func (*NullNotifier) NotifyCreateRepository(doer *models.User, u *models.User, repo *models.Repository) {
}

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

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

// NotifyMigrateRepository places a place holder function
func (*NullNotifier) NotifyMigrateRepository(doer *models.User, u *models.User, repo *models.Repository) {
}
Expand All @@ -126,3 +122,11 @@ func (*NullNotifier) NotifyCreateRef(doer *models.User, repo *models.Repository,
// NotifyDeleteRef notifies branch or tag deleteion to notifiers
func (*NullNotifier) NotifyDeleteRef(doer *models.User, repo *models.Repository, refType, refFullName string) {
}

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

// NotifyTransferRepository places a place holder function
func (*NullNotifier) NotifyTransferRepository(doer *models.User, repo *models.Repository, oldOwnerName string) {
}
49 changes: 28 additions & 21 deletions modules/notification/notification.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,27 +101,6 @@ func NotifyDeleteComment(doer *models.User, c *models.Comment) {
}
}

// NotifyDeleteRepository notifies delete repository to notifiers
func NotifyDeleteRepository(doer *models.User, repo *models.Repository) {
for _, notifier := range notifiers {
notifier.NotifyDeleteRepository(doer, repo)
}
}

// NotifyForkRepository notifies fork repository to notifiers
func NotifyForkRepository(doer *models.User, oldRepo, repo *models.Repository) {
for _, notifier := range notifiers {
notifier.NotifyForkRepository(doer, oldRepo, repo)
}
}

// 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 Expand Up @@ -200,6 +179,34 @@ func NotifyMigrateRepository(doer *models.User, u *models.User, repo *models.Rep
}
}

// NotifyTransferRepository notifies create repository to notifiers
func NotifyTransferRepository(doer *models.User, repo *models.Repository, newOwnerName string) {
for _, notifier := range notifiers {
notifier.NotifyTransferRepository(doer, repo, newOwnerName)
}
}

// NotifyDeleteRepository notifies delete repository to notifiers
func NotifyDeleteRepository(doer *models.User, repo *models.Repository) {
for _, notifier := range notifiers {
notifier.NotifyDeleteRepository(doer, repo)
}
}

// NotifyForkRepository notifies fork repository to notifiers
func NotifyForkRepository(doer *models.User, oldRepo, repo *models.Repository) {
for _, notifier := range notifiers {
notifier.NotifyForkRepository(doer, oldRepo, repo)
}
}

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

// NotifyPushCommits notifies commits pushed to notifiers
func NotifyPushCommits(pusher *models.User, repo *models.Repository, refName, oldCommitID, newCommitID string, commits *models.PushCommits) {
for _, notifier := range notifiers {
Expand Down
Loading