From adfb141d1bb4e3f27da34e0866e37829c27bccda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 30 Sep 2022 02:35:09 +0200 Subject: [PATCH 1/3] Simplify URL validation in related content We were using `Setting["url"]` to verify the content belonged to the application URL, but we can use `root_url` instead. Note that means we need to include the port when filling in forms in the tests, since in tests URL helpers like `polymorphic_url` don't include the port, but a port is automatically added when actually making the request. --- app/controllers/related_contents_controller.rb | 4 ++-- app/views/relationable/_form.html.erb | 2 +- spec/shared/system/relationable.rb | 13 ++++++------- spec/support/common_actions.rb | 6 +++++- 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/app/controllers/related_contents_controller.rb b/app/controllers/related_contents_controller.rb index 2d8ecfa7e..3db7a5475 100644 --- a/app/controllers/related_contents_controller.rb +++ b/app/controllers/related_contents_controller.rb @@ -18,7 +18,7 @@ class RelatedContentsController < ApplicationController elsif related_content.duplicate? flash[:error] = t("related_content.error_duplicate") else - flash[:error] = t("related_content.error", url: Setting["url"]) + flash[:error] = t("related_content.error", url: root_url) end redirect_to polymorphic_path(related_content.parent_relationable) end @@ -41,7 +41,7 @@ class RelatedContentsController < ApplicationController end def valid_url? - params[:url].start_with?(Setting["url"]) + params[:url].start_with?(root_url) end def child_relationable_params diff --git a/app/views/relationable/_form.html.erb b/app/views/relationable/_form.html.erb index 2fc1b69ae..4f2def3aa 100644 --- a/app/views/relationable/_form.html.erb +++ b/app/views/relationable/_form.html.erb @@ -13,7 +13,7 @@
<%= text_field_tag :url, "", "aria-describedby": "related_content_help_text", - placeholder: t("related_content.placeholder", url: setting["url"]) %> + placeholder: t("related_content.placeholder", url: root_url) %> <%= hidden_field_tag :relationable_klass, relationable.class.name %> <%= hidden_field_tag :relationable_id, relationable.id %> diff --git a/spec/shared/system/relationable.rb b/spec/shared/system/relationable.rb index 50246fb29..28612d413 100644 --- a/spec/shared/system/relationable.rb +++ b/spec/shared/system/relationable.rb @@ -6,7 +6,6 @@ shared_examples "relationable" do |relationable_model_name| before do integration_session.host = Capybara.app_host # TODO: remove after upgrading to Rails 6.1 - Setting["url"] = Capybara.app_host end scenario "related contents are listed" do @@ -40,7 +39,7 @@ shared_examples "relationable" do |relationable_model_name| expect(page).to have_css ".add-related-content[aria-expanded='true']" within("#related_content") do - fill_in "Link to related content", with: polymorphic_url(related1) + fill_in "Link to related content", with: polymorphic_url(related1, port: app_port) click_button "Add" end @@ -57,7 +56,7 @@ shared_examples "relationable" do |relationable_model_name| click_button "Add related content" within("#related_content") do - fill_in "Link to related content", with: polymorphic_url(related2) + fill_in "Link to related content", with: polymorphic_url(related2, port: app_port) click_button "Add" end @@ -77,7 +76,7 @@ shared_examples "relationable" do |relationable_model_name| click_button "Add" end - expect(page).to have_content "Link not valid. Remember to start with #{Capybara.app_host}." + expect(page).to have_content "Link not valid. Remember to start with #{app_host}/." end scenario "returns error when relating content URL to itself" do @@ -87,7 +86,7 @@ shared_examples "relationable" do |relationable_model_name| click_button "Add related content" within("#related_content") do - fill_in "Link to related content", with: polymorphic_url(relationable) + fill_in "Link to related content", with: polymorphic_url(relationable, port: app_port) click_button "Add" end @@ -111,7 +110,7 @@ shared_examples "relationable" do |relationable_model_name| click_button "Add related content" within("#related_content") do - fill_in "Link to related content", with: "#{Capybara.app_host}/mypath/#{related.id}" + fill_in "Link to related content", with: "#{app_host}/mypath/#{related.id}" click_button "Add" end @@ -129,7 +128,7 @@ shared_examples "relationable" do |relationable_model_name| click_button "Add related content" within("#related_content") do - fill_in "url", with: polymorphic_url(related1) + fill_in "url", with: polymorphic_url(related1, port: app_port) click_button "Add" end diff --git a/spec/support/common_actions.rb b/spec/support/common_actions.rb index 84ae807a7..51da757f9 100644 --- a/spec/support/common_actions.rb +++ b/spec/support/common_actions.rb @@ -19,7 +19,11 @@ module CommonActions include Verifications def app_host - "#{Capybara.app_host}:#{Capybara::Server.ports.values.last}" + "#{Capybara.app_host}:#{app_port}" + end + + def app_port + Capybara::Server.ports.values.last end def fill_in_signup_form(email = "manuela@consul.dev", password = "judgementday") From 17ff1ac74cd443a30ab4e24f00e8db305c78063f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 30 Sep 2022 03:45:19 +0200 Subject: [PATCH 2/3] Simplify article publisher URL We've been using the `url` Setting for a long time, but since then we've added a few references to `root_url` to this file, so we're now adding consistency. We're also removing a now unnecessary condition. --- app/views/shared/_social_media_meta_tags.html.erb | 4 +--- spec/system/social_media_meta_tags_spec.rb | 4 +--- 2 files changed, 2 insertions(+), 6 deletions(-) diff --git a/app/views/shared/_social_media_meta_tags.html.erb b/app/views/shared/_social_media_meta_tags.html.erb index af48d2bda..01811be41 100644 --- a/app/views/shared/_social_media_meta_tags.html.erb +++ b/app/views/shared/_social_media_meta_tags.html.erb @@ -8,9 +8,7 @@ " /> " /> -<% if setting["url"] %> - " /> -<% end %> + <% if setting["facebook_handle"] %> " /> <% end %> diff --git a/spec/system/social_media_meta_tags_spec.rb b/spec/system/social_media_meta_tags_spec.rb index b7e751396..f5b52412c 100644 --- a/spec/system/social_media_meta_tags_spec.rb +++ b/spec/system/social_media_meta_tags_spec.rb @@ -8,7 +8,6 @@ describe "Social media meta tags" do "Citizen participation tool for an open, transparent and democratic government." end let(:twitter_handle) { "@consul_test" } - let(:url) { "http://consul.dev" } let(:facebook_handle) { "consultest" } let(:org_name) { "CONSUL TEST" } @@ -17,7 +16,6 @@ describe "Social media meta tags" do Setting["meta_title"] = meta_title Setting["meta_description"] = meta_description Setting["twitter_handle"] = twitter_handle - Setting["url"] = url Setting["facebook_handle"] = facebook_handle Setting["org_name"] = org_name end @@ -30,7 +28,7 @@ describe "Social media meta tags" do expect(page).to have_meta "twitter:description", with: meta_description expect(page).to have_meta "twitter:image", with: "#{app_host}/social_media_icon_twitter.png" expect(page).to have_property "og:title", with: meta_title - expect(page).to have_property "article:publisher", with: url + expect(page).to have_property "article:publisher", with: "#{app_host}/" expect(page).to have_property "article:author", with: "https://www.facebook.com/#{facebook_handle}" expect(page).to have_property "og:url", with: "#{app_host}/" expect(page).to have_property "og:image", with: "#{app_host}/social_media_icon.png" From f8716585108d3b3893e12af2e21d9b153dbf89f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 30 Sep 2022 04:01:20 +0200 Subject: [PATCH 3/3] Use default_url_options to generate absolute URLs In the dev seeds, we were using `Setting["url"]/proposals`, but we can use `proposals_url` instead, similar to what we do in the rest of the application. We can do a similar thing in the sitemap. This way the sitemap will also work on installations who haven't manually set the "url" setting. Since we aren't using `Setting["url"]` anywhere anymore, we're removing it. This setting was mainly redundant, since we already had the `server_name` in the secrets. Furthermore, `server_name` is automatically configured when running the installer, while `Setting["url"]` had to be manually set in the admin section the application was installed. Note we're using `ActionMailer::Base` setting to generate URLs. Sounds a bit strange, but it's a standard way Rails provides to generate URLs outside the context of a request. --- app/models/setting.rb | 1 - config/locales/en/settings.yml | 2 -- config/locales/es/settings.yml | 2 -- config/sitemap.rb | 2 +- db/dev_seeds/admin_notifications.rb | 5 ++++- db/dev_seeds/settings.rb | 1 - 6 files changed, 5 insertions(+), 8 deletions(-) diff --git a/app/models/setting.rb b/app/models/setting.rb index 9fa1e7d68..58cb8bb06 100644 --- a/app/models/setting.rb +++ b/app/models/setting.rb @@ -159,7 +159,6 @@ class Setting < ApplicationRecord "twitter_handle": nil, "twitter_hashtag": nil, "youtube_handle": nil, - "url": "http://example.com", # Public-facing URL of the app. # CONSUL installation's organization name "org_name": "CONSUL", "meta_title": nil, diff --git a/config/locales/en/settings.yml b/config/locales/en/settings.yml index fa9287be3..22e600c0b 100644 --- a/config/locales/en/settings.yml +++ b/config/locales/en/settings.yml @@ -40,8 +40,6 @@ en: telegram_handle_description: "If filled in it will appear in the footer" instagram_handle: "Instagram handle" instagram_handle_description: "If filled in it will appear in the footer" - url: "Main URL" - url_description: "Main URL of your website" org_name: "Site name" org_name_description: "This name will appear on mailers subject, help pages..." related_content_score_threshold: "Related content score threshold" diff --git a/config/locales/es/settings.yml b/config/locales/es/settings.yml index 146213632..3aea75100 100644 --- a/config/locales/es/settings.yml +++ b/config/locales/es/settings.yml @@ -40,8 +40,6 @@ es: telegram_handle_description: "Si está rellenado aparecerá en el pie de página" instagram_handle: "Usuario de Instagram" instagram_handle_description: "Si está rellenado aparecerá en el pie de página" - url: "URL general de la web" - url_description: "URL principal de tu web" org_name: "Nombre del sitio" org_name_description: "Este nombre aparecerá en el asunto de emails, páginas de ayuda..." related_content_score_threshold: "Umbral de puntuación de contenido relacionado" diff --git a/config/sitemap.rb b/config/sitemap.rb index 24b0e8ce3..3a2664e55 100644 --- a/config/sitemap.rb +++ b/config/sitemap.rb @@ -9,7 +9,7 @@ SitemapGenerator::Sitemap.namer = SitemapGenerator::SimpleNamer.new(:sitemap, ex # default host SitemapGenerator::Sitemap.verbose = false if Rails.env.test? -SitemapGenerator::Sitemap.default_host = Setting["url"] +SitemapGenerator::Sitemap.default_host = root_url(ActionMailer::Base.default_url_options) # sitemap generator SitemapGenerator::Sitemap.create do diff --git a/db/dev_seeds/admin_notifications.rb b/db/dev_seeds/admin_notifications.rb index 47e2a8702..e75139c7d 100644 --- a/db/dev_seeds/admin_notifications.rb +++ b/db/dev_seeds/admin_notifications.rb @@ -4,7 +4,10 @@ section "Creating Admin Notifications & Templates" do %i[title body].index_with do |attribute| -> { I18n.t("seeds.admin_notifications.proposal.#{attribute}") } end - ).merge(link: "#{Setting["url"]}/proposals", segment_recipient: "administrators") + ).merge( + link: Rails.application.routes.url_helpers.proposals_url(ActionMailer::Base.default_url_options), + segment_recipient: "administrators" + ) ).deliver AdminNotification.create!( diff --git a/db/dev_seeds/settings.rb b/db/dev_seeds/settings.rb index 364773ab6..280e56c63 100644 --- a/db/dev_seeds/settings.rb +++ b/db/dev_seeds/settings.rb @@ -18,7 +18,6 @@ section "Creating Settings" do "telegram_handle": "CONSUL", "twitter_handle": "@consul_dev", "twitter_hashtag": "#consul_dev", - "url": "http://localhost:3000", "votes_for_proposal_success": "100", "youtube_handle": "CONSUL" }.each do |name, value|