Bug 17457

Summary: Use SearchWithISBNVariations in acquisition advanced search (histsearch.pl)
Product: Koha Reporter: Marjorie Barry-Vila <marjorie.barry-vila>
Component: AcquisitionsAssignee: David Bourgault <david.bourgault>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: david.bourgault, jonathan.druart, katrin.fischer, m.de.rooy, magnus, nick, patrick.robitaille, philippe.blouin
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 13528    
Bug Blocks:    
Attachments: Bug 17457 - Adv. Acquisition search with ISBN variations
Bug 17457 - Add tests in Acquisition.t
Bug 17457 - Small fix to prevent variable name collision
Bug 17457 - Adv. Acquisition search with ISBN variations
Bug 17457 - Adv. Acquisition search with ISBN variations
Bug 17457 - Adv. Acquisition search with ISBN variations
Bug 17457 - Add tests for each marc flavour

Description Marjorie Barry-Vila 2016-10-17 14:01:05 UTC
Hi,

In acquisition advanced search, it will be great that isbn index use system preference SearchWithISBNVariations.

Regards,

Marjorie
Comment 1 David Bourgault 2017-09-25 20:24:03 UTC
Created attachment 67355 [details] [review]
Bug 17457 - Adv. Acquisition search with ISBN variations

Advanced acquisition search will now follow the "SearchWithISBNVariations" system preference.

Test plan :
0) Make sure you have orders pending or completed
1) Enable SearchWithISBNVariations if it is not enabled
2) Search for one of your orders by its ISBN, it should appear
3) Search for the same order by a variation of its ISBN, I used this website to find it : http://www.hahnlibrary.net/libraries/isbncalc.html
You should not get a result.
4) Apply patch
5) Repeat step 2-3. You should get a hit both times.
Comment 2 David Bourgault 2017-09-27 20:21:19 UTC
Created attachment 67416 [details] [review]
Bug 17457 - Add tests in Acquisition.t

Adds tests.
Comment 3 David Bourgault 2017-09-27 20:23:22 UTC
To test : 
0) Previous test plan
6) prove t/db_dependent/Acquisition.t
Comment 4 David Bourgault 2017-09-27 20:25:07 UTC
Created attachment 67417 [details] [review]
Bug 17457 - Small fix to prevent variable name collision
Comment 5 David Bourgault 2017-09-27 20:30:35 UTC
Created attachment 67418 [details] [review]
Bug 17457 - Adv. Acquisition search with ISBN variations

New patch : I did some git manipulation mistakes and previous patches wouldn't not apply, sorry :(

Advanced acquisition search will now follow the "SearchWithISBNVariations" system preference.

Test plan :
0) Make sure you have orders pending or completed
1) Enable SearchWithISBNVariations if it is not enabled
2) Search for one of your orders by its ISBN, it should appear
3) Search for the same order by a variation of its ISBN, I used this website to find it : http://www.hahnlibrary.net/libraries/isbncalc.html
You should not get a result.
4) Apply patch
5) Repeat step 2-3. You should get a hit both times.
6) prove t/db_dependent/Acquisition.t
Comment 6 Katrin Fischer 2017-10-08 11:22:48 UTC
Please don't forget to assign your bugs to yourself :)
Comment 7 Katrin Fischer 2017-10-08 11:38:32 UTC
- QA tools pass
- Tests pass

Nice little feature!
Comment 8 Katrin Fischer 2017-10-08 11:39:24 UTC
Created attachment 67820 [details] [review]
Bug 17457 - Adv. Acquisition search with ISBN variations

Advanced acquisition search will now follow the "SearchWithISBNVariations" system preference.

Test plan :
0) Make sure you have orders pending or completed
1) Enable SearchWithISBNVariations if it is not enabled
2) Search for one of your orders by its ISBN, it should appear
3) Search for the same order by a variation of its ISBN, I used this website to find it : http://www.hahnlibrary.net/libraries/isbncalc.html
You should not get a result.
4) Apply patch
5) Repeat step 2-3. You should get a hit both times.
6) prove t/db_dependent/Acquisition.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Marcel de Rooy 2018-01-05 06:58:31 UTC
Created attachment 70279 [details] [review]
Bug 17457 - Adv. Acquisition search with ISBN variations

Advanced acquisition search will now follow the "SearchWithISBNVariations" system preference.

Test plan :
0) Make sure you have orders pending or completed
1) Enable SearchWithISBNVariations if it is not enabled
2) Search for one of your orders by its ISBN, it should appear
3) Search for the same order by a variation of its ISBN, I used this website to find it : http://www.hahnlibrary.net/libraries/isbncalc.html
You should not get a result.
4) Apply patch
5) Repeat step 2-3. You should get a hit both times.
6) prove t/db_dependent/Acquisition.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Jonathan Druart 2018-01-15 14:49:53 UTC
Tests are MARC21 specific, please provide a version for UNIMARC.
I would suggest to create a MARC::Record manually instead of using a MARC XML.
See what is done in t/db_dependent/Biblio.t
Comment 11 David Bourgault 2018-01-24 20:59:29 UTC
Created attachment 70910 [details] [review]
Bug 17457 - Add tests for each marc flavour

+ Added a `run_flavoured_tests` sub that runs for each MARC flavour
* Moved SearchWithISBNVariation tests to the new sub

I copied a lot of the necessary utility functions over Biblio.t (as of Jonathan's suggestion). I also tried to name and organize the code in such a way that it can be reused if anyone else needs to run "flavoured" tests in the future.

prove t/db_dependant/Acquisition.t  OK
QA tool green
Comment 12 Marcel de Rooy 2018-03-16 07:29:54 UTC
(In reply to Jonathan Druart from comment #10)
> Tests are MARC21 specific, please provide a version for UNIMARC.
> I would suggest to create a MARC::Record manually instead of using a MARC
> XML.
> See what is done in t/db_dependent/Biblio.t

Imho adding a sample UNIMARC record with a ISBN would have been much simpler than following Biblio.t. This is unnecessary code complexity/duplication (120 lines extra for a UNIMARC ISBN).

David is to be commended for this efforts. But personally I would prefer to push without the second patch.

I will pass QA without additional signoff to let Jonathan handle it.
Comment 13 Jonathan Druart 2018-03-23 15:35:30 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 14 Nick Clemens 2018-03-26 11:59:50 UTC
Enhancement, not backported for 17.11