CLDC 1865 Refactor questions to avoid race conditions (#1255)

* Extract time helper methods

* Add missing specs for BulkUploadController#start

* refactor ManagingOrganisation question

* refactor StockOwner question

app/helpers/collection_time_helper.rb

@@ -0,0 +1,16 @@
+module CollectionTimeHelper
+  def current_collection_start_year
+    today = Time.zone.now
+    window_end_date = Time.zone.local(today.year, 4, 1)
+    today < window_end_date ? today.year - 1 : today.year
+  end
+
+  def collection_start_date(date)
+    window_end_date = Time.zone.local(date.year, 4, 1)
+    date < window_end_date ? Time.zone.local(date.year - 1, 4, 1) : Time.zone.local(date.year, 4, 1)
+  end
+
+  def current_collection_start_date
+    Time.zone.local(current_collection_start_year, 4, 1)
+  end
+end

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

@@ -1,48 +1,45 @@
 class Form::Lettings::Questions::ManagingOrganisation < ::Form::Question
-  attr_accessor :current_user, :log
-
   def initialize(id, hsh, page)
     super
     @id = "managing_organisation_id"
     @check_answer_label = "Managing agent"
     @header = "Which organisation manages this letting?"
     @type = "select"
-    @answer_options = answer_options
   end
 
-  def answer_options
+  def answer_options(log = nil, user = nil)
     opts = { "" => "Select an option" }
 
     return opts unless ActiveRecord::Base.connected?
-    return opts unless current_user
+    return opts unless user
     return opts unless log
 
     if log.managing_organisation.present?
       opts = opts.merge({ log.managing_organisation.id => log.managing_organisation.name })
     end
 
-    if current_user.support?
+    if user.support?
       if log.owning_organisation.holds_own_stock?
         opts[log.owning_organisation.id] = "#{log.owning_organisation.name} (Owning organisation)"
       end
     else
-      opts[current_user.organisation.id] = "#{current_user.organisation.name} (Your organisation)"
+      opts[user.organisation.id] = "#{user.organisation.name} (Your organisation)"
     end
 
-    opts.merge(managing_organisations_answer_options)
+    orgs = if user.support?
+             log.owning_organisation
+           else
+             user.organisation
+           end.managing_agents.pluck(:id, :name).to_h
+
+    opts.merge(orgs)
   end
 
   def displayed_answer_options(log, user)
-    @current_user = user
+    answer_options(log, user)
-    @log = log
-
-    answer_options
   end
 
-  def label_from_value(value, log = nil, user = nil)
+  def label_from_value(value, _log = nil, _user = nil)
-    @log = log
-    @current_user = user
-
     return unless value
 
     answer_options[value]
@@ -53,25 +50,20 @@ class Form::Lettings::Questions::ManagingOrganisation < ::Form::Question
   end
 
   def hidden_in_check_answers?(log, user = nil)
-    @current_user = user
+    user.nil? || !@page.routed_to?(log, user)
-    @current_user.nil? || !@page.routed_to?(log, user)
   end
 
   def enabled
     true
   end
 
+  def answer_label(log, _current_user = nil)
+    Organisation.find_by(id: log.managing_organisation_id)&.name
+  end
+
 private
 
   def selected_answer_option_is_derived?(_log)
     true
   end
-
-  def managing_organisations_answer_options
-    if current_user.support?
-      log.owning_organisation
-    else
-      current_user.organisation
-    end.managing_agents.pluck(:id, :name).to_h
-  end
 end

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

@@ -1,6 +1,4 @@
 class Form::Lettings::Questions::StockOwner < ::Form::Question
-  attr_accessor :current_user, :log
-
   def initialize(id, hsh, page)
     super
     @id = "owning_organisation_id"
@@ -9,38 +7,38 @@ class Form::Lettings::Questions::StockOwner < ::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 current_user
+    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
 
-    if !current_user.support? && current_user.organisation.holds_own_stock?
+    if !user.support? && user.organisation.holds_own_stock?
-      answer_opts[current_user.organisation.id] = "#{current_user.organisation.name} (Your organisation)"
+      answer_opts[user.organisation.id] = "#{user.organisation.name} (Your organisation)"
     end
 
+    stock_owners_answer_options = if user.support?
+                                    Organisation
+                                  else
+                                    user.organisation.stock_owners
+                                  end.pluck(:id, :name).to_h
+
     answer_opts.merge(stock_owners_answer_options)
   end
 
   def displayed_answer_options(log, user = nil)
-    @current_user = user
+    answer_options(log, user)
-    @log = log
-
-    answer_options
   end
 
   def label_from_value(value, log = nil, user = nil)
-    @log = log
-    @current_user = user
-
     return unless value
 
-    answer_options[value]
+    answer_options(log, user)[value]
   end
 
   def derived?
@@ -48,13 +46,11 @@ class Form::Lettings::Questions::StockOwner < ::Form::Question
   end
 
   def hidden_in_check_answers?(_log, user = nil)
-    @current_user = user
+    return false if user.support?
 
-    return false if current_user.support?
+    stock_owners = user.organisation.stock_owners
 
-    stock_owners = current_user.organisation.stock_owners
+    if user.organisation.holds_own_stock?
-
-    if current_user.organisation.holds_own_stock?
       stock_owners.count.zero?
     else
       stock_owners.count <= 1
@@ -70,12 +66,4 @@ private
   def selected_answer_option_is_derived?(_log)
     true
   end
-
-  def stock_owners_answer_options
-    if current_user.support?
-      Organisation
-    else
-      current_user.organisation.stock_owners
-    end.pluck(:id, :name).to_h
-  end
 end

app/models/form_handler.rb

@@ -1,5 +1,6 @@
 class FormHandler
   include Singleton
+  include CollectionTimeHelper
   attr_reader :forms
 
   def initialize
@@ -44,21 +45,6 @@ class FormHandler
     forms
   end
 
-  def current_collection_start_year
-    today = Time.zone.now
-    window_end_date = Time.zone.local(today.year, 4, 1)
-    today < window_end_date ? today.year - 1 : today.year
-  end
-
-  def collection_start_date(date)
-    window_end_date = Time.zone.local(date.year, 4, 1)
-    date < window_end_date ? Time.zone.local(date.year - 1, 4, 1) : Time.zone.local(date.year, 4, 1)
-  end
-
-  def current_collection_start_date
-    Time.zone.local(current_collection_start_year, 4, 1)
-  end
-
   def form_name_from_start_year(year, type)
     form_mappings = { 0 => "current_#{type}", 1 => "previous_#{type}", -1 => "next_#{type}" }
     form_mappings[current_collection_start_year - year]

spec/helpers/collection_time_helper_spec.rb

@@ -0,0 +1,34 @@
+require "rails_helper"
+
+RSpec.describe CollectionTimeHelper do
+  let(:current_user) { create(:user, :data_coordinator) }
+  let(:user) { create(:user, :data_coordinator) }
+
+  around do |example|
+    Timecop.freeze(now) do
+      example.run
+    end
+  end
+
+  describe "Current collection start year" do
+    context "when the date is after 1st of April" do
+      let(:now) { Time.utc(2022, 8, 3) }
+
+      it "returns the same year as the current start year" do
+        expect(current_collection_start_year).to eq(2022)
+      end
+
+      it "returns the correct current start date" do
+        expect(current_collection_start_date).to eq(Time.zone.local(2022, 4, 1))
+      end
+    end
+
+    context "with the date before 1st of April" do
+      let(:now) { Time.utc(2022, 2, 3) }
+
+      it "returns the previous year as the current start year" do
+        expect(current_collection_start_year).to eq(2021)
+      end
+    end
+  end
+end

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

@@ -199,4 +199,36 @@ RSpec.describe Form::Lettings::Questions::ManagingOrganisation, type: :model do
       end
     end
   end
+
+  describe "#answer_label" do
+    context "when answered" do
+      let(:managing_organisation) { create(:organisation) }
+      let(:log) { create(:lettings_log, managing_organisation:) }
+
+      it "returns org name" do
+        expect(question.answer_label(log)).to eq(managing_organisation.name)
+      end
+    end
+
+    context "when unanswered" do
+      let(:log) { create(:lettings_log, managing_organisation: nil) }
+
+      it "returns nil" do
+        expect(question.answer_label(log)).to be_nil
+      end
+    end
+
+    context "when org does not exist" do
+      let(:managing_organisation) { create(:organisation) }
+      let(:log) { create(:lettings_log, managing_organisation:) }
+
+      before do
+        allow(Organisation).to receive(:find_by).and_return(nil)
+      end
+
+      it "returns nil" do
+        expect(question.answer_label(log)).to be_nil
+      end
+    end
+  end
 end

spec/models/form_handler_spec.rb

@@ -76,10 +76,6 @@ RSpec.describe FormHandler do
     context "when the date is after 1st of April" do
       let(:now) { Time.utc(2022, 8, 3) }
 
-      it "returns the same year as the current start year" do
-        expect(form_handler.current_collection_start_year).to eq(2022)
-      end
-
       it "returns the correct current lettings form name" do
         expect(form_handler.form_name_from_start_year(2022, "lettings")).to eq("current_lettings")
       end
@@ -103,19 +99,11 @@ RSpec.describe FormHandler do
       it "returns the correct next sales form name" do
         expect(form_handler.form_name_from_start_year(2023, "sales")).to eq("next_sales")
       end
-
-      it "returns the correct current start date" do
-        expect(form_handler.current_collection_start_date).to eq(Time.zone.local(2022, 4, 1))
-      end
     end
 
     context "with the date before 1st of April" do
       let(:now) { Time.utc(2022, 2, 3) }
 
-      it "returns the previous year as the current start year" do
-        expect(form_handler.current_collection_start_year).to eq(2021)
-      end
-
       it "returns the correct current lettings form name" do
         expect(form_handler.form_name_from_start_year(2021, "lettings")).to eq("current_lettings")
       end

spec/requests/bulk_upload_controller_spec.rb

@@ -13,6 +13,14 @@ RSpec.describe BulkUploadController, type: :request do
   end
 
   context "when a user is not signed in" do
+    describe "GET #start" do
+      before { get start_bulk_upload_lettings_logs_path, headers:, params: {} }
+
+      it "does not let you see the bulk upload page" do
+        expect(response).to redirect_to("/account/sign-in")
+      end
+    end
+
     describe "GET #show" do
       before { get url, headers:, params: {} }
 
@@ -50,6 +58,40 @@ RSpec.describe BulkUploadController, type: :request do
       end
     end
 
+    describe "GET #start" do
+      before do
+        Timecop.freeze(time)
+        get start_bulk_upload_lettings_logs_path
+      end
+
+      after do
+        Timecop.unfreeze
+      end
+
+      context "when not crossover period" do
+        let(:time) { Time.utc(2022, 2, 8) }
+
+        it "redirects to bulk upload path" do
+          expect(request).to redirect_to(
+            bulk_upload_lettings_log_path(
+              id: "prepare-your-file",
+              form: { year: 2021 },
+            ),
+          )
+        end
+      end
+
+      context "when crossover period" do
+        let(:time) { Time.utc(2022, 6, 8) }
+
+        it "redirects to bulk upload path" do
+          expect(request).to redirect_to(
+            bulk_upload_lettings_log_path(id: "year"),
+          )
+        end
+      end
+    end
+
     describe "POST #bulk upload" do
       context "with a valid file based on the upload template" do
         let(:request) { post url, params: { bulk_upload: { lettings_log_bulk_upload: valid_file } } }