Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

214: Fix Youtube URL parsing #217

Merged
merged 2 commits into from Nov 12, 2020
Merged

214: Fix Youtube URL parsing #217

merged 2 commits into from Nov 12, 2020

Conversation

mikevallano
Copy link
Owner

Related Issues & PRs

Closes #214

Problems Solved

  • Now you can paste youtube urls with whatever additional url params are there
  • FULLSCREEN MODE now works! 馃帀

Screenshots

Fullscreen mode and licecap don't work together, but it works!
trailer_after

Things Learned

  • I modified the code from this SO post: https://stackoverflow.com/a/6557013
  • We should probably put some sort of validation in place to ensure nobody can enter invalid stuffs. Since we don't currently have it, I'll open a separate issue for it.

@mikevallano mikevallano self-assigned this Nov 11, 2020
trailer: "https://www.youtube.com/watch?v=#{youtube_id}&ab_channel=A24" }
expect(movie.reload.trailer).to eq(youtube_id)
end

Copy link
Collaborator

@lortza lortza Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to add a couple of more cases in here to cover some of the logic in these private methods:

Suggested change
it 'leaves non-youtube urls as-is' do
original_trailer = 'https://www.example.com'
patch :update,
{ id: movie.id,
tmdb_id: movie.tmdb_id,
trailer: original_trailer }
expect(movie.reload.trailer).to eq(original_trailer)
end
it 'handles empty trailer urls' do
trailer = ''
patch :update,
{ id: movie.id,
tmdb_id: movie.tmdb_id,
trailer: trailer }
expect(movie.reload.trailer).to eq(trailer)
end

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃 I did add those, but it's a bit weird because that's not how we want it to work.

But since it is how it works, I agree it'd be helpful to have the tests there.

I put them in a context and added a comment referencing the other issue that aims to fix it.

Comment on lines 83 to 92
def required_params
if params[:trailer].present?
params[:trailer] = params[:trailer].gsub('https://www.youtube.com/watch?v=', '')
end
params[:trailer] = parse_youtube_url(params[:trailer]) if params[:trailer].present?
params.permit(:trailer)
end

def parse_youtube_url(trailer)
return trailer unless trailer.include?('youtube.com')
uri = Addressable::URI.parse(trailer)
uri.query_values["v"] if uri.query_values
end
Copy link
Collaborator

@lortza lortza Nov 12, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I was reading this, I was a little confused about what was being returned. In one case, we're returning a whole url, in another, we're returning just a youtube_id, but it took me a beat to get there. When I rearrange the two methods like this, it's a little bit more clear to me. Of course, it's your call. I know ternaries aren't everyone's jam.

  def required_params
    trailer_url = params[:trailer]
    params[:trailer] = trailer_url.include?('youtube.com') ? youtube_id : trailer_url
    params.permit(:trailer)
  end

  def youtube_id
    uri = Addressable::URI.parse(params[:trailer])
    uri.query_values['v'] if uri.query_values
  end

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

馃憤 Ooh, me likey. Much better!

@mikevallano mikevallano merged commit d249ea5 into master Nov 12, 2020
@mikevallano mikevallano deleted the 214-parse-trailer-urls branch November 12, 2020 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix trailer urls
2 participants