diff --git a/app/decorators/application_decorator.rb b/app/decorators/application_decorator.rb index a5a88bdee..100b26d3f 100644 --- a/app/decorators/application_decorator.rb +++ b/app/decorators/application_decorator.rb @@ -1,15 +1,17 @@ class ApplicationDecorator < Draper::Decorator delegate_all - def link_target - object.respond_to?(:website_url) && object.website_url.present? ? - object.website_url : - default_link_target + def object_link_target + if object.respond_to?(:link_target) + object.link_target + else + object_default_link_target + end end private - def default_link_target + def object_default_link_target h.polymorphic_path(object) end end \ No newline at end of file diff --git a/app/decorators/resource_decorator.rb b/app/decorators/resource_decorator.rb index 0f8a699e0..57a43921f 100644 --- a/app/decorators/resource_decorator.rb +++ b/app/decorators/resource_decorator.rb @@ -4,12 +4,6 @@ def detail(length: nil) length ? text&.truncate(length) : text # TODO - rename field end - def featured_url - return "" if url.nil? - url.empty? ? h.resource_path(resource) : url - end - - def main_image_url if main_image&.file&.attached? Rails.application.routes.url_helpers.url_for(main_image.file) diff --git a/app/models/community_news.rb b/app/models/community_news.rb index f6d771536..6f187885a 100644 --- a/app/models/community_news.rb +++ b/app/models/community_news.rb @@ -51,4 +51,8 @@ def self.search_by_params(params) community_news = community_news.published_search(params[:published_search]) if params[:published_search].present? community_news end + + def external_url + reference_url + end end diff --git a/app/models/concerns/linkable.rb b/app/models/concerns/linkable.rb index 7664a0d2a..5d99a2c82 100644 --- a/app/models/concerns/linkable.rb +++ b/app/models/concerns/linkable.rb @@ -1,13 +1,52 @@ module Linkable extend ActiveSupport::Concern + included do + # Override this in each model with something like: + # def internal_url + # Rails.application.routes.url_helpers.story_path(self) + # end + end + def link_target - website_url.presence || default_link_target + if external_link? + normalized_url(external_url) + else + default_link_target + end + end + + def external_link? + external_url.present? && valid_external_url?(external_url) + end + + # Models must implement this method. + def external_url + raise NotImplementedError, "Models including Linkable must define #external_url" end private + def valid_external_url?(value) + return false if value.blank? + + # Only normalize *naked domains*, not scheme-bearing strings + if value =~ /\A[\w.-]+\.[a-z]{2,}/i + value = "https://#{value}" unless value =~ /\Ahttps?:\/\//i + end + + uri = URI.parse(value) + uri.host.present? && %w[http https].include?(uri.scheme&.downcase) + rescue URI::InvalidURIError + false + end + def default_link_target Rails.application.routes.url_helpers.polymorphic_path(self) end + + def normalized_url(value) + return "" if value.blank? + value =~ /\Ahttp(s)?:\/\// ? value : "https://#{value}" + end end diff --git a/app/models/event.rb b/app/models/event.rb index b80cff83f..90156bde1 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -1,5 +1,5 @@ class Event < ApplicationRecord - include Linkable, TagFilterable, WindowsTypeFilterable + include TagFilterable, WindowsTypeFilterable belongs_to :created_by, class_name: "User", optional: true has_many :bookmarks, as: :bookmarkable, dependent: :destroy diff --git a/app/models/facilitator.rb b/app/models/facilitator.rb index c2b6e97aa..b18744343 100644 --- a/app/models/facilitator.rb +++ b/app/models/facilitator.rb @@ -1,5 +1,5 @@ class Facilitator < ApplicationRecord - include Linkable, TagFilterable, WindowsTypeFilterable + include TagFilterable, WindowsTypeFilterable belongs_to :created_by, class_name: "User" belongs_to :updated_by, class_name: "User" diff --git a/app/models/project.rb b/app/models/project.rb index 526632359..6da312e19 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1,5 +1,5 @@ class Project < ApplicationRecord - include Linkable, TagFilterable, WindowsTypeFilterable + include TagFilterable, WindowsTypeFilterable belongs_to :project_status belongs_to :project_obligation, optional: true diff --git a/app/models/quote.rb b/app/models/quote.rb index 0e1f22321..c76f0bed4 100644 --- a/app/models/quote.rb +++ b/app/models/quote.rb @@ -1,5 +1,5 @@ class Quote < ApplicationRecord - include Linkable, TagFilterable, WindowsTypeFilterable + include TagFilterable, WindowsTypeFilterable belongs_to :workshop, optional: true has_many :bookmarks, as: :bookmarkable, dependent: :destroy diff --git a/app/models/resource.rb b/app/models/resource.rb index 6ac22ed04..76081028a 100644 --- a/app/models/resource.rb +++ b/app/models/resource.rb @@ -109,6 +109,10 @@ def name title || id end + def external_url + url + end + def download_attachment main_image || gallery_images.first || attachments.first end diff --git a/app/models/story.rb b/app/models/story.rb index 7a2e91889..b50c18d4e 100644 --- a/app/models/story.rb +++ b/app/models/story.rb @@ -62,6 +62,10 @@ def name title end + def external_url + website_url + end + def organization_name project&.name end diff --git a/app/models/workshop.rb b/app/models/workshop.rb index a4d616c57..540334a31 100644 --- a/app/models/workshop.rb +++ b/app/models/workshop.rb @@ -1,5 +1,5 @@ class Workshop < ApplicationRecord - include Linkable, TagFilterable, WindowsTypeFilterable + include TagFilterable, WindowsTypeFilterable include Rails.application.routes.url_helpers belongs_to :windows_type diff --git a/app/views/community_news/index.html.erb b/app/views/community_news/index.html.erb index c7ad5e9c5..b0ff34b35 100644 --- a/app/views/community_news/index.html.erb +++ b/app/views/community_news/index.html.erb @@ -42,7 +42,7 @@ <%= link_to (image_tag news.main_image.file, class: "w-full rounded shadow-sm border border-gray-200", alt: news.title), - community_news_path(news) %> + news.link_target, target: "_blank", rel: "noopener" %> <% else %> @@ -52,7 +52,7 @@ <%= link_to title_with_badges(news, show_hidden_badge: current_user.super_user?).html_safe, - community_news_path(news), + news.link_target, target: "_blank", rel: "noopener", class: "font-bold hover:text-indigo-800 hover:underline" %> @@ -78,7 +78,7 @@ <% end %> <%= link_to "View", - community_news_path(news), + news.link_target, target: "_blank", rel: "noopener", class: "btn btn-secondary-outline" %> diff --git a/app/views/community_news/show.html.erb b/app/views/community_news/show.html.erb index 0996866ee..7478a5882 100644 --- a/app/views/community_news/show.html.erb +++ b/app/views/community_news/show.html.erb @@ -16,6 +16,11 @@ <%= link_to "Edit", edit_community_news_path(@community_news), class: "btn btn-secondary-outline admin-only bg-blue-100" %> <% end %> + + <% if @community_news.external_url.present? %> + <%= link_to "Website", safe_url(@community_news.external_url), target: "_blank", rel: "noopener", + class: "btn btn-secondary-outline" %> + <% end %> diff --git a/app/views/dashboard/_community_news_item.html.erb b/app/views/dashboard/_community_news_item.html.erb index 762d62ce7..2668f9fa4 100644 --- a/app/views/dashboard/_community_news_item.html.erb +++ b/app/views/dashboard/_community_news_item.html.erb @@ -6,7 +6,8 @@ - <%= link_to community_news_path(news), + + <%= link_to news.link_target, target: "_blank", rel: "noopener", class: "flex flex-row items-start gap-4 block z-10" do %> diff --git a/app/views/dashboard/_story.html.erb b/app/views/dashboard/_story.html.erb index af1204ca6..2349eab76 100644 --- a/app/views/dashboard/_story.html.erb +++ b/app/views/dashboard/_story.html.erb @@ -6,7 +6,7 @@ - <%= link_to story_path(story), + <%= link_to story.link_target, target: "_blank", rel: "noopener", class: "flex flex-col md:flex-row md:items-center gap-4 cursor-pointer block" do %> diff --git a/app/views/resources/_resource_card.html.erb b/app/views/resources/_resource_card.html.erb index 9dfb8ba7e..d0709179d 100644 --- a/app/views/resources/_resource_card.html.erb +++ b/app/views/resources/_resource_card.html.erb @@ -21,7 +21,7 @@ ">
- <%= link_to resource_path(resource.object), class: "block w-full h-full" do %> + <%= link_to resource.link_target, target: "_blank", rel: "noopener", class: "block w-full h-full" do %>
@@ -46,7 +46,7 @@
- <%= link_to resource_path(resource.object), + <%= link_to resource.link_target, target: "_blank", rel: "noopener", class: "inline-flex min-w-0 max-w-full text-lg font-semibold text-gray-900 leading-tight hover:underline" do %> <%= title_with_badges(resource, show_hidden_badge: current_user.super_user?).html_safe %> diff --git a/app/views/resources/show.html.erb b/app/views/resources/show.html.erb index df3fcaab4..460d2d4c0 100644 --- a/app/views/resources/show.html.erb +++ b/app/views/resources/show.html.erb @@ -12,6 +12,11 @@ class: "admin-only bg-blue-100 btn btn-primary-outline") %> <% end %> + <% if @resource.external_url.present? %> + <%= link_to "Website", safe_url(@resource.external_url), target: "_blank", rel: "noopener", + class: "btn btn-secondary-outline" %> + <% end %> + <%= link_to ("" + " Download ").html_safe, resource_download_path(@resource), diff --git a/app/views/stories/_story_card.html.erb b/app/views/stories/_story_card.html.erb index b1b2ffbfb..ebebf6027 100644 --- a/app/views/stories/_story_card.html.erb +++ b/app/views/stories/_story_card.html.erb @@ -1,6 +1,6 @@ <%# app/views/stories/_story_card.html.erb %> <%# locals: story:, href: story_path(story) by default, show_meta: true, show_excerpt: true %> -<% href = local_assigns.fetch(:href, story_path(story)) %> +<% href = local_assigns.fetch(:href, story.link_target, target: "_blank", rel: "noopener") %> <% show_meta = local_assigns.fetch(:show_meta, true) %> <% show_excerpt = local_assigns.fetch(:show_excerpt, true) %> diff --git a/app/views/stories/edit.html.erb b/app/views/stories/edit.html.erb index d5f60cd91..da0c038e7 100644 --- a/app/views/stories/edit.html.erb +++ b/app/views/stories/edit.html.erb @@ -4,7 +4,8 @@

Edit <%= @story.class.model_name.human %>

- <%= link_to "Website", safe_url(@story.website_url), class: "btn btn-secondary-outline" if @story.website_url %> + <%= link_to "Website", safe_url(@story.external_url), target: "_blank", rel: "noopener", + class: "btn btn-secondary-outline" if @story.external_url %> <%= link_to "View", story_path(@story), class: "btn btn-secondary-outline" %> diff --git a/app/views/stories/index.html.erb b/app/views/stories/index.html.erb index b2c07e17e..c28496988 100644 --- a/app/views/stories/index.html.erb +++ b/app/views/stories/index.html.erb @@ -47,7 +47,7 @@ <%= link_to (image_tag story.main_image.file, class: "w-full rounded shadow-sm border border-gray-200", alt: story.title), - story_path(story) %> + story.link_target, target: "_blank", rel: "noopener" %> <% else %> @@ -57,7 +57,7 @@ <%= link_to title_with_badges(story, show_hidden_badge: current_user.super_user?).html_safe, - story_path(story), + story.link_target, target: "_blank", rel: "noopener", class: "font-bold hover:text-indigo-800 hover:underline" %> @@ -75,7 +75,8 @@ <%= story.updated_at.strftime("%b %d, %Y") %> - <%= link_to "View", story_path(story), class: "btn btn-secondary-outline" %> + <%= link_to "View", story.link_target, target: "_blank", rel: "noopener", + class: "btn btn-secondary-outline" %> <% if current_user.super_user? %> <%= link_to "Edit", edit_story_path(story), class: "admin-only bg-blue-100 btn btn-secondary-outline" %> diff --git a/app/views/stories/show.html.erb b/app/views/stories/show.html.erb index 4c26f64f8..831e44a02 100644 --- a/app/views/stories/show.html.erb +++ b/app/views/stories/show.html.erb @@ -10,8 +10,8 @@ <%= render "bookmarks/editable_bookmark_button", resource: @story %> - <% if @story.website_url.present? %> - <%= link_to "Website", safe_url(@story.website_url), + <% if @story.external_url.present? %> + <%= link_to "Website", safe_url(@story.external_url), target: "_blank", rel: "noopener", class: "btn btn-secondary-outline" %> <% end %> diff --git a/app/views/taggings/_tagged_item_card.html.erb b/app/views/taggings/_tagged_item_card.html.erb index 0648e6014..b4756cfb6 100644 --- a/app/views/taggings/_tagged_item_card.html.erb +++ b/app/views/taggings/_tagged_item_card.html.erb @@ -24,7 +24,7 @@
- <%= link_to item.link_target, class: "block w-full h-full" do %> + <%= link_to item.object_link_target, class: "block w-full h-full" do %> <% if item.class == ResourceDecorator %>
@@ -57,7 +57,7 @@
- <%= link_to item.link_target, + <%= link_to item.object_link_target, class: "inline-flex min-w-0 max-w-full text-lg font-semibold text-gray-900 leading-tight hover:underline" do %> <%= item.title.truncate(50) %> diff --git a/spec/models/concerns/linkable_spec.rb b/spec/models/concerns/linkable_spec.rb index 6b94f41f3..2ab698aec 100644 --- a/spec/models/concerns/linkable_spec.rb +++ b/spec/models/concerns/linkable_spec.rb @@ -1,35 +1,101 @@ -# spec/models/concerns/linkable_spec.rb require "rails_helper" -RSpec.describe Linkable do - let(:model_class) do - Class.new(ApplicationRecord) do - self.table_name = "stories" - include Linkable +RSpec.describe Linkable, type: :module do + let(:model) { LinkableTestModel.new(id: 123, external_url_value: url) } + + describe "#valid_external_url?" do + subject { model.send(:valid_external_url?, url) } + + context "with a fully qualified http URL" do + let(:url) { "http://example.com" } + it { is_expected.to eq true } + end + + context "with a fully qualified https URL" do + let(:url) { "https://example.com" } + it { is_expected.to eq true } + end + + context "with a missing scheme" do + let(:url) { "example.com" } + it { is_expected.to eq true } # normalized to https://example.com + end + + context "with an invalid URL" do + let(:url) { "not a url" } + it { is_expected.to eq false } + end + + context "with a non-http scheme" do + let(:url) { "ftp://example.com" } + it { is_expected.to eq false } + end + + context "with nil" do + let(:url) { nil } + it { is_expected.to eq false } end end - let(:record) { model_class.new(id: 123, website_url: website_url) } + describe "#external_link?" do + subject { model.external_link? } + + context "when external_url is valid" do + let(:url) { "example.com" } + it { is_expected.to eq true } + end - before do - allow(Rails.application.routes.url_helpers) - .to receive(:polymorphic_path) - .and_return("/stories/123") + context "when external_url is blank" do + let(:url) { "" } + it { is_expected.to eq false } + end + + context "when external_url is invalid" do + let(:url) { "not a url" } + it { is_expected.to eq false } + end end - context "when website_url is present" do - let(:website_url) { "https://example.com" } + describe "#normalized_url" do + subject { model.send(:normalized_url, url) } + + context "when URL already has http" do + let(:url) { "http://example.com" } + it { is_expected.to eq "http://example.com" } + end + + context "when URL already has https" do + let(:url) { "https://example.com" } + it { is_expected.to eq "https://example.com" } + end - it "returns the website_url" do - expect(record.link_target).to eq("https://example.com") + context "when URL is missing scheme" do + let(:url) { "example.com" } + it { is_expected.to eq "https://example.com" } + end + + context "when blank" do + let(:url) { "" } + it { is_expected.to eq "" } end end - context "when website_url is blank" do - let(:website_url) { nil } + describe "#link_target" do + subject { model.link_target } + + context "when external_url is present and valid" do + let(:url) { "example.com" } + it { is_expected.to eq "https://example.com" } + end + + context "when external_url is invalid" do + let(:url) { "not a url" } + it { is_expected.to eq "/fake/123" } + end - it "falls back to the polymorphic path" do - expect(record.link_target).to eq("/stories/123") + context "when external_url is blank" do + let(:url) { "" } + it { is_expected.to eq "/fake/123" } end end end diff --git a/spec/support/linkable_test_model.rb b/spec/support/linkable_test_model.rb new file mode 100644 index 000000000..1ad83d09c --- /dev/null +++ b/spec/support/linkable_test_model.rb @@ -0,0 +1,17 @@ +# spec/support/linkable_test_model.rb +class LinkableTestModel + include ActiveModel::Model + include Rails.application.routes.url_helpers + include Linkable + + attr_accessor :id, :external_url_value + + def external_url + external_url_value + end + + # for internal link fallback + def default_link_target + "/fake/#{id}" + end +end