From 27fc8e826a2a63f109df4eb2977d933c2539411c Mon Sep 17 00:00:00 2001 From: pezholio Date: Fri, 21 Feb 2025 16:48:03 +0000 Subject: [PATCH 1/2] Return list of steps when a content block is new This will give us a reliable list of steps including all the subschema steps (if relevant) --- .../app/controllers/concerns/workflow.rb | 16 +-- .../controllers/concerns/workflow/steps.rb | 13 ++- .../concerns/workflow/steps_test.rb | 98 ++++++++++++++++++- 3 files changed, 115 insertions(+), 12 deletions(-) diff --git a/lib/engines/content_block_manager/app/controllers/concerns/workflow.rb b/lib/engines/content_block_manager/app/controllers/concerns/workflow.rb index 0fe4dcdda0a..fbd6fc9a2a6 100644 --- a/lib/engines/content_block_manager/app/controllers/concerns/workflow.rb +++ b/lib/engines/content_block_manager/app/controllers/concerns/workflow.rb @@ -1,15 +1,15 @@ module Workflow - class Step < Data.define(:name, :show_action, :update_action) + class Step < Data.define(:name, :show_action, :update_action, :included_in_create_journey) SUBSCHEMA_PREFIX = "embedded_".freeze ALL = [ - Step.new(:edit_draft, :edit_draft, :update_draft), - Step.new(:review_links, :review_links, :redirect_to_next_step), - Step.new(:internal_note, :internal_note, :update_internal_note), - Step.new(:change_note, :change_note, :update_change_note), - Step.new(:schedule_publishing, :schedule_publishing, :validate_schedule), - Step.new(:review, :review, :validate_review_page), - Step.new(:confirmation, :confirmation, nil), + Step.new(:edit_draft, :edit_draft, :update_draft, true), + Step.new(:review_links, :review_links, :redirect_to_next_step, false), + Step.new(:internal_note, :internal_note, :update_internal_note, false), + Step.new(:change_note, :change_note, :update_change_note, false), + Step.new(:schedule_publishing, :schedule_publishing, :validate_schedule, false), + Step.new(:review, :review, :validate_review_page, true), + Step.new(:confirmation, :confirmation, nil, true), ].freeze def is_subschema? diff --git a/lib/engines/content_block_manager/app/controllers/concerns/workflow/steps.rb b/lib/engines/content_block_manager/app/controllers/concerns/workflow/steps.rb index 09385feb312..3dea7b9f7cd 100644 --- a/lib/engines/content_block_manager/app/controllers/concerns/workflow/steps.rb +++ b/lib/engines/content_block_manager/app/controllers/concerns/workflow/steps.rb @@ -7,17 +7,18 @@ module Workflow::Steps def steps @steps ||= if @schema.subschemas.any? - standard_steps = Workflow::Step::ALL.map(&:dup) + standard_steps = all_steps.map(&:dup) extra_steps = @schema.subschemas.map do |subschema| Workflow::Step.new( "#{Workflow::Step::SUBSCHEMA_PREFIX}#{subschema.id}".to_sym, "#{Workflow::Step::SUBSCHEMA_PREFIX}#{subschema.id}".to_sym, :redirect_to_next_subschema_or_continue, + true, ) end standard_steps.insert(1, extra_steps).flatten! else - Workflow::Step::ALL + all_steps end end @@ -35,6 +36,14 @@ def next_step private + def all_steps + if @content_block_edition.document.is_new_block? + Workflow::Step::ALL.select { |s| s.included_in_create_journey == true } + else + Workflow::Step::ALL + end + end + def initialize_edition_and_schema @content_block_edition = ContentBlockManager::ContentBlock::Edition.find(params[:id]) @schema = ContentBlockManager::ContentBlock::Schema.find_by_block_type(@content_block_edition.document.block_type) diff --git a/lib/engines/content_block_manager/test/unit/app/controllers/concerns/workflow/steps_test.rb b/lib/engines/content_block_manager/test/unit/app/controllers/concerns/workflow/steps_test.rb index 016446f5e79..7cb862ac7db 100644 --- a/lib/engines/content_block_manager/test/unit/app/controllers/concerns/workflow/steps_test.rb +++ b/lib/engines/content_block_manager/test/unit/app/controllers/concerns/workflow/steps_test.rb @@ -85,6 +85,51 @@ class Workflow::StepsTest < ActionDispatch::IntegrationTest end end + describe "when the content block is new" do + let(:step) { "something" } + + before do + content_block_document.expects(:is_new_block?).returns(true) + end + + it "removes steps not included in the create journey" do + assert_equal workflow.steps, [ + Workflow::Step.new(:edit_draft, :edit_draft, :update_draft, true), + Workflow::Step.new(:review, :review, :validate_review_page, true), + Workflow::Step.new(:confirmation, :confirmation, nil, true), + ].flatten + end + + describe "#next_step" do + [ + %i[edit_draft review], + %i[review confirmation], + ].each do |current_step, expected_step| + describe "when current_step is #{current_step}" do + let(:step) { current_step } + + it "returns #{expected_step} step" do + assert_equal workflow.next_step.name, expected_step + end + end + end + end + + describe "#previous_step" do + [ + %i[review edit_draft], + ].each do |current_step, expected_step| + describe "when current_step is #{current_step}" do + let(:step) { current_step } + + it "returns #{expected_step} step" do + assert_equal workflow.previous_step.name, expected_step + end + end + end + end + end + describe "when a schema has subschemas" do let(:subschemas) do [ @@ -101,8 +146,8 @@ class Workflow::StepsTest < ActionDispatch::IntegrationTest it "inserts the subschemas into the flow" do assert_equal workflow.steps, [ Workflow::Step::ALL[0], - Workflow::Step.new(:embedded_something, :embedded_something, :redirect_to_next_subschema_or_continue), - Workflow::Step.new(:embedded_something_else, :embedded_something_else, :redirect_to_next_subschema_or_continue), + Workflow::Step.new(:embedded_something, :embedded_something, :redirect_to_next_subschema_or_continue, true), + Workflow::Step.new(:embedded_something_else, :embedded_something_else, :redirect_to_next_subschema_or_continue, true), Workflow::Step::ALL[1..], ].flatten end @@ -148,5 +193,54 @@ class Workflow::StepsTest < ActionDispatch::IntegrationTest end end end + + describe "and the content block is new" do + before do + content_block_document.expects(:is_new_block?).returns(true) + end + + it "removes steps not included in the create journey" do + assert_equal workflow.steps, [ + Workflow::Step.new(:edit_draft, :edit_draft, :update_draft, true), + Workflow::Step.new(:embedded_something, :embedded_something, :redirect_to_next_subschema_or_continue, true), + Workflow::Step.new(:embedded_something_else, :embedded_something_else, :redirect_to_next_subschema_or_continue, true), + Workflow::Step.new(:review, :review, :validate_review_page, true), + Workflow::Step.new(:confirmation, :confirmation, nil, true), + ].flatten + end + + describe "#next_step" do + [ + %i[edit_draft embedded_something], + %i[embedded_something embedded_something_else], + %i[embedded_something_else review], + %i[review confirmation], + ].each do |current_step, expected_step| + describe "when current_step is #{current_step}" do + let(:step) { current_step } + + it "returns #{expected_step} step" do + assert_equal workflow.next_step.name, expected_step + end + end + end + end + + describe "#previous_step" do + [ + %i[embedded_something edit_draft], + %i[embedded_something_else embedded_something], + %i[review embedded_something_else], + ].each do |current_step, expected_step| + describe "when current_step is #{current_step}" do + let(:step) { current_step } + + it "returns #{expected_step} step" do + assert_equal workflow.previous_step.name, expected_step + end + end + end + end + end end end From 63563476dca95678db4d448e18ce489a9e87f96c Mon Sep 17 00:00:00 2001 From: pezholio Date: Fri, 21 Feb 2025 17:01:52 +0000 Subject: [PATCH 2/2] Remove logic to check new block in workflow Now the steps are set correctly for new blocks, we can navigate forwards and backwards safely. --- .../concerns/workflow/show_methods.rb | 9 +--- .../controllers/concerns/workflow/steps.rb | 2 +- .../concerns/workflow/update_methods.rb | 25 +--------- .../features/create_pension_object.feature | 12 +++++ .../content_block_manager_steps.rb | 4 ++ .../concerns/workflow/show_methods_test.rb | 46 ++++--------------- .../concerns/workflow/steps_test.rb | 8 ++-- 7 files changed, 32 insertions(+), 74 deletions(-) diff --git a/lib/engines/content_block_manager/app/controllers/concerns/workflow/show_methods.rb b/lib/engines/content_block_manager/app/controllers/concerns/workflow/show_methods.rb index 2bbd01c64c8..e48bba7cf03 100644 --- a/lib/engines/content_block_manager/app/controllers/concerns/workflow/show_methods.rb +++ b/lib/engines/content_block_manager/app/controllers/concerns/workflow/show_methods.rb @@ -69,12 +69,9 @@ def confirmation end def back_path - step = is_reviewing_new_block? ? :edit_draft : previous_step.name - return nil unless step - content_block_manager.content_block_manager_content_block_workflow_path( @content_block_edition, - step:, + step: previous_step.name, ) end included do @@ -83,10 +80,6 @@ def back_path private - def is_reviewing_new_block? - current_step.name == :review && @content_block_edition.document.is_new_block? - end - def embedded_objects(subschema_name) @subschema = @schema.subschema(subschema_name) @step_name = current_step.name diff --git a/lib/engines/content_block_manager/app/controllers/concerns/workflow/steps.rb b/lib/engines/content_block_manager/app/controllers/concerns/workflow/steps.rb index 3dea7b9f7cd..5420a82b097 100644 --- a/lib/engines/content_block_manager/app/controllers/concerns/workflow/steps.rb +++ b/lib/engines/content_block_manager/app/controllers/concerns/workflow/steps.rb @@ -12,7 +12,7 @@ def steps Workflow::Step.new( "#{Workflow::Step::SUBSCHEMA_PREFIX}#{subschema.id}".to_sym, "#{Workflow::Step::SUBSCHEMA_PREFIX}#{subschema.id}".to_sym, - :redirect_to_next_subschema_or_continue, + :redirect_to_next_step, true, ) end diff --git a/lib/engines/content_block_manager/app/controllers/concerns/workflow/update_methods.rb b/lib/engines/content_block_manager/app/controllers/concerns/workflow/update_methods.rb index bcd08d49327..aeb20123d9f 100644 --- a/lib/engines/content_block_manager/app/controllers/concerns/workflow/update_methods.rb +++ b/lib/engines/content_block_manager/app/controllers/concerns/workflow/update_methods.rb @@ -14,14 +14,7 @@ def update_draft ) @content_block_edition.save! - if @content_block_edition.document.is_new_block? - redirect_to content_block_manager.content_block_manager_content_block_workflow_path( - id: @content_block_edition.id, - step: :review, - ) - else - redirect_to_next_step - end + redirect_to_next_step rescue ActiveRecord::RecordInvalid @schema = ContentBlockManager::ContentBlock::Schema.find_by_block_type(@content_block_edition.document.block_type) @form = ContentBlockManager::ContentBlock::EditionForm::Edit.new(content_block_edition: @content_block_edition, schema: @schema) @@ -29,22 +22,6 @@ def update_draft render :edit_draft end - def redirect_to_next_subschema_or_continue - @content_block_edition = ContentBlockManager::ContentBlock::Edition.find(params[:id]) - if @content_block_edition.document.is_new_block? - if next_step.is_subschema? - redirect_to_next_step - else - redirect_to content_block_manager.content_block_manager_content_block_workflow_path( - id: @content_block_edition.id, - step: :review, - ) - end - else - redirect_to_next_step - end - end - def validate_schedule @content_block_edition = ContentBlockManager::ContentBlock::Edition.find(params[:id]) diff --git a/lib/engines/content_block_manager/features/create_pension_object.feature b/lib/engines/content_block_manager/features/create_pension_object.feature index e0dd488a7ac..380c3fdba0d 100644 --- a/lib/engines/content_block_manager/features/create_pension_object.feature +++ b/lib/engines/content_block_manager/features/create_pension_object.feature @@ -45,3 +45,15 @@ Feature: Create a content object When I save and continue And I review and confirm my answers are correct Then I should be taken to the confirmation page for a new "pension" + + Scenario: GDS editor clicks back and is taken back to rates + When I visit the Content Block Manager home page + And I click to create an object + And I click on the "pension" schema + When I complete the form with the following fields: + | title | description | organisation | instructions_to_publishers | + | my basic pension | this is basic | Ministry of Example | this is important | + And I click the back link + And I click save + Then I should be on the "embedded_rates" step + diff --git a/lib/engines/content_block_manager/features/step_definitions/content_block_manager_steps.rb b/lib/engines/content_block_manager/features/step_definitions/content_block_manager_steps.rb index 4fb39ae52ca..fed1f406956 100644 --- a/lib/engines/content_block_manager/features/step_definitions/content_block_manager_steps.rb +++ b/lib/engines/content_block_manager/features/step_definitions/content_block_manager_steps.rb @@ -509,3 +509,7 @@ I18n.t("activerecord.errors.models.content_block_manager/content_block/edition.invalid", attribute: attribute.humanize), ) end + +And(/^I click the back link$/) do + click_on "Back" +end diff --git a/lib/engines/content_block_manager/test/unit/app/controllers/concerns/workflow/show_methods_test.rb b/lib/engines/content_block_manager/test/unit/app/controllers/concerns/workflow/show_methods_test.rb index 2aea10257bf..980c40176f3 100644 --- a/lib/engines/content_block_manager/test/unit/app/controllers/concerns/workflow/show_methods_test.rb +++ b/lib/engines/content_block_manager/test/unit/app/controllers/concerns/workflow/show_methods_test.rb @@ -31,47 +31,19 @@ class Workflow::ShowMethodsTest < ActionDispatch::IntegrationTest describe "#back_path" do let(:content_block_edition) { build_stubbed(:content_block_edition) } - let(:stub_document) { stub(:document, is_new_block?: is_new_block) } - before do - content_block_edition.stubs(:document).returns(stub_document) - end - - describe "#back_path" do - describe "when editing an existing block" do - let(:is_new_block) { false } - - it "returns the name of the next step" do - current_step_name = "my_step" - expected_step_name = "something" - - current_step = mock("Workflow::Step", name: current_step_name) - previous_step = mock("Workflow::Step", name: expected_step_name) - - test_class = ShowMethodsTestClass.new(current_step:, previous_step:, content_block_edition:) - - assert_equal test_class.back_path, content_block_manager.content_block_manager_content_block_workflow_path( - content_block_edition, - step: expected_step_name, - ) - end - end - end - - describe "when editing an existing block" do - let(:is_new_block) { true } + it "returns the name of the previous step" do + expected_step_name = "something" - it "returns the edit draft step" do - current_step = mock("Workflow::Step", name: :review) - previous_step = mock("Workflow::Step") + current_step = mock("Workflow::Step") + previous_step = mock("Workflow::Step", name: expected_step_name) - test_class = ShowMethodsTestClass.new(current_step:, previous_step:, content_block_edition:) + test_class = ShowMethodsTestClass.new(current_step:, previous_step:, content_block_edition:) - assert_equal test_class.back_path, content_block_manager.content_block_manager_content_block_workflow_path( - content_block_edition, - step: :edit_draft, - ) - end + assert_equal test_class.back_path, content_block_manager.content_block_manager_content_block_workflow_path( + content_block_edition, + step: expected_step_name, + ) end end end diff --git a/lib/engines/content_block_manager/test/unit/app/controllers/concerns/workflow/steps_test.rb b/lib/engines/content_block_manager/test/unit/app/controllers/concerns/workflow/steps_test.rb index 7cb862ac7db..7cd92456766 100644 --- a/lib/engines/content_block_manager/test/unit/app/controllers/concerns/workflow/steps_test.rb +++ b/lib/engines/content_block_manager/test/unit/app/controllers/concerns/workflow/steps_test.rb @@ -146,8 +146,8 @@ class Workflow::StepsTest < ActionDispatch::IntegrationTest it "inserts the subschemas into the flow" do assert_equal workflow.steps, [ Workflow::Step::ALL[0], - Workflow::Step.new(:embedded_something, :embedded_something, :redirect_to_next_subschema_or_continue, true), - Workflow::Step.new(:embedded_something_else, :embedded_something_else, :redirect_to_next_subschema_or_continue, true), + Workflow::Step.new(:embedded_something, :embedded_something, :redirect_to_next_step, true), + Workflow::Step.new(:embedded_something_else, :embedded_something_else, :redirect_to_next_step, true), Workflow::Step::ALL[1..], ].flatten end @@ -202,8 +202,8 @@ class Workflow::StepsTest < ActionDispatch::IntegrationTest it "removes steps not included in the create journey" do assert_equal workflow.steps, [ Workflow::Step.new(:edit_draft, :edit_draft, :update_draft, true), - Workflow::Step.new(:embedded_something, :embedded_something, :redirect_to_next_subschema_or_continue, true), - Workflow::Step.new(:embedded_something_else, :embedded_something_else, :redirect_to_next_subschema_or_continue, true), + Workflow::Step.new(:embedded_something, :embedded_something, :redirect_to_next_step, true), + Workflow::Step.new(:embedded_something_else, :embedded_something_else, :redirect_to_next_step, true), Workflow::Step.new(:review, :review, :validate_review_page, true), Workflow::Step.new(:confirmation, :confirmation, nil, true), ].flatten