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

Move organization related structs into sub package #18518

Merged
merged 20 commits into from
Mar 29, 2022
Merged

Move organization related structs into sub package #18518

merged 20 commits into from
Mar 29, 2022

Conversation

lunny
Copy link
Member

@lunny lunny commented Feb 1, 2022 •
edited
Loading

This PR creates a new sub package models/organization and move most organization related code there.

@lunny lunny added the type/refactoring Existing code has been cleaned up. There should be no new functionality. label Feb 1, 2022
@techknowlogick techknowlogick added this to the 1.17.0 milestone Feb 2, 2022
@lunny lunny force-pushed the lunny/model_org2 branch 3 times, most recently from cc424f2 to 41d2afa Compare February 17, 2022 09:47
@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Feb 28, 2022
KN4CK3R
KN4CK3R reviewed Feb 28, 2022
View reviewed changes
models/organization/team_user.go Outdated Show resolved Hide resolved
routers/api/v1/org/member.go Outdated Show resolved Hide resolved
routers/api/v1/org/team.go Outdated Show resolved Hide resolved
routers/web/org/teams.go Outdated Show resolved Hide resolved
routers/web/repo/issue.go Outdated Show resolved Hide resolved
@lunny lunny force-pushed the lunny/model_org2 branch 2 times, most recently from 727033b to 0dd262a Compare March 3, 2022 07:25
@codecov-commenter
Copy link

Codecov Report

Merging #18518 (ed5f6f1) into main (bc0d2c8) will decrease coverage by 0.10%.
The diff coverage is 68.37%.

@@            Coverage Diff             @@
##             main   #18518      +/-   ##
==========================================
- Coverage   46.58%   46.47%   -0.11%     
==========================================
  Files         855      863       +8     
  Lines      122953   122950       -3     
==========================================
- Hits        57276    57146     -130     
- Misses      58764    58879     +115     
- Partials     6913     6925      +12
Impacted Files Coverage Δ
models/action.go 48.61% <0.00%> (ø)
models/branches.go 47.21% <0.00%> (ø)
models/db/list_options.go 42.00% <0.00%> (-5.73%) ⬇️
models/error.go 35.89% <ø> (-2.42%) ⬇️
models/issue_comment.go 51.30% <0.00%> (-1.02%) ⬇️
models/organization/team.go 64.87% <ø> (ø)
models/organization/team_repo.go 74.07% <ø> (ø)
models/organization/team_unit.go 21.05% <ø> (ø)
models/organization/team_user.go 82.69% <ø> (ø)
models/protected_tag.go 67.64% <ø> (ø)
... and 111 more

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 83a2f79...ed5f6f1. Read the comment docs.

@Gusted
Copy link
Contributor

Gusted commented Mar 20, 2022

Maybe these kinds of refactors should be done in batches. 93 files? it's quite a lot to go trough and not to miss anything, it has happen before that these refactors changed a little aspect of code and broke something(templates NPE).

@lunny
Copy link
Member Author

lunny commented Mar 20, 2022

Maybe these kinds of refactors should be done in batches. 93 files? it's quite a lot to go trough and not to miss anything, it has happen before that these refactors changed a little aspect of code and broke something(templates NPE).

What did you mean in batches?

Although there are 93 files changed, but more of them are move and rename. The previous templates NPE because of some field references removed from Go code but not from templates. But I think those refactors affect little bugs rather than the number of changed files in related PRs. The benefit of these PRs are greater than not do it when more features added and models becomes big and big. The codebase will have a better readability and maintainability.

@Gusted
Copy link
Contributor

Gusted commented Mar 20, 2022

Yeah NVM about that comment, misread the code.

But to note, while most of these are automated changes with simple copy & paste, the generated diff still show the raw changes and can be hard to skim trough.

Gusted
Gusted reviewed Mar 20, 2022
View reviewed changes
models/organization/org_user.go Outdated Show resolved Hide resolved
models/organization/org_user.go Outdated Show resolved Hide resolved
models/organization/team.go Show resolved Hide resolved
@6543
Copy link
Member

6543 commented Mar 21, 2022

@lunny TestWithOwnerUser and other failed

@lunny lunny force-pushed the lunny/model_org2 branch 5 times, most recently from dc29e80 to abf24f9 Compare March 27, 2022 11:12
@lunny lunny requested review from Gusted, KN4CK3R and 6543 March 27, 2022 11:13
Gusted
Gusted reviewed Mar 27, 2022
View reviewed changes
Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

2 nitpicks and 1 general question

models/organization/team_user.go Outdated Show resolved Hide resolved
models/organization/team_user.go Outdated Show resolved Hide resolved
routers/api/v1/org/org.go Outdated Show resolved Hide resolved
Gusted
Gusted suggested changes Mar 27, 2022
View reviewed changes
Copy link
Contributor

@Gusted Gusted left a comment

Choose a reason for hiding this comment

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

As per: #18518 (comment) replace db.DefaultContext -> ctx where possible.

@lunny
Copy link
Member Author

lunny commented Mar 29, 2022

make L-G-T-M work.

@lunny lunny merged commit b06b9a0 into go-gitea:main Mar 29, 2022
@lunny lunny deleted the lunny/model_org2 branch March 29, 2022 06:29
zjjhot added a commit to zjjhot/gitea that referenced this pull request Mar 30, 2022
@zjjhot
Merge remote-tracking branch 'giteaoffical/main'
f6c0470 
* giteaoffical/main: (31 commits)
  Add Package Registry (go-gitea#16510)
  Show messages for users if the ROOT_URL is wrong, show JavaScript errors (go-gitea#18971)
  [skip ci] Updated translations via Crowdin
  Make git.OpenRepository accept Context (go-gitea#19260)
  Use full output of git show-ref --tags to get tags for PushUpdateAddTag (go-gitea#19235)
  When conflicts have been previously detected ensure that they can be resolved (go-gitea#19247)
  More commit info from API (go-gitea#19252)
  Move some issue methods as functions (go-gitea#19255)
  Move project files into models/project sub package (go-gitea#17704)
  Granular webhook events in editHook (go-gitea#19251)
  Provide configuration to allow camo-media proxying (go-gitea#12802)
  Move init repository related functions to modules (go-gitea#19159)
  Move organization related structs into sub package (go-gitea#18518)
  Refactor repo clone button and repo clone links, fix JS error on empty repo page (go-gitea#19208)
  Show last cron messages on monitor page (go-gitea#19223)
  Allow API to create file on empty repo (go-gitea#19224)
  Use goproxy.io instead of goproxy.cn (go-gitea#19242)
  New cron task: delete old system notices (go-gitea#19219)
  Let web and API routes have different auth methods group (go-gitea#19168)
  Only send webhook events to active system webhooks and only deliver to active hooks (go-gitea#19234)
  ...
noerw added a commit to noerw/gitea that referenced this pull request Mar 30, 2022
@noerw
Fix creating new org teams
676a257 
regression from go-gitea#18518
@noerw noerw mentioned this pull request Mar 30, 2022
Closed
1 task
@go-gitea go-gitea locked and limited conversation to collaborators Apr 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@wxiaoguang wxiaoguang wxiaoguang approved these changes

@Gusted Gusted Gusted approved these changes

@6543 6543 6543 approved these changes

@KN4CK3R KN4CK3R Awaiting requested review from KN4CK3R

Assignees
No one assigned
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Milestone
1.17.0
Development

Successfully merging this pull request may close these issues.
8 participants
@lunny @codecov-commenter @Gusted @6543 @wxiaoguang @KN4CK3R @techknowlogick @GiteaBot