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

[API] orgEditTeam make Fields optional #9556

Merged
merged 22 commits into from
Jan 9, 2020
Merged

[API] orgEditTeam make Fields optional #9556

merged 22 commits into from
Jan 9, 2020

Conversation

6543
Copy link
Member

@6543 6543 commented Dec 31, 2019 •
edited
Loading

as title

@6543 6543 mentioned this pull request Dec 31, 2019
Closed
7 tasks
@lafriks lafriks added this to the 1.12.0 milestone Dec 31, 2019
@lafriks lafriks added the type/enhancement An improvement of existing functionality label Dec 31, 2019
zeripath
zeripath approved these changes Dec 31, 2019
View reviewed changes
Copy link
Contributor

@zeripath zeripath left a comment

Choose a reason for hiding this comment

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

We also need team name based team endpoints for the API. (However we'll have to keep giving out the id to not break existing users)

@GiteaBot GiteaBot added the lgtm/need 1 This PR needs approval from one additional maintainer to be merged. label Dec 31, 2019
@GiteaBot GiteaBot 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 Jan 1, 2020
lunny
lunny requested changes Jan 1, 2020
View reviewed changes
Copy link
Member

@lunny lunny left a comment

Choose a reason for hiding this comment

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

Oh. This will not work. See models.UpdateTeam.

@codecov-io
Copy link

codecov-io commented Jan 1, 2020 •
edited
Loading

Codecov Report

Merging #9556 into master will increase coverage by 0.13%.
The diff coverage is 64.01%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9556      +/-   ##
==========================================
+ Coverage   42.09%   42.23%   +0.13%     
==========================================
  Files         584      587       +3     
  Lines       77497    77958     +461     
==========================================
+ Hits        32624    32925     +301     
- Misses      40859    40993     +134     
- Partials     4014     4040      +26
Impacted Files Coverage Δ
routers/api/v1/repo/pull.go 35.81% <0%> (-0.11%) ⬇️
models/issue_comment.go 45.61% <0%> (-0.97%) ⬇️
routers/api/v1/admin/user.go 28.47% <0%> (ø) ⬆️
routers/api/v1/api.go 74.38% <100%> (+0.41%) ⬆️
routers/api/v1/notify/user.go 41.74% <41.74%> (ø)
models/notification.go 64.3% <62.68%> (-2.08%) ⬇️
models/issue.go 55.02% <63.63%> (+0.07%) ⬆️
routers/api/v1/notify/threads.go 76.62% <76.62%> (ø)
routers/api/v1/notify/repo.go 80.8% <80.8%> (ø)
modules/log/event.go 64.61% <0%> (-1.03%) ⬇️
... and 6 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 1bd9e32...cb9b3a9. Read the comment docs.

@6543
Copy link
Member Author

6543 commented Jan 1, 2020 •
edited
Loading

@lunny tested and worked!

https://github.com/go-gitea/gitea/pull/9556/files#diff-3456f19402661bf53eee0c00f4b034c6R206 make sure name is not empty ...

and if you mean https://github.com/go-gitea/gitea/blob/master/models/org_team.go#L581-L586
this only make sure two teams with same name can NOT exist ...

@lunny
Copy link
Member

lunny commented Jan 2, 2020

Have you tested a blank name? Or could you add a test for that situation?

@6543 6543 changed the title API: orgEditTeam make Name optional API: orgEditTeam make Fields optional Jan 2, 2020
@6543 6543 changed the title API: orgEditTeam make Fields optional [WIP] [API] orgEditTeam make Name optional Jan 2, 2020
@6543 6543 force-pushed the api-orgEditTeam_make-Name-optional branch from 728ba18 to ecc15a5 Compare January 2, 2020 07:09
@6543 6543 changed the title [WIP] [API] orgEditTeam make Name optional [API] orgEditTeam make Name optional Jan 2, 2020
@6543
Copy link
Member Author

6543 commented Jan 2, 2020

@lunny added Test and make all Fields optional

@6543 6543 changed the title [API] orgEditTeam make Name optional [API] orgEditTeam make Fields optional Jan 2, 2020
6543
6543 commented Jan 2, 2020
View reviewed changes
integrations/api_team_test.go Outdated Show resolved Hide resolved
@6543
Copy link
Member Author

6543 commented Jan 2, 2020 •
edited
Loading

@lunny I looked throu the whole function and found out that the API need all fields to be filed otherwhies it changes - eaven if you don't intend to :(

now all fields are optional (beside id ...)
and the TEST make sure it is optional :D

lafriks
lafriks requested changes Jan 2, 2020
View reviewed changes
routers/api/v1/org/team.go Outdated Show resolved Hide resolved
routers/api/v1/org/team.go Outdated Show resolved Hide resolved
routers/api/v1/org/team.go Outdated Show resolved Hide resolved
6543 and others added 2 commits January 2, 2020 14:58
@6543
Copy link
Member Author

6543 commented Jan 2, 2020

@lafriks added your suggestions

@6543

This comment has been minimized.

Sign in to view
lunny
lunny requested changes Jan 4, 2020
View reviewed changes
routers/api/v1/org/team.go Show resolved Hide resolved
routers/api/v1/org/team.go Outdated Show resolved Hide resolved
routers/api/v1/org/team.go Show resolved Hide resolved
@lunny
Copy link
Member

lunny commented Jan 4, 2020

@6543 You should not insert records with id on mssql since it's autoincrement. But mysql will support that.

@6543
Copy link
Member Author

6543 commented Jan 4, 2020

@lunny done

https://github.com/go-gitea/gitea/pull/9556/files#diff-f13bb9abff87d33ca9cfc067797eb48bL609-R609

@6543 6543 requested a review from lunny January 4, 2020 05:54
@zeripath zeripath merged commit 1080c76 into go-gitea:master Jan 9, 2020
@6543 6543 deleted the api-orgEditTeam_make-Name-optional branch January 9, 2020 13:55
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@zeripath zeripath zeripath approved these changes

@lafriks lafriks lafriks approved these changes

@lunny lunny lunny 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/enhancement An improvement of existing functionality
Projects
None yet
Milestone
1.12.0
Development

Successfully merging this pull request may close these issues.
6 participants
@6543 @codecov-io @lunny @lafriks @zeripath @GiteaBot