Bug 20486 - Add --marc_conditions option to export_records.pl
Summary: Add --marc_conditions option to export_records.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Gustafsson
QA Contact: David Gustafsson
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-03-28 12:30 UTC by David Gustafsson
Modified: 2020-01-06 20:15 UTC (History)
15 users (show)

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


Attachments
Bug 20486: Add --marc_conditions option to export_records.pl (13.02 KB, patch)
2018-03-28 12:34 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20486: Add --marc_conditions option to export_records.pl (13.05 KB, patch)
2018-03-30 18:26 UTC, Maksim Sen
Details | Diff | Splinter Review
Bug 20486: Add --marc_conditions option to export_records.pl (13.31 KB, patch)
2018-04-11 12:48 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20486: Add --marc_conditions option to export_records.pl (13.30 KB, patch)
2018-04-25 19:04 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20486: Add --marc_conditions option to export_records.pl (13.30 KB, patch)
2018-05-07 12:10 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20486: Add --marc_conditions option to export_records.pl (13.33 KB, patch)
2018-09-23 17:04 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 20486: Add --marc_conditions option to export_records.pl (13.47 KB, patch)
2018-10-10 00:51 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 20486: Add --marc_conditions option to export_records.pl (13.43 KB, patch)
2018-10-31 17:05 UTC, David Gustafsson
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description David Gustafsson 2018-03-28 12:30:10 UTC
We had the need to filter out certain record based on marc-fields when exporting, so added a --marc_conditions options where filter conditions can be passed to the export script. For example: --marc_conditions="942a=!1".
Comment 1 David Gustafsson 2018-03-28 12:34:04 UTC
Created attachment 73384 [details] [review]
Bug 20486: Add --marc_conditions option to export_records.pl

Add --marc_conditions option to export_records.pl script, for
excluding records based on conditions applied to MARC-record
data.

How to test:
1) Run tests in t/Exporter/Record.t
2) All tests should pass

Sponsored-by: Gothenburg University Library
Comment 2 David Gustafsson 2018-03-28 12:35:04 UTC
Oops, condition in first comment should be: --marc_conditions="942a!=1" (not =!)
Comment 3 Maksim Sen 2018-03-30 18:26:47 UTC
Created attachment 73497 [details] [review]
Bug 20486: Add --marc_conditions option to export_records.pl

Add --marc_conditions option to export_records.pl script, for
excluding records based on conditions applied to MARC-record
data.

How to test:
1) Run tests in t/Exporter/Record.t
2) All tests should pass

Sponsored-by: Gothenburg University Library
Signed-off-by: Maksim Sen <maksim.sen@inlibro.com>
Comment 4 David Gustafsson 2018-04-11 12:48:27 UTC
Created attachment 73999 [details] [review]
Bug 20486: Add --marc_conditions option to export_records.pl

Add --marc_conditions option to export_records.pl script, for
excluding records based on conditions applied to MARC-record
data.

How to test:
1) Run tests in t/Exporter/Record.t
2) All tests should pass

Sponsored-by: Gothenburg University Library
Comment 5 David Gustafsson 2018-04-11 12:50:25 UTC
Fix a problem where _get_record_for_export returns undef both on error and if not matching condition, so impossible to catch the error, now logging all errors inside of _get_record_for_export instead.
Comment 6 Katrin Fischer 2018-04-22 12:55:54 UTC
Comment on attachment 73999 [details] [review]
Bug 20486: Add --marc_conditions option to export_records.pl

Review of attachment 73999 [details] [review]:
-----------------------------------------------------------------

Same question as on bug 20465: this adds functionality to the CLI script that is not available in the export tool in the GUI. Do we want them to diverge like that? Requesting feedback.

Also:

QA scripts shows a POD issue:
 FAIL	misc/export_records.pl
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   FAIL	  pod
		 
		Apparent command =back not preceded by blank line
		*** ERROR: 
		
		=over on line 263 without closing =back (at head1)
		 in file misc/export_records.pl

::: Koha/Exporter/Record.pm
@@ +77,5 @@
> +                        return undef unless all { $op->($_, $match_value) } @target_values;
> +                    }
> +                }
> +            }
> +            return undef if $no_target && $operator ne '!='; # TODO: ??

Gives me an uneasy feeling. What was the thought here?
Comment 7 David Gustafsson 2018-04-25 19:04:04 UTC
Created attachment 74853 [details] [review]
Bug 20486: Add --marc_conditions option to export_records.pl

Add --marc_conditions option to export_records.pl script, for
excluding records based on conditions applied to MARC-record
data.

How to test:
1) Run tests in t/Exporter/Record.t
2) All tests should pass

Sponsored-by: Gothenburg University Library
Comment 8 David Gustafsson 2018-04-25 19:08:24 UTC
I fixed the pod issue. The reason for the "# TODO: ??" was that not completely sure how ta handle the case where marc target cannot be found for different operators, but think it's handled correctly so removed this. Unfortunately we do not have the resources to port this functionality to the web-UI as well as we do not use it. This was initially just supposed to be a hard coded hack for our local needs but I generalized the matching functionality so other targets that the ones we use can be used for filtering in case someone else needed this.
Comment 9 David Gustafsson 2018-05-07 12:10:04 UTC
Created attachment 75110 [details] [review]
Bug 20486: Add --marc_conditions option to export_records.pl

Add --marc_conditions option to export_records.pl script, for
excluding records based on conditions applied to MARC-record
data.

How to test:
1) Run tests in t/Exporter/Record.t
2) All tests should pass

Sponsored-by: Gothenburg University Library
Comment 10 David Gustafsson 2018-05-07 12:10:49 UTC
Rebased against Koha master.
Comment 11 Michal Denar 2018-09-23 17:04:02 UTC
Created attachment 79266 [details] [review]
Bug 20486: Add --marc_conditions option to export_records.pl

Add --marc_conditions option to export_records.pl script, for
excluding records based on conditions applied to MARC-record
data.

How to test:
1) Run tests in t/Exporter/Record.t
2) All tests should pass

Sponsored-by: Gothenburg University Library

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 12 Chris Cormack 2018-10-10 00:51:41 UTC
Created attachment 80318 [details] [review]
Bug 20486: Add --marc_conditions option to export_records.pl

Add --marc_conditions option to export_records.pl script, for
excluding records based on conditions applied to MARC-record
data.

How to test:
1) Run tests in t/db_dependent/Exporter/Record.t
2) All tests should pass

Sponsored-by: Gothenburg University Library

Signed-off-by: Maksim Sen <maksim.sen@inlibro.com>
Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 13 Chris Cormack 2018-10-10 00:58:54 UTC
I have passed QA

But the question about the divergent functionality between the CLI and the web interface remains unanswered.
I will leave that in the RM hands to decide
Comment 14 Nick Clemens 2018-10-31 15:08:20 UTC
I would like a second opinion here, I would like the scripts to have the same options, but don't consider it a blocker. Can another QA member (or more) weigh in
Comment 15 Jonathan Druart 2018-10-31 15:27:44 UTC
I would not consider it blocker either.

Side note: return undef will raise a perlcritic warning:
  "return" statement with explicit "undef"
Comment 16 Josef Moravec 2018-10-31 16:22:42 UTC
The ui part of this could be done as follow-up bug, no blocker for me. But the return explicit undef should be fixed, we want to pass perlcritic
Comment 17 David Gustafsson 2018-10-31 17:05:59 UTC
Created attachment 81762 [details] [review]
Bug 20486: Add --marc_conditions option to export_records.pl

Add --marc_conditions option to export_records.pl script, for
excluding records based on conditions applied to MARC-record
data.

How to test:
1) Run tests in t/db_dependent/Exporter/Record.t
2) All tests should pass

Sponsored-by: Gothenburg University Library

Signed-off-by: Maksim Sen <maksim.sen@inlibro.com>
Signed-off-by: Michal Denar <black23@gmail.com>
Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 18 David Gustafsson 2018-10-31 17:06:16 UTC
Ok, fixed!
Comment 19 Nick Clemens 2018-11-08 02:26:18 UTC
Awesome work all!

Pushed to master for 18.11
Comment 20 Martin Renvoize 2018-11-16 13:34:36 UTC
Enhancement, will not be backported to 18.05.x series.
Comment 21 Koha Team University Lyon 3 2019-01-17 12:54:33 UTC
Hello,
I have questions about this one : is it possible to add several filters? If it's the case, which is the separator between the several filters? 
Could we use whatever operator in the filter (= or > or < )?

Thanks in advance!

Sonia
Comment 22 Jonathan Druart 2019-02-15 01:04:52 UTC
(In reply to Koha Team Lyon 3 from comment #21)
> Hello,
> I have questions about this one : is it possible to add several filters? If
> it's the case, which is the separator between the several filters? 
> Could we use whatever operator in the filter (= or > or < )?
> 
> Thanks in advance!
> 
> Sonia

Bonjour Sonia,

Reading the POD, it says: 
"""
[...]
with multiple conditions separated by commas (,).
For example: --marc_conditions="035a!=(EXAMPLE)123,041a=swe".
Multiple conditions are all required to match.
"""

The operators are <, >, = and !=