Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Allow urls for popular links to be relative url. #2288

Merged
merged 1 commit into from
Aug 16, 2024
Merged

Allow urls for popular links to be relative url. #2288

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions app/models/popular_links_edition.rb
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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
Loading