CLDC-2128: Validate supplied year in bulk upload pages (#2839)

* CLDC-2128: Validate supplied year in bulk upload pages

* Fix lint

* Refactor

app/controllers/bulk_upload_lettings_logs_controller.rb

@@ -1,6 +1,7 @@
 class BulkUploadLettingsLogsController < ApplicationController
   before_action :authenticate_user!
-  before_action :validate_data_protection_agrement_signed!
+  before_action :validate_data_protection_agreement_signed!
+  before_action :validate_year!, except: %w[start]
 
   def start
     if have_choice_of_year?
@@ -24,12 +25,26 @@ class BulkUploadLettingsLogsController < ApplicationController
 
 private
 
-  def validate_data_protection_agrement_signed!
+  def validate_data_protection_agreement_signed!
     return if @current_user.organisation.data_protection_confirmed?
 
     redirect_to lettings_logs_path
   end
 
+  def validate_year!
+    return if params[:id] == "year"
+    return if params[:id] == "guidance" && params.dig(:form, :year).nil?
+
+    allowed_years = [current_year]
+    allowed_years.push(current_year - 1) if FormHandler.instance.lettings_in_crossover_period?
+    allowed_years.push(current_year + 1) if FeatureToggle.allow_future_form_use?
+
+    provided_year = params.dig(:form, :year)&.to_i
+    return if allowed_years.include?(provided_year)
+
+    render_not_found
+  end
+
   def current_year
     FormHandler.instance.current_collection_start_year
   end
@@ -48,8 +63,6 @@ private
                 Forms::BulkUploadLettings::PrepareYourFile.new(form_params)
               when "guidance"
                 Forms::BulkUploadLettings::Guidance.new(form_params.merge(referrer: params[:referrer]))
-              when "needstype"
-                Forms::BulkUploadLettings::Needstype.new(form_params)
               when "upload-your-file"
                 Forms::BulkUploadLettings::UploadYourFile.new(form_params.merge(current_user:))
               when "checking-file"
@@ -60,6 +73,6 @@ private
   end
 
   def form_params
-    params.fetch(:form, {}).permit(:year, :needstype, :file, :organisation_id)
+    params.fetch(:form, {}).permit(:year, :file, :organisation_id)
   end
 end

app/controllers/bulk_upload_sales_logs_controller.rb

@@ -1,6 +1,7 @@
 class BulkUploadSalesLogsController < ApplicationController
   before_action :authenticate_user!
-  before_action :validate_data_protection_agrement_signed!
+  before_action :validate_data_protection_agreement_signed!
+  before_action :validate_year!, except: %w[start]
 
   def start
     if have_choice_of_year?
@@ -24,12 +25,26 @@ class BulkUploadSalesLogsController < ApplicationController
 
 private
 
-  def validate_data_protection_agrement_signed!
+  def validate_data_protection_agreement_signed!
     return if @current_user.organisation.data_protection_confirmed?
 
     redirect_to sales_logs_path
   end
 
+  def validate_year!
+    return if params[:id] == "year"
+    return if params[:id] == "guidance" && params.dig(:form, :year).nil?
+
+    allowed_years = [current_year]
+    allowed_years.push(current_year - 1) if FormHandler.instance.sales_in_crossover_period?
+    allowed_years.push(current_year + 1) if FeatureToggle.allow_future_form_use?
+
+    provided_year = params.dig(:form, :year)&.to_i
+    return if allowed_years.include?(provided_year)
+
+    render_not_found
+  end
+
   def current_year
     FormHandler.instance.current_collection_start_year
   end

app/models/forms/bulk_upload_lettings/needstype.rb

@@ -1,39 +0,0 @@
-module Forms
-  module BulkUploadLettings
-    class Needstype
-      include ActiveModel::Model
-      include ActiveModel::Attributes
-      include Rails.application.routes.url_helpers
-
-      attribute :needstype, :integer
-      attribute :year, :integer
-      attribute :organisation_id, :integer
-
-      validates :needstype, presence: true
-
-      def view_path
-        "bulk_upload_lettings_logs/forms/needstype"
-      end
-
-      def options
-        [OpenStruct.new(id: 1, name: "General needs"), OpenStruct.new(id: 2, name: "Supported housing")]
-      end
-
-      def back_path
-        bulk_upload_lettings_log_path(id: "prepare-your-file", form: { year:, needstype:, organisation_id: }.compact)
-      end
-
-      def next_path
-        bulk_upload_lettings_log_path(id: "upload-your-file", form: { year:, needstype:, organisation_id: }.compact)
-      end
-
-      def year_combo
-        "#{year} to #{year + 1}"
-      end
-
-      def save!
-        true
-      end
-    end
-  end
-end

app/views/bulk_upload_lettings_logs/forms/needstype.erb

@@ -1,23 +0,0 @@
-<% content_for :before_content do %>
-  <%= govuk_back_link href: @form.back_path %>
-<% end %>
-
-<div class="govuk-grid-row">
-  <div class="govuk-grid-column-two-thirds-from-desktop">
-    <%= form_with model: @form, scope: :form, url: bulk_upload_lettings_log_path(id: "needstype"), method: :patch do |f| %>
-      <%= f.govuk_error_summary %>
-      <%= f.hidden_field :year %>
-      <%= f.hidden_field :organisation_id %>
-
-      <%= f.govuk_collection_radio_buttons :needstype,
-            @form.options,
-            :id,
-            :name,
-            hint: { text: I18n.t("hints.bulk_upload.needstype") },
-            legend: { text: "What is the needs type?", size: "l" },
-            caption: { text: "Upload lettings logs in bulk (#{@form.year_combo})", size: "l" } %>
-
-      <%= f.govuk_submit %>
-    <% end %>
-  </div>
-</div>

spec/requests/bulk_upload_lettings_logs_controller_spec.rb

@@ -73,5 +73,116 @@ RSpec.describe BulkUploadLettingsLogsController, type: :request do
         expect(response.body).to include("How to upload logs in bulk")
       end
     end
+
+    context "when no year is specified" do
+      it "shows guidance page with links defaulting to the current year" do
+        get "/lettings-logs/bulk-upload-logs/guidance"
+
+        expect(response.body).to include("Download the lettings bulk upload template (#{current_collection_start_year} to #{current_collection_start_year + 1})")
+      end
+    end
+
+    context "when an invalid year is specified" do
+      it "shows not found" do
+        get "/lettings-logs/bulk-upload-logs/guidance?form%5Byear%5D=10000"
+
+        expect(response).to be_not_found
+      end
+    end
+  end
+
+  describe "GET /lettings-logs/bulk-upload-logs/year" do
+    it "does not require a year to be specified" do
+      get "/lettings-logs/bulk-upload-logs/year"
+
+      expect(response).to be_ok
+    end
+  end
+
+  pages_requiring_year_specification = %w[prepare-your-file upload-your-file checking-file]
+  pages_requiring_year_specification.each do |page_id|
+    describe "GET /lettings-logs/bulk-upload-logs/#{page_id}" do
+      context "when no year is provided" do
+        it "returns not found" do
+          get "/lettings-logs/bulk-upload-logs/#{page_id}"
+
+          expect(response).to be_not_found
+        end
+      end
+
+      context "when requesting the previous year in a crossover period" do
+        before do
+          allow(FormHandler.instance).to receive(:lettings_in_crossover_period?).and_return(true)
+        end
+
+        it "succeeds" do
+          get "/lettings-logs/bulk-upload-logs/#{page_id}?form%5Byear%5D=#{current_collection_start_year - 1}"
+
+          expect(response).to be_ok
+        end
+      end
+
+      context "when requesting the previous year outside a crossover period" do
+        before do
+          allow(FormHandler.instance).to receive(:lettings_in_crossover_period?).and_return(false)
+        end
+
+        it "returns not found" do
+          get "/lettings-logs/bulk-upload-logs/#{page_id}?form%5Byear%5D=#{current_collection_start_year - 1}"
+
+          expect(response).to be_not_found
+        end
+      end
+
+      context "when requesting the current year" do
+        it "succeeds" do
+          get "/lettings-logs/bulk-upload-logs/#{page_id}?form%5Byear%5D=#{current_collection_start_year}"
+
+          expect(response).to be_ok
+        end
+      end
+
+      if page_id != "prepare-your-file"
+        context "when requesting the next year with future form use toggled on" do
+          before do
+            allow(FeatureToggle).to receive(:allow_future_form_use?).and_return(true)
+          end
+
+          it "succeeds" do
+            get "/lettings-logs/bulk-upload-logs/#{page_id}?form%5Byear%5D=#{current_collection_start_year + 1}"
+
+            expect(response).to be_ok
+          end
+        end
+      end
+
+      context "when requesting the next year with future form use toggled off" do
+        before do
+          allow(FeatureToggle).to receive(:allow_future_form_use?).and_return(false)
+        end
+
+        it "returns not found" do
+          get "/lettings-logs/bulk-upload-logs/#{page_id}?form%5Byear%5D=#{current_collection_start_year + 1}"
+
+          expect(response).to be_not_found
+        end
+      end
+
+      context "when requesting a far future year" do
+        it "returns not found" do
+          get "/lettings-logs/bulk-upload-logs/#{page_id}?form%5Byear%5D=9990"
+
+          expect(response).to be_not_found
+        end
+      end
+
+      context "when requesting a nonsense value for year" do
+        it "returns not found" do
+          get "/lettings-logs/bulk-upload-logs/#{page_id}?form%5Byear%5D=thisisnotayear"
+
+          expect(response).to be_not_found
+        end
+      end
+    end
   end
 end

spec/requests/bulk_upload_sales_logs_controller_spec.rb

@@ -73,5 +73,116 @@ RSpec.describe BulkUploadSalesLogsController, type: :request do
         expect(response.body).to include("How to upload logs in bulk")
       end
     end
+
+    context "when no year is specified" do
+      it "shows guidance page with links defaulting to the current year" do
+        get "/sales-logs/bulk-upload-logs/guidance"
+
+        expect(response.body).to include("Download the sales bulk upload template (#{current_collection_start_year} to #{current_collection_start_year + 1})")
+      end
+    end
+
+    context "when an invalid year is specified" do
+      it "shows not found" do
+        get "/sales-logs/bulk-upload-logs/guidance?form%5Byear%5D=10000"
+
+        expect(response).to be_not_found
+      end
+    end
+  end
+
+  describe "GET /sales-logs/bulk-upload-logs/year" do
+    it "does not require a year to be specified" do
+      get "/sales-logs/bulk-upload-logs/year"
+
+      expect(response).to be_ok
+    end
+  end
+
+  pages_requiring_year_specification = %w[prepare-your-file upload-your-file checking-file]
+  pages_requiring_year_specification.each do |page_id|
+    describe "GET /sales-logs/bulk-upload-logs/#{page_id}" do
+      context "when no year is provided" do
+        it "returns not found" do
+          get "/sales-logs/bulk-upload-logs/#{page_id}"
+
+          expect(response).to be_not_found
+        end
+      end
+
+      context "when requesting the previous year in a crossover period" do
+        before do
+          allow(FormHandler.instance).to receive(:sales_in_crossover_period?).and_return(true)
+        end
+
+        it "succeeds" do
+          get "/sales-logs/bulk-upload-logs/#{page_id}?form%5Byear%5D=#{current_collection_start_year - 1}"
+
+          expect(response).to be_ok
+        end
+      end
+
+      context "when requesting the previous year outside a crossover period" do
+        before do
+          allow(FormHandler.instance).to receive(:sales_in_crossover_period?).and_return(false)
+        end
+
+        it "returns not found" do
+          get "/sales-logs/bulk-upload-logs/#{page_id}?form%5Byear%5D=#{current_collection_start_year - 1}"
+
+          expect(response).to be_not_found
+        end
+      end
+
+      context "when requesting the current year" do
+        it "succeeds" do
+          get "/sales-logs/bulk-upload-logs/#{page_id}?form%5Byear%5D=#{current_collection_start_year}"
+
+          expect(response).to be_ok
+        end
+      end
+
+      if page_id != "prepare-your-file"
+        context "when requesting the next year with future form use toggled on" do
+          before do
+            allow(FeatureToggle).to receive(:allow_future_form_use?).and_return(true)
+          end
+
+          it "succeeds" do
+            get "/sales-logs/bulk-upload-logs/#{page_id}?form%5Byear%5D=#{current_collection_start_year + 1}"
+
+            expect(response).to be_ok
+          end
+        end
+      end
+
+      context "when requesting the next year with future form use toggled off" do
+        before do
+          allow(FeatureToggle).to receive(:allow_future_form_use?).and_return(false)
+        end
+
+        it "returns not found" do
+          get "/sales-logs/bulk-upload-logs/#{page_id}?form%5Byear%5D=#{current_collection_start_year + 1}"
+
+          expect(response).to be_not_found
+        end
+      end
+
+      context "when requesting a far future year" do
+        it "returns not found" do
+          get "/sales-logs/bulk-upload-logs/#{page_id}?form%5Byear%5D=9990"
+
+          expect(response).to be_not_found
+        end
+      end
+
+      context "when requesting a nonsense value for year" do
+        it "returns not found" do
+          get "/sales-logs/bulk-upload-logs/#{page_id}?form%5Byear%5D=thisisnotayear"
+
+          expect(response).to be_not_found
+        end
+      end
+    end
   end
 end