Bug 19633 - Use alphanumeric error codes in upload
Summary: Use alphanumeric error codes in upload
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-11-16 10:42 UTC by Marcel de Rooy
Modified: 2019-10-14 19:57 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19633: Use alphanumeric error codes in upload (7.81 KB, patch)
2018-02-22 11:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19633: Use alphanumeric error codes in upload (7.84 KB, patch)
2018-03-15 10:08 UTC, Brendan Gallagher
Details | Diff | Splinter Review
Bug 19633: Use alphanumeric error codes in upload (7.73 KB, patch)
2018-04-23 13:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19633: (QA follow-up) Really remove these ugly numbers (4.90 KB, patch)
2018-06-06 13:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19633: Use alphanumeric error codes in upload (7.78 KB, patch)
2018-07-20 16:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19633: (QA follow-up) Really remove these ugly numbers (4.96 KB, patch)
2018-07-20 16:39 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.