Bug 36473 - updatetotalissues.pl should not die on a bad record
Summary: updatetotalissues.pl should not die on a bad record
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords: additional_work_needed
Depends on:
Blocks: 36474
  Show dependency treegraph
 
Reported: 2024-04-01 14:31 UTC by Nick Clemens
Modified: 2024-04-18 11:17 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the misc/cronjobs/update_totalissues.pl script so that it skips records with invalid data, instead of just stopping. (This also means that the 'Most-circulated items' report now shows the correct data - if the script stopped because of invalid records, the report may have not picked up circulated items.)
Version(s) released in:
24.05.00


Attachments
Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl (1.40 KB, patch)
2024-04-01 14:39 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl (1.44 KB, patch)
2024-04-02 09:12 UTC, David Nind
Details | Diff | Splinter Review
Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl (1.40 KB, patch)
2024-04-02 13:45 UTC, Laura Escamilla
Details | Diff | Splinter Review
Bug 36473: Unit tests (1.45 KB, patch)
2024-04-02 13:45 UTC, Laura Escamilla
Details | Diff | Splinter Review
Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl (1.40 KB, patch)
2024-04-02 13:45 UTC, Laura Escamilla
Details | Diff | Splinter Review
Bug 36473: Unit tests (1.45 KB, patch)
2024-04-02 18:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl (1.45 KB, patch)
2024-04-02 18:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 36473: Unit tests (1.57 KB, patch)
2024-04-05 08:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl (1.48 KB, patch)
2024-04-05 08:52 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36473: Unit tests (1.57 KB, patch)
2024-04-12 08:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl (1.54 KB, patch)
2024-04-12 08:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 36473: [ALTERNATIVE PATCH] Prevent UpdateTotalIssues failure to crash update_totalissues.pl (1.27 KB, patch)
2024-04-18 07:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 36473: (follow-up) Correct biblio to bibliographic (927 bytes, patch)
2024-04-18 10:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 36473: (follow-up) Add biblionumber to warning (1.79 KB, patch)
2024-04-18 10:36 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 36473: (follow-up) Additionally warn the stringified exception (2.46 KB, patch)
2024-04-18 11:17 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2024-04-01 14:31:23 UTC
See bug 36013 comment 3
Comment 1 Nick Clemens 2024-04-01 14:39:35 UTC Comment hidden (obsolete)
Comment 2 David Nind 2024-04-02 09:12:18 UTC Comment hidden (obsolete)
Comment 3 Laura Escamilla 2024-04-02 13:45:13 UTC Comment hidden (obsolete)
Comment 4 Laura Escamilla 2024-04-02 13:45:48 UTC Comment hidden (obsolete)
Comment 5 Laura Escamilla 2024-04-02 13:45:50 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens 2024-04-02 18:01:38 UTC
Created attachment 164289 [details] [review]
Bug 36473: Unit tests
Comment 7 Nick Clemens 2024-04-02 18:01:40 UTC
Created attachment 164290 [details] [review]
Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl

This patch wraps the call for record in an eval, and catches any invalid metadata
exceptions, letting the warning show, but allowing the script to continue
To test:
1 - In default KTD record 369 has problems, otherwise you need to break a record
2 - Run : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v
3 - It dies at record 369 (or the one you broke)
4 - Apply patch
5 - Run again
6 - It succeeds, but skips the bad record

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 Marcel de Rooy 2024-04-05 08:51:31 UTC
            if ( C4::Context->preference('UpdateTotalIssuesOnCirc') ) {
                UpdateTotalIssues( $item_object->biblionumber, 1, undef, { skip_holds_queue => 1 } );
            }

IIUC AddIssue will no longer crash on an invalid metadata record, right? Is that really what we want?
Comment 9 Marcel de Rooy 2024-04-05 08:52:07 UTC
Created attachment 164458 [details] [review]
Bug 36473: Unit tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Tidied.
Comment 10 Marcel de Rooy 2024-04-05 08:52:09 UTC
Created attachment 164459 [details] [review]
Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl

This patch wraps the call for record in an eval, and catches any invalid metadata
exceptions, letting the warning show, but allowing the script to continue
To test:
1 - In default KTD record 369 has problems, otherwise you need to break a record
2 - Run : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v
3 - It dies at record 369 (or the one you broke)
4 - Apply patch
5 - Run again
6 - It succeeds, but skips the bad record

Signed-off-by: David Nind <david@davidnind.com>
Comment 11 Marcel de Rooy 2024-04-05 08:52:51 UTC
Changing status for feedback
Comment 12 Nick Clemens 2024-04-08 13:35:55 UTC
(In reply to Marcel de Rooy from comment #8)
>             if ( C4::Context->preference('UpdateTotalIssuesOnCirc') ) {
>                 UpdateTotalIssues( $item_object->biblionumber, 1, undef, {
> skip_holds_queue => 1 } );
>             }
> 
> IIUC AddIssue will no longer crash on an invalid metadata record, right? Is
> that really what we want?

This was an unintended consequence, however, I do think this is what we want - there will be a warning in the logs - but why should we prevent checking out an item with a bad record? We should be handling that on creation or editing of a record.
Comment 13 Marcel de Rooy 2024-04-12 08:53:48 UTC
Created attachment 164805 [details] [review]
Bug 36473: Unit tests

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Tidied.
Comment 14 Marcel de Rooy 2024-04-12 08:53:51 UTC
Created attachment 164806 [details] [review]
Bug 36473: Handle Invalid Metadata Exceptions in totalissues.pl

This patch wraps the call for record in an eval, and catches any invalid metadata
exceptions, letting the warning show, but allowing the script to continue
To test:
1 - In default KTD record 369 has problems, otherwise you need to break a record
2 - Run : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v
3 - It dies at record 369 (or the one you broke)
4 - Apply patch
5 - Run again
6 - It succeeds, but skips the bad record

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Katrin Fischer 2024-04-12 10:12:58 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 16 Katrin Fischer 2024-04-13 11:22:51 UTC
It looks like this might make the tests fail (needs further investigation):

t_db_dependent_Biblio_t.UpdateTotalIssues on Invalid record (2144)
 Error Details

not ok 21 - UpdateTotalIssues on Invalid record
Comment 17 Katrin Fischer 2024-04-16 15:15:45 UTC
I have a hunch that this broke tests:

prove t/db_dependent/Biblio.t

    #   Failed test 'Expected warning found'
    #   at t/db_dependent/Biblio.t line 1039.
    # found warning: UpdateTotalIssues could not get bibliographic record at /kohadevbox/koha/C4/Biblio.pm line 3065.
    # expected to find warning: UpdateTotalIssues could not get biblio record
    # Looks like you failed 1 test of 2.

#   Failed test 'UpdateTotalIssues on Invalid record'
#   at t/db_dependent/Biblio.t line 1043.
# Looks like you failed 1 test of 21.
t/db_dependent/Biblio.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/21 subtests
Comment 18 Jonathan Druart 2024-04-18 07:28:21 UTC
I don't understand this fix. Why don't you eval/try-catch in the script instead of inside UpdateTotalIssues?

Also the warn should display the error, otherwise you are potentially hidden useful warnings.

Please fix ASAP, Jenkins is failing!
Comment 19 Jonathan Druart 2024-04-18 07:32:46 UTC
Created attachment 165039 [details] [review]
Bug 36473: [ALTERNATIVE PATCH] Prevent UpdateTotalIssues failure to crash update_totalissues.pl
Comment 20 Jonathan Druart 2024-04-18 07:33:14 UTC
This is how I would have fixed it initially.
Comment 21 Nick Clemens 2024-04-18 10:36:22 UTC
Created attachment 165077 [details] [review]
Bug 36473: (follow-up) Correct biblio to bibliographic
Comment 22 Nick Clemens 2024-04-18 10:36:24 UTC
Created attachment 165078 [details] [review]
Bug 36473: (follow-up) Add biblionumber to warning
Comment 23 Nick Clemens 2024-04-18 11:17:55 UTC
Created attachment 165079 [details] [review]
Bug 36473: (follow-up) Additionally warn the stringified exception