Bug 19633

Summary: Use alphanumeric error codes in upload
Product: Koha Reporter: Marcel de Rooy <m.de.rooy>
Component: Architecture, internals, and plumbingAssignee: Marcel de Rooy <m.de.rooy>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, martin.renvoize, nick
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19420
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14294
Whiteboard:
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 19633: Use alphanumeric error codes in upload
Bug 19633: Use alphanumeric error codes in upload
Bug 19633: Use alphanumeric error codes in upload
Bug 19633: (QA follow-up) Really remove these ugly numbers
Bug 19633: Use alphanumeric error codes in upload
Bug 19633: (QA follow-up) Really remove these ugly numbers

Description Marcel de Rooy 2017-11-16 10:42:58 UTC
See comment 21 on bug 19420
Comment 1 Marcel de Rooy 2018-02-22 11:51:02 UTC
Created attachment 72084 [details] [review]
Bug 19633: Use alphanumeric error codes in upload

The error codes 1 to 7 are used in Uploader.pm or tools/upload.pl.
It would be nice to use alphanumeric codes instead.
No behavior change expected.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Verify that a regular upload with tools/upload.pl still works.
[3] Rename upload_path in your koha-conf.xml. Restart Plack, flush the cache
    and try to upload to a category. Correct error message?
[4] Upload the same file twice to the same category.
    Correct error message the second time?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 2 Brendan Gallagher 2018-03-15 10:08:02 UTC
Created attachment 72919 [details] [review]
Bug 19633: Use alphanumeric error codes in upload

The error codes 1 to 7 are used in Uploader.pm or tools/upload.pl.
It would be nice to use alphanumeric codes instead.
No behavior change expected.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Verify that a regular upload with tools/upload.pl still works.
[3] Rename upload_path in your koha-conf.xml. Restart Plack, flush the cache
    and try to upload to a category. Correct error message?
[4] Upload the same file twice to the same category.
    Correct error message the second time?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 3 Marcel de Rooy 2018-04-23 13:13:39 UTC
Created attachment 74722 [details] [review]
Bug 19633: Use alphanumeric error codes in upload

The error codes 1 to 7 are used in Uploader.pm or tools/upload.pl.
It would be nice to use alphanumeric codes instead.
No behavior change expected.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Verify that a regular upload with tools/upload.pl still works.
[3] Rename upload_path in your koha-conf.xml. Restart Plack, flush the cache
    and try to upload to a category. Correct error message?
[4] Upload the same file twice to the same category.
    Correct error message the second time?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>
Comment 4 Marcel de Rooy 2018-04-23 13:14:04 UTC
Rebased in connection with [% Asset.js("js/file-upload.js") %]
Comment 5 Jonathan Druart 2018-05-30 20:35:33 UTC
Ok but I would have written

  use constant UPLERR_ALREADY_EXISTS => 'UPLERR_ALREADY_EXISTS';
instead of
  use constant ERRCODE_1    => 'UPLERR_ALREADY_EXISTS';

or even get rid of the constant if only used once.

The goal is to make the code more readable, but when you see ERRCODE_X in the perl script you still do not know what it means :)
Comment 6 Marcel de Rooy 2018-05-31 06:40:55 UTC
(In reply to Jonathan Druart from comment #5)
> Ok but I would have written
> 
>   use constant UPLERR_ALREADY_EXISTS => 'UPLERR_ALREADY_EXISTS';
> instead of
>   use constant ERRCODE_1    => 'UPLERR_ALREADY_EXISTS';
> 
> or even get rid of the constant if only used once.
> 
> The goal is to make the code more readable, but when you see ERRCODE_X in
> the perl script you still do not know what it means :)

We still can do that later. This was kind of a safety step in between. Still knowing the old error number..
Comment 7 Marcel de Rooy 2018-06-06 13:54:22 UTC
Created attachment 75856 [details] [review]
Bug 19633: (QA follow-up) Really remove these ugly numbers

See BZ comment5. We now remove the numbers also from the constant names.

Test plan:
Read the changes.
Git grep "ERRCODE_"
Run t/db_dependent/Upload.t (Note: You may see one failure here; it is fixed
on bug 20727. So depends on who reaches master first.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2018-06-06 13:55:22 UTC
(In reply to Jonathan Druart from comment #5)
> Ok but I would have written
> 
>   use constant UPLERR_ALREADY_EXISTS => 'UPLERR_ALREADY_EXISTS';
> instead of
>   use constant ERRCODE_1    => 'UPLERR_ALREADY_EXISTS';
> 
> or even get rid of the constant if only used once.
> 
> The goal is to make the code more readable, but when you see ERRCODE_X in
> the perl script you still do not know what it means :)

Giving in for high pressure
Comment 9 Jonathan Druart 2018-07-20 16:38:57 UTC
Created attachment 77172 [details] [review]
Bug 19633: Use alphanumeric error codes in upload

The error codes 1 to 7 are used in Uploader.pm or tools/upload.pl.
It would be nice to use alphanumeric codes instead.
No behavior change expected.

Test plan:
[1] Run t/db_dependent/Upload.t
[2] Verify that a regular upload with tools/upload.pl still works.
[3] Rename upload_path in your koha-conf.xml. Restart Plack, flush the cache
    and try to upload to a category. Correct error message?
[4] Upload the same file twice to the same category.
    Correct error message the second time?

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Brendan Gallagher <brendan@bywatersolutions.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 10 Jonathan Druart 2018-07-20 16:39:00 UTC
Created attachment 77173 [details] [review]
Bug 19633: (QA follow-up) Really remove these ugly numbers

See BZ comment5. We now remove the numbers also from the constant names.

Test plan:
Read the changes.
Git grep "ERRCODE_"
Run t/db_dependent/Upload.t (Note: You may see one failure here; it is fixed
on bug 20727. So depends on who reaches master first.)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 11 Nick Clemens 2018-08-10 12:56:02 UTC
Awesome work all!

Pushged to master for 18.11
Comment 12 Martin Renvoize 2018-08-10 13:29:09 UTC
Enhancement, won't be backported to 18.05.x series.