The export tool (tools/export.pl) is badly designed and can be call from the command line. The part of the code used when called from the command line should be moved to a specific script. The common code must be moved to a package, with tests provided.
Created attachment 41889 [details] [review] Bug 14722: Add Koha::Biblioitem[s]
Created attachment 41890 [details] [review] Bug 14722: Refactor the export tool Why a refactoring was need for this script? The export tool (tools/export.pl) can be called from the command line and some parts of code were unnecessarity complicated (just look at the code, you will understand). Worse still, the script does not provide the same options for both interface. For instance you cannot export records given a range of biblionumbers, authids, callnumbers, etc. from the commandline. What does this patch? 1/ Important: The script tools/export.pl does not work anymore if called from the command line (should be in the release notes). 2/ The code used to generated a file (csv, iso2709 or xml) has been moved to a new module (Koha::Exporter::Record) and tests have been provided. 3/ No change is done on the web interface 4/ Some new options have been added to the commandline script (misc/export.pl): - starting_authid - ending_authid - authtype - starting_biblionumber - ending_biblionumber - itemtype - starting_callnumber - ending_callnumber - start_accession - end_accession 5/ There is a change in the behavior if an error occurs: Can't call method "as_usmarc" on an undefined value at Koha/Exporter/Record.pm line 114. record (number 5530) is invalid and therefore not exported because its reopening generates warnings above at Koha/Exporter/Record.pm line 117. Before this patch, they were not displayed (using the command line). What does not do this patch? It does not provide the 'clean', 'timestamp' and 'deleted_barcodes' options to the web interface (same as before). What about the perfs? With a DB with ~800 biblios (MARC21) Before: perl tools/export.pl 14.79s user 0.83s system 71% cpu 21.905 total After: perl misc/export.pl 17.19s user 0.84s system 75% cpu 24.018 total With a DB with ~6400 biblios (UNIMARC) Before: perl tools/export.pl 26.55s user 0.76s system 76% cpu 35.498 total After: perl misc/export.pl 26.78s user 0.84s system 80% cpu 34.494 total How to test this patch? Test plan: A. Web interface: 1/ On the current master, export some records, biblios and authorities (with the 3 differents exports) playing with the different filters (item type, libraries, callnumber, accession date, don't export items, remove non-local items, don't export fields, etc.). 2/ Apply this patch, export again the same records, and compare the generated files. They must be identical! 3/ Confirm that the export features on the checkout list (circ/circulation.pl) works as before this patch. B. The command line 1/ On the current master, export some records, biblios and authorities (with the 2 differents exports) playing with the different options (date, deleted_barcodes, clean). 2/ Apply this patch, export again the same records, and compare the generated files. They must be identical!
Created attachment 42280 [details] [review] Bug 14722: Add Koha::Biblioitem[s] Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Created attachment 42281 [details] [review] Bug 14722: Refactor the export tool Why a refactoring was need for this script? The export tool (tools/export.pl) can be called from the command line and some parts of code were unnecessarity complicated (just look at the code, you will understand). Worse still, the script does not provide the same options for both interface. For instance you cannot export records given a range of biblionumbers, authids, callnumbers, etc. from the commandline. What does this patch? 1/ Important: The script tools/export.pl does not work anymore if called from the command line (should be in the release notes). 2/ The code used to generated a file (csv, iso2709 or xml) has been moved to a new module (Koha::Exporter::Record) and tests have been provided. 3/ No change is done on the web interface 4/ Some new options have been added to the commandline script (misc/export.pl): - starting_authid - ending_authid - authtype - starting_biblionumber - ending_biblionumber - itemtype - starting_callnumber - ending_callnumber - start_accession - end_accession 5/ There is a change in the behavior if an error occurs: Can't call method "as_usmarc" on an undefined value at Koha/Exporter/Record.pm line 114. record (number 5530) is invalid and therefore not exported because its reopening generates warnings above at Koha/Exporter/Record.pm line 117. Before this patch, they were not displayed (using the command line). What does not do this patch? It does not provide the 'clean', 'timestamp' and 'deleted_barcodes' options to the web interface (same as before). What about the perfs? With a DB with ~800 biblios (MARC21) Before: perl tools/export.pl 14.79s user 0.83s system 71% cpu 21.905 total After: perl misc/export.pl 17.19s user 0.84s system 75% cpu 24.018 total With a DB with ~6400 biblios (UNIMARC) Before: perl tools/export.pl 26.55s user 0.76s system 76% cpu 35.498 total After: perl misc/export.pl 26.78s user 0.84s system 80% cpu 34.494 total How to test this patch? Test plan: A. Web interface: 1/ On the current master, export some records, biblios and authorities (with the 3 differents exports) playing with the different filters (item type, libraries, callnumber, accession date, don't export items, remove non-local items, don't export fields, etc.). 2/ Apply this patch, export again the same records, and compare the generated files. They must be identical! 3/ Confirm that the export features on the checkout list (circ/circulation.pl) works as before this patch. B. The command line 1/ On the current master, export some records, biblios and authorities (with the 2 differents exports) playing with the different options (date, deleted_barcodes, clean). 2/ Apply this patch, export again the same records, and compare the generated files. They must be identical! Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
A few small things: 1) Could we rename the script from just export.pl to export_records.pl? I think this would make it easier to spot and be in line with the export_borrowers.pl script already in the same directory. 2) Running the script without any argument exports all your bibliographic records - this could create quite a big file for a bigger catalog. Could it be better to display the help and have a -c switch for confirming or similar? 3) Some of the options have no documentation. For example: start_accession and end_accession - is this the date_created from the record? 4) Are csv and auths incompatible? It seems that ./misc/export.pl --filename=koha2.mrc --format=csv --record-type=auths Creates a file with bib data.
Overall - big improvement. Please take a look at my comments soon!
Created attachment 43578 [details] [review] Bug 14722: rename the script
Created attachment 43579 [details] [review] Bug 14722: Add POD for some options
Created attachment 43580 [details] [review] Bug 14722: CSV is only available for biblio records
(In reply to Katrin Fischer from comment #5) > A few small things: > > 1) Could we rename the script from just export.pl to export_records.pl? I > think this would make it easier to spot and be in line with the > export_borrowers.pl script already in the same directory. Yes, agreed. Feel free to squash patches 2 and 3. > 2) Running the script without any argument exports all your bibliographic > records - this could create quite a big file for a bigger catalog. Could it > be better to display the help and have a -c switch for confirming or similar? It's the current behavior. If we want to change it, it should be done in another bug report I think. > 3) Some of the options have no documentation. For example: start_accession > and end_accession - is this the date_created from the record? I documented all the options in a new patch. > 4) Are csv and auths incompatible? It seems that > ./misc/export.pl --filename=koha2.mrc --format=csv --record-type=auths > Creates a file with bib data. Exactly, it does not work. I have added a safeguard in the script.
Created attachment 43673 [details] [review] [PASSED QA] Bug 14722: Add Koha::Biblioitem[s] Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 43674 [details] [review] [PASSED QA] Bug 14722: Refactor the export tool Why a refactoring was need for this script? The export tool (tools/export.pl) can be called from the command line and some parts of code were unnecessarity complicated (just look at the code, you will understand). Worse still, the script does not provide the same options for both interface. For instance you cannot export records given a range of biblionumbers, authids, callnumbers, etc. from the commandline. What does this patch? 1/ Important: The script tools/export.pl does not work anymore if called from the command line (should be in the release notes). 2/ The code used to generated a file (csv, iso2709 or xml) has been moved to a new module (Koha::Exporter::Record) and tests have been provided. 3/ No change is done on the web interface 4/ Some new options have been added to the commandline script (misc/export.pl): - starting_authid - ending_authid - authtype - starting_biblionumber - ending_biblionumber - itemtype - starting_callnumber - ending_callnumber - start_accession - end_accession 5/ There is a change in the behavior if an error occurs: Can't call method "as_usmarc" on an undefined value at Koha/Exporter/Record.pm line 114. record (number 5530) is invalid and therefore not exported because its reopening generates warnings above at Koha/Exporter/Record.pm line 117. Before this patch, they were not displayed (using the command line). What does not do this patch? It does not provide the 'clean', 'timestamp' and 'deleted_barcodes' options to the web interface (same as before). What about the perfs? With a DB with ~800 biblios (MARC21) Before: perl tools/export.pl 14.79s user 0.83s system 71% cpu 21.905 total After: perl misc/export.pl 17.19s user 0.84s system 75% cpu 24.018 total With a DB with ~6400 biblios (UNIMARC) Before: perl tools/export.pl 26.55s user 0.76s system 76% cpu 35.498 total After: perl misc/export.pl 26.78s user 0.84s system 80% cpu 34.494 total How to test this patch? Test plan: A. Web interface: 1/ On the current master, export some records, biblios and authorities (with the 3 differents exports) playing with the different filters (item type, libraries, callnumber, accession date, don't export items, remove non-local items, don't export fields, etc.). 2/ Apply this patch, export again the same records, and compare the generated files. They must be identical! 3/ Confirm that the export features on the checkout list (circ/circulation.pl) works as before this patch. B. The command line 1/ On the current master, export some records, biblios and authorities (with the 2 differents exports) playing with the different options (date, deleted_barcodes, clean). 2/ Apply this patch, export again the same records, and compare the generated files. They must be identical! Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 43675 [details] [review] [PASSED QA] Bug 14722: rename the script Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 43676 [details] [review] [PASSED QA] Bug 14722: Add POD for some options Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 43677 [details] [review] [PASSED QA] Bug 14722: CSV is only available for biblio records Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Patches pushed to master. Nice job Jonathan!
Is the script "misc/exportauth.pl" then obsolete ?
Updating title and importance to have the release notes automatically filled.
*** Bug 14682 has been marked as a duplicate of this bug. ***