Bug 21294

Summary: Add identification of boolean fields in the database
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: 1joynelson, jonathan.druart, katrin.fischer, martin.renvoize, pasi.kallinen, tomascohen
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=23133
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=25152
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.05.00
Attachments: Bug 21294: Replace BOOLEAN with TINYINT(1) in our DB structure
Bug 21294: Replace BOOLEAN with TINYINT(1) in our DB structure

Description Tomás Cohen Arazi 2018-08-30 17:33:18 UTC
The coding guidelines require a specific handling for boolean attributes [1]. QA should require this for new devs, but the current DB structure lacks this information in many places.

We should add this metadata on the current schema files.

[1] https://wiki.koha-community.org/wiki/Coding_Guidelines#SQL12:_Booleans
Comment 1 Martin Renvoize 2019-06-20 15:53:32 UTC
Adopting this bug as we should really be moving all cases of 'BOOLEAN' to 'TINYINT(1)' and ensuring the class files match as expected.

Also, we've suggested a QA script enhancement on https://gitlab.com/koha-community/qa-test-tools/issues/20 to catch such cases.
Comment 2 Jonathan Druart 2019-12-17 10:42:42 UTC
*** Bug 24216 has been marked as a duplicate of this bug. ***
Comment 3 Katrin Fischer 2020-01-03 06:46:45 UTC
Wondering if locating candidates for Booleans could be a good Academy/beginners task.
Comment 4 Jonathan Druart 2020-04-15 14:20:26 UTC
Created attachment 103013 [details] [review]
Bug 21294: Replace BOOLEAN with TINYINT(1) in our DB structure

To match the coding guideline SQL12.
We do this for consistency.
Note that we do not need an update DB as BOOLEAN is an alias for
TINYINT(1).

The is_boolean flags have been added to the schema.

Test plan:
1. `dbic` should run correctly
2. kohastructure.sql should be syntaxicaly correct
3. `git grep -i boolean installer/data/mysql/kohastructure.sql`
should not return relevant occurrences.
Comment 5 Tomás Cohen Arazi 2020-04-15 21:13:40 UTC
Should we also add the is_boolean entry to the already existing TINYINT(1)? I would do it on a as-needed basis. As-needed would imply accepting (in QA) a commit that does it, on bugs that require it. For example, if we suddenly want to embed something that is missing that in a totally different place (e.g. $order->suggestions and Koha::Schema::Result::Suggestion missing is_boolean on an attribute) we would accept that.
Comment 6 Martin Renvoize 2020-04-16 06:05:37 UTC
I'd be happy to do it codebase wide at this point.. seems a good idea to try and catch them all
Comment 7 Tomás Cohen Arazi 2020-04-16 11:16:50 UTC
(In reply to Martin Renvoize from comment #6)
> I'd be happy to do it codebase wide at this point.. seems a good idea to try
> and catch them all

Ok!
Comment 8 Jonathan Druart 2020-04-16 13:22:22 UTC
IMO that's for a follow-up bug report.

Here we cleaned the DB structure, I can remove the schema changes (is_boolean) if you think it belongs to another bug report.
Comment 9 Tomás Cohen Arazi 2020-04-17 13:48:56 UTC
Created attachment 103165 [details] [review]
Bug 21294: Replace BOOLEAN with TINYINT(1) in our DB structure

To match the coding guideline SQL12.
We do this for consistency.
Note that we do not need an update DB as BOOLEAN is an alias for
TINYINT(1).

The is_boolean flags have been added to the schema.

Test plan:
1. `dbic` should run correctly
2. kohastructure.sql should be syntaxicaly correct
3. `git grep -i boolean installer/data/mysql/kohastructure.sql`
should not return relevant occurrences.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 10 Martin Renvoize 2020-04-17 15:14:30 UTC
(In reply to Jonathan Druart from comment #8)
> IMO that's for a follow-up bug report.
> 
> Here we cleaned the DB structure, I can remove the schema changes
> (is_boolean) if you think it belongs to another bug report.

It's looking good to me and I think it brings a real improvement to our consistency.. I'd be inclined to keep the patch as is.
Comment 11 Tomás Cohen Arazi 2020-04-17 16:52:27 UTC
Yeah
Comment 12 Jonathan Druart 2020-04-24 10:06:45 UTC
(In reply to Tomás Cohen Arazi from comment #11)
> Yeah

Is it a PQA stamp? :)
Comment 13 Martin Renvoize 2020-04-29 16:35:41 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 14 Joy Nelson 2020-05-08 22:27:18 UTC
enhancement not backported to 19.11