Bug 11917

Summary: upload patron image can raise a software error
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: ToolsAssignee: Galen Charlton <gmcharlt>
Status: CLOSED INVALID QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: m.de.rooy
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 11917: upload image - catch error if cardnumber does not exist.
Bug 11917: upload image - catch error if cardnumber does not exist.
[SIGNED-OFF] Bug 11917: upload image - catch error if cardnumber does not exist.

Description Jonathan Druart 2014-03-10 13:24:35 UTC

    
Comment 1 Jonathan Druart 2014-03-10 13:25:49 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2014-03-10 17:16:51 UTC
I'm not sure if I'm doing something wrong, but I get the same results before and after the patch. I see a warning box that says "Patron image failed to upload." In the table of results I see "ERROR: Image not imported because the database returned an error. Please refer to the error log for more details."

I'm not sure what this patch is supposed to change, but I wonder if we able to make the error message specific? Something about the card number not being found?
Comment 3 Jonathan Druart 2014-03-11 08:13:36 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2014-03-11 08:15:17 UTC
It appends when DEBUG=0.
Comment 5 Owen Leonard 2014-03-11 15:15:42 UTC
Created attachment 26117 [details] [review]
[SIGNED-OFF] Bug 11917: upload image - catch error if cardnumber does not exist.

On uploading a patron image, if the given cardnumber does not exist,
a software error is raised

DBD::mysql::st execute failed: Column 'borrowernumber' cannot be null at
/home/koha/src/C4/Members.pm line 1916.

Test plan:
SET DEBUG=1 in your apache configuration
go on tools/picture-upload.pl, choose an image and put a cardnumber
which does not exist in DB.
With the patch, you should get a friendly error message.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 6 Marcel de Rooy 2014-03-13 13:18:37 UTC
QA Comment:
I appreciate the idea behind the patch, but do not really see the actual benefits of this code. Without DEBUG=1 (as we assume in normal operation), I do not see any difference.
I think that the routine in Members, PutPatronImage, should be addressed in this report. It combines the insert into patronimage with the select on cardnumber. It could check cardnumber first.
Note that if I see "There is a database error. See the log", I would rather see the error message right away. And the message is passed back by PutPatronImage.

Failed QA
Comment 7 Jonathan Druart 2014-03-13 13:30:32 UTC
(In reply to M. de Rooy from comment #6)

I opened this bug qaing another one. I didn't know that the issue only appeared for DEBUG=1.
I mark it as invalid, feel free to reopen it if you think it is useful.