Merge pull request #1727 from alphagov/prepare-for-switching-step-ids-to-be-external-ids

stephencdaly · web-flow · commit a2bb79335def · 2025-11-18T10:47:45.000Z
Prepare for switching step ids to be external ids
diff --git a/app/controllers/forms/base_controller.rb b/app/controllers/forms/base_controller.rb
@@ -8,7 +8,7 @@ def redirect_to_friendly_url_start
       LogEventService.log_form_start unless mode.preview?
     end
 
-    rescue_from ActiveResource::ResourceNotFound, Flow::StepFactory::PageNotFoundError, RepeatableStep::AnswerIndexError do
+    rescue_from ActiveResource::ResourceNotFound, RepeatableStep::AnswerIndexError do
       I18n.with_locale(locale) do
         render template: "errors/not_found", status: :not_found
       end
diff --git a/app/controllers/forms/page_controller.rb b/app/controllers/forms/page_controller.rb
@@ -38,7 +38,11 @@ def save
 
     def prepare_step
       page_slug = params.require(:page_slug)
-      @step = current_context.find_or_create(page_slug)
+      begin
+        @step = current_context.find_or_create(page_slug)
+      rescue Flow::StepFactory::PageNotFoundError
+        return redirect_to form_page_path(@form.id, @form.form_slug, current_context.next_page_slug)
+      end
 
       if @step.respond_to?(:answer_index)
         @step.answer_index = answer_index
diff --git a/app/lib/flow/step_factory.rb b/app/lib/flow/step_factory.rb
@@ -1,7 +1,6 @@
 module Flow
   class StepFactory
     START_PAGE = "_start".freeze
-    PAGE_SLUG_REGEX = Regexp.union([Page::PAGE_ID_REGEX, Regexp.new(CheckYourAnswersStep::CHECK_YOUR_ANSWERS_PAGE_SLUG)])
 
     class PageNotFoundError < StandardError
       def initialize(msg = "Page not found.")
@@ -22,6 +21,7 @@ def create_step(page_slug_or_start)
 
       # for now, we use the page id as slug
       page = @form.pages.find { |p| p.id.to_s == page_slug }
+      page = @form.pages.find { |p| p.respond_to?(:database_id) && p.database_id.to_s == page_slug } if page.nil?
       raise PageNotFoundError, "Can't find page #{page_slug}" if page.nil?
 
       question = QuestionRegister.from_page(page)
diff --git a/app/lib/store/access.rb b/app/lib/store/access.rb
@@ -3,5 +3,9 @@ module Access
     def page_key(step)
       step.page_id.to_s
     end
+
+    def database_id_key(step)
+      step.database_id&.to_s
+    end
   end
 end
diff --git a/app/lib/store/database_answer_store.rb b/app/lib/store/database_answer_store.rb
@@ -7,7 +7,11 @@ def initialize(answers)
     end
 
     def get_stored_answer(step)
-      @answers[page_key(step)]
+      if @answers.key?(page_key(step))
+        @answers[page_key(step)]
+      elsif database_id_key(step) && @answers.key?(database_id_key(step))
+        @answers[database_id_key(step)]
+      end
     end
   end
 end
diff --git a/app/lib/store/session_answer_store.rb b/app/lib/store/session_answer_store.rb
@@ -16,11 +16,20 @@ def save_step(step, answer)
     end
 
     def get_stored_answer(step)
-      @store.dig(ANSWERS_KEY, @form_key, page_key(step))
+      return nil if answers.nil?
+
+      if answers.key?(page_key(step))
+        answers[page_key(step)]
+      elsif database_id_key(step) && answers.key?(database_id_key(step))
+        answers[database_id_key(step)]
+      end
     end
 
     def clear_stored_answer(step)
-      @store.dig(ANSWERS_KEY, @form_key)&.delete(page_key(step))
+      return nil if answers.nil?
+
+      answers.delete(page_key(step)) if answers.key?(page_key(step))
+      answers.delete(database_id_key(step)) if database_id_key(step) && answers.key?(database_id_key(step))
     end
 
     def clear
diff --git a/app/models/form.rb b/app/models/form.rb
@@ -27,6 +27,7 @@ def pages
         "id" => step["id"],
         "position" => step["position"],
         "next_page" => step["next_step_id"],
+        "database_id" => step["database_id"], # temporary attribute while we switch to using external IDs
       }
       if step["type"] == "question_page"
         attrs.merge!(step["data"])
diff --git a/app/models/page.rb b/app/models/page.rb
@@ -1,5 +1,6 @@
 class Page < ActiveResource::Base
-  PAGE_ID_REGEX = /\d+/
+  PAGE_ID_REGEX_FOR_ROUTES = /(?:[a-zA-Z0-9]{8}|\d+)/
+  PAGE_ID_REGEX = /\A#{PAGE_ID_REGEX_FOR_ROUTES}\z/
 
   self.site = Settings.forms_api.base_url
   self.prefix = "/api/v2/forms/:form_id/"
diff --git a/app/models/step.rb b/app/models/step.rb
@@ -14,6 +14,11 @@ def page_id
     page&.id.to_s
   end
 
+  # Temporary attribute while we switch to using external IDs so we don't lose in-progress user sessions
+  def database_id
+    page.database_id if page && page.respond_to?(:database_id)
+  end
+
   def page_number
     page&.position
   end
diff --git a/config/routes.rb b/config/routes.rb
@@ -24,7 +24,7 @@
       get "/submitted" => "forms/submitted#submitted", as: :form_submitted
       get "/privacy" => "forms/privacy_page#show", as: :form_privacy
 
-      page_constraints = { page_slug: Flow::StepFactory::PAGE_SLUG_REGEX }
+      page_constraints = { page_slug: Regexp.union([Page::PAGE_ID_REGEX_FOR_ROUTES, Regexp.new(CheckYourAnswersStep::CHECK_YOUR_ANSWERS_PAGE_SLUG)]) }
       answer_constraints = { answer_index: /\d+/ }
       page_answer_defaults = { answer_index: 1 }
 
diff --git a/spec/lib/flow/step_factory_spec.rb b/spec/lib/flow/step_factory_spec.rb
@@ -37,6 +37,16 @@
           expect(step.next_page_slug).to eq(CheckYourAnswersStep::CHECK_YOUR_ANSWERS_PAGE_SLUG)
         end
       end
+
+      context "when the page slug is the database ID" do
+        let(:page) { build_stubbed :page, id: "page-1", database_id: "123", has_next_page?: true, next_page: "page-2" }
+
+        it "finds the page to create the step using the database_id" do
+          step = factory.create_step("123")
+          expect(step).to be_a(Step)
+          expect(step.id).to eq("page-1")
+        end
+      end
     end
 
     context "when creating a repeating step" do
@@ -82,18 +92,4 @@
       factory.start_step
     end
   end
-
-  describe Flow::StepFactory::PAGE_SLUG_REGEX do
-    it "matches valid page_id values" do
-      %w[1 123 0123456789 check-your-answers].each do |string|
-        expect(described_class).to match string
-      end
-    end
-
-    it "does not match invalid page_id values" do
-      %w[no ten inspect_your_answers /secret/login.php].each do |string|
-        expect(described_class).not_to match string
-      end
-    end
-  end
 end
diff --git a/spec/lib/store/database_answer_store_spec.rb b/spec/lib/store/database_answer_store_spec.rb
@@ -10,4 +10,41 @@
   let(:answers) { { page_key(step) => answer, "1111" => "another answer" } }
 
   it_behaves_like "an answer store"
+
+  describe "#get_stored_answer" do
+    context "when the answer was stored using the database_id as the key" do
+      let(:external_id) { "abc123" }
+      let(:database_id) { "123487" }
+      let(:answer) { "test answer" }
+      let(:answers) { { database_id.to_s => answer, "1111" => "another answer" } }
+
+      context "when the step has a database_id" do
+        let(:step) { instance_double(Step, { page_id: external_id, database_id: }) }
+
+        it "gets the answer by the database ID" do
+          expect(answer_store.get_stored_answer(step)).to eq(answer)
+        end
+      end
+
+      context "when the step does not have a database_id" do
+        let(:step) { instance_double(Step, { page_id: external_id, database_id: nil }) }
+
+        it "returns nil" do
+          expect(answer_store.get_stored_answer(step)).to be_nil
+        end
+      end
+    end
+
+    context "when there are answers stored using both the id and database_id of the step" do
+      let(:external_id) { "abc123" }
+      let(:database_id) { "123487" }
+      let(:answer) { "test answer" }
+      let(:step) { instance_double(Step, { page_id: external_id, database_id: }) }
+      let(:answers) { { database_id.to_s => "an answer we will ignore", external_id => answer } }
+
+      it "returns the answer stored using the step id" do
+        expect(answer_store.get_stored_answer(step)).to eq(answer)
+      end
+    end
+  end
 end
diff --git a/spec/lib/store/session_answer_store_spec.rb b/spec/lib/store/session_answer_store_spec.rb
@@ -6,7 +6,9 @@
 
   let(:store) { {} }
   let(:form_id) { 1 }
-  let(:step) { OpenStruct.new({ page_id: "5" }) }
+  let(:step_id) { 5 }
+  let(:database_id) { nil }
+  let(:step) { instance_double(Step, { page_id: step_id, database_id: }) }
   let(:other_step) { OpenStruct.new({ page_id: "1" }) }
   let(:reference) { Faker::Alphanumeric.alphanumeric(number: 8).upcase }
   let(:requested_email_confirmation) { true }
@@ -28,19 +30,65 @@
   end
 
   describe "#clear_stored_answer" do
-    before do
-      answer_store.save_step(step, "test answer")
+    context "when the answer was stored using the step id as the key" do
+      before do
+        answer_store.save_step(step, "test answer")
+      end
+
+      it "clears a single answer for a step" do
+        expect(answer_store.get_stored_answer(step)).to eq("test answer")
+        answer_store.clear_stored_answer(step)
+        expect(answer_store.get_stored_answer(step)).to be_nil
+      end
+
+      it "does not error if removing a step which doesn't exist in the store" do
+        answer_store.clear_stored_answer(other_step)
+        expect(answer_store.get_stored_answer(step)).to eq("test answer")
+      end
     end
 
-    it "clears a single answer for a step" do
-      expect(answer_store.get_stored_answer(step)).to eq("test answer")
-      answer_store.clear_stored_answer(step)
-      expect(answer_store.get_stored_answer(step)).to be_nil
+    context "when the the answer was stored using the database_id as the key" do
+      let(:step_id) { "abcABC12" }
+      let(:database_id) { 123 }
+      let(:answer) { "test answer" }
+      let(:store) do
+        {
+          answers: {
+            form_id.to_s => {
+              database_id.to_s => answer,
+            },
+          },
+        }
+      end
+
+      it "clears a single answer for a step" do
+        expect(answer_store.get_stored_answer(step)).to eq("test answer")
+        answer_store.clear_stored_answer(step)
+        expect(answer_store.get_stored_answer(step)).to be_nil
+      end
     end
 
-    it "does not error if removing a step which doesn't exist in the store" do
-      answer_store.clear_stored_answer(other_step)
-      expect(answer_store.get_stored_answer(step)).to eq("test answer")
+    context "when there are answers stored using both the id and database_id of the step" do
+      let(:step_id) { "abcABC12" }
+      let(:database_id) { 123 }
+      let(:answer) { "test answer" }
+
+      let(:store) do
+        {
+          answers: {
+            form_id.to_s => {
+              database_id.to_s => "an answer we will ignore",
+              step_id => answer,
+            },
+          },
+        }
+      end
+
+      it "clears both answers" do
+        expect(answer_store.get_stored_answer(step)).to eq("test answer")
+        answer_store.clear_stored_answer(step)
+        expect(answer_store.answers).to eq({})
+      end
     end
   end
 
@@ -66,17 +114,71 @@
   end
 
   describe "#get_stored_answer" do
-    before do
-      answer_store.save_step(step, "test answer")
-      answer_store.save_step(other_step, "test2 answer")
+    context "when the answer was stored using the step id as the key" do
+      before do
+        answer_store.save_step(step, "test answer")
+        answer_store.save_step(other_step, "test2 answer")
+      end
+
+      it "returns the answer for the first step" do
+        expect(answer_store.get_stored_answer(step)).to eq("test answer")
+      end
+
+      it "returns the answer for the second step" do
+        expect(answer_store.get_stored_answer(other_step)).to eq("test2 answer")
+      end
     end
 
-    it "returns the answer for the first step" do
-      expect(answer_store.get_stored_answer(step)).to eq("test answer")
+    context "when the answer was stored using the database_id as the key" do
+      let(:step_id) { "abcABC12" }
+      let(:step_answer_store_key) { 123 }
+      let(:answer) { "test answer" }
+      let(:store) do
+        {
+          answers: {
+            form_id.to_s => {
+              step_answer_store_key.to_s => answer,
+            },
+          },
+        }
+      end
+
+      context "when the step has a database_id" do
+        let(:database_id) { step_answer_store_key }
+
+        it "gets the answer by the database ID" do
+          expect(answer_store.get_stored_answer(step)).to eq(answer)
+        end
+      end
+
+      context "when the step does not have a database_id" do
+        let(:database_id) { nil }
+
+        it "returns nil" do
+          expect(answer_store.get_stored_answer(step)).to be_nil
+        end
+      end
     end
 
-    it "returns the answer for the second step" do
-      expect(answer_store.get_stored_answer(other_step)).to eq("test2 answer")
+    context "when there are answers stored using both the id and database_id of the step" do
+      let(:step_id) { "abcABC12" }
+      let(:database_id) { 123 }
+      let(:answer) { "test answer" }
+
+      let(:store) do
+        {
+          answers: {
+            form_id.to_s => {
+              database_id.to_s => "an answer we will ignore",
+              step_id => answer,
+            },
+          },
+        }
+      end
+
+      it "returns the answer stored using the step id" do
+        expect(answer_store.get_stored_answer(step)).to eq(answer)
+      end
     end
   end
 
diff --git a/spec/models/form_spec.rb b/spec/models/form_spec.rb
@@ -90,6 +90,19 @@
         expect(pages[0]).to have_attributes(id: 9, next_page: 10, answer_type: "date", question_text: "Question one")
         expect(pages[1]).to have_attributes(id: 10, answer_type: "address", question_text: "Question two")
       end
+
+      context "when the steps have a database_id" do
+        let(:steps) do
+          [
+            { id: "abc123", next_step_id: 10, database_id: 9, type: "question_page", data: { answer_type: "date", question_text: "Question one" } },
+          ]
+        end
+
+        it "includes the database_id in the pages" do
+          pages = form.pages
+          expect(pages[0]).to have_attributes(id: "abc123", database_id: 9)
+        end
+      end
     end
   end
 
diff --git a/spec/models/page_spec.rb b/spec/models/page_spec.rb
diff --git a/spec/models/step_spec.rb b/spec/models/step_spec.rb
diff --git a/spec/requests/forms/base_controller_spec.rb b/spec/requests/forms/base_controller_spec.rb
diff --git a/spec/requests/forms/page_controller_spec.rb b/spec/requests/forms/page_controller_spec.rb

Original file line number	Diff line number	Diff line change
`@@ -27,6 +27,7 @@ def pages`
`27`	`27`	`"id" => step["id"],`
`28`	`28`	`"position" => step["position"],`
`29`	`29`	`"next_page" => step["next_step_id"],`
	`30`	`+ "database_id" => step["database_id"], # temporary attribute while we switch to using external IDs`
`30`	`31`	`}`
`31`	`32`	`if step["type"] == "question_page"`
`32`	`33`	`attrs.merge!(step["data"])`