Bug 17457 - Use SearchWithISBNVariations in acquisition advanced search (histsearch.pl)
Summary: Use SearchWithISBNVariations in acquisition advanced search (histsearch.pl)
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Bourgault
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 13528
Blocks:
  Show dependency treegraph
 
Reported: 2016-10-17 14:01 UTC by Marjorie Barry-Vila
Modified: 2018-12-03 20:03 UTC (History)
8 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 17457 - Adv. Acquisition search with ISBN variations (2.07 KB, patch)
2017-09-25 20:24 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 17457 - Add tests in Acquisition.t (4.66 KB, patch)
2017-09-27 20:21 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 17457 - Small fix to prevent variable name collision (802 bytes, patch)
2017-09-27 20:25 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 17457 - Adv. Acquisition search with ISBN variations (5.41 KB, patch)
2017-09-27 20:30 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 17457 - Adv. Acquisition search with ISBN variations (5.32 KB, patch)
2017-10-08 11:39 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 17457 - Adv. Acquisition search with ISBN variations (5.42 KB, patch)
2018-01-05 06:58 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17457 - Add tests for each marc flavour (8.23 KB, patch)
2018-01-24 20:59 UTC, David Bourgault
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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