From 21a1a533aa2133b8947e78adfa80398c419715cf Mon Sep 17 00:00:00 2001 From: Jessica Jones Date: Mon, 14 Oct 2024 16:37:43 +0100 Subject: [PATCH 1/3] Rename constants --- app/presenters/content_item/contents_list.rb | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/app/presenters/content_item/contents_list.rb b/app/presenters/content_item/contents_list.rb index c420fa877..72f046e27 100644 --- a/app/presenters/content_item/contents_list.rb +++ b/app/presenters/content_item/contents_list.rb @@ -1,9 +1,9 @@ module ContentItem module ContentsList - CHARACTER_LIMIT = 415 - CHARACTER_LIMIT_WITH_IMAGE = 224 - TABLE_ROW_LIMIT = 13 - TABLE_ROW_LIMIT_WITH_IMAGE = 6 + MINIMUM_CHARACTER_COUNT = 415 + MINIMUM_CHARACTER_COUNT_IF_IMAGE_PRESENT = 224 + MINIMUM_TABLE_ROW_COUNT = 13 + MINIMUM_TABLE_ROW_COUNT_IF_IMAGE_PRESENT = 6 def contents @contents ||= @@ -40,7 +40,7 @@ def extract_headings_with_ids end def first_item_has_long_content? - first_item_character_count > CHARACTER_LIMIT + first_item_character_count > MINIMUM_CHARACTER_COUNT end def first_item_content @@ -61,7 +61,7 @@ def first_item_character_count end def first_item_has_long_table? - first_item_table_rows > TABLE_ROW_LIMIT + first_item_table_rows > MINIMUM_TABLE_ROW_COUNT end def find_first_table @@ -92,11 +92,11 @@ def first_item_has_image? end def first_item_has_image_and_long_content? - first_item_has_image? && first_item_character_count > CHARACTER_LIMIT_WITH_IMAGE + first_item_has_image? && first_item_character_count > MINIMUM_CHARACTER_COUNT_IF_IMAGE_PRESENT end def first_item_has_image_and_long_table? - first_item_has_image? && first_item_table_rows > TABLE_ROW_LIMIT_WITH_IMAGE + first_item_has_image? && first_item_table_rows > MINIMUM_TABLE_ROW_COUNT_IF_IMAGE_PRESENT end def parsed_body From 6dbefab2ed477a6328f57227494b4561f811f239 Mon Sep 17 00:00:00 2001 From: Jessica Jones Date: Tue, 15 Oct 2024 11:42:19 +0100 Subject: [PATCH 2/3] Ensure show_contents_list? returns a boolean The original implementation could result in the method returning nil rather than true or false. Which is unexpected, and has been a barrier to changing when and where we display contents lists because of confusing test failures. --- app/presenters/content_item/contents_list.rb | 33 ++++++++------------ 1 file changed, 13 insertions(+), 20 deletions(-) diff --git a/app/presenters/content_item/contents_list.rb b/app/presenters/content_item/contents_list.rb index 72f046e27..2fb462803 100644 --- a/app/presenters/content_item/contents_list.rb +++ b/app/presenters/content_item/contents_list.rb @@ -23,14 +23,23 @@ def show_contents_list? return true if contents_items.count > 2 return false if no_first_item? - first_item_has_long_content? || - first_item_has_long_table? || - first_item_has_image_and_long_content? || - first_item_has_image_and_long_table? + first_item_size_requirements_met?(character_count, table_row_count) end private + def first_item_size_requirements_met?(char_count, table_row_count) + first_item_character_count > char_count || first_item_table_rows > table_row_count + end + + def character_count + first_item_has_image? ? MINIMUM_CHARACTER_COUNT_IF_IMAGE_PRESENT : MINIMUM_CHARACTER_COUNT + end + + def table_row_count + first_item_has_image? ? MINIMUM_TABLE_ROW_COUNT_IF_IMAGE_PRESENT : MINIMUM_TABLE_ROW_COUNT + end + def extract_headings_with_ids headings = parsed_body.css("h2").map do |heading| id = heading.attribute("id") @@ -39,10 +48,6 @@ def extract_headings_with_ids headings.compact end - def first_item_has_long_content? - first_item_character_count > MINIMUM_CHARACTER_COUNT - end - def first_item_content element = first_item first_item_text = "" @@ -60,10 +65,6 @@ def first_item_character_count @first_item_character_count ||= first_item_content.length end - def first_item_has_long_table? - first_item_table_rows > MINIMUM_TABLE_ROW_COUNT - end - def find_first_table element = first_item @@ -91,14 +92,6 @@ def first_item_has_image? end end - def first_item_has_image_and_long_content? - first_item_has_image? && first_item_character_count > MINIMUM_CHARACTER_COUNT_IF_IMAGE_PRESENT - end - - def first_item_has_image_and_long_table? - first_item_has_image? && first_item_table_rows > MINIMUM_TABLE_ROW_COUNT_IF_IMAGE_PRESENT - end - def parsed_body @parsed_body ||= Nokogiri::HTML(body) end From da0f7c1f4ce28db5dfd78fd660be91eef4ae60af Mon Sep 17 00:00:00 2001 From: Jessica Jones Date: Tue, 15 Oct 2024 11:43:33 +0100 Subject: [PATCH 3/3] Improve specificity of test This test is to check that the contents method returns the memoised @contents when repeatedly called, rather than running line 10 https://github.com/alphagov/government-frontend/blob/6dbefab2ed477a6328f57227494b4561f811f239/app/presenters/content_item/contents_list.rb#L10 The original test checked if the code on line 11 was run, ie the contents_items method. But the content_items method is called in two places in this module, within both the contents method (which this test is testing) but also within the show_contents_items? method.This means changes to the show_contents_items? code unexpectedly breaks this test. This commit reduces the scope of the test so that unexpected test failures no longer occur. --- test/presenters/content_item/contents_list_test.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/presenters/content_item/contents_list_test.rb b/test/presenters/content_item/contents_list_test.rb index a9e9fb3a7..6008b6dd1 100644 --- a/test/presenters/content_item/contents_list_test.rb +++ b/test/presenters/content_item/contents_list_test.rb @@ -17,7 +17,7 @@ def body end end - @contents_list.expects(:contents_items).returns([{ text: "A heading", id: "custom" }]).once + @contents_list.expects(:show_contents_list?).returns(true).once @contents_list.contents @contents_list.contents end