Download as
Browse Code »

Commit 818caf0b5d1fc4f0cb2889ca5bd9e2d0d7fd8ac8

Authored by Sebastian Ziebell
1 parent 8045a81b
Exists in master and in 4 other branches 6-8-stable, 7-0-stable, 7-0-stable-spb, spb-stable

API: refined status code handling when adding or updating a project member 

When a user is added to a project that is already a member of, a status code 201 is now returned to
signal an idempotent operation. If something fails then instead of returning error code 404 different
more specific error codes are returned. Status code 400 (Bad request) is returned when a required
attribute, e.g. `access_level` is not given or 422 if there is a semantic error, e.g. should
the `access_level` have an unsupported value.

Specs are added to check these status codes.
Showing 2 changed files with 69 additions and 9 deletions   Show diff stats
lib/api/projects.rb
  Diff comments   View file @818caf0
@@ -89,15 +89,26 @@ module Gitlab @@ -89,15 +89,26 @@ module Gitlab
89 # POST /projects/:id/members 89 # POST /projects/:id/members
90 post ":id/members" do 90 post ":id/members" do
91 authorize! :admin_project, user_project 91 authorize! :admin_project, user_project
92 - users_project = user_project.users_projects.new(  
93 - user_id: params[:user_id],  
94 - project_access: params[:access_level]  
95 - )  
96 92
97 - if users_project.save  
98 - @member = users_project.user 93 + error!("User id not given", 400) if !params.has_key? :user_id
  94 + error!("Access level not given", 400) if !params.has_key? :access_level
  95 +
  96 + # either the user is already a team member or a new one
  97 + team_member = user_project.team_member_by_id(params[:user_id])
  98 + if team_member.nil?
  99 + team_member = user_project.users_projects.new(
  100 + user_id: params[:user_id],
  101 + project_access: params[:access_level]
  102 + )
  103 + end
  104 +
  105 + if team_member.save
  106 + @member = team_member.user
99 present @member, with: Entities::ProjectMember, project: user_project 107 present @member, with: Entities::ProjectMember, project: user_project
100 else 108 else
  109 + if team_member.errors[:project_access].any?
  110 + error!(team_member.errors[:project_access], 422)
  111 + end
101 not_found! 112 not_found!
102 end 113 end
103 end 114 end
@@ -112,12 +123,18 @@ module Gitlab @@ -112,12 +123,18 @@ module Gitlab
112 # PUT /projects/:id/members/:user_id 123 # PUT /projects/:id/members/:user_id
113 put ":id/members/:user_id" do 124 put ":id/members/:user_id" do
114 authorize! :admin_project, user_project 125 authorize! :admin_project, user_project
115 - users_project = user_project.users_projects.find_by_user_id params[:user_id]  
116 126
117 - if users_project.update_attributes(project_access: params[:access_level])  
118 - @member = users_project.user 127 + team_member = user_project.users_projects.find_by_user_id(params[:user_id])
  128 + error!("Access level not given", 400) if !params.has_key? :access_level
  129 + error!("User can not be found", 404) if team_member.nil?
  130 +
  131 + if team_member.update_attributes(project_access: params[:access_level])
  132 + @member = team_member.user
119 present @member, with: Entities::ProjectMember, project: user_project 133 present @member, with: Entities::ProjectMember, project: user_project
120 else 134 else
  135 + if team_member.errors[:project_access].any?
  136 + error!(team_member.errors[:project_access], 422)
  137 + end
121 not_found! 138 not_found!
122 end 139 end
123 end 140 end
spec/requests/api/projects_spec.rb
  Diff comments   View file @818caf0
@@ -177,6 +177,34 @@ describe Gitlab::API do @@ -177,6 +177,34 @@ describe Gitlab::API do
177 json_response['email'].should == user2.email 177 json_response['email'].should == user2.email
178 json_response['access_level'].should == UsersProject::DEVELOPER 178 json_response['access_level'].should == UsersProject::DEVELOPER
179 end 179 end
  180 +
  181 + it "should return a 201 status if user is already project member" do
  182 + post api("/projects/#{project.id}/members", user), user_id: user2.id,
  183 + access_level: UsersProject::DEVELOPER
  184 + expect {
  185 + post api("/projects/#{project.id}/members", user), user_id: user2.id,
  186 + access_level: UsersProject::DEVELOPER
  187 + }.not_to change { UsersProject.count }.by(1)
  188 +
  189 + response.status.should == 201
  190 + json_response['email'].should == user2.email
  191 + json_response['access_level'].should == UsersProject::DEVELOPER
  192 + end
  193 +
  194 + it "should return a 400 error when user id is not given" do
  195 + post api("/projects/#{project.id}/members", user), access_level: UsersProject::MASTER
  196 + response.status.should == 400
  197 + end
  198 +
  199 + it "should return a 400 error when access level is not given" do
  200 + post api("/projects/#{project.id}/members", user), user_id: user2.id
  201 + response.status.should == 400
  202 + end
  203 +
  204 + it "should return a 422 error when access level is not known" do
  205 + post api("/projects/#{project.id}/members", user), user_id: user2.id, access_level: 1234
  206 + response.status.should == 422
  207 + end
180 end 208 end
181 209
182 describe "PUT /projects/:id/members/:user_id" do 210 describe "PUT /projects/:id/members/:user_id" do
@@ -186,6 +214,21 @@ describe Gitlab::API do @@ -186,6 +214,21 @@ describe Gitlab::API do
186 json_response['email'].should == user3.email 214 json_response['email'].should == user3.email
187 json_response['access_level'].should == UsersProject::MASTER 215 json_response['access_level'].should == UsersProject::MASTER
188 end 216 end
  217 +
  218 + it "should return a 404 error if user_id is not found" do
  219 + put api("/projects/#{project.id}/members/1234", user), access_level: UsersProject::MASTER
  220 + response.status.should == 404
  221 + end
  222 +
  223 + it "should return a 400 error when access level is not given" do
  224 + put api("/projects/#{project.id}/members/#{user3.id}", user)
  225 + response.status.should == 400
  226 + end
  227 +
  228 + it "should return a 422 error when access level is not known" do
  229 + put api("/projects/#{project.id}/members/#{user3.id}", user), access_level: 123
  230 + response.status.should == 422
  231 + end
189 end 232 end
190 233
191 describe "DELETE /projects/:id/members/:user_id" do 234 describe "DELETE /projects/:id/members/:user_id" do