From 451e25532ff43de8151b71ced8246f709c08bf92 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Tue, 20 Jun 2017 21:32:49 +0200 Subject: [PATCH] Make MergeRequest respond to assignee_ids & assignee_ids= To make it simpler to assign users to an Issuable, make MergeRequest support the attribute `assignee_ids`. --- app/models/concerns/issuable.rb | 6 +--- app/models/merge_request.rb | 10 ++++++- .../quick_actions/interpret_service.rb | 29 ++++++++----------- spec/models/merge_request_spec.rb | 16 ++++++++++ .../quick_actions/interpret_service_spec.rb | 18 ++++++------ 5 files changed, 47 insertions(+), 32 deletions(-) diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 0a476efdaa9..1bebd55a089 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -108,11 +108,7 @@ module Issuable end def has_multiple_assignees? - supports_multiple_assignees? && assignees.count > 1 - end - - def supports_multiple_assignees? - respond_to?(:assignee_ids) + assignees.count > 1 end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index ea22ab53587..77da8413904 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -197,11 +197,19 @@ class MergeRequest < ActiveRecord::Base } end - # This method is needed for compatibility with issues to not mess view and other code + # These method are needed for compatibility with issues to not mess view and other code def assignees Array(assignee) end + def assignee_ids + Array(assignee_id) + end + + def assignee_ids=(ids) + write_attribute(:assignee_id, ids.last) + end + def assignee_or_author?(user) author_id == user.id || assignee_id == user.id end diff --git a/app/services/quick_actions/interpret_service.rb b/app/services/quick_actions/interpret_service.rb index 4ceabaf021e..df13976fb3b 100644 --- a/app/services/quick_actions/interpret_service.rb +++ b/app/services/quick_actions/interpret_service.rb @@ -107,13 +107,12 @@ module QuickActions command :assign do |users| next if users.empty? - if issuable.allows_multiple_assignees? - @updates[:assignee_ids] = issuable.assignees.pluck(:id) + users.map(&:id) - elsif issuable.supports_multiple_assignees? - @updates[:assignee_ids] = [users.last.id] - else - @updates[:assignee_id] = users.last.id - end + @updates[:assignee_ids] = + if issuable.allows_multiple_assignees? + issuable.assignees.pluck(:id) + users.map(&:id) + else + [users.last.id] + end end desc do @@ -138,16 +137,12 @@ module QuickActions # When multiple users are assigned, all will be unassigned if multiple assignees are no longer allowed users = extract_users(unassign_param) if issuable.allows_multiple_assignees? - if issuable.supports_multiple_assignees? - @updates[:assignee_ids] = - if users&.any? - issuable.assignees.pluck(:id) - users.map(&:id) - else - [] - end - else - @updates[:assignee_id] = nil - end + @updates[:assignee_ids] = + if users&.any? + issuable.assignees.pluck(:id) - users.map(&:id) + else + [] + end end desc do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index a56bc524a98..945189b922b 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -105,6 +105,22 @@ describe MergeRequest, models: true do end end + describe '#assignee_ids' do + it 'returns an array of the assigned user id' do + subject.assignee_id = 123 + + expect(subject.assignee_ids).to eq([123]) + end + end + + describe '#assignee_ids=' do + it 'sets assignee_id to the last id in the array' do + subject.assignee_ids = [123, 456] + + expect(subject.assignee_id).to eq(456) + end + end + describe '#assignee_or_author?' do let(:user) { create(:user) } diff --git a/spec/services/quick_actions/interpret_service_spec.rb b/spec/services/quick_actions/interpret_service_spec.rb index b1997e64557..35373675894 100644 --- a/spec/services/quick_actions/interpret_service_spec.rb +++ b/spec/services/quick_actions/interpret_service_spec.rb @@ -359,7 +359,7 @@ describe QuickActions::InterpretService, services: true do let(:content) { "/assign @#{developer.username}" } context 'Issue' do - it 'fetches assignee and populates assignee_id if content contains /assign' do + it 'fetches assignee and populates assignee_ids if content contains /assign' do _, updates = service.execute(content, issue) expect(updates[:assignee_ids]).to match_array([developer.id]) @@ -367,10 +367,10 @@ describe QuickActions::InterpretService, services: true do end context 'Merge Request' do - it 'fetches assignee and populates assignee_id if content contains /assign' do + it 'fetches assignee and populates assignee_ids if content contains /assign' do _, updates = service.execute(content, merge_request) - expect(updates).to eq(assignee_id: developer.id) + expect(updates).to eq(assignee_ids: [developer.id]) end end end @@ -383,7 +383,7 @@ describe QuickActions::InterpretService, services: true do end context 'Issue' do - it 'fetches assignee and populates assignee_id if content contains /assign' do + it 'fetches assignee and populates assignee_ids if content contains /assign' do _, updates = service.execute(content, issue) expect(updates[:assignee_ids]).to match_array([developer.id]) @@ -391,10 +391,10 @@ describe QuickActions::InterpretService, services: true do end context 'Merge Request' do - it 'fetches assignee and populates assignee_id if content contains /assign' do + it 'fetches assignee and populates assignee_ids if content contains /assign' do _, updates = service.execute(content, merge_request) - expect(updates).to eq(assignee_id: developer.id) + expect(updates).to eq(assignee_ids: [developer.id]) end end end @@ -422,11 +422,11 @@ describe QuickActions::InterpretService, services: true do end context 'Merge Request' do - it 'populates assignee_id: nil if content contains /unassign' do - merge_request.update(assignee_id: developer.id) + it 'populates assignee_ids: [] if content contains /unassign' do + merge_request.update(assignee_ids: [developer.id]) _, updates = service.execute(content, merge_request) - expect(updates).to eq(assignee_id: nil) + expect(updates).to eq(assignee_ids: []) end end end