From 15c49e0c10dae0238548489de92189953ebd2bef Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 11 Oct 2019 02:31:50 +0200 Subject: [PATCH 1/3] Remove obsolete URL helpers We now use `polymorphic_hierarchy_path` instead. --- app/controllers/comments_controller.rb | 4 +--- app/controllers/notifications_controller.rb | 2 -- app/helpers/custom_urls_helper.rb | 9 --------- 3 files changed, 1 insertion(+), 14 deletions(-) delete mode 100644 app/helpers/custom_urls_helper.rb diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index 6504c3f27..7bb5b126a 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -1,6 +1,4 @@ class CommentsController < ApplicationController - include CustomUrlsHelper - before_action :authenticate_user!, only: :create before_action :load_commentable, only: :create before_action :verify_resident_for_commentable!, only: :create @@ -104,7 +102,7 @@ class CommentsController < ApplicationController return if current_user.administrator? || current_user.moderator? if @commentable.respond_to?(:comments_closed?) && @commentable.comments_closed? - redirect_to @commentable, alert: t("comments.comments_closed") + redirect_to polymorphic_hierarchy_path(@commentable), alert: t("comments.comments_closed") end end diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb index e2293f2c3..ce257ae7d 100644 --- a/app/controllers/notifications_controller.rb +++ b/app/controllers/notifications_controller.rb @@ -1,6 +1,4 @@ class NotificationsController < ApplicationController - include CustomUrlsHelper - before_action :authenticate_user! skip_authorization_check diff --git a/app/helpers/custom_urls_helper.rb b/app/helpers/custom_urls_helper.rb deleted file mode 100644 index 1519e771e..000000000 --- a/app/helpers/custom_urls_helper.rb +++ /dev/null @@ -1,9 +0,0 @@ -module CustomUrlsHelper - def legislation_question_url(question) - legislation_process_question_url(question.process, question) - end - - def legislation_annotation_url(annotation) - legislation_process_question_url(annotation.draft_version.process, annotation.draft_version, annotation) - end -end From 11e52dbe98b93a3f9a5eab4174492e959d20c368 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 11 Oct 2019 02:28:00 +0200 Subject: [PATCH 2/3] Remove kaminari_path The main reason to use it was the `rel` attribute for previous/next pages not being indexed correctly by certain search engines when using a relative URL. However, AFAIK that only applied to `` tags, not to `` tags, and only if a `` tag was defined. In any case, it looks like the same search engines don't use the `rel` attribute for previous/next to index pages anymore. --- app/helpers/application_helper.rb | 4 ---- app/views/kaminari/_first_page.html.erb | 2 +- app/views/kaminari/_last_page.html.erb | 2 +- app/views/kaminari/_next_page.html.erb | 2 +- app/views/kaminari/_page.html.erb | 2 +- app/views/kaminari/_prev_page.html.erb | 2 +- spec/features/admin/site_customization/documents_spec.rb | 2 +- spec/features/proposals_spec.rb | 2 +- 8 files changed, 7 insertions(+), 11 deletions(-) diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb index 1033d0420..9785c46cf 100644 --- a/app/helpers/application_helper.rb +++ b/app/helpers/application_helper.rb @@ -55,10 +55,6 @@ module ApplicationHelper SiteCustomization::ContentBlock.block_for(name, locale) end - def kaminari_path(url) - "#{root_url.chomp("\/")}#{url}" - end - def self.asset_data_base64(path) asset = (Rails.application.assets || ::Sprockets::Railtie.build_environment(Rails.application)) .find_asset(path) diff --git a/app/views/kaminari/_first_page.html.erb b/app/views/kaminari/_first_page.html.erb index a5335a30b..c97a20740 100644 --- a/app/views/kaminari/_first_page.html.erb +++ b/app/views/kaminari/_first_page.html.erb @@ -1,3 +1,3 @@
  • - <%= link_to t("views.pagination.first"), kaminari_path(url), :remote => remote %> + <%= link_to t("views.pagination.first"), url, :remote => remote %>
  • diff --git a/app/views/kaminari/_last_page.html.erb b/app/views/kaminari/_last_page.html.erb index 697b3bd15..cc2929aa0 100644 --- a/app/views/kaminari/_last_page.html.erb +++ b/app/views/kaminari/_last_page.html.erb @@ -1,3 +1,3 @@
  • - <%= link_to t("views.pagination.last"), kaminari_path(url), :remote => remote %> + <%= link_to t("views.pagination.last"), url, :remote => remote %>
  • diff --git a/app/views/kaminari/_next_page.html.erb b/app/views/kaminari/_next_page.html.erb index 366367031..6da585a67 100644 --- a/app/views/kaminari/_next_page.html.erb +++ b/app/views/kaminari/_next_page.html.erb @@ -1,3 +1,3 @@
  • - <%= link_to t("views.pagination.next"), kaminari_path(url), :rel => "next", :remote => remote %> + <%= link_to t("views.pagination.next"), url, :rel => "next", :remote => remote %>
  • diff --git a/app/views/kaminari/_page.html.erb b/app/views/kaminari/_page.html.erb index 5d9e7a0d8..c82b8968c 100644 --- a/app/views/kaminari/_page.html.erb +++ b/app/views/kaminari/_page.html.erb @@ -5,6 +5,6 @@ <% else %>
  • - <%= link_to page, kaminari_path(url), { :remote => remote, :rel => page.next? ? "next" : page.prev? ? "prev" : nil } %> + <%= link_to page, url, { :remote => remote, :rel => page.next? ? "next" : page.prev? ? "prev" : nil } %>
  • <% end %> diff --git a/app/views/kaminari/_prev_page.html.erb b/app/views/kaminari/_prev_page.html.erb index d0147ff5c..2ee045967 100644 --- a/app/views/kaminari/_prev_page.html.erb +++ b/app/views/kaminari/_prev_page.html.erb @@ -1,3 +1,3 @@
  • - <%= link_to t("views.pagination.previous"), kaminari_path(url), :rel => "prev", :remote => remote %> + <%= link_to t("views.pagination.previous"), url, :rel => "prev", :remote => remote %>
  • diff --git a/spec/features/admin/site_customization/documents_spec.rb b/spec/features/admin/site_customization/documents_spec.rb index 92f322fac..3d13b30b8 100644 --- a/spec/features/admin/site_customization/documents_spec.rb +++ b/spec/features/admin/site_customization/documents_spec.rb @@ -47,7 +47,7 @@ describe "Documents" do within("ul.pagination") do expect(page).to have_content("1") - expect(page).to have_link("2", href: admin_site_customization_documents_url(page: 2)) + expect(page).to have_link("2", href: admin_site_customization_documents_path(page: 2)) expect(page).not_to have_content("3") click_link "Next", exact: false end diff --git a/spec/features/proposals_spec.rb b/spec/features/proposals_spec.rb index b4b6e0cfa..a048f7c3c 100644 --- a/spec/features/proposals_spec.rb +++ b/spec/features/proposals_spec.rb @@ -103,7 +103,7 @@ describe "Proposals" do within("ul.pagination") do expect(page).to have_content("1") - expect(page).to have_link("2", href: "http://www.example.com/proposals?page=2") + expect(page).to have_link("2", href: "/proposals?page=2") expect(page).not_to have_content("3") click_link "Next", exact: false end From 27468b0b7b9bc12a18bd89e94f843721859caa34 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Fri, 11 Oct 2019 03:06:24 +0200 Subject: [PATCH 3/3] Use relative URLs where possible In general, we always use relative URLs (using `_path`), but sometimes we were accidentally using absolute URLs (using `_url`). It's been reported i might cause some isuses if accepting both HTTP and HTTPS connections, although we've never seen the case. In any case, this change makes the code more consistent and makes the generated HTML cleaner. --- .../admin/poll/active_polls_controller.rb | 4 +-- .../admin/widget/cards_controller.rb | 2 +- .../concerns/access_denied_handler.rb | 2 +- .../management/users_controller.rb | 2 +- .../users/omniauth_callbacks_controller.rb | 2 +- .../users/registrations_controller.rb | 2 +- app/helpers/direct_uploads_helper.rb | 12 +++++---- app/helpers/documents_helper.rb | 26 ++++++++++--------- app/helpers/images_helper.rb | 20 +++++++------- .../admin/poll/active_polls/edit.html.erb | 2 +- .../ballot/_investment_for_sidebar.html.erb | 4 +-- .../budgets/investments/_ballot.html.erb | 6 ++--- app/views/budgets/investments/index.html.erb | 2 +- app/views/dashboard/_form.html.erb | 3 +-- app/views/legislation/proposals/edit.html.erb | 2 +- app/views/legislation/proposals/new.html.erb | 2 +- app/views/management/proposals/new.html.erb | 2 +- app/views/proposals/edit.html.erb | 2 +- app/views/proposals/new.html.erb | 2 +- app/views/shared/_map.html.erb | 2 +- app/views/shared/_suggest.html.erb | 2 +- spec/features/campaigns_spec.rb | 8 +++--- 22 files changed, 58 insertions(+), 53 deletions(-) diff --git a/app/controllers/admin/poll/active_polls_controller.rb b/app/controllers/admin/poll/active_polls_controller.rb index 17690a803..5dbc5cff7 100644 --- a/app/controllers/admin/poll/active_polls_controller.rb +++ b/app/controllers/admin/poll/active_polls_controller.rb @@ -5,7 +5,7 @@ class Admin::Poll::ActivePollsController < Admin::Poll::BaseController def create if @active_poll.update(active_poll_params) - redirect_to admin_polls_url, notice: t("flash.actions.update.active_poll") + redirect_to admin_polls_path, notice: t("flash.actions.update.active_poll") else render :edit end @@ -16,7 +16,7 @@ class Admin::Poll::ActivePollsController < Admin::Poll::BaseController def update if @active_poll.update(active_poll_params) - redirect_to admin_polls_url, notice: t("flash.actions.update.active_poll") + redirect_to admin_polls_path, notice: t("flash.actions.update.active_poll") else render :edit end diff --git a/app/controllers/admin/widget/cards_controller.rb b/app/controllers/admin/widget/cards_controller.rb index eb1e6ba79..514df854e 100644 --- a/app/controllers/admin/widget/cards_controller.rb +++ b/app/controllers/admin/widget/cards_controller.rb @@ -60,7 +60,7 @@ class Admin::Widget::CardsController < Admin::BaseController if @card.site_customization_page_id redirect_to admin_site_customization_page_cards_path(page), notice: notice else - redirect_to admin_homepage_url, notice: notice + redirect_to admin_homepage_path, notice: notice end end diff --git a/app/controllers/concerns/access_denied_handler.rb b/app/controllers/concerns/access_denied_handler.rb index d924baf56..080a77283 100644 --- a/app/controllers/concerns/access_denied_handler.rb +++ b/app/controllers/concerns/access_denied_handler.rb @@ -4,7 +4,7 @@ module AccessDeniedHandler included do rescue_from CanCan::AccessDenied do |exception| respond_to do |format| - format.html { redirect_to main_app.root_url, alert: exception.message } + format.html { redirect_to main_app.root_path, alert: exception.message } format.json { render json: { error: exception.message }, status: :forbidden } end end diff --git a/app/controllers/management/users_controller.rb b/app/controllers/management/users_controller.rb index 7a063e82e..93cb6f9bd 100644 --- a/app/controllers/management/users_controller.rb +++ b/app/controllers/management/users_controller.rb @@ -32,7 +32,7 @@ class Management::UsersController < Management::BaseController def logout destroy_session - redirect_to management_root_url, notice: t("management.sessions.signed_out_managed_user") + redirect_to management_root_path, notice: t("management.sessions.signed_out_managed_user") end private diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 67edc4e26..ccd3f8e1a 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -36,7 +36,7 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController set_flash_message(:notice, :success, kind: provider.to_s.capitalize) if is_navigational_format? else session["devise.#{provider}_data"] = auth - redirect_to new_user_registration_url + redirect_to new_user_registration_path end end diff --git a/app/controllers/users/registrations_controller.rb b/app/controllers/users/registrations_controller.rb index cb5e256af..56211a65f 100644 --- a/app/controllers/users/registrations_controller.rb +++ b/app/controllers/users/registrations_controller.rb @@ -26,7 +26,7 @@ class Users::RegistrationsController < Devise::RegistrationsController def delete current_user.erase(erase_params[:erase_reason]) sign_out - redirect_to root_url, notice: t("devise.registrations.destroyed") + redirect_to root_path, notice: t("devise.registrations.destroyed") end def success diff --git a/app/helpers/direct_uploads_helper.rb b/app/helpers/direct_uploads_helper.rb index d697b5127..54a6c958b 100644 --- a/app/helpers/direct_uploads_helper.rb +++ b/app/helpers/direct_uploads_helper.rb @@ -3,11 +3,13 @@ module DirectUploadsHelper def render_destroy_upload_link(direct_upload) label = direct_upload.resource_relation == "image" ? "images" : "documents" link_to t("#{label}.form.delete_button"), - direct_upload_destroy_url("direct_upload[resource_type]": direct_upload.resource_type, - "direct_upload[resource_id]": direct_upload.resource_id, - "direct_upload[resource_relation]": direct_upload.resource_relation, - "direct_upload[cached_attachment]": direct_upload.relation.cached_attachment, - format: :json), + direct_upload_destroy_path( + "direct_upload[resource_type]": direct_upload.resource_type, + "direct_upload[resource_id]": direct_upload.resource_id, + "direct_upload[resource_relation]": direct_upload.resource_relation, + "direct_upload[cached_attachment]": direct_upload.relation.cached_attachment, + format: :json + ), method: :delete, remote: true, class: "delete remove-cached-attachment" diff --git a/app/helpers/documents_helper.rb b/app/helpers/documents_helper.rb index 1386796bb..eeb517f46 100644 --- a/app/helpers/documents_helper.rb +++ b/app/helpers/documents_helper.rb @@ -19,13 +19,15 @@ module DocumentsHelper def render_destroy_document_link(builder, document) if !document.persisted? && document.cached_attachment.present? link_to t("documents.form.delete_button"), - direct_upload_destroy_url("direct_upload[resource_type]": document.documentable_type, - "direct_upload[resource_id]": document.documentable_id, - "direct_upload[resource_relation]": "documents", - "direct_upload[cached_attachment]": document.cached_attachment), - method: :delete, - remote: true, - class: "delete remove-cached-attachment" + direct_upload_destroy_path( + "direct_upload[resource_type]": document.documentable_type, + "direct_upload[resource_id]": document.documentable_id, + "direct_upload[resource_relation]": "documents", + "direct_upload[cached_attachment]": document.cached_attachment + ), + method: :delete, + remote: true, + class: "delete remove-cached-attachment" else link_to_remove_association document.new_record? ? t("documents.form.cancel_button") : t("documents.form.delete_button"), builder, class: "delete remove-document" end @@ -38,15 +40,15 @@ module DocumentsHelper accept: accepted_content_types_extensions(document.documentable_type.constantize), class: "js-document-attachment", data: { - url: document_direct_upload_url(document), + url: document_direct_upload_path(document), nested_document: true } end - def document_direct_upload_url(document) - direct_uploads_url("direct_upload[resource_type]": document.documentable_type, - "direct_upload[resource_id]": document.documentable_id, - "direct_upload[resource_relation]": "documents") + def document_direct_upload_path(document) + direct_uploads_path("direct_upload[resource_type]": document.documentable_type, + "direct_upload[resource_id]": document.documentable_id, + "direct_upload[resource_relation]": "documents") end def document_item_link(document) diff --git a/app/helpers/images_helper.rb b/app/helpers/images_helper.rb index 632634aee..4d5b620f2 100644 --- a/app/helpers/images_helper.rb +++ b/app/helpers/images_helper.rb @@ -28,10 +28,12 @@ module ImagesHelper def render_destroy_image_link(builder, image) if !image.persisted? && image.cached_attachment.present? link_to t("images.form.delete_button"), - direct_upload_destroy_url("direct_upload[resource_type]": image.imageable_type, - "direct_upload[resource_id]": image.imageable_id, - "direct_upload[resource_relation]": "image", - "direct_upload[cached_attachment]": image.cached_attachment), + direct_upload_destroy_path( + "direct_upload[resource_type]": image.imageable_type, + "direct_upload[resource_id]": image.imageable_id, + "direct_upload[resource_relation]": "image", + "direct_upload[cached_attachment]": image.cached_attachment + ), method: :delete, remote: true, class: "delete remove-cached-attachment" @@ -47,7 +49,7 @@ module ImagesHelper accept: imageable_accepted_content_types_extensions, class: "js-image-attachment", data: { - url: image_direct_upload_url(imageable), + url: image_direct_upload_path(imageable), nested_image: true } end @@ -59,10 +61,10 @@ module ImagesHelper show_caption: show_caption end - def image_direct_upload_url(imageable) - direct_uploads_url("direct_upload[resource_type]": imageable.class.name, - "direct_upload[resource_id]": imageable.id, - "direct_upload[resource_relation]": "image") + def image_direct_upload_path(imageable) + direct_uploads_path("direct_upload[resource_type]": imageable.class.name, + "direct_upload[resource_id]": imageable.id, + "direct_upload[resource_relation]": "image") end end diff --git a/app/views/admin/poll/active_polls/edit.html.erb b/app/views/admin/poll/active_polls/edit.html.erb index 6d8b39c67..a4cebcdcd 100644 --- a/app/views/admin/poll/active_polls/edit.html.erb +++ b/app/views/admin/poll/active_polls/edit.html.erb @@ -3,5 +3,5 @@

    <%= t("admin.active_polls.edit.title") %>

    - <%= render "form", form_url: admin_active_polls_url(@active_poll) %> + <%= render "form", form_url: admin_active_polls_path(@active_poll) %>
    diff --git a/app/views/budgets/ballot/_investment_for_sidebar.html.erb b/app/views/budgets/ballot/_investment_for_sidebar.html.erb index 82db87e87..a0c268a35 100644 --- a/app/views/budgets/ballot/_investment_for_sidebar.html.erb +++ b/app/views/budgets/ballot/_investment_for_sidebar.html.erb @@ -3,8 +3,8 @@ <%= investment.formatted_price %> <% if @budget.balloting? %> - <%= link_to budget_ballot_line_url(id: investment.id, - investments_ids: investment_ids), + <%= link_to budget_ballot_line_path(id: investment.id, + investments_ids: investment_ids), title: t("budgets.ballots.show.remove"), class: "remove-investment-project", method: :delete, diff --git a/app/views/budgets/investments/_ballot.html.erb b/app/views/budgets/investments/_ballot.html.erb index 73fe2fe8b..fb712b423 100644 --- a/app/views/budgets/investments/_ballot.html.erb +++ b/app/views/budgets/investments/_ballot.html.erb @@ -28,9 +28,9 @@

    <% if investment.should_show_ballots? %> <%= link_to t("budgets.investments.investment.add"), - budget_ballot_lines_url(investment_id: investment.id, - budget_id: investment.budget_id, - investments_ids: investment_ids), + budget_ballot_lines_path(investment_id: investment.id, + budget_id: investment.budget_id, + investments_ids: investment_ids), class: "button button-support small expanded", title: t("budgets.investments.investment.support_title"), method: :post, diff --git a/app/views/budgets/investments/index.html.erb b/app/views/budgets/investments/index.html.erb index d167cf53a..023fa1579 100644 --- a/app/views/budgets/investments/index.html.erb +++ b/app/views/budgets/investments/index.html.erb @@ -55,7 +55,7 @@ <% end %> - <%= render("shared/advanced_search", search_path: budget_investments_url(@budget)) %> + <%= render("shared/advanced_search", search_path: budget_investments_path(@budget)) %> <% if unfeasible_or_unselected_filter %>