Bug 23133

Summary: Rename reserves.item_level_hold for reserves.item_level
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Architecture, internals, and plumbingAssignee: Tomás Cohen Arazi <tomascohen>
Status: RESOLVED DUPLICATE QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: jonathan.druart, m.de.rooy, martin.renvoize
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21294
Change sponsored?: --- Patch complexity: ---
Who signed the patch off: Text to go in the release notes:
Version(s) released in:
Bug Depends on: 9834    
Bug Blocks:    
Attachments: Bug 23133: DB update
Bug 23133: Rename uses of item_level_hold
Bug 23133: API cleanup
Bug 23133: DB update
Bug 23133: Rename uses of item_level_hold
Bug 23133: API cleanup

Description Tomás Cohen Arazi 2019-06-17 15:22:23 UTC
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 ) { ...
  if ( $hold->item_level ) { ...
Comment 1 Tomás Cohen Arazi 2019-06-17 15:50:01 UTC
Created attachment 90679 [details] [review]
Bug 23133: DB update
Comment 2 Tomás Cohen Arazi 2019-06-17 15:50:04 UTC
Created attachment 90680 [details] [review]
Bug 23133: Rename uses of item_level_hold
Comment 3 Tomás Cohen Arazi 2019-06-17 15:50:08 UTC
Created attachment 90681 [details] [review]
Bug 23133: API cleanup
Comment 4 Tomás Cohen Arazi 2019-06-17 15:51:37 UTC
Ok, my two cents. I included the schema updates, let me know if you need it separate.
Comment 5 Jonathan Druart 2019-06-19 03:06:50 UTC
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?
Comment 6 Tomás Cohen Arazi 2019-06-19 15:42:58 UTC
(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!
Comment 7 Martin Renvoize 2019-06-20 15:47:57 UTC
Moving boolean discussion to: https://gitlab.com/koha-community/qa-test-tools/issues/20
Comment 8 Martin Renvoize 2019-06-20 15:50:53 UTC
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
Comment 9 Martin Renvoize 2019-06-20 15:55:19 UTC
That'll be the schema updates.. I'll work around it.
Comment 10 Martin Renvoize 2019-06-20 16:14:54 UTC
Created attachment 90857 [details] [review]
Bug 23133: DB update

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2019-06-20 16:14:57 UTC
Created attachment 90858 [details] [review]
Bug 23133: Rename uses of item_level_hold

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2019-06-20 16:15:00 UTC
Created attachment 90859 [details] [review]
Bug 23133: API cleanup

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2019-06-20 16:16:48 UTC
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.
Comment 14 Marcel de Rooy 2019-06-21 06:18:56 UTC
 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.
Comment 15 Tomás Cohen Arazi 2019-06-21 12:37:10 UTC
I had errors generating the updated schema with the is_boolean declaration. That's why I added it like this.
Comment 16 Tomás Cohen Arazi 2020-05-11 11:49:06 UTC
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 ***