From 396f85e438ddc9bcd89f5a557980ce82b71e098b Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Thu, 18 Aug 2016 17:01:50 +0100 Subject: [PATCH] Add expiration date to group memberships --- app/assets/javascripts/dispatcher.js | 1 + .../groups/group_members_controller.rb | 9 +++++-- app/models/group.rb | 24 ++++++++++++------- app/models/project.rb | 4 ++-- app/models/project_team.rb | 6 ++--- .../group_members/_new_group_member.html.haml | 7 ++++++ app/views/groups/group_members/update.js.haml | 1 + app/views/shared/members/_member.html.haml | 13 +++++----- features/steps/group/members.rb | 2 +- features/steps/project/team_management.rb | 2 +- lib/api/entities.rb | 7 +++--- lib/api/members.rb | 8 ++++--- ...r_adds_member_with_expiration_date_spec.rb | 2 +- spec/requests/api/members_spec.rb | 6 +++-- 14 files changed, 58 insertions(+), 34 deletions(-) diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index 0bdab796daf..f385fe24d0b 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -126,6 +126,7 @@ new NotificationsDropdown(); break; case 'groups:group_members:index': + new MemberExpirationDate(); new GroupMembers(); new UsersSelect(); break; diff --git a/app/controllers/groups/group_members_controller.rb b/app/controllers/groups/group_members_controller.rb index 9fc41a12536..272164cd0cc 100644 --- a/app/controllers/groups/group_members_controller.rb +++ b/app/controllers/groups/group_members_controller.rb @@ -21,7 +21,12 @@ class Groups::GroupMembersController < Groups::ApplicationController end def create - @group.add_users(params[:user_ids].split(','), params[:access_level], current_user) + @group.add_users( + params[:user_ids].split(','), + params[:access_level], + current_user: current_user, + expires_at: params[:expires_at] + ) redirect_to group_group_members_path(@group), notice: 'Users were successfully added.' end @@ -63,7 +68,7 @@ class Groups::GroupMembersController < Groups::ApplicationController protected def member_params - params.require(:group_member).permit(:access_level, :user_id) + params.require(:group_member).permit(:access_level, :user_id, :expires_at) end # MembershipActions concern diff --git a/app/models/group.rb b/app/models/group.rb index 11c39bbdfe4..c48869ae465 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -95,34 +95,40 @@ class Group < Namespace end end - def add_users(user_ids, access_level, current_user = nil) + def add_users(user_ids, access_level, current_user: nil, expires_at: nil) user_ids.each do |user_id| - Member.add_user(self.group_members, user_id, access_level, current_user: current_user) + Member.add_user( + self.group_members, + user_id, + access_level, + current_user: current_user, + expires_at: expires_at + ) end end - def add_user(user, access_level, current_user = nil) - add_users([user], access_level, current_user) + def add_user(user, access_level, current_user: nil, expires_at: nil) + add_users([user], access_level, current_user: current_user, expires_at: expires_at) end def add_guest(user, current_user = nil) - add_user(user, Gitlab::Access::GUEST, current_user) + add_user(user, Gitlab::Access::GUEST, current_user: current_user) end def add_reporter(user, current_user = nil) - add_user(user, Gitlab::Access::REPORTER, current_user) + add_user(user, Gitlab::Access::REPORTER, current_user: current_user) end def add_developer(user, current_user = nil) - add_user(user, Gitlab::Access::DEVELOPER, current_user) + add_user(user, Gitlab::Access::DEVELOPER, current_user: current_user) end def add_master(user, current_user = nil) - add_user(user, Gitlab::Access::MASTER, current_user) + add_user(user, Gitlab::Access::MASTER, current_user: current_user) end def add_owner(user, current_user = nil) - add_user(user, Gitlab::Access::OWNER, current_user) + add_user(user, Gitlab::Access::OWNER, current_user: current_user) end def has_owner?(user) diff --git a/app/models/project.rb b/app/models/project.rb index 043da030468..f9c48a546e6 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1003,8 +1003,8 @@ class Project < ActiveRecord::Base project_members.find_by(user_id: user) end - def add_user(user, access_level, current_user = nil) - team.add_user(user, access_level, current_user) + def add_user(user, access_level, current_user: nil, expires_at: nil) + team.add_user(user, access_level, current_user: current_user, expires_at: expires_at) end def default_branch diff --git a/app/models/project_team.rb b/app/models/project_team.rb index a65d271d262..ab6ea2aae36 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -17,7 +17,7 @@ class ProjectTeam if users.respond_to?(:each) add_users(users, access, current_user: current_user) else - add_user(users, access, current_user) + add_user(users, access, current_user: current_user) end end @@ -43,8 +43,8 @@ class ProjectTeam ) end - def add_user(user, access, current_user = nil) - add_users([user], access, current_user: current_user) + def add_user(user, access, current_user: nil, expires_at: nil) + add_users([user], access, current_user: current_user, expires_at: expires_at) end # Remove all users from project team diff --git a/app/views/groups/group_members/_new_group_member.html.haml b/app/views/groups/group_members/_new_group_member.html.haml index 9bb9f962177..33228f31786 100644 --- a/app/views/groups/group_members/_new_group_member.html.haml +++ b/app/views/groups/group_members/_new_group_member.html.haml @@ -14,5 +14,12 @@ Read more about role permissions %strong= link_to "here", help_page_path("user/permissions"), class: "vlink" + .form-group + = f.label :expires_at, 'Access expiration date', class: 'control-label' + .col-sm-10 + .clearable-input + = text_field_tag :expires_at, nil, class: 'form-control js-access-expiration-date', placeholder: 'Select access expiration date' + %i.clear-icon.js-clear-input + .form-actions = f.submit 'Add users to group', class: "btn btn-create" diff --git a/app/views/groups/group_members/update.js.haml b/app/views/groups/group_members/update.js.haml index da71de4cd1e..742f9d7a433 100644 --- a/app/views/groups/group_members/update.js.haml +++ b/app/views/groups/group_members/update.js.haml @@ -1,2 +1,3 @@ :plain $("##{dom_id(@group_member)}").replaceWith('#{escape_javascript(render('shared/members/member', member: @group_member))}'); + new MemberExpirationDate(); diff --git a/app/views/shared/members/_member.html.haml b/app/views/shared/members/_member.html.haml index 2adbf9277fa..09f53268747 100644 --- a/app/views/shared/members/_member.html.haml +++ b/app/views/shared/members/_member.html.haml @@ -82,12 +82,11 @@ = label_tag "member_access_level_#{member.id}", 'Project access', class: 'control-label' .col-sm-10 = f.select :access_level, options_for_select(member.class.access_level_roles, member.access_level), {}, class: 'form-control', id: "member_access_level_#{member.id}" - - if member.is_a?(ProjectMember) - .form-group - = label_tag "member_expires_at_#{member.id}", 'Access expiration date', class: 'control-label' - .col-sm-10 - .clearable-input - = f.text_field :expires_at, class: 'form-control js-access-expiration-date', placeholder: 'Select access expiration date', id: "member_expires_at_#{member.id}" - %i.clear-icon.js-clear-input + .form-group + = label_tag "member_expires_at_#{member.id}", 'Access expiration date', class: 'control-label' + .col-sm-10 + .clearable-input + = f.text_field :expires_at, class: 'form-control js-access-expiration-date', placeholder: 'Select access expiration date', id: "member_expires_at_#{member.id}" + %i.clear-icon.js-clear-input .prepend-top-10 = f.submit 'Save', class: 'btn btn-save btn-sm' diff --git a/features/steps/group/members.rb b/features/steps/group/members.rb index 956a0026e1a..e9b45823c67 100644 --- a/features/steps/group/members.rb +++ b/features/steps/group/members.rb @@ -117,7 +117,7 @@ class Spinach::Features::GroupMembers < Spinach::FeatureSteps page.within "#group_member_#{member.id}" do click_button 'Edit' - select 'Developer', from: 'group_member_access_level' + select 'Developer', from: "member_access_level_#{member.id}" click_on 'Save' end end diff --git a/features/steps/project/team_management.rb b/features/steps/project/team_management.rb index 334ce7dd3db..e920f5a706b 100644 --- a/features/steps/project/team_management.rb +++ b/features/steps/project/team_management.rb @@ -66,7 +66,7 @@ class Spinach::Features::ProjectTeamManagement < Spinach::FeatureSteps project_member = project.project_members.find_by(user_id: user.id) page.within "#project_member_#{project_member.id}" do click_button 'Edit' - select "Reporter", from: "project_member_access_level" + select "Reporter", from: "member_access_level_#{project_member.id}" click_button "Save" end end diff --git a/lib/api/entities.rb b/lib/api/entities.rb index dd9d8515853..64ee511bbd7 100644 --- a/lib/api/entities.rb +++ b/lib/api/entities.rb @@ -97,6 +97,10 @@ module API member = options[:member] || options[:members].find { |m| m.user_id == user.id } member.access_level end + expose :expires_at do |user, options| + member = options[:member] || options[:members].find { |m| m.user_id == user.id } + member.expires_at + end end class AccessRequester < UserBasic @@ -104,9 +108,6 @@ module API access_requester = options[:access_requester] || options[:access_requesters].find { |m| m.user_id == user.id } access_requester.requested_at end - expose :expires_at do |user, options| - options[:project].project_members.find_by(user_id: user.id).expires_at - end end class Group < Grape::Entity diff --git a/lib/api/members.rb b/lib/api/members.rb index a45285b9c3d..94c16710d9a 100644 --- a/lib/api/members.rb +++ b/lib/api/members.rb @@ -49,6 +49,7 @@ module API # id (required) - The group/project ID # user_id (required) - The user ID of the new member # access_level (required) - A valid access level + # expires_at (optional) - Date string in the format YEAR-MONTH-DAY # # Example Request: # POST /groups/:id/members @@ -72,7 +73,7 @@ module API conflict!('Member already exists') if source_type == 'group' && member unless member - source.add_user(params[:user_id], params[:access_level], current_user) + source.add_user(params[:user_id], params[:access_level], current_user: current_user, expires_at: params[:expires_at]) member = source.members.find_by(user_id: params[:user_id]) end @@ -81,7 +82,7 @@ module API else # Since `source.add_user` doesn't return a member object, we have to # build a new one and populate its errors in order to render them. - member = source.members.build(attributes_for_keys([:user_id, :access_level])) + member = source.members.build(attributes_for_keys([:user_id, :access_level, :expires_at])) member.valid? # populate the errors # This is to ensure back-compatibility but 400 behavior should be used @@ -97,6 +98,7 @@ module API # id (required) - The group/project ID # user_id (required) - The user ID of the member # access_level (required) - A valid access level + # expires_at (optional) - Date string in the format YEAR-MONTH-DAY # # Example Request: # PUT /groups/:id/members/:user_id @@ -107,7 +109,7 @@ module API required_attributes! [:user_id, :access_level] member = source.members.find_by!(user_id: params[:user_id]) - attrs = attributes_for_keys [:access_level] + attrs = attributes_for_keys [:access_level, :expires_at] if member.update_attributes(attrs) present member.user, with: Entities::Member, member: member diff --git a/spec/features/projects/members/master_adds_member_with_expiration_date_spec.rb b/spec/features/projects/members/master_adds_member_with_expiration_date_spec.rb index 5dd38de34d2..430c384ac2e 100644 --- a/spec/features/projects/members/master_adds_member_with_expiration_date_spec.rb +++ b/spec/features/projects/members/master_adds_member_with_expiration_date_spec.rb @@ -19,7 +19,7 @@ feature 'Projects > Members > Master adds member with expiration date', feature: page.within '.users-project-form' do select2(new_member.id, from: '#user_ids', multiple: true) - fill_in 'Access expiration date', with: '2016-08-10' + fill_in 'expires_at', with: '2016-08-10' click_on 'Add users to project' end diff --git a/spec/requests/api/members_spec.rb b/spec/requests/api/members_spec.rb index a56ee30f7b1..1e365bf353a 100644 --- a/spec/requests/api/members_spec.rb +++ b/spec/requests/api/members_spec.rb @@ -122,12 +122,13 @@ describe API::Members, api: true do it 'creates a new member' do expect do post api("/#{source_type.pluralize}/#{source.id}/members", master), - user_id: stranger.id, access_level: Member::DEVELOPER + user_id: stranger.id, access_level: Member::DEVELOPER, expires_at: '2016-08-05' expect(response).to have_http_status(201) end.to change { source.members.count }.by(1) expect(json_response['id']).to eq(stranger.id) expect(json_response['access_level']).to eq(Member::DEVELOPER) + expect(json_response['expires_at']).to eq('2016-08-05') end end @@ -183,11 +184,12 @@ describe API::Members, api: true do context 'when authenticated as a master/owner' do it 'updates the member' do put api("/#{source_type.pluralize}/#{source.id}/members/#{developer.id}", master), - access_level: Member::MASTER + access_level: Member::MASTER, expires_at: '2016-08-05' expect(response).to have_http_status(200) expect(json_response['id']).to eq(developer.id) expect(json_response['access_level']).to eq(Member::MASTER) + expect(json_response['expires_at']).to eq('2016-08-05') end end