Bug 17231 - HTML5MediaYouTube should recognize youtu.be links from youtube as well at the full links
Summary: HTML5MediaYouTube should recognize youtu.be links from youtube as well at the...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-08-31 17:20 UTC by Nick Clemens
Modified: 2017-12-07 22:16 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17231 - HTML5MediaYouTube should recognize youtu.be links from youtube as well at the full links (1.34 KB, patch)
2016-08-31 17:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17231 - HTML5MediaYouTube should recognize youtu.be links from youtube as well at the full links (2.44 KB, patch)
2016-09-01 11:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17231 - HTML5MediaYouTube should recognize youtu.be links from youtube as well at the full links (2.49 KB, patch)
2016-09-01 14:56 UTC, Claire Gravely
Details | Diff | Splinter Review
Bug 17231 - (QA Followup) (921 bytes, patch)
2016-09-07 13:04 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 17231 - HTML5MediaYouTube should recognize youtu.be links from youtube as well at the full links (2.57 KB, patch)
2016-09-08 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17231 - (QA Followup) (995 bytes, patch)
2016-09-08 08:04 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2016-08-31 17:20:00 UTC
To recreate:
With HTML5MediaYouTube enabled and WWW::YouTube::Download installed add an 856 to a record like:
https://www.youtube.com/watch?v=tu0qtEwb9gE
and not the media player works
then use a link like:
https://youtu.be/tu0qtEwb9gE
And note there is no media player
Comment 1 Nick Clemens 2016-08-31 17:41:56 UTC Comment hidden (obsolete)
Comment 2 Claire Gravely 2016-09-01 09:03:15 UTC
After applying the patch:  I get the following error when doing a catalogue search on the intranet for a record which already has a link in 856 $u 

GET http://www.youtube.com/watch?v=http://bulbapedia.bulbagarden.net/wiki/Jigglypuff_(Pok%C3%A9mon) failed. status: 404 Not Found at /home/claire/koha/C4/HTML5Media.pm line 106.

Search for a record without an 856 $u works fine.
Searching for a record with a youtube.com or youtu.be link works fine.
Comment 3 Claire Gravely 2016-09-01 10:22:47 UTC
(In reply to Claire Gravely from comment #2)
> After applying the patch:  I get the following error when doing a catalogue
> search on the intranet for a record which already has a link in 856 $u 
> 
> GET
> http://www.youtube.com/watch?v=http://bulbapedia.bulbagarden.net/wiki/
> Jigglypuff_(Pok%C3%A9mon) failed. status: 404 Not Found at
> /home/claire/koha/C4/HTML5Media.pm line 106.
> 
> Search for a record without an 856 $u works fine.
> Searching for a record with a youtube.com or youtu.be link works fine.

Also applies to OPAC if HTML5MediaEnabled is set to OPAC and staff client.
Comment 4 Nick Clemens 2016-09-01 11:17:24 UTC Comment hidden (obsolete)
Comment 5 Nick Clemens 2016-09-01 11:19:38 UTC
Fixed! That was bad grepping on my part, worked cause it matched everything :-)

I added an eval to catch those errors as well, say if you link to something like:
http://www.nytimes.com/topic/company/youtube

Which will match, but is not a youtube video


(In reply to Claire Gravely from comment #3)
> (In reply to Claire Gravely from comment #2)
> > After applying the patch:  I get the following error when doing a catalogue
> > search on the intranet for a record which already has a link in 856 $u 
> Also applies to OPAC if HTML5MediaEnabled is set to OPAC and staff client.
Comment 6 Claire Gravely 2016-09-01 12:27:39 UTC
(In reply to Nick Clemens from comment #5)
> Fixed! That was bad grepping on my part, worked cause it matched everything
> :-)
> 
> I added an eval to catch those errors as well, say if you link to something
> like:
> http://www.nytimes.com/topic/company/youtube
> 
> Which will match, but is not a youtube video
> 
> 
> (In reply to Claire Gravely from comment #3)
> > (In reply to Claire Gravely from comment #2)
> > > After applying the patch:  I get the following error when doing a catalogue
> > > search on the intranet for a record which already has a link in 856 $u 
> > Also applies to OPAC if HTML5MediaEnabled is set to OPAC and staff client.

It now searches ok :), but if the record has two 856 $u (one for a link and one for a video for example) it does not embed the youtube video. 

I should note, that adding the two kinds of youtube style link does work fine on items without additional 856$u fields.
Comment 7 Nick Clemens 2016-09-01 14:24:32 UTC
(In reply to Claire Gravely from comment #6)
> It now searches ok :), but if the record has two 856 $u (one for a link and
> one for a video for example) it does not embed the youtube video. 

Hmm...It works if you put two separate 856 fields, each with their own $u

In master a link plus a youtube $u in a single 856 causes an error 

In master as well, two regular links in a single 856 only seems to display the first link anyways. I think this is not a bug but intended functionality
Comment 8 Claire Gravely 2016-09-01 14:28:09 UTC
(In reply to Nick Clemens from comment #7)
> (In reply to Claire Gravely from comment #6)
> > It now searches ok :), but if the record has two 856 $u (one for a link and
> > one for a video for example) it does not embed the youtube video. 
> 
> Hmm...It works if you put two separate 856 fields, each with their own $u
> 
> In master a link plus a youtube $u in a single 856 causes an error 
> 
> In master as well, two regular links in a single 856 only seems to display
> the first link anyways. I think this is not a bug but intended functionality

I meant two separate 856$u fields. Will retest!
Comment 9 Claire Gravely 2016-09-01 14:56:05 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2016-09-07 11:22:57 UTC
Comment on attachment 55093 [details] [review]
Bug 17231 - HTML5MediaYouTube should recognize youtu.be links from youtube as well at the full links

Review of attachment 55093 [details] [review]:
-----------------------------------------------------------------

::: C4/HTML5Media.pm
@@ +98,4 @@
>          # src
>          if ( $HTML5Media_field->subfield('u') ) {
>              $HTML5Media{srcblock} = $HTML5Media_field->subfield('u');
> +            if (grep /youtube|youtu[.]be/, $HTML5Media_field->subfield('u') ) {

/youtu\.?be/
seems easier to read, don't you think?

@@ +109,5 @@
> +                                'fmt' => '43' #webm is the only format compatible to all modern browsers. maybe check for available qualities
> +                            }
> +                        );
> +                    };
> +                    if ($@) { warn $@; }

What kind of errors do you handle here?
Comment 11 Nick Clemens 2016-09-07 13:04:22 UTC Comment hidden (obsolete)
Comment 12 Nick Clemens 2016-09-07 13:07:46 UTC
(In reply to Jonathan Druart from comment #10)
> Comment on attachment 55093 [details] [review] [review]
> /youtu\.?be/
> seems easier to read, don't you think?

Yeah, I can get behind that
 
> @@ +109,5 @@
> > +                                'fmt' => '43' #webm is the only format compatible to all modern browsers. maybe check for available qualities
> > +                            }
> > +                        );
> > +                    };
> > +                    if ($@) { warn $@; }
> 
> What kind of errors do you handle here?
if an URL contains 'youtube' but isn't a www.youtube.com link the downloader will fail - this way it fails nicer - something like this: http://www.nytimes.com/topic/company/youtube
Comment 13 Jonathan Druart 2016-09-07 13:42:55 UTC
(In reply to Nick Clemens from comment #12)
> > @@ +109,5 @@
> > > +                                'fmt' => '43' #webm is the only format compatible to all modern browsers. maybe check for available qualities
> > > +                            }
> > > +                        );
> > > +                    };
> > > +                    if ($@) { warn $@; }
> > 
> > What kind of errors do you handle here?
> if an URL contains 'youtube' but isn't a www.youtube.com link the downloader
> will fail - this way it fails nicer - something like this:
> http://www.nytimes.com/topic/company/youtube

So should not we test the regex from ^?
Comment 14 Nick Clemens 2016-09-07 15:32:00 UTC
(In reply to Jonathan Druart from comment #13)
 
> So should not we test the regex from ^?

Hmm...we could, but I would still argue for the eval - the downloader doesn't like links like: https://www.youtube.com/watch?v=kE2Akm55miw

Preferable to fail on creating the link than die altogether I think
Comment 15 Jonathan Druart 2016-09-08 08:04:17 UTC
Created attachment 55324 [details] [review]
Bug 17231 - HTML5MediaYouTube should recognize youtu.be links from youtube as well at the full links

To test:
Enable HTML5MediaYouTube and ensure WWW::YouTube::Download is installed
Add an 856$u to a record like: https://www.youtube.com/watch?v=tu0qtEwb9gE
Verify you can see the embedded player
Use the youtube shortened like (from the share button) like:
https://youtu.be/tu0qtEwb9gE
Verify you can see the embedded player

Signed-off-by: Claire Gravely <claire_gravely@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 16 Jonathan Druart 2016-09-08 08:04:57 UTC
Created attachment 55325 [details] [review]
Bug 17231 - (QA Followup)

Nicer grep

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 17 Kyle M Hall 2016-09-08 13:06:06 UTC
Pushed to master for 16.11, thanks Nick!
Comment 18 Frédéric Demians 2016-09-08 20:14:26 UTC
Pushed in 16.05. Will be in 16.05.04.