Bug 20551 - Add option for including deleted records in export_records.pl
Summary: Add option for including deleted records in export_records.pl
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: David Gustafsson
QA Contact: Testopia
URL:
Keywords:
: 17693 (view as bug list)
Depends on:
Blocks: 23009
  Show dependency treegraph
 
Reported: 2018-04-10 09:17 UTC by David Gustafsson
Modified: 2021-02-05 15:04 UTC (History)
7 users (show)

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


Attachments
Bug 20551: Add option for including deleted records in export_records.pl (16.12 KB, patch)
2018-04-10 09:33 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20551: Add option for including deleted records in export_records.pl (18.27 KB, patch)
2018-04-10 12:19 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20551: Add option for including deleted records in export_records.pl (18.36 KB, patch)
2018-05-07 12:15 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20551: Add option for including deleted records in export_records.pl (17.55 KB, patch)
2018-12-04 15:12 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20551: Add option for including deleted records in export_records.pl (17.52 KB, patch)
2018-12-06 15:37 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20551: Add option for including deleted records in export_records.pl (18.01 KB, patch)
2018-12-06 15:54 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20551: Add option for including deleted records in export_records.pl (18.83 KB, patch)
2019-05-29 12:24 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20551: Fix option name in pod (1.08 KB, patch)
2019-05-29 14:39 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20551: Rename "marcflavour" field to "schema" (1.66 KB, patch)
2019-05-31 10:42 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20551: Add option for including deleted records in export_records.pl (18.82 KB, patch)
2019-10-03 14:24 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20551: Fix option name in pod (1.08 KB, patch)
2019-10-03 14:24 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20551: Rename "marcflavour" field to "schema" (836 bytes, patch)
2019-10-03 14:24 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20551: Improve documentation and make compatible with more options (2.24 KB, patch)
2020-05-27 14:50 UTC, David Gustafsson
Details | Diff | Splinter Review
Bug 20551: Add option for including deleted records in export_records.pl (18.87 KB, patch)
2021-02-05 15:04 UTC, Frank Hansen
Details | Diff | Splinter Review
Bug 20551: Fix option name in pod (1.13 KB, patch)
2021-02-05 15:04 UTC, Frank Hansen
Details | Diff | Splinter Review
Bug 20551: Rename "marcflavour" field to "schema" (888 bytes, patch)
2021-02-05 15:04 UTC, Frank Hansen
Details | Diff | Splinter Review
Bug 20551: Improve documentation and make compatible with more options (2.29 KB, patch)
2021-02-05 15:04 UTC, Frank Hansen
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-04-10 09:17:23 UTC
Add an option "--include_deleted" to also include possibly deleted biblios in export. This option can only be used together with --date, and --record-type=biblio. The rational behind this is that use case does not makes much sense (if you are doing a full export, why would you include deleted biblios) and thus some extra complexity can be avoided.
 
Superficially related to https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17693, not really quite different as this includes the whole biblio-record (with record status set to "d" in leader) in the regular export, not just biblio ids. This is a requirement for synchronizing with the discovery-system we are currently using for example and imho is a more standard and MARC-compliant way of supporting export of record deletions.
Comment 1 David Gustafsson 2018-04-10 09:33:17 UTC Comment hidden (obsolete)
Comment 2 David Gustafsson 2018-04-10 12:19:56 UTC Comment hidden (obsolete)
Comment 3 David Gustafsson 2018-04-10 12:20:38 UTC
As some might need it, also added an '--deleted_only' option, to export only the deleted biblios.
Comment 4 David Gustafsson 2018-05-07 12:15:41 UTC Comment hidden (obsolete)
Comment 5 David Gustafsson 2018-05-07 12:15:56 UTC
Rebased against Koha master.
Comment 6 David Gustafsson 2018-12-04 15:12:48 UTC
Created attachment 82843 [details] [review]
Bug 20551: Add option for including deleted records in export_records.pl

Add option "--include_deleted" to include deleted
biblios in export_records.pl as marc records with
record status (in leader) set to "d" and
"--deleted_only" to export only deleted biblios.

How to test:
1) Run tests in t/db_dependent/Exporter/Record.t
2) All tests should pass
3) Delete a biblio record in Koha
4) Run the export script as:
   `export_records.pl --date=<date-just-before-deletion>
   --include_deleted`
5) Open the generated koha.mrc, and verify that contains
   the recently deleted record, and that the record header
   has record status "d".
6) Perform the same check with:
   `export_records.pl --date=<date-just-before-deletion>
      --include_deleted --format=xml`
7) Run:
   `export_records.pl --date=<date-just-before-deletion>
      --deleted_only --format=xml`
    and verify that only the deleted biblio was included
    in the export.

Sponsored-by: Gothenburg University Library
Comment 7 David Gustafsson 2018-12-04 15:14:51 UTC
Rebase against master (untested, will test later and verify that still works).
Comment 8 Owen Leonard 2018-12-05 15:44:06 UTC
The functionality of this works as described in the test plan. However, the QA tools raise this warning:

 FAIL   Koha/Exporter/Record.pm
   FAIL   critic
                # Subroutines::ProhibitExplicitReturnUndef: Got 1 violation(s).

And t/db_dependent/Exporter/Record.t returns this error:

t/db_dependent/Exporter/Record.t .. 1/6     # No tests run!

#   Failed test 'No tests run for subtest "export xml"'
#   at t/db_dependent/Exporter/Record.t line 187.
    # No tests run!

#   Failed test 'No tests run for subtest "export iso2709"'
#   at t/db_dependent/Exporter/Record.t line 219.
# Looks like you failed 2 tests of 6.
t/db_dependent/Exporter/Record.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/6 subtests

Test Summary Report
-------------------
t/db_dependent/Exporter/Record.t (Wstat: 512 Tests: 6 Failed: 2)
  Failed tests:  2-3
  Non-zero exit status: 2
Files=1, Tests=6,  3 wallclock secs ( 0.02 usr  0.01 sys +  1.41 cusr  0.69 csys =  2.13 CPU)
Result: FAIL
Comment 9 David Gustafsson 2018-12-06 15:37:23 UTC
Created attachment 82914 [details] [review]
Bug 20551: Add option for including deleted records in export_records.pl

Add option "--include_deleted" to include deleted
biblios in export_records.pl as marc records with
record status (in leader) set to "d" and
"--deleted_only" to export only deleted biblios.

How to test:
1) Run tests in t/db_dependent/Exporter/Record.t
2) All tests should pass
3) Delete a biblio record in Koha
4) Run the export script as:
   `export_records.pl --date=<date-just-before-deletion>
   --include_deleted`
5) Open the generated koha.mrc, and verify that contains
   the recently deleted record, and that the record header
   has record status "d".
6) Perform the same check with:
   `export_records.pl --date=<date-just-before-deletion>
      --include_deleted --format=xml`
7) Run:
   `export_records.pl --date=<date-just-before-deletion>
      --deleted_only --format=xml`
    and verify that only the deleted biblio was included
    in the export.

Sponsored-by: Gothenburg University Library
Comment 10 David Gustafsson 2018-12-06 15:38:58 UTC
Found the culprit (next in map that accidentally got there in rebase, where should just return empty list), now tests should pass.
Comment 11 David Gustafsson 2018-12-06 15:39:48 UTC
Forgot to fix the perl critic issue, will do.
Comment 12 David Gustafsson 2018-12-06 15:54:38 UTC
Created attachment 82918 [details] [review]
Bug 20551: Add option for including deleted records in export_records.pl

Add option "--include_deleted" to include deleted
biblios in export_records.pl as marc records with
record status (in leader) set to "d" and
"--deleted_only" to export only deleted biblios.

How to test:
1) Run tests in t/db_dependent/Exporter/Record.t
2) All tests should pass
3) Delete a biblio record in Koha
4) Run the export script as:
   `export_records.pl --date=<date-just-before-deletion>
   --include_deleted`
5) Open the generated koha.mrc, and verify that contains
   the recently deleted record, and that the record header
   has record status "d".
6) Perform the same check with:
   `export_records.pl --date=<date-just-before-deletion>
      --include_deleted --format=xml`
7) Run:
   `export_records.pl --date=<date-just-before-deletion>
      --deleted_only --format=xml`
    and verify that only the deleted biblio was included
    in the export.

Sponsored-by: Gothenburg University Library
Comment 13 Frank Hansen 2019-05-13 09:45:01 UTC
Patch didn't apply for me. I got:

CONFLICT (content): Merge conflict in misc/export_records.pl
Failed to merge in the changes.
Patch failed at 0001 Bug 20551: Add option for including deleted records in export_records.pl
Comment 14 Katrin Fischer 2019-05-13 11:27:05 UTC
Same for me, marking "Patch doesn't apply". Frnak, you can set that status too if you run into this the next time!
Comment 15 David Gustafsson 2019-05-29 12:24:30 UTC
Created attachment 90184 [details] [review]
Bug 20551: Add option for including deleted records in export_records.pl

Add option "--include_deleted" to include deleted
biblios in export_records.pl as marc records with
record status (in leader) set to "d" and
"--deleted_only" to export only deleted biblios.

How to test:
1) Run tests in t/db_dependent/Exporter/Record.t
2) All tests should pass
3) Delete a biblio record in Koha
4) Run the export script as:
   `export_records.pl --date=<date-just-before-deletion>
   --include_deleted`
5) Open the generated koha.mrc, and verify that contains
   the recently deleted record, and that the record header
   has record status "d".
6) Perform the same check with:
   `export_records.pl --date=<date-just-before-deletion>
      --include_deleted --format=xml`
7) Run:
   `export_records.pl --date=<date-just-before-deletion>
      --deleted_only --format=xml`
    and verify that only the deleted biblio was included
    in the export.

Sponsored-by: Gothenburg University Library
Comment 16 David Gustafsson 2019-05-29 12:24:47 UTC
Rebased against master.
Comment 17 David Gustafsson 2019-05-29 14:39:30 UTC
Created attachment 90190 [details] [review]
Bug 20551: Fix option name in pod
Comment 18 David Gustafsson 2019-05-31 10:42:29 UTC
Created attachment 90220 [details] [review]
Bug 20551: Rename "marcflavour" field to "schema"
Comment 19 Eric Bégin 2019-10-03 14:11:49 UTC
Test during hackfest 2019

error: sha1 information is lacking or useless (Koha/Exporter/Record.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 20551: Rename "marcflavour" field to "schema"
Comment 20 David Gustafsson 2019-10-03 14:24:44 UTC
Created attachment 93619 [details] [review]
Bug 20551: Add option for including deleted records in export_records.pl

Add option "--include_deleted" to include deleted
biblios in export_records.pl as marc records with
record status (in leader) set to "d" and
"--deleted_only" to export only deleted biblios.

How to test:
1) Run tests in t/db_dependent/Exporter/Record.t
2) All tests should pass
3) Delete a biblio record in Koha
4) Run the export script as:
   `export_records.pl --date=<date-just-before-deletion>
   --include_deleted`
5) Open the generated koha.mrc, and verify that contains
   the recently deleted record, and that the record header
   has record status "d".
6) Perform the same check with:
   `export_records.pl --date=<date-just-before-deletion>
      --include_deleted --format=xml`
7) Run:
   `export_records.pl --date=<date-just-before-deletion>
      --deleted_only --format=xml`
    and verify that only the deleted biblio was included
    in the export.

Sponsored-by: Gothenburg University Library
Comment 21 David Gustafsson 2019-10-03 14:24:49 UTC
Created attachment 93620 [details] [review]
Bug 20551: Fix option name in pod
Comment 22 David Gustafsson 2019-10-03 14:24:52 UTC
Created attachment 93621 [details] [review]
Bug 20551: Rename "marcflavour" field to "schema"
Comment 23 David Gustafsson 2019-10-03 14:25:25 UTC
Should now be rebased against master.
Comment 24 Frank Hansen 2020-04-14 11:32:03 UTC
(In reply to David Gustafsson from comment #23)
> Should now be rebased against master.

The patch itself applies and works fine for me. However, the test doesn't pass:

t/db_dependent/Exporter/Record.t .. 1/6
    #   Failed test 'Deleted record has the correct leader value'
    #   at t/db_dependent/Exporter/Record.t line 186.
    #          got: '00134dam a22000617a 4500'
    #     expected: '00136dam a22000617a 4500'
    # Looks like you failed 1 test of 4.

#   Failed test 'export xml'
#   at t/db_dependent/Exporter/Record.t line 187.

    #   Failed test 'Deleted record has the correct leader value'
    #   at t/db_dependent/Exporter/Record.t line 218.
    #          got: '00134dam a22000617a 4500'
    #     expected: '00136dam a22000617a 4500'
    # Looks like you failed 1 test of 4.

#   Failed test 'export iso2709'
#   at t/db_dependent/Exporter/Record.t line 219.
t/db_dependent/Exporter/Record.t .. 5/6 # Looks like you failed 2 tests of 6.
t/db_dependent/Exporter/Record.t .. Dubious, test returned 2 (wstat 512, 0x200)
Failed 2/6 subtests

Test Summary Report
-------------------
t/db_dependent/Exporter/Record.t (Wstat: 512 Tests: 6 Failed: 2)
  Failed tests:  2-3
  Non-zero exit status: 2
Files=1, Tests=6,  4 wallclock secs ( 0.02 usr  0.00 sys +  2.94 cusr  0.28 csys =  3.24 CPU)
Result: FAIL
Comment 25 Frank Hansen 2020-04-15 08:49:21 UTC
The test failed because I ran the test on an empty database.
I will now sign off the patch.
Comment 26 Jonathan Druart 2020-04-15 15:22:12 UTC
1. Doesn't it make sense to have this for authority records as well?

2. Why is --date mandatory?

3. POD does not tell that --deleted_only and --include_deleted is only available for bibs

4. --id_list_file does not work with --deleted_only

5. Items are not exported, I guess that's expected but maybe we should have it noted somewhere?
Comment 27 Jonathan Druart 2020-04-16 10:30:50 UTC
*** Bug 17693 has been marked as a duplicate of this bug. ***
Comment 28 Frank Hansen 2020-04-17 09:38:05 UTC
(In reply to Jonathan Druart from comment #26)
> 1. Doesn't it make sense to have this for authority records as well?

The authority records are stored in auth_header table. But deleted records is not kept anywhere else. As I can see, this would require to keep deleted authority records in an another table like deletedauth_header.
Comment 29 Katrin Fischer 2020-05-23 11:43:54 UTC
Hi Frank, you are right, we do not keep the deleted authorities, so that takes care of 1. I think some of the other questions could still be answered/improved:


(In reply to Jonathan Druart from comment #26)
> 1. Doesn't it make sense to have this for authority records as well?

Because we don't keep them ;)

> 2. Why is --date mandatory?

Not sure about this one, can you explain?

> 3. POD does not tell that --deleted_only and --include_deleted is only
> available for bibs

Easy fix to documentation I think and it could help people who are not aware of database structure.

> 4. --id_list_file does not work with --deleted_only

Cannot tell about this one, but sounds worth looking into.

> 5. Items are not exported, I guess that's expected but maybe we should have
> it noted somewhere?

Maybe also a note in the docs?
Comment 30 David Gustafsson 2020-05-27 14:37:21 UTC
(In reply to Jonathan Druart from comment #26)
> 1. Doesn't it make sense to have this for authority records as well?
> 
> 2. Why is --date mandatory?
> 
> 3. POD does not tell that --deleted_only and --include_deleted is only
> available for bibs
> 
> 4. --id_list_file does not work with --deleted_only
> 
> 5. Items are not exported, I guess that's expected but maybe we should have
> it noted somewhere?

2) I think I just assumed it was required since included in the SQL-query, also before the patch. Did not realize that 
"SELECT * FROM biblio_metadata WHERE timestamp > ''" is equivalent with "SELECT * FROM biblio_metadata" if every timestamp entry has a value. So with that in mind an empty timestamp is ok, though the code is a real eyesore :)

3) I think it's already pretty clear since will produce an error message is --included_deleted or --deleted_only is used in conjunction with record_type other than 'bibs'. But I clarified this in the usage documentation for said options. 

4) Did not work since have to add special case for @deleted_record_ids, fixed this.

5). There actually is a "deleteditems" table so I guess it would be possible to include these, even though it's hard to imagine why you would want to do this. Since the default is to include items perhaps it would be best to leave this as is and never include items for deleted biblios, or you will get more data than you probably want by default. It would also increase code complexity, probably with little benefit. If someone requests this it can be fixed later perhaps. Will point it out in the documentation that items are not included.
Comment 31 David Gustafsson 2020-05-27 14:50:16 UTC
Created attachment 105403 [details] [review]
Bug 20551: Improve documentation and make compatible with more options
Comment 32 Frank Hansen 2021-02-05 15:04:39 UTC
Created attachment 116408 [details] [review]
Bug 20551: Add option for including deleted records in export_records.pl

Add option "--include_deleted" to include deleted
biblios in export_records.pl as marc records with
record status (in leader) set to "d" and
"--deleted_only" to export only deleted biblios.

How to test:
1) Run tests in t/db_dependent/Exporter/Record.t
2) All tests should pass
3) Delete a biblio record in Koha
4) Run the export script as:
   `export_records.pl --date=<date-just-before-deletion>
   --include_deleted`
5) Open the generated koha.mrc, and verify that contains
   the recently deleted record, and that the record header
   has record status "d".
6) Perform the same check with:
   `export_records.pl --date=<date-just-before-deletion>
      --include_deleted --format=xml`
7) Run:
   `export_records.pl --date=<date-just-before-deletion>
      --deleted_only --format=xml`
    and verify that only the deleted biblio was included
    in the export.

Sponsored-by: Gothenburg University Library

Great job!

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Comment 33 Frank Hansen 2021-02-05 15:04:43 UTC
Created attachment 116409 [details] [review]
Bug 20551: Fix option name in pod

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Comment 34 Frank Hansen 2021-02-05 15:04:46 UTC
Created attachment 116410 [details] [review]
Bug 20551: Rename "marcflavour" field to "schema"

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>
Comment 35 Frank Hansen 2021-02-05 15:04:50 UTC
Created attachment 116411 [details] [review]
Bug 20551: Improve documentation and make compatible with more options

Signed-off-by: Frank Hansen <frank.hansen@ub.lu.se>