|
Description
Martin Renvoize (ashimema)
2015-01-07 13:39:30 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 I'll work on this. Created attachment 35133 [details] [review] Bug 13531 - bulkmarcimport bombs if no match is found Changed the die statment to a warn allowing the import to continue. Created 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. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Thank Thomas, caught the error nicely and good clean handling :) Signing Off 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; Created attachment 35260 [details] [review] 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; Created attachment 35295 [details] [review] [SIGNED OFF] 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> Awesome speedy followup there Thomas.. thanks :) Martin, shouldn't we call the printlog subroutine too? (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 (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 (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 Created attachment 35486 [details] [review] Follow up Not sure if I did the logging correctly, but I had a go! Created attachment 36625 [details] [review] [SIGNED OFF] 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.) I see the warn, but not the message in the log file. Leaving this for Martin to check again. Looks good to me Katrin.. those errors will only appear in pretty specific cases.. your probably just not causing them? Created attachment 37044 [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> 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> 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> 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> Patches pushed to master. Thanks Thomas! Pushed to 3.18.x will be in 3.18.6 |