CLDC-2140 Scheme and location updates (#1455)

* feat: update scheme status so incomplete unless has active locations

* feat: update scheme typeahead text

* feat: reject incomplete locations

* feat: show completed/incomplete locations and validate when completed == 0

* feat: improve copy

* feat: change active -> completed

* feat: update scheme typeahead text

* feat: reject incomplete locations

* feat: show completed/incomplete locations and validate when completed == 0

* feat: improve copy

* feat: change to confirmed to add clarification in code

* feat: update scheme typeahead text

* feat: reject incomplete locations

* feat: show completed/incomplete locations and validate when completed == 0

* feat: improve copy

* feat: update scheme typeahead text

* feat: reject incomplete locations

* feat: only confirm locations if complete (old) AND save button clicked (new)

* feat: add unconfirmed scope

* refactor: complete -> confirm

* feat: fix tests

* feat: update scheme typeahead text

* feat: reject incomplete locations

* feat: show completed/incomplete locations and validate when completed == 0

* feat: improve copy

* feat: update scheme typeahead text

* feat: reject incomplete locations

* feat: only confirm locations if complete (old) AND save button clicked (new)

* feat: add unconfirmed scope

* refactor: complete -> confirm

* feat: fix tests

* feat: fix more tests

* feat: fix more tests

* refactor: rubocop

* refactor: rubocop

* refactor: rubocop

* feat: add tests for incomplete schemes

* refactor: linting

* feat: test incomplete count too

* feat: test unconfirmed locations aren't options in location_id

* feat: test validation

* feat: test when scheme is confirmed

* feat: test when scheme is confirmed for support user

* feat: test when location is confirmed

* refactor: linting

* refactor: consistent use of factorybot or not within a file

* feat: confirm locations when complete even if save button not clicked, and update tests

* refactor: simplify

* refactor: simplify

* refactor: move location helper methods to helpers

* refactor: move scheme helper methods to helpers

* refactor: formatting

* refactor: po response

* refactor: review response

app/controllers/locations_controller.rb

@@ -134,7 +134,7 @@ class LocationsController < ApplicationController
   def check_answers; end
 
   def confirm
-    flash[:notice] = "#{@location.postcode} #{@location.startdate.blank? || @location.startdate < Time.zone.now ? 'has been' : 'will be'} added to this scheme"
+    flash[:notice] = helpers.location_creation_success_notice(@location)
     redirect_to scheme_locations_path(@scheme)
   end
 

app/helpers/locations_helper.rb

@@ -78,6 +78,12 @@ module LocationsHelper
     return govuk_button_link_to "Reactivate this location", scheme_location_new_reactivation_path(location.scheme, location) if location.deactivated?
   end
 
+  def location_creation_success_notice(location)
+    if location.confirmed
+      "#{location.postcode} #{location.startdate.blank? || location.startdate.before?(Time.zone.now) ? 'has been' : 'will be'} added to this scheme"
+    end
+  end
+
 private
 
   ActivePeriod = Struct.new(:from, :to)

app/helpers/question_view_helper.rb

@@ -13,6 +13,26 @@ module QuestionViewHelper
     }
   end
 
+  def answer_option_synonyms(resource)
+    return unless resource.instance_of?(Scheme)
+
+    resource.locations.map(&:postcode).join(",")
+  end
+
+  def answer_option_append(resource)
+    return unless resource.instance_of?(Scheme)
+
+    confirmed_locations_count = resource.locations.confirmed.size
+    unconfirmed_locations_count = resource.locations.unconfirmed.size
+    "#{confirmed_locations_count} completed #{'location'.pluralize(confirmed_locations_count)}, #{unconfirmed_locations_count} incomplete #{'location'.pluralize(unconfirmed_locations_count)}"
+  end
+
+  def answer_option_hint(resource)
+    return unless resource.instance_of?(Scheme)
+
+    [resource.primary_client_group, resource.secondary_client_group].compact.join(", ")
+  end
+
 private
 
   def label_size(page_header, conditional, question)

app/models/form/lettings/questions/location_id.rb

@@ -27,7 +27,7 @@ class Form::Lettings::Questions::LocationId < ::Form::Question
   def displayed_answer_options(lettings_log, _user = nil)
     return {} unless lettings_log.scheme
 
-    scheme_location_ids = lettings_log.scheme.locations.pluck(:id)
+    scheme_location_ids = lettings_log.scheme.locations.confirmed.pluck(:id)
     answer_options.select { |k, _v| scheme_location_ids.include?(k.to_i) }
   end
 

app/models/form/question.rb

@@ -226,24 +226,6 @@ class Form::Question
     label
   end
 
-  def answer_option_synonyms(resource)
-    return unless resource.respond_to?(:synonyms)
-
-    resource.synonyms
-  end
-
-  def answer_option_append(resource)
-    return unless resource.respond_to?(:appended_text)
-
-    resource.appended_text
-  end
-
-  def answer_option_hint(resource)
-    return unless resource.respond_to?(:hint)
-
-    resource.hint
-  end
-
   def answer_selected?(log, answer)
     return false unless type == "select"
 

app/models/location.rb

@@ -24,6 +24,7 @@ class Location < ApplicationRecord
   scope :started, -> { where("startdate <= ?", Time.zone.today).or(where(startdate: nil)) }
   scope :active, -> { where(confirmed: true).and(started) }
   scope :confirmed, -> { where(confirmed: true) }
+  scope :unconfirmed, -> { where.not(confirmed: true) }
 
   LOCAL_AUTHORITIES = LocalAuthority.all.map { |la| [la.name, la.code] }.to_h
 

app/models/scheme.rb

@@ -161,18 +161,6 @@ class Scheme < ApplicationRecord
     ]
   end
 
-  def synonyms
-    locations.map(&:postcode).join(",")
-  end
-
-  def appended_text
-    "#{locations.count { |location| location.startdate.blank? || location.startdate <= Time.zone.today }} locations"
-  end
-
-  def hint
-    [primary_client_group, secondary_client_group].filter(&:present?).join(", ")
-  end
-
   def care_acts_options_with_hints
     hints = { "Yes – part registered as a care home": "A proportion of units are registered as being a care home." }
 

app/models/validations/setup_validations.rb

@@ -20,6 +20,14 @@ module Validations::SetupValidations
     location_during_startdate_validation(record, :location_id)
   end
 
+  def validate_scheme_has_confirmed_locations_validation(record)
+    return unless record.scheme
+
+    unless record.scheme.locations.confirmed.any?
+      record.errors.add :scheme_id, I18n.t("validations.scheme.no_completed_locations")
+    end
+  end
+
   def validate_scheme(record)
     location_during_startdate_validation(record, :scheme_id)
     scheme_during_startdate_validation(record, :scheme_id)

app/views/form/_select_question.html.erb

@@ -9,9 +9,9 @@
   hint: { text: question.hint_text&.html_safe }) do %>
     <% answers.each do |answer| %>
       <option value="<%= answer.id %>"
-      data-synonyms="<%= question.answer_option_synonyms(answer.resource) %>"
-      data-append="<%= question.answer_option_append(answer.resource) %>"
-      data-hint="<%= question.answer_option_hint(answer.resource) %>"
+        data-synonyms="<%= answer_option_synonyms(answer.resource) %>"
+        data-append="<%= answer_option_append(answer.resource) %>"
+        data-hint="<%= answer_option_hint(answer.resource) %>"
         <%= question.answer_selected?(@log, answer) ? "selected" : "" %>
         <%= answer.id == "" ? "disabled" : "" %>><%= answer.name || answer.resource %></option>
     <% end %>

config/locales/en.yml

@@ -467,6 +467,7 @@ en:
         during_deactivated_period: "The scheme is already deactivated during this date, please enter a different date"
       owning_organisation:
         does_not_own_stock: "Enter an organisation that owns housing stock"
+      no_completed_locations: "This scheme cannot be chosen as it has no completed locations"
 
     location:
       postcode_blank: "Enter a postcode"

spec/features/form/accessible_autocomplete_spec.rb

@@ -75,6 +75,7 @@ RSpec.describe "Accessible Autocomplete" do
     before do
       FactoryBot.create(:location, scheme:, postcode: "W6 0ST", startdate: Time.zone.local(2022, 1, 1))
       FactoryBot.create(:location, scheme:, postcode: "SE6 1LB", startdate: Time.zone.local(2022, 1, 1))
+      FactoryBot.create(:location, scheme:, postcode: nil, startdate: Time.zone.local(2022, 1, 1), confirmed: false)
       lettings_log.update!(needstype: 2)
       visit("/lettings-logs/#{lettings_log.id}/scheme")
     end
@@ -87,7 +88,7 @@ RSpec.describe "Accessible Autocomplete" do
     it "displays appended text next to the options", js: true do
       find("#lettings-log-scheme-id-field").click.native.send_keys("w", "6", :down, :enter)
       expect(find(".autocomplete__option", visible: :hidden, text: scheme.service_name)).to be_present
-      expect(find("span", visible: :hidden, text: "2 locations")).to be_present
+      expect(find("span", visible: :hidden, text: "2 completed locations, 1 incomplete location")).to be_present
     end
 
     it "displays hint text under the options", js: true do

spec/helpers/locations_helper_spec.rb

@@ -178,7 +178,7 @@ RSpec.describe LocationsHelper do
     context "when location has no local authority" do
       before do
         LocalAuthorityLink.create!(local_authority_id: LocalAuthority.find_by(code: "E07000030").id, linked_local_authority_id: LocalAuthority.find_by(code: "E06000063").id)
-        location.update!(location_code: nil)
+        location.update!(location_code: nil, confirmed: false)
       end
 
       it "returns correct display attributes" do

spec/models/form/lettings/questions/location_id_spec.rb

@@ -48,11 +48,13 @@ RSpec.describe Form::Lettings::Questions::LocationId, type: :model do
   context "when getting available locations" do
     let(:scheme) { FactoryBot.create(:scheme) }
     let!(:lettings_log) do
-      FactoryBot.create(:lettings_log, owning_organisation: scheme.owning_organisation, scheme:, needstype: 2)
+      FactoryBot.create(:lettings_log, owning_organisation: scheme.owning_organisation, needstype: 2)
     end
 
     context "when there are no locations" do
-      it "the displayed_answer_options is an empty hash" do
+      it "the displayed_answer_options is an empty hash (the scheme should have failed validation anyway so don't validate here)" do
+        lettings_log.scheme = scheme
+        lettings_log.save!(validate: false)
         expect(question.displayed_answer_options(lettings_log)).to eq({})
       end
     end
@@ -70,6 +72,7 @@ RSpec.describe Form::Lettings::Questions::LocationId, type: :model do
         before do
           FactoryBot.create(:location, scheme:, startdate: Time.utc(2022, 5, 13))
           FactoryBot.create(:location, scheme:, startdate: Time.utc(2023, 1, 1))
+          lettings_log.update!(scheme:)
         end
 
         it "the displayed_answer_options is an empty hash" do
@@ -77,10 +80,11 @@ RSpec.describe Form::Lettings::Questions::LocationId, type: :model do
         end
       end
 
-      context "and the locations have a no startdate" do
+      context "and the locations have no startdate" do
         before do
           FactoryBot.create(:location, scheme:, startdate: nil)
           FactoryBot.create(:location, scheme:, startdate: nil)
+          lettings_log.update!(scheme:)
         end
 
         it "the displayed_answer_options shows the locations" do
@@ -92,6 +96,7 @@ RSpec.describe Form::Lettings::Questions::LocationId, type: :model do
         before do
           FactoryBot.create(:location, scheme:, startdate: Time.utc(2022, 4, 10))
           FactoryBot.create(:location, scheme:, startdate: Time.utc(2022, 5, 12))
+          lettings_log.update!(scheme:)
         end
 
         it "the displayed_answer_options shows the locations" do
@@ -103,9 +108,22 @@ RSpec.describe Form::Lettings::Questions::LocationId, type: :model do
         before do
           FactoryBot.create(:location, scheme:, startdate: Time.utc(2022, 10, 10))
           FactoryBot.create(:location, scheme:, startdate: Time.utc(2022, 5, 12))
+          lettings_log.update!(scheme:)
+        end
+
+        it "the displayed_answer_options shows only the active location" do
+          expect(question.displayed_answer_options(lettings_log).count).to eq(1)
+        end
+      end
+
+      context "and some locations are not confirmed" do
+        before do
+          FactoryBot.create(:location, scheme:, postcode: nil)
+          FactoryBot.create(:location, scheme:)
+          lettings_log.update!(scheme:)
         end
 
-        it "the displayed_answer_options shows the active location" do
+        it "the displayed_answer_options shows only the confirmed location" do
           expect(question.displayed_answer_options(lettings_log).count).to eq(1)
         end
       end

spec/models/form/lettings/questions/scheme_id_spec.rb

@@ -123,13 +123,17 @@ RSpec.describe Form::Lettings::Questions::SchemeId, type: :model do
     end
 
     context "when the question is answered" do
-      it "returns 'select an option' as selected answer" do
+      before do
+        FactoryBot.create(:location, scheme:)
+      end
+
+      it "returns scheme as selected answer" do
         lettings_log.update!(scheme:)
         answers = question.displayed_answer_options(lettings_log).map do |key, value|
           OpenStruct.new(id: key, name: value.respond_to?(:service_name) ? value.service_name : nil, resource: value)
         end
         answers.each do |answer|
-          if answer.id == scheme.id
+          if answer.id.to_i == scheme.id
             expect(question.answer_selected?(lettings_log, answer)).to eq(true)
           else
             expect(question.answer_selected?(lettings_log, answer)).to eq(false)

spec/models/lettings_log_spec.rb

@@ -2339,7 +2339,7 @@ RSpec.describe LettingsLog do
 
   describe "resetting invalidated fields" do
     let(:scheme) { create(:scheme, owning_organisation: created_by_user.organisation) }
-    let(:location) { create(:location, location_code: "E07000223", scheme:) }
+    let!(:location) { create(:location, location_code: "E07000223", scheme:) }
     let(:lettings_log) do
       create(
         :lettings_log,

spec/models/location_spec.rb

@@ -788,7 +788,7 @@ RSpec.describe Location, type: :model do
     before do
       FactoryBot.create(:location, name: "ABC", postcode: "NW1 8RR", startdate: Time.zone.today)
       FactoryBot.create(:location, name: "XYZ", postcode: "SE1 6HJ", startdate: Time.zone.today + 1.day)
-      FactoryBot.create(:location, name: "GHQ", postcode: "EW1 7JK", startdate: Time.zone.today - 1.day, units: nil)
+      FactoryBot.create(:location, name: "GHQ", postcode: "EW1 7JK", startdate: Time.zone.today - 1.day, units: nil, confirmed: false)
       FactoryBot.create(:location, name: "GHQ", postcode: "EW1 7JK", startdate: nil)
     end
 

spec/models/validations/date_validations_spec.rb

@@ -4,9 +4,9 @@ RSpec.describe Validations::DateValidations do
   subject(:date_validator) { validator_class.new }
 
   let(:validator_class) { Class.new { include Validations::DateValidations } }
-  let(:record) { FactoryBot.create(:lettings_log) }
-  let(:scheme) { FactoryBot.create(:scheme, end_date: Time.zone.today - 5.days) }
-  let(:scheme_no_end_date) { FactoryBot.create(:scheme, end_date: nil) }
+  let(:record) { create(:lettings_log) }
+  let(:scheme) { create(:scheme, end_date: Time.zone.today - 5.days) }
+  let(:scheme_no_end_date) { create(:scheme, end_date: nil) }
 
   describe "tenancy start date" do
     it "must be a valid date" do
@@ -165,7 +165,7 @@ RSpec.describe Validations::DateValidations do
       let(:scheme) { create(:scheme) }
 
       before do
-        FactoryBot.create(:location, scheme:)
+        create(:location, scheme:)
         create(:scheme_deactivation_period, deactivation_date: Time.zone.local(2022, 6, 4), reactivation_date: Time.zone.local(2022, 8, 4), scheme:)
         scheme.reload
       end
@@ -190,7 +190,7 @@ RSpec.describe Validations::DateValidations do
       let(:scheme) { create(:scheme) }
 
       before do
-        FactoryBot.create(:location, scheme:)
+        create(:location, scheme:)
         create(:scheme_deactivation_period, deactivation_date: Time.zone.local(2022, 6, 4), reactivation_date: Time.zone.local(2022, 8, 4), scheme:)
         create(:scheme_deactivation_period, deactivation_date: Time.zone.local(2022, 6, 2), reactivation_date: Time.zone.local(2022, 8, 3), scheme:)
         create(:scheme_deactivation_period, deactivation_date: Time.zone.local(2022, 6, 1), reactivation_date: Time.zone.local(2022, 9, 4), scheme:)

spec/models/validations/setup_validations_spec.rb

@@ -4,7 +4,7 @@ RSpec.describe Validations::SetupValidations do
   subject(:setup_validator) { setup_validator_class.new }
 
   let(:setup_validator_class) { Class.new { include Validations::SetupValidations } }
-  let(:record) { FactoryBot.create(:lettings_log) }
+  let(:record) { create(:lettings_log) }
 
   describe "tenancy start date" do
     context "when in 22/23 collection" do
@@ -188,7 +188,7 @@ RSpec.describe Validations::SetupValidations do
       let(:scheme) { create(:scheme, created_at: Time.zone.local(2022, 4, 1)) }
 
       before do
-        FactoryBot.create(:location, scheme:)
+        create(:location, scheme:)
         create(:scheme_deactivation_period, deactivation_date: Time.zone.local(2022, 6, 4), reactivation_date: Time.zone.local(2022, 8, 4), scheme:)
         scheme.reload
       end
@@ -213,7 +213,7 @@ RSpec.describe Validations::SetupValidations do
       let(:scheme) { create(:scheme, created_at: Time.zone.local(2022, 4, 1)) }
 
       before do
-        FactoryBot.create(:location, scheme:)
+        create(:location, scheme:)
         create(:scheme_deactivation_period, deactivation_date: Time.zone.local(2022, 6, 4), reactivation_date: Time.zone.local(2022, 8, 4), scheme:)
         create(:scheme_deactivation_period, deactivation_date: Time.zone.local(2022, 6, 2), reactivation_date: Time.zone.local(2022, 8, 3), scheme:)
         create(:scheme_deactivation_period, deactivation_date: Time.zone.local(2022, 6, 1), reactivation_date: Time.zone.local(2022, 9, 4), scheme:)
@@ -310,8 +310,8 @@ RSpec.describe Validations::SetupValidations do
   end
 
   describe "#validate_organisation" do
-    let(:user) { FactoryBot.create(:user) }
-    let(:other_organisation) { FactoryBot.create(:organisation) }
+    let(:user) { create(:user) }
+    let(:other_organisation) { create(:organisation) }
 
     it "validates if neither managing nor owning organisation is the same as created_by user organisation" do
       record.created_by = user
@@ -368,4 +368,36 @@ RSpec.describe Validations::SetupValidations do
       expect(record.errors["managing_organisation_id"]).to be_empty
     end
   end
+
+  describe "#validate_scheme_has_confirmed_locations_validation" do
+    let(:scheme) { create(:scheme) }
+
+    context "with a scheme that has no confirmed locations" do
+      before do
+        create(:location, scheme:, postcode: nil)
+        scheme.reload
+      end
+
+      it "produces an error" do
+        record.scheme = scheme
+        setup_validator.validate_scheme_has_confirmed_locations_validation(record)
+        expect(record.errors["scheme_id"])
+          .to include(match I18n.t("validations.scheme.no_completed_locations"))
+      end
+    end
+
+    context "with a scheme that has confirmed locations" do
+      before do
+        create(:location, scheme:)
+        scheme.reload
+      end
+
+      it "does not produce an error" do
+        record.scheme = scheme
+        setup_validator.validate_scheme_has_confirmed_locations_validation(record)
+        expect(record.errors["scheme_id"])
+          .to be_empty
+      end
+    end
+  end
 end

spec/requests/locations_controller_spec.rb

@@ -1263,6 +1263,7 @@ RSpec.describe LocationsController, type: :request do
           patch "/schemes/#{scheme.id}/locations/#{location.id}/confirm", params:
         end
 
+        context "when location is complete" do
           it "confirms location" do
             expect(Location.last.confirmed).to eq(true)
           end
@@ -1274,6 +1275,21 @@ RSpec.describe LocationsController, type: :request do
           end
         end
 
+        context "when location is not complete" do
+          let(:location) { FactoryBot.create(:location, scheme:, startdate: Time.zone.local(2000, 1, 1), postcode: nil) }
+
+          it "does not confirm location" do
+            expect(Location.last.confirmed).to eq(false)
+          end
+
+          it "redirects correctly and does not display success banner" do
+            follow_redirect!
+            expect(page).not_to have_content("Success")
+            expect(page).not_to have_content("added to this scheme")
+          end
+        end
+      end
+
       context "when trying to edit check_answers of location that belongs to another organisation" do
         let(:another_scheme)  { FactoryBot.create(:scheme) }
         let(:another_location)  { FactoryBot.create(:location, scheme: another_scheme) }
@@ -1308,6 +1324,7 @@ RSpec.describe LocationsController, type: :request do
           patch "/schemes/#{scheme.id}/locations/#{location.id}/confirm", params:
         end
 
+        context "when location is complete" do
           it "confirms location" do
             expect(Location.last.confirmed).to eq(true)
           end
@@ -1319,6 +1336,21 @@ RSpec.describe LocationsController, type: :request do
           end
         end
 
+        context "when location is not complete" do
+          let(:location) { FactoryBot.create(:location, scheme:, startdate: Time.zone.local(2000, 1, 1), postcode: nil) }
+
+          it "does not confirm location" do
+            expect(Location.last.confirmed).to eq(false)
+          end
+
+          it "redirects correctly and does not display success banner" do
+            follow_redirect!
+            expect(page).not_to have_content("Success")
+            expect(page).not_to have_content("added to this scheme")
+          end
+        end
+      end
+
       context "when the requested location does not exist" do
         let(:location) { OpenStruct.new(id: (Location.maximum(:id) || 0) + 1, scheme:) }
 

spec/requests/schemes_controller_spec.rb

@@ -658,6 +658,26 @@ RSpec.describe SchemesController, type: :request do
         end
       end
 
+      context "when scheme is completed but not yet confirmed" do
+        let(:params) { { scheme: { page: "check-answers" } } }
+
+        it "is not confirmed" do
+          expect(scheme_to_update.confirmed).to eq(nil)
+        end
+
+        context "when confirming finished scheme" do
+          let(:params) { { scheme: { confirmed: true, page: "check-answers" } } }
+
+          before do
+            scheme_to_update.reload
+          end
+
+          it "confirms scheme" do
+            expect(scheme_to_update.confirmed).to eq(true)
+          end
+        end
+      end
+
       context "when required params are missing" do
         let(:params) do
           { scheme: {
@@ -921,6 +941,35 @@ RSpec.describe SchemesController, type: :request do
         patch "/schemes/#{scheme_to_update.id}", params:
       end
 
+      context "when confirming unfinished scheme" do
+        let(:params) { { scheme: { owning_organisation_id: user.organisation.id, arrangement_type: nil, confirmed: true, page: "check-answers" } } }
+
+        it "does not allow the scheme to be confirmed" do
+          expect(response).to have_http_status(:unprocessable_entity)
+          expect(page).to have_content(I18n.t("activerecord.errors.models.scheme.attributes.arrangement_type.invalid"))
+        end
+      end
+
+      context "when scheme is completed but not yet confirmed" do
+        let(:params) { { scheme: { page: "check-answers" } } }
+
+        it "is not confirmed" do
+          expect(scheme_to_update.confirmed).to eq(nil)
+        end
+
+        context "when confirming finished scheme" do
+          let(:params) { { scheme: { confirmed: true, page: "check-answers" } } }
+
+          before do
+            scheme_to_update.reload
+          end
+
+          it "confirms scheme" do
+            expect(scheme_to_update.confirmed).to eq(true)
+          end
+        end
+      end
+
       context "when required params are missing" do
         let(:params) do
           { scheme: {