From 6a7d08912da5036ed540b8632b9a7d10ca5bdd2c Mon Sep 17 00:00:00 2001 From: Tomas Apodaca Date: Tue, 19 Feb 2019 14:46:30 -0800 Subject: [PATCH 1/4] rename list_of_assignees, move error to copy --- app/models/bva_dispatch_task_distributor.rb | 2 +- app/models/colocated_task_distributor.rb | 4 ++-- app/models/judge_assign_task_distributor.rb | 2 +- app/models/round_robin_task_distributor.rb | 14 +++++++------- app/models/tasks/bva_dispatch_task.rb | 4 ---- client/COPY.json | 3 ++- spec/models/colocated_task_distributor_spec.rb | 2 +- spec/models/organizations/bva_dispatch_spec.rb | 4 ++-- spec/models/organizations/colocated_spec.rb | 4 ++-- spec/models/round_robin_task_distributor_spec.rb | 10 +++++----- 10 files changed, 23 insertions(+), 26 deletions(-) diff --git a/app/models/bva_dispatch_task_distributor.rb b/app/models/bva_dispatch_task_distributor.rb index 77b832e8988..8633543e5eb 100644 --- a/app/models/bva_dispatch_task_distributor.rb +++ b/app/models/bva_dispatch_task_distributor.rb @@ -1,5 +1,5 @@ class BvaDispatchTaskDistributor < RoundRobinTaskDistributor - def initialize(list_of_assignees: BvaDispatch.singleton.users.order(:id).pluck(:css_id), + def initialize(assignee_pool: BvaDispatch.singleton.users.order(:id).pluck(:css_id), task_class: BvaDispatchTask) super end diff --git a/app/models/colocated_task_distributor.rb b/app/models/colocated_task_distributor.rb index f3fb4d5a9f0..f3b4179e327 100644 --- a/app/models/colocated_task_distributor.rb +++ b/app/models/colocated_task_distributor.rb @@ -1,5 +1,5 @@ class ColocatedTaskDistributor < RoundRobinTaskDistributor - def initialize(list_of_assignees: Colocated.singleton.non_admins.sort_by(&:id).pluck(:css_id), + def initialize(assignee_pool: Colocated.singleton.non_admins.sort_by(&:id).pluck(:css_id), task_class: Task) super end @@ -8,7 +8,7 @@ def next_assignee(options = {}) open_assignee = options.dig(:appeal) &.tasks &.active - &.find_by(assigned_to: User.where(css_id: list_of_assignees)) + &.find_by(assigned_to: User.where(css_id: assignee_pool)) &.assigned_to open_assignee || super() diff --git a/app/models/judge_assign_task_distributor.rb b/app/models/judge_assign_task_distributor.rb index a0928cb4682..ae4ed8fafb3 100644 --- a/app/models/judge_assign_task_distributor.rb +++ b/app/models/judge_assign_task_distributor.rb @@ -1,5 +1,5 @@ class JudgeAssignTaskDistributor < RoundRobinTaskDistributor - def initialize(list_of_assignees: Constants::RampJudges::USERS[Rails.current_env], + def initialize(assignee_pool: Constants::RampJudges::USERS[Rails.current_env], task_class: JudgeAssignTask) super end diff --git a/app/models/round_robin_task_distributor.rb b/app/models/round_robin_task_distributor.rb index 8419bb51ad9..778c5868102 100644 --- a/app/models/round_robin_task_distributor.rb +++ b/app/models/round_robin_task_distributor.rb @@ -1,10 +1,10 @@ class RoundRobinTaskDistributor include ActiveModel::Model - attr_accessor :list_of_assignees, :task_class + attr_accessor :assignee_pool, :task_class def assignee_users - User.where(css_id: list_of_assignees) + User.where(css_id: assignee_pool) end def latest_task @@ -16,22 +16,22 @@ def last_assignee_css_id end def last_assignee_index - list_of_assignees.index(last_assignee_css_id) + assignee_pool.index(last_assignee_css_id) end def next_assignee_index return 0 unless last_assignee_css_id return 0 unless last_assignee_index - (last_assignee_index + 1) % list_of_assignees.length + (last_assignee_index + 1) % assignee_pool.length end def next_assignee_css_id - if list_of_assignees.blank? - fail Caseflow::Error::RoundRobinTaskDistributorError, message: "list_of_assignees cannot be empty" + if assignee_pool.blank? + fail Caseflow::Error::RoundRobinTaskDistributorError, message: COPY::TASK_DISTRIBUTOR_ASSIGNEE_POOL_EMPTY_MESSAGE end - list_of_assignees[next_assignee_index] + assignee_pool[next_assignee_index] end def next_assignee(_options = {}) diff --git a/app/models/tasks/bva_dispatch_task.rb b/app/models/tasks/bva_dispatch_task.rb index 7a276cfbd3b..0150f3107a4 100644 --- a/app/models/tasks/bva_dispatch_task.rb +++ b/app/models/tasks/bva_dispatch_task.rb @@ -27,10 +27,6 @@ def outcode(appeal, params, user) private - def list_of_assignees - BvaDispatch.singleton.users.order(:id).pluck(:css_id) - end - def create_decision_document!(params) DecisionDocument.create!(params).tap do |decision_document| decision_document.submit_for_processing! diff --git a/client/COPY.json b/client/COPY.json index 5b4b95f16a9..c69d1bef297 100644 --- a/client/COPY.json +++ b/client/COPY.json @@ -360,6 +360,7 @@ "WITHDRAW_HEARING_MODAL_TITLE": "Confirm withdraw hearing", "WITHDRAW_HEARING_MODAL_BODY": "Withdrawing this hearing request will remove this veteran from the hearing pool.", "WITHDRAW_HEARING_SUCCESS_MESSAGE_TITLE": "You have successfully withdrawn %s's hearing request", - "WITHDRAW_HEARING_SUCCESS_MESSAGE_BODY": "%s has been removed from the hearing schedule and has been sent to the next appropriate location" + "WITHDRAW_HEARING_SUCCESS_MESSAGE_BODY": "%s has been removed from the hearing schedule and has been sent to the next appropriate location", + "TASK_DISTRIBUTOR_ASSIGNEE_POOL_EMPTY_MESSAGE": "assignee_pool cannot be empty" } diff --git a/spec/models/colocated_task_distributor_spec.rb b/spec/models/colocated_task_distributor_spec.rb index 916e3b8f389..6410f9fd599 100644 --- a/spec/models/colocated_task_distributor_spec.rb +++ b/spec/models/colocated_task_distributor_spec.rb @@ -12,7 +12,7 @@ describe ".next_assignee" do let(:iterations) { 6 } - context "when the list_of_assignees is a populated array" do + context "when the assignee_pool is a populated array" do let(:total_distribution_count) { iterations * assignee_pool_size } before do diff --git a/spec/models/organizations/bva_dispatch_spec.rb b/spec/models/organizations/bva_dispatch_spec.rb index 0e1c75f24e3..8f414bc3ccb 100644 --- a/spec/models/organizations/bva_dispatch_spec.rb +++ b/spec/models/organizations/bva_dispatch_spec.rb @@ -18,7 +18,7 @@ it "should throw an error" do expect { subject }.to raise_error do |error| expect(error).to be_a(Caseflow::Error::RoundRobinTaskDistributorError) - expect(error.message).to eq("list_of_assignees cannot be empty") + expect(error.message).to eq(COPY::TASK_DISTRIBUTOR_ASSIGNEE_POOL_EMPTY_MESSAGE) end end end @@ -45,7 +45,7 @@ it "should throw an error" do expect { subject }.to raise_error do |error| expect(error).to be_a(Caseflow::Error::RoundRobinTaskDistributorError) - expect(error.message).to eq("list_of_assignees cannot be empty") + expect(error.message).to eq(COPY::TASK_DISTRIBUTOR_ASSIGNEE_POOL_EMPTY_MESSAGE) end end end diff --git a/spec/models/organizations/colocated_spec.rb b/spec/models/organizations/colocated_spec.rb index 61ec243b282..13c2e76c0f6 100644 --- a/spec/models/organizations/colocated_spec.rb +++ b/spec/models/organizations/colocated_spec.rb @@ -19,7 +19,7 @@ it "should throw an error" do expect { subject }.to raise_error do |error| expect(error).to be_a(Caseflow::Error::RoundRobinTaskDistributorError) - expect(error.message).to eq("list_of_assignees cannot be empty") + expect(error.message).to eq(COPY::TASK_DISTRIBUTOR_ASSIGNEE_POOL_EMPTY_MESSAGE) end end end @@ -53,7 +53,7 @@ it "should throw an error" do expect { subject }.to raise_error do |error| expect(error).to be_a(Caseflow::Error::RoundRobinTaskDistributorError) - expect(error.message).to eq("list_of_assignees cannot be empty") + expect(error.message).to eq(COPY::TASK_DISTRIBUTOR_ASSIGNEE_POOL_EMPTY_MESSAGE) end end end diff --git a/spec/models/round_robin_task_distributor_spec.rb b/spec/models/round_robin_task_distributor_spec.rb index f238546375b..3adaa226222 100644 --- a/spec/models/round_robin_task_distributor_spec.rb +++ b/spec/models/round_robin_task_distributor_spec.rb @@ -3,7 +3,7 @@ let!(:assignee_pool) { FactoryBot.create_list(:user, assignee_pool_size) } let(:task_class) { Task } let(:round_robin_distributor) do - RoundRobinTaskDistributor.new(list_of_assignees: assignee_pool.pluck(:css_id), task_class: task_class) + RoundRobinTaskDistributor.new(assignee_pool: assignee_pool.pluck(:css_id), task_class: task_class) end describe ".latest_task" do @@ -40,18 +40,18 @@ end describe ".next_assignee" do - context "when the list_of_assignees is an empty array" do - let(:round_robin_distributor) { RoundRobinTaskDistributor.new(list_of_assignees: [], task_class: task_class) } + context "when the assignee_pool is an empty array" do + let(:round_robin_distributor) { RoundRobinTaskDistributor.new(assignee_pool: [], task_class: task_class) } it "should raise an error" do expect { round_robin_distributor.next_assignee }.to(raise_error) do |error| expect(error).to be_a(Caseflow::Error::RoundRobinTaskDistributorError) - expect(error.message).to eq("list_of_assignees cannot be empty") + expect(error.message).to eq(COPY::TASK_DISTRIBUTOR_ASSIGNEE_POOL_EMPTY_MESSAGE) end end end - context "when the list_of_assignees is a populated array" do + context "when the assignee_pool is a populated array" do let(:iterations) { 4 } let(:total_distribution_count) { iterations * assignee_pool_size } From 8d0f31304af5d8737bd1ffd1cb57ec13f1c4d5a5 Mon Sep 17 00:00:00 2001 From: Tomas Apodaca Date: Tue, 19 Feb 2019 15:09:55 -0800 Subject: [PATCH 2/4] this model's no longer used as of fc609bad9e --- app/models/judge_assign_task_distributor.rb | 6 ------ 1 file changed, 6 deletions(-) delete mode 100644 app/models/judge_assign_task_distributor.rb diff --git a/app/models/judge_assign_task_distributor.rb b/app/models/judge_assign_task_distributor.rb deleted file mode 100644 index ae4ed8fafb3..00000000000 --- a/app/models/judge_assign_task_distributor.rb +++ /dev/null @@ -1,6 +0,0 @@ -class JudgeAssignTaskDistributor < RoundRobinTaskDistributor - def initialize(assignee_pool: Constants::RampJudges::USERS[Rails.current_env], - task_class: JudgeAssignTask) - super - end -end From 6006e7c460f4190be1033a5ebcf3a8d9f102b931 Mon Sep 17 00:00:00 2001 From: Tomas Apodaca Date: Tue, 19 Feb 2019 15:14:20 -0800 Subject: [PATCH 3/4] use User objects instead of css_ids --- app/models/bva_dispatch_task_distributor.rb | 2 +- app/models/colocated_task_distributor.rb | 4 ++-- app/models/round_robin_task_distributor.rb | 20 ++++++------------- .../round_robin_task_distributor_spec.rb | 2 +- 4 files changed, 10 insertions(+), 18 deletions(-) diff --git a/app/models/bva_dispatch_task_distributor.rb b/app/models/bva_dispatch_task_distributor.rb index 8633543e5eb..a11541980c1 100644 --- a/app/models/bva_dispatch_task_distributor.rb +++ b/app/models/bva_dispatch_task_distributor.rb @@ -1,5 +1,5 @@ class BvaDispatchTaskDistributor < RoundRobinTaskDistributor - def initialize(assignee_pool: BvaDispatch.singleton.users.order(:id).pluck(:css_id), + def initialize(assignee_pool: BvaDispatch.singleton.users.order(:id), task_class: BvaDispatchTask) super end diff --git a/app/models/colocated_task_distributor.rb b/app/models/colocated_task_distributor.rb index f3b4179e327..7cac6ea37da 100644 --- a/app/models/colocated_task_distributor.rb +++ b/app/models/colocated_task_distributor.rb @@ -1,5 +1,5 @@ class ColocatedTaskDistributor < RoundRobinTaskDistributor - def initialize(assignee_pool: Colocated.singleton.non_admins.sort_by(&:id).pluck(:css_id), + def initialize(assignee_pool: Colocated.singleton.non_admins.sort_by(&:id), task_class: Task) super end @@ -8,7 +8,7 @@ def next_assignee(options = {}) open_assignee = options.dig(:appeal) &.tasks &.active - &.find_by(assigned_to: User.where(css_id: assignee_pool)) + &.find_by(assigned_to: assignee_pool) &.assigned_to open_assignee || super() diff --git a/app/models/round_robin_task_distributor.rb b/app/models/round_robin_task_distributor.rb index 778c5868102..436187efce1 100644 --- a/app/models/round_robin_task_distributor.rb +++ b/app/models/round_robin_task_distributor.rb @@ -3,38 +3,30 @@ class RoundRobinTaskDistributor attr_accessor :assignee_pool, :task_class - def assignee_users - User.where(css_id: assignee_pool) - end - def latest_task - task_class.where(assigned_to: assignee_users).max_by(&:created_at) + task_class.where(assigned_to: assignee_pool).max_by(&:created_at) end - def last_assignee_css_id - latest_task&.assigned_to&.css_id + def last_assignee + latest_task&.assigned_to end def last_assignee_index - assignee_pool.index(last_assignee_css_id) + assignee_pool.index(last_assignee) end def next_assignee_index - return 0 unless last_assignee_css_id + return 0 unless last_assignee return 0 unless last_assignee_index (last_assignee_index + 1) % assignee_pool.length end - def next_assignee_css_id + def next_assignee(_options = {}) if assignee_pool.blank? fail Caseflow::Error::RoundRobinTaskDistributorError, message: COPY::TASK_DISTRIBUTOR_ASSIGNEE_POOL_EMPTY_MESSAGE end assignee_pool[next_assignee_index] end - - def next_assignee(_options = {}) - User.find_by_css_id_or_create_with_default_station_id(next_assignee_css_id) - end end diff --git a/spec/models/round_robin_task_distributor_spec.rb b/spec/models/round_robin_task_distributor_spec.rb index 3adaa226222..43cd338954f 100644 --- a/spec/models/round_robin_task_distributor_spec.rb +++ b/spec/models/round_robin_task_distributor_spec.rb @@ -3,7 +3,7 @@ let!(:assignee_pool) { FactoryBot.create_list(:user, assignee_pool_size) } let(:task_class) { Task } let(:round_robin_distributor) do - RoundRobinTaskDistributor.new(assignee_pool: assignee_pool.pluck(:css_id), task_class: task_class) + RoundRobinTaskDistributor.new(assignee_pool: assignee_pool, task_class: task_class) end describe ".latest_task" do From 187ec72d4cafe064db99f75d8494e2510fa63e5b Mon Sep 17 00:00:00 2001 From: Tomas Apodaca Date: Tue, 19 Feb 2019 16:36:11 -0800 Subject: [PATCH 4/4] move distributor failure to validation add validations for a non-blank task_class and an assignee_pool containing only users. --- app/models/round_robin_task_distributor.rb | 15 +++++++- client/COPY.json | 2 +- .../models/organizations/bva_dispatch_spec.rb | 4 +- spec/models/organizations/colocated_spec.rb | 4 +- .../round_robin_task_distributor_spec.rb | 38 +++++++++++++++---- 5 files changed, 49 insertions(+), 14 deletions(-) diff --git a/app/models/round_robin_task_distributor.rb b/app/models/round_robin_task_distributor.rb index 436187efce1..9789c093688 100644 --- a/app/models/round_robin_task_distributor.rb +++ b/app/models/round_robin_task_distributor.rb @@ -1,6 +1,9 @@ class RoundRobinTaskDistributor include ActiveModel::Model + validates :task_class, :assignee_pool, presence: true + validate :assignee_pool_must_contain_only_users + attr_accessor :assignee_pool, :task_class def latest_task @@ -23,10 +26,18 @@ def next_assignee_index end def next_assignee(_options = {}) - if assignee_pool.blank? - fail Caseflow::Error::RoundRobinTaskDistributorError, message: COPY::TASK_DISTRIBUTOR_ASSIGNEE_POOL_EMPTY_MESSAGE + unless valid? + fail Caseflow::Error::RoundRobinTaskDistributorError, message: errors.full_messages.join(", ") end assignee_pool[next_assignee_index] end + + private + + def assignee_pool_must_contain_only_users + unless assignee_pool.all? { |a| a.is_a?(User) } + errors.add(:assignee_pool, COPY::TASK_DISTRIBUTOR_ASSIGNEE_POOL_USERS_ONLY_MESSAGE) + end + end end diff --git a/client/COPY.json b/client/COPY.json index c69d1bef297..4bd304798db 100644 --- a/client/COPY.json +++ b/client/COPY.json @@ -362,5 +362,5 @@ "WITHDRAW_HEARING_SUCCESS_MESSAGE_TITLE": "You have successfully withdrawn %s's hearing request", "WITHDRAW_HEARING_SUCCESS_MESSAGE_BODY": "%s has been removed from the hearing schedule and has been sent to the next appropriate location", - "TASK_DISTRIBUTOR_ASSIGNEE_POOL_EMPTY_MESSAGE": "assignee_pool cannot be empty" + "TASK_DISTRIBUTOR_ASSIGNEE_POOL_USERS_ONLY_MESSAGE": "must contain only Users" } diff --git a/spec/models/organizations/bva_dispatch_spec.rb b/spec/models/organizations/bva_dispatch_spec.rb index 8f414bc3ccb..8104386cd2a 100644 --- a/spec/models/organizations/bva_dispatch_spec.rb +++ b/spec/models/organizations/bva_dispatch_spec.rb @@ -18,7 +18,7 @@ it "should throw an error" do expect { subject }.to raise_error do |error| expect(error).to be_a(Caseflow::Error::RoundRobinTaskDistributorError) - expect(error.message).to eq(COPY::TASK_DISTRIBUTOR_ASSIGNEE_POOL_EMPTY_MESSAGE) + expect(error.message).to eq("Assignee pool can't be blank") end end end @@ -45,7 +45,7 @@ it "should throw an error" do expect { subject }.to raise_error do |error| expect(error).to be_a(Caseflow::Error::RoundRobinTaskDistributorError) - expect(error.message).to eq(COPY::TASK_DISTRIBUTOR_ASSIGNEE_POOL_EMPTY_MESSAGE) + expect(error.message).to eq("Assignee pool can't be blank") end end end diff --git a/spec/models/organizations/colocated_spec.rb b/spec/models/organizations/colocated_spec.rb index 13c2e76c0f6..77ac910937a 100644 --- a/spec/models/organizations/colocated_spec.rb +++ b/spec/models/organizations/colocated_spec.rb @@ -19,7 +19,7 @@ it "should throw an error" do expect { subject }.to raise_error do |error| expect(error).to be_a(Caseflow::Error::RoundRobinTaskDistributorError) - expect(error.message).to eq(COPY::TASK_DISTRIBUTOR_ASSIGNEE_POOL_EMPTY_MESSAGE) + expect(error.message).to eq("Assignee pool can't be blank") end end end @@ -53,7 +53,7 @@ it "should throw an error" do expect { subject }.to raise_error do |error| expect(error).to be_a(Caseflow::Error::RoundRobinTaskDistributorError) - expect(error.message).to eq(COPY::TASK_DISTRIBUTOR_ASSIGNEE_POOL_EMPTY_MESSAGE) + expect(error.message).to eq("Assignee pool can't be blank") end end end diff --git a/spec/models/round_robin_task_distributor_spec.rb b/spec/models/round_robin_task_distributor_spec.rb index 43cd338954f..7452e99ab6b 100644 --- a/spec/models/round_robin_task_distributor_spec.rb +++ b/spec/models/round_robin_task_distributor_spec.rb @@ -40,18 +40,42 @@ end describe ".next_assignee" do - context "when the assignee_pool is an empty array" do - let(:round_robin_distributor) { RoundRobinTaskDistributor.new(assignee_pool: [], task_class: task_class) } + context "the distributor is invalid" do + context "the assignee_pool is empty" do + let!(:assignee_pool) { [] } - it "should raise an error" do - expect { round_robin_distributor.next_assignee }.to(raise_error) do |error| - expect(error).to be_a(Caseflow::Error::RoundRobinTaskDistributorError) - expect(error.message).to eq(COPY::TASK_DISTRIBUTOR_ASSIGNEE_POOL_EMPTY_MESSAGE) + it "raises an error" do + expect { round_robin_distributor.next_assignee }.to(raise_error) do |error| + expect(error).to be_a(Caseflow::Error::RoundRobinTaskDistributorError) + expect(error.message).to eq("Assignee pool can't be blank") + end + end + end + + context "the task_class is blank" do + let(:task_class) { nil } + + it "raises an error" do + expect { round_robin_distributor.next_assignee }.to(raise_error) do |error| + expect(error).to be_a(Caseflow::Error::RoundRobinTaskDistributorError) + expect(error.message).to eq("Task class can't be blank") + end + end + end + + context "the assignee_pool contains items that aren't Users" do + let!(:assignee_pool) { FactoryBot.create_list(:user, assignee_pool_size).pluck(:css_id) } + + it "raises an error" do + expect { round_robin_distributor.next_assignee }.to(raise_error) do |error| + expect(error).to be_a(Caseflow::Error::RoundRobinTaskDistributorError) + expect(error.message).to eq("Assignee pool #{COPY::TASK_DISTRIBUTOR_ASSIGNEE_POOL_USERS_ONLY_MESSAGE}") + end end end end - context "when the assignee_pool is a populated array" do + context "the assignee_pool is a populated array" do let(:iterations) { 4 } let(:total_distribution_count) { iterations * assignee_pool_size }