Allow only YouTube/Vimeo URLs on 'video_url' attribute #1854
Conversation
app/models/proposal.rb
Outdated
| @@ -15,6 +15,7 @@ class Proposal < ActiveRecord::Base | |||
| include ActsAsParanoidAliases | |||
|
|
|||
| RETIRE_OPTIONS = %w(duplicated started unfeasible done other) | |||
| VALID_DOMAINS = %w(www.youtube.com youtube.com youtu.be www.vimeo.com vimeo.com) | |||
There was a problem hiding this comment.
VALID_VIDEO_DOMAINS would be more descriptive as a constant name, what do you think?
app/models/proposal.rb
Outdated
| @@ -118,6 +121,14 @@ def votable_by?(user) | |||
| user && user.level_two_or_three_verified? | |||
| end | |||
|
|
|||
| def valid_video_url? | |||
| return if video_url.blank? | |||
There was a problem hiding this comment.
I think the constant holding the valid video domains and the body of this function should be shared on a common helper for the near future (other models are for sure getting video urls this next week :) ). Like at https://github.com/consul/consul/blob/master/app/helpers/embed_videos_helper.rb. And this valid_video_url? could just be using it.
| @@ -155,7 +155,7 @@ | |||
| description 'Proposal description' | |||
| question 'Proposal question' | |||
| external_url 'http://external_documention.es' | |||
| video_url 'http://video_link.com' | |||
| video_url 'https://youtu.be/nhuNb0XtRhQ' | |||
There was a problem hiding this comment.
😸 I was expecting "Gandalf Sax Guy 10 Hours HD" or good old RickRolling ^_^ But cool 👏
| @@ -67,6 +67,13 @@ | |||
| end | |||
| end | |||
|
|
|||
| describe "#video_url" do | |||
| it "should not be valid when URL is not from Youtube or Vimeo" do | |||
There was a problem hiding this comment.
Really great to have test for it 👍 💯 . What do you think about testing valid scenarios? Maybe looping through the valid video domains array checking the helper valid_video_url? method validates them all?
There was a problem hiding this comment.
Just for clarification: You mean replacing this test with one where the valid_video_url? method tests each domain in the valid_video_domains array?
There was a problem hiding this comment.
Not replacing, but increasing. I mean a test should both check valid and invalid scenarios, right? And in this particular case, the valid scenarios are only a few so checking all of them may not be too slow I think (but I may be wrong and just checking one could be enough)
There was a problem hiding this comment.
I think one example should do. I created a .txt file under /fixtures/files with 5 valid URLs to test all the use cases, but according to Knapsack, this is too slow.
Creating an array on the test could work, but looks ugly since we have all these Youtube/Vimeo links.
What do you think?
aitbw
force-pushed
the
aperez-valid-video-url
branch
from
aa4119f
to
e44cd17
Compare
Fixes consuldemocracy#1841 On branch aperez-valid-video-url Changes to be committed: modified: app/models/proposal.rb modified: spec/factories.rb modified: spec/models/proposal_spec.rb Allow only YouTube/Vimeo URLs on 'video_url' attribute Fixes consuldemocracy#1841 On branch aperez-valid-video-url Changes to be committed: modified: app/helpers/embed_videos_helper.rb modified: app/models/proposal.rb modified: spec/features/management/proposals_spec.rb modified: spec/features/proposals_spec.rb modified: spec/features/tags/proposals_spec.rb modified: spec/models/proposal_spec.rb
zamuro
force-pushed
the
aperez-valid-video-url
branch
from
e44cd17
to
2181a65
Compare
Where
Fixes #1841
What
This PR solves issue #1841, by allowing only valid YouTube/Vimeo URLs if the
video_urlattribute is present.How
A custom validator called
valid_video_url?was defined on theProposalmodel to check whether the submittedvideo_urlis valid. The method is only executed when the aforementioned attribute is not blank. A constant titledVALID_DOMAINSwas declared to include all the possible hosts an URL could match.Screenshots
Here's how the form looks when an invalid URL is submitted
Test
Test coverage was slightly increased in order to solve this issue.