Bug 18200

Summary: Fix a potential issue with preceding space in GetMarcUrls
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: MARC Bibliographic data supportAssignee: Marcel de Rooy <m.de.rooy>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: katrin.fischer, kyle, veron
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 18200: Fix a potential issue with preceding space in GetMarcUrls
Bug 18200: Fix a potential issue with preceding space in GetMarcUrls
Bug 18200: Fix a potential issue with preceding space in GetMarcUrls

Description Marcel de Rooy 2017-03-02 13:22:38 UTC
Saw a small issue with a preceding space in 856$u once. Obviously, it is quite simple to prevent it.
The current code puts http:// in front for that space.
Proposed patch trims the url and adds few trivial tests in Biblio.t.
Comment 1 Marcel de Rooy 2017-03-02 13:36:58 UTC
Created attachment 60793 [details] [review]
Bug 18200: Fix a potential issue with preceding space in GetMarcUrls

Trims the URL in order prevent prefixing a space with http://
Normally you won't have a preceding space here, but I saw it happening
one day and it does not cost much to resolve it.

Bonus: Adding few simple tests in t/db_dependent/Biblio.t.

Test plan:
[1] Run t/db_dependent/Biblio.t
[2] Add a 856$u with preceding space (MARC21)
[3] Check opac-detail, Online access with OPACXSLTDetailsDisplay empty.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Marc Véron 2017-03-03 12:27:09 UTC
Created attachment 60808 [details] [review]
Bug 18200: Fix a potential issue with preceding space in GetMarcUrls

Trims the URL in order prevent prefixing a space with http://
Normally you won't have a preceding space here, but I saw it happening
one day and it does not cost much to resolve it.

Bonus: Adding few simple tests in t/db_dependent/Biblio.t.

Test plan:
[1] Run t/db_dependent/Biblio.t
[2] Add a 856$u with preceding space (MARC21)
[3] Check opac-detail, Online access with OPACXSLTDetailsDisplay empty.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Followed test plan, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 3 Marc Véron 2017-03-03 12:33:16 UTC
Question: The biblionumber is always added to the link, something like:

http://XXXXXXXXXXX/cgi-bin/koha/tracklinks.pl?uri=http://www.yyyyyyyyyy.com&biblionumber=52

It happens with and without patch (with OPACXSLTDetailsDisplay empty). 

Is this behaviour expected?
Comment 4 Marcel de Rooy 2017-03-04 17:16:08 UTC
(In reply to Marc Véron from comment #3)
> Question: The biblionumber is always added to the link, something like:
> 
> http://XXXXXXXXXXX/cgi-bin/koha/tracklinks.pl?uri=http://www.yyyyyyyyyy.
> com&biblionumber=52
> 
> It happens with and without patch (with OPACXSLTDetailsDisplay empty). 
> 
> Is this behaviour expected?

This behavior is triggered by pref TrackClicks. It manipulates the URL and appends a biblionumber.
Thanks for testing.
Comment 5 Jonathan Druart 2017-03-13 13:47:05 UTC
Created attachment 61036 [details] [review]
Bug 18200: Fix a potential issue with preceding space in GetMarcUrls

Trims the URL in order prevent prefixing a space with http://
Normally you won't have a preceding space here, but I saw it happening
one day and it does not cost much to resolve it.

Bonus: Adding few simple tests in t/db_dependent/Biblio.t.

Test plan:
[1] Run t/db_dependent/Biblio.t
[2] Add a 856$u with preceding space (MARC21)
[3] Check opac-detail, Online access with OPACXSLTDetailsDisplay empty.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Followed test plan, works as expected
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 6 Kyle M Hall 2017-03-31 14:56:33 UTC
Pushed to master for 17.05, thanks Marcel!
Comment 7 Katrin Fischer 2017-04-02 17:46:43 UTC
This won't get ported back to 16.11.x as it is an enhancement.