Opinionated report, but I think new column names should match our latest practices. i.e. don't make it read: if ( $hold->item_level_hold ) { ... but if ( $hold->item_level ) { ...
Created attachment 90679 [details] [review] Bug 23133: DB update
Created attachment 90680 [details] [review] Bug 23133: Rename uses of item_level_hold
Created attachment 90681 [details] [review] Bug 23133: API cleanup
Ok, my two cents. I included the schema updates, let me know if you need it separate.
suspend in the same table is still BOOLEAN. If SQL12 is applied we should patch all existing occurrences, and have a check in the QA script. Is there a bug report already?
(In reply to Jonathan Druart from comment #5) > suspend in the same table is still BOOLEAN. BOOLEAN is just an alias for TINYINT(1), I think it is worth changing anyway unless... > If SQL12 is applied we should patch all existing occurrences, and have a > check in the QA script. Is there a bug report already? we have this kind of thing, and we actually prefer to add them as BOOLEAN in kohastructure.sql, because we have TINYINT(1) fields that are not really booleans (for example marc_subfield_structure.tab). There isn't a bug report yet (sorry for that), and it would be great to have the QA tools catch this problems. If BOOLEAN helps detect them, then lets change the rule and use BOOLEAN!
Moving boolean discussion to: https://gitlab.com/koha-community/qa-test-tools/issues/20
Sorry mate, but I can't apply this one.. error: sha1 information is lacking or useless (Koha/Schema/Result/Reserve.pm). error: could not build fake ancestor Patch failed at 0001 Bug 23133: DB update
That'll be the schema updates.. I'll work around it.
Created attachment 90857 [details] [review] Bug 23133: DB update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 90858 [details] [review] Bug 23133: Rename uses of item_level_hold Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 90859 [details] [review] Bug 23133: API cleanup Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I'm happy with the changes as they are here, but do agree it would be nice to see the BOOLEAN handling and QA handling of such cases dealt with.. lets get this one rolling whilst we discus and implement an approach to resolving those issues in the gitlab bug linked above and bug 21294 here.
Koha/Schema/Result/OldReserve.pm | 8 ++++---- Koha/Schema/Result/Reserve.pm | 8 ++++---- .../atomicupdate/bug_23133_item_level_holds.perl | 19 +++++++++++++++++++ installer/data/mysql/kohastructure.sql | 4 ++-- I would not recommend doing this in one patch. And thought that we even had a rule for that? But can't find it. DBIx schema files should be separate and marked as do_not_push, if the RM wants to generate them himself.
I had errors generating the updated schema with the is_boolean declaration. That's why I added it like this.
While this patches are correct and shouldn't be FQA, I'm marking as duplicate, as bug 25260 tackles it already. *** This bug has been marked as a duplicate of bug 25260 ***