From d6ef35eef071507957d9a9dc70b110891163dee4 Mon Sep 17 00:00:00 2001 From: natdeanlewissoftwire <94526761+natdeanlewissoftwire@users.noreply.github.com> Date: Tue, 26 Sep 2023 12:17:34 +0100 Subject: [PATCH] CLDC-2816 Fix scheme filter bug (#1940) * feat: move ordering after filtering to avoid ORDER BY using expressions not in SELECT DISTINCT list * feat: add multiple schemes so tests will fail if this bug appears again --- app/controllers/organisations_controller.rb | 4 +- app/controllers/schemes_controller.rb | 4 +- app/models/scheme.rb | 14 ++--- spec/models/scheme_spec.rb | 57 +++++++++++++-------- 4 files changed, 47 insertions(+), 32 deletions(-) diff --git a/app/controllers/organisations_controller.rb b/app/controllers/organisations_controller.rb index d94b7460b..cce0c60fd 100644 --- a/app/controllers/organisations_controller.rb +++ b/app/controllers/organisations_controller.rb @@ -21,9 +21,9 @@ class OrganisationsController < ApplicationController end def schemes - all_schemes = Scheme.where(owning_organisation: [@organisation] + @organisation.parent_organisations).order_by_completion.order_by_service_name + all_schemes = Scheme.where(owning_organisation: [@organisation] + @organisation.parent_organisations) - @pagy, @schemes = pagy(filter_manager.filtered_schemes(all_schemes, search_term, session_filters)) + @pagy, @schemes = pagy(filter_manager.filtered_schemes(all_schemes, search_term, session_filters).order_by_completion.order_by_service_name) @searched = search_term.presence @total_count = all_schemes.size @filter_type = "schemes" diff --git a/app/controllers/schemes_controller.rb b/app/controllers/schemes_controller.rb index e026a70b9..44341b48f 100644 --- a/app/controllers/schemes_controller.rb +++ b/app/controllers/schemes_controller.rb @@ -13,9 +13,9 @@ class SchemesController < ApplicationController def index redirect_to schemes_organisation_path(current_user.organisation) unless current_user.support? - all_schemes = Scheme.order_by_completion.order_by_service_name + all_schemes = Scheme.all - @pagy, @schemes = pagy(filter_manager.filtered_schemes(all_schemes, search_term, session_filters)) + @pagy, @schemes = pagy(filter_manager.filtered_schemes(all_schemes, search_term, session_filters).order_by_completion.order_by_service_name) @searched = search_term.presence @total_count = all_schemes.size @filter_type = "schemes" diff --git a/app/models/scheme.rb b/app/models/scheme.rb index 6afe8cd81..bab388b5d 100644 --- a/app/models/scheme.rb +++ b/app/models/scheme.rb @@ -43,9 +43,9 @@ class Scheme < ApplicationRecord scope :incomplete, lambda { where.not(confirmed: true) .or(where.not(id: Location.select(:scheme_id).where(confirmed: true).distinct)) - .where.not(id: joins(:scheme_deactivation_periods).reactivating_soon.pluck(:id, :service_name, :confirmed)) - .where.not(id: joins(:scheme_deactivation_periods).deactivated.pluck(:id, :service_name, :confirmed)) - .where.not(id: joins(:scheme_deactivation_periods).deactivating_soon.pluck(:id, :service_name, :confirmed)) + .where.not(id: joins(:scheme_deactivation_periods).reactivating_soon.pluck(:id)) + .where.not(id: joins(:scheme_deactivation_periods).deactivated.pluck(:id)) + .where.not(id: joins(:scheme_deactivation_periods).deactivating_soon.pluck(:id)) } scope :deactivated, lambda { @@ -64,10 +64,10 @@ class Scheme < ApplicationRecord } scope :active_status, lambda { - where.not(id: joins(:scheme_deactivation_periods).reactivating_soon.pluck(:id, :service_name, :confirmed)) - .where.not(id: joins(:scheme_deactivation_periods).deactivated.pluck(:id, :service_name, :confirmed)) - .where.not(id: incomplete.pluck(:id, :service_name, :confirmed)) - .where.not(id: joins(:scheme_deactivation_periods).deactivating_soon.pluck(:id, :service_name, :confirmed)) + where.not(id: joins(:scheme_deactivation_periods).reactivating_soon.pluck(:id)) + .where.not(id: joins(:scheme_deactivation_periods).deactivated.pluck(:id)) + .where.not(id: incomplete.pluck(:id)) + .where.not(id: joins(:scheme_deactivation_periods).deactivating_soon.pluck(:id)) } validate :validate_confirmed diff --git a/spec/models/scheme_spec.rb b/spec/models/scheme_spec.rb index 0001d55f7..1e578fed5 100644 --- a/spec/models/scheme_spec.rb +++ b/spec/models/scheme_spec.rb @@ -111,76 +111,91 @@ RSpec.describe Scheme, type: :model do context "when filtering by status" do let!(:incomplete_scheme) { FactoryBot.create(:scheme, :incomplete, service_name: "name") } + let!(:incomplete_scheme_2) { FactoryBot.create(:scheme, :incomplete, service_name: "name") } let(:active_scheme) { FactoryBot.create(:scheme) } + let(:active_scheme_2) { FactoryBot.create(:scheme) } let(:deactivating_soon_scheme) { FactoryBot.create(:scheme) } + let(:deactivating_soon_scheme_2) { FactoryBot.create(:scheme) } let(:deactivated_scheme) { FactoryBot.create(:scheme) } + let(:deactivated_scheme_2) { FactoryBot.create(:scheme) } let(:reactivating_soon_scheme) { FactoryBot.create(:scheme) } + let(:reactivating_soon_scheme_2) { FactoryBot.create(:scheme) } before do scheme.destroy! scheme_1.destroy! scheme_2.destroy! - Timecop.freeze(2022, 6, 7) - FactoryBot.create(:scheme_deactivation_period, deactivation_date: Time.zone.local(2022, 8, 8), scheme: deactivating_soon_scheme) + FactoryBot.create(:scheme_deactivation_period, deactivation_date: Time.zone.tomorrow, scheme: deactivating_soon_scheme) + FactoryBot.create(:scheme_deactivation_period, deactivation_date: Time.zone.tomorrow, scheme: deactivating_soon_scheme_2) deactivating_soon_scheme.save! - FactoryBot.create(:scheme_deactivation_period, deactivation_date: Time.zone.local(2022, 6, 6), scheme: deactivated_scheme) + deactivating_soon_scheme_2.save! + FactoryBot.create(:scheme_deactivation_period, deactivation_date: Time.zone.yesterday, scheme: deactivated_scheme) + FactoryBot.create(:scheme_deactivation_period, deactivation_date: Time.zone.yesterday, scheme: deactivated_scheme_2) deactivated_scheme.save! - FactoryBot.create(:scheme_deactivation_period, deactivation_date: Time.zone.local(2022, 6, 7), reactivation_date: Time.zone.local(2022, 6, 8), scheme: reactivating_soon_scheme) + deactivated_scheme_2.save! + FactoryBot.create(:scheme_deactivation_period, deactivation_date: Time.zone.yesterday, reactivation_date: Time.zone.tomorrow, scheme: reactivating_soon_scheme) + FactoryBot.create(:scheme_deactivation_period, deactivation_date: Time.zone.yesterday, reactivation_date: Time.zone.tomorrow, scheme: reactivating_soon_scheme_2) reactivating_soon_scheme.save! + reactivating_soon_scheme_2.save! FactoryBot.create(:location, scheme: active_scheme, confirmed: true) - end - - after do - Timecop.unfreeze + FactoryBot.create(:location, scheme: active_scheme_2, confirmed: true) end context "when filtering by incomplete status" do it "returns only incomplete schemes" 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_scheme) + expect(described_class.filter_by_status(%w[incomplete]).count).to eq(2) + expect(described_class.filter_by_status(%w[incomplete])).to include(incomplete_scheme) + expect(described_class.filter_by_status(%w[incomplete])).to include(incomplete_scheme_2) end end context "when filtering by incomplete status and searching" do it "returns only incomplete schemes" do - expect(described_class.search_by("name").filter_by_status(%w[incomplete]).count).to eq(1) - expect(described_class.search_by("name").filter_by_status(%w[incomplete]).first).to eq(incomplete_scheme) + expect(described_class.search_by("name").filter_by_status(%w[incomplete]).count).to eq(2) + expect(described_class.search_by("name").filter_by_status(%w[incomplete])).to include(incomplete_scheme) + expect(described_class.search_by("name").filter_by_status(%w[incomplete])).to include(incomplete_scheme_2) end end context "when filtering by active status" do it "returns only active schemes" 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_scheme) + expect(described_class.filter_by_status(%w[active]).count).to eq(2) + expect(described_class.filter_by_status(%w[active])).to include(active_scheme) + expect(described_class.filter_by_status(%w[active])).to include(active_scheme_2) end end context "when filtering by deactivating_soon status" do it "returns only deactivating_soon schemes" 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_scheme) + expect(described_class.filter_by_status(%w[deactivating_soon]).count).to eq(2) + expect(described_class.filter_by_status(%w[deactivating_soon])).to include(deactivating_soon_scheme) + expect(described_class.filter_by_status(%w[deactivating_soon])).to include(deactivating_soon_scheme_2) end end context "when filtering by deactivated status" do it "returns only deactivated schemes" 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_scheme) + expect(described_class.filter_by_status(%w[deactivated]).count).to eq(2) + expect(described_class.filter_by_status(%w[deactivated])).to include(deactivated_scheme) + expect(described_class.filter_by_status(%w[deactivated])).to include(deactivated_scheme_2) end end context "when filtering by reactivating_soon status" do it "returns only reactivating_soon schemes" 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_scheme) + expect(described_class.filter_by_status(%w[reactivating_soon]).count).to eq(2) + expect(described_class.filter_by_status(%w[reactivating_soon])).to include(reactivating_soon_scheme) + expect(described_class.filter_by_status(%w[reactivating_soon])).to include(reactivating_soon_scheme_2) end end context "when filtering by multiple statuses" do it "returns relevant schemes" do - expect(described_class.filter_by_status(%w[deactivating_soon reactivating_soon]).count).to eq(2) + expect(described_class.filter_by_status(%w[deactivating_soon reactivating_soon]).count).to eq(4) expect(described_class.filter_by_status(%w[deactivating_soon reactivating_soon])).to include(reactivating_soon_scheme) + expect(described_class.filter_by_status(%w[deactivating_soon reactivating_soon])).to include(reactivating_soon_scheme_2) expect(described_class.filter_by_status(%w[deactivating_soon reactivating_soon])).to include(deactivating_soon_scheme) + expect(described_class.filter_by_status(%w[deactivating_soon reactivating_soon])).to include(deactivating_soon_scheme_2) end end end