Currently error is always: Upload status: Failed Though there are four reasons it could fail, they are codes buried in Koha/Uploader.pm
Created attachment 67685 [details] [review] Bug 19420: Improve display of errors from failure of uploading file during stage import To test: 1 - sudo chown www-data:www-data /tmp/koha_kohadev_upload/ (or your relevant dir and user) 2 - Attempt to stage a file 3 - Error message is: Upload status: Failed 4 - Apply patch 5 - Repeat upload 6 - Error is now: Failed - Directory is not writeable: /tmp/koha_kohadev_upload 7 - Trigger the other errors if you can and note better responses
I think it would be nicer to deal with the codes in the templates to have them translatable. What do you think?
That's fair, I can look into that, this was mostly just in response to having to chase down that code, may take a while before I get back here
Created attachment 68292 [details] [review] Bug 19420 - Improve display of errors from failure of uploading file during stage import To test: 1 - sudo chown www-data:www-data /tmp/koha_kohadev_upload/ (or your relevant dir and user) 2 - Attempt to stage a file 3 - Error message is: Upload status: Failed 4 - Apply patch 5 - Repeat upload 6 - Error is now: Failed - Directory is not writeable: /tmp/koha_kohadev_upload 7 - Trigger the other errors if you can and note better responses
Created attachment 68365 [details] [review] Bug 19420 - Improve display of errors from failure of uploading file during stage import To test: 1 - sudo chown www-data:www-data /tmp/koha_kohadev_upload/ (or your relevant dir and user) 2 - Attempt to stage a file 3 - Error message is: Upload status: Failed 4 - Apply patch 5 - Repeat upload 6 - Error is now: Failed - Directory is not writeable: /tmp/koha_kohadev_upload 7 - Trigger the other errors if you can and note better responses Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
(In reply to Nick Clemens from comment #0) > Currently error is always: > Upload status: Failed > Though there are four reasons it could fail, they are codes buried in > Koha/Uploader.pm Who wrote that ?! ;)
t/db_dependent/Upload.t not ok 3 - Errcode 1 [already exists] reported # Failed test 'Errcode 1 [already exists] reported' # at t/db_dependent/Upload.t line 148. # got: 'HASH(0x7071fe8)' # expected: '1' # Looks like you failed 1 test of 3. not ok 6 - Add same file in same category # Failed test 'Add same file in same category' # at t/db_dependent/Upload.t line 149. Will have a look now.
Created attachment 68909 [details] [review] Bug 19420: (QA Follow-up) Fix test, correct POD The test needs the code level in the error hash now. And POD is adjusted accordingly.
We need similar changes in other places: git grep "file-upload.js" koha-tmpl/intranet-tmpl/prog/en/modules/offline_circ/process_koc.tt:<script type="text/javascript" src="[% interface %]/[% theme %]/js/file-upload.js"></script> koha-tmpl/intranet-tmpl/prog/en/modules/tools/stage-marc-import.tt:<script type="text/javascript" src="[% interface %]/[% theme %]/js/file-upload.js"></script> koha-tmpl/intranet-tmpl/prog/en/modules/tools/upload-images.tt:<script type="text/javascript" src="[% interface %]/[% theme %]/js/file-upload.js"></script> koha-tmpl/intranet-tmpl/prog/en/modules/tools/upload.tt:<script type="text/javascript" src="[% interface %]/[% theme %]/js/file-upload.js"></script> Please adjust
Created attachment 68910 [details] [review] Bug 19420 - Add errors on other pages Eventually these should all be using the same code in a js file upload.tt already dealt with these errors, but has diff code, made it work with new error syntax
Upload local cover image Upload status: Failed - (2) Root directory for uploads not defined No, the directory was not writable. This went wrong since the errcode is 2, but you forgot to add a zero entry in you errMsgs array. So they are one position out of sync. Please add a leading entry here (see also upload.tt) var errMsgs = [ _("File already exists"), _("Directory is not writeable"), _("Root directory for uploads not defined"), _("Temporary directory for uploads not defined") ]; Adjust same thing in two other places ? //Next three errors have no code path to trigger that I see I am very confident that I tested all codes when I wrote this. They may be hard to simulate, but I did. Please remove this comment.
Created attachment 68941 [details] [review] Bug 19420 - Add errors on other pages Fix error array (add '0' value) Update errors on other places using upload.js Eventually these should all be using the same code in a js file upload.tt already dealt with these errors, but has diff code, made it work with new error syntax
(In reply to Marcel de Rooy from comment #11) > Upload local cover image > Upload status: Failed - (2) Root directory for uploads not defined > No, the directory was not writable. > This went wrong since the errcode is 2, but you forgot to add a zero entry > in you errMsgs array. So they are one position out of sync. > Please add a leading entry here (see also upload.tt) > var errMsgs = [ _("File already exists"), _("Directory is not writeable"), > _("Root directory for uploads not defined"), _("Temporary directory for > uploads not defined") ]; > Adjust same thing in two other places ? Adjusted :-) > > //Next three errors have no code path to trigger that I see > I am very confident that I tested all codes when I wrote this. They may be > hard to simulate, but I did. Please remove this comment. Removed, I though we only passed 1-4 back in the pm file, my apologies
Looking here again. I still found an issue. Please wait..
(In reply to Nick Clemens from comment #13) > Removed, I though we only passed 1-4 back in the pm file, my apologies Ah. See the last follow-up. upload.pl passed these codes. As you already said, this still needs some improvement. I crossed a TODO also.
Created attachment 68960 [details] [review] Bug 19420: Improve display of errors from failure of uploading file during stage import To test: 1 - sudo chown www-data:www-data /tmp/koha_kohadev_upload/ (or your relevant dir and user) 2 - Attempt to stage a file 3 - Error message is: Upload status: Failed 4 - Apply patch 5 - Repeat upload 6 - Error is now: Failed - Directory is not writeable: /tmp/koha_kohadev_upload 7 - Trigger the other errors if you can and note better responses Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 68961 [details] [review] Bug 19420: (QA Follow-up) Fix test, correct POD The test needs the code level in the error hash now. And POD is adjusted accordingly. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 68962 [details] [review] Bug 19420: (Follow-up) Add errors on other pages Fix error array (add '0' value) Update errors on other places using upload.js Eventually these should all be using the same code in a js file upload.tt already dealt with these errors, but has diff code, made it work with new error syntax Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Amended: Removed the added js comment in upload.tt
Created attachment 68963 [details] [review] Bug 19420: (QA Follow-up) Change in error reporting affects upload.pl The change from the first patch requires a similar change in upload.pl when a few error codes are passed to the template. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Tested by deleting a temporary file manually and downloading or deleting the file from the upload search results.
(In reply to Nick Clemens from comment #13) > (In reply to Marcel de Rooy from comment #11) > > Upload local cover image > > Upload status: Failed - (2) Root directory for uploads not defined > > No, the directory was not writable. > > This went wrong since the errcode is 2, but you forgot to add a zero entry > > in you errMsgs array. So they are one position out of sync. > > Please add a leading entry here (see also upload.tt) > > var errMsgs = [ _("File already exists"), _("Directory is not writeable"), > > _("Root directory for uploads not defined"), _("Temporary directory for > > uploads not defined") ]; > > Adjust same thing in two other places ? > Adjusted :-) Works now. Thanks
errcode should be "already_exists", "no_tmp_dir", etc. No 0, 1, 2, ... This is a terrible and error prone pattern. Not blocker as it existed before this patch set.
Pushed to master for 17.11, thanks to everybody involved!