Fix Orphan Document scenario for Document Collection

Document Collection Groups assume that documents added to the document groups always have a latest_edition attached to it since users are only allowed to search for and add published documents to the group. However, since there is behaviour to unpublish and delete the edition, the document is left orphaned, which is currently causing server errors for users who have previously added this document to their Document Collection Group. Fix is to render the unavailable document so that the user would know to remove this orphaned document relationship from their document collection group. Co-Authored-By: Nikin Nagewadia <[email protected]>
alphagov · Nov 9, 2023 · 5eca54c · 5eca54c
1 parent a37d7bd
commit 5eca54c
Show file tree

Hide file tree

Showing 6 changed files with 147 additions and 57 deletions.
diff --git a/app/helpers/admin/document_collection_group_memberships_helper.rb b/app/helpers/admin/document_collection_group_memberships_helper.rb
@@ -1,17 +1,23 @@
 module Admin::DocumentCollectionGroupMembershipsHelper
   def document_collection_group_member_title(membership)
-    if membership.non_whitehall_link
-      membership.non_whitehall_link.title
-    else
-      membership.document.latest_edition.title
-    end
+    return membership.non_whitehall_link.title if membership.non_whitehall_link
+
+    membership.document.latest_edition.title
   end
 
   def document_collection_group_member_url(membership)
-    if membership.non_whitehall_link
-      Plek.website_root + membership.non_whitehall_link.base_path
-    else
-      membership.document.latest_edition.public_url
+    return Plek.website_root + membership.non_whitehall_link.base_path if membership.non_whitehall_link
+
+    membership.document.latest_edition.public_url
+  end
+
+  def document_collection_group_member_unavailable?(membership)
+    !membership.non_whitehall_link && !membership.document&.latest_edition
+  end
+
+  def unavailable_document_count(memberships)
+    memberships.count do |membership|
+      document_collection_group_member_unavailable?(membership)
     end
   end
 end
diff --git a/app/views/admin/document_collection_group_memberships/confirm_destroy.html.erb b/app/views/admin/document_collection_group_memberships/confirm_destroy.html.erb
@@ -4,8 +4,9 @@
 
 <div class="govuk-grid-row">
   <div class="govuk-grid-column-two-thirds">
-    <%= form_with url: admin_document_collection_group_document_collection_group_membership_path(@collection, @group, @membership), method: :delete do |form| %>
-      <p class="govuk-body govuk-!-margin-bottom-6">Are you sure you want to remove "<%= document_collection_group_member_title(@membership) %>" from this collection?</p>
+    <%= form_with url: admin_document_collection_group_document_collection_group_membership_path(@collection, @group, @membership), method: :delete do |_| %>
+      <% title = document_collection_group_member_unavailable?(@membership) ? tag.span("Unavailable Document", class: "govuk-!-font-weight-bold") : document_collection_group_member_title(@membership) %>
+      <p class="govuk-body govuk-!-margin-bottom-6">Are you sure you want to remove "<%= title %>" from this collection?</p>
 
        <div class="govuk-button-group">
         <%= render "govuk_publishing_components/components/button", {

diff --git a/app/views/admin/document_collection_group_memberships/index.html.erb b/app/views/admin/document_collection_group_memberships/index.html.erb
@@ -8,6 +8,19 @@
 <% content_for :context, @collection.title %>
 <% content_for :title_margin_bottom, 4 %>
 
+<% if unavailable_document_count(@group.memberships) > 0 %>
+  <% content_for :banner, render("govuk_publishing_components/components/notice", {
+    title: sanitize("<p>Remove #{unavailable_document_count(@group.memberships)} unavailable document(s) within the group.</p>"),
+    show_banner_title: true,
+    data_attributes: {
+      module: "gem-track-click",
+      "track-category": "important-banner",
+      "track-action": "unavailable-documents-exist-in-document-collection-group",
+      "track-label": "Important",
+    },
+  }) %>
+<% end %>
+
 <div class="govuk-grid-row">
   <div class="govuk-grid-column-two-thirds">
     <p class="govuk-body">
@@ -19,64 +32,50 @@
       items: secondary_navigation_tabs_items(@group, request.path),
     } %>
 
-    <% if @group.memberships.many? %>
-      <div class="govuk-grid-row">
-        <div class="govuk-grid-column-full app-view-document-collection-group-memberships-index__heading ">
-          <%= render "govuk_publishing_components/components/heading", {
-            text: "Documents",
-            margin_bottom: 6,
-          } %>
-          <ul class="govuk-list">
-            <li>
-              <%= link_to "Add document", admin_document_collection_group_search_options_path(@collection, @group), class: "govuk-link" %>
-            </li>
+    <div class="govuk-grid-row">
+      <div class="govuk-grid-column-full app-view-document-collection-group-memberships-index__heading ">
+        <%= render "govuk_publishing_components/components/heading", {
+          text: "Documents",
+          margin_bottom: 6,
+        } %>
+        <ul class="govuk-list">
+          <li>
+            <%= link_to "Add document", admin_document_collection_group_search_options_path(@collection, @group), class: "govuk-link" %>
+          </li>
 
+          <% if @group.memberships.many? %>
             <li>
               <%= link_to "Reorder document", reorder_admin_document_collection_group_document_collection_group_memberships_path(@collection, @group), class: "govuk-link" %>
             </li>
-          </ul>
-        </div>
+          <% end %>
+        </ul>
       </div>
-    <% else %>
-      <div class="govuk-grid-row">
-        <div class="govuk-grid-column-full app-view-document-collection-group-memberships-index__heading">
-          <%= render "govuk_publishing_components/components/heading", {
-            text: "Documents",
-            margin_bottom: 6,
-          } %>
-
-          <ul class="govuk-list">
-            <li>
-              <%= link_to "Add document", admin_document_collection_group_search_options_path(@collection, @group), class: "govuk-link" %>
-            </li>
-          </ul>
-        </div>
-      </div>
-    <% end %>
+    </div>
 
     <% if @group.memberships.present? %>
       <div class="govuk-table--with-actions app-view-document-collection-group-memberships-index__table">
         <%= render "govuk_publishing_components/components/table", {
           rows: @group.memberships.map do |membership|
-            title = document_collection_group_member_title(membership)
-            url = document_collection_group_member_url(membership)
-            [
-              {
-                text: title,
-              },
-              {
-                text: link_to(sanitize("View #{tag.span(title, class: "govuk-visually-hidden")}"), url, class: "govuk-link") +
-                  link_to(sanitize("Remove #{tag.span(title, class: "govuk-visually-hidden")}"), confirm_destroy_admin_document_collection_group_document_collection_group_membership_path(@collection, @group, membership), class: "govuk-link gem-link--destructive govuk-!-margin-left-3"),
-              },
-            ]
+            if document_collection_group_member_unavailable?(membership)
+              title = tag.span("Unavailable Document", class: "govuk-!-font-weight-bold")
+              remove_link = link_to(sanitize("Remove #{tag.span(title, class: "govuk-visually-hidden")}"), confirm_destroy_admin_document_collection_group_document_collection_group_membership_path(@collection, @group, membership), class: "govuk-link gem-link--destructive govuk-!-margin-left-3")
+              links = remove_link
+            else
+              title = document_collection_group_member_title(membership)
+              url = document_collection_group_member_url(membership)
+              view_link = link_to(sanitize("View #{tag.span(title, class: "govuk-visually-hidden")}"), url, class: "govuk-link")
+              remove_link = link_to(sanitize("Remove #{tag.span(title, class: "govuk-visually-hidden")}"), confirm_destroy_admin_document_collection_group_document_collection_group_membership_path(@collection, @group, membership), class: "govuk-link gem-link--destructive govuk-!-margin-left-3")
+              links = view_link + remove_link
+            end
+
+            [{ text: title }, { text: links } ]
           end,
         } %>
       </div>
     <% else %>
       <%= render "govuk_publishing_components/components/warning_text", {
         text: "There are no documents inside this group",
       } %>
-
       <hr class="govuk-section-break govuk-section-break--m govuk-section-break--visible">
     <% end %>
   </div>

diff --git a/app/views/admin/document_collection_group_memberships/reorder.html.erb b/app/views/admin/document_collection_group_memberships/reorder.html.erb
@@ -11,9 +11,10 @@
       } %>
       <%= render "govuk_publishing_components/components/reorderable_list", {
         items: @group.memberships.map do |membership|
+          title = document_collection_group_member_unavailable?(membership) ? tag.span("Unavailable Document", class: "govuk-!-font-weight-bold") : document_collection_group_member_title(membership)
           {
             id: membership.id,
-            title: document_collection_group_member_title(membership),
+            title: title,
           }
         end,
       } %>

diff --git a/test/functional/admin/document_collection_group_memberships_controller_test.rb b/test/functional/admin/document_collection_group_memberships_controller_test.rb
@@ -130,4 +130,42 @@ def id_params
     assert_redirected_to admin_document_collection_group_document_collection_group_memberships_path(@collection, @group),
                          notice: "'#{title}' added to '#{@group.heading}'"
   end
+
+  view_test "GET #index should render 'unavailable documents' and notification" do
+    document = create(:document, latest_edition: nil)
+    whitehall_membership = create(:document_collection_group_membership, document:)
+    whitehall_confirm_destroy_path = confirm_destroy_admin_document_collection_group_document_collection_group_membership_path(@collection, @group, whitehall_membership)
+
+    @group.memberships << whitehall_membership
+
+    get :index, params: { document_collection_id: @collection, group_id: @group }
+
+    assert_select ".govuk-table__row", { count: 0, text: "View Unavailable Document" }
+    assert_select ".govuk-table__row", text: /Remove Unavailable Document/
+    assert_select ".govuk-table__row", a: whitehall_confirm_destroy_path
+    assert_select ".govuk-notification-banner__heading", text: "Remove 1 unavailable document(s) within the group."
+  end
+
+  view_test "GET #reorder should render 'unavailable documents'" do
+    document = create(:document, latest_edition: nil)
+    whitehall_membership = create(:document_collection_group_membership, document:)
+    @group.memberships << whitehall_membership
+    get :reorder, params: { document_collection_id: @collection, group_id: @group }
+
+    assert_response :success
+    assert_select "h1", /Reorder documents/
+    assert_select ".gem-c-reorderable-list__title", text: "Unavailable Document"
+  end
+
+  view_test "GET #confirm_destroy should render 'unavailable documents'" do
+    document = create(:document, latest_edition: nil)
+    whitehall_membership = create(:document_collection_group_membership, document:)
+    @group.memberships << whitehall_membership
+
+    get :confirm_destroy, params: { document_collection_id: @collection, group_id: @group, id: whitehall_membership }
+
+    assert_response :success
+    assert_select "h1", /Remove document/
+    assert_select "p", /Are you sure you want to remove "Unavailable Document" from this collection?/
+  end
 end
diff --git a/test/unit/app/helpers/admin/document_collection_group_memberships_helper_test.rb b/test/unit/app/helpers/admin/document_collection_group_memberships_helper_test.rb
@@ -5,7 +5,7 @@ class Admin::DocumentCollectionGroupMembershipsHelperTest < ActionView::TestCase
     @document_collection_group = build(:document_collection, :with_group).groups.first
   end
 
-  test "document_collection_group_member_title should return title if membership is a non_whitehall_link" do
+  test "#document_collection_group_member_title should return title if membership is a non_whitehall_link" do
     non_whitehall_link = DocumentCollectionNonWhitehallLink.new(
       base_path: "GOVUK PATH",
       title: "GOVUK TITLE",
@@ -14,14 +14,14 @@ class Admin::DocumentCollectionGroupMembershipsHelperTest < ActionView::TestCase
     assert_equal "GOVUK TITLE", document_collection_group_member_title(membership)
   end
 
-  test "document_collection_group_member_title should return title if membership is a document" do
+  test "#document_collection_group_member_title should return title if membership is a document" do
     edition = build(:edition, title: "DOC TITLE")
     document = build(:document, slug: "DOC PATH", latest_edition: edition)
     membership = @document_collection_group.memberships.build(document:)
     assert_equal "DOC TITLE", document_collection_group_member_title(membership)
   end
 
-  test "document_collection_group_member_url should return full url if membership is a non_whitehall_link" do
+  test "#document_collection_group_member_url should return full url if membership is a non_whitehall_link" do
     non_whitehall_link = DocumentCollectionNonWhitehallLink.new(
       base_path: "/GOVUK-PATH",
       title: "GOVUK TITLE",
@@ -30,10 +30,55 @@ class Admin::DocumentCollectionGroupMembershipsHelperTest < ActionView::TestCase
     assert_equal "https://www.test.gov.uk/GOVUK-PATH", document_collection_group_member_url(membership)
   end
 
-  test "document_collection_group_member_url should return public url if membership is a document" do
+  test "#document_collection_group_member_url should return public url if membership is a document" do
     edition = build(:edition, title: "DOC TITLE")
     document = build(:document, slug: "DOC-PATH", latest_edition: edition)
     membership = @document_collection_group.memberships.build(document:)
     assert_equal "https://www.test.gov.uk/government/generic-editions/DOC-PATH", document_collection_group_member_url(membership)
   end
+
+  test "#document_collection_group_member_unavailable? should return true if membership is a document without a latest_edition" do
+    document = build(:document, slug: "DOC PATH", latest_edition: nil)
+    membership = @document_collection_group.memberships.build(document:)
+    assert_equal true, document_collection_group_member_unavailable?(membership)
+  end
+
+  test "#document_collection_group_member_unavailable? should return false if membership is a document with a latest_edition" do
+    edition = build(:edition, title: "DOC TITLE")
+    document = build(:document, slug: "DOC PATH", latest_edition: edition)
+    membership = @document_collection_group.memberships.build(document:)
+    assert_equal false, document_collection_group_member_unavailable?(membership)
+  end
+
+  test "#document_collection_group_member_unavailable? should return false if membership is a non_whitehall_link" do
+    non_whitehall_link = DocumentCollectionNonWhitehallLink.new(
+      base_path: "GOVUK PATH",
+      title: "GOVUK TITLE",
+    )
+    membership = @document_collection_group.memberships.build(non_whitehall_link:)
+    assert_equal false, document_collection_group_member_unavailable?(membership)
+  end
+
+  test "#unavailable_document_count should return the number of documents without editions" do
+    document = build(:document, slug: "DOC PATH", latest_edition: nil)
+    @document_collection_group.memberships.build(document:)
+    @document_collection_group.memberships.build(document:)
+    assert_equal 2, unavailable_document_count(@document_collection_group.memberships)
+  end
+
+  test "#unavailable_document_count should return 0 if there are documents with editions" do
+    edition = build(:edition, title: "DOC TITLE")
+    document = build(:document, slug: "DOC PATH", latest_edition: edition)
+    @document_collection_group.memberships.build(document:)
+    assert_equal 0, unavailable_document_count(@document_collection_group.memberships)
+  end
+
+  test "#unavailable_document_count should not count non_whitehall_links" do
+    non_whitehall_link = DocumentCollectionNonWhitehallLink.new(
+      base_path: "GOVUK PATH",
+      title: "GOVUK TITLE",
+    )
+    @document_collection_group.memberships.build(non_whitehall_link:)
+    assert_equal 0, unavailable_document_count(@document_collection_group.memberships)
+  end
 end