From d88bb44e284de2dd049b054a206562e89aef52ae Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Mon, 31 Jul 2023 11:56:02 +0100 Subject: [PATCH 01/12] Bump word-wrap from 1.2.3 to 1.2.5 (#1808) Bumps [word-wrap](https://github.com/jonschlinkert/word-wrap) from 1.2.3 to 1.2.5. - [Release notes](https://github.com/jonschlinkert/word-wrap/releases) - [Commits](https://github.com/jonschlinkert/word-wrap/compare/1.2.3...1.2.5) --- updated-dependencies: - dependency-name: word-wrap dependency-type: indirect ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- yarn.lock | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/yarn.lock b/yarn.lock index dddaec069..8e34a628c 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5856,9 +5856,9 @@ wildcard@^2.0.0: integrity sha512-JcKqAHLPxcdb9KM49dufGXn2x3ssnfjbcaQdLlfZsL9rH9wgDQjUtDxbo8NE0F6SFvydeu1VhZe7hZuHsB2/pw== word-wrap@^1.2.3: - version "1.2.3" - resolved "https://registry.yarnpkg.com/word-wrap/-/word-wrap-1.2.3.tgz#610636f6b1f703891bd34771ccb17fb93b47079c" - integrity sha512-Hz/mrNwitNRh/HUAtM/VT/5VH+ygD6DV7mYKZAtHOrbs8U7lvPS6xf7EJKMF0uW1KJCl0H701g3ZGus+muE5vQ== + version "1.2.5" + resolved "https://registry.yarnpkg.com/word-wrap/-/word-wrap-1.2.5.tgz#d2c45c6dd4fbce621a66f136cbe328afd0410b34" + integrity sha512-BN22B5eaMMI9UMtjrGd5g5eCYPpCPDUy0FJXbYsaT5zYxjFOckS53SQDE3pWkVoWpHXVb3BrYcEN4Twa55B5cA== wrap-ansi@^7.0.0: version "7.0.0" From 4bc1625247517a1023f3b06c7930ccdf4670653b Mon Sep 17 00:00:00 2001 From: Jack <113976590+bibblobcode@users.noreply.github.com> Date: Tue, 1 Aug 2023 09:31:14 +0100 Subject: [PATCH 02/12] Add owning org filter to schemes (#1809) * Add owning org filter to schemes * Use current_user helper method * Add filtering specs * Check if org holds own stock --- app/helpers/filters_helper.rb | 6 ++ app/models/scheme.rb | 1 + app/services/filter_manager.rb | 20 ++++- app/views/logs/_log_filters.html.erb | 10 +-- app/views/schemes/_scheme_filters.html.erb | 21 ++++++ spec/helpers/filters_helper_spec.rb | 70 +++++++++++++++++ spec/models/scheme_spec.rb | 19 +++++ spec/requests/schemes_controller_spec.rb | 88 ++++++++++++++++++++-- 8 files changed, 220 insertions(+), 15 deletions(-) diff --git a/app/helpers/filters_helper.rb b/app/helpers/filters_helper.rb index be2f7ced5..33e109565 100644 --- a/app/helpers/filters_helper.rb +++ b/app/helpers/filters_helper.rb @@ -91,6 +91,12 @@ module FiltersHelper [OpenStruct.new(id: "", name: "Select an option")] + organisation_options.map { |org| OpenStruct.new(id: org.id, name: org.name) } end + def show_scheme_managing_org_filter?(user) + org = user.organisation + + user.support? || org.stock_owners.count > 1 || (org.holds_own_stock? && org.stock_owners.count.positive?) + end + private def applied_filters_count(filter_type) diff --git a/app/models/scheme.rb b/app/models/scheme.rb index ff24abdad..4767b92cf 100644 --- a/app/models/scheme.rb +++ b/app/models/scheme.rb @@ -19,6 +19,7 @@ class Scheme < ApplicationRecord scope :order_by_completion, -> { order("schemes.confirmed ASC NULLS FIRST") } scope :order_by_service_name, -> { order(service_name: :asc) } + scope :filter_by_owning_organisation, ->(owning_organisation, _user = nil) { where(owning_organisation:) } scope :filter_by_status, lambda { |statuses, _user = nil| filtered_records = all scopes = [] diff --git a/app/services/filter_manager.rb b/app/services/filter_manager.rb index a473cf271..63b13ecdc 100644 --- a/app/services/filter_manager.rb +++ b/app/services/filter_manager.rb @@ -50,11 +50,12 @@ class FilterManager users end - def self.filter_schemes(schemes, search_term, filters, user) + def self.filter_schemes(schemes, search_term, filters, user, all_orgs) schemes = filter_by_search(schemes, search_term) filters.each do |category, values| next if Array(values).reject(&:empty?).blank? + next if category == "owning_organisation" && all_orgs schemes = schemes.public_send("filter_by_#{category}", values, user) end @@ -72,7 +73,8 @@ class FilterManager def deserialize_filters_from_session(specific_org) current_filters = session[session_name_for(filter_type)] new_filters = current_filters.present? ? JSON.parse(current_filters) : {} - if @filter_type.include?("logs") + + if filter_type.include?("logs") current_user.logs_filters(specific_org:).each do |filter| new_filters[filter] = params[filter] if params[filter].present? end @@ -84,10 +86,18 @@ class FilterManager new_filters["user"] = current_user.id.to_s if params["assigned_to"] == "you" end - if (@filter_type.include?("schemes") || @filter_type.include?("users")) && params["status"].present? + if (filter_type.include?("schemes") || filter_type.include?("users")) && params["status"].present? new_filters["status"] = params["status"] end + if filter_type.include?("schemes") + current_user.logs_filters(specific_org:).each do |filter| + new_filters[filter] = params[filter] if params[filter].present? + end + + new_filters = new_filters.except("owning_organisation") if params["owning_organisation_select"] == "all" + end + new_filters end @@ -102,7 +112,9 @@ class FilterManager end def filtered_schemes(schemes, search_term, filters) - FilterManager.filter_schemes(schemes, search_term, filters, current_user) + all_orgs = params["owning_organisation_select"] == "all" + + FilterManager.filter_schemes(schemes, search_term, filters, current_user, all_orgs) end def bulk_upload diff --git a/app/views/logs/_log_filters.html.erb b/app/views/logs/_log_filters.html.erb index cc3e3b4fb..1034379db 100644 --- a/app/views/logs/_log_filters.html.erb +++ b/app/views/logs/_log_filters.html.erb @@ -55,7 +55,7 @@ type: "select", label: "User", category: "user", - options: assigned_to_filter_options(@current_user), + options: assigned_to_filter_options(current_user), }, }, }, @@ -63,7 +63,7 @@ category: "assigned_to", } %> - <% if @current_user.support? || @current_user.organisation.stock_owners.count > 1 && request.path == "/lettings-logs" %> + <% if current_user.support? || current_user.organisation.stock_owners.count > 1 && request.path == "/lettings-logs" %> <%= render partial: "filters/radio_filter", locals: { f:, options: { @@ -74,7 +74,7 @@ type: "select", label: "Owning Organisation", category: "owning_organisation", - options: owning_organisation_filter_options(@current_user), + options: owning_organisation_filter_options(current_user), }, }, }, @@ -83,7 +83,7 @@ } %> <% end %> - <% if (@current_user.support? || @current_user.organisation.managing_agents.count > 1) && request.path == "/lettings-logs" %> + <% if (current_user.support? || current_user.organisation.managing_agents.count > 1) && request.path == "/lettings-logs" %> <%= render partial: "filters/radio_filter", locals: { f:, options: { @@ -94,7 +94,7 @@ type: "select", label: "Managed by", category: "managing_organisation", - options: managing_organisation_filter_options(@current_user), + options: managing_organisation_filter_options(current_user), }, }, }, diff --git a/app/views/schemes/_scheme_filters.html.erb b/app/views/schemes/_scheme_filters.html.erb index 086d6fd84..e723f7469 100644 --- a/app/views/schemes/_scheme_filters.html.erb +++ b/app/views/schemes/_scheme_filters.html.erb @@ -22,6 +22,27 @@ label: "Status", category: "status", } %> + + <% if show_scheme_managing_org_filter?(current_user) %> + <%= render partial: "filters/radio_filter", locals: { + f:, + options: { + "all": { label: "Any owning organisation" }, + "specific_org": { + label: "Specific owning organisation", + conditional_filter: { + type: "select", + label: "Owning Organisation", + category: "owning_organisation", + options: owning_organisation_filter_options(current_user), + }, + }, + }, + label: "Owned by", + category: "owning_organisation_select", + } %> + <% end %> + <%= f.govuk_submit "Apply filters", class: "govuk-!-margin-bottom-0" %> <% end %> diff --git a/spec/helpers/filters_helper_spec.rb b/spec/helpers/filters_helper_spec.rb index 6ab012907..da36068b1 100644 --- a/spec/helpers/filters_helper_spec.rb +++ b/spec/helpers/filters_helper_spec.rb @@ -290,4 +290,74 @@ RSpec.describe FiltersHelper do end end end + + describe "#show_scheme_managing_org_filter?" do + context "when support user" do + let(:user) { create(:user, :support, organisation: create(:organisation, stock_owners: [])) } + + it "returns true" do + expect(show_scheme_managing_org_filter?(user)).to be true + end + end + + context "when not support user" do + let(:stock_owner1) { create(:organisation) } + let(:stock_owner2) { create(:organisation) } + + context "when org's stock_owners > 1" do + let(:user) { create(:user, organisation: create(:organisation, holds_own_stock: false)) } + + before do + create( + :organisation_relationship, + child_organisation: user.organisation, + parent_organisation: stock_owner1, + ) + create( + :organisation_relationship, + child_organisation: user.organisation, + parent_organisation: stock_owner2, + ) + end + + it "returns true" do + expect(show_scheme_managing_org_filter?(user)).to be true + end + end + + context "when org's stock_owners == 1" do + before do + create( + :organisation_relationship, + child_organisation: user.organisation, + parent_organisation: stock_owner1, + ) + end + + context "when holds own stock" do + let(:user) { create(:user, organisation: create(:organisation, holds_own_stock: true)) } + + it "returns true" do + expect(show_scheme_managing_org_filter?(user)).to be true + end + end + + context "when does not hold own stock" do + let(:user) { create(:user, organisation: create(:organisation, holds_own_stock: false)) } + + it "returns false" do + expect(show_scheme_managing_org_filter?(user)).to be false + end + end + end + + context "when org's stock_owners == 0" do + let(:user) { create(:user) } + + it "returns false" do + expect(show_scheme_managing_org_filter?(user)).to be false + end + end + end + end end diff --git a/spec/models/scheme_spec.rb b/spec/models/scheme_spec.rb index 0bdcb940a..049da077a 100644 --- a/spec/models/scheme_spec.rb +++ b/spec/models/scheme_spec.rb @@ -90,6 +90,25 @@ RSpec.describe Scheme, type: :model do end end + context "when filtering by owning organisation" do + let(:organisation_1) { create(:organisation) } + let(:organisation_2) { create(:organisation) } + let(:organisation_3) { create(:organisation) } + + before do + create(:scheme, owning_organisation: organisation_1) + create(:scheme, owning_organisation: organisation_1) + create(:scheme, owning_organisation: organisation_2) + create(:scheme, owning_organisation: organisation_2) + end + + it "filters by given owning organisation" do + expect(described_class.filter_by_owning_organisation([organisation_1]).count).to eq(2) + expect(described_class.filter_by_owning_organisation([organisation_1, organisation_2]).count).to eq(4) + expect(described_class.filter_by_owning_organisation([organisation_3]).count).to eq(0) + end + end + context "when filtering by status" do let!(:incomplete_scheme) { FactoryBot.create(:scheme, :incomplete) } let(:active_scheme) { FactoryBot.create(:scheme) } diff --git a/spec/requests/schemes_controller_spec.rb b/spec/requests/schemes_controller_spec.rb index b145d7288..50d83a5b3 100644 --- a/spec/requests/schemes_controller_spec.rb +++ b/spec/requests/schemes_controller_spec.rb @@ -57,13 +57,13 @@ RSpec.describe SchemesController, type: :request do end context "when parent organisation has schemes" do - let(:parent_organisation) { FactoryBot.create(:organisation) } - let!(:parent_schemes) { FactoryBot.create_list(:scheme, 5, owning_organisation: parent_organisation) } + let(:parent_organisation) { create(:organisation) } + let!(:parent_schemes) { create_list(:scheme, 5, owning_organisation: parent_organisation) } before do create(:organisation_relationship, parent_organisation:, child_organisation: user.organisation) parent_schemes.each do |scheme| - FactoryBot.create(:location, scheme:) + create(:location, scheme:) end get "/schemes" end @@ -77,6 +77,48 @@ RSpec.describe SchemesController, type: :request do end context "when filtering" do + context "with owning organisation filter" do + context "when user org does not have owning orgs" do + it "does not show filter" do + expect(page).not_to have_content("Owned by") + end + end + + context "when user org has owning orgs" do + let!(:organisation1) { create(:organisation) } + let!(:scheme1) { create(:scheme, owning_organisation: organisation1) } + let!(:scheme2) { create(:scheme, owning_organisation: user.organisation) } + + before do + org = user.organisation + org.stock_owners = [organisation1, user.organisation] + org.save! + end + + context "when filtering by all owning orgs" do + it "shows schemes for all owning orgs" do + get "/schemes?owning_organisation_select=all", headers:, params: {} + follow_redirect! + + expect(page).to have_content("Owned by") + expect(page).to have_link(scheme1.service_name) + expect(page).to have_link(scheme2.service_name) + end + end + + context "when filtering by an owning org" do + it "when filtering by an owning org" do + get "/schemes?owning_organisation=#{organisation1.id}", headers:, params: {} + follow_redirect! + + expect(page).to have_content("Owned by") + expect(page).to have_link(scheme1.service_name) + expect(page).not_to have_link(scheme2.service_name) + end + end + end + end + context "with status filter" do let!(:incomplete_scheme) { create(:scheme, :incomplete, owning_organisation: user.organisation) } let(:active_scheme) { create(:scheme, owning_organisation: user.organisation) } @@ -282,6 +324,40 @@ RSpec.describe SchemesController, type: :request do end context "when filtering" do + context "with owning organisation filter" do + context "when user org does not have owning orgs" do + it "shows the filter" do + expect(page).to have_content("Owned by") + end + end + + context "when user org has owning orgs" do + let!(:organisation1) { create(:organisation) } + let!(:scheme1) { create(:scheme, owning_organisation: organisation1) } + let!(:scheme2) { create(:scheme, owning_organisation: user.organisation) } + + context "when filtering by all owning orgs" do + it "shows schemes for all owning orgs" do + get "/schemes?owning_organisation_select=all", headers:, params: {} + + expect(page).to have_content("Owned by") + expect(page).to have_link(scheme1.service_name) + expect(page).to have_link(scheme2.service_name) + end + end + + context "when filtering by an owning org" do + it "when filtering by an owning org" do + get "/schemes?owning_organisation=#{organisation1.id}", headers:, params: {} + + expect(page).to have_content("Owned by") + expect(page).to have_link(scheme1.service_name) + expect(page).not_to have_link(scheme2.service_name) + end + end + end + end + context "with status filter" do let!(:incomplete_scheme) { create(:scheme, :incomplete) } let(:active_scheme) { create(:scheme) } @@ -457,11 +533,11 @@ RSpec.describe SchemesController, type: :request do end context "when coordinator attempts to see scheme belonging to a parent organisation" do - let(:parent_organisation) { FactoryBot.create(:organisation) } - let!(:specific_scheme) { FactoryBot.create(:scheme, owning_organisation: parent_organisation) } + let(:parent_organisation) { create(:organisation) } + let!(:specific_scheme) { create(:scheme, owning_organisation: parent_organisation) } before do - FactoryBot.create(:location, scheme: specific_scheme) + create(:location, scheme: specific_scheme) create(:organisation_relationship, parent_organisation:, child_organisation: user.organisation) get "/schemes/#{specific_scheme.id}" end From 23bd5adc517009afb0b1a2ab37af7cfcb1fbe4c8 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Tue, 1 Aug 2023 16:25:41 +0100 Subject: [PATCH 03/12] CLDC-2247 Add locations filter (#1785) * Add location filters to the controller * Display the status filter and clear filters * refactor status filter into scope * Update incomplete, make reactivated scope work in conjunction with other scopes. Lint * specs * styling * Update test name * uncomment a test * Move filters under navigation --- app/controllers/locations_controller.rb | 13 +++- app/controllers/sessions_controller.rb | 3 +- app/helpers/filters_helper.rb | 17 ++++- app/models/location.rb | 50 +++++++++++++ app/services/filter_manager.rb | 17 ++++- .../locations/_location_filters.html.erb | 30 ++++++++ app/views/locations/index.html.erb | 12 ++- spec/factories/location.rb | 5 ++ spec/features/schemes_spec.rb | 37 ++++++++++ spec/models/location_spec.rb | 73 +++++++++++++++++++ spec/requests/locations_controller_spec.rb | 48 ++++++++++++ 11 files changed, 293 insertions(+), 12 deletions(-) create mode 100644 app/views/locations/_location_filters.html.erb diff --git a/app/controllers/locations_controller.rb b/app/controllers/locations_controller.rb index e55ee10d6..f89325b41 100644 --- a/app/controllers/locations_controller.rb +++ b/app/controllers/locations_controller.rb @@ -6,15 +6,18 @@ class LocationsController < ApplicationController before_action :find_location, except: %i[create index] before_action :find_scheme before_action :scheme_and_location_present, except: %i[create index] + before_action :session_filters, if: :current_user, only: %i[index] + before_action -> { filter_manager.serialize_filters_to_session }, if: :current_user, only: %i[index] before_action :authorize_user, except: %i[index create] def index authorize @scheme - @pagy, @locations = pagy(filtered_collection(@scheme.locations, search_term)) + @pagy, @locations = pagy(filter_manager.filtered_locations(@scheme.locations, search_term, session_filters)) @total_count = @scheme.locations.size @searched = search_term.presence + @filter_type = "scheme_locations" end def create @@ -297,4 +300,12 @@ private params[:referrer] == "check_answers" end helper_method :return_to_check_your_answers? + + def filter_manager + FilterManager.new(current_user:, session:, params:, filter_type: "scheme_locations") + end + + def session_filters + filter_manager.session_filters + end end diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index cdcd05c9f..225f05f8b 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -1,8 +1,9 @@ class SessionsController < ApplicationController def clear_filters session[session_name_for(params[:filter_type])] = "{}" + path_params = params[:path_params].presence || {} - redirect_to send("#{params[:filter_type]}_path") + redirect_to send("#{params[:filter_type]}_path", scheme_id: path_params[:scheme_id]) end private diff --git a/app/helpers/filters_helper.rb b/app/helpers/filters_helper.rb index 33e109565..e2da5b0ed 100644 --- a/app/helpers/filters_helper.rb +++ b/app/helpers/filters_helper.rb @@ -56,6 +56,17 @@ module FiltersHelper }.freeze end + def location_status_filters + { + "incomplete" => "Incomplete", + "active" => "Active", + "deactivating_soon" => "Deactivating soon", + "activating_soon" => "Activating soon", + "reactivating_soon" => "Reactivating soon", + "deactivated" => "Deactivated", + }.freeze + end + def selected_option(filter, filter_type) return false unless session[session_name_for(filter_type)] @@ -80,9 +91,9 @@ module FiltersHelper applied_filters_count(filter_type).zero? ? "No filters applied" : "#{pluralize(applied_filters_count(filter_type), 'filter')} applied" end - def reset_filters_link(filter_type) + def reset_filters_link(filter_type, path_params = {}) if applied_filters_count(filter_type).positive? - govuk_link_to "Clear", clear_filters_path(filter_type:) + govuk_link_to "Clear", clear_filters_path(filter_type:, path_params:) end end @@ -104,6 +115,8 @@ private end def applied_filters(filter_type) + return {} unless session[session_name_for(filter_type)] + JSON.parse(session[session_name_for(filter_type)]) end diff --git a/app/models/location.rb b/app/models/location.rb index 0dd6ba522..f25fd7d5d 100644 --- a/app/models/location.rb +++ b/app/models/location.rb @@ -27,6 +27,56 @@ class Location < ApplicationRecord scope :active_in_2_weeks, -> { where(confirmed: true).and(started_in_2_weeks) } scope :confirmed, -> { where(confirmed: true) } scope :unconfirmed, -> { where.not(confirmed: true) } + scope :filter_by_status, lambda { |statuses, _user = nil| + filtered_records = all + scopes = [] + + statuses.each do |status| + if respond_to?(status, true) + scopes << (status == "active" ? send("active_status") : send(status)) + end + end + + if scopes.any? + filtered_records = filtered_records + .left_outer_joins(:location_deactivation_periods) + .order("location_deactivation_periods.created_at DESC") + .merge(scopes.reduce(&:or)) + end + + filtered_records + } + + scope :incomplete, lambda { + where.not(confirmed: true) + } + + scope :deactivated, lambda { + merge(LocationDeactivationPeriod.deactivations_without_reactivation) + .where("location_deactivation_periods.deactivation_date <= ?", Time.zone.now) + } + + scope :deactivating_soon, lambda { + merge(LocationDeactivationPeriod.deactivations_without_reactivation) + .where("location_deactivation_periods.deactivation_date > ?", Time.zone.now) + } + + scope :reactivating_soon, lambda { + where.not("location_deactivation_periods.reactivation_date IS NULL") + .where("location_deactivation_periods.reactivation_date > ?", Time.zone.now) + } + + scope :activating_soon, lambda { + where("startdate > ?", Time.zone.now) + } + + scope :active_status, lambda { + where.not(id: joins(:location_deactivation_periods).reactivating_soon.pluck(:id)) + .where.not(id: joins(:location_deactivation_periods).deactivated.pluck(:id)) + .where.not(id: incomplete.pluck(:id)) + .where.not(id: joins(:location_deactivation_periods).deactivating_soon.pluck(:id)) + .where.not(id: activating_soon.pluck(:id)) + } LOCAL_AUTHORITIES = LocalAuthority.all.map { |la| [la.name, la.code] }.to_h diff --git a/app/services/filter_manager.rb b/app/services/filter_manager.rb index 63b13ecdc..c8665752d 100644 --- a/app/services/filter_manager.rb +++ b/app/services/filter_manager.rb @@ -62,6 +62,17 @@ class FilterManager schemes end + def self.filter_locations(locations, search_term, filters, user) + locations = filter_by_search(locations, search_term) + + filters.each do |category, values| + next if Array(values).reject(&:empty?).blank? + + locations = locations.public_send("filter_by_#{category}", values, user) + end + locations.order(created_at: :desc) + end + def serialize_filters_to_session(specific_org: false) session[session_name_for(filter_type)] = session_filters(specific_org:).to_json end @@ -86,7 +97,7 @@ class FilterManager new_filters["user"] = current_user.id.to_s if params["assigned_to"] == "you" end - if (filter_type.include?("schemes") || filter_type.include?("users")) && params["status"].present? + if (filter_type.include?("schemes") || filter_type.include?("users") || filter_type.include?("scheme_locations")) && params["status"].present? new_filters["status"] = params["status"] end @@ -117,6 +128,10 @@ class FilterManager FilterManager.filter_schemes(schemes, search_term, filters, current_user, all_orgs) end + def filtered_locations(locations, search_term, filters) + FilterManager.filter_locations(locations, search_term, filters, current_user) + end + def bulk_upload id = (logs_filters["bulk_upload_id"] || []).reject(&:blank?)[0] @bulk_upload ||= current_user.bulk_uploads.find_by(id:) diff --git a/app/views/locations/_location_filters.html.erb b/app/views/locations/_location_filters.html.erb new file mode 100644 index 000000000..d5bdeb156 --- /dev/null +++ b/app/views/locations/_location_filters.html.erb @@ -0,0 +1,30 @@ +
+
+
+

Filters

+
+ +
+ <%= form_with url: scheme_locations_path(@scheme), html: { method: :get } do |f| %> +
+

+ <%= filters_applied_text(@filter_type) %> +

+

+ <%= reset_filters_link(@filter_type, { scheme_id: @scheme.id }) %> +

+
+ + <%= render partial: "filters/checkbox_filter", + locals: { + f:, + options: location_status_filters, + label: "Status", + category: "status", + } %> + + <%= f.govuk_submit "Apply filters", class: "govuk-!-margin-bottom-0" %> + <% end %> +
+
+
diff --git a/app/views/locations/index.html.erb b/app/views/locations/index.html.erb index 7641bbd48..ac6e3a0df 100644 --- a/app/views/locations/index.html.erb +++ b/app/views/locations/index.html.erb @@ -9,21 +9,19 @@ <% end %> <%= render partial: "organisations/headings", locals: { main: @scheme.service_name, sub: nil } %> +
-
-
- <%= render SubNavigationComponent.new(items: scheme_items(request.path, @scheme.id, "Locations")) %> + <%= render SubNavigationComponent.new(items: scheme_items(request.path, @scheme.id, "Locations")) %> + <%= render partial: "locations/location_filters" %> +

Locations

+
<%= render SearchComponent.new(current_user:, search_label: "Search by location name or postcode", value: @searched) %> <%= govuk_section_break(visible: true, size: "m") %> -
-
-
-
<%= govuk_table do |table| %> <%= table.caption(classes: %w[govuk-!-font-size-19 govuk-!-font-weight-regular]) do |caption| %> <%= render(SearchResultCaptionComponent.new(searched: @searched, count: @pagy.count, item_label:, total_count: @total_count, item: "locations", path: request.path)) %> diff --git a/spec/factories/location.rb b/spec/factories/location.rb index f43da0ac8..00fa0d77f 100644 --- a/spec/factories/location.rb +++ b/spec/factories/location.rb @@ -21,6 +21,11 @@ FactoryBot.define do old_visible_id { "111" } end + trait :incomplete do + units { nil } + confirmed { false } + end + trait :with_old_visible_id do old_visible_id { rand(9_999_999).to_s } end diff --git a/spec/features/schemes_spec.rb b/spec/features/schemes_spec.rb index c88b831ec..73972c4c3 100644 --- a/spec/features/schemes_spec.rb +++ b/spec/features/schemes_spec.rb @@ -284,6 +284,43 @@ RSpec.describe "Schemes scheme Features" do end end + context "when filtering locations" do + before do + click_link("Locations") + end + + context "when no filters are selected" do + it "displays the filters component with no clear button" do + expect(page).to have_content("No filters applied") + expect(page).not_to have_content("Clear") + end + end + + context "when I have selected filters" do + before do + check("Active") + check("Incomplete") + click_button("Apply filters") + end + + it "displays the filters component with a correct count and clear button" do + expect(page).to have_content("2 filters applied") + expect(page).to have_content("Clear") + end + + context "when clearing the filters" do + before do + click_link("Clear") + end + + it "clears the filters and displays the filter component as before" do + expect(page).to have_content("No filters applied") + expect(page).not_to have_content("Clear") + end + end + end + end + context "when the user clicks add location" do before do click_link("Locations") diff --git a/spec/models/location_spec.rb b/spec/models/location_spec.rb index 1fe359136..f24a5f4fa 100644 --- a/spec/models/location_spec.rb +++ b/spec/models/location_spec.rb @@ -930,6 +930,79 @@ RSpec.describe Location, type: :model do end end + describe "filter by status" do + let!(:incomplete_location) { FactoryBot.create(:location, :incomplete, startdate: Time.zone.local(2022, 4, 1)) } + let!(:active_location) { FactoryBot.create(:location, startdate: Time.zone.local(2022, 4, 1)) } + let(:deactivating_soon_location) { FactoryBot.create(:location, startdate: Time.zone.local(2022, 4, 1)) } + let(:deactivated_location) { FactoryBot.create(:location, startdate: Time.zone.local(2022, 4, 1)) } + let(:reactivating_soon_location) { FactoryBot.create(:location, startdate: Time.zone.local(2022, 4, 1)) } + let!(:activating_soon_location) { FactoryBot.create(:location, startdate: Time.zone.local(2022, 7, 7)) } + + before do + Timecop.freeze(2022, 6, 7) + FactoryBot.create(:location_deactivation_period, deactivation_date: Time.zone.local(2022, 8, 8), location: deactivating_soon_location) + deactivating_soon_location.save! + FactoryBot.create(:location_deactivation_period, deactivation_date: Time.zone.local(2022, 6, 6), location: deactivated_location) + deactivated_location.save! + FactoryBot.create(:location_deactivation_period, deactivation_date: Time.zone.local(2022, 6, 7), reactivation_date: Time.zone.local(2022, 6, 8), location: reactivating_soon_location) + reactivating_soon_location.save! + end + + after do + Timecop.unfreeze + end + + context "when filtering by incomplete status" do + it "returns only incomplete locations" do + expect(described_class.filter_by_status(%w[incomplete]).count).to eq(1) + expect(described_class.filter_by_status(%w[incomplete]).first).to eq(incomplete_location) + end + end + + context "when filtering by active status" do + it "returns only active locations" do + expect(described_class.filter_by_status(%w[active]).count).to eq(1) + expect(described_class.filter_by_status(%w[active]).first).to eq(active_location) + end + end + + context "when filtering by deactivating_soon status" do + it "returns only deactivating_soon locations" do + expect(described_class.filter_by_status(%w[deactivating_soon]).count).to eq(1) + expect(described_class.filter_by_status(%w[deactivating_soon]).first).to eq(deactivating_soon_location) + end + end + + context "when filtering by deactivated status" do + it "returns only deactivated locations" do + expect(described_class.filter_by_status(%w[deactivated]).count).to eq(1) + expect(described_class.filter_by_status(%w[deactivated]).first).to eq(deactivated_location) + end + end + + context "when filtering by reactivating_soon status" do + it "returns only reactivating_soon locations" do + expect(described_class.filter_by_status(%w[reactivating_soon]).count).to eq(1) + expect(described_class.filter_by_status(%w[reactivating_soon]).first).to eq(reactivating_soon_location) + end + end + + context "when filtering by activating_soon status" do + it "returns only activating_soon locations" do + expect(described_class.filter_by_status(%w[activating_soon]).count).to eq(1) + expect(described_class.filter_by_status(%w[activating_soon]).first).to eq(activating_soon_location) + end + end + + context "when filtering by multiple statuses" do + it "returns relevant locations" do + expect(described_class.filter_by_status(%w[deactivating_soon activating_soon]).count).to eq(2) + expect(described_class.filter_by_status(%w[deactivating_soon activating_soon])).to include(activating_soon_location) + expect(described_class.filter_by_status(%w[deactivating_soon activating_soon])).to include(deactivating_soon_location) + end + end + end + describe "available_from" do context "when there is a startdate" do let(:location) { FactoryBot.build(:location, startdate: Time.zone.local(2022, 4, 6)) } diff --git a/spec/requests/locations_controller_spec.rb b/spec/requests/locations_controller_spec.rb index 52b1c087f..7dccbec1b 100644 --- a/spec/requests/locations_controller_spec.rb +++ b/spec/requests/locations_controller_spec.rb @@ -119,6 +119,54 @@ RSpec.describe LocationsController, type: :request do it "returns 200" do expect(response).to be_successful end + + context "when filtering" do + context "with status filter" do + let(:scheme) { create(:scheme, owning_organisation: user.organisation) } + let!(:incomplete_location) { create(:location, :incomplete, scheme:, startdate: Time.zone.local(2022, 4, 1)) } + let!(:active_location) { create(:location, scheme:, startdate: Time.zone.local(2022, 4, 1)) } + let!(:deactivated_location) { create(:location, scheme:, startdate: Time.zone.local(2022, 4, 1)) } + + before do + create(:location_deactivation_period, deactivation_date: Time.zone.local(2022, 4, 1), location: deactivated_location) + end + + it "shows locations for multiple selected statuses" do + get "/schemes/#{scheme.id}/locations?status[]=incomplete&status[]=active", headers:, params: {} + expect(page).to have_link(incomplete_location.postcode) + expect(page).to have_link(active_location.postcode) + end + + it "shows filtered incomplete locations" do + get "/schemes/#{scheme.id}/locations?status[]=incomplete", headers:, params: {} + expect(page).to have_link(incomplete_location.postcode) + expect(page).not_to have_link(active_location.postcode) + end + + it "shows filtered active locations" do + get "/schemes/#{scheme.id}/locations?status[]=active", headers:, params: {} + expect(page).to have_link(active_location.postcode) + expect(page).not_to have_link(incomplete_location.postcode) + end + + it "shows filtered deactivated locations" do + get "/schemes/#{scheme.id}/locations?status[]=deactivated", headers:, params: {} + expect(page).to have_link(deactivated_location.postcode) + expect(page).not_to have_link(active_location.postcode) + expect(page).not_to have_link(incomplete_location.postcode) + end + + it "does not reset the filters" do + get "/schemes/#{scheme.id}/locations?status[]=incomplete", headers:, params: {} + expect(page).to have_link(incomplete_location.postcode) + expect(page).not_to have_link(active_location.postcode) + + get "/schemes/#{scheme.id}/locations", headers:, params: {} + expect(page).to have_link(incomplete_location.postcode) + expect(page).not_to have_link(active_location.postcode) + end + end + end end context "when signed in as a data coordinator user" do From 6e8acbc9275f0af9e5eeede2335b10c4f36a9f58 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Wed, 2 Aug 2023 08:07:37 +0100 Subject: [PATCH 04/12] Set housingneeds other to no if any of the housing needs are given (#1812) --- .../bulk_upload/lettings/year2022/row_parser.rb | 1 + .../bulk_upload/lettings/year2023/row_parser.rb | 1 + .../bulk_upload/lettings/year2022/row_parser_spec.rb | 10 ++++++++++ .../bulk_upload/lettings/year2023/row_parser_spec.rb | 10 ++++++++++ 4 files changed, 22 insertions(+) diff --git a/app/services/bulk_upload/lettings/year2022/row_parser.rb b/app/services/bulk_upload/lettings/year2022/row_parser.rb index 642c8efe8..9bef9312f 100644 --- a/app/services/bulk_upload/lettings/year2022/row_parser.rb +++ b/app/services/bulk_upload/lettings/year2022/row_parser.rb @@ -1466,6 +1466,7 @@ private def housingneeds_other return 1 if field_58 == 1 + return 0 if [field_55, field_56, field_57].include?(1) end def ethnic_group_from_ethnic diff --git a/app/services/bulk_upload/lettings/year2023/row_parser.rb b/app/services/bulk_upload/lettings/year2023/row_parser.rb index 4a12e9083..8d023792c 100644 --- a/app/services/bulk_upload/lettings/year2023/row_parser.rb +++ b/app/services/bulk_upload/lettings/year2023/row_parser.rb @@ -1410,6 +1410,7 @@ private def housingneeds_other return 1 if field_86 == 1 + return 0 if [field_83, field_84, field_85].include?(1) end def prevloc diff --git a/spec/services/bulk_upload/lettings/year2022/row_parser_spec.rb b/spec/services/bulk_upload/lettings/year2022/row_parser_spec.rb index b91caa969..6b5eaa2ae 100644 --- a/spec/services/bulk_upload/lettings/year2022/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/year2022/row_parser_spec.rb @@ -1914,6 +1914,16 @@ RSpec.describe BulkUpload::Lettings::Year2022::RowParser do expect(parser.log.housingneeds_other).to eq(1) end end + + context "when field_58 is nil and one housingneeds option is selected" do + let(:attributes) { { bulk_upload:, field_58: nil, field_55: "1" } } + + it "sets to 0" do + expect(parser.errors[:field_58]).to be_blank + expect(parser.errors[:field_55]).to be_blank + expect(parser.log.housingneeds_other).to eq(0) + end + end end end diff --git a/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb b/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb index e12785738..cac166f46 100644 --- a/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb +++ b/spec/services/bulk_upload/lettings/year2023/row_parser_spec.rb @@ -754,6 +754,16 @@ RSpec.describe BulkUpload::Lettings::Year2023::RowParser do expect(parser.errors[:field_87]).to be_present end end + + context "when one item selected and field_86 is blank" do + let(:attributes) { setup_section_params.merge({ field_83: "1", field_86: nil }) } + + it "sets other disabled access needs as no" do + expect(parser.errors[:field_83]).to be_blank + expect(parser.errors[:field_86]).to be_blank + expect(parser.log.housingneeds_other).to eq(0) + end + end end describe "#field_89, field_98 - 99" do From 9abde36f944e15c1d1c175b54a3e80d6ab5aa529 Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Wed, 2 Aug 2023 12:36:33 +0100 Subject: [PATCH 05/12] CLDC-2570 Add merge organisations service and rake task (#1801) * Create merge_organisations_service * Refactor tests * update schemes merge * rename scope * merge letings logs * Merge sales logs * refactor merge organisations service * Rollback transaction if there's an error merging orgs * Add logging * add merge_organisations rake task * mark organisations as merged * update rake task * Update rake task add tests for multiple merging orgs * Add more tests for merging relationships * Update merge logging * set absorbing organisation relationship --- app/controllers/locations_controller.rb | 4 +- app/controllers/schemes_controller.rb | 4 +- app/models/lettings_log.rb | 2 +- app/models/sales_log.rb | 1 + .../merge/merge_organisations_service.rb | 131 +++++++ db/schema.rb | 2 +- lib/tasks/merge_organisations.rake | 12 + spec/lib/tasks/merge_organisations_spec.rb | 41 +++ .../merge/merge_organisations_service_spec.rb | 323 ++++++++++++++++++ 9 files changed, 514 insertions(+), 6 deletions(-) create mode 100644 app/services/merge/merge_organisations_service.rb create mode 100644 lib/tasks/merge_organisations.rake create mode 100644 spec/lib/tasks/merge_organisations_spec.rb create mode 100644 spec/services/merge/merge_organisations_service_spec.rb diff --git a/app/controllers/locations_controller.rb b/app/controllers/locations_controller.rb index f89325b41..83e90c373 100644 --- a/app/controllers/locations_controller.rb +++ b/app/controllers/locations_controller.rb @@ -173,7 +173,7 @@ class LocationsController < ApplicationController end def deactivate_confirm - @affected_logs = @location.lettings_logs.visible.filter_by_before_startdate(params[:deactivation_date]) + @affected_logs = @location.lettings_logs.visible.after_date(params[:deactivation_date]) if @affected_logs.count.zero? deactivate else @@ -274,7 +274,7 @@ private end def reset_location_and_scheme_for_logs! - logs = @location.lettings_logs.visible.filter_by_before_startdate(params[:deactivation_date].to_time) + logs = @location.lettings_logs.visible.after_date(params[:deactivation_date].to_time) logs.update!(location: nil, scheme: nil, unresolved: true) logs end diff --git a/app/controllers/schemes_controller.rb b/app/controllers/schemes_controller.rb index a433832b5..e026a70b9 100644 --- a/app/controllers/schemes_controller.rb +++ b/app/controllers/schemes_controller.rb @@ -51,7 +51,7 @@ class SchemesController < ApplicationController end def deactivate_confirm - @affected_logs = @scheme.lettings_logs.visible.filter_by_before_startdate(params[:deactivation_date]) + @affected_logs = @scheme.lettings_logs.visible.after_date(params[:deactivation_date]) if @affected_logs.count.zero? deactivate else @@ -335,7 +335,7 @@ private end def reset_location_and_scheme_for_logs! - logs = @scheme.lettings_logs.visible.filter_by_before_startdate(params[:deactivation_date].to_time) + logs = @scheme.lettings_logs.visible.after_date(params[:deactivation_date].to_time) logs.update!(location: nil, scheme: nil, unresolved: true) logs end diff --git a/app/models/lettings_log.rb b/app/models/lettings_log.rb index 0ac0e4bf5..bc4a44a5f 100644 --- a/app/models/lettings_log.rb +++ b/app/models/lettings_log.rb @@ -50,7 +50,7 @@ class LettingsLog < Log .or(filter_by_postcode(param)) .or(filter_by_id(param)) } - scope :filter_by_before_startdate, ->(date) { where("lettings_logs.startdate >= ?", date) } + scope :after_date, ->(date) { where("lettings_logs.startdate >= ?", date) } scope :unresolved, -> { where(unresolved: true) } scope :filter_by_organisation, ->(org, _user = nil) { where(owning_organisation: org).or(where(managing_organisation: org)) } diff --git a/app/models/sales_log.rb b/app/models/sales_log.rb index fc57f3ba3..2a0a6d5c4 100644 --- a/app/models/sales_log.rb +++ b/app/models/sales_log.rb @@ -51,6 +51,7 @@ class SalesLog < Log .where.not(postcode_full: nil) .where("age1 IS NOT NULL OR age1_known = 1 OR age1_known = 2") } + scope :after_date, ->(date) { where("saledate >= ?", date) } OPTIONAL_FIELDS = %w[purchid othtype].freeze RETIREMENT_AGES = { "M" => 65, "F" => 60, "X" => 65 }.freeze diff --git a/app/services/merge/merge_organisations_service.rb b/app/services/merge/merge_organisations_service.rb new file mode 100644 index 000000000..5a417ef02 --- /dev/null +++ b/app/services/merge/merge_organisations_service.rb @@ -0,0 +1,131 @@ +class Merge::MergeOrganisationsService + def initialize(absorbing_organisation_id:, merging_organisation_ids:) + @absorbing_organisation = Organisation.find(absorbing_organisation_id) + @merging_organisations = Organisation.find(merging_organisation_ids) + end + + def call + ActiveRecord::Base.transaction do + @merged_users = {} + @merged_schemes = {} + merge_organisation_details + @merging_organisations.each do |merging_organisation| + merge_rent_periods(merging_organisation) + merge_organisation_relationships(merging_organisation) + merge_users(merging_organisation) + merge_schemes_and_locations(merging_organisation) + merge_lettings_logs(merging_organisation) + merge_sales_logs(merging_organisation) + mark_organisation_as_merged(merging_organisation) + end + @absorbing_organisation.save! + log_success_message + rescue ActiveRecord::RecordInvalid => e + Rails.logger.error("Organisation merge failed with: #{e.message}") + raise ActiveRecord::Rollback + end + end + +private + + def merge_organisation_details + @absorbing_organisation.holds_own_stock = merge_boolean_organisation_attribute("holds_own_stock") + end + + def merge_rent_periods(merging_organisation) + merging_organisation.rent_periods.each do |rent_period| + @absorbing_organisation.organisation_rent_periods << OrganisationRentPeriod.new(rent_period:) unless @absorbing_organisation.rent_periods.include?(rent_period) + end + end + + def merge_organisation_relationships(merging_organisation) + merging_organisation.parent_organisation_relationships.each do |parent_organisation_relationship| + if parent_relationship_exists_on_absorbing_organisation?(parent_organisation_relationship) + parent_organisation_relationship.destroy! + else + parent_organisation_relationship.update!(child_organisation: @absorbing_organisation) + end + end + merging_organisation.child_organisation_relationships.each do |child_organisation_relationship| + if child_relationship_exists_on_absorbing_organisation?(child_organisation_relationship) + child_organisation_relationship.destroy! + else + child_organisation_relationship.update!(parent_organisation: @absorbing_organisation) + end + end + end + + def merge_users(merging_organisation) + @merged_users[merging_organisation.name] = merging_organisation.users.map { |user| { name: user.name, email: user.email } } + merging_organisation.users.update_all(organisation_id: @absorbing_organisation.id) + end + + def merge_schemes_and_locations(merging_organisation) + @merged_schemes[merging_organisation.name] = [] + merging_organisation.owned_schemes.each do |scheme| + next if scheme.deactivated? + + new_scheme = Scheme.create!(scheme.attributes.except("id", "owning_organisation_id").merge(owning_organisation: @absorbing_organisation)) + scheme.locations.each do |location| + new_scheme.locations << Location.new(location.attributes.except("id", "scheme_id")) unless location.deactivated? + end + @merged_schemes[merging_organisation.name] << { name: new_scheme.service_name, code: new_scheme.id } + SchemeDeactivationPeriod.create!(scheme:, deactivation_date: Time.zone.now) + end + end + + def merge_lettings_logs(merging_organisation) + merging_organisation.owned_lettings_logs.after_date(Time.zone.today).each do |lettings_log| + if lettings_log.scheme.present? + scheme_to_set = @absorbing_organisation.owned_schemes.find_by(service_name: lettings_log.scheme.service_name) + location_to_set = scheme_to_set.locations.find_by(name: lettings_log.location&.name, postcode: lettings_log.location&.postcode) + + lettings_log.scheme = scheme_to_set if scheme_to_set.present? + lettings_log.location = location_to_set if location_to_set.present? + end + lettings_log.owning_organisation = @absorbing_organisation + lettings_log.save! + end + merging_organisation.managed_lettings_logs.after_date(Time.zone.today).each do |lettings_log| + lettings_log.managing_organisation = @absorbing_organisation + lettings_log.save! + end + end + + def merge_sales_logs(merging_organisation) + merging_organisation.sales_logs.after_date(Time.zone.today).each do |sales_log| + sales_log.update(owning_organisation: @absorbing_organisation) + end + end + + def mark_organisation_as_merged(merging_organisation) + merging_organisation.update(merge_date: Time.zone.today, absorbing_organisation: @absorbing_organisation) + end + + def log_success_message + @merged_users.each do |organisation_name, users| + Rails.logger.info("Merged users from #{organisation_name}:") + users.each do |user| + Rails.logger.info("\t#{user[:name]} (#{user[:email]})") + end + end + @merged_schemes.each do |organisation_name, schemes| + Rails.logger.info("New schemes from #{organisation_name}:") + schemes.each do |scheme| + Rails.logger.info("\t#{scheme[:name]} (S#{scheme[:code]})") + end + end + end + + def merge_boolean_organisation_attribute(attribute) + @absorbing_organisation[attribute] ||= @merging_organisations.any? { |merging_organisation| merging_organisation[attribute] } + end + + def parent_relationship_exists_on_absorbing_organisation?(parent_organisation_relationship) + parent_organisation_relationship.parent_organisation == @absorbing_organisation || @absorbing_organisation.parent_organisation_relationships.where(parent_organisation: parent_organisation_relationship.parent_organisation).exists? + end + + def child_relationship_exists_on_absorbing_organisation?(child_organisation_relationship) + child_organisation_relationship.child_organisation == @absorbing_organisation || @absorbing_organisation.child_organisation_relationships.where(child_organisation: child_organisation_relationship.child_organisation).exists? + end +end diff --git a/db/schema.rb b/db/schema.rb index ee5c1c1ad..7b5167a24 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_07_25_081029) do +ActiveRecord::Schema[7.0].define(version: 2023_07_19_150610) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" diff --git a/lib/tasks/merge_organisations.rake b/lib/tasks/merge_organisations.rake new file mode 100644 index 000000000..820343883 --- /dev/null +++ b/lib/tasks/merge_organisations.rake @@ -0,0 +1,12 @@ +namespace :merge do + desc "Merge organisations into one" + task :merge_organisations, %i[absorbing_organisation_id merging_organisation_ids] => :environment do |_task, args| + absorbing_organisation_id = args[:absorbing_organisation_id] + merging_organisation_ids = args[:merging_organisation_ids]&.split(" ")&.map(&:to_i) + + raise "Usage: rake merge:merge_organisations[absorbing_organisation_id, merging_organisation_ids]" if merging_organisation_ids.blank? || absorbing_organisation_id.blank? + + service = Merge::MergeOrganisationsService.new(absorbing_organisation_id:, merging_organisation_ids:) + service.call + end +end diff --git a/spec/lib/tasks/merge_organisations_spec.rb b/spec/lib/tasks/merge_organisations_spec.rb new file mode 100644 index 000000000..4155a4389 --- /dev/null +++ b/spec/lib/tasks/merge_organisations_spec.rb @@ -0,0 +1,41 @@ +require "rails_helper" +require "rake" + +RSpec.describe "emails" do + describe ":merge_organisations", type: :task do + subject(:task) { Rake::Task["merge:merge_organisations"] } + + let(:organisation) { create(:organisation) } + let(:merging_organisation) { create(:organisation) } + + let(:merge_organisations_service) { Merge::MergeOrganisationsService.new(absorbing_organisation_id: organisation.id, merging_organisation_ids: [merging_organisation.id]) } + + before do + allow(Merge::MergeOrganisationsService).to receive(:new).and_return(merge_organisations_service) + allow(merge_organisations_service).to receive(:call).and_return(nil) + Rake.application.rake_require("tasks/merge_organisations") + Rake::Task.define_task(:environment) + task.reenable + end + + context "when the rake task is run" do + it "raises an error when no parameters are given" do + expect { task.invoke(nil) }.to raise_error(RuntimeError, "Usage: rake merge:merge_organisations[absorbing_organisation_id, merging_organisation_ids]") + end + + it "raises an error when only absorbing organisation is given" do + expect { task.invoke(1, nil) }.to raise_error(RuntimeError, "Usage: rake merge:merge_organisations[absorbing_organisation_id, merging_organisation_ids]") + end + + it "raises an error when only merging organisations are given" do + expect { task.invoke(nil, "1 2") }.to raise_error(RuntimeError, "Usage: rake merge:merge_organisations[absorbing_organisation_id, merging_organisation_ids]") + end + + it "raises runs the service with correct organisation IDs" do + expect(Merge::MergeOrganisationsService).to receive(:new).with(absorbing_organisation_id: 1, merging_organisation_ids: [2, 3]).once + expect(merge_organisations_service).to receive(:call).once + task.invoke(1, "2 3") + end + end + end +end diff --git a/spec/services/merge/merge_organisations_service_spec.rb b/spec/services/merge/merge_organisations_service_spec.rb new file mode 100644 index 000000000..2c2af0f3e --- /dev/null +++ b/spec/services/merge/merge_organisations_service_spec.rb @@ -0,0 +1,323 @@ +require "rails_helper" + +RSpec.describe Merge::MergeOrganisationsService do + subject(:merge_organisations_service) { described_class.new(absorbing_organisation_id: absorbing_organisation.id, merging_organisation_ids: [merging_organisation_ids]) } + + let(:absorbing_organisation) { create(:organisation, holds_own_stock: false) } + let(:absorbing_organisation_user) { create(:user, organisation: absorbing_organisation) } + + describe "#call" do + context "when merging a single organisation into an existing organisation" do + let(:merging_organisation) { create(:organisation, holds_own_stock: true, name: "fake org") } + + let(:merging_organisation_ids) { [merging_organisation.id] } + let!(:merging_organisation_user) { create(:user, organisation: merging_organisation, name: "fake name", email: "fake@email.com") } + + it "moves the users from merging organisation to absorbing organisation" do + expect(Rails.logger).to receive(:info).with("Merged users from fake org:") + expect(Rails.logger).to receive(:info).with("\tDanny Rojas (#{merging_organisation.data_protection_officers.first.email})") + expect(Rails.logger).to receive(:info).with("\tfake name (fake@email.com)") + expect(Rails.logger).to receive(:info).with("New schemes from fake org:") + merge_organisations_service.call + + merging_organisation_user.reload + expect(merging_organisation_user.organisation).to eq(absorbing_organisation) + end + + it "sets merge date on merged organisation" do + merge_organisations_service.call + + merging_organisation.reload + expect(merging_organisation.merge_date.to_date).to eq(Time.zone.today) + expect(merging_organisation.absorbing_organisation_id).to eq(absorbing_organisation.id) + end + + it "combines organisation data" do + merge_organisations_service.call + + absorbing_organisation.reload + expect(absorbing_organisation.holds_own_stock).to eq(true) + end + + it "rolls back if there's an error" do + allow(Organisation).to receive(:find).with([merging_organisation_ids]).and_return(Organisation.find(merging_organisation_ids)) + allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation) + allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid) + expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid") + merge_organisations_service.call + + absorbing_organisation.reload + merging_organisation.reload + expect(absorbing_organisation.holds_own_stock).to eq(false) + expect(merging_organisation.merge_date).to eq(nil) + expect(merging_organisation.absorbing_organisation_id).to eq(nil) + expect(merging_organisation_user.organisation).to eq(merging_organisation) + end + + context "and merging organisation rent periods" do + before do + OrganisationRentPeriod.create!(organisation: absorbing_organisation, rent_period: 1) + OrganisationRentPeriod.create!(organisation: absorbing_organisation, rent_period: 3) + OrganisationRentPeriod.create!(organisation: merging_organisation, rent_period: 1) + OrganisationRentPeriod.create!(organisation: merging_organisation, rent_period: 2) + end + + it "combines organisation rent periods" do + expect(absorbing_organisation.rent_periods.count).to eq(2) + merge_organisations_service.call + + absorbing_organisation.reload + expect(absorbing_organisation.rent_periods.count).to eq(3) + expect(absorbing_organisation.rent_periods).to include(1) + expect(absorbing_organisation.rent_periods).to include(2) + expect(absorbing_organisation.rent_periods).to include(3) + end + + it "rolls back if there's an error" do + allow(Organisation).to receive(:find).with([merging_organisation_ids]).and_return(Organisation.find(merging_organisation_ids)) + allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation) + allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid) + expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid") + merge_organisations_service.call + + absorbing_organisation.reload + merging_organisation.reload + expect(absorbing_organisation.rent_periods.count).to eq(2) + expect(merging_organisation.rent_periods.count).to eq(2) + end + end + + context "and merging organisation relationships" do + let(:other_organisation) { create(:organisation) } + let!(:merging_organisation_relationship) { create(:organisation_relationship, parent_organisation: merging_organisation) } + let!(:absorbing_organisation_relationship) { create(:organisation_relationship, parent_organisation: absorbing_organisation) } + + before do + create(:organisation_relationship, parent_organisation: absorbing_organisation, child_organisation: merging_organisation) + create(:organisation_relationship, parent_organisation: merging_organisation, child_organisation: other_organisation) + create(:organisation_relationship, parent_organisation: absorbing_organisation, child_organisation: other_organisation) + end + + it "combines organisation relationships" do + merge_organisations_service.call + + absorbing_organisation.reload + expect(absorbing_organisation.child_organisations).to include(other_organisation) + expect(absorbing_organisation.child_organisations).to include(absorbing_organisation_relationship.child_organisation) + expect(absorbing_organisation.child_organisations).to include(merging_organisation_relationship.child_organisation) + expect(absorbing_organisation.child_organisations).not_to include(merging_organisation) + expect(absorbing_organisation.parent_organisations.count).to eq(0) + expect(absorbing_organisation.child_organisations.count).to eq(3) + end + + it "rolls back if there's an error" do + allow(Organisation).to receive(:find).with([merging_organisation_ids]).and_return(Organisation.find(merging_organisation_ids)) + allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation) + allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid) + expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid") + merge_organisations_service.call + + absorbing_organisation.reload + expect(absorbing_organisation.child_organisations.count).to eq(3) + expect(absorbing_organisation.child_organisations).to include(other_organisation) + expect(absorbing_organisation.child_organisations).to include(merging_organisation) + expect(absorbing_organisation.child_organisations).to include(absorbing_organisation_relationship.child_organisation) + end + end + + context "and merging organisation schemes and locations" do + let!(:scheme) { create(:scheme, owning_organisation: merging_organisation) } + let!(:location) { create(:location, scheme:) } + let!(:deactivated_location) { create(:location, scheme:) } + let!(:deactivated_scheme) { create(:scheme, owning_organisation: merging_organisation) } + let!(:owned_lettings_log) { create(:lettings_log, :sh, scheme:, location:, startdate: Time.zone.tomorrow, owning_organisation: merging_organisation) } + let!(:owned_lettings_log_no_location) { create(:lettings_log, :sh, scheme:, startdate: Time.zone.tomorrow, owning_organisation: merging_organisation) } + + before do + create(:location, scheme:, name: "fake location", postcode: "A1 1AA") + create(:location, scheme: deactivated_scheme) + create(:scheme_deactivation_period, scheme: deactivated_scheme, deactivation_date: Time.zone.today - 1.month) + create(:location_deactivation_period, location: deactivated_location, deactivation_date: Time.zone.today - 1.month) + create(:lettings_log, scheme:, location:, startdate: Time.zone.yesterday) + create(:lettings_log, startdate: Time.zone.tomorrow, managing_organisation: merging_organisation) + end + + it "combines organisation schemes and locations" do + expect(Rails.logger).to receive(:info).with("Merged users from fake org:") + expect(Rails.logger).to receive(:info).with("\tDanny Rojas (#{merging_organisation.data_protection_officers.first.email})") + expect(Rails.logger).to receive(:info).with("\tfake name (fake@email.com)") + expect(Rails.logger).to receive(:info).with("New schemes from fake org:") + expect(Rails.logger).to receive(:info).with(/\t#{scheme.service_name} \(S/) + merge_organisations_service.call + + absorbing_organisation.reload + expect(absorbing_organisation.owned_schemes.count).to eq(1) + expect(absorbing_organisation.owned_schemes.first.service_name).to eq(scheme.service_name) + expect(absorbing_organisation.owned_schemes.first.locations.count).to eq(2) + expect(absorbing_organisation.owned_schemes.first.locations.first.postcode).to eq(location.postcode) + expect(scheme.scheme_deactivation_periods.count).to eq(1) + expect(scheme.scheme_deactivation_periods.first.deactivation_date.to_date).to eq(Time.zone.today) + end + + it "moves relevant logs and assigns the new scheme" do + merge_organisations_service.call + + absorbing_organisation.reload + merging_organisation.reload + expect(absorbing_organisation.owned_lettings_logs.count).to eq(2) + expect(absorbing_organisation.managed_lettings_logs.count).to eq(1) + expect(absorbing_organisation.owned_lettings_logs.find(owned_lettings_log.id).scheme).to eq(absorbing_organisation.owned_schemes.first) + expect(absorbing_organisation.owned_lettings_logs.find(owned_lettings_log.id).location).to eq(absorbing_organisation.owned_schemes.first.locations.first) + expect(absorbing_organisation.owned_lettings_logs.find(owned_lettings_log_no_location.id).scheme).to eq(absorbing_organisation.owned_schemes.first) + expect(absorbing_organisation.owned_lettings_logs.find(owned_lettings_log_no_location.id).location).to eq(nil) + end + + it "rolls back if there's an error" do + allow(Organisation).to receive(:find).with([merging_organisation_ids]).and_return(Organisation.find(merging_organisation_ids)) + allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation) + allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid) + expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid") + merge_organisations_service.call + + absorbing_organisation.reload + merging_organisation.reload + expect(absorbing_organisation.owned_schemes.count).to eq(0) + expect(scheme.scheme_deactivation_periods.count).to eq(0) + expect(owned_lettings_log.owning_organisation).to eq(merging_organisation) + expect(owned_lettings_log_no_location.owning_organisation).to eq(merging_organisation) + end + end + + context "and merging sales logs" do + let!(:sales_log) { create(:sales_log, saledate: Time.zone.tomorrow, owning_organisation: merging_organisation) } + + before do + create(:sales_log, saledate: Time.zone.yesterday, owning_organisation: merging_organisation) + end + + it "moves relevant logs" do + merge_organisations_service.call + + absorbing_organisation.reload + expect(SalesLog.filter_by_owning_organisation(absorbing_organisation).count).to eq(1) + expect(SalesLog.filter_by_owning_organisation(absorbing_organisation).first).to eq(sales_log) + end + + it "rolls back if there's an error" do + allow(Organisation).to receive(:find).with([merging_organisation_ids]).and_return(Organisation.find(merging_organisation_ids)) + allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation) + allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid) + expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid") + merge_organisations_service.call + + absorbing_organisation.reload + expect(absorbing_organisation.sales_logs.count).to eq(0) + expect(sales_log.owning_organisation).to eq(merging_organisation) + end + end + end + + context "when merging a multiple organisations into an existing organisation" do + let(:merging_organisation) { create(:organisation, holds_own_stock: true, name: "fake org") } + let(:merging_organisation_too) { create(:organisation, holds_own_stock: true, name: "second org") } + + let(:merging_organisation_ids) { [merging_organisation.id, merging_organisation_too.id] } + let!(:merging_organisation_user) { create(:user, organisation: merging_organisation, name: "fake name", email: "fake@email.com") } + + before do + create_list(:user, 5, organisation: merging_organisation_too) + end + + it "moves the users from merging organisations to absorbing organisation" do + expect(Rails.logger).to receive(:info).with("Merged users from fake org:") + expect(Rails.logger).to receive(:info).with("\tDanny Rojas (#{merging_organisation.data_protection_officers.first.email})") + expect(Rails.logger).to receive(:info).with("\tfake name (fake@email.com)") + expect(Rails.logger).to receive(:info).with("Merged users from second org:") + expect(Rails.logger).to receive(:info).with(/\tDanny Rojas/).exactly(6).times + expect(Rails.logger).to receive(:info).with("New schemes from fake org:") + expect(Rails.logger).to receive(:info).with("New schemes from second org:") + merge_organisations_service.call + + merging_organisation_user.reload + expect(merging_organisation_user.organisation).to eq(absorbing_organisation) + end + + it "sets merge date and absorbing organisation on merged organisations" do + merge_organisations_service.call + + merging_organisation.reload + merging_organisation_too.reload + expect(merging_organisation.merge_date.to_date).to eq(Time.zone.today) + expect(merging_organisation.absorbing_organisation_id).to eq(absorbing_organisation.id) + expect(merging_organisation_too.merge_date.to_date).to eq(Time.zone.today) + expect(merging_organisation_too.absorbing_organisation_id).to eq(absorbing_organisation.id) + end + + it "combines organisation data" do + merge_organisations_service.call + + absorbing_organisation.reload + expect(absorbing_organisation.holds_own_stock).to eq(true) + end + + it "rolls back if there's an error" do + allow(Organisation).to receive(:find).with([merging_organisation_ids]).and_return(Organisation.find(merging_organisation_ids)) + allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation) + allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid) + expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid") + merge_organisations_service.call + + absorbing_organisation.reload + merging_organisation.reload + expect(absorbing_organisation.holds_own_stock).to eq(false) + expect(merging_organisation.merge_date).to eq(nil) + expect(merging_organisation.absorbing_organisation_id).to eq(nil) + expect(merging_organisation_user.organisation).to eq(merging_organisation) + end + + context "and merging organisation relationships" do + let(:other_organisation) { create(:organisation) } + let!(:merging_organisation_relationship) { create(:organisation_relationship, parent_organisation: merging_organisation) } + let!(:absorbing_organisation_relationship) { create(:organisation_relationship, parent_organisation: absorbing_organisation) } + + before do + create(:organisation_relationship, parent_organisation: merging_organisation, child_organisation: absorbing_organisation) + create(:organisation_relationship, parent_organisation: merging_organisation, child_organisation: other_organisation) + create(:organisation_relationship, parent_organisation: absorbing_organisation, child_organisation: other_organisation) + create(:organisation_relationship, parent_organisation: merging_organisation, child_organisation: merging_organisation_too) + end + + it "combines organisation relationships" do + merge_organisations_service.call + + absorbing_organisation.reload + expect(absorbing_organisation.child_organisations).to include(other_organisation) + expect(absorbing_organisation.child_organisations).to include(absorbing_organisation_relationship.child_organisation) + expect(absorbing_organisation.child_organisations).to include(merging_organisation_relationship.child_organisation) + expect(absorbing_organisation.child_organisations).not_to include(merging_organisation) + expect(absorbing_organisation.parent_organisations).not_to include(merging_organisation) + expect(absorbing_organisation.child_organisations).not_to include(merging_organisation_too) + expect(absorbing_organisation.parent_organisations).not_to include(merging_organisation_too) + expect(absorbing_organisation.parent_organisations.count).to eq(0) + expect(absorbing_organisation.child_organisations.count).to eq(3) + end + + it "rolls back if there's an error" do + allow(Organisation).to receive(:find).with([merging_organisation_ids]).and_return(Organisation.find(merging_organisation_ids)) + allow(Organisation).to receive(:find).with(absorbing_organisation.id).and_return(absorbing_organisation) + allow(absorbing_organisation).to receive(:save!).and_raise(ActiveRecord::RecordInvalid) + expect(Rails.logger).to receive(:error).with("Organisation merge failed with: Record invalid") + merge_organisations_service.call + + absorbing_organisation.reload + merging_organisation.reload + expect(absorbing_organisation.child_organisations.count).to eq(2) + expect(absorbing_organisation.parent_organisations.count).to eq(1) + expect(absorbing_organisation.child_organisations).to include(other_organisation) + expect(absorbing_organisation.parent_organisations).to include(merging_organisation) + expect(absorbing_organisation.child_organisations).to include(absorbing_organisation_relationship.child_organisation) + end + end + end + end +end From 364fb1e198c911582a85a681c4a538d691ceb38e Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Wed, 2 Aug 2023 14:08:18 +0100 Subject: [PATCH 06/12] CLDC-2554 Update back to all logs link (#1811) * Update resume link * Add tests * Update copy --- app/views/bulk_upload_lettings_results/resume.html.erb | 2 +- app/views/bulk_upload_sales_results/resume.html.erb | 2 +- spec/requests/lettings_logs_controller_spec.rb | 7 +++++++ spec/requests/sales_logs_controller_spec.rb | 7 +++++++ 4 files changed, 16 insertions(+), 2 deletions(-) diff --git a/app/views/bulk_upload_lettings_results/resume.html.erb b/app/views/bulk_upload_lettings_results/resume.html.erb index a5af1bd9d..15a670b0f 100644 --- a/app/views/bulk_upload_lettings_results/resume.html.erb +++ b/app/views/bulk_upload_lettings_results/resume.html.erb @@ -8,4 +8,4 @@ You’ve completed all the logs that had errors from your bulk upload.

-<%= govuk_button_link_to "Back to all logs", lettings_logs_path, button: true %> +<%= govuk_button_link_to "Return to lettings logs", clear_filters_path(filter_type: "lettings_logs"), button: true %> diff --git a/app/views/bulk_upload_sales_results/resume.html.erb b/app/views/bulk_upload_sales_results/resume.html.erb index f0e243f27..cd5b4a755 100644 --- a/app/views/bulk_upload_sales_results/resume.html.erb +++ b/app/views/bulk_upload_sales_results/resume.html.erb @@ -8,4 +8,4 @@ You’ve completed all the logs that had errors from your bulk upload.

-<%= govuk_button_link_to "Back to all logs", sales_logs_path, button: true %> +<%= govuk_button_link_to "Return to sales logs", clear_filters_path(filter_type: "sales_logs"), button: true %> diff --git a/spec/requests/lettings_logs_controller_spec.rb b/spec/requests/lettings_logs_controller_spec.rb index 4e4cb96c2..6cf53acc9 100644 --- a/spec/requests/lettings_logs_controller_spec.rb +++ b/spec/requests/lettings_logs_controller_spec.rb @@ -562,6 +562,13 @@ RSpec.describe LettingsLogsController, type: :request do expect(response).to redirect_to(resume_bulk_upload_lettings_result_path(bulk_upload)) end + + it "allows returning to all logs" do + get "/lettings-logs?bulk_upload_id[]=#{bulk_upload.id}" + + follow_redirect! + expect(page).to have_link("Return to lettings logs", href: clear_filters_path(filter_type: "lettings_logs")) + end end end diff --git a/spec/requests/sales_logs_controller_spec.rb b/spec/requests/sales_logs_controller_spec.rb index e4e8322e7..41e3bb1d2 100644 --- a/spec/requests/sales_logs_controller_spec.rb +++ b/spec/requests/sales_logs_controller_spec.rb @@ -438,6 +438,13 @@ RSpec.describe SalesLogsController, type: :request do expect(response).to redirect_to(resume_bulk_upload_sales_result_path(bulk_upload)) end + + it "allows returning to all logs" do + get "/sales-logs?bulk_upload_id[]=#{bulk_upload.id}" + + follow_redirect! + expect(page).to have_link("Return to sales logs", href: clear_filters_path(filter_type: "sales_logs")) + end end end From f66cef8f05aca1fa152d01a8ec631c067222bcbc Mon Sep 17 00:00:00 2001 From: natdeanlewissoftwire <94526761+natdeanlewissoftwire@users.noreply.github.com> Date: Wed, 2 Aug 2023 16:58:30 +0100 Subject: [PATCH 07/12] feat: update helpdesk link (#1800) --- app/views/organisations/show.html.erb | 2 +- spec/requests/organisations_controller_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/views/organisations/show.html.erb b/app/views/organisations/show.html.erb index cc504c9f5..47b023fbd 100644 --- a/app/views/organisations/show.html.erb +++ b/app/views/organisations/show.html.erb @@ -37,7 +37,7 @@ <%= data_sharing_agreement_row(organisation: @organisation, user: current_user, summary_list:) %> <% end %> <% if FeatureToggle.merge_organisations_enabled? %> -

Is your organisation merging with another? <%= govuk_link_to "Let us know using this form", merge_request_organisation_path(@organisation) %>

+

To report a merge or update your organisation details, <%= govuk_link_to "contact the helpdesk", "https://dluhcdigital.atlassian.net/servicedesk/customer/portal/6/group/11" %>.

<% end %>
diff --git a/spec/requests/organisations_controller_spec.rb b/spec/requests/organisations_controller_spec.rb index 896f760ba..bacb5ca33 100644 --- a/spec/requests/organisations_controller_spec.rb +++ b/spec/requests/organisations_controller_spec.rb @@ -268,8 +268,8 @@ RSpec.describe OrganisationsController, type: :request do end it "displays a link to merge organisations" do - expect(page).to have_content("Is your organisation merging with another?") - expect(page).to have_link("Let us know using this form", href: "/organisations/#{organisation.id}/merge-request") + expect(page).to have_content("To report a merge or update your organisation details, ") + expect(page).to have_link("contact the helpdesk", href: "https://dluhcdigital.atlassian.net/servicedesk/customer/portal/6/group/11") end end From fb389414a0449e7e7698dd6f4d8ed15f29d008af Mon Sep 17 00:00:00 2001 From: natdeanlewissoftwire <94526761+natdeanlewissoftwire@users.noreply.github.com> Date: Thu, 3 Aug 2023 10:24:57 +0100 Subject: [PATCH 08/12] CLDC-2504 Add owning org select to sales if there are stock owning absorbed orgs (#1815) * feat: wip update sales org select * feat: update routing and hidden in check answers methods * feat: set sales org id as derived * feat: update tests * refactor: linting * feat: update tests and radio options * feat: update test * feat: add value helper test for non-constant value question * refactor: lint * feat: freeze time in unrelated test * refactor: linting --- app/models/form/lettings/pages/stock_owner.rb | 5 +- .../form/lettings/questions/stock_owner.rb | 2 +- app/models/form/question.rb | 27 ++-- app/models/form/sales/pages/organisation.rb | 23 +++- .../sales/questions/owning_organisation_id.rb | 50 +++++-- .../form/sales/questions/postcode_known.rb | 1 + .../completed_2022_23_sales_bulk_upload.csv | 2 +- spec/models/form/question_spec.rb | 52 +++++--- .../form/sales/pages/organisation_spec.rb | 126 ++++++++++++++++-- .../questions/owning_organisation_id_spec.rb | 90 ++++++++++++- .../forms/bulk_upload_sales/year_spec.rb | 6 + spec/requests/sales_logs_controller_spec.rb | 2 + .../bulk_upload/sales/validator_spec.rb | 4 +- .../sales/year2022/row_parser_spec.rb | 4 +- .../sales/year2023/row_parser_spec.rb | 4 +- 15 files changed, 337 insertions(+), 61 deletions(-) diff --git a/app/models/form/lettings/pages/stock_owner.rb b/app/models/form/lettings/pages/stock_owner.rb index d92824f0e..dee5d5f71 100644 --- a/app/models/form/lettings/pages/stock_owner.rb +++ b/app/models/form/lettings/pages/stock_owner.rb @@ -14,9 +14,12 @@ class Form::Lettings::Pages::StockOwner < ::Form::Page return false unless current_user return true if current_user.support? - stock_owners = current_user.organisation.stock_owners + stock_owners = current_user.organisation.stock_owners + current_user.organisation.absorbed_organisations.where(holds_own_stock: true) if current_user.organisation.holds_own_stock? + if current_user.organisation.absorbed_organisations.any?(&:holds_own_stock?) + return true + end return true if stock_owners.count >= 1 log.update!(owning_organisation: current_user.organisation) diff --git a/app/models/form/lettings/questions/stock_owner.rb b/app/models/form/lettings/questions/stock_owner.rb index dc6179b21..ef01e6538 100644 --- a/app/models/form/lettings/questions/stock_owner.rb +++ b/app/models/form/lettings/questions/stock_owner.rb @@ -49,7 +49,7 @@ class Form::Lettings::Questions::StockOwner < ::Form::Question def hidden_in_check_answers?(_log, user = nil) return false if user.support? - stock_owners = user.organisation.stock_owners + stock_owners = user.organisation.stock_owners + user.organisation.absorbed_organisations.where(holds_own_stock: true) if user.organisation.holds_own_stock? stock_owners.count.zero? diff --git a/app/models/form/question.rb b/app/models/form/question.rb index 2f09a061d..c27dca9c2 100644 --- a/app/models/form/question.rb +++ b/app/models/form/question.rb @@ -116,20 +116,20 @@ class Form::Question if is_derived_or_has_inferred_check_answers_value?(log) "Change" elsif type == "checkbox" - answer_options.keys.any? { |key| value_is_yes?(log[key]) } ? "Change" : "Answer" + answer_options.keys.any? { |key| value_is_yes?(log[key], log.lettings?) } ? "Change" : "Answer" else log[id].blank? ? "Answer" : "Change" end end def unanswered?(log) - return answer_options.keys.none? { |key| value_is_yes?(log[key]) } if type == "checkbox" + return answer_options.keys.none? { |key| value_is_yes?(log[key], log.lettings?) } if type == "checkbox" log[id].blank? end def completed?(log) - return answer_options.keys.any? { |key| value_is_yes?(log[key]) } if type == "checkbox" + return answer_options.keys.any? { |key| value_is_yes?(log[key], log.lettings?) } if type == "checkbox" log[id].present? || !log.respond_to?(id.to_sym) || has_inferred_display_value?(log) end @@ -166,23 +166,23 @@ class Form::Question label || value.to_s end - def value_is_yes?(value) + def value_is_yes?(value, is_lettings) case type when "checkbox" value == 1 when "radio" - RADIO_YES_VALUE[id.to_sym]&.include?(value) + is_lettings ? RADIO_YES_VALUE_LETTINGS[id.to_sym]&.include?(value) : RADIO_YES_VALUE_SALES[id.to_sym]&.include?(value) else %w[yes].include?(value.downcase) end end - def value_is_no?(value) + def value_is_no?(value, is_lettings) case type when "checkbox" value && value.zero? when "radio" - RADIO_NO_VALUE[id.to_sym]&.include?(value) + is_lettings ? RADIO_NO_VALUE_LETTINGS[id.to_sym]&.include?(value) : RADIO_NO_VALUE_SALES[id.to_sym]&.include?(value) else %w[no].include?(value.downcase) end @@ -272,9 +272,9 @@ private def checkbox_answer_label(log) answer = [] - return "Yes" if id == "declaration" && value_is_yes?(log["declaration"]) + return "Yes" if id == "declaration" && value_is_yes?(log["declaration"], log.lettings?) - answer_options.each { |key, options| value_is_yes?(log[key]) ? answer << options["value"] : nil } + answer_options.each { |key, options| value_is_yes?(log[key], log.lettings?) ? answer << options["value"] : nil } answer.join(", ") end @@ -319,7 +319,7 @@ private RADIO_YES_VALUE = { renewal: [1], postcode_known: [1], - ppcodenk: [1], + pcodenk: [0], previous_la_known: [1], first_time_property_let_as_social_housing: [1], wchair: [1], @@ -343,7 +343,7 @@ private RADIO_NO_VALUE = { renewal: [0], postcode_known: [0], - ppcodenk: [0], + pcodenk: [1], previous_la_known: [0], first_time_property_let_as_social_housing: [0], wchair: [0], @@ -364,6 +364,11 @@ private net_income_value_check: [1], }.freeze + RADIO_YES_VALUE_LETTINGS = RADIO_YES_VALUE.merge({ ppcodenk: [1] }) + RADIO_YES_VALUE_SALES = RADIO_YES_VALUE.merge({ ppcodenk: [0] }) + RADIO_NO_VALUE_LETTINGS = RADIO_NO_VALUE.merge({ ppcodenk: [0] }) + RADIO_NO_VALUE_SALES = RADIO_NO_VALUE.merge({ ppcodenk: [1] }) + RADIO_DONT_KNOW_VALUE = { sheltered: [3], underoccupation_benefitcap: [3], diff --git a/app/models/form/sales/pages/organisation.rb b/app/models/form/sales/pages/organisation.rb index 1d61b86ac..8f6728821 100644 --- a/app/models/form/sales/pages/organisation.rb +++ b/app/models/form/sales/pages/organisation.rb @@ -10,7 +10,26 @@ class Form::Sales::Pages::Organisation < ::Form::Page ] end - def routed_to?(_log, current_user) - !!current_user&.support? + def routed_to?(log, current_user) + return false unless current_user + return true if current_user.support? + + stock_owners = current_user.organisation.stock_owners + current_user.organisation.absorbed_organisations.where(holds_own_stock: true) + + if current_user.organisation.holds_own_stock? + if current_user.organisation.absorbed_organisations.any?(&:holds_own_stock?) + return true + end + return true if stock_owners.count >= 1 + + log.update!(owning_organisation: current_user.organisation) + else + return false if stock_owners.count.zero? + return true if stock_owners.count > 1 + + log.update!(owning_organisation: stock_owners.first) + end + + false end end diff --git a/app/models/form/sales/questions/owning_organisation_id.rb b/app/models/form/sales/questions/owning_organisation_id.rb index fa838f744..0db5ec1a0 100644 --- a/app/models/form/sales/questions/owning_organisation_id.rb +++ b/app/models/form/sales/questions/owning_organisation_id.rb @@ -7,37 +7,63 @@ class Form::Sales::Questions::OwningOrganisationId < ::Form::Question @type = "select" end - def answer_options + def answer_options(log = nil, user = nil) answer_opts = { "" => "Select an option" } + return answer_opts unless ActiveRecord::Base.connected? + return answer_opts unless user + return answer_opts unless log + + if log.owning_organisation_id.present? + answer_opts = answer_opts.merge({ log.owning_organisation.id => log.owning_organisation.name }) + end - Organisation.select(:id, :name).each_with_object(answer_opts) do |organisation, hsh| - hsh[organisation.id] = organisation.name - hsh + if !user.support? && user.organisation.holds_own_stock? + answer_opts[user.organisation.id] = "#{user.organisation.name} (Your organisation)" end + + user_answer_options = if user.support? + Organisation.where(holds_own_stock: true) + else + user.organisation.stock_owners + user.organisation.absorbed_organisations.where(holds_own_stock: true) + end.pluck(:id, :name).to_h + + answer_opts.merge(user_answer_options) end - def displayed_answer_options(_log, _user = nil) - answer_options + def displayed_answer_options(log, user = nil) + answer_options(log, user) end - def label_from_value(value, _log = nil, _user = nil) + def label_from_value(value, log = nil, user = nil) return unless value - answer_options[value] + answer_options(log, user)[value] + end + + def derived? + true end - def hidden_in_check_answers?(_log, current_user) - !current_user.support? + def hidden_in_check_answers?(_log, user = nil) + return false if user.support? + + stock_owners = user.organisation.stock_owners + user.organisation.absorbed_organisations.where(holds_own_stock: true) + + if user.organisation.holds_own_stock? + stock_owners.count.zero? + else + stock_owners.count <= 1 + end end - def derived? + def enabled true end private def selected_answer_option_is_derived?(_log) - false + true end end diff --git a/app/models/form/sales/questions/postcode_known.rb b/app/models/form/sales/questions/postcode_known.rb index 208f8df22..10a3e2765 100644 --- a/app/models/form/sales/questions/postcode_known.rb +++ b/app/models/form/sales/questions/postcode_known.rb @@ -19,6 +19,7 @@ class Form::Sales::Questions::PostcodeKnown < ::Form::Question }, ], } + @disable_clearing_if_not_routed_or_dynamic_answer_options = true end ANSWER_OPTIONS = { diff --git a/spec/fixtures/files/completed_2022_23_sales_bulk_upload.csv b/spec/fixtures/files/completed_2022_23_sales_bulk_upload.csv index 807a58710..1eb4a01f9 100644 --- a/spec/fixtures/files/completed_2022_23_sales_bulk_upload.csv +++ b/spec/fixtures/files/completed_2022_23_sales_bulk_upload.csv @@ -116,4 +116,4 @@ OR If field 39 = 3 - 9",If field 113 = 2 or 3,If field 113 = 1 or 3,If field 113 = 1 or 2 Bulk upload format and duplicate check,Yes,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,, Field number,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19,20,21,22,23,24,25,26,27,28,29,30,31,32,33,34,35,36,37,38,39,40,41,42,43,44,45,46,47,48,49,50,51,52,53,54,55,56,57,58,59,60,61,62,63,64,65,66,67,68,69,70,71,72,73,74,75,76,77,78,79,80,81,82,83,84,85,86,87,88,89,90,91,92,93,94,95,96,97,98,99,100,101,102,103,104,105,106,107,108,109,110,111,112,113,114,115,116,117,118,119,120,121,122,123,124,125 -,22 test BU,22,2,23,,1,32,32,,,,,M,F,,,,,R,,,,,1,2,,,,,12,18,30000,15000,1,1,20000,3,,1,E09000008,A1,1AA,1,,1,1,,3,3,2,1,1,E09000008,CR0,4BB,3,2,2,23,3,22,30,3,22,3,1,1,250000,25,42500,3,20000,,800,200,,,,,,,,,,,,,,,,,3,,,3,,5,1,,,,,,4,20,,,,2,5,1,1,1,,1,1,1,1,0,10,10,1,1,, +,22 test BU,22,2,23,,1,32,32,,,,,M,F,,,,,R,,,,,1,2,,,,,12,18,30000,15000,1,1,20000,3,,1,E09000008,A1,1AA,1,,1,1,,3,3,2,1,1,E09000008,CR0,4BB,3,2,2,23,3,22,30,3,22,3,1,1,250000,25,42500,3,20000,,800,200,,,,,,,,,,,,,,,,,123,,,3,,5,1,,,,,,4,20,,,,2,5,1,1,1,,1,1,1,1,0,10,10,1,1,, diff --git a/spec/models/form/question_spec.rb b/spec/models/form/question_spec.rb index 751523d37..08d08032f 100644 --- a/spec/models/form/question_spec.rb +++ b/spec/models/form/question_spec.rb @@ -60,15 +60,37 @@ RSpec.describe Form::Question, type: :model do end it "has a yes value helper" do - expect(question).to be_value_is_yes("Yes") - expect(question).to be_value_is_yes("YES") - expect(question).not_to be_value_is_yes("random") + expect(question).to be_value_is_yes("Yes", true) + expect(question).to be_value_is_yes("YES", true) + expect(question).not_to be_value_is_yes("random", true) end it "has a no value helper" do - expect(question).to be_value_is_no("No") - expect(question).to be_value_is_no("NO") - expect(question).not_to be_value_is_no("random") + expect(question).to be_value_is_no("No", true) + expect(question).to be_value_is_no("NO", true) + expect(question).not_to be_value_is_no("random", true) + end + + context "when there are different value helper values for lettings and sales" do + context "with a lettings log" do + let(:lettings_log) { FactoryBot.build(:lettings_log, :in_progress) } + let(:question) { Form::Lettings::Questions::Ppcodenk.new(nil, nil, Form::Lettings::Pages::PreviousPostcode.new("previous_postcode", nil, Form::Lettings::Subsections::HouseholdSituation.new(nil, nil, Form::Lettings::Sections::Household))) } + + it "has the correct values" do + expect(question.value_is_yes?(1, lettings_log.lettings?)).to be true + expect(question.value_is_no?(0, lettings_log.lettings?)).to be true + end + end + + context "with a sales log" do + let(:sales_log) { FactoryBot.build(:sales_log, :in_progress) } + let(:question) { Form::Sales::Questions::PreviousPostcodeKnown.new(nil, nil, Form::Sales::Pages::LastAccommodation.new("previous_postcode", nil, Form::Sales::Subsections::HouseholdSituation.new(nil, nil, Form::Sales::Sections::Household))) } + + it "has the correct values" do + expect(question.value_is_yes?(0, sales_log.lettings?)).to be true + expect(question.value_is_no?(1, sales_log.lettings?)).to be true + end + end end context "when type is numeric" do @@ -108,10 +130,10 @@ RSpec.describe Form::Question, type: :model do let(:question_id) { "illness" } it "maps those options" do - expect(question).to be_value_is_yes(1) - expect(question).not_to be_value_is_no(1) + expect(question).to be_value_is_yes(1, true) + expect(question).not_to be_value_is_no(1, true) expect(question).not_to be_value_is_refused(1) - expect(question).to be_value_is_no(2) + expect(question).to be_value_is_no(2, true) expect(question).to be_value_is_dont_know(3) end end @@ -123,8 +145,8 @@ RSpec.describe Form::Question, type: :model do let(:question_id) { "layear" } it "maps those options" do - expect(question).not_to be_value_is_yes(7) - expect(question).not_to be_value_is_no(7) + expect(question).not_to be_value_is_yes(7, true) + expect(question).not_to be_value_is_no(7, true) expect(question).not_to be_value_is_refused(7) expect(question).to be_value_is_dont_know(7) end @@ -209,13 +231,13 @@ RSpec.describe Form::Question, type: :model do end it "can map yes values" do - expect(question).to be_value_is_yes(1) - expect(question).not_to be_value_is_yes(0) + expect(question).to be_value_is_yes(1, true) + expect(question).not_to be_value_is_yes(0, true) end it "can map no values" do - expect(question).to be_value_is_no(0) - expect(question).not_to be_value_is_no(1) + expect(question).to be_value_is_no(0, true) + expect(question).not_to be_value_is_no(1, true) end end diff --git a/spec/models/form/sales/pages/organisation_spec.rb b/spec/models/form/sales/pages/organisation_spec.rb index 62156031f..710edbecc 100644 --- a/spec/models/form/sales/pages/organisation_spec.rb +++ b/spec/models/form/sales/pages/organisation_spec.rb @@ -7,7 +7,6 @@ RSpec.describe Form::Sales::Pages::Organisation, type: :model do let(:page_definition) { nil } let(:subsection) { instance_double(Form::Subsection) } let(:form) { instance_double(Form) } - let(:lettings_log) { instance_double(LettingsLog) } it "has correct subsection" do expect(page.subsection).to eq(subsection) @@ -33,19 +32,126 @@ RSpec.describe Form::Sales::Pages::Organisation, type: :model do expect(page.depends_on).to be nil end - context "when the current user is a support user" do - let(:support_user) { FactoryBot.build(:user, :support) } + describe "#routed_to?" do + let(:log) { create(:lettings_log, owning_organisation_id: nil) } - it "is shown" do - expect(page.routed_to?(lettings_log, support_user)).to be true + context "when user nil" do + it "is not shown" do + expect(page.routed_to?(log, nil)).to eq(false) + end + + it "does not update owning_organisation_id" do + expect { page.routed_to?(log, nil) }.not_to change(log.reload, :owning_organisation).from(nil) + end end - end - context "when the current user is not a support user" do - let(:user) { FactoryBot.build(:user) } + context "when support" do + let(:user) { create(:user, :support) } + + it "is shown" do + expect(page.routed_to?(log, user)).to eq(true) + end + + it "does not update owning_organisation_id" do + expect { page.routed_to?(log, user) }.not_to change(log.reload, :owning_organisation).from(nil) + end + end + + context "when not support" do + context "when does not hold own stock" do + let(:user) do + create(:user, :data_coordinator, organisation: create(:organisation, holds_own_stock: false)) + end + + context "with 0 stock_owners" do + it "is not shown" do + expect(page.routed_to?(log, user)).to eq(false) + end + + it "does not update owning_organisation_id" do + expect { page.routed_to?(log, user) }.not_to change(log.reload, :owning_organisation) + end + end + + context "with 1 stock_owners" do + let(:stock_owner) { create(:organisation) } + + before do + create( + :organisation_relationship, + child_organisation: user.organisation, + parent_organisation: stock_owner, + ) + end + + it "is not shown" do + expect(page.routed_to?(log, user)).to eq(false) + end + + it "updates owning_organisation_id" do + expect { page.routed_to?(log, user) }.to change(log.reload, :owning_organisation).from(nil).to(stock_owner) + end + end + + context "with >1 stock_owners" do + let(:stock_owner1) { create(:organisation) } + let(:stock_owner2) { create(:organisation) } + + before do + create( + :organisation_relationship, + child_organisation: user.organisation, + parent_organisation: stock_owner1, + ) + create( + :organisation_relationship, + child_organisation: user.organisation, + parent_organisation: stock_owner2, + ) + end + + it "is not shown" do + expect(page.routed_to?(log, user)).to eq(true) + end + + it "updates owning_organisation_id" do + expect { page.routed_to?(log, user) }.not_to change(log.reload, :owning_organisation) + end + end + end + + context "when holds own stock" do + let(:user) do + create(:user, :data_coordinator, organisation: create(:organisation, holds_own_stock: true)) + end + + context "with 0 stock_owners" do + it "is not shown" do + expect(page.routed_to?(log, user)).to eq(false) + end + + it "updates owning_organisation_id to user organisation" do + expect { + page.routed_to?(log, user) + }.to change(log.reload, :owning_organisation).from(nil).to(user.organisation) + end + end + + context "with >0 stock_owners" do + before do + create(:organisation_relationship, child_organisation: user.organisation) + create(:organisation_relationship, child_organisation: user.organisation) + end + + it "is shown" do + expect(page.routed_to?(log, user)).to eq(true) + end - it "is not shown" do - expect(page.routed_to?(lettings_log, user)).to be false + it "does not update owning_organisation_id" do + expect { page.routed_to?(log, user) }.not_to change(log.reload, :owning_organisation).from(nil) + end + end + end end end end diff --git a/spec/models/form/sales/questions/owning_organisation_id_spec.rb b/spec/models/form/sales/questions/owning_organisation_id_spec.rb index f6dc02c27..815a42319 100644 --- a/spec/models/form/sales/questions/owning_organisation_id_spec.rb +++ b/spec/models/form/sales/questions/owning_organisation_id_spec.rb @@ -3,6 +3,7 @@ require "rails_helper" RSpec.describe Form::Sales::Questions::OwningOrganisationId, type: :model do subject(:question) { described_class.new(question_id, question_definition, page) } + let(:user) { FactoryBot.create(:user, :data_coordinator) } let(:question_id) { nil } let(:question_definition) { nil } let(:page) { instance_double(Form::Page) } @@ -43,8 +44,93 @@ RSpec.describe Form::Sales::Questions::OwningOrganisationId, type: :model do expect(question.hint_text).to be_nil end - it "has the correct answer options" do - expect(question.answer_options).to eq(expected_answer_options) + describe "answer options" do + let(:options) { { "" => "Select an option" } } + + context "when current_user nil" do + it "shows default options" do + expect(question.answer_options).to eq(options) + end + end + + context "when user is not support" do + let(:user_org) { create(:organisation, name: "User org") } + let(:user) { create(:user, :data_coordinator, organisation: user_org) } + + let(:owning_org_1) { create(:organisation, name: "Owning org 1") } + let(:owning_org_2) { create(:organisation, name: "Owning org 2") } + let!(:org_rel) do + create(:organisation_relationship, child_organisation: user.organisation, parent_organisation: owning_org_2) + end + let(:log) { create(:lettings_log, owning_organisation: owning_org_1) } + + context "when user's org owns stock" do + let(:options) do + { + "" => "Select an option", + owning_org_1.id => "Owning org 1", + user.organisation.id => "User org (Your organisation)", + owning_org_2.id => "Owning org 2", + } + end + + it "shows current stock owner at top, followed by user's org (with hint), followed by the stock owners of the user's org" do + user.organisation.update!(holds_own_stock: true) + expect(question.displayed_answer_options(log, user)).to eq(options) + end + + context "when the owning-managing organisation relationship is deleted" do + let(:options) do + { + "" => "Select an option", + user.organisation.id => "User org (Your organisation)", + owning_org_2.id => "Owning org 2", + } + end + + it "doesn't remove the housing provider from the list of allowed housing providers" do + log.update!(owning_organisation: owning_org_2) + expect(question.displayed_answer_options(log, user)).to eq(options) + org_rel.destroy! + expect(question.displayed_answer_options(log, user)).to eq(options) + end + end + end + + context "when user's org doesn't own stock" do + let(:options) do + { + "" => "Select an option", + owning_org_1.id => "Owning org 1", + owning_org_2.id => "Owning org 2", + } + end + + it "shows current stock owner at top, followed by the stock owners of the user's org" do + user.organisation.update!(holds_own_stock: false) + expect(question.displayed_answer_options(log, user)).to eq(options) + end + end + end + + context "when user is support" do + let(:user) { create(:user, :support) } + + let(:log) { create(:lettings_log) } + + let(:non_stock_organisation) { create(:organisation, holds_own_stock: false) } + let(:expected_opts) do + Organisation.where(holds_own_stock: true).each_with_object(options) do |organisation, hsh| + hsh[organisation.id] = organisation.name + hsh + end + end + + it "shows orgs where organisation holds own stock" do + expect(question.displayed_answer_options(log, user)).to eq(expected_opts) + expect(question.displayed_answer_options(log, user)).not_to include(non_stock_organisation.id) + end + end end it "is marked as derived" do diff --git a/spec/models/forms/bulk_upload_sales/year_spec.rb b/spec/models/forms/bulk_upload_sales/year_spec.rb index 6509643ce..e8f2a04e5 100644 --- a/spec/models/forms/bulk_upload_sales/year_spec.rb +++ b/spec/models/forms/bulk_upload_sales/year_spec.rb @@ -3,6 +3,12 @@ require "rails_helper" RSpec.describe Forms::BulkUploadSales::Year do subject(:form) { described_class.new } + around do |example| + Timecop.freeze(Time.zone.now) do + example.run + end + end + describe "#options" do it "returns correct years" do expect(form.options.map(&:id)).to eql([2023, 2022]) diff --git a/spec/requests/sales_logs_controller_spec.rb b/spec/requests/sales_logs_controller_spec.rb index 41e3bb1d2..abe10af8e 100644 --- a/spec/requests/sales_logs_controller_spec.rb +++ b/spec/requests/sales_logs_controller_spec.rb @@ -65,6 +65,8 @@ RSpec.describe SalesLogsController, type: :request do context "with a request containing invalid json parameters" do let(:params) do { + "owning_organisation_id": owning_organisation.id, + "created_by_id": user.id, "saledate": Time.zone.today, "purchid": "1", "ownershipsch": 1, diff --git a/spec/services/bulk_upload/sales/validator_spec.rb b/spec/services/bulk_upload/sales/validator_spec.rb index 165abf8df..730a2b643 100644 --- a/spec/services/bulk_upload/sales/validator_spec.rb +++ b/spec/services/bulk_upload/sales/validator_spec.rb @@ -4,7 +4,7 @@ RSpec.describe BulkUpload::Sales::Validator do subject(:validator) { described_class.new(bulk_upload:, path:) } let(:user) { create(:user, organisation:) } - let(:organisation) { create(:organisation, old_visible_id: "3") } + let(:organisation) { create(:organisation, old_visible_id: "123") } let(:bulk_upload) { create(:bulk_upload, user:) } let(:path) { file.path } let(:file) { Tempfile.new } @@ -104,7 +104,7 @@ RSpec.describe BulkUpload::Sales::Validator do error = BulkUploadError.find_by(row: "6", field: "field_92", category: "setup") expect(error.field).to eql("field_92") - expect(error.error).to eql("The owning organisation code is incorrect") + expect(error.error).to eql("You must answer owning organisation") expect(error.purchaser_code).to eql("22 test BU") expect(error.row).to eql("6") expect(error.cell).to eql("CO6") diff --git a/spec/services/bulk_upload/sales/year2022/row_parser_spec.rb b/spec/services/bulk_upload/sales/year2022/row_parser_spec.rb index 37e68b26c..3b18333df 100644 --- a/spec/services/bulk_upload/sales/year2022/row_parser_spec.rb +++ b/spec/services/bulk_upload/sales/year2022/row_parser_spec.rb @@ -407,7 +407,7 @@ RSpec.describe BulkUpload::Sales::Year2022::RowParser do it "is not permitted as setup error" do setup_errors = parser.errors.select { |e| e.options[:category] == :setup } - expect(setup_errors.find { |e| e.attribute == :field_92 }.message).to eql("The owning organisation code is incorrect") + expect(setup_errors.find { |e| e.attribute == :field_92 }.message).to eql("You must answer owning organisation") end it "blocks log creation" do @@ -421,7 +421,7 @@ RSpec.describe BulkUpload::Sales::Year2022::RowParser do it "is not permitted as a setup error" do setup_errors = parser.errors.select { |e| e.options[:category] == :setup } - expect(setup_errors.find { |e| e.attribute == :field_92 }.message).to eql("The owning organisation code is incorrect") + expect(setup_errors.find { |e| e.attribute == :field_92 }.message).to eql("You must answer owning organisation") end it "blocks log creation" do diff --git a/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb b/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb index cddc042c6..e1137ea84 100644 --- a/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb +++ b/spec/services/bulk_upload/sales/year2023/row_parser_spec.rb @@ -384,7 +384,7 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do let(:attributes) { setup_section_params.merge(field_1: nil) } it "is not permitted as setup error" do - expect(parser.errors.where(:field_1, category: :setup).map(&:message)).to eql(["The owning organisation code is incorrect"]) + expect(parser.errors.where(:field_1, category: :setup).map(&:message)).to eql(["You must answer owning organisation"]) end it "blocks log creation" do @@ -396,7 +396,7 @@ RSpec.describe BulkUpload::Sales::Year2023::RowParser do let(:attributes) { { bulk_upload:, field_1: "donotexist" } } it "is not permitted as a setup error" do - expect(parser.errors.where(:field_1, category: :setup).map(&:message)).to eql(["The owning organisation code is incorrect"]) + expect(parser.errors.where(:field_1, category: :setup).map(&:message)).to eql(["You must answer owning organisation"]) end it "blocks log creation" do From 1a8ef9a82c38b4696b171bb68f65ac5d698892e7 Mon Sep 17 00:00:00 2001 From: Jack <113976590+bibblobcode@users.noreply.github.com> Date: Thu, 3 Aug 2023 11:24:53 +0100 Subject: [PATCH 09/12] Remove first_time_property_let_as_social_housing from optional (#1817) --- app/models/lettings_log.rb | 2 +- spec/models/lettings_log_spec.rb | 2 -- 2 files changed, 1 insertion(+), 3 deletions(-) diff --git a/app/models/lettings_log.rb b/app/models/lettings_log.rb index bc4a44a5f..2f11e31ff 100644 --- a/app/models/lettings_log.rb +++ b/app/models/lettings_log.rb @@ -73,7 +73,7 @@ class LettingsLog < Log } AUTOGENERATED_FIELDS = %w[id status created_at updated_at discarded_at].freeze - OPTIONAL_FIELDS = %w[first_time_property_let_as_social_housing tenancycode propcode chcharge].freeze + OPTIONAL_FIELDS = %w[tenancycode propcode chcharge].freeze RENT_TYPE_MAPPING_LABELS = { 1 => "Social Rent", 2 => "Affordable Rent", 3 => "Intermediate Rent" }.freeze HAS_BENEFITS_OPTIONS = [1, 6, 8, 7].freeze NUM_OF_WEEKS_FROM_PERIOD = { 2 => 26, 3 => 13, 4 => 12, 5 => 50, 6 => 49, 7 => 48, 8 => 47, 9 => 46, 1 => 52, 10 => 53 }.freeze diff --git a/spec/models/lettings_log_spec.rb b/spec/models/lettings_log_spec.rb index 0d0750f57..b34377543 100644 --- a/spec/models/lettings_log_spec.rb +++ b/spec/models/lettings_log_spec.rb @@ -2246,7 +2246,6 @@ RSpec.describe LettingsLog do it "returns optional fields" do expect(lettings_log.optional_fields).to eq(%w[ - first_time_property_let_as_social_housing tenancycode propcode chcharge @@ -2260,7 +2259,6 @@ RSpec.describe LettingsLog do it "returns optional fields" do expect(lettings_log.optional_fields).to eq(%w[ - first_time_property_let_as_social_housing tenancycode propcode chcharge From 4693ed58b3119059f4c869bb8945e6e85f30b82a Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Tue, 8 Aug 2023 15:16:59 +0100 Subject: [PATCH 10/12] Infer Incref (#1823) --- .../lettings_log_variables.rb | 16 +++++++++ lib/tasks/correct_incref_values.rake | 6 ++++ spec/lib/tasks/correct_incref_values_spec.rb | 36 +++++++++++++++++++ spec/models/lettings_log_spec.rb | 9 +++-- 4 files changed, 65 insertions(+), 2 deletions(-) create mode 100644 lib/tasks/correct_incref_values.rake create mode 100644 spec/lib/tasks/correct_incref_values_spec.rb diff --git a/app/models/derived_variables/lettings_log_variables.rb b/app/models/derived_variables/lettings_log_variables.rb index d29f21ce2..960ecad52 100644 --- a/app/models/derived_variables/lettings_log_variables.rb +++ b/app/models/derived_variables/lettings_log_variables.rb @@ -128,6 +128,22 @@ private incref: 1, }, }, + { + conditions: { + net_income_known: 0, + }, + derived_values: { + incref: 0, + }, + }, + { + conditions: { + net_income_known: 1, + }, + derived_values: { + incref: 2, + }, + }, ].freeze def clear_inapplicable_derived_values! diff --git a/lib/tasks/correct_incref_values.rake b/lib/tasks/correct_incref_values.rake new file mode 100644 index 000000000..0b1921998 --- /dev/null +++ b/lib/tasks/correct_incref_values.rake @@ -0,0 +1,6 @@ +desc "Alter incref values for non imported lettings logs in the database" +task correct_incref_values: :environment do + LettingsLog.where(old_id: nil, net_income_known: 0).update!(incref: 0) + LettingsLog.where(old_id: nil, net_income_known: 1).update!(incref: 2) + LettingsLog.where(old_id: nil, net_income_known: 2).update!(incref: 1) +end diff --git a/spec/lib/tasks/correct_incref_values_spec.rb b/spec/lib/tasks/correct_incref_values_spec.rb new file mode 100644 index 000000000..fc046dcf6 --- /dev/null +++ b/spec/lib/tasks/correct_incref_values_spec.rb @@ -0,0 +1,36 @@ +require "rails_helper" +require "rake" + +RSpec.describe "correct_incref_values" do + describe ":correct_incref_values", type: :task do + subject(:task) { Rake::Task["correct_incref_values"] } + + before do + Rake.application.rake_require("tasks/correct_incref_values") + Rake::Task.define_task(:environment) + task.reenable + end + + context "when the rake task is run" do + let!(:lettings_log) { create(:lettings_log, :completed) } + + it "updates lettings logs with net_income_known 0 (yes) to have incref 0 (no)" do + lettings_log.update!(net_income_known: 0, incref: nil) + task.invoke + expect(lettings_log.reload.incref).to eq(0) + end + + it "updates lettings logs with net_income_known 1 (no) to have incref 2 (don't know)" do + lettings_log.update!(net_income_known: 1, incref: nil) + task.invoke + expect(lettings_log.reload.incref).to eq(2) + end + + it "updates lettings logs with net_income_known 2 (prefers not to say) to have incref 1 (yes)" do + lettings_log.update!(net_income_known: 2, incref: nil) + task.invoke + expect(lettings_log.reload.incref).to eq(1) + end + end + end +end diff --git a/spec/models/lettings_log_spec.rb b/spec/models/lettings_log_spec.rb index b34377543..1ae394c3d 100644 --- a/spec/models/lettings_log_spec.rb +++ b/spec/models/lettings_log_spec.rb @@ -248,8 +248,13 @@ RSpec.describe LettingsLog do end it "correctly derives and saves incref" do - record_from_db = described_class.find(lettings_log.id) - expect(record_from_db["incref"]).to eq(1) + expect(lettings_log.reload.incref).to eq(1) + + lettings_log.update!(net_income_known: 1) + expect(lettings_log.reload.incref).to eq(2) + + lettings_log.update!(net_income_known: 0) + expect(lettings_log.reload.incref).to eq(0) end it "correctly derives and saves renttype" do From 1c6da769d7acd00d4968d0fb52e877a98d625f60 Mon Sep 17 00:00:00 2001 From: Jack <113976590+bibblobcode@users.noreply.github.com> Date: Tue, 8 Aug 2023 16:17:18 +0100 Subject: [PATCH 11/12] CLDC-1425 Send notification to old email address when changing it (#1820) * Send notification to old email address when changing it * send email changed notification to old email * Add specs * send both emails * Only send required confirmation emails * Show flash notice * Use reconfirmable template only if not confirmed * Don't commit .env.development * new email flow also when updating own email --- .gitignore | 1 + app/controllers/users_controller.rb | 11 +- app/mailers/devise_notify_mailer.rb | 59 ++++++++-- app/models/user.rb | 4 +- app/services/feature_toggle.rb | 4 + app/views/users/show.html.erb | 7 +- config/locales/en.yml | 2 + spec/features/user_spec.rb | 2 +- spec/mailers/resend_invitation_mailer_spec.rb | 24 +++- spec/requests/users_controller_spec.rb | 111 +++++++++++++++++- 10 files changed, 204 insertions(+), 21 deletions(-) diff --git a/.gitignore b/.gitignore index 93a5a3a05..b2b589114 100644 --- a/.gitignore +++ b/.gitignore @@ -42,6 +42,7 @@ yarn-debug.log* .yarn-integrity .env +.env.development # Code coverage results /coverage diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 5f619ad36..ac9a767f2 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -53,16 +53,21 @@ class UsersController < ApplicationController if @user == current_user bypass_sign_in @user flash[:notice] = I18n.t("devise.passwords.updated") if user_params.key?("password") + if user_params.key?("email") && FeatureToggle.new_email_journey? + flash[:notice] = I18n.t("devise.email.updated", email: @user.unconfirmed_email) + end + redirect_to account_path else user_name = @user.name&.possessive || @user.email.possessive - case user_params[:active] - when "false" + if user_params[:active] == "false" @user.update!(confirmed_at: nil, sign_in_count: 0, initial_confirmation_sent: false) flash[:notice] = I18n.t("devise.activation.deactivated", user_name:) - when "true" + elsif user_params[:active] == "true" @user.send_confirmation_instructions flash[:notice] = I18n.t("devise.activation.reactivated", user_name:) + elsif user_params.key?("email") && FeatureToggle.new_email_journey? + flash[:notice] = I18n.t("devise.email.updated", email: @user.unconfirmed_email) end redirect_to user_path(@user) end diff --git a/app/mailers/devise_notify_mailer.rb b/app/mailers/devise_notify_mailer.rb index 5560e7925..533bd76b6 100644 --- a/app/mailers/devise_notify_mailer.rb +++ b/app/mailers/devise_notify_mailer.rb @@ -35,12 +35,17 @@ class DeviseNotifyMailer < Devise::Mailer end def confirmation_instructions(record, token, _opts = {}) - username = record.email - if email_changed(record) - username = record.unconfirmed_email - send_confirmation_email(record.unconfirmed_email, record, token, username) + if email_changed?(record) + if new_email_journey? + send_email_changed_to_old_email(record) + send_email_changed_to_new_email(record, token) + else + send_confirmation_email(record.unconfirmed_email, record, token, record.unconfirmed_email) + send_confirmation_email(record.email, record, token, record.unconfirmed_email) + end + else + send_confirmation_email(record.email, record, token, record.email) end - send_confirmation_email(record.email, record, token, username) end def intercept_send?(email) @@ -54,10 +59,48 @@ class DeviseNotifyMailer < Devise::Mailer Rails.application.credentials[:email_allowlist] || [] end -private + def send_email_changed_to_old_email(record) + return true if intercept_send?(record.email) - def email_changed(record) - record.confirmable_template == User::CONFIRMABLE_TEMPLATE_ID && (record.unconfirmed_email.present? && record.unconfirmed_email != record.email) + send_email( + record.email, + User::FOR_OLD_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, + { + new_email: record.unconfirmed_email, + old_email: record.email, + }, + ) + end + + def send_email_changed_to_new_email(record, token) + return true if intercept_send?(record.email) + + link = "#{user_confirmation_url}?confirmation_token=#{token}" + + send_email( + record.unconfirmed_email, + User::FOR_NEW_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, + { + new_email: record.unconfirmed_email, + old_email: record.email, + link:, + }, + ) + end + + def email_changed?(record) + ( + record.confirmable_template == User::CONFIRMABLE_TEMPLATE_ID && ( + record.unconfirmed_email.present? && record.unconfirmed_email != record.email) + ) || ( + new_email_journey? && + record.versions.last.changeset.key?("unconfirmed_email") && + record.confirmed? + ) + end + + def new_email_journey? + FeatureToggle.new_email_journey? end def send_confirmation_email(email, record, token, username) diff --git a/app/models/user.rb b/app/models/user.rb index 48177d635..eccdaec47 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -121,6 +121,8 @@ class User < ApplicationRecord RECONFIRMABLE_TEMPLATE_ID = "bcdec787-f0a7-46e9-8d63-b3e0a06ee455".freeze BETA_ONBOARDING_TEMPLATE_ID = "b48bc2cd-5887-4611-8296-d0ab3ed0e7fd".freeze USER_REACTIVATED_TEMPLATE_ID = "ac45a899-490e-4f59-ae8d-1256fc0001f9".freeze + FOR_OLD_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID = "3eb80517-1051-4dfc-b4cc-cb18228a3829".freeze + FOR_NEW_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID = "0cdd0be1-7fa5-4808-8225-ae4c5a002352".freeze def reset_password_notify_template RESET_PASSWORD_TEMPLATE_ID @@ -131,7 +133,7 @@ class User < ApplicationRecord USER_REACTIVATED_TEMPLATE_ID elsif was_migrated_from_softwire? && last_sign_in_at.blank? BETA_ONBOARDING_TEMPLATE_ID - elsif initial_confirmation_sent + elsif initial_confirmation_sent && !confirmed? RECONFIRMABLE_TEMPLATE_ID else CONFIRMABLE_TEMPLATE_ID diff --git a/app/services/feature_toggle.rb b/app/services/feature_toggle.rb index d7df829ad..aed14d3e6 100644 --- a/app/services/feature_toggle.rb +++ b/app/services/feature_toggle.rb @@ -33,4 +33,8 @@ class FeatureToggle def self.deduplication_flow_enabled? !Rails.env.production? && !Rails.env.staging? end + + def self.new_email_journey? + !Rails.env.production? + end end diff --git a/app/views/users/show.html.erb b/app/views/users/show.html.erb index 844a0c965..bcdfd02b2 100644 --- a/app/views/users/show.html.erb +++ b/app/views/users/show.html.erb @@ -111,12 +111,11 @@ <%= govuk_button_to "Resend invite link", resend_invite_user_path(@user), secondary: true %> <% end %> <% else %> - - This user has been deactivated. <%= govuk_button_link_to "Reactivate user", reactivate_user_path(@user) %> - + + This user has been deactivated. <%= govuk_button_link_to "Reactivate user", reactivate_user_path(@user) %> + <% end %> <% end %>
-
diff --git a/config/locales/en.yml b/config/locales/en.yml index fb29431e1..5b7654c12 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -684,6 +684,8 @@ Make sure these answers are correct." hint_text: "This is more than 5 times the income, which is higher than we would expect." devise: + email: + updated: An email has been sent to %{email} to confirm this change. two_factor_authentication: success: "Two-factor authentication successful" attempt_failed: "Attempt failed" diff --git a/spec/features/user_spec.rb b/spec/features/user_spec.rb index f568ef96b..e1cb65f66 100644 --- a/spec/features/user_spec.rb +++ b/spec/features/user_spec.rb @@ -529,7 +529,7 @@ RSpec.describe "User Features" do end before do - other_user.update!(initial_confirmation_sent: true) + other_user.update!(initial_confirmation_sent: true, confirmed_at: nil) allow(user).to receive(:need_two_factor_authentication?).and_return(false) sign_in(user) visit(user_path(user.id)) diff --git a/spec/mailers/resend_invitation_mailer_spec.rb b/spec/mailers/resend_invitation_mailer_spec.rb index 02a6189d4..a5eadad20 100644 --- a/spec/mailers/resend_invitation_mailer_spec.rb +++ b/spec/mailers/resend_invitation_mailer_spec.rb @@ -57,9 +57,31 @@ RSpec.describe ResendInvitationMailer do end it "sends a reinvitation" do - expect(notify_client).to receive(:send_email).with(email_address: "active_user@example.com", template_id: User::RECONFIRMABLE_TEMPLATE_ID, personalisation:).once + expect(notify_client).to receive(:send_email).with(email_address: "active_user@example.com", template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation:).once described_class.new.resend_invitation_email(active_user) end end + + context "with unconfirmed user after the initial invitation has been sent" do + let!(:unconfirmed_user) { create(:user, organisation:, confirmation_token: "dluch", initial_confirmation_sent: true, old_user_id: "234", sign_in_count: 0, confirmed_at: nil) } + + let(:personalisation) do + { + name: unconfirmed_user.name, + email: unconfirmed_user.email, + organisation: unconfirmed_user.organisation.name, + link: include("/account/confirmation?confirmation_token=#{unconfirmed_user.confirmation_token}"), + } + end + + before do + LegacyUser.destroy_all + end + + it "sends a reinvitation" do + expect(notify_client).to receive(:send_email).with(email_address: unconfirmed_user.email, template_id: User::RECONFIRMABLE_TEMPLATE_ID, personalisation:).once + described_class.new.resend_invitation_email(unconfirmed_user) + end + end end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 984090863..ec2e0dde8 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -1538,7 +1538,7 @@ RSpec.describe UsersController, type: :request do expect(whodunnit_actor.id).to eq(user.id) end - context "when user changes email, dpo and key contact" do + context "when user changes email, dpo and key contact", :aggregate_failures do let(:params) { { id: user.id, user: { name: new_name, email: new_email, is_dpo: "true", is_key_contact: "true" } } } let(:personalisation) do { @@ -1551,6 +1551,8 @@ RSpec.describe UsersController, type: :request do before do user.legacy_users.destroy_all + + allow(FeatureToggle).to receive(:new_email_journey?).and_return(false) end it "allows changing email and dpo" do @@ -1566,6 +1568,43 @@ RSpec.describe UsersController, type: :request do expect(notify_client).to receive(:send_email).with(email_address: user.email, template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation:).once request end + + context "with new email journy enabled" do + before do + allow(FeatureToggle).to receive(:new_email_journey?).and_return(true) + end + + it "shows flash notice" do + patch("/users/#{other_user.id}", headers:, params:) + + expect(flash[:notice]).to eq("An email has been sent to #{new_email} to confirm this change.") + end + + it "sends new flow emails" do + expect(notify_client).to receive(:send_email).with( + email_address: other_user.email, + template_id: User::FOR_OLD_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, + personalisation: { + new_email:, + old_email: other_user.email, + }, + ).once + + expect(notify_client).to receive(:send_email).with( + email_address: new_email, + template_id: User::FOR_NEW_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, + personalisation: { + new_email:, + old_email: other_user.email, + link: include("/account/confirmation?confirmation_token="), + }, + ).once + + expect(notify_client).not_to receive(:send_email) + + patch "/users/#{other_user.id}", headers:, params: + end + end end context "when we update the user password" do @@ -1679,13 +1718,28 @@ RSpec.describe UsersController, type: :request do expect(page).to have_content(other_user.reload.email.to_s) end - context "when the support user tries to update the user’s password" do + context "when the support user tries to update the user’s password", :aggregate_failures do let(:params) do { - id: user.id, user: { password: new_name, password_confirmation: new_name, name: "new name" } + id: user.id, user: { password: new_name, password_confirmation: new_name, name: "new name", email: new_email } } end + let(:personalisation) do + { + name: params[:user][:name], + email: new_email, + organisation: other_user.organisation.name, + link: include("/account/confirmation?confirmation_token="), + } + end + + before do + other_user.legacy_users.destroy_all + + allow(FeatureToggle).to receive(:new_email_journey?).and_return(false) + end + it "does not update the password" do expect { patch "/users/#{other_user.id}", headers:, params: } .not_to change(other_user, :encrypted_password) @@ -1695,6 +1749,57 @@ RSpec.describe UsersController, type: :request do expect { patch "/users/#{other_user.id}", headers:, params: } .to change { other_user.reload.name }.from("Danny Rojas").to("new name") end + + it "allows changing email" do + expect { patch "/users/#{other_user.id}", headers:, params: } + .to change { other_user.reload.unconfirmed_email }.from(nil).to(new_email) + end + + it "sends a confirmation email to both emails" do + expect(notify_client).to receive(:send_email).with(email_address: other_user.email, template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation:).once + expect(notify_client).to receive(:send_email).with(email_address: new_email, template_id: User::CONFIRMABLE_TEMPLATE_ID, personalisation:).once + + expect(notify_client).not_to receive(:send_email) + + patch "/users/#{other_user.id}", headers:, params: + end + + context "with new user email flow enabled" do + before do + allow(FeatureToggle).to receive(:new_email_journey?).and_return(true) + end + + it "shows flash notice" do + patch("/users/#{other_user.id}", headers:, params:) + + expect(flash[:notice]).to eq("An email has been sent to #{new_email} to confirm this change.") + end + + it "sends new flow emails" do + expect(notify_client).to receive(:send_email).with( + email_address: other_user.email, + template_id: User::FOR_OLD_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, + personalisation: { + new_email:, + old_email: other_user.email, + }, + ).once + + expect(notify_client).to receive(:send_email).with( + email_address: new_email, + template_id: User::FOR_NEW_EMAIL_CHANGED_BY_OTHER_USER_TEMPLATE_ID, + personalisation: { + new_email:, + old_email: other_user.email, + link: include("/account/confirmation?confirmation_token="), + }, + ).once + + expect(notify_client).not_to receive(:send_email) + + patch "/users/#{other_user.id}", headers:, params: + end + end end end end From 8a3f18d3acc24750654cdcd94fae7f0ec4e4632f Mon Sep 17 00:00:00 2001 From: kosiakkatrina <54268893+kosiakkatrina@users.noreply.github.com> Date: Wed, 9 Aug 2023 09:45:08 +0100 Subject: [PATCH 12/12] CLDC-2621 Flag orgs without coordinators (#1818) * Add import report service * Call ImportReportService from rake task * Move generate_logs_report to import service * Add organisations without active data coordinators to the report * pluralize method names * update suffixes --- app/services/imports/import_report_service.rb | 57 +++++++++++++++ lib/tasks/full_import.rake | 34 ++------- spec/lib/tasks/full_import_spec.rb | 44 ++++++++++++ .../imports/import_report_service_spec.rb | 70 +++++++++++++++++++ 4 files changed, 177 insertions(+), 28 deletions(-) create mode 100644 app/services/imports/import_report_service.rb create mode 100644 spec/lib/tasks/full_import_spec.rb create mode 100644 spec/services/imports/import_report_service_spec.rb diff --git a/app/services/imports/import_report_service.rb b/app/services/imports/import_report_service.rb new file mode 100644 index 000000000..61d43a9eb --- /dev/null +++ b/app/services/imports/import_report_service.rb @@ -0,0 +1,57 @@ +module Imports + class ImportReportService + def initialize(storage_service, institutions_csv, logger = Rails.logger) + @storage_service = storage_service + @logger = logger + @institutions_csv = institutions_csv + end + + BYTE_ORDER_MARK = "\uFEFF".freeze # Required to ensure Excel always reads CSV as UTF-8 + + def create_reports(report_suffix) + generate_missing_data_coordinators_report(report_suffix) + generate_logs_report(report_suffix) + end + + def generate_missing_data_coordinators_report(report_suffix) + report_csv = "Organisation ID,Old Organisation ID,Organisation Name\n" + organisations = @institutions_csv.map { |row| Organisation.find_by(name: row[0]) }.compact + organisations.each do |organisation| + if organisation.users.none? { |user| user.data_coordinator? && user.active? } + report_csv += "#{organisation.id},#{organisation.old_visible_id},#{organisation.name}\n" + end + end + + report_name = "OrganisationsWithoutDataCoordinators_#{report_suffix}" + @storage_service.write_file(report_name, BYTE_ORDER_MARK + report_csv) + + @logger.info("Missing data coordinators report available in s3 import bucket at #{report_name}") + end + + def generate_logs_report(report_suffix) + Rails.logger.info("Generating migrated logs report") + + rep = CSV.generate do |report| + headers = ["Institution name", "Id", "Old Completed lettings logs", "Old In progress lettings logs", "Old Completed sales logs", "Old In progress sales logs", "New Completed lettings logs", "New In Progress lettings logs", "New Completed sales logs", "New In Progress sales logs"] + report << headers + + @institutions_csv.each do |row| + name = row[0] + organisation = Organisation.find_by(name:) + next unless organisation + + completed_sales_logs = organisation.owned_sales_logs.where(status: "completed").count + in_progress_sales_logs = organisation.owned_sales_logs.where(status: "in_progress").count + completed_lettings_logs = organisation.owned_lettings_logs.where(status: "completed").count + in_progress_lettings_logs = organisation.owned_lettings_logs.where(status: "in_progress").count + report << row.push(completed_lettings_logs, in_progress_lettings_logs, completed_sales_logs, in_progress_sales_logs) + end + end + + report_name = "MigratedLogsReport_#{report_suffix}" + @storage_service.write_file(report_name, BYTE_ORDER_MARK + rep) + + @logger.info("Logs report available in s3 import bucket at #{report_name}") + end + end +end diff --git a/lib/tasks/full_import.rake b/lib/tasks/full_import.rake index 780fe46fc..28af8c4dc 100644 --- a/lib/tasks/full_import.rake +++ b/lib/tasks/full_import.rake @@ -94,41 +94,19 @@ namespace :import do end desc "Generate migrated logs report" - task :generate_report, %i[institutions_csv_name] => :environment do |_task, args| + task :generate_reports, %i[institutions_csv_name] => :environment do |_task, args| institutions_csv_name = args[:institutions_csv_name] - raise "Usage: rake import:generate_report['institutions_csv_name']" if institutions_csv_name.blank? + raise "Usage: rake import:generate_reports['institutions_csv_name']" if institutions_csv_name.blank? s3_service = Storage::S3Service.new(Configuration::PaasConfigurationService.new, ENV["IMPORT_PAAS_INSTANCE"]) - csv = CSV.parse(s3_service.get_file_io(institutions_csv_name), headers: true) - - Rails.logger.info("Generating migrated logs report") - - rep = CSV.generate do |report| - headers = ["Institution name", "Id", "Old Completed lettings logs", "Old In progress lettings logs", "Old Completed sales logs", "Old In progress sales logs", "New Completed lettings logs", "New In Progress lettings logs", "New Completed sales logs", "New In Progress sales logs"] - report << headers - - csv.each do |row| - name = row[0] - organisation = Organisation.find_by(name:) - next unless organisation - - completed_sales_logs = organisation.owned_sales_logs.where(status: "completed").count - in_progress_sales_logs = organisation.owned_sales_logs.where(status: "in_progress").count - completed_lettings_logs = organisation.owned_lettings_logs.where(status: "completed").count - in_progress_lettings_logs = organisation.owned_lettings_logs.where(status: "in_progress").count - report << row.push(completed_lettings_logs, in_progress_lettings_logs, completed_sales_logs, in_progress_sales_logs) - end - end - - report_name = "MigratedLogsReport_#{institutions_csv_name}" - s3_service.write_file(report_name, rep) + institutions_csv = CSV.parse(s3_service.get_file_io(institutions_csv_name), headers: true) - Rails.logger.info("Logs report available in s3 import bucket at #{report_name}") + Imports::ImportReportService.new(s3_service, institutions_csv).create_reports(institutions_csv_name) end desc "Run import from logs step to end" - task :logs_onwards, %i[institutions_csv_name] => %i[environment logs trigger_invites generate_report] + task :logs_onwards, %i[institutions_csv_name] => %i[environment logs trigger_invites generate_reports] desc "Run a full import for the institutions listed in the named file on s3" - task :full, %i[institutions_csv_name] => %i[environment initial logs trigger_invites generate_report] + task :full, %i[institutions_csv_name] => %i[environment initial logs trigger_invites generate_reports] end diff --git a/spec/lib/tasks/full_import_spec.rb b/spec/lib/tasks/full_import_spec.rb new file mode 100644 index 000000000..f263f15fc --- /dev/null +++ b/spec/lib/tasks/full_import_spec.rb @@ -0,0 +1,44 @@ +require "rails_helper" +require "rake" + +describe "full import", type: :task do + let(:instance_name) { "paas_import_instance" } + let(:paas_config_service) { instance_double(Configuration::PaasConfigurationService) } + let(:storage_service) { instance_double(Storage::S3Service) } + let(:orgs_list) { "Institution name,Id,Old Completed lettings logs,Old In progress lettings logs,Old Completed sales logs,Old In progress sales logs\norg1,1.zip,0,0,0,0\norg2,2.zip,0,0,0,0" } + + before do + allow(Storage::S3Service).to receive(:new).and_return(storage_service) + allow(storage_service).to receive(:write_file).and_return(nil) + allow(storage_service).to receive(:get_file_io).and_return(orgs_list) + allow(Configuration::PaasConfigurationService).to receive(:new).and_return(paas_config_service) + allow(ENV).to receive(:[]) + allow(ENV).to receive(:[]).with("IMPORT_PAAS_INSTANCE").and_return(instance_name) + end + + describe "import:generate_reports" do + subject(:task) { Rake::Task["import:generate_reports"] } + + before do + Rake.application.rake_require("tasks/full_import") + Rake::Task.define_task(:environment) + task.reenable + end + + context "when generating report" do + let(:import_report_service) { instance_double(Imports::ImportReportService) } + + before do + allow(Imports::ImportReportService).to receive(:new).and_return(import_report_service) + end + + it "creates a report using given organisation csv" do + expect(Storage::S3Service).to receive(:new).with(paas_config_service, instance_name) + expect(Imports::ImportReportService).to receive(:new).with(storage_service, CSV.parse(orgs_list, headers: true)) + expect(import_report_service).to receive(:create_reports).with("some_name") + + task.invoke("some_name") + end + end + end +end diff --git a/spec/services/imports/import_report_service_spec.rb b/spec/services/imports/import_report_service_spec.rb new file mode 100644 index 000000000..1d241754d --- /dev/null +++ b/spec/services/imports/import_report_service_spec.rb @@ -0,0 +1,70 @@ +require "rails_helper" + +RSpec.describe Imports::ImportReportService do + subject(:report_service) { described_class.new(storage_service, institutions_csv) } + + let(:storage_service) { instance_double(Storage::S3Service) } + + describe "#generate_missing_data_coordinators_report" do + context "when all organisations have data coordinators" do + let!(:organisation) { create(:organisation, old_visible_id: "1", name: "org1") } + let(:institutions_csv) { CSV.parse("Institution name,Id,Old Completed lettings logs,Old In progress lettings logs,Old Completed sales logs,Old In progress sales logs\norg1,1,2,1,4,3", headers: true) } + + before do + create(:user, :data_coordinator, organisation:) + end + + it "writes an empty organisations without a data coordinators report" do + expect(storage_service).to receive(:write_file).with("OrganisationsWithoutDataCoordinators_report_suffix.csv", "\uFEFFOrganisation ID,Old Organisation ID,Organisation Name\n") + + report_service.generate_missing_data_coordinators_report("report_suffix.csv") + end + end + + context "when some organisations have no data coordinators" do + let!(:organisation) { create(:organisation, old_visible_id: "1", name: "org1") } + let!(:organisation2) { create(:organisation, old_visible_id: "2", name: "org2") } + let!(:organisation3) { create(:organisation, old_visible_id: "3", name: "org3") } + let(:institutions_csv) { CSV.parse("Institution name,Id,Old Completed lettings logs,Old In progress lettings logs,Old Completed sales logs,Old In progress sales logs\norg1,1,2,1,4,3\norg2,2,5,6,5,7\norg3,3,5,6,5,7", headers: true) } + + before do + create(:user, :data_coordinator, organisation:) + end + + it "writes an empty organisations without a data coordinators report" do + expect(storage_service).to receive(:write_file).with("OrganisationsWithoutDataCoordinators_report_suffix.csv", "\uFEFFOrganisation ID,Old Organisation ID,Organisation Name\n#{organisation2.id},2,org2\n#{organisation3.id},3,org3\n") + + report_service.generate_missing_data_coordinators_report("report_suffix.csv") + end + end + + context "when organisation has an inactive data coordinator" do + let!(:organisation) { create(:organisation, old_visible_id: "1", name: "org1") } + let(:institutions_csv) { CSV.parse("Institution name,Id,Old Completed lettings logs,Old In progress lettings logs,Old Completed sales logs,Old In progress sales logs\norg1,1,2,1,4,3", headers: true) } + + before do + create(:user, :data_coordinator, organisation:, active: false) + end + + it "includes that organisation in the data coordinators report" do + expect(storage_service).to receive(:write_file).with("OrganisationsWithoutDataCoordinators_report_suffix.csv", "\uFEFFOrganisation ID,Old Organisation ID,Organisation Name\n#{organisation.id},1,org1\n") + + report_service.generate_missing_data_coordinators_report("report_suffix.csv") + end + end + end + + describe "#generate_logs_report" do + let(:institutions_csv) { CSV.parse("Institution name,Id,Old Completed lettings logs,Old In progress lettings logs,Old Completed sales logs,Old In progress sales logs\norg1,1,2,1,4,3\norg2,2,5,6,5,7", headers: true) } + + before do + create(:organisation, old_visible_id: "1", name: "org1") + create(:organisation, old_visible_id: "2", name: "org2") + end + + it "generates a report with imported logs" do + expect(storage_service).to receive(:write_file).with("MigratedLogsReport_report_suffix.csv", "\uFEFFInstitution name,Id,Old Completed lettings logs,Old In progress lettings logs,Old Completed sales logs,Old In progress sales logs,New Completed lettings logs,New In Progress lettings logs,New Completed sales logs,New In Progress sales logs\norg1,1,2,1,4,3,0,0,0,0\norg2,2,5,6,5,7,0,0,0,0\n") + report_service.generate_logs_report("report_suffix.csv") + end + end +end