From 236b58ab0137f04381e92ba3bbb4ba357aab5338 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 May 2024 04:25:26 +0200 Subject: [PATCH 1/9] Remove duplication in code to validate video URL We were using the same code, and the same regular expressions, in two places. To do so, we were including a helper inside a model, which is something we don't usually do. --- app/helpers/embed_videos_helper.rb | 15 ++----------- app/models/concerns/videoable.rb | 28 ++++++++++++++++++++++++ app/models/poll/question/answer/video.rb | 13 +---------- app/models/proposal.rb | 4 +--- 4 files changed, 32 insertions(+), 28 deletions(-) create mode 100644 app/models/concerns/videoable.rb diff --git a/app/helpers/embed_videos_helper.rb b/app/helpers/embed_videos_helper.rb index 08a3179f8..4d050149b 100644 --- a/app/helpers/embed_videos_helper.rb +++ b/app/helpers/embed_videos_helper.rb @@ -1,7 +1,4 @@ module EmbedVideosHelper - VIMEO_REGEX = /vimeo.*(staffpicks\/|channels\/|videos\/|video\/|\/)([^#\&\?]*).*/ - YOUTUBE_REGEX = /youtu.*(be\/|v\/|u\/\w\/|embed\/|watch\?v=|\&v=)([^#\&\?]*).*/ - def embedded_video_code(resource) link = resource.video_url title = t("proposals.show.embed_video_title", proposal: resource.title) @@ -12,10 +9,10 @@ module EmbedVideosHelper end if server == "Vimeo" - reg_exp = VIMEO_REGEX + reg_exp = resource.class::VIMEO_REGEX src = "https://player.vimeo.com/video/" elsif server == "YouTube" - reg_exp = YOUTUBE_REGEX + reg_exp = resource.class::YOUTUBE_REGEX src = "https://www.youtube.com/embed/" end @@ -29,12 +26,4 @@ module EmbedVideosHelper "" end end - - def valid_video_url? - return if video_url.blank? - return if video_url.match(VIMEO_REGEX) - return if video_url.match(YOUTUBE_REGEX) - - errors.add(:video_url, :invalid) - end end diff --git a/app/models/concerns/videoable.rb b/app/models/concerns/videoable.rb new file mode 100644 index 000000000..1febafcce --- /dev/null +++ b/app/models/concerns/videoable.rb @@ -0,0 +1,28 @@ +module Videoable + extend ActiveSupport::Concern + + included do + validate :valid_video_url? + end + + VIMEO_REGEX = /vimeo.*(staffpicks\/|channels\/|videos\/|video\/|\/)([^#\&\?]*).*/ + YOUTUBE_REGEX = /youtu.*(be\/|v\/|u\/\w\/|embed\/|watch\?v=|\&v=)([^#\&\?]*).*/ + + def valid_video_url? + url = send(video_url_field) + + return if url.blank? + return if url.match(VIMEO_REGEX) + return if url.match(YOUTUBE_REGEX) + + errors.add(video_url_field, :invalid) + end + + def video_url_field + if has_attribute?(:video_url) + :video_url + else + :url + end + end +end diff --git a/app/models/poll/question/answer/video.rb b/app/models/poll/question/answer/video.rb index 6cf9e7cc6..e353dd365 100644 --- a/app/models/poll/question/answer/video.rb +++ b/app/models/poll/question/answer/video.rb @@ -1,17 +1,6 @@ class Poll::Question::Answer::Video < ApplicationRecord belongs_to :answer, class_name: "Poll::Question::Answer" - - VIMEO_REGEX = /vimeo.*(staffpicks\/|channels\/|videos\/|video\/|\/)([^#\&\?]*).*/ - YOUTUBE_REGEX = /youtu.*(be\/|v\/|u\/\w\/|embed\/|watch\?v=|\&v=)([^#\&\?]*).*/ + include Videoable validates :title, presence: true - validate :valid_url? - - def valid_url? - return if url.blank? - return if url.match(VIMEO_REGEX) - return if url.match(YOUTUBE_REGEX) - - errors.add(:url, :invalid) - end end diff --git a/app/models/proposal.rb b/app/models/proposal.rb index f1e4f7198..ca4518b07 100644 --- a/app/models/proposal.rb +++ b/app/models/proposal.rb @@ -14,7 +14,7 @@ class Proposal < ApplicationRecord include Mappable include Notifiable include Documentable - include EmbedVideosHelper + include Videoable include Relationable include Milestoneable include Randomizable @@ -59,8 +59,6 @@ class Proposal < ApplicationRecord validates :terms_of_service, acceptance: { allow_nil: false }, on: :create - validate :valid_video_url? - before_validation :set_responsible_name before_save :calculate_hot_score, :calculate_confidence_score From 579e332cf84b4796e3659f1b10b411f67beb2cd8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 May 2024 04:29:02 +0200 Subject: [PATCH 2/9] Extract component to render an embedded video --- .../shared/embedded_video_component.html.erb | 5 +++++ .../shared/embedded_video_component.rb} | 22 ++++++++++++++----- app/views/legislation/proposals/show.html.erb | 8 +------ app/views/proposals/_info.html.erb | 8 +------ 4 files changed, 23 insertions(+), 20 deletions(-) create mode 100644 app/components/shared/embedded_video_component.html.erb rename app/{helpers/embed_videos_helper.rb => components/shared/embedded_video_component.rb} (55%) diff --git a/app/components/shared/embedded_video_component.html.erb b/app/components/shared/embedded_video_component.html.erb new file mode 100644 index 000000000..fbfff8353 --- /dev/null +++ b/app/components/shared/embedded_video_component.html.erb @@ -0,0 +1,5 @@ +
+
+
+
+
diff --git a/app/helpers/embed_videos_helper.rb b/app/components/shared/embedded_video_component.rb similarity index 55% rename from app/helpers/embed_videos_helper.rb rename to app/components/shared/embedded_video_component.rb index 4d050149b..bfa362a72 100644 --- a/app/helpers/embed_videos_helper.rb +++ b/app/components/shared/embedded_video_component.rb @@ -1,7 +1,17 @@ -module EmbedVideosHelper - def embedded_video_code(resource) - link = resource.video_url - title = t("proposals.show.embed_video_title", proposal: resource.title) +class Shared::EmbeddedVideoComponent < ApplicationComponent + attr_reader :record + + def initialize(record) + @record = record + end + + def render? + record.video_url.present? + end + + def embedded_video_code + link = record.video_url + title = t("proposals.show.embed_video_title", proposal: record.title) if link =~ /vimeo.*/ server = "Vimeo" elsif link =~ /youtu*.*/ @@ -9,10 +19,10 @@ module EmbedVideosHelper end if server == "Vimeo" - reg_exp = resource.class::VIMEO_REGEX + reg_exp = record.class::VIMEO_REGEX src = "https://player.vimeo.com/video/" elsif server == "YouTube" - reg_exp = resource.class::YOUTUBE_REGEX + reg_exp = record.class::YOUTUBE_REGEX src = "https://www.youtube.com/embed/" end diff --git a/app/views/legislation/proposals/show.html.erb b/app/views/legislation/proposals/show.html.erb index b1485fd99..7ef241fac 100644 --- a/app/views/legislation/proposals/show.html.erb +++ b/app/views/legislation/proposals/show.html.erb @@ -65,13 +65,7 @@
<%= @proposal.summary %>
- <% if @proposal.video_url.present? %> -
-
-
-
-
- <% end %> + <%= render Shared::EmbeddedVideoComponent.new(@proposal) %> <%= auto_link_already_sanitized_html wysiwyg(@proposal.description) %> diff --git a/app/views/proposals/_info.html.erb b/app/views/proposals/_info.html.erb index 23def8f68..3e443b243 100644 --- a/app/views/proposals/_info.html.erb +++ b/app/views/proposals/_info.html.erb @@ -30,13 +30,7 @@
<%= @proposal.summary %>
-<% if @proposal.video_url.present? %> -
-
-
-
-
-<% end %> +<%= render Shared::EmbeddedVideoComponent.new(@proposal) %> <%= auto_link_already_sanitized_html wysiwyg(@proposal.description) %> From b07132d8d4e9c42f289fac4005284810d6e2d381 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 May 2024 04:40:31 +0200 Subject: [PATCH 3/9] Extract methods in embedded video component No that it's no longer a helper, we can extract method without fearing they will have the same name as other helper methods. --- .../shared/embedded_video_component.rb | 64 +++++++++++++------ 1 file changed, 43 insertions(+), 21 deletions(-) diff --git a/app/components/shared/embedded_video_component.rb b/app/components/shared/embedded_video_component.rb index bfa362a72..5e8636914 100644 --- a/app/components/shared/embedded_video_component.rb +++ b/app/components/shared/embedded_video_component.rb @@ -10,30 +10,52 @@ class Shared::EmbeddedVideoComponent < ApplicationComponent end def embedded_video_code - link = record.video_url - title = t("proposals.show.embed_video_title", proposal: record.title) - if link =~ /vimeo.*/ - server = "Vimeo" - elsif link =~ /youtu*.*/ - server = "YouTube" - end - - if server == "Vimeo" - reg_exp = record.class::VIMEO_REGEX - src = "https://player.vimeo.com/video/" - elsif server == "YouTube" - reg_exp = record.class::YOUTUBE_REGEX - src = "https://www.youtube.com/embed/" - end - - if reg_exp - match = link.match(reg_exp) - end - if match && match[2] - '' + "" else "" end end + + private + + def link + record.video_url + end + + def title + t("proposals.show.embed_video_title", proposal: record.title) + end + + def server + if link =~ /vimeo.*/ + "Vimeo" + elsif link =~ /youtu*.*/ + "YouTube" + end + end + + def reg_exp + if server == "Vimeo" + record.class::VIMEO_REGEX + elsif server == "YouTube" + record.class::YOUTUBE_REGEX + end + end + + def src + if server == "Vimeo" + "https://player.vimeo.com/video/" + elsif server == "YouTube" + "https://www.youtube.com/embed/" + end + end + + def match + @match ||= link.match(reg_exp) if reg_exp + end + + def iframe_attributes + tag.attributes(src: "#{src}#{match[2]}", style: "border:0;", allowfullscreen: true, title: title) + end end From fee5b8a13ca1a62342f7b5764f23675ea78869fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 May 2024 04:41:16 +0200 Subject: [PATCH 4/9] Remove redundant code in embedded video component When using `<%= `, `nil` is converted to an empty string, so there's no need to explicitely return an empty string. --- app/components/shared/embedded_video_component.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/components/shared/embedded_video_component.rb b/app/components/shared/embedded_video_component.rb index 5e8636914..52054d4a0 100644 --- a/app/components/shared/embedded_video_component.rb +++ b/app/components/shared/embedded_video_component.rb @@ -12,8 +12,6 @@ class Shared::EmbeddedVideoComponent < ApplicationComponent def embedded_video_code if match && match[2] "" - else - "" end end From cc8acdcc87dbbf44519ab1c1baf31161d0640efd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 May 2024 04:49:10 +0200 Subject: [PATCH 5/9] Make method name consistent in embedded video component We were using `reg_exp` as the method name, when it returned `VIMEO_REGEX` or `YOUTUBE_REGEX`. So using `regex` as the method name is less confusing. --- app/components/shared/embedded_video_component.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/components/shared/embedded_video_component.rb b/app/components/shared/embedded_video_component.rb index 52054d4a0..117d230f6 100644 --- a/app/components/shared/embedded_video_component.rb +++ b/app/components/shared/embedded_video_component.rb @@ -33,7 +33,7 @@ class Shared::EmbeddedVideoComponent < ApplicationComponent end end - def reg_exp + def regex if server == "Vimeo" record.class::VIMEO_REGEX elsif server == "YouTube" @@ -50,7 +50,7 @@ class Shared::EmbeddedVideoComponent < ApplicationComponent end def match - @match ||= link.match(reg_exp) if reg_exp + @match ||= link.match(regex) if regex end def iframe_attributes From 48178ffd43ddb6b61ae156c72630d36e854e04a6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 May 2024 22:33:53 +0200 Subject: [PATCH 6/9] Make embedded video tests more precise Now we're also testing that there's an iframe with the URL; before this change, the test would pass even if the JavaScript generating the iframe wouldn't work. --- spec/system/proposals_spec.rb | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/spec/system/proposals_spec.rb b/spec/system/proposals_spec.rb index 47db7e6ba..5726495ea 100644 --- a/spec/system/proposals_spec.rb +++ b/spec/system/proposals_spec.rb @@ -301,23 +301,30 @@ describe "Proposals" do context "Embedded video" do scenario "Show YouTube video" do proposal = create(:proposal, video_url: "http://www.youtube.com/watch?v=a7UFm6ErMPU") + visit proposal_path(proposal) - expect(page).to have_css "div[id='js-embedded-video']" - expect(page.html).to include "https://www.youtube.com/embed/a7UFm6ErMPU" + + within "#js-embedded-video" do + expect(page).to have_css "iframe[src='https://www.youtube.com/embed/a7UFm6ErMPU']" + end end scenario "Show Vimeo video" do proposal = create(:proposal, video_url: "https://vimeo.com/7232823") + visit proposal_path(proposal) - expect(page).to have_css "div[id='js-embedded-video']" - expect(page.html).to include "https://player.vimeo.com/video/7232823" + + within "#js-embedded-video" do + expect(page).to have_css "iframe[src='https://player.vimeo.com/video/7232823']" + end end scenario "Dont show video" do proposal = create(:proposal, video_url: nil) visit proposal_path(proposal) - expect(page).not_to have_css "div[id='js-embedded-video']" + + expect(page).not_to have_css "#js-embedded-video" end end From 738314e685e3a24d72937561b4d4a703131814b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 May 2024 22:42:20 +0200 Subject: [PATCH 7/9] Add basic tests for embedded video component --- .../shared/embedded_video_component_spec.rb | 43 +++++++++++++++++++ 1 file changed, 43 insertions(+) create mode 100644 spec/components/shared/embedded_video_component_spec.rb diff --git a/spec/components/shared/embedded_video_component_spec.rb b/spec/components/shared/embedded_video_component_spec.rb new file mode 100644 index 000000000..8a0a705a6 --- /dev/null +++ b/spec/components/shared/embedded_video_component_spec.rb @@ -0,0 +1,43 @@ +require "rails_helper" + +describe Shared::EmbeddedVideoComponent do + describe "src attribute" do + before do + dummy_class = Class.new do + include ActiveModel::Model + attr_accessor :title, :video_url + + include Videoable + end + + stub_const("DummyClass", dummy_class) + end + + let(:record) { DummyClass.new(title: "Dummy Video", video_url: "") } + let(:component) { Shared::EmbeddedVideoComponent.new(record) } + + it "does not render anything for empty URls" do + render_inline component + + expect(page).not_to be_rendered + end + + it "embeds a youtube video for youtube URLs" do + allow(record).to receive(:video_url).and_return "http://www.youtube.com/watch?v=a7UFm6ErMPU" + embed_url = "https://www.youtube.com/embed/a7UFm6ErMPU" + + render_inline component + + expect(page).to have_css "[data-video-code*='src=\"#{embed_url}\"']" + end + + it "embeds a vimeo video for vimeo URLs" do + allow(record).to receive(:video_url).and_return "https://vimeo.com/7232823" + embed_url = "https://player.vimeo.com/video/7232823" + + render_inline component + + expect(page).to have_css "[data-video-code*='src=\"#{embed_url}\"']" + end + end +end From 442156b1cc76aca9b095cf9cb996d9a7c84eef22 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 May 2024 23:36:57 +0200 Subject: [PATCH 8/9] Don't use cookies when embedding youtube videos When embedding a video in our site YouTube stores cookies in the user's computer that aren't necessary to watch the video, so we'd have to make people accept those cookies before letting them watch the video. Using a URL that doesn't use cookies, like mentioned in YouTube Help [1], is easier, though, and respects people's privacy without affecting the user experience. That I've found some references saying that youtube does store cookies once you hit the "play" button even when using the nocookie server [2]. Not sure whether that's an old behavior or I'm doing something wrong, but I don't see this is the case; even after playing the video, cookies aren't stored on my browser. [1] https://support.google.com/youtube/answer/171780#zippy=%2Cturn-on-privacy-enhanced-mode [2] https://www.cnet.com/news/privacy/youtubes-new-nocookie-feature-continues-to-serve-cookies/ --- app/components/shared/embedded_video_component.rb | 2 +- spec/components/shared/embedded_video_component_spec.rb | 2 +- spec/system/proposals_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/components/shared/embedded_video_component.rb b/app/components/shared/embedded_video_component.rb index 117d230f6..2fd8c1950 100644 --- a/app/components/shared/embedded_video_component.rb +++ b/app/components/shared/embedded_video_component.rb @@ -45,7 +45,7 @@ class Shared::EmbeddedVideoComponent < ApplicationComponent if server == "Vimeo" "https://player.vimeo.com/video/" elsif server == "YouTube" - "https://www.youtube.com/embed/" + "https://www.youtube-nocookie.com/embed/" end end diff --git a/spec/components/shared/embedded_video_component_spec.rb b/spec/components/shared/embedded_video_component_spec.rb index 8a0a705a6..b13361db7 100644 --- a/spec/components/shared/embedded_video_component_spec.rb +++ b/spec/components/shared/embedded_video_component_spec.rb @@ -24,7 +24,7 @@ describe Shared::EmbeddedVideoComponent do it "embeds a youtube video for youtube URLs" do allow(record).to receive(:video_url).and_return "http://www.youtube.com/watch?v=a7UFm6ErMPU" - embed_url = "https://www.youtube.com/embed/a7UFm6ErMPU" + embed_url = "https://www.youtube-nocookie.com/embed/a7UFm6ErMPU" render_inline component diff --git a/spec/system/proposals_spec.rb b/spec/system/proposals_spec.rb index 5726495ea..3fbcb5b92 100644 --- a/spec/system/proposals_spec.rb +++ b/spec/system/proposals_spec.rb @@ -305,7 +305,7 @@ describe "Proposals" do visit proposal_path(proposal) within "#js-embedded-video" do - expect(page).to have_css "iframe[src='https://www.youtube.com/embed/a7UFm6ErMPU']" + expect(page).to have_css "iframe[src='https://www.youtube-nocookie.com/embed/a7UFm6ErMPU']" end end From ee64efe6597d3cac3adb9f9f601f4f3112d47479 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Javi=20Mart=C3=ADn?= Date: Sun, 19 May 2024 05:15:28 +0200 Subject: [PATCH 9/9] Use the Do Not Track parameter in vimeo videos With this parameter, Vimeo no longer uses cookies that identifies users browsing our site. They do still store some cookies, though; quoting from Vimeo player parameters overview: > When DNT is enabled, Vimeo deploys one essential cookie via the > embeddable player: > The __cf_bm cookie, which is part of Cloudflare's Bot Management > service and helps mitigate risk associated with spam and bot traffic. Not sure whether this counts as essential cookies in our case; they're essential for Vimeo, but for us, they're third-party cookies, after all. [1] https://help.vimeo.com/hc/en-us/articles/12426260232977-Player-parameters-overview --- app/components/shared/embedded_video_component.rb | 6 +++--- spec/components/shared/embedded_video_component_spec.rb | 2 +- spec/system/proposals_spec.rb | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/components/shared/embedded_video_component.rb b/app/components/shared/embedded_video_component.rb index 2fd8c1950..9f4cdbd6a 100644 --- a/app/components/shared/embedded_video_component.rb +++ b/app/components/shared/embedded_video_component.rb @@ -43,9 +43,9 @@ class Shared::EmbeddedVideoComponent < ApplicationComponent def src if server == "Vimeo" - "https://player.vimeo.com/video/" + "https://player.vimeo.com/video/#{match[2]}?dnt=1" elsif server == "YouTube" - "https://www.youtube-nocookie.com/embed/" + "https://www.youtube-nocookie.com/embed/#{match[2]}" end end @@ -54,6 +54,6 @@ class Shared::EmbeddedVideoComponent < ApplicationComponent end def iframe_attributes - tag.attributes(src: "#{src}#{match[2]}", style: "border:0;", allowfullscreen: true, title: title) + tag.attributes(src: src, style: "border:0;", allowfullscreen: true, title: title) end end diff --git a/spec/components/shared/embedded_video_component_spec.rb b/spec/components/shared/embedded_video_component_spec.rb index b13361db7..e4d9e05f6 100644 --- a/spec/components/shared/embedded_video_component_spec.rb +++ b/spec/components/shared/embedded_video_component_spec.rb @@ -33,7 +33,7 @@ describe Shared::EmbeddedVideoComponent do it "embeds a vimeo video for vimeo URLs" do allow(record).to receive(:video_url).and_return "https://vimeo.com/7232823" - embed_url = "https://player.vimeo.com/video/7232823" + embed_url = "https://player.vimeo.com/video/7232823?dnt=1" render_inline component diff --git a/spec/system/proposals_spec.rb b/spec/system/proposals_spec.rb index 3fbcb5b92..459f7c619 100644 --- a/spec/system/proposals_spec.rb +++ b/spec/system/proposals_spec.rb @@ -315,7 +315,7 @@ describe "Proposals" do visit proposal_path(proposal) within "#js-embedded-video" do - expect(page).to have_css "iframe[src='https://player.vimeo.com/video/7232823']" + expect(page).to have_css "iframe[src='https://player.vimeo.com/video/7232823?dnt=1']" end end