Integration and functionality testing

- Handle blank important note saving, whether an important note is present.
- Tests for New Edition and Edition History functionality are in place. These will have to be un-skipped in the future, when the functionality is ready.
- Refactor for PR comments

app/controllers/notes_controller.rb

@@ -22,11 +22,6 @@ def create
     end
   end
 
-  def resolve
-    resolve_important_note
-    redirect_to history_edition_path(parent)
-  end
-
   def resource
     parent
   end
@@ -38,5 +33,6 @@ def resolve_important_note
       current_user.resolve_important_note(parent)
       flash[:success] = "Note resolved"
     end
+    redirect_to history_edition_path(parent)
   end
 end

app/views/editions/_important_note.html.erb

@@ -1,6 +1,9 @@
+<% note_author_name = User.find_by(id: @edition.important_note&.requester_id).name %>
+<% note_date = @edition.important_note&.created_at&.strftime("%d %B %Y") %>
+<% note_details = @edition.important_note&.comment %>
 <%= render "govuk_publishing_components/components/notice", {
-  description_govspeak: sanitize("<p class='govuk-body govuk-!-font-weight-bold govuk-!-text-break-word'>#{h(@edition.important_note&.comment)}</p>") +
-    sanitize("<p class='govuk-body-m'>#{h(User.find_by(id: @edition.important_note&.requester_id)&.name)} added an important note on
-                  #{h(@edition.important_note&.created_at&.strftime("%d %B %Y"))}</p>"),
-  show_banner_title: true
+  description_govspeak:
+    sanitize("<p class='govuk-body govuk-!-font-weight-bold govuk-!-text-break-word'>#{h(note_details)}</p>") +
+      sanitize("<p class='govuk-body-m'>#{h(note_author_name)} added an important note on #{h(note_date)}</p>"),
+  show_banner_title: true,
 } %>

app/views/editions/secondary_nav_tabs/update_important_note.html.erb

@@ -5,11 +5,13 @@
 
 <div class="govuk-grid-row">
   <div class="govuk-grid-column-two-thirds">
-    <p class="govuk-body">Add important notes that anyone who works on this edition needs to see, eg “(Doesn’t) need fact check, don’t publish.”.
-      Each edition can have only one important note at a time.</p>
+    <p class="govuk-body">
+      Add important notes that anyone who works on this edition needs to see, eg “(Doesn’t) need fact check, don’t publish.”.
+      Each edition can have only one important note at a time.
+    </p>
 
-    <%= form_for(:note, :url=> notes_path) do |f| %>
-      <%= hidden_field_tag :edition_id, resource.id %>
+    <%= form_for(:note, :url=> notes_path) do %>
+      <%= hidden_field_tag :edition_id, @edition.id %>
       <%= hidden_field_tag "note[type]", Action::IMPORTANT_NOTE %>
 
       <%= render "govuk_publishing_components/components/textarea", {

app/views/editions/show.html.erb

@@ -20,17 +20,12 @@
 
 <div class="govuk-grid-row">
   <div class="govuk-grid-column-two-thirds editions__edit__summary">
-      <%= render "govuk_publishing_components/components/summary_list", {
-        items: document_summary_items(@resource),
-      } %>
-  </div>
-
-  <div>
-    <%= render "govuk_publishing_components/components/notice", {
-      title: @edition.important_note.comment,
-      description_govspeak: sanitize("<p>#{User.find_by(id: @edition.important_note.requester_id).name} added an important note on #{@edition.important_note.created_at.strftime("%d %B %Y")}</p>"),
-      show_banner_title: true
+    <%= render "govuk_publishing_components/components/summary_list", {
+      items: document_summary_items(@resource),
     } %>
+    <% if @edition.important_note %>
+      <%= render partial: "important_note" %>
+    <% end %>
   </div>
 
   <% if @edition.in_review? && current_user.has_editor_permissions?(@edition) %>

test/functional/notes_controller_test.rb

@@ -49,7 +49,7 @@ class NotesControllerTest < ActionController::TestCase
       end
     end
 
-    context "when an Important note is provided" do
+    context "when an important note is provided" do
       should "confirm the note was successfully recorded" do
         post :create,
              params: {
@@ -66,9 +66,50 @@ class NotesControllerTest < ActionController::TestCase
         assert_redirected_to history_edition_path(@edition)
         assert_includes flash[:success], "Note recorded"
       end
-    end
 
-    # TODO: Test that a blank note is saved and message is "Note resolved"
+      should "resolve important note if an empty note is saved, when an important note is already present" do
+        post :create,
+             params: {
+               edition_id: @edition.id,
+               note: {
+                 type: "important_note",
+                 comment: @note_text,
+               },
+             }
+
+        @edition.reload
+
+        post :create,
+             params: {
+               edition_id: @edition.id,
+               note: {
+                 type: "important_note",
+                 comment: "",
+               },
+             }
+
+        @edition.reload
+
+        assert_equal("", @edition.important_note.comment)
+        assert_redirected_to history_edition_path(@edition)
+        assert_includes flash[:success], "Note resolved"
+      end
+
+      should "redirect to edit page if user saves an empty note, when there's no important note on the document already" do
+        post :create,
+             params: {
+               edition_id: @edition.id,
+               note: {
+                 type: "important_note",
+                 comment: "",
+               },
+             }
+
+        @edition.reload
+
+        assert_redirected_to history_edition_path(@edition)
+      end
+    end
 
     context "Welsh editors" do
       setup do

test/integration/edition_edit_test.rb

@@ -14,11 +14,9 @@ class EditionEditTest < IntegrationTest
   end
 
   context "edit page" do
-    setup do
+    should "show all the tabs when user has required permission and edition is published" do
       visit_published_edition
-    end
 
-    should "show all the tabs when user has required permission and edition is published" do
       assert page.has_text?("Edit")
       assert page.has_text?("Tagging")
       assert page.has_text?("Metadata")
@@ -29,6 +27,8 @@ class EditionEditTest < IntegrationTest
     end
 
     should "show document summary and title" do
+      visit_published_edition
+
       assert page.has_title?("Edit page title")
 
       row = find_all(".govuk-summary-list__row")
@@ -41,6 +41,8 @@ class EditionEditTest < IntegrationTest
     end
 
     should "indicate when an edition does not have an assignee" do
+      visit_published_edition
+
       within all(".govuk-summary-list__row")[0] do
         assert_selector(".govuk-summary-list__key", text: "Assigned to")
         assert_selector(".govuk-summary-list__value", text: "None")
@@ -57,13 +59,27 @@ class EditionEditTest < IntegrationTest
     end
 
     should "display the important note if an important note exists" do
-      setup do
-        @note_text = "This is really really urgent!"
-        create_important_note_for_edition(@published_edition, @note_text)
-      end
+      note_text = "This is really really urgent!"
+      create_draft_edition
+      create_important_note_for_edition(@draft_edition, note_text)
+      visit edition_path(@draft_edition)
+
+      assert page.has_text?("Important")
+      assert page.has_text?(note_text)
+    end
+
+    should "display only the most recent important note at the top" do
+      first_note = "This is really really urgent!"
+      second_note = "This should display only!"
+      create_draft_edition
+      create_important_note_for_edition(@draft_edition, first_note)
+      create_important_note_for_edition(@draft_edition, second_note)
+
+      visit edition_path(@draft_edition)
 
       assert page.has_text?("Important")
-      assert page.has_text?(@note_text)
+      assert page.has_text?(second_note)
+      assert page.has_no_text?(first_note)
     end
   end
 
@@ -182,13 +198,10 @@ class EditionEditTest < IntegrationTest
   end
 
   context "Update important note page" do
-    setup do
-      visit_draft_edition
-      click_link("History and notes")
-      click_link("Update important note")
-    end
-
     should "render the 'Update important note' page" do
+      create_draft_edition
+      visit history_update_important_note_edition_path(@draft_edition)
+
       within :css, ".gem-c-heading" do
         assert page.has_css?("h1", text: "Update important note")
         assert page.has_css?(".gem-c-heading__context", text: @draft_edition.title)
@@ -207,10 +220,43 @@ class EditionEditTest < IntegrationTest
     end
 
     should "pre-populate with the existing note" do
-      create_important_note_for_edition(@draft_edition, "An updated note")
-      click_link("Update important note")
+      note_text = "This is really really urgent!"
+      create_draft_edition
+      create_important_note_for_edition(@draft_edition, note_text)
+      visit history_update_important_note_edition_path(@draft_edition)
+
+      assert page.has_field?("Important note", with: note_text)
+    end
+
+    # TODO: Test to be switched on when the Edition notes history is implemented.
+    should "not show important notes in edition history" do
+      skip "Until this functionality is complete: #603 - History and notes tab (excluding sidebar) [Edit page]"
+      note_text = "This is really really urgent!"
+      note_text_2 = "Another note"
+      note_text_3 = "Yet another note"
+      create_draft_edition
+      create_important_note_for_edition(@draft_edition, note_text)
+      create_important_note_for_edition(@draft_edition, note_text_2)
+      create_important_note_for_edition(@draft_edition, note_text_3)
+      visit edition_path(@draft_edition)
+      click_link("History and notes")
+
+      # TODO: Expand 'All Notes' sections! Currently in progress.
+      # New asserts go here
+    end
+
+    # TODO: Test to be switched on when the "Create new edition" functionality has been implemented.
+    should "not be carried forward to new editions" do
+      skip "Until this functionality is complete: #601 - Edit page for Published edition (answer and help)"
+      note_text = "This important note should not appear on a new edition."
+      create_published_edition
+      create_important_note_for_edition(@published_edition, note_text)
+      visit edition_path(@published_edition)
 
-      assert page.has_field?("Important note", with: "An updated note")
+      assert page.has_content? note_text
+
+      click_on "Create new edition"
+      assert page.has_no_text?("Important note")
     end
   end
 
@@ -854,8 +900,12 @@ class EditionEditTest < IntegrationTest
 
 private
 
-  def visit_draft_edition
+  def create_draft_edition
     @draft_edition = FactoryBot.create(:edition, title: "Edit page title", state: "draft", overview: "metatags", in_beta: 1, body: "The body")
+  end
+
+  def visit_draft_edition
+    create_draft_edition
     visit edition_path(@draft_edition)
   end
 
@@ -928,7 +978,6 @@ def create_published_edition
       state: "published",
       slug: "can-i-get-a-driving-licence",
     )
-    visit edition_path(@published_edition)
   end
 
   def visit_retired_edition_in_published
@@ -962,6 +1011,7 @@ def visit_old_edition_of_published_edition
   def create_important_note_for_edition(edition, note_text)
     FactoryBot.create(
       :action,
+      requester: @govuk_editor,
       request_type: Action::IMPORTANT_NOTE,
       edition: edition,
       comment: note_text,

test/support/factories.rb

@@ -384,4 +384,10 @@
       end
     end
   end
+
+  factory :action do
+    request_type { Action::IMPORTANT_NOTE }
+    edition
+    comment { "Default comment" }
+  end
 end