Bug 12995 - script update_totalissues.pl stops on corrupted record
Summary: script update_totalissues.pl stops on corrupted record
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Fridolin Somers
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks: 13230
  Show dependency treegraph
 
Reported: 2014-09-25 13:28 UTC by Fridolin Somers
Modified: 2015-12-03 22:11 UTC (History)
6 users (show)

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


Attachments
Bug 12995 - script update_totalissues.pl stops on corrupted record (2.05 KB, patch)
2014-09-25 13:35 UTC, Fridolin Somers
Details | Diff | Splinter Review
Signed off patch (2.12 KB, patch)
2014-10-16 17:13 UTC, Cindy Murdock Ames
Details | Diff | Splinter Review
Bug 12995 - script update_totalissues.pl stops on corrupted record (1.06 KB, patch)
2014-10-20 13:16 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 12995 - script update_totalissues.pl stops on corrupted record - UT (1.07 KB, patch)
2014-10-20 13:17 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 12995 - script update_totalissues.pl stops on corrupted record (4.23 KB, patch)
2014-10-20 13:22 UTC, Fridolin Somers
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12995 - script update_totalissues.pl stops on corrupted record (4.43 KB, patch)
2014-10-23 15:41 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12995 - script update_totalissues.pl stops on corrupted record - UT (1.26 KB, patch)
2014-10-23 15:41 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 12995 - script update_totalissues.pl stops on corrupted record - UT (1.38 KB, patch)
2014-10-24 06:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12995 - script update_totalissues.pl stops on corrupted record (4.44 KB, patch)
2014-10-24 21:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12995 - script update_totalissues.pl stops on corrupted record - UT (1.38 KB, patch)
2014-10-24 21:33 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin Somers 2014-09-25 13:28:16 UTC
When running update_totalissues.pl cronjob, it will stop on a corrupted record.
It should alert and process next record.
Comment 1 Fridolin Somers 2014-09-25 13:35:12 UTC Comment hidden (obsolete)
Comment 2 Cindy Murdock Ames 2014-10-16 17:13:13 UTC Comment hidden (obsolete)
Comment 3 Marcel de Rooy 2014-10-17 07:38:23 UTC
QA Comment:
Fridolin, although our coding guidelines do not say anything about the use of eval, I do not think that this is the most elegant solution. It also will degrade performance btw.
The actual problem is somewhere else and I would prefer to solve it there.

In this case the corrupted record will be discovered by GetMarcBiblio (it also contains an eval already btw!).
Note that GetMarcBiblio returns undef now, but this return value is NOT checked by UpdateTotalIssues. So here we come closer.. 
UpdateTotalIssues should test that value and not just say $record->field..
(There will be [much] more instances where this happens.]

So, we could do at least two things now:
[1] Return from UpdateTotalIssues: return if !$record; the cronjob continues. (Note that there is a warn in GetMarcBiblio.)
[2] Also return a value to the caller (note that only the cronjob and Circulation.pm call this routine). Print the number of skipped records in the cronjob.

Setting status to Failed QA
Comment 4 Fridolin Somers 2014-10-20 09:37:04 UTC
(In reply to M. de Rooy from comment #3)
Thanks for your advice, I agree 100%.
Comment 5 Fridolin Somers 2014-10-20 13:16:20 UTC Comment hidden (obsolete)
Comment 6 Fridolin Somers 2014-10-20 13:17:58 UTC Comment hidden (obsolete)
Comment 7 Fridolin Somers 2014-10-20 13:22:29 UTC Comment hidden (obsolete)
Comment 8 Fridolin Somers 2014-10-20 13:25:48 UTC
Here we are, new patches.
I've added a simple unit test on C4::Biblio::UpdateTotalIssues.

One may create more complex unit tests to cover all cases.
Comment 9 Owen Leonard 2014-10-23 15:41:44 UTC Comment hidden (obsolete)
Comment 10 Owen Leonard 2014-10-23 15:41:54 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2014-10-24 06:19:49 UTC Comment hidden (obsolete)
Comment 12 Marcel de Rooy 2014-10-24 07:48:30 UTC
I have the impression that the improved patch became obsoleted and do not know if it was qa-ed ?
Comment 13 Katrin Fischer 2014-10-24 21:33:07 UTC
Created attachment 32695 [details] [review]
[PASSED QA] Bug 12995 - script update_totalissues.pl stops on corrupted record

When running update_totalissues.pl cronjob, it will stop on a corrupted
record.

This patch changes UpdateTotalIssues so that it return 1 if processing
record has succeded. Also, if mapping with biblioitems.totalissues does
not exist, the method has nothing to do so it stops and returns 1.

When processing a corrupted record, script now alerts about the error on
this biblionumber (if script is verbose) and process next record.
A total number of records with error will be printed at the end of the
script.

Test plan :
- Create a dabase with a few biblios and some issues
- Modify first biblio record (use direct sql update) : set empty value
  in biblioitems.marcxml
- Launch script : misc/cronjobs/update_totalissues.pl --use-stats --commit=1000 -v

=> Without patch : the script stops at first record
=> With patch : the script prints error for first record and processes
   all records

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
I was able to confirm the problem before the patch and successfully
follow the test plan.

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2014-10-24 21:33:28 UTC
Created attachment 32696 [details] [review]
[PASSED QA] Bug 12995 - script update_totalissues.pl stops on corrupted record - UT

This patch adds an unit test on C4::Biblio::UpdateTotalIssues method
call on non existing record

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
I can't comment on the correctness of the test other than to say it ran
successfully.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Improves CLI script, works as described.

Note: A small improvement would be to output the problematic biblionumber.
Comment 15 Katrin Fischer 2014-10-24 21:33:56 UTC
Sorry, my fault - missed to attach the first patch.
Comment 16 Tomás Cohen Arazi 2014-10-31 17:53:38 UTC
Patches pushed to master.

Thanks Fridolin!
Comment 17 Robin Sheat 2014-11-11 02:31:13 UTC
(In reply to Katrin Fischer from comment #14)
> Created attachment 32696 [details] [review] [review]
> [PASSED QA] Bug 12995 - script update_totalissues.pl stops on corrupted
> record - UT
> 
> This patch adds an unit test on C4::Biblio::UpdateTotalIssues method
> call on non existing record
> 
> Signed-off-by: Owen Leonard <oleonard@myacpl.org>
> I can't comment on the correctness of the test other than to say it ran
> successfully.
> 
> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
> Improves CLI script, works as described.
> 
> Note: A small improvement would be to output the problematic biblionumber.

This patch causes the package builds to fail, as it requires a database.
Comment 18 Fridolin Somers 2014-11-12 15:31:33 UTC
(In reply to Robin Sheat from comment #17)
> This patch causes the package builds to fail, as it requires a database.
Oh, carp.
How can we fix the UT ? mock the db ?
Comment 19 Mark Tompsett 2014-11-12 18:00:19 UTC
(In reply to Fridolin SOMERS from comment #18)
> (In reply to Robin Sheat from comment #17)
> > This patch causes the package builds to fail, as it requires a database.
> Oh, carp.
> How can we fix the UT ? mock the db ?

See bug 13230. There's a patch for this.