From 2b71995e104c512fc04161b3507378176249023a Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Sat, 20 May 2017 00:10:10 +0800
Subject: [PATCH 01/13] Add shared_runners_minutes_limit to groups and users
API
---
lib/api/entities.rb | 11 +++++++++++
lib/api/groups.rb | 5 +++--
lib/api/users.rb | 15 +++++++++++----
spec/requests/api/groups_spec.rb | 8 ++++++++
spec/requests/api/users_spec.rb | 9 +++++++++
5 files changed, 42 insertions(+), 6 deletions(-)
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index c4425749d4ebf..699bfbdd2bc56 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -38,6 +38,14 @@ class UserPublic < User
expose :can_create_project?, as: :can_create_project
expose :two_factor_enabled?, as: :two_factor_enabled
expose :external
+
+ # EE-only
+ expose :namespace, using: 'API::Entities::UserNamespace'
+ end
+
+ # EE-only
+ class UserNamespace < Grape::Entity
+ expose :shared_runners_minutes_limit
end
class UserWithPrivateDetails < UserPublic
@@ -181,6 +189,9 @@ class Group < Grape::Entity
class GroupDetail < Group
expose :projects, using: Entities::Project
expose :shared_projects, using: Entities::Project
+
+ # EE-only
+ expose :shared_runners_minutes_limit
end
class RepoCommit < Grape::Entity
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index d1d80cabf2b0f..92878aa3ada7d 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -17,6 +17,7 @@ class Groups < Grape::API
optional :membership_lock, type: Boolean, desc: 'Prevent adding new members to project membership within this group'
optional :ldap_cn, type: String, desc: 'LDAP Common Name'
optional :ldap_access, type: Integer, desc: 'A valid access level'
+ optional :shared_runners_minutes_limit, type: Integer, desc: 'Pipeline minutes quota for this group'
all_or_none_of :ldap_cn, :ldap_access
end
@@ -118,8 +119,8 @@ def present_groups(groups, options = {})
optional :name, type: String, desc: 'The name of the group'
optional :path, type: String, desc: 'The path of the group'
use :optional_params
- at_least_one_of :name, :path, :description, :visibility,
- :lfs_enabled, :request_access_enabled
+
+ at_least_one_of(*@declared_params.flatten - [:id])
end
put ':id' do
group = find_group!(params[:id])
diff --git a/lib/api/users.rb b/lib/api/users.rb
index a10030aa31e99..f83901a87dec0 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -30,6 +30,11 @@ def find_user(params)
optional :skip_confirmation, type: Boolean, default: false, desc: 'Flag indicating the account is confirmed'
optional :external, type: Boolean, desc: 'Flag indicating the user is an external user'
all_or_none_of :extern_uid, :provider
+
+ # EE
+ optional :namespace_attributes, type: Hash do
+ optional :shared_runners_minutes_limit, type: Integer, desc: 'Pipeline minutes quota for this user'
+ end
end
end
@@ -137,10 +142,8 @@ def find_user(params)
optional :name, type: String, desc: 'The name of the user'
optional :username, type: String, desc: 'The username of the user'
use :optional_attributes
- at_least_one_of :email, :password, :name, :username, :skype, :linkedin,
- :twitter, :website_url, :organization, :projects_limit,
- :extern_uid, :provider, :bio, :location, :admin,
- :can_create_group, :confirm, :external
+
+ at_least_one_of(*@declared_params.flatten - [:id])
end
put ":id" do
authenticated_as_admin!
@@ -172,6 +175,10 @@ def find_user(params)
user_params[:password_expires_at] = Time.now if user_params[:password].present?
+ # EE
+ namespace_attributes = user_params[:namespace_attributes]
+ namespace_attributes[:id] = user.namespace_id if namespace_attributes
+
if user.update_attributes(user_params.except(:extern_uid, :provider))
present user, with: Entities::UserPublic
else
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
index e3ed4c3232d3c..3b8cebf89c543 100644
--- a/spec/requests/api/groups_spec.rb
+++ b/spec/requests/api/groups_spec.rb
@@ -272,6 +272,14 @@
expect(response).to have_http_status(404)
end
+
+ # EE
+ it 'updates the group for shared_runners_minutes_limit' do
+ put api("/groups/#{group1.id}", user1), shared_runners_minutes_limit: 133
+
+ expect(response).to have_http_status(200)
+ expect(json_response['shared_runners_minutes_limit']).to eq(133)
+ end
end
context 'when authenticated as the admin' do
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 758c61f50e7f2..a0bc0ae2c1262 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -425,6 +425,15 @@
expect(user.reload.external?).to be_truthy
end
+ it "updates shared_runners_minutes_limit" do
+ put api("/users/#{user.id}", admin), { namespace_attributes: { shared_runners_minutes_limit: 133 } }
+
+ expect(response).to have_http_status(200)
+ expect(json_response.dig('namespace', 'shared_runners_minutes_limit'))
+ .to eq(133)
+ expect(user.reload.namespace.shared_runners_minutes_limit).to eq(133)
+ end
+
it "does not update admin status" do
put api("/users/#{admin_user.id}", admin), { can_create_group: false }
expect(response).to have_http_status(200)
--
GitLab
From 6a0a4dfea2bf9c1d55a8409f2bf4a85d0f30ed14 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 23 May 2017 22:58:13 +0800
Subject: [PATCH 02/13] Use delegate so that we set
shared_runners_minutes_limit
from the user directly rather than from the namespace.
---
app/models/user.rb | 4 +++-
lib/api/entities.rb | 5 -----
lib/api/users.rb | 8 +-------
spec/requests/api/users_spec.rb | 4 ++--
4 files changed, 6 insertions(+), 15 deletions(-)
diff --git a/app/models/user.rb b/app/models/user.rb
index f2f3e6460e065..557252b2499a3 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -63,7 +63,7 @@ def update_tracked_fields!(request)
#
# Namespace for personal projects
- has_one :namespace, -> { where type: nil }, dependent: :destroy, foreign_key: :owner_id
+ has_one :namespace, -> { where type: nil }, dependent: :destroy, foreign_key: :owner_id, autosave: true
# Profile
has_many :keys, -> do
@@ -182,6 +182,8 @@ def update_tracked_fields!(request)
alias_attribute :private_token, :authentication_token
delegate :path, to: :namespace, allow_nil: true, prefix: true
+ delegate :shared_runners_minutes_limit, :shared_runners_minutes_limit=,
+ to: :namespace
accepts_nested_attributes_for :namespace
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 699bfbdd2bc56..0301447b8abac 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -40,11 +40,6 @@ class UserPublic < User
expose :external
# EE-only
- expose :namespace, using: 'API::Entities::UserNamespace'
- end
-
- # EE-only
- class UserNamespace < Grape::Entity
expose :shared_runners_minutes_limit
end
diff --git a/lib/api/users.rb b/lib/api/users.rb
index f83901a87dec0..055c1e85cda6a 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -32,9 +32,7 @@ def find_user(params)
all_or_none_of :extern_uid, :provider
# EE
- optional :namespace_attributes, type: Hash do
- optional :shared_runners_minutes_limit, type: Integer, desc: 'Pipeline minutes quota for this user'
- end
+ optional :shared_runners_minutes_limit, type: Integer, desc: 'Pipeline minutes quota for this user'
end
end
@@ -175,10 +173,6 @@ def find_user(params)
user_params[:password_expires_at] = Time.now if user_params[:password].present?
- # EE
- namespace_attributes = user_params[:namespace_attributes]
- namespace_attributes[:id] = user.namespace_id if namespace_attributes
-
if user.update_attributes(user_params.except(:extern_uid, :provider))
present user, with: Entities::UserPublic
else
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index a0bc0ae2c1262..ce2b608106253 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -426,10 +426,10 @@
end
it "updates shared_runners_minutes_limit" do
- put api("/users/#{user.id}", admin), { namespace_attributes: { shared_runners_minutes_limit: 133 } }
+ put api("/users/#{user.id}", admin), { shared_runners_minutes_limit: 133 }
expect(response).to have_http_status(200)
- expect(json_response.dig('namespace', 'shared_runners_minutes_limit'))
+ expect(json_response['shared_runners_minutes_limit'])
.to eq(133)
expect(user.reload.namespace.shared_runners_minutes_limit).to eq(133)
end
--
GitLab
From 0106d2f928a4f9b61fcd1213080e89315baa8e83 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 24 May 2017 01:42:22 +0800
Subject: [PATCH 03/13] There's little point to check having at least one
argument at all. Feedback:
https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1942#note_30495480
---
lib/api/groups.rb | 2 --
lib/api/users.rb | 2 --
spec/requests/api/users_spec.rb | 2 +-
3 files changed, 1 insertion(+), 5 deletions(-)
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 92878aa3ada7d..625816483971d 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -119,8 +119,6 @@ def present_groups(groups, options = {})
optional :name, type: String, desc: 'The name of the group'
optional :path, type: String, desc: 'The path of the group'
use :optional_params
-
- at_least_one_of(*@declared_params.flatten - [:id])
end
put ':id' do
group = find_group!(params[:id])
diff --git a/lib/api/users.rb b/lib/api/users.rb
index 055c1e85cda6a..c2f6260359333 100644
--- a/lib/api/users.rb
+++ b/lib/api/users.rb
@@ -140,8 +140,6 @@ def find_user(params)
optional :name, type: String, desc: 'The name of the user'
optional :username, type: String, desc: 'The username of the user'
use :optional_attributes
-
- at_least_one_of(*@declared_params.flatten - [:id])
end
put ":id" do
authenticated_as_admin!
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index ce2b608106253..837ff983831d9 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -431,7 +431,7 @@
expect(response).to have_http_status(200)
expect(json_response['shared_runners_minutes_limit'])
.to eq(133)
- expect(user.reload.namespace.shared_runners_minutes_limit).to eq(133)
+ expect(user.reload.shared_runners_minutes_limit).to eq(133)
end
it "does not update admin status" do
--
GitLab
From 32e3955b4a1ae605d9318ab628e05a48a33773b6 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 25 May 2017 04:57:21 +0800
Subject: [PATCH 04/13] Make sure only the admin could update
shared_runners_minutes_limit
---
app/models/user.rb | 2 ++
lib/api/groups.rb | 3 +++
spec/requests/api/groups_spec.rb | 13 ++++++++++---
spec/requests/api/users_spec.rb | 13 ++++++++++---
4 files changed, 25 insertions(+), 6 deletions(-)
diff --git a/app/models/user.rb b/app/models/user.rb
index 557252b2499a3..c0b31e7b22219 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -182,6 +182,8 @@ def update_tracked_fields!(request)
alias_attribute :private_token, :authentication_token
delegate :path, to: :namespace, allow_nil: true, prefix: true
+
+ # EE-only
delegate :shared_runners_minutes_limit, :shared_runners_minutes_limit=,
to: :namespace
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 625816483971d..074789d4466da 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -124,6 +124,9 @@ def present_groups(groups, options = {})
group = find_group!(params[:id])
authorize! :admin_group, group
+ # EE
+ authenticated_as_admin! if params[:shared_runners_minutes_limit]
+
if ::Groups::UpdateService.new(group, current_user, declared_params(include_missing: false)).execute
present group, with: Entities::GroupDetail, current_user: current_user
else
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
index 3b8cebf89c543..89482500ee81a 100644
--- a/spec/requests/api/groups_spec.rb
+++ b/spec/requests/api/groups_spec.rb
@@ -274,11 +274,10 @@
end
# EE
- it 'updates the group for shared_runners_minutes_limit' do
+ it 'returns 403 for updating shared_runners_minutes_limit' do
put api("/groups/#{group1.id}", user1), shared_runners_minutes_limit: 133
- expect(response).to have_http_status(200)
- expect(json_response['shared_runners_minutes_limit']).to eq(133)
+ expect(response).to have_http_status(403)
end
end
@@ -289,6 +288,14 @@
expect(response).to have_http_status(200)
expect(json_response['name']).to eq(new_group_name)
end
+
+ # EE
+ it 'updates the group for shared_runners_minutes_limit' do
+ put api("/groups/#{group1.id}", admin), shared_runners_minutes_limit: 133
+
+ expect(response).to have_http_status(200)
+ expect(json_response['shared_runners_minutes_limit']).to eq(133)
+ end
end
context 'when authenticated as an user that can see the group' do
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index 837ff983831d9..b4cb01f1e75c5 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -447,9 +447,16 @@
expect(user.reload.email).not_to eq('invalid email')
end
- it "is not available for non admin users" do
- put api("/users/#{user.id}", user), attributes_for(:user)
- expect(response).to have_http_status(403)
+ context 'when the current user is not an admin' do
+ it "is not available" do
+ put api("/users/#{user.id}", user), attributes_for(:user)
+ expect(response).to have_http_status(403)
+ end
+
+ it "cannot update their own shared_runners_minutes_limit" do
+ put api("/users/#{user.id}", user), { shared_runners_minutes_limit: 133 }
+ expect(response).to have_http_status(403)
+ end
end
it "returns 404 for non-existing user" do
--
GitLab
From d88c4c12f7897cbc6fc090f58d9e0e80d4a13346 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 25 May 2017 05:44:34 +0800
Subject: [PATCH 05/13] Test for group creation and remove duplicated test
---
lib/api/groups.rb | 3 +++
spec/requests/api/groups_spec.rb | 38 +++++++++++++++++++-------------
2 files changed, 26 insertions(+), 15 deletions(-)
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 074789d4466da..b3f0ac6c34e54 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -90,6 +90,9 @@ def present_groups(groups, options = {})
group_access: params.delete(:ldap_access)
}
+ # EE
+ authenticated_as_admin! if params[:shared_runners_minutes_limit]
+
group = ::Groups::CreateService.new(current_user, declared_params(include_missing: false)).execute
if group.persisted?
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
index 89482500ee81a..0779d161959f9 100644
--- a/spec/requests/api/groups_spec.rb
+++ b/spec/requests/api/groups_spec.rb
@@ -494,23 +494,31 @@
group_attributes = attributes_for(:group, ldap_cn: 'ldap-group', ldap_access: Gitlab::Access::DEVELOPER)
expect { post api("/groups", admin), group_attributes }.to change{ LdapGroupLink.count }.by(1)
end
- end
- end
- describe "PUT /groups" do
- context "when authenticated as user without group permissions" do
- it "does not create group" do
- put api("/groups/#{group2.id}", user1), attributes_for(:group)
- expect(response.status).to eq(404)
- end
- end
+ # EE
+ context 'when shared_runners_minutes_limit is given' do
+ context 'when the current user is not an admin' do
+ it "does not create a group with shared_runners_minutes_limit" do
+ group = attributes_for(:group, { shared_runners_minutes_limit: 133 })
- context "when authenticated as user with group permissions" do
- it "updates group" do
- group2.update(owner: user2)
- put api("/groups/#{group2.id}", user2), { name: 'Renamed' }
- expect(response.status).to eq(200)
- expect(group2.reload.name).to eq('Renamed')
+ post api("/groups", user3), group
+
+ expect(response).to have_http_status(403)
+ end
+ end
+
+ context 'when the current user is an admin' do
+ it "creates a group with shared_runners_minutes_limit" do
+ group = attributes_for(:group, { shared_runners_minutes_limit: 133 })
+
+ post api("/groups", admin), group
+
+ created_group = Group.find(json_response['id'])
+
+ expect(response).to have_http_status(201)
+ expect(created_group.shared_runners_minutes_limit).to eq(133)
+ end
+ end
end
end
end
--
GitLab
From 308aa151c12601b764daf2b5088cdd540805c1cd Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 25 May 2017 05:44:59 +0800
Subject: [PATCH 06/13] Update doc and add changelog entry
---
.../unreleased-ee/add-api-shared_runners_minutes_limit.yml | 4 ++++
doc/api/groups.md | 3 +++
doc/api/users.md | 5 ++++-
lib/api/groups.rb | 2 +-
4 files changed, 12 insertions(+), 2 deletions(-)
create mode 100644 changelogs/unreleased-ee/add-api-shared_runners_minutes_limit.yml
diff --git a/changelogs/unreleased-ee/add-api-shared_runners_minutes_limit.yml b/changelogs/unreleased-ee/add-api-shared_runners_minutes_limit.yml
new file mode 100644
index 0000000000000..0569c1045ef3f
--- /dev/null
+++ b/changelogs/unreleased-ee/add-api-shared_runners_minutes_limit.yml
@@ -0,0 +1,4 @@
+---
+title: Add shared_runners_minutes_limit to groups and users API
+merge_request: 1942
+author:
diff --git a/doc/api/groups.md b/doc/api/groups.md
index 7b772a99bb50e..b5bc627344f1d 100644
--- a/doc/api/groups.md
+++ b/doc/api/groups.md
@@ -140,6 +140,7 @@ Example response:
"full_name": "Twitter",
"full_path": "twitter",
"parent_id": null,
+ "shared_runners_minutes_limit": 133,
"projects": [
{
"id": 7,
@@ -290,6 +291,7 @@ Parameters:
- `lfs_enabled` (optional) - Enable/disable Large File Storage (LFS) for the projects in this group
- `request_access_enabled` (optional) - Allow users to request member access.
- `parent_id` (optional) - The parent group id for creating nested group.
+- `shared_runners_minutes_limit` (optional) - (admin-only) Pipeline minutes quota for this group
## Transfer project to group
@@ -323,6 +325,7 @@ PUT /groups/:id
| `visibility` | string | no | The visibility level of the group. Can be `private`, `internal`, or `public`. |
| `lfs_enabled` (optional) | boolean | no | Enable/disable Large File Storage (LFS) for the projects in this group |
| `request_access_enabled` | boolean | no | Allow users to request member access. |
+| `shared_runners_minutes_limit` | integer | no | (admin-only) Pipeline minutes quota for this group |
```bash
curl --request PUT --header "PRIVATE-TOKEN: 9koXpg98eAheJpvBs5tK" "https://gitlab.example.com/api/v4/groups/5?name=Experimental"
diff --git a/doc/api/users.md b/doc/api/users.md
index 8b57f8c0aaeaa..76ce729c399d4 100644
--- a/doc/api/users.md
+++ b/doc/api/users.md
@@ -220,7 +220,8 @@ Parameters:
"can_create_group": true,
"can_create_project": true,
"two_factor_enabled": true,
- "external": false
+ "external": false,
+ "shared_runners_minutes_limit": 133
}
```
@@ -253,6 +254,7 @@ Parameters:
- `can_create_group` (optional) - User can create groups - true or false
- `confirm` (optional) - Require confirmation - true (default) or false
- `external` (optional) - Flags the user as external - true or false(default)
+- `shared_runners_minutes_limit` (optional) - Pipeline minutes quota for this user
## User modification
@@ -281,6 +283,7 @@ Parameters:
- `admin` (optional) - User is admin - true or false (default)
- `can_create_group` (optional) - User can create groups - true or false
- `external` (optional) - Flags the user as external - true or false(default)
+- `shared_runners_minutes_limit` (optional) - Pipeline minutes quota for this user
On password update, user will be forced to change it upon next login.
Note, at the moment this method does only return a `404` error,
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index b3f0ac6c34e54..4be02c4fbb824 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -17,7 +17,7 @@ class Groups < Grape::API
optional :membership_lock, type: Boolean, desc: 'Prevent adding new members to project membership within this group'
optional :ldap_cn, type: String, desc: 'LDAP Common Name'
optional :ldap_access, type: Integer, desc: 'A valid access level'
- optional :shared_runners_minutes_limit, type: Integer, desc: 'Pipeline minutes quota for this group'
+ optional :shared_runners_minutes_limit, type: Integer, desc: '(admin-only) Pipeline minutes quota for this group'
all_or_none_of :ldap_cn, :ldap_access
end
--
GitLab
From 46af4ac6ee4f86eab917561b0cace21b025e33a8 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 25 May 2017 21:13:48 +0800
Subject: [PATCH 07/13] Use GroupDetail for POST /groups
Feedback:
https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1942#note_30680817
---
lib/api/groups.rb | 2 +-
spec/requests/api/groups_spec.rb | 4 +---
2 files changed, 2 insertions(+), 4 deletions(-)
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 4be02c4fbb824..9f597ff316953 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -104,7 +104,7 @@ def present_groups(groups, options = {})
)
end
- present group, with: Entities::Group, current_user: current_user
+ present group, with: Entities::GroupDetail, current_user: current_user
else
render_api_error!("Failed to save group #{group.errors.messages}", 400)
end
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
index 355bf287750a0..f5e580684f555 100644
--- a/spec/requests/api/groups_spec.rb
+++ b/spec/requests/api/groups_spec.rb
@@ -513,10 +513,8 @@
post api("/groups", admin), group
- created_group = Group.find(json_response['id'])
-
expect(response).to have_http_status(201)
- expect(created_group.shared_runners_minutes_limit).to eq(133)
+ expect(json_response['shared_runners_minutes_limit']).to eq(133)
end
end
end
--
GitLab
From 631f7026fc0134ae1e6882df429162d23d4bd992 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 26 May 2017 18:11:23 +0800
Subject: [PATCH 08/13] Don't check for admin permission if we're not
going to change shared_runners_minutes_limit
Feedback:
https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1942/diffs#note_30680494
---
lib/api/groups.rb | 5 ++++-
spec/requests/api/groups_spec.rb | 8 ++++++++
2 files changed, 12 insertions(+), 1 deletion(-)
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 9f597ff316953..722f3eb609f55 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -128,7 +128,10 @@ def present_groups(groups, options = {})
authorize! :admin_group, group
# EE
- authenticated_as_admin! if params[:shared_runners_minutes_limit]
+ if params[:shared_runners_minutes_limit].to_i !=
+ group.shared_runners_minutes_limit.to_i
+ authenticated_as_admin!
+ end
if ::Groups::UpdateService.new(group, current_user, declared_params(include_missing: false)).execute
present group, with: Entities::GroupDetail, current_user: current_user
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
index f5e580684f555..6b03d72a02c24 100644
--- a/spec/requests/api/groups_spec.rb
+++ b/spec/requests/api/groups_spec.rb
@@ -279,6 +279,14 @@
expect(response).to have_http_status(403)
end
+
+ it 'returns 200 if shared_runners_minutes_limit is not changing' do
+ group1.update(shared_runners_minutes_limit: 133)
+
+ put api("/groups/#{group1.id}", user1), shared_runners_minutes_limit: 133
+
+ expect(response).to have_http_status(200)
+ end
end
context 'when authenticated as the admin' do
--
GitLab
From 14af431a07b87f9bc798d9e98ce18325b82407c2 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 1 Jun 2017 02:43:26 +0800
Subject: [PATCH 09/13] Add 3 delegation tests for user
Feedback:
https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1942/diffs#note_30950569
---
spec/models/user_spec.rb | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 61c239f305105..53343606b6a82 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -13,6 +13,14 @@
it { is_expected.to include_module(TokenAuthenticatable) }
end
+ describe 'delegations' do
+ it { is_expected.to delegate_method(:path).to(:namespace).with_prefix }
+
+ # EE
+ it { is_expected.to delegate_method(:shared_runners_minutes_limit).to(:namespace) }
+ it { is_expected.to delegate_method(:shared_runners_minutes_limit=).to(:namespace).with_arguments(133) }
+ end
+
describe 'associations' do
it { is_expected.to have_one(:namespace) }
it { is_expected.to have_many(:snippets).dependent(:destroy) }
--
GitLab
From 0ed083662df7d6b6bec97424e1558d48a3444e6d Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Thu, 1 Jun 2017 03:13:13 +0800
Subject: [PATCH 10/13] Move EE specific delegation to EE::User
---
app/models/ee/user.rb | 3 +++
app/models/user.rb | 4 ----
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/app/models/ee/user.rb b/app/models/ee/user.rb
index cd578d2905d75..cf96291a0d28b 100644
--- a/app/models/ee/user.rb
+++ b/app/models/ee/user.rb
@@ -15,6 +15,9 @@ module User
# column directly.
validate :auditor_requires_license_add_on, if: :auditor
validate :cannot_be_admin_and_auditor
+
+ delegate :shared_runners_minutes_limit, :shared_runners_minutes_limit=,
+ to: :namespace
end
module ClassMethods
diff --git a/app/models/user.rb b/app/models/user.rb
index dba16faf1bdc6..9bb594446ef54 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -190,10 +190,6 @@ def update_tracked_fields!(request)
delegate :path, to: :namespace, allow_nil: true, prefix: true
- # EE-only
- delegate :shared_runners_minutes_limit, :shared_runners_minutes_limit=,
- to: :namespace
-
accepts_nested_attributes_for :namespace
state_machine :state, initial: :active do
--
GitLab
From eaa4bfa7aa9aa317a4497b4e926fd1e4184b210e Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Sat, 3 Jun 2017 03:21:13 +0800
Subject: [PATCH 11/13] Fix updating shared_runners_minutes_limit; Check
effects
made by the API. TODO: Need to do this for CE and all
the API tests as well
---
lib/api/groups.rb | 10 +++++++---
spec/requests/api/groups_spec.rb | 24 +++++++++++++++++++-----
spec/requests/api/users_spec.rb | 20 ++++++++++++++------
3 files changed, 40 insertions(+), 14 deletions(-)
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 722f3eb609f55..b439646683f3a 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -128,9 +128,13 @@ def present_groups(groups, options = {})
authorize! :admin_group, group
# EE
- if params[:shared_runners_minutes_limit].to_i !=
- group.shared_runners_minutes_limit.to_i
- authenticated_as_admin!
+ if params[:shared_runners_minutes_limit]
+ group.shared_runners_minutes_limit =
+ params[:shared_runners_minutes_limit].to_i
+
+ if group.shared_runners_minutes_limit_changed?
+ authenticated_as_admin!
+ end
end
if ::Groups::UpdateService.new(group, current_user, declared_params(include_missing: false)).execute
diff --git a/spec/requests/api/groups_spec.rb b/spec/requests/api/groups_spec.rb
index 6b03d72a02c24..5204844c55070 100644
--- a/spec/requests/api/groups_spec.rb
+++ b/spec/requests/api/groups_spec.rb
@@ -275,7 +275,9 @@
# EE
it 'returns 403 for updating shared_runners_minutes_limit' do
- put api("/groups/#{group1.id}", user1), shared_runners_minutes_limit: 133
+ expect do
+ put api("/groups/#{group1.id}", user1), shared_runners_minutes_limit: 133
+ end.not_to change { group1.shared_runners_minutes_limit }
expect(response).to have_http_status(403)
end
@@ -283,7 +285,9 @@
it 'returns 200 if shared_runners_minutes_limit is not changing' do
group1.update(shared_runners_minutes_limit: 133)
- put api("/groups/#{group1.id}", user1), shared_runners_minutes_limit: 133
+ expect do
+ put api("/groups/#{group1.id}", user1), shared_runners_minutes_limit: 133
+ end.not_to change { group1.shared_runners_minutes_limit }
expect(response).to have_http_status(200)
end
@@ -299,7 +303,10 @@
# EE
it 'updates the group for shared_runners_minutes_limit' do
- put api("/groups/#{group1.id}", admin), shared_runners_minutes_limit: 133
+ expect do
+ put api("/groups/#{group1.id}", admin), shared_runners_minutes_limit: 133
+ end.to change { group1.reload.shared_runners_minutes_limit }
+ .from(nil).to(133)
expect(response).to have_http_status(200)
expect(json_response['shared_runners_minutes_limit']).to eq(133)
@@ -509,7 +516,9 @@
it "does not create a group with shared_runners_minutes_limit" do
group = attributes_for(:group, { shared_runners_minutes_limit: 133 })
- post api("/groups", user3), group
+ expect do
+ post api("/groups", user3), group
+ end.not_to change { Group.count }
expect(response).to have_http_status(403)
end
@@ -519,8 +528,13 @@
it "creates a group with shared_runners_minutes_limit" do
group = attributes_for(:group, { shared_runners_minutes_limit: 133 })
- post api("/groups", admin), group
+ expect do
+ post api("/groups", admin), group
+ end.to change { Group.count }.by(1)
+ created_group = Group.find(json_response['id'])
+
+ expect(created_group.shared_runners_minutes_limit).to eq(133)
expect(response).to have_http_status(201)
expect(json_response['shared_runners_minutes_limit']).to eq(133)
end
diff --git a/spec/requests/api/users_spec.rb b/spec/requests/api/users_spec.rb
index c13d621512a0b..f4699da829bb7 100644
--- a/spec/requests/api/users_spec.rb
+++ b/spec/requests/api/users_spec.rb
@@ -425,13 +425,15 @@
expect(user.reload.external?).to be_truthy
end
+ # EE
it "updates shared_runners_minutes_limit" do
- put api("/users/#{user.id}", admin), { shared_runners_minutes_limit: 133 }
+ expect do
+ put api("/users/#{user.id}", admin), { shared_runners_minutes_limit: 133 }
+ end.to change { user.reload.shared_runners_minutes_limit }
+ .from(nil).to(133)
expect(response).to have_http_status(200)
- expect(json_response['shared_runners_minutes_limit'])
- .to eq(133)
- expect(user.reload.shared_runners_minutes_limit).to eq(133)
+ expect(json_response['shared_runners_minutes_limit']).to eq(133)
end
it "does not update admin status" do
@@ -449,12 +451,18 @@
context 'when the current user is not an admin' do
it "is not available" do
- put api("/users/#{user.id}", user), attributes_for(:user)
+ expect do
+ put api("/users/#{user.id}", user), attributes_for(:user)
+ end.not_to change { user.reload.attributes }
+
expect(response).to have_http_status(403)
end
it "cannot update their own shared_runners_minutes_limit" do
- put api("/users/#{user.id}", user), { shared_runners_minutes_limit: 133 }
+ expect do
+ put api("/users/#{user.id}", user), { shared_runners_minutes_limit: 133 }
+ end.not_to change { user.reload.shared_runners_minutes_limit }
+
expect(response).to have_http_status(403)
end
end
--
GitLab
From 9a44c9a6bed95a62dda3236e23615eec3d521987 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Mon, 5 Jun 2017 16:52:57 +0800
Subject: [PATCH 12/13] Just check the value, simple and stupid
Feedback:
https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1942#note_31511130
---
lib/api/groups.rb | 11 ++++-------
1 file changed, 4 insertions(+), 7 deletions(-)
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index b439646683f3a..5cdfaeb435308 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -128,13 +128,10 @@ def present_groups(groups, options = {})
authorize! :admin_group, group
# EE
- if params[:shared_runners_minutes_limit]
- group.shared_runners_minutes_limit =
- params[:shared_runners_minutes_limit].to_i
-
- if group.shared_runners_minutes_limit_changed?
- authenticated_as_admin!
- end
+ if params[:shared_runners_minutes_limit].present? &&
+ group.shared_runners_minutes_limit.to_i !=
+ params[:shared_runners_minutes_limit].to_i
+ authenticated_as_admin!
end
if ::Groups::UpdateService.new(group, current_user, declared_params(include_missing: false)).execute
--
GitLab
From c14bc19d11ba8dc034e9428652d664a87f782255 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Mon, 5 Jun 2017 17:39:38 +0800
Subject: [PATCH 13/13] I think Rubocop Style/MultilineOperationIndentation is
broken
---
lib/api/groups.rb | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 5cdfaeb435308..4b3abc7a752c8 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -130,7 +130,7 @@ def present_groups(groups, options = {})
# EE
if params[:shared_runners_minutes_limit].present? &&
group.shared_runners_minutes_limit.to_i !=
- params[:shared_runners_minutes_limit].to_i
+ params[:shared_runners_minutes_limit].to_i
authenticated_as_admin!
end
--
GitLab