Bug 18200 - Fix a potential issue with preceding space in GetMarcUrls
Summary: Fix a potential issue with preceding space in GetMarcUrls
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic data support (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-02 13:22 UTC by Marcel de Rooy
Modified: 2017-04-02 17:46 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18200: Fix a potential issue with preceding space in GetMarcUrls (2.66 KB, patch)
2017-03-02 13:36 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 18200: Fix a potential issue with preceding space in GetMarcUrls (2.79 KB, patch)
2017-03-03 12:27 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18200: Fix a potential issue with preceding space in GetMarcUrls (2.86 KB, patch)
2017-03-13 13:47 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 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.