Bug 28869 - Optionally restrict authorised values to tinyint
Summary: Optionally restrict authorised values to tinyint
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Staff interface (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Marcel de Rooy
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 35990
  Show dependency treegraph
 
Reported: 2021-08-16 22:26 UTC by Lucas Gass
Modified: 2024-03-27 13:20 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 28869: Database revision (1.93 KB, patch)
2024-02-05 09:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28869: DBIx schema changes for AuthorisedValueCategory (1.82 KB, patch)
2024-02-05 09:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28869: Implement authorised_value_categories.is_integer_only (14.74 KB, patch)
2024-02-05 09:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28869: (follow-up) Add html restriction for tinyint (2.00 KB, patch)
2024-02-05 09:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28869: Unit tests (3.78 KB, patch)
2024-02-05 09:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28869: Database revision (1.94 KB, patch)
2024-02-26 06:01 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28869: DBIx schema changes for AuthorisedValueCategory (1.84 KB, patch)
2024-02-26 06:01 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28869: Implement authorised_value_categories.is_integer_only (14.76 KB, patch)
2024-02-26 06:01 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28869: (follow-up) Add html restriction for tinyint (2.02 KB, patch)
2024-02-26 06:01 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28869: Unit tests (3.80 KB, patch)
2024-02-26 06:01 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 28869: Database revision (1.98 KB, patch)
2024-03-27 13:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28869: DBIx schema changes for AuthorisedValueCategory (1.88 KB, patch)
2024-03-27 13:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28869: Implement authorised_value_categories.is_integer_only (14.66 KB, patch)
2024-03-27 13:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28869: (follow-up) Add html restriction for tinyint (2.06 KB, patch)
2024-03-27 13:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28869: Unit tests (3.84 KB, patch)
2024-03-27 13:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 28869: (follow-up) Switch to cud-edit_category for CSRF (1.99 KB, patch)
2024-03-27 13:19 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass 2021-08-16 22:26:43 UTC
The A.V. category of STACK maps to items.stack. In the schema (for master) it claims items.stack but if I 'describe items' it tells me it is tinyint(1).


Tinyint can be anything between -128 and 127. 


To recreate:
-Add AV's for STACK that are greater than 127. 
-Edit an item and attempt to change the shelving control number to that value.
-See an error
-Add AV's less than 127
-Edit an item, things seem to work fine. 


Should items.stack be int?
Comment 1 Lucas Gass 2021-08-16 22:31:43 UTC
TYPO:

In the schema (for master) it claims items.stack is the data type BIT but if I 'describe items' it tells me it is tinyint(1).
Comment 2 Katrin Fischer 2023-07-01 13:11:53 UTC
Lucas, I am not sure what the use case for this item field usually is. We can easily increase it, but what should we use instead?
Comment 3 Marcel de Rooy 2023-12-07 08:45:20 UTC
(In reply to Lucas Gass from comment #0)

> The A.V. category of STACK maps to items.stack.

Somehow in our local production db I do have the AV STACK category (without any value under it) and no mappings to STACK in the frameworks!
We do not use the field but it contains a small number of 127s. Should have hidden the field in the item editor ;)

> Tinyint can be anything between -128 and 127.

MySQL defaults to signed. If we SHOULD alter it to UNSIGNED. We have 0 to 255. Which might be just a bit more useful in the library environment. See also below.

> -Edit an item, things seem to work fine. 

If you enter a value over 127, it is converted to 127.
This is not really fine imo :) It is just silently adjusted/capped.

> Should items.stack be int?

This actually is a harder question than it looks like.
One aspect is that the item editor just silently ignores non-numeric input in this field. So a text field would be nice?
But authorised_values.id is an INT. But "unfortunately", Koha does not save the id in the corresponding item field but the actual value which is varchar(80). (Using  a TEXT variant here would have performance impacts.)

In conclusion, for items.stack I would opt for varchar(80) too here. It does however trigger a few remarks having a wider scope (postponed to next comment).
Comment 4 Marcel de Rooy 2023-12-07 15:04:57 UTC
The promised comment is still coming btw.
Comment 5 Marcel de Rooy 2023-12-07 15:25:05 UTC
Just some related questions here.

Should we save authorised_values.id in connected fields? Perhaps theoretically, but it does not sound very practical at this point in our codebase. So NO.

Should we adjust other item fields that are of integer type connected to AV where the same problem actually may occur? Move them all to varchar(80)? It feels a bit like overkill?
These are:
| stack                             | tinyint(1)   |  # on this report
| notforloan                        | tinyint(1)   |
| damaged                           | tinyint(1)   |
| itemlost                          | tinyint(1)   |
| withdrawn                         | tinyint(1)   |
| restricted                        | tinyint(1)   |

IF doing so, we could look at default values. They are not completely consistent, since damaged and withdrawn default to 0.
`restricted` tinyint(1) DEFAULT NULL COMMENT 'authorized value defining use restrictions for this item (MARC21 952$5)',
`stack` tinyint(1) DEFAULT NULL,
`ccode` varchar(80) DEFAULT NULL COMMENT 'authorized value for the collection code associated with this item (MARC21 952$8)',
`location` varchar(80) DEFAULT NULL COMMENT 'authorized value for the shelving location for this item (MARC21 952$c)',

I thought about warning for a wrong value in an item field of type integer/date BEFORE saving. Now we can just enter something in various fields, save, get no warning and come back later to discover that our data is lost. E.g. the example of 127 for a higher number. (Coming back as 127 with a warning in the editor.) Or just 0 for a string.

But since this originates from AV. We should perhaps add a restriction THERE! Why not add an AV column that allows you to only enter tinyints instead of 80 char codes? If we solve it in AV, there will be no problem in items?
Comment 6 Marcel de Rooy 2023-12-07 15:26:56 UTC
Note that things only 'explode' in strict SQL mode probably. Otherwise they are just silently converted.
Comment 7 Katrin Fischer 2023-12-23 13:05:30 UTC
(In reply to Marcel de Rooy from comment #5)
> Just some related questions here.
> 
> Should we save authorised_values.id in connected fields? Perhaps
> theoretically, but it does not sound very practical at this point in our
> codebase. So NO.

Agreed. 
 
> Should we adjust other item fields that are of integer type connected to AV
> where the same problem actually may occur? Move them all to varchar(80)? It
> feels a bit like overkill?
> These are:
> | stack                             | tinyint(1)   |  # on this report
> | notforloan                        | tinyint(1)   |
> | damaged                           | tinyint(1)   |
> | itemlost                          | tinyint(1)   |
> | withdrawn                         | tinyint(1)   |
> | restricted                        | tinyint(1)   |
> 
> IF doing so, we could look at default values. They are not completely
> consistent, since damaged and withdrawn default to 0.
> `restricted` tinyint(1) DEFAULT NULL COMMENT 'authorized value defining use
> restrictions for this item (MARC21 952$5)',
> `stack` tinyint(1) DEFAULT NULL,
> `ccode` varchar(80) DEFAULT NULL COMMENT 'authorized value for the
> collection code associated with this item (MARC21 952$8)',
> `location` varchar(80) DEFAULT NULL COMMENT 'authorized value for the
> shelving location for this item (MARC21 952$c)',

Standardizing the default values would be nice, but I am very hesitant about changing the datatype on the listed fields to varchar.

My doubts are about the handling in our code and in reports. I know that we make numerical comparisons on these fields, like notforloan negative and positive values are treated differently within the codebase. Restricted = 1 has a special meaning. I would leave the status as is and only change stack, as it doesn't seem to belong in the list (not a status).

> I thought about warning for a wrong value in an item field of type
> integer/date BEFORE saving. Now we can just enter something in various
> fields, save, get no warning and come back later to discover that our data
> is lost. E.g. the example of 127 for a higher number. (Coming back as 127
> with a warning in the editor.) Or just 0 for a string.

I have helped a lot of people "fix" this problem over time, so better handling in the GUI would be desirable. Maybe instead of fixing the item editor etc. we should fix the authorised values editor instead.

> But since this originates from AV. We should perhaps add a restriction
> THERE! Why not add an AV column that allows you to only enter tinyints
> instead of 80 char codes? If we solve it in AV, there will be no problem in
> items?

Agreed :)
Comment 8 Marcel de Rooy 2024-02-05 09:46:49 UTC
Renaming title from
If authorized values for STACK (shelving control number) are > 127 things explode
to
Optionally restrict authorised values to tinyint
Comment 9 Marcel de Rooy 2024-02-05 09:51:15 UTC
Created attachment 161745 [details] [review]
Bug 28869: Database revision

Test plan:
Run updatedatabase.pl

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 10 Marcel de Rooy 2024-02-05 09:51:17 UTC
Created attachment 161746 [details] [review]
Bug 28869: DBIx schema changes for AuthorisedValueCategory

Note: Also adds boolean flag in the custom part!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2024-02-05 09:51:19 UTC
Created attachment 161747 [details] [review]
Bug 28869: Implement authorised_value_categories.is_integer_only

Test plan:
Add category with/without integer restriction.
Test adding/editing values.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2024-02-05 09:51:21 UTC
Created attachment 161748 [details] [review]
Bug 28869: (follow-up) Add html restriction for tinyint

Several item fields are signed tinyints, connected to
authorised value categories like NOT_LOAN etc. They only
accept -128 up to 127.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 13 Marcel de Rooy 2024-02-05 09:51:23 UTC
Created attachment 161749 [details] [review]
Bug 28869: Unit tests

Test plan:
Run t/db_dependent/AuthorisedValues.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2024-02-05 09:52:50 UTC
=== Note for QA
 WARN   Koha/Schema/Result/AuthorisedValueCategory.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 16, now: 18)
=> PLEASE IGNORE (from automated build)

 FAIL   Koha/AuthorisedValue.pm
   FAIL   valid
                Constant subroutine Koha::AuthorisedValue::NUM_PATTERN redefined
                Constant subroutine Koha::AuthorisedValue::NUM_PATTERN_JS redefined
=> PLEASE IGNORE (false failures for constants, related to module dependencies outside report scope). Note too that qa tools should say WARN here and not FAIL.
Comment 15 Marcel de Rooy 2024-02-05 09:54:12 UTC
For suggesting further improvements and extending this development, please go to the omnibus on bug 35990.
Comment 16 Victor Grousset/tuxayo 2024-02-26 06:01:32 UTC
Created attachment 162412 [details] [review]
Bug 28869: Database revision

Test plan:
Run updatedatabase.pl

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 17 Victor Grousset/tuxayo 2024-02-26 06:01:34 UTC
Created attachment 162413 [details] [review]
Bug 28869: DBIx schema changes for AuthorisedValueCategory

Note: Also adds boolean flag in the custom part!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 18 Victor Grousset/tuxayo 2024-02-26 06:01:37 UTC
Created attachment 162414 [details] [review]
Bug 28869: Implement authorised_value_categories.is_integer_only

Test plan:
Add category with/without integer restriction.
Test adding/editing values.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 19 Victor Grousset/tuxayo 2024-02-26 06:01:39 UTC
Created attachment 162415 [details] [review]
Bug 28869: (follow-up) Add html restriction for tinyint

Several item fields are signed tinyints, connected to
authorised value categories like NOT_LOAN etc. They only
accept -128 up to 127.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 20 Victor Grousset/tuxayo 2024-02-26 06:01:42 UTC
Created attachment 162416 [details] [review]
Bug 28869: Unit tests

Test plan:
Run t/db_dependent/AuthorisedValues.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 21 Victor Grousset/tuxayo 2024-02-26 06:01:56 UTC
It works :)
Comment 22 Marcel de Rooy 2024-03-27 13:19:43 UTC
Created attachment 163996 [details] [review]
Bug 28869: Database revision

Test plan:
Run updatedatabase.pl

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 23 Marcel de Rooy 2024-03-27 13:19:46 UTC
Created attachment 163997 [details] [review]
Bug 28869: DBIx schema changes for AuthorisedValueCategory

Note: Also adds boolean flag in the custom part!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 24 Marcel de Rooy 2024-03-27 13:19:48 UTC
Created attachment 163998 [details] [review]
Bug 28869: Implement authorised_value_categories.is_integer_only

Test plan:
Add category with/without integer restriction.
Test adding/editing values.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 25 Marcel de Rooy 2024-03-27 13:19:50 UTC
Created attachment 163999 [details] [review]
Bug 28869: (follow-up) Add html restriction for tinyint

Several item fields are signed tinyints, connected to
authorised value categories like NOT_LOAN etc. They only
accept -128 up to 127.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 26 Marcel de Rooy 2024-03-27 13:19:52 UTC
Created attachment 164000 [details] [review]
Bug 28869: Unit tests

Test plan:
Run t/db_dependent/AuthorisedValues.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 27 Marcel de Rooy 2024-03-27 13:19:55 UTC
Created attachment 164001 [details] [review]
Bug 28869: (follow-up) Switch to cud-edit_category for CSRF

No test plan.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2024-03-27 13:20:20 UTC
Rebased and added CSRF follow-up