11912 – GetMarcISBN wrongly prepends a space to ISBNs

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:	Main
Hardware:	All All

Importance:	P5 - low minor
Assignee:	Tomás Cohen Arazi (tcohen)
QA Contact:	Testopia

URL:
Keywords:

Depends on:
Blocks:

Reported:	2014-03-09 21:28 UTC by Tomás Cohen Arazi (tcohen)
Modified:	2015-06-04 23:30 UTC (History)
CC List:	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:
Circulation function:

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 (tcohen)	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 (tcohen)	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 (tcohen)	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 (tcohen)	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 (tcohen)	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
Show Obsolete (9) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Tomás Cohen Arazi (tcohen) 2014-03-09 21:28:26 UTC

Comment 1 Tomás Cohen Arazi (tcohen) 2014-03-09 21:43:10 UTC Comment hidden (obsolete)

Created attachment 25968 [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+

Comment 2 Tomás Cohen Arazi (tcohen) 2014-03-09 21:43:16 UTC Comment hidden (obsolete)

Created attachment 25969 [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

Comment 3 Tomás Cohen Arazi (tcohen) 2014-03-10 15:11:00 UTC Comment hidden (obsolete)

Created attachment 26018 [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

Regards
To+

Sponsored-by: Universidad Nacional de Cordoba

Comment 4 Tomás Cohen Arazi (tcohen) 2014-03-10 15:16:22 UTC Comment hidden (obsolete)

Created attachment 26019 [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.

Regards
To+

Sponsored-by: Universidad Nacional de Cordoba

Comment 5 Tomás Cohen Arazi (tcohen) 2014-03-13 13:42:05 UTC Comment hidden (obsolete)

Created attachment 26277 [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

Comment 6 Nicolas Legrand 2014-03-13 14:26:43 UTC Comment hidden (obsolete)

Created attachment 26286 [details] [review]
[SIGNED-OFF] 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>

Comment 7 Nicolas Legrand 2014-03-13 14:28:52 UTC Comment hidden (obsolete)

Created attachment 26287 [details] [review]
[SIGNED-OFF] 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>

Comment 8 Nicolas Legrand 2014-03-13 14:29:11 UTC Comment hidden (obsolete)

Created attachment 26288 [details] [review]
[SIGNED-OFF] 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>

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)

Created attachment 26474 [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 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 (khall) 2014-08-13 18:24:23 UTC

Picked for 3.12.15!