From 5eca54cc92bb3e39226b963b70e2bee0b29d4256 Mon Sep 17 00:00:00 2001 From: Minno Dang Date: Thu, 9 Nov 2023 17:47:08 +0000 Subject: [PATCH] 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 --- ...ent_collection_group_memberships_helper.rb | 24 +++--- .../confirm_destroy.html.erb | 5 +- .../index.html.erb | 81 +++++++++---------- .../reorder.html.erb | 3 +- ...ction_group_memberships_controller_test.rb | 38 +++++++++ ...ollection_group_memberships_helper_test.rb | 53 +++++++++++- 6 files changed, 147 insertions(+), 57 deletions(-) diff --git a/app/helpers/admin/document_collection_group_memberships_helper.rb b/app/helpers/admin/document_collection_group_memberships_helper.rb index 25c1b18838e..8298b141a3a 100644 --- 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 index 37c1ae91a0f..fe470fdf851 100644 --- 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 @@
- <%= form_with url: admin_document_collection_group_document_collection_group_membership_path(@collection, @group, @membership), method: :delete do |form| %> -

Are you sure you want to remove "<%= document_collection_group_member_title(@membership) %>" from this collection?

+ <%= 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) %> +

Are you sure you want to remove "<%= title %>" from this collection?

<%= 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 index 8d843f63a4a..48209dc7b02 100644 --- 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("

Remove #{unavailable_document_count(@group.memberships)} unavailable document(s) within the group.

"), + 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 %> +

@@ -19,56 +32,43 @@ items: secondary_navigation_tabs_items(@group, request.path), } %> - <% if @group.memberships.many? %> -

-
- <%= render "govuk_publishing_components/components/heading", { - text: "Documents", - margin_bottom: 6, - } %> -
    -
  • - <%= link_to "Add document", admin_document_collection_group_search_options_path(@collection, @group), class: "govuk-link" %> -
    • +
    +
    + <%= render "govuk_publishing_components/components/heading", { + text: "Documents", + margin_bottom: 6, + } %> +
      +
    • + <%= link_to "Add document", admin_document_collection_group_search_options_path(@collection, @group), class: "govuk-link" %> +
      • + <% if @group.memberships.many? %>
    • <%= link_to "Reorder document", reorder_admin_document_collection_group_document_collection_group_memberships_path(@collection, @group), class: "govuk-link" %>
      • -
    -
    + <% end %> +
- <% else %> -
-
- <%= render "govuk_publishing_components/components/heading", { - text: "Documents", - margin_bottom: 6, - } %> - -
    -
  • - <%= link_to "Add document", admin_document_collection_group_search_options_path(@collection, @group), class: "govuk-link" %> -
    • -
-
-
- <% end %> +
<% if @group.memberships.present? %>
<%= 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, } %>
@@ -76,7 +76,6 @@ <%= render "govuk_publishing_components/components/warning_text", { text: "There are no documents inside this group", } %> -
<% end %>
diff --git a/app/views/admin/document_collection_group_memberships/reorder.html.erb b/app/views/admin/document_collection_group_memberships/reorder.html.erb index 92894ad54b1..949ca796af4 100644 --- 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 index 5ca9aff0455..0db8480bfae 100644 --- 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 index a1974c671d9..cc292b68ec5 100644 --- 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