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?
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).
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?
(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).
The promised comment is still coming btw.
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?
Note that things only 'explode' in strict SQL mode probably. Otherwise they are just silently converted.
(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 :)
Renaming title from If authorized values for STACK (shelving control number) are > 127 things explode to Optionally restrict authorised values to tinyint
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>
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>
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>
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>
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>
=== 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.
For suggesting further improvements and extending this development, please go to the omnibus on bug 35990.
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>
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>
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>
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>
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>
It works :)
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>
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>
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>
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>
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>
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>
Rebased and added CSRF follow-up
Created attachment 166697 [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> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166698 [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> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166699 [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> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166700 [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> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166701 [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> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 166702 [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> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Can someone quickly check on these QA script failures for me please? FAIL Koha/AuthorisedValue.pm FAIL valid Constant subroutine Koha::AuthorisedValue::NUM_PATTERN redefined Constant subroutine Koha::AuthorisedValue::NUM_PATTERN_JS redefined
(In reply to Katrin Fischer from comment #35) > Can someone quickly check on these QA script failures for me please? > > FAIL Koha/AuthorisedValue.pm > FAIL valid > Constant subroutine Koha::AuthorisedValue::NUM_PATTERN redefined > Constant subroutine Koha::AuthorisedValue::NUM_PATTERN_JS redefined Comment14
Some notes: * Would it not have been enough to name it: is_integer? :) * The database update is missing the AFTER/BEFORE. I've added it.
(In reply to Katrin Fischer from comment #37) > * Would it not have been enough to name it: is_integer? :) Good question. No strong opinion. Since we normally allow string values, the only comes from that perspective. > * The database update is missing the AFTER/BEFORE. I've added it. Thanks.
Pushed for 24.05! Well done everyone, thank you!
Something appears missing here, tests are exploding. Can you please double check my pushed DBIC change and anything missing for REST API maybe? > Error: Properties not allowed: is_integer_only.
Created attachment 166906 [details] [review] Bug 28869: (RM follow-up) Add missing field in api definition We missed adding the new field to the api definition for authorised_value_category. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Follow-up pushed.
(In reply to Martin Renvoize from comment #42) > Follow-up pushed. Thank you!
A "datatype" column would have been more flexible here IMO.
Not backported to 23.11.x
Database change - no update required to the manual.