Bug 19420

Summary: Improve display of errors from failure of uploading file during stage import
Product: Koha Reporter: Nick Clemens <nick>
Component: ToolsAssignee: Nick Clemens <nick>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, katrin.fischer, m.de.rooy
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19633
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 19420: Improve display of errors from failure of uploading file during stage import
Bug 19420 - Improve display of errors from failure of uploading file during stage import
Bug 19420 - Improve display of errors from failure of uploading file during stage import
Bug 19420: (QA Follow-up) Fix test, correct POD
Bug 19420 - Add errors on other pages
Bug 19420 - Add errors on other pages
Bug 19420: Improve display of errors from failure of uploading file during stage import
Bug 19420: (QA Follow-up) Fix test, correct POD
Bug 19420: (Follow-up) Add errors on other pages
Bug 19420: (QA Follow-up) Change in error reporting affects upload.pl

Description Nick Clemens 2017-10-06 10:03:25 UTC
Currently error is always:
Upload status: Failed
Though there are four reasons it could fail, they are codes buried in Koha/Uploader.pm
Comment 1 Nick Clemens 2017-10-06 10:06:24 UTC
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
Comment 2 Katrin Fischer 2017-10-07 12:34:23 UTC
I think it would be nicer to deal with the codes in the templates to have them translatable. What do you think?
Comment 3 Nick Clemens 2017-10-10 17:02:48 UTC
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
Comment 4 Nick Clemens 2017-10-19 10:43:34 UTC
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
Comment 5 Katrin Fischer 2017-10-22 08:52:11 UTC
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>
Comment 6 Marcel de Rooy 2017-11-03 09:10:05 UTC
(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 ?! ;)
Comment 7 Marcel de Rooy 2017-11-03 09:13:17 UTC
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.
Comment 8 Marcel de Rooy 2017-11-03 09:21:36 UTC
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.
Comment 9 Marcel de Rooy 2017-11-03 09:29:34 UTC
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
Comment 10 Nick Clemens 2017-11-03 10:41:23 UTC
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
Comment 11 Marcel de Rooy 2017-11-03 11:24:26 UTC
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.
Comment 12 Nick Clemens 2017-11-04 00:19:41 UTC
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
Comment 13 Nick Clemens 2017-11-04 00:21:44 UTC
(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
Comment 14 Marcel de Rooy 2017-11-06 09:48:16 UTC
Looking here again. I still found an issue. Please wait..
Comment 15 Marcel de Rooy 2017-11-06 10:39:27 UTC
(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.
Comment 16 Marcel de Rooy 2017-11-06 10:40:41 UTC
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>
Comment 17 Marcel de Rooy 2017-11-06 10:40:46 UTC
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>
Comment 18 Marcel de Rooy 2017-11-06 10:40:49 UTC
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
Comment 19 Marcel de Rooy 2017-11-06 10:40:58 UTC
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.
Comment 20 Marcel de Rooy 2017-11-06 10:42:36 UTC
(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
Comment 21 Jonathan Druart 2017-11-08 16:18:28 UTC
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.
Comment 22 Jonathan Druart 2017-11-08 16:51:01 UTC
Pushed to master for 17.11, thanks to everybody involved!