Skip to content

Commit

Permalink
Merge pull request #2288 from alphagov/make-urls-relative-popular-links
Browse files Browse the repository at this point in the history 
Allow urls for popular links to be relative url.
  • Loading branch information
@syed-ali-tw
syed-ali-tw committed Aug 16, 2024
2 parents aee8ede + 8632004 commit 02052d9
Show file tree
Hide file tree
Showing 6 changed files with 69 additions and 69 deletions.
6 changes: 3 additions & 3 deletions app/models/popular_links_edition.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,12 +11,12 @@ def all_valid_urls_and_titles_are_present
link_items.each_with_index do |item, index|
errors.add("url#{index + 1}", "URL is required for Link #{index + 1}") unless url_present?(item)
errors.add("title#{index + 1}", "Title is required for Link #{index + 1}") unless title_present?(item)
errors.add("url#{index + 1}", "URL is invalid for Link #{index + 1}, all URLs should have at least one '.' and no spaces.") if url_present?(item) && url_has_spaces_or_has_no_dot?(item[:url])
errors.add("url#{index + 1}", "URL is invalid for Link #{index + 1}, all URLs should start with '/'") if url_present?(item) && url_is_not_valid_relative_url?(item[:url])
end
end

def url_has_spaces_or_has_no_dot?(url)
url.include?(" ") || url.exclude?(".")
def url_is_not_valid_relative_url?(url)
!url.start_with?("/")
end

def title_present?(item)
Expand Down
88 changes: 44 additions & 44 deletions test/functional/homepage_controller_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -59,18 +59,18 @@ class HomepageControllerTest < ActionController::TestCase

should "update latest PopularLinksEdition with changed title and url" do
assert_equal "title1", @popular_links.link_items[0][:title]
assert_equal "https://www.url1.com", @popular_links.link_items[0][:url]
assert_equal "/url1", @popular_links.link_items[0][:url]

new_title = "title has changed"
new_url = "https://www.changedurl.com"
new_url = "/changedurl"
patch :update, params: { id: @popular_links.id,
"popular_links" =>
{ "1" => { "title" => new_title, "url" => new_url },
"2" => { "title" => "title2", "url" => "https://www.url2.com" },
"3" => { "title" => "title3", "url" => "https://www.url3.com" },
"4" => { "title" => "title4", "url" => "https://www.url4.com" },
"5" => { "title" => "title5", "url" => "https://www.url5.com" },
"6" => { "title" => "title6", "url" => "https://www.url6.com" } } }
"2" => { "title" => "title2", "url" => "/url2" },
"3" => { "title" => "title3", "url" => "/url3" },
"4" => { "title" => "title4", "url" => "/url4" },
"5" => { "title" => "title5", "url" => "/url5" },
"6" => { "title" => "title6", "url" => "/url6" } } }

assert_equal new_title, PopularLinksEdition.last.link_items[0][:title]
assert_equal new_url, PopularLinksEdition.last.link_items[0][:url]
Expand All @@ -81,31 +81,31 @@ class HomepageControllerTest < ActionController::TestCase

patch :update, params: { id: @popular_links.id,
"popular_links" =>
{ "1" => { "title" => "title", "url" => "url.com" },
"2" => { "title" => "title2", "url" => "https://www.url2.com" },
"3" => { "title" => "title3", "url" => "https://www.url3.com" },
"4" => { "title" => "title4", "url" => "https://www.url4.com" },
"5" => { "title" => "title5", "url" => "https://www.url5.com" },
"6" => { "title" => "title6", "url" => "https://www.url6.com" } } }
{ "1" => { "title" => "title", "url" => "/url" },
"2" => { "title" => "title2", "url" => "/url2" },
"3" => { "title" => "title3", "url" => "/url3" },
"4" => { "title" => "title4", "url" => "/url4" },
"5" => { "title" => "title5", "url" => "/url5" },
"6" => { "title" => "title6", "url" => "/url6" } } }
end

should "redirect to show path on success" do
new_title = "title has changed"

patch :update, params: { id: @popular_links.id,
"popular_links" =>
{ "1" => { "title" => new_title, "url" => "https://www.url1.com" },
"2" => { "title" => "title2", "url" => "https://www.url2.com" },
"3" => { "title" => "title3", "url" => "https://www.url3.com" },
"4" => { "title" => "title4", "url" => "https://www.url4.com" },
"5" => { "title" => "title5", "url" => "https://www.url5.com" },
"6" => { "title" => "title6", "url" => "https://www.url6.com" } } }
{ "1" => { "title" => new_title, "url" => "/url1" },
"2" => { "title" => "title2", "url" => "/url2" },
"3" => { "title" => "title3", "url" => "/url3" },
"4" => { "title" => "title4", "url" => "/url4" },
"5" => { "title" => "title5", "url" => "/url5" },
"6" => { "title" => "title6", "url" => "/url6" } } }

assert_redirected_to show_popular_links_path
end

should "render edit template on validation error" do
links_with_missing_items = { "1" => { "title" => "title has changed", "url" => "https://www.url1.com" } }
links_with_missing_items = { "1" => { "title" => "title has changed", "url" => "/url1" } }

patch :update, params: { id: @popular_links.id,
"popular_links" => links_with_missing_items }
Expand All @@ -123,12 +123,12 @@ class HomepageControllerTest < ActionController::TestCase

patch :update, params: { id: @popular_links.id,
"popular_links" =>
{ "1" => { "title" => new_title, "url" => "https://www.url1.com" },
"2" => { "title" => "title2", "url" => "https://www.url2.com" },
"3" => { "title" => "title3", "url" => "https://www.url3.com" },
"4" => { "title" => "title4", "url" => "https://www.url4.com" },
"5" => { "title" => "title5", "url" => "https://www.url5.com" },
"6" => { "title" => "title6", "url" => "https://www.url6.com" } } }
{ "1" => { "title" => new_title, "url" => "/url1" },
"2" => { "title" => "title2", "url" => "/url2" },
"3" => { "title" => "title3", "url" => "/url3" },
"4" => { "title" => "title4", "url" => "/url4" },
"5" => { "title" => "title5", "url" => "/url5" },
"6" => { "title" => "title6", "url" => "/url6" } } }

assert_equal "Due to an application error, the edition couldn't be saved.", flash[:danger]
end
Expand All @@ -138,12 +138,12 @@ class HomepageControllerTest < ActionController::TestCase

patch :update, params: { id: @popular_links.id,
"popular_links" =>
{ "1" => { "title" => new_title, "url" => "https://www.url1.com" },
"2" => { "title" => "title2", "url" => "https://www.url2.com" },
"3" => { "title" => "title3", "url" => "https://www.url3.com" },
"4" => { "title" => "title4", "url" => "https://www.url4.com" },
"5" => { "title" => "title5", "url" => "https://www.url5.com" },
"6" => { "title" => "title6", "url" => "https://www.url6.com" } } }
{ "1" => { "title" => new_title, "url" => "/url1" },
"2" => { "title" => "title2", "url" => "/url2" },
"3" => { "title" => "title3", "url" => "/url3" },
"4" => { "title" => "title4", "url" => "/url4" },
"5" => { "title" => "title5", "url" => "/url5" },
"6" => { "title" => "title6", "url" => "/url6" } } }

assert_template "homepage/popular_links/edit"
end
Expand All @@ -159,12 +159,12 @@ class HomepageControllerTest < ActionController::TestCase

patch :update, params: { id: @popular_links.id,
"popular_links" =>
{ "1" => { "title" => new_title, "url" => "https://www.url1.com" },
"2" => { "title" => "title2", "url" => "https://www.url2.com" },
"3" => { "title" => "title3", "url" => "https://www.url3.com" },
"4" => { "title" => "title4", "url" => "https://www.url4.com" },
"5" => { "title" => "title5", "url" => "https://www.url5.com" },
"6" => { "title" => "title6", "url" => "https://www.url6.com" } } }
{ "1" => { "title" => new_title, "url" => "/url1" },
"2" => { "title" => "title2", "url" => "/url2" },
"3" => { "title" => "title3", "url" => "/url3" },
"4" => { "title" => "title4", "url" => "/url4" },
"5" => { "title" => "title5", "url" => "/url5" },
"6" => { "title" => "title6", "url" => "/url6" } } }

assert_template "homepage/popular_links/edit"
end
Expand All @@ -174,12 +174,12 @@ class HomepageControllerTest < ActionController::TestCase

patch :update, params: { id: @popular_links.id,
"popular_links" =>
{ "1" => { "title" => new_title, "url" => "https://www.url1.com" },
"2" => { "title" => "title2", "url" => "https://www.url2.com" },
"3" => { "title" => "title3", "url" => "https://www.url3.com" },
"4" => { "title" => "title4", "url" => "https://www.url4.com" },
"5" => { "title" => "title5", "url" => "https://www.url5.com" },
"6" => { "title" => "title6", "url" => "https://www.url6.com" } } }
{ "1" => { "title" => new_title, "url" => "/url1" },
"2" => { "title" => "title2", "url" => "/url2" },
"3" => { "title" => "title3", "url" => "/url3" },
"4" => { "title" => "title4", "url" => "/url4" },
"5" => { "title" => "title5", "url" => "/url5" },
"6" => { "title" => "title6", "url" => "/url6" } } }

assert_equal "Popular links save was unsuccessful due to a service problem. Please wait for a few minutes and try again.", flash[:danger]
end
Expand Down
10 changes: 5 additions & 5 deletions test/integration/homepage_popular_links_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -130,11 +130,11 @@ class HomepagePopularLinksTest < JavascriptIntegrationTest
end

should "trim spaces from start and end of urls" do
fill_in "popular_links[1][url]", with: " www.abc.com "
fill_in "popular_links[1][url]", with: " /abc "
click_button("Save")

assert page.has_text?("www.abc.com")
assert_not page.has_text?(" www.abc.com ")
assert page.has_text?("/abc")
assert_not page.has_text?(" /abc ")
end

should "render create page when 'Cancel' is clicked" do
Expand All @@ -144,10 +144,10 @@ class HomepagePopularLinksTest < JavascriptIntegrationTest
end

should "not save any changes when 'Cancel' is clicked" do
fill_in "popular_links[1][url]", with: "www.abc.com"
fill_in "popular_links[1][url]", with: "/abc"
click_link("Cancel")

assert_not page.has_text?("www.abc.com")
assert_not page.has_text?("/abc")
end
end

Expand Down
20 changes: 10 additions & 10 deletions test/models/popular_links_edition_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,12 @@ class PopularLinksEditionTest < ActiveSupport::TestCase
end

should "validate all links have url and title" do
link_items = [{ url: "https://www.url1.com", title: "" },
link_items = [{ url: "/url1", title: "" },
{ title: "title2" },
{ url: "https://www.url3.com", title: "title3" },
{ url: "https://www.url4.com", title: "title4" },
{ url: "https://www.url5.com", title: "title5" },
{ url: "https://www.url6.com", title: "title6" }]
{ url: "/url3", title: "title3" },
{ url: "/url4", title: "title4" },
{ url: "/url5", title: "title5" },
{ url: "/url6", title: "title6" }]
popular_links = FactoryBot.build(:popular_links, link_items:)

assert_not popular_links.valid?
Expand All @@ -31,14 +31,14 @@ class PopularLinksEditionTest < ActiveSupport::TestCase
should "validate all urls are valid" do
link_items = [{ url: "", title: "" },
{ url: "invalid", title: "title2" },
{ url: "www.abc.co.uk", title: "title3" },
{ url: "www.cde.co.uk", title: "title4" },
{ url: "www.efg.co.uk", title: "title5" },
{ url: "www.ijk.com", title: "title6" }]
{ url: "/abc", title: "title3" },
{ url: "/cde", title: "title4" },
{ url: "/efg", title: "title5" },
{ url: "/hij", title: "title6" }]
popular_links = FactoryBot.build(:popular_links, link_items:)

assert_not popular_links.valid?
assert popular_links.errors.messages[:url2].include?("URL is invalid for Link 2, all URLs should have at least one '.' and no spaces.")
assert popular_links.errors.messages[:url2].include?("URL is invalid for Link 2, all URLs should start with '/'")
assert popular_links.errors.messages[:url1].include?("URL is required for Link 1")
end

Expand Down
2 changes: 1 addition & 1 deletion test/support/factories.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@

factory :popular_links, class: "PopularLinksEdition" do
title { "Homepage Popular Links" }
link_items { [{ url: "https://www.url1.com", title: "title1" }, { url: "https://www.url2.com", title: "title2" }, { url: "https://www.url3.com", title: "title3" }, { url: "https://www.url4.com", title: "title4" }, { url: "https://www.url5.com", title: "title5" }, { url: "https://www.url6.com", title: "title6" }] }
link_items { [{ url: "/url1", title: "title1" }, { url: "/url2", title: "title2" }, { url: "/url3", title: "title3" }, { url: "/url4", title: "title4" }, { url: "/url5", title: "title5" }, { url: "/url6", title: "title6" }] }
end

factory :programme_edition, parent: :edition, class: "ProgrammeEdition" do
Expand Down
12 changes: 6 additions & 6 deletions test/unit/presenters/formats/popular_links_presenter_test.rb
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,12 +20,12 @@ def subject
assert_equal "link_collection", @result[:document_type]
assert_equal "publisher", @result[:publishing_app]
assert_equal "frontend", @result[:rendering_app]
assert_equal ({ link_items: [{ url: "https://www.url1.com", title: "title1" },
{ url: "https://www.url2.com", title: "title2" },
{ url: "https://www.url3.com", title: "title3" },
{ url: "https://www.url4.com", title: "title4" },
{ url: "https://www.url5.com", title: "title5" },
{ url: "https://www.url6.com", title: "title6" }] }),
assert_equal ({ link_items: [{ url: "/url1", title: "title1" },
{ url: "/url2", title: "title2" },
{ url: "/url3", title: "title3" },
{ url: "/url4", title: "title4" },
{ url: "/url5", title: "title5" },
{ url: "/url6", title: "title6" }] }),
@result[:details]
end

Expand Down

0 comments on commit 02052d9

Please sign in to comment.