Bug 36687 - itemtypes.notforloan should be tinyint and NOT NULL
Summary: itemtypes.notforloan should be tinyint and NOT NULL
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Lucas Gass
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-04-24 16:50 UTC by Lucas Gass
Modified: 2024-05-28 21:48 UTC (History)
4 users (show)

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


Attachments
Bug 36687: Update itemtpes.notforloan to not allow NULL values and change to tinyint(1) (2.37 KB, patch)
2024-04-24 17:13 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 36687: Schema changes (1.55 KB, patch)
2024-04-24 17:13 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 36687: Update itemtypes.notforloan to not allow NULL values and change to tinyint(1) (2.97 KB, patch)
2024-04-29 22:54 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 36687: Schema updates (1.55 KB, patch)
2024-04-29 23:04 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 36687: Update itemtypes.notforloan to not allow NULL values and change to tinyint(1) (3.05 KB, patch)
2024-04-30 10:50 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 36687: Schema updates (1.63 KB, patch)
2024-04-30 10:50 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
The place it can be set (23.63 KB, image/png)
2024-05-07 11:11 UTC, Tomás Cohen Arazi
Details
Bug 36687: Update itemtypes.notforloan to not allow NULL values and change to tinyint(1) (3.09 KB, patch)
2024-05-08 13:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[DO NOT PUSH] Bug 36687: Schema updates (1.68 KB, patch)
2024-05-08 13:18 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 36687: (RM follow-up) Fix unit tests (10.62 KB, patch)
2024-05-09 08:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 36687: (RM follow-up) Fix more unit tests (1.38 KB, patch)
2024-05-09 08:11 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass 2024-04-24 16:50:03 UTC
kohastructure says itemtypes.notforloan is "1 if the item is not for loan, 0 if the item is available for loan". It is also defined as smallint(6).

Unlike items.notforloan it cannot be any other value than 0 or 1. 

I think this should be NOT NULL DEFAULT 0 and tinyint(1).
Comment 1 Lucas Gass 2024-04-24 17:13:38 UTC
Created attachment 165483 [details] [review]
Bug 36687: Update itemtpes.notforloan to not allow NULL values and change to tinyint(1)
Comment 2 Lucas Gass 2024-04-24 17:13:39 UTC
Created attachment 165484 [details] [review]
Bug 36687: Schema changes
Comment 3 Tomás Cohen Arazi 2024-04-26 13:08:38 UTC
Hi, I think you need to first set any NULL values to 0 before the ALTER TABLE.

I got this error when manually setting some itemtypes.notforloan values to NULL before running your DBRev.

```
ERROR - {UNKNOWN}: DBI Exception: DBD::mysql::db do failed: Data truncated for column 'notforloan' at row 1 at /kohadevbox/koha/C4/Installer.pm line 741
```

Also, I think the is_boolean flag should be added to the schema file.
Comment 4 Lucas Gass 2024-04-29 22:54:57 UTC
Created attachment 165819 [details] [review]
Bug 36687: Update itemtypes.notforloan to not allow NULL values and change to tinyint(1)

To test:
1. Have some itemtypes.notforloan values set to NULL.
2. APPLY PACTH, updatedatabase, restart_all
3. When you updatedatabase those values should be set to 0.
4. Try updating an itemtypes.notforloan value to NULL. You cannot.
Comment 5 Lucas Gass 2024-04-29 23:04:07 UTC
Created attachment 165820 [details] [review]
Bug 36687: Schema updates
Comment 6 Lucas Gass 2024-04-29 23:05:50 UTC
> Also, I think the is_boolean flag should be added to the schema file.
Is it not already there? 

343     '+notforloan'                   => { is_boolean => 1 },
Comment 7 Matt Blenkinsop 2024-04-30 10:50:17 UTC
Created attachment 165840 [details] [review]
Bug 36687: Update itemtypes.notforloan to not allow NULL values and change to tinyint(1)

To test:
1. Have some itemtypes.notforloan values set to NULL.
2. APPLY PACTH, updatedatabase, restart_all
3. When you updatedatabase those values should be set to 0.
4. Try updating an itemtypes.notforloan value to NULL. You cannot.

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 8 Matt Blenkinsop 2024-04-30 10:50:19 UTC
Created attachment 165841 [details] [review]
Bug 36687: Schema updates

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Comment 9 Marcel de Rooy 2024-05-07 10:10:14 UTC
(In reply to Lucas Gass from comment #0)
> kohastructure says itemtypes.notforloan is "1 if the item is not for loan, 0
> if the item is available for loan". It is also defined as smallint(6).
> 
> Unlike items.notforloan it cannot be any other value than 0 or 1. 
> 
> I think this should be NOT NULL DEFAULT 0 and tinyint(1).

Seems wrong to me. We have several notforloan values. Not only 0 and 1.
Comment 10 Tomás Cohen Arazi 2024-05-07 11:10:46 UTC
(In reply to Marcel de Rooy from comment #9)
> (In reply to Lucas Gass from comment #0)
> > kohastructure says itemtypes.notforloan is "1 if the item is not for loan, 0
> > if the item is available for loan". It is also defined as smallint(6).
> > 
> > Unlike items.notforloan it cannot be any other value than 0 or 1. 
> > 
> > I think this should be NOT NULL DEFAULT 0 and tinyint(1).
> 
> Seems wrong to me. We have several notforloan values. Not only 0 and 1.

This is itemtypes.notforloan. Which can only be set to 0 or 1 in the UI.
See the attachment I'm uploading.
Comment 11 Tomás Cohen Arazi 2024-05-07 11:11:10 UTC
Created attachment 166257 [details]
The place it can be set
Comment 12 Marcel de Rooy 2024-05-07 11:12:26 UTC
(In reply to Tomás Cohen Arazi from comment #10)
> (In reply to Marcel de Rooy from comment #9)
> > (In reply to Lucas Gass from comment #0)
> > > kohastructure says itemtypes.notforloan is "1 if the item is not for loan, 0
> > > if the item is available for loan". It is also defined as smallint(6).
> > > 
> > > Unlike items.notforloan it cannot be any other value than 0 or 1. 
> > > 
> > > I think this should be NOT NULL DEFAULT 0 and tinyint(1).
> > 
> > Seems wrong to me. We have several notforloan values. Not only 0 and 1.
> 
> This is itemtypes.notforloan. Which can only be set to 0 or 1 in the UI.
> See the attachment I'm uploading.

Ok Where are my glasses :)
Comment 13 Marcel de Rooy 2024-05-07 11:13:01 UTC
(In reply to Marcel de Rooy from comment #12)
> (In reply to Tomás Cohen Arazi from comment #10)
> > (In reply to Marcel de Rooy from comment #9)
> > > (In reply to Lucas Gass from comment #0)
> > > > kohastructure says itemtypes.notforloan is "1 if the item is not for loan, 0
> > > > if the item is available for loan". It is also defined as smallint(6).
> > > > 
> > > > Unlike items.notforloan it cannot be any other value than 0 or 1. 
> > > > 
> > > > I think this should be NOT NULL DEFAULT 0 and tinyint(1).
> > > 
> > > Seems wrong to me. We have several notforloan values. Not only 0 and 1.
> > 
> > This is itemtypes.notforloan. Which can only be set to 0 or 1 in the UI.
> > See the attachment I'm uploading.
> 
> Ok Where are my glasses :)

Ah the description mentioned items..
Comment 14 Tomás Cohen Arazi 2024-05-08 13:18:57 UTC
Created attachment 166413 [details] [review]
Bug 36687: Update itemtypes.notforloan to not allow NULL values and change to tinyint(1)

To test:
1. Have some itemtypes.notforloan values set to NULL.
2. APPLY PACTH, updatedatabase, restart_all
3. When you updatedatabase those values should be set to 0.
4. Try updating an itemtypes.notforloan value to NULL. You cannot.

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2024-05-08 13:18:59 UTC
Created attachment 166414 [details] [review]
[DO NOT PUSH] Bug 36687: Schema updates

Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Lucas Gass 2024-05-08 14:05:47 UTC
Thanks Tomas.
Comment 17 Katrin Fischer 2024-05-08 16:25:14 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 18 Martin Renvoize 2024-05-09 08:11:51 UTC
Created attachment 166447 [details] [review]
Bug 36687: (RM follow-up) Fix unit tests

Set notforloan to '0' as apposed to 'undef' for tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 19 Martin Renvoize 2024-05-09 08:11:54 UTC
Created attachment 166448 [details] [review]
Bug 36687: (RM follow-up) Fix more unit tests

Use currency.active to test `int(1) DEFAULT NULL` now that itemtypes.notforloan
is `NOT NULL DEFAULT '0'`

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2024-05-09 08:24:38 UTC
Follow-ups Pushed as RM backup.
Comment 21 Fridolin Somers 2024-05-24 14:56:32 UTC
Pushed to 23.11.x for 23.11.06
Comment 22 Lucas Gass 2024-05-28 21:48:54 UTC
Enhancement will not be backported to 23.05.x