Bug 23133 - Rename reserves.item_level_hold for reserves.item_level
Summary: Rename reserves.item_level_hold for reserves.item_level
Status: RESOLVED DUPLICATE of bug 25260
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on: 9834
Blocks:
  Show dependency treegraph
 
Reported: 2019-06-17 15:22 UTC by Tomás Cohen Arazi
Modified: 2020-05-11 11:49 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 23133: DB update (5.45 KB, patch)
2019-06-17 15:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23133: Rename uses of item_level_hold (3.41 KB, patch)
2019-06-17 15:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23133: API cleanup (878 bytes, patch)
2019-06-17 15:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23133: DB update (5.14 KB, patch)
2019-06-20 16:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23133: Rename uses of item_level_hold (3.47 KB, patch)
2019-06-20 16:14 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23133: API cleanup (943 bytes, patch)
2019-06-20 16:15 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 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 ) { ...
but
  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 ***