Bug 11912 - GetMarcISBN wrongly prepends a space to ISBNs
Summary: GetMarcISBN wrongly prepends a space to ISBNs
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic data support (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-03-09 21:28 UTC by Tomás Cohen Arazi
Modified: 2015-06-04 23:30 UTC (History)
3 users (show)

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


Attachments
Bug 11912: (regression tests) GetMarcISBN wrongly prepends a space to ISBNs (13.21 KB, patch)
2014-03-09 21:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11912: GetMarcISBN wrongly prepends a space to ISBNs (1.51 KB, patch)
2014-03-09 21:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11912: (refactoring followup) GetMarcISBN should implement its advertised API (4.23 KB, patch)
2014-03-10 15:11 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11912: (refactoring followup) GetMarcISBN should implement its advertised API (6.46 KB, patch)
2014-03-10 15:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 11912: (refactoring followup) GetMarcISBN should implement its advertised API (7.32 KB, patch)
2014-03-13 13:42 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11912: (regression tests) GetMarcISBN wrongly prepends a space to ISBNs (13.28 KB, patch)
2014-03-13 14:26 UTC, Nicolas Legrand
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11912: GetMarcISBN wrongly prepends a space to ISBNs (1.58 KB, patch)
2014-03-13 14:28 UTC, Nicolas Legrand
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11912: (refactoring followup) GetMarcISBN should implement its advertised API (7.39 KB, patch)
2014-03-13 14:29 UTC, Nicolas Legrand
Details | Diff | Splinter Review
Bug 11912: (regression tests) GetMarcISBN wrongly prepends a space to ISBNs (13.33 KB, patch)
2014-03-20 10:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11912: GetMarcISBN wrongly prepends a space to ISBNs (1.63 KB, patch)
2014-03-20 10:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11912: (refactoring followup) GetMarcISBN should implement its advertised API (7.46 KB, patch)
2014-03-20 10:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11912: Use the common way to mock syspref (2.40 KB, patch)
2014-03-20 10:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11912: Use the common way to mock syspref (2.40 KB, patch)
2014-03-20 10:19 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 Tomás Cohen Arazi 2014-03-09 21:28:26 UTC

    
Comment 1 Tomás Cohen Arazi 2014-03-09 21:43:10 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2014-03-09 21:43:16 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2014-03-10 15:11:00 UTC Comment hidden (obsolete)
Comment 4 Tomás Cohen Arazi 2014-03-10 15:16:22 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2014-03-13 13:42:05 UTC Comment hidden (obsolete)
Comment 6 Nicolas Legrand 2014-03-13 14:26:43 UTC Comment hidden (obsolete)
Comment 7 Nicolas Legrand 2014-03-13 14:28:52 UTC Comment hidden (obsolete)
Comment 8 Nicolas Legrand 2014-03-13 14:29:11 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2014-03-20 10:16:43 UTC
Created attachment 26471 [details] [review]
Bug 11912: (regression tests) GetMarcISBN wrongly prepends a space to ISBNs

This patch makes the tests run in both MARC21 and UNIMARC contexts. It previously
run only for MARC21. It mocks what needs to be mocked.

To test, run
 - prove t/db_dependent/Biblio.t
=> Notice the first ISBN has a space in front of it and those tests fails.

Thanks for any feedback
To+

Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 10 Jonathan Druart 2014-03-20 10:16:49 UTC
Created attachment 26472 [details] [review]
Bug 11912: GetMarcISBN wrongly prepends a space to ISBNs

This patch makes the logic inside GetMarcISBN simpler and
fixes the issue.

To test:
- Run the regression tests:
  prove -v t/db_dependent/Biblio.t
=> FAIL
- Apply the patch
- Run:
  prove -v t/db_dependent/Biblio.t
=> SUCCESS
- Verify that opac-detail.pl and catalogue/detail.pl look as usual regarding ISBN
- Sign off

Regards
To+

Sponsored-by: Universidad Nacional de Cordoba
Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 11 Jonathan Druart 2014-03-20 10:16:54 UTC
Created attachment 26473 [details] [review]
Bug 11912: (refactoring followup) GetMarcISBN should implement its advertised API

The current implementation of GetMarcISBN contradicts the documented API.
It currently returns an array of hashes with only one key (marcisbn) which
doesn't add any value to it.

I chose to fix GetMarcISBN to honour the API instead of changing the docs,
because it seems a really silly change.

To test:
- Run:
  prove t/db_dependent/Biblio.t
=> SUCCESS
- catalogue/detail.pl should correctly show ISBNs.
- opac/opac-detail.pl should correctly show ISBNs in both prog and bootstrap.
- opac-opac-sendshelf.pl should correctly show ISBNs in the email.
- Sign off :-D

Edit: forgot to git add some templates and fixing the unit tests.

Regards
To+

Sponsored-by: Universidad Nacional de Cordoba
Signed-off-by: Nicolas Legrand <nicolas.legrand@bulac.fr>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 12 Jonathan Druart 2014-03-20 10:17:00 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2014-03-20 10:19:45 UTC
Created attachment 26475 [details] [review]
Bug 11912: Use the common way to mock syspref

t::lib::Mocks::mock_preference provides a simple way to mock syspref.

Test plan:
Verify that t/db_dependent/Biblio.t returns green.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 14 Galen Charlton 2014-04-19 18:42:29 UTC
Pushed to master.  Thanks, Tomás!
Comment 15 Fridolin Somers 2014-08-13 07:42:24 UTC
Pushed to 3.14.x, will be in 3.14.10
Comment 16 Kyle M Hall 2014-08-13 18:24:23 UTC
Picked for 3.12.15!