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.

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

Fix orgnization user watch repository #2670

Merged
merged 4 commits into from
Oct 14, 2017
Merged

Fix orgnization user watch repository #2670

merged 4 commits into from
Oct 14, 2017

Conversation

lunny
Copy link
Member

@lunny lunny commented Oct 7, 2017 •
edited
Loading

fix #2667

@lunny lunny changed the title ix migrate watch Fix orgnization user watch repository Oct 7, 2017
@lunny lunny added the type/bug label Oct 7, 2017
@lunny lunny added this to the 1.3.0 milestone Oct 7, 2017
@codecov-io
Copy link

codecov-io commented Oct 7, 2017 •
edited
Loading

Codecov Report

Merging #2670 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2670   +/-   ##
=======================================
  Coverage   27.38%   27.38%           
=======================================
  Files          86       86           
  Lines       17010    17010           
=======================================
  Hits         4658     4658           
  Misses      11673    11673           
  Partials      679      679
Impacted Files Coverage Δ
models/repo.go 13.14% <0%> (ø) ⬆️

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 a4cd461...6e6f29d. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 7, 2017
@lafriks
Copy link
Member

lafriks commented Oct 7, 2017

Does user creating repository for organization need to be added automatically as watcher?

ethantkoenig
ethantkoenig suggested changes Oct 7, 2017
View reviewed changes
models/repo.go
@@ -1295,7 +1295,7 @@ func createRepository(e *xorm.Session, doer, u *User, repo *Repository) (err err
}
}

if err = watchRepo(e, u.ID, repo.ID, true); err != nil {
if err = watchRepo(e, doer.ID, repo.ID, true); err != nil {
Copy link
Member

Choose a reason for hiding this comment

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

Need to update other call to watchRepo(..) on line 1483

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.

models/migrations/migrations.go Outdated
@@ -140,6 +140,8 @@ var migrations = []Migration{
NewMigration("remove duplicate unit types", removeDuplicateUnitTypes),
// v45 -> v46
NewMigration("remove index column from repo_unit table", removeIndexColumnFromRepoUnitTable),
// v46 -> v47
NewMigration("remove orgnization watch repositories", removeOrgnizationWatchRepo),
Copy link
Member

Choose a reason for hiding this comment

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

spelling (orgnization -> organization), both the comment and function name.

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.

@lunny
Copy link
Member Author

lunny commented Oct 9, 2017

@lafriks I think yes.

@ethantkoenig
Copy link
Member

LGTM

Would it be worth changing watchRepo(..) to take a user argument, instead of a user-id, and check that the user is an individual? Right now, I think there is no way for the user-id to be an organization, but it might protect against future bugs. Just a thought.

@tboerger tboerger 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 Oct 9, 2017
@lunny
Copy link
Member Author

lunny commented Oct 9, 2017

@ethantkoenig Good idea. But for keeping the PR simple, I think that could be another PR.

Morlinest
Morlinest suggested changes Oct 14, 2017
View reviewed changes
models/migrations/v46.go Outdated

func removeOrganizationWatchRepo(x *xorm.Engine) error {
// Watch is connection request for receiving repository notification.
type Watch struct {
Copy link
Member

Choose a reason for hiding this comment

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

Not used, can be removed.

@lunny
Copy link
Member Author

lunny commented Oct 14, 2017

@Morlinest fixed and rebased.

@Morlinest
Copy link
Member

LGTM

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Oct 14, 2017
@lunny lunny merged commit 514006a into go-gitea:master Oct 14, 2017
@lunny lunny deleted the lunny/fix_migrate_watch branch October 14, 2017 14:48
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Morlinest Morlinest Morlinest approved these changes

@ethantkoenig ethantkoenig ethantkoenig 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. type/bug
Projects
None yet
Milestone
1.3.0
Development

Successfully merging this pull request may close these issues.

Mirrors Repo BUG
6 participants
@lunny @codecov-io @lafriks @ethantkoenig @Morlinest @tboerger