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

Add units to team #947

Merged
merged 8 commits into from
May 18, 2017
Merged

Add units to team #947

merged 8 commits into from
May 18, 2017

Conversation

lunny
Copy link
Member

@lunny lunny commented Feb 15, 2017

This PR allows you create one team who can only see issues or pull request or wikis.

@lunny lunny added type/feature Completely new functionality. Can only be merged if feature freeze is not active. pr/wip This PR is not ready for review labels Feb 15, 2017
@lunny lunny added this to the 1.2.0 milestone Feb 15, 2017
@lunny lunny removed the pr/wip This PR is not ready for review label Apr 27, 2017
@lunny lunny changed the title [WIP] Add units to team Add units to team Apr 27, 2017
@lunny
Copy link
Member Author

lunny commented Apr 27, 2017

This is done and could be reviewed. When add/edit a team on organization, you could chose which units could be organization's repositories operated on the by this team. For example, we can define tester team which can only operate issues on all repositories the tester team could visit.

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

@bkcsoft bkcsoft left a comment

Choose a reason for hiding this comment

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

Few comments

UnitTypeWiki,
UnitTypeSettings,
UnitTypeExternalWiki,
UnitTypeExternalTracker,
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need External as a separate unit? Can we enable both internal wiki and external wiki at the same time? (same for Tracker)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think yes. Yes, we can.

4,
}

UnitWiki = Unit{
UnitTypeWiki,
"repo.wiki",
"/wiki",
"repo.wiki_desc",
"repo.wiki.desc",
5,
}

UnitExternalWiki = Unit{
UnitTypeExternalWiki,
Copy link
Member

Choose a reason for hiding this comment

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

Same as above

1,
}

UnitExternalTracker = Unit{
UnitTypeExternalTracker,
"repo.issues",
"repo.ext_issues",
Copy link
Member

Choose a reason for hiding this comment

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

same as above

}
}

ctx.Handle(404, "Home", fmt.Errorf(ctx.Tr("units.error.no_unit_allowed_repo")))
Copy link
Member

Choose a reason for hiding this comment

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

Why are we leaking information that this repo exists but have nothing to show? IMO no valid Units should be the same as Private.

Copy link
Member Author

Choose a reason for hiding this comment

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

The error message is only for log. The UI will only show 404.

}, reqSignIn, context.RepoAssignment(), context.UnitTypes(), context.LoadRepoUnits(), context.CheckUnit(models.UnitTypeIssues))

// Releases
m.Group("/:username/:reponame", func() {
Copy link
Member

Choose a reason for hiding this comment

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

Why was this moved?

Copy link
Member Author

Choose a reason for hiding this comment

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

Move this for easy add middleware contex.CheckUnit. Before this, releases URLs are put on many places.

@bkcsoft
Copy link
Member

bkcsoft commented Apr 28, 2017

LGTM

@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 Apr 28, 2017
@lunny lunny force-pushed the lunny/team_units branch 2 times, most recently from e2da2af to 299b4e9 Compare May 4, 2017 08:01
return err
}

_, err = x.Where("1=1").Update(&Team{
Copy link
Member

@sapk sapk May 18, 2017

Choose a reason for hiding this comment

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

Not an expert in xorm but can't .Where("1=1") be removed ?

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.

@sapk
Copy link
Member

sapk commented May 18, 2017

This need rebase.
I will try to test it but looking at code, I could give my trusted 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 May 18, 2017
@lunny
Copy link
Member Author

lunny commented May 18, 2017

@sapk rebased

@lunny lunny merged commit fd6034a into go-gitea:master May 18, 2017
@lunny lunny deleted the lunny/team_units branch May 18, 2017 14:56
lunny added a commit to lunny/gitea that referenced this pull request May 19, 2017
bkcsoft added a commit that referenced this pull request May 19, 2017
@kubatyszko
Copy link
Contributor

I know this PR is closed, but I believe I found an issue that might be related.
In my installation, SOME repos can't browse source, opening the repo (even clicking on <>code) redirects to /releases.

I think this PR modified a lot of routes related to releases, I wonder if that has anything to do with it...

@lunny
Copy link
Member Author

lunny commented Aug 1, 2017

@kubatyszko could you post an issue to describe that?

@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.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants