Merge pull request #9992 from alphagov/content-modelling/936-dont-all…

…ow-new-embedded-objects-to-be-created-when-editing

(936) Don't allow new embedded objects to be created when editing

lib/engines/content_block_manager/app/assets/stylesheets/content_block_manager/views/_content_block_manager_show.scss

@@ -14,7 +14,7 @@
 
   &__create-button-wrapper {
     display: flex;
-    align-self: flex-start;
+    align-self: start;
     justify-content: flex-end;
   }
 }

lib/engines/content_block_manager/app/controllers/concerns/workflow/show_methods.rb

@@ -83,6 +83,7 @@ def back_path
   def embedded_objects(subschema_name)
     @subschema = @schema.subschema(subschema_name)
     @step_name = current_step.name
+    @action = @content_block_edition.document.is_new_block? ? "Add" : "Edit"
 
     if @subschema
       render :embedded_objects

lib/engines/content_block_manager/app/controllers/concerns/workflow/steps.rb

@@ -8,14 +8,16 @@ module Workflow::Steps
   def steps
     @steps ||= if @schema.subschemas.any?
                  standard_steps = all_steps.map(&:dup)
-                 extra_steps = @schema.subschemas.map do |subschema|
+                 extra_steps = @schema.subschemas.map { |subschema|
+                   next if skip_subschema?(subschema)
+
                    Workflow::Step.new(
                      "#{Workflow::Step::SUBSCHEMA_PREFIX}#{subschema.id}".to_sym,
                      "#{Workflow::Step::SUBSCHEMA_PREFIX}#{subschema.id}".to_sym,
                      :redirect_to_next_step,
                      true,
                    )
-                 end
+                 }.compact
                  standard_steps.insert(1, extra_steps).flatten!
                else
                  all_steps
@@ -50,6 +52,11 @@ def initialize_edition_and_schema
   end
 
   def index
-    steps.find_index { |step| step.name == params[:step].to_sym }
+    steps.find_index { |step| step.name == params[:step]&.to_sym } || 0
+  end
+
+  def skip_subschema?(subschema)
+    !@content_block_edition.document.is_new_block? &&
+      !@content_block_edition.has_entries_for_subschema_id?(subschema.id)
   end
 end

lib/engines/content_block_manager/app/controllers/content_block_manager/content_block/editions_controller.rb

@@ -1,4 +1,8 @@
 class ContentBlockManager::ContentBlock::EditionsController < ContentBlockManager::BaseController
+  include Workflow::Steps
+
+  skip_before_action :initialize_edition_and_schema
+
   def new
     if params[:document_id]
       @title = "Edit a content block"
@@ -18,8 +22,8 @@ def new
 
   def create
     @schema = ContentBlockManager::ContentBlock::Schema.find_by_block_type(block_type_param)
-    new_edition = ContentBlockManager::CreateEditionService.new(@schema).call(edition_params, document_id: params[:document_id])
-    redirect_to content_block_manager.content_block_manager_content_block_workflow_path(id: new_edition.id, step: next_step)
+    @content_block_edition = ContentBlockManager::CreateEditionService.new(@schema).call(edition_params, document_id: params[:document_id])
+    redirect_to content_block_manager.content_block_manager_content_block_workflow_path(id: @content_block_edition.id, step: next_step.name)
   rescue ActiveRecord::RecordInvalid => e
     @form = ContentBlockManager::ContentBlock::EditionForm.for(content_block_edition: e.record, schema: @schema)
     render "content_block_manager/content_block/editions/new"
@@ -36,14 +40,4 @@ def destroy
   def block_type_param
     params.require("content_block/edition").require("document_attributes").require(:block_type)
   end
-
-  def next_step
-    if @schema.subschemas.any?
-      "embedded_#{@schema.subschemas.first.id}"
-    elsif params[:document_id]
-      :review_links
-    else
-      :review
-    end
-  end
 end

lib/engines/content_block_manager/app/models/content_block_manager/content_block/edition.rb

@@ -59,6 +59,10 @@ def update_object_with_details(object_type, object_name, body)
       def key_for_object(object)
         object["name"]&.parameterize.presence || SecureRandom.alphanumeric.downcase
       end
+
+      def has_entries_for_subschema_id?(subschema_id)
+        details[subschema_id].present?
+      end
     end
   end
 end

lib/engines/content_block_manager/app/views/content_block_manager/content_block/editions/workflow/embedded_objects.html.erb

@@ -1,5 +1,5 @@
 <% content_for :context, context %>
-<% content_for :title, "Add #{@subschema.name.humanize(capitalize: false)}" %>
+<% content_for :title, "#{@action} #{@subschema.name.humanize(capitalize: false)}" %>
 <% content_for :title_margin_bottom, 4 %>
 <% content_for :back_link do %>
   <%= render "govuk_publishing_components/components/back_link", {
@@ -27,20 +27,24 @@
           </div>
           <% end %>
       <% else %>
-        <%= render "govuk_publishing_components/components/hint", {
-          text: "Create #{@subschema.name.downcase} for this #{@schema.name}. Alternatively, #{@subschema.name.downcase} can be created and edited later too.",
-        } %>
+        <% if I18n.exists?("content_block_edition.create.embedded_objects.#{@subschema.id}") %>
+          <%= render "govuk_publishing_components/components/hint", {
+            text: t("content_block_edition.create.embedded_objects.#{@subschema.id}"),
+          } %>
+        <% end %>
       <% end %>
 
-      <%= render "govuk_publishing_components/components/button", {
-        text: "Add a #{@subschema.name.singularize.downcase}",
-        href: content_block_manager.new_embedded_object_content_block_manager_content_block_edition_path(
-          @content_block_edition,
-          object_type: @subschema.block_type,
-        ),
-        secondary_solid: true,
-        margin_bottom: 6,
-      } %>
+      <% if @content_block_edition.document.is_new_block? %>
+        <%= render "govuk_publishing_components/components/button", {
+          text: "Add a #{@subschema.name.singularize.downcase}",
+          href: content_block_manager.new_embedded_object_content_block_manager_content_block_edition_path(
+            @content_block_edition,
+            object_type: @subschema.block_type,
+          ),
+          secondary_solid: true,
+          margin_bottom: 6,
+        } %>
+      <% end %>
     <% end %>
 
     <%= render ContentBlockManager::Shared::ContinueOrCancelButtonGroup.new(

lib/engines/content_block_manager/config/locales/en.yml

@@ -38,6 +38,8 @@ en:
       title: "Edit %{block_type}"
     create:
       title: "Create %{block_type}"
+      embedded_objects:
+        rates: Enter rate details, including the amount. You can do this now or later.
     confirmation_page:
       scheduled:
         banner: "%{block_type} scheduled to publish on %{date}"

lib/engines/content_block_manager/features/create_pension_object.feature

@@ -21,7 +21,7 @@ Feature: Create a content object
     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  |
-    Then I should be on the "embedded_rates" step
+    Then I should be on the "add_embedded_rates" step
     When I save and continue
     Then I am asked to review my answers for a "pension"
     And I review and confirm my answers are correct
@@ -41,7 +41,7 @@ Feature: Create a content object
     And I complete the "rate" form with the following fields:
       | name     | amount  | frequency |
       | New rate | £127.91 | a month  |
-    Then I should be on the "embedded_rates" step
+    Then I should be on the "add_embedded_rates" step
     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"
@@ -57,7 +57,7 @@ Feature: Create a content object
       | my basic pension | this is basic | Ministry of Example | this is important  |
     When I click to add a new "rate"
     And I click the cancel link
-    Then I should be on the "embedded_rates" step
+    Then I should be on the "add_embedded_rates" step
 
   Scenario: GDS editor clicks back and is taken back to rates
     When I visit the Content Block Manager home page
@@ -68,5 +68,5 @@ Feature: Create a content object
       | 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
+    Then I should be on the "add_embedded_rates" step
 

lib/engines/content_block_manager/features/edit_pension_object.feature

@@ -23,13 +23,14 @@ Feature: Edit a pension object
     Then I should be on the "edit" step
     And I should see a back link to the document page
     When I fill out the form
-    Then I should be on the "embedded_rates" step
+    Then I should be on the "edit_embedded_rates" step
     And I should see the rates for that block
+    And I should not see a button to add a new "rate"
     When I click to edit the first rate
     When I complete the "rate" form with the following fields:
       | name    | amount  | frequency |
       | My rate | £122.50 | a week    |
-    Then I should be on the "embedded_rates" step
+    Then I should be on the "edit_embedded_rates" step
     And I should see the updated rates for that block
     When I save and continue
     Then I should be on the "review_links" step
@@ -55,27 +56,14 @@ Feature: Edit a pension object
     And I should see the edition diff in a table
     And I should see details of my "rate"
 
-  Scenario: GDS Editor edits a pension object and creates a new rate
+  Scenario: Rate steps are skipped when there has been no rates added
+    Given my pension content block has no rates
     When I visit the Content Block Manager home page
     And I click to view the document
     And I click to edit the "pension"
     When I fill out the form
-    And I click to add a new "rate"
-    And I complete the "rate" form with the following fields:
-      | name     | amount  | frequency |
-      | New rate | £127.91 | a month   |
-    Then I should be on the "embedded_rates" step
-    And I should see the updated rates for that block
-    When I save and continue
-    And I continue after reviewing the links
-    And I add an internal note
-    And I add a change note
-    And I choose to publish the change now
-    When I review and confirm my answers are correct
-    Then I should be taken to the confirmation page for a published block
-    When I click to view the content block
-    Then the edition should have been updated successfully
-    And I should see details of my "rate"
+    Then I should be on the "review_links" step
+    And I should see a back link to the "edit_draft" step
 
   Scenario: GDS editor sees notification about an in-progress draft
     When I visit the Content Block Manager home page

lib/engines/content_block_manager/features/step_definitions/content_block_manager_steps.rb

@@ -505,3 +505,8 @@
 And(/^I click the back link$/) do
   click_on "Back"
 end
+
+Given(/^my pension content block has no rates$/) do
+  @content_block.details["rates"] = {}
+  @content_block.save!
+end

lib/engines/content_block_manager/features/step_definitions/embedded_object_steps.rb

@@ -112,3 +112,7 @@
     assert_text @details[k]
   end
 end
+
+And("I should not see a button to add a new {string}") do |object_type|
+  assert_no_text "Add a #{object_type}"
+end

lib/engines/content_block_manager/features/step_definitions/form_step_steps.rb

@@ -13,7 +13,9 @@
     should_be_on_review_step(object_type: @content_block.document.block_type)
   when "change_note"
     should_be_on_change_note_step
-  when /#{Workflow::Step::SUBSCHEMA_PREFIX}(.*)/
-    should_be_on_subschema_step(::Regexp.last_match(1))
+  when /add_#{Workflow::Step::SUBSCHEMA_PREFIX}(.*)/
+    should_be_on_subschema_step(::Regexp.last_match(1), "Add")
+  when /edit_#{Workflow::Step::SUBSCHEMA_PREFIX}(.*)/
+    should_be_on_subschema_step(::Regexp.last_match(1), "Edit")
   end
 end

lib/engines/content_block_manager/features/support/form_step_helpers.rb

@@ -44,6 +44,6 @@ def should_be_on_change_note_step
   assert_text "Do users have to know the content has changed?"
 end
 
-def should_be_on_subschema_step(subschema)
-  assert_text "Add #{subschema.humanize(capitalize: false)}"
+def should_be_on_subschema_step(subschema, prefix)
+  assert_text "#{prefix} #{subschema.humanize(capitalize: false)}"
 end

lib/engines/content_block_manager/test/integration/content_block/editions_test.rb

@@ -132,6 +132,10 @@ class ContentBlockManager::ContentBlock::EditionsTest < ActionDispatch::Integrat
         let(:subschemas) { [stub("subschema", id: "my_subschema_name")] }
         let!(:schema) { stub_request_for_schema("email_address", subschemas:) }
 
+        before do
+          ContentBlockManager::ContentBlock::Edition.any_instance.stubs(:has_entries_for_subschema_id?).with("my_subschema_name").returns(true)
+        end
+
         it "redirects to the first subschema step when successful" do
           redirects_to_step(:embedded_my_subschema_name) do
             post content_block_manager.content_block_manager_content_block_document_editions_path(content_block_document), params: {
@@ -147,6 +151,10 @@ class ContentBlockManager::ContentBlock::EditionsTest < ActionDispatch::Integrat
     end
 
     describe "when creating a new block" do
+      before do
+        ContentBlockManager::ContentBlock::Document.any_instance.stubs(:is_new_block?).returns(true)
+      end
+
       it "creates a new content block with params generated by the schema" do
         post content_block_manager.content_block_manager_content_block_editions_path, params: {
           something: "else",

lib/engines/content_block_manager/test/integration/content_block/workflow_test.rb

@@ -313,6 +313,10 @@ class ContentBlockManager::ContentBlock::WorkflowTest < ActionDispatch::Integrat
 
         let!(:schema) { stub_request_for_schema("email_address", subschemas:) }
 
+        before do
+          ContentBlockManager::ContentBlock::Edition.any_instance.stubs(:has_entries_for_subschema_id?).returns(true)
+        end
+
         describe "#show" do
           it "shows the form for the first subschema" do
             get content_block_manager.content_block_manager_content_block_workflow_path(id: edition.id, step: "embedded_subschema_1")

lib/engines/content_block_manager/test/unit/app/controllers/concerns/workflow/steps_test.rb

@@ -89,7 +89,7 @@ class Workflow::StepsTest < ActionDispatch::IntegrationTest
     let(:step) { "something" }
 
     before do
-      content_block_document.expects(:is_new_block?).returns(true)
+      content_block_document.expects(:is_new_block?).at_least_once.returns(true)
     end
 
     it "removes steps not included in the create journey" do
@@ -142,6 +142,11 @@ class Workflow::StepsTest < ActionDispatch::IntegrationTest
 
     let(:step) { "something" }
 
+    before do
+      content_block_edition.stubs(:has_entries_for_subschema_id?).with("something").returns(true)
+      content_block_edition.stubs(:has_entries_for_subschema_id?).with("something_else").returns(true)
+    end
+
     describe "#steps" do
       it "inserts the subschemas into the flow" do
         assert_equal workflow.steps, [
@@ -151,6 +156,21 @@ class Workflow::StepsTest < ActionDispatch::IntegrationTest
           Workflow::Step::ALL[1..],
         ].flatten
       end
+
+      describe "when there are entries missing for a given subschema" do
+        before do
+          content_block_edition.stubs(:has_entries_for_subschema_id?).with("something").returns(false)
+          content_block_edition.stubs(:has_entries_for_subschema_id?).with("something_else").returns(true)
+        end
+
+        it "skips the subschemas without data" do
+          assert_equal workflow.steps, [
+            Workflow::Step::ALL[0],
+            Workflow::Step.new(:embedded_something_else, :embedded_something_else, :redirect_to_next_step, true),
+            Workflow::Step::ALL[1..],
+          ].flatten
+        end
+      end
     end
 
     describe "#next_step" do
@@ -196,7 +216,7 @@ class Workflow::StepsTest < ActionDispatch::IntegrationTest
 
     describe "and the content block is new" do
       before do
-        content_block_document.expects(:is_new_block?).returns(true)
+        content_block_document.expects(:is_new_block?).at_least_once.returns(true)
       end
 
       it "removes steps not included in the create journey" do

lib/engines/content_block_manager/test/unit/app/models/content_block_edition_test.rb

@@ -269,4 +269,18 @@ class ContentBlockManager::ContentBlockEditionTest < ActiveSupport::TestCase
       assert_equal content_block_edition.first_class_details, { "my" => "details" }
     end
   end
+
+  describe "#has_entries_for_subschema_id?" do
+    it "returns false when there are no entries for a subschema ID" do
+      content_block_edition.details["foo"] = {}
+
+      assert_not content_block_edition.has_entries_for_subschema_id?("foo")
+    end
+
+    it "returns true when there entries for a subschema ID" do
+      content_block_edition.details["foo"] = { "something" => { "foo" => "bar" } }
+
+      assert content_block_edition.has_entries_for_subschema_id?("foo")
+    end
+  end
 end