From 5490e2ecb037cb75ec5eac323631c6b2d1eeef27 Mon Sep 17 00:00:00 2001 From: Phil Lee Date: Fri, 17 Feb 2023 10:20:39 +0000 Subject: [PATCH] CLDC-1892 Bulk upload validation for managing organisation (#1315) * bulk upload validation for owning org * block log creation at row parser level * bubble up block_log_creation so it blocks logs * owning org id looks up serveral fields * validate bulk upload managing org --- app/models/organisation.rb | 27 ++++++ .../bulk_upload/lettings/row_parser.rb | 67 ++++++++++++-- .../bulk_upload/lettings/validator.rb | 1 + spec/factories/organisation.rb | 4 + .../bulk_upload/lettings/row_parser_spec.rb | 88 +++++++++++++++++++ .../bulk_upload/lettings/validator_spec.rb | 20 ++++- 6 files changed, 199 insertions(+), 8 deletions(-) diff --git a/app/models/organisation.rb b/app/models/organisation.rb index 89d7d1f34..32d3540d1 100644 --- a/app/models/organisation.rb +++ b/app/models/organisation.rb @@ -17,8 +17,21 @@ class Organisation < ApplicationRecord has_many :managing_agent_relationships, foreign_key: :parent_organisation_id, class_name: "OrganisationRelationship" has_many :managing_agents, through: :managing_agent_relationships, source: :child_organisation + def affiliated_stock_owners + ids = [] + + if holds_own_stock? && persisted? + ids << id + end + + ids.concat(stock_owners.pluck(:id)) + + Organisation.where(id: ids) + end + scope :search_by_name, ->(name) { where("name ILIKE ?", "%#{name}%") } scope :search_by, ->(param) { search_by_name(param) } + has_paper_trail auto_strip_attributes :name @@ -35,6 +48,20 @@ class Organisation < ApplicationRecord validates :name, presence: { message: I18n.t("validations.organisation.name_missing") } validates :provider_type, presence: { message: I18n.t("validations.organisation.provider_type_missing") } + def self.find_by_id_on_mulitple_fields(id) + return if id.nil? + + if id.start_with?("ORG") + where(id: id[3..]).first + else + where(old_visible_id: id).first + end + end + + def can_be_managed_by?(organisation:) + organisation == self || managing_agents.include?(organisation) + end + def lettings_logs LettingsLog.filter_by_organisation(self) end diff --git a/app/services/bulk_upload/lettings/row_parser.rb b/app/services/bulk_upload/lettings/row_parser.rb index 005d5caf9..bdb35dbc6 100644 --- a/app/services/bulk_upload/lettings/row_parser.rb +++ b/app/services/bulk_upload/lettings/row_parser.rb @@ -3,6 +3,7 @@ class BulkUpload::Lettings::RowParser include ActiveModel::Attributes attribute :bulk_upload + attribute :block_log_creation, :boolean, default: -> { false } attribute :field_1, :integer attribute :field_2 @@ -114,9 +115,9 @@ class BulkUpload::Lettings::RowParser attribute :field_108, :string attribute :field_109, :string attribute :field_110 - attribute :field_111, :integer + attribute :field_111, :string attribute :field_112, :string - attribute :field_113, :integer + attribute :field_113, :string attribute :field_114, :integer attribute :field_115 attribute :field_116, :integer @@ -155,6 +156,13 @@ class BulkUpload::Lettings::RowParser validate :validate_dont_know_disabled_needs_conjunction validate :validate_no_and_dont_know_disabled_needs_conjunction + validate :validate_owning_org_permitted + validate :validate_owning_org_owns_stock + validate :validate_owning_org_exists + + validate :validate_managing_org_related + validate :validate_managing_org_exists + def valid? errors.clear @@ -173,15 +181,60 @@ class BulkUpload::Lettings::RowParser end def blank_row? - attribute_set.to_hash.reject { |k, _| %w[bulk_upload].include?(k) }.values.compact.empty? + attribute_set.to_hash.reject { |k, _| %w[bulk_upload block_log_creation].include?(k) }.values.compact.empty? end def log @log ||= LettingsLog.new(attributes_for_log) end + def block_log_creation! + self.block_log_creation = true + end + + def block_log_creation? + block_log_creation + end + private + def validate_managing_org_related + if owning_organisation && managing_organisation && !owning_organisation.can_be_managed_by?(organisation: managing_organisation) + block_log_creation! + errors.add(:field_113, "This managing organisation does not have a relationship with the owning organisation") + end + end + + def validate_managing_org_exists + if managing_organisation.nil? + errors.delete(:field_113) + errors.add(:field_113, "The managing organisation code is incorrect") + end + end + + def validate_owning_org_owns_stock + if owning_organisation && !owning_organisation.holds_own_stock? + block_log_creation! + errors.delete(:field_111) + errors.add(:field_111, "The owning organisation code provided is for an organisation that does not own stock") + end + end + + def validate_owning_org_exists + if owning_organisation.nil? + errors.delete(:field_111) + errors.add(:field_111, "The owning organisation code is incorrect") + end + end + + def validate_owning_org_permitted + if owning_organisation && !bulk_upload.user.organisation.affiliated_stock_owners.include?(owning_organisation) + block_log_creation! + errors.delete(:field_111) + errors.add(:field_111, "You do not have permission to add logs for this owning organisation") + end + end + def validate_no_and_dont_know_disabled_needs_conjunction if field_59 == 1 && field_60 == 1 errors.add(:field_59, I18n.t("validations.household.housingneeds.no_and_dont_know_disabled_needs_conjunction")) @@ -483,15 +536,19 @@ private end def owning_organisation - Organisation.find_by(old_visible_id: field_111) + Organisation.find_by_id_on_mulitple_fields(field_111) end def owning_organisation_id owning_organisation&.id end + def managing_organisation + Organisation.find_by_id_on_mulitple_fields(field_113) + end + def managing_organisation_id - Organisation.find_by(old_visible_id: field_113)&.id + managing_organisation&.id end def attributes_for_log diff --git a/app/services/bulk_upload/lettings/validator.rb b/app/services/bulk_upload/lettings/validator.rb index 6f37c0f3a..992f06196 100644 --- a/app/services/bulk_upload/lettings/validator.rb +++ b/app/services/bulk_upload/lettings/validator.rb @@ -176,6 +176,7 @@ class BulkUpload::Lettings::Validator def create_logs? return false if any_setup_sections_incomplete? return false if over_column_error_threshold? + return false if row_parsers.any?(&:block_log_creation?) row_parsers.all? { |row_parser| row_parser.log.valid? } end diff --git a/spec/factories/organisation.rb b/spec/factories/organisation.rb index 7aee3ad66..fa2650eb5 100644 --- a/spec/factories/organisation.rb +++ b/spec/factories/organisation.rb @@ -17,6 +17,10 @@ FactoryBot.define do trait :prp do provider_type { "PRP" } end + + trait :does_not_own_stock do + holds_own_stock { false } + end end factory :organisation_rent_period do diff --git a/spec/services/bulk_upload/lettings/row_parser_spec.rb b/spec/services/bulk_upload/lettings/row_parser_spec.rb index 4c0788033..36f145a5e 100644 --- a/spec/services/bulk_upload/lettings/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/row_parser_spec.rb @@ -8,8 +8,10 @@ RSpec.describe BulkUpload::Lettings::RowParser do let(:attributes) { { bulk_upload: } } let(:bulk_upload) { create(:bulk_upload, :lettings, user:) } let(:user) { create(:user, organisation: owning_org) } + let(:owning_org) { create(:organisation, :with_old_visible_id) } let(:managing_org) { create(:organisation, :with_old_visible_id) } + let(:setup_section_params) do { bulk_upload:, @@ -23,6 +25,10 @@ RSpec.describe BulkUpload::Lettings::RowParser do } end + before do + create(:organisation_relationship, parent_organisation: owning_org, child_organisation: managing_org) + end + around do |example| FormHandler.instance.use_real_forms! @@ -472,6 +478,68 @@ RSpec.describe BulkUpload::Lettings::RowParser do end end + describe "#field_111" do # owning org + context "when cannot find owning org" do + let(:attributes) { { bulk_upload:, field_111: "donotexist" } } + + it "is not permitted" do + expect(parser.errors[:field_111]).to eql(["The owning organisation code is incorrect"]) + end + end + + context "when org is not stock owning" do + let(:owning_org) { create(:organisation, :with_old_visible_id, :does_not_own_stock) } + + let(:attributes) { { bulk_upload:, field_111: owning_org.old_visible_id } } + + it "is not permitted" do + expect(parser.errors[:field_111]).to eql(["The owning organisation code provided is for an organisation that does not own stock"]) + end + + it "blocks log creation" do + expect(parser).to be_block_log_creation + end + end + + context "when not affiliated with owning org" do + let(:unaffiliated_org) { create(:organisation, :with_old_visible_id) } + + let(:attributes) { { bulk_upload:, field_111: unaffiliated_org.old_visible_id } } + + it "is not permitted" do + expect(parser.errors[:field_111]).to eql(["You do not have permission to add logs for this owning organisation"]) + end + + it "blocks log creation" do + expect(parser).to be_block_log_creation + end + end + end + + describe "#field_113" do # managing org + context "when cannot find managing org" do + let(:attributes) { { bulk_upload:, field_113: "donotexist" } } + + it "is not permitted" do + expect(parser.errors[:field_113]).to eql(["The managing organisation code is incorrect"]) + end + end + + context "when not affiliated with managing org" do + let(:unaffiliated_org) { create(:organisation, :with_old_visible_id) } + + let(:attributes) { { bulk_upload:, field_111: owning_org.old_visible_id, field_113: unaffiliated_org.old_visible_id } } + + it "is not permitted" do + expect(parser.errors[:field_113]).to eql(["This managing organisation does not have a relationship with the owning organisation"]) + end + + it "blocks log creation" do + expect(parser).to be_block_log_creation + end + end + end + describe "#field_134" do context "when an unpermitted value" do let(:attributes) { { bulk_upload:, field_134: 3 } } @@ -506,6 +574,26 @@ RSpec.describe BulkUpload::Lettings::RowParser do end describe "#log" do + describe "#owning_organisation" do + context "when lookup is via id prefixed with ORG" do + let(:attributes) { { bulk_upload:, field_111: "ORG#{owning_org.id}" } } + + it "assigns the correct org" do + expect(parser.log.owning_organisation).to eql(owning_org) + end + end + end + + describe "#managing_organisation" do + context "when lookup is via id prefixed with ORG" do + let(:attributes) { { bulk_upload:, field_113: "ORG#{managing_org.id}" } } + + it "assigns the correct org" do + expect(parser.log.managing_organisation).to eql(managing_org) + end + end + end + describe "#cbl" do context "when field_75 is yes ie 1" do let(:attributes) { { bulk_upload:, field_75: 1 } } diff --git a/spec/services/bulk_upload/lettings/validator_spec.rb b/spec/services/bulk_upload/lettings/validator_spec.rb index 263c83163..0aaeaac78 100644 --- a/spec/services/bulk_upload/lettings/validator_spec.rb +++ b/spec/services/bulk_upload/lettings/validator_spec.rb @@ -87,7 +87,7 @@ RSpec.describe BulkUpload::Lettings::Validator do end end - describe "#should_create_logs?" do + describe "#create_logs?" do context "when all logs are valid" do let(:target_path) { file_fixture("2022_23_lettings_bulk_upload.csv") } @@ -111,9 +111,7 @@ RSpec.describe BulkUpload::Lettings::Validator do expect(validator).not_to be_create_logs end end - end - describe "#create_logs?" do context "when a log is not valid?" do let(:log_1) { build(:lettings_log, :completed, created_by: user) } let(:log_2) { build(:lettings_log, :completed, created_by: user) } @@ -146,6 +144,22 @@ RSpec.describe BulkUpload::Lettings::Validator do end end + context "when a single log wants to block log creation" do + let(:unaffiliated_org) { create(:organisation) } + + let(:log_1) { build(:lettings_log, :completed, renttype: 1, created_by: user, owning_organisation: unaffiliated_org) } + + before do + file.write(BulkUpload::LogToCsv.new(log: log_1, line_ending: "\r\n", col_offset: 0).to_csv_row) + file.close + end + + it "will not create logs" do + validator.call + expect(validator).not_to be_create_logs + end + end + context "when a log has incomplete setup secion" do let(:log) { build(:lettings_log, :in_progress, created_by: user, startdate: Time.zone.local(2022, 5, 1)) }