Description
Nick Clemens (kidclamp)
2016-08-31 17:20:00 UTC
Created attachment 55076 [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 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. (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. Created attachment 55091 [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 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. (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. (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 (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! Created attachment 55093 [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 Sorry - all working as expected!! Signed-off-by: Claire Gravely <claire_gravely@hotmail.com> 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? Created attachment 55298 [details] [review] Bug 17231 - (QA Followup) Nicer grep (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 (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 ^? (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 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> Created attachment 55325 [details] [review] Bug 17231 - (QA Followup) Nicer grep Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Pushed to master for 16.11, thanks Nick! Pushed in 16.05. Will be in 16.05.04. |