Bug 13531 - bulkmarcimport bombs if no match is found
Summary: bulkmarcimport bombs if no match is found
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: MARC Bibliographic record staging/import (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Galen Charlton
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-01-07 13:39 UTC by Martin Renvoize
Modified: 2016-01-19 04:37 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13531 - bulkmarcimport bombs if no match is found Changed the die statment to a warn allowing the import to continue. (1.34 KB, patch)
2015-01-13 03:33 UTC, Thomas Wright
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13531 - bulkmarcimport bombs if no match is found Changed the die statment to a warn allowing the import to continue. (1.40 KB, patch)
2015-01-13 09:46 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 13531: QA follow up (1.33 KB, patch)
2015-01-14 20:22 UTC, Thomas Wright
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13531: QA follow up (1.40 KB, patch)
2015-01-15 10:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Follow up (960 bytes, patch)
2015-01-23 12:20 UTC, Thomas Wright
Details | Diff | Splinter Review
[SIGNED OFF] Bug 13531 - Follow up (1.09 KB, patch)
2015-03-05 16:33 UTC, Magnus Enger
Details | Diff | Splinter Review
[PASSED QA] Bug 13531 - bulkmarcimport bombs if no match is found Changed the die statment to a warn allowing the import to continue. (1.46 KB, patch)
2015-03-20 13:57 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13531 - bulkmarcimport bombs if no match is found Changed the die statment to a warn allowing the import to continue. (1.46 KB, patch)
2015-03-20 14:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13531: QA follow up (1.46 KB, patch)
2015-03-20 14:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 13531 - Follow up (1.16 KB, patch)
2015-03-20 14:19 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2015-01-07 13:39:30 UTC
When using bulkmarcimport with the below command it would appear that if the match attribute is missing from the import file that the script dies at that point in the import.

../kohaclone/misc/migration_tools/bulkmarcimport.pl -filter="952" -update -match="Control-number,001" -file Results/all_ches_bib.mrc -v -l Logs/overlay.log

I feel this should report the error, but more conventiently continue with the rest of the import batch thereafter.
Comment 1 Martin Renvoize 2015-01-07 13:40:41 UTC
The errors presented are:

DBD::mysql::st execute failed: Column 'biblio_auth_number' cannot be null at /home/koha/kohaclone/C4/Biblio.pm line 2871.

followed by:

unable to search the database for duplicates : No query entered at ../kohaclone/misc/migration_tools/bulkmarcimport.pl line 272, <GEN13> line 60379
Comment 2 Thomas Wright 2015-01-13 02:29:55 UTC
I'll work on this.
Comment 3 Thomas Wright 2015-01-13 03:33:07 UTC Comment hidden (obsolete)
Comment 4 Martin Renvoize 2015-01-13 09:46:03 UTC Comment hidden (obsolete)
Comment 5 Martin Renvoize 2015-01-13 09:47:07 UTC
Thank Thomas, caught the error nicely and good clean handling :)

Signing Off
Comment 6 Martin Renvoize 2015-01-14 08:09:37 UTC
Comment on attachment 35147 [details] [review]
[SIGNED OFF] Bug 13531 - bulkmarcimport bombs if no match is found Changed the die statment to a warn allowing the import to continue.

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

A minor QA comment.

::: misc/migration_tools/bulkmarcimport.pl
@@ +271,5 @@
>          my ( $error, $results, $totalhits ) = C4::Search::SimpleSearch( $query, 0, 3, [$server] );
> +        # changed to warn so able to continue with one broken record
> +        if ( defined $error ) {
> +            warn "unable to search the database for duplicates : $error";
> +            next;

For consistency with the rest of the script, should this perhaps be:

next RECORD;
Comment 7 Thomas Wright 2015-01-14 20:22:21 UTC Comment hidden (obsolete)
Comment 8 Martin Renvoize 2015-01-15 10:01:22 UTC Comment hidden (obsolete)
Comment 9 Martin Renvoize 2015-01-15 10:01:52 UTC
Awesome speedy followup there Thomas.. thanks :)
Comment 10 Jonathan Druart 2015-01-15 11:35:11 UTC
Martin, shouldn't we call the printlog subroutine too?
Comment 11 Martin Renvoize 2015-01-16 11:13:50 UTC
(In reply to Jonathan Druart from comment #10)
> Martin, shouldn't we call the printlog subroutine too?

Indeed, I think your right Jonathan.. this is something worth logging to file rather than just dumping to stdout.. it looks like in the above invocations it does both.. which seems a little messy to me.. but I suppose it does make the user aware that they should look at the log file.

Cheers for pointing it out.. perhaps another qa followup.

I have a feeling Thomas has completed his academy placement now.. I'll leave it for him for a few days just in case he want to complete the work, but go ahead and do it myself if it doesn't get done.

Cheers
Comment 12 Thomas Wright 2015-01-18 10:17:11 UTC
(In reply to Martin Renvoize from comment #11)
> (In reply to Jonathan Druart from comment #10)
> > Martin, shouldn't we call the printlog subroutine too?
> 
> Indeed, I think your right Jonathan.. this is something worth logging to
> file rather than just dumping to stdout.. it looks like in the above
> invocations it does both.. which seems a little messy to me.. but I suppose
> it does make the user aware that they should look at the log file.
> 
> Cheers for pointing it out.. perhaps another qa followup.
> 
> I have a feeling Thomas has completed his academy placement now.. I'll leave
> it for him for a few days just in case he want to complete the work, but go
> ahead and do it myself if it doesn't get done.
> 
> Cheers

Hey Martin,

The academy is over, but I am hoping to continue with contribution to Koha.
Without Chris to guide me could you please advise me as to where I would find an example of the log syntax. Not sure if I will be able to figure it out, but i'll try! 

Thanks
Comment 13 Martin Renvoize 2015-01-19 08:06:17 UTC
(In reply to Thomas Wright from comment #12)
> Hey Martin,
> 
> The academy is over, but I am hoping to continue with contribution to Koha.
> Without Chris to guide me could you please advise me as to where I would
> find an example of the log syntax. Not sure if I will be able to figure it
> out, but i'll try! 
> 
> Thanks

That's great news that your tempted to stick around Thomas, we can always do with more keen contributors and you'll find even outside the Catalyst environment you should get allot of support for us.. Have you discovered the IRC channel, it's a great place to seek advice if/whenever your stuck :)

As for the log syntax, this script is a bit of a special case for logging (there's a whole debate regarding logging in general for koha going on at the moment). But, having said that, the bulkmarcimport tool already impliments it's own loggin system; An example of this can be seen aroud line: 404 (This should be the best example for your particular required followup ;) ).. to see how the logging itself works (just for interest) you could take a look at the routine itself found on lines 592 -> 595.

Cheers

Martin
Comment 14 Thomas Wright 2015-01-23 12:20:43 UTC Comment hidden (obsolete)
Comment 15 Thomas Wright 2015-01-23 12:21:58 UTC
Not sure if I did the logging correctly, but I had a go!
Comment 16 Magnus Enger 2015-03-05 16:33:01 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2015-03-16 00:47:29 UTC
I see the warn, but not the message in the log file. Leaving this for Martin to check again.
Comment 18 Martin Renvoize 2015-03-16 09:23:34 UTC
Looks good to me Katrin.. those errors will only appear in pretty specific cases.. your probably just not causing them?
Comment 19 Kyle M Hall 2015-03-20 13:57:44 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2015-03-20 14:19:16 UTC
Created attachment 37045 [details] [review]
[PASSED QA] Bug 13531 - bulkmarcimport bombs if no match is found Changed the die statment to a warn allowing the import to continue.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2015-03-20 14:19:23 UTC
Created attachment 37046 [details] [review]
[PASSED QA] Bug 13531: QA follow up

A minor QA comment.

::: misc/migration_tools/bulkmarcimport.pl
@@ +271,5 @@
>          my ( $error, $results, $totalhits ) = C4::Search::SimpleSearch( $query, 0, 3, [$server] );
> +        # changed to warn so able to continue with one broken record
> +        if ( defined $error ) {
> +            warn "unable to search the database for duplicates : $error";
> +            next;

For consistency with the rest of the script, should this perhaps be:

next RECORD;

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2015-03-20 14:19:25 UTC
Created attachment 37047 [details] [review]
[PASSED QA] Bug 13531 - Follow up

Add logging of errors.

Signed-off-by: Magnus Enger <magnus@enger.priv.no>
More errors are indeed showing up in the log.
(I took the liberty of changing the commit message a little bit.)

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Tomás Cohen Arazi 2015-03-31 14:04:55 UTC
Patches pushed to master.

Thanks Thomas!
Comment 24 Chris Cormack 2015-04-03 03:46:39 UTC
Pushed to 3.18.x will be in 3.18.6