Bug 25260 - Merge 'reserves' and 'old_reserves' into a new 'holds' table
Summary: Merge 'reserves' and 'old_reserves' into a new 'holds' table
Status: Needs Signoff
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: https://gitlab.com/thekesolutions/Koh...
Keywords:
: 23133 (view as bug list)
Depends on: 12556 22789 25534 28744
Blocks:
  Show dependency treegraph
 
Reported: 2020-04-23 01:37 UTC by Tomás Cohen Arazi
Modified: 2022-06-08 09:25 UTC (History)
12 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Large patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 25260: Add a 'holds' table (20.01 KB, patch)
2020-04-29 21:11 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 25260: Adapt Koha::Hold(s) (12.45 KB, patch)
2020-04-29 21:11 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 25260: Atomic update (11.77 KB, patch)
2020-04-29 21:11 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 25260: Schema updates [DO NOT PUSH] (31.39 KB, patch)
2020-04-29 21:11 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 25260: Adapt all the things (194.13 KB, patch)
2020-04-29 21:11 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 25260: Add a 'holds' table (20.02 KB, patch)
2020-04-29 21:21 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 25260: Adapt Koha::Hold(s) (12.46 KB, patch)
2020-04-29 21:21 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 25260: Atomic update (11.83 KB, patch)
2020-04-29 21:21 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 25260: Schema updates [DO NOT PUSH] (31.45 KB, patch)
2020-04-29 21:21 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 25260: Adapt all the things (194.21 KB, patch)
2020-04-29 21:21 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 25260: Add a 'holds' table (19.99 KB, patch)
2020-04-30 20:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Adapt Koha::Hold(s) (12.51 KB, patch)
2020-04-30 20:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Atomic update (11.82 KB, patch)
2020-04-30 20:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Schema updates [DO NOT PUSH] (31.35 KB, patch)
2020-04-30 20:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Adapt all the things (197.08 KB, patch)
2020-04-30 20:52 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Atomic update (13.96 KB, patch)
2021-03-04 16:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25260: Add a 'holds' table (27.94 KB, patch)
2021-03-04 16:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25260: Adapt Koha::Hold(s) (23.15 KB, patch)
2021-03-04 16:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25260: Schema files (11.91 KB, patch)
2021-03-04 16:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25260: Sane TestBuilder defaults (1.09 KB, patch)
2021-03-04 16:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25260: Adapt all the things (208.50 KB, patch)
2021-03-04 16:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25260: (QA follow-up) Fix any bad dates prior to migration (1.15 KB, patch)
2021-03-04 16:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25260: Atomic update (13.96 KB, patch)
2021-07-11 23:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Add a 'holds' table (27.70 KB, patch)
2021-07-11 23:40 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Adapt Koha::Hold(s) (23.30 KB, patch)
2021-07-11 23:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Schema files (8.58 KB, patch)
2021-07-11 23:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Sane TestBuilder defaults (1.14 KB, patch)
2021-07-11 23:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Adapt all the things (207.24 KB, patch)
2021-07-11 23:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: (QA follow-up) Fix any bad dates prior to migration (1.20 KB, patch)
2021-07-11 23:41 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Atomic update (13.96 KB, patch)
2021-07-12 21:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Add a 'holds' table (27.70 KB, patch)
2021-07-12 21:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Adapt Koha::Hold(s) (23.30 KB, patch)
2021-07-12 21:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Schema files (31.59 KB, patch)
2021-07-12 21:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Sane TestBuilder defaults (1.14 KB, patch)
2021-07-12 21:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Adapt all the things (233.17 KB, patch)
2021-07-12 21:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: (QA follow-up) Fix any bad dates prior to migration (1.20 KB, patch)
2021-07-12 21:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Atomic update (14.01 KB, patch)
2021-07-20 16:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Add a 'holds' table (27.70 KB, patch)
2021-07-20 16:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Adapt Koha::Hold(s) (23.30 KB, patch)
2021-07-20 16:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Schema files (33.77 KB, patch)
2021-07-20 16:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Sane TestBuilder defaults (1.14 KB, patch)
2021-07-20 16:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Adapt all the things (257.17 KB, patch)
2021-07-20 16:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: (QA follow-up) Fix any bad dates prior to migration (1.20 KB, patch)
2021-07-20 16:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: ILSDI is tied to old DB structure for holds (1009 bytes, patch)
2021-07-20 16:36 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Atomic update (14.01 KB, patch)
2021-07-23 04:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Add a 'holds' table (27.70 KB, patch)
2021-07-23 04:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Adapt Koha::Hold(s) (23.30 KB, patch)
2021-07-23 04:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Schema files (33.77 KB, patch)
2021-07-23 04:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Sane TestBuilder defaults (1.14 KB, patch)
2021-07-23 04:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Adapt all the things (257.14 KB, patch)
2021-07-23 04:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: (QA follow-up) Fix any bad dates prior to migration (1.20 KB, patch)
2021-07-23 04:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: ILSDI is tied to old DB structure for holds (1009 bytes, patch)
2021-07-23 04:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: (QA follow-up) Rename id => hold_id (35.29 KB, patch)
2021-07-23 04:04 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: (follow-up) Remove warning (1014 bytes, patch)
2021-07-23 04:05 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Adapt all the things (257.14 KB, patch)
2021-07-28 15:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: (QA follow-up) Fix any bad dates prior to migration (1.20 KB, patch)
2021-07-28 15:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: ILSDI is tied to old DB structure for holds (1009 bytes, patch)
2021-07-28 15:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: (QA follow-up) Rename id => hold_id (36.71 KB, patch)
2021-07-28 15:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: (follow-up) Remove warning (1014 bytes, patch)
2021-07-28 15:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 25260: Fix pendingreserves.pl (9.83 KB, patch)
2021-08-02 18:32 UTC, Tomás Cohen Arazi
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 2020-04-23 01:37:17 UTC
Reengineering the current 'reserves' and 'old_reserves' tables, we propose to introduce a new table, following our current agreements on the design and addressing or use cases.
An RFC will be submitted and linked here.
Comment 1 Tomás Cohen Arazi 2020-04-23 01:40:34 UTC Comment hidden (obsolete)
Comment 2 Agustín Moyano 2020-04-29 21:11:19 UTC Comment hidden (obsolete)
Comment 3 Agustín Moyano 2020-04-29 21:11:22 UTC Comment hidden (obsolete)
Comment 4 Agustín Moyano 2020-04-29 21:11:26 UTC Comment hidden (obsolete)
Comment 5 Agustín Moyano 2020-04-29 21:11:30 UTC Comment hidden (obsolete)
Comment 6 Agustín Moyano 2020-04-29 21:11:35 UTC Comment hidden (obsolete)
Comment 7 Agustín Moyano 2020-04-29 21:21:34 UTC Comment hidden (obsolete)
Comment 8 Agustín Moyano 2020-04-29 21:21:38 UTC Comment hidden (obsolete)
Comment 9 Agustín Moyano 2020-04-29 21:21:41 UTC Comment hidden (obsolete)
Comment 10 Agustín Moyano 2020-04-29 21:21:45 UTC Comment hidden (obsolete)
Comment 11 Agustín Moyano 2020-04-29 21:21:51 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2020-04-30 20:52:16 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi 2020-04-30 20:52:24 UTC Comment hidden (obsolete)
Comment 14 Tomás Cohen Arazi 2020-04-30 20:52:31 UTC Comment hidden (obsolete)
Comment 15 Tomás Cohen Arazi 2020-04-30 20:52:38 UTC Comment hidden (obsolete)
Comment 16 Tomás Cohen Arazi 2020-04-30 20:52:47 UTC Comment hidden (obsolete)
Comment 17 Tomás Cohen Arazi 2020-05-11 11:49:06 UTC
*** Bug 23133 has been marked as a duplicate of this bug. ***
Comment 18 Tomás Cohen Arazi 2020-07-23 15:39:05 UTC
I've added a remote branch on the URL field on this bug. That's where I'll keep a rebased patchset up-to-date. Please test and let me know if I should add your signature to the patchset.

https://gitlab.com/thekesolutions/Koha/-/tree/bug_25260_holds_table
Comment 19 Tomás Cohen Arazi 2020-07-23 15:41:06 UTC
In order to test this, because of the DB structure changes, you need to either:
- Have a master DB, checkout this branch and upgrade
- Perform the webinstaller steps

This is because the sample data in misc4dev is not adapted to this changes, so the handy reset_all command won't work (most probably).
Comment 20 Jonathan Druart 2020-08-26 13:45:01 UTC
Thinking loudly

What happens if you create an item, place a hold on it, remove the hold, delete the item.
In current master the old_reserves entry still exist (for stats for instance)
Comment 21 Tomás Cohen Arazi 2020-08-26 13:49:15 UTC
(In reply to Jonathan Druart from comment #20)
> Thinking loudly
> 
> What happens if you create an item, place a hold on it, remove the hold,
> delete the item.
> In current master the old_reserves entry still exist (for stats for instance)

That behaviour should be preserved and it would be a bug if it didn't.
Both biblio_id and item_id are defined as ON DELETE SET NULL. So I guess this should be ok.
Comment 22 Séverine Queune 2020-10-30 16:05:21 UTC
Applying: Bug 25260: Add a 'holds' table
Using index info to reconstruct a base tree...
M       installer/data/mysql/kohastructure.sql
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/kohastructure.sql
CONFLICT (content): Merge conflict in installer/data/mysql/kohastructure.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 25260: Add a 'holds' table
Comment 23 Tomás Cohen Arazi 2020-11-03 15:28:59 UTC
Things to fix:
- New status: 'processing' (bug 12556)
- New column: cancellation_reason (bug 25534)
Comment 24 Tomás Cohen Arazi 2020-11-03 15:31:50 UTC
New column: non_priority (bug 22789)
Comment 25 Tomás Cohen Arazi 2021-01-05 21:07:05 UTC
Remote branch updated. Some things need fixing, help welcome.
Comment 26 Martin Renvoize 2021-01-06 16:43:18 UTC
I'll jump on to taking a look at this asap.. love the direction it takes.
Comment 27 Martin Renvoize 2021-02-02 16:21:29 UTC
I keep coming back to this, but am struggling to assign time to it :(.  Is anyone else able to test to help move it forward?
Comment 28 Martin Renvoize 2021-03-04 16:18:35 UTC Comment hidden (obsolete)
Comment 29 Martin Renvoize 2021-03-04 16:18:38 UTC Comment hidden (obsolete)
Comment 30 Martin Renvoize 2021-03-04 16:18:41 UTC Comment hidden (obsolete)
Comment 31 Martin Renvoize 2021-03-04 16:18:46 UTC Comment hidden (obsolete)
Comment 32 Martin Renvoize 2021-03-04 16:18:50 UTC Comment hidden (obsolete)
Comment 33 Martin Renvoize 2021-03-04 16:18:54 UTC Comment hidden (obsolete)
Comment 34 Martin Renvoize 2021-03-04 16:18:58 UTC Comment hidden (obsolete)
Comment 35 Martin Renvoize 2021-03-04 16:20:50 UTC
I'm just running the whole test suite now.. but in all manual testing this is working great.. I'll set it to signed off and report back if any tests fail.
Comment 36 Martin Renvoize 2021-04-01 12:44:43 UTC
Any QA people interested here.. this is a significant cleanup piece and would be great to see it progress.
Comment 37 Marcel de Rooy 2021-07-09 07:43:42 UTC
Does not apply. Looks like lots of changes in the Schema patch do not belong here too. Please cleanup.
Comment 38 Tomás Cohen Arazi 2021-07-11 23:40:42 UTC Comment hidden (obsolete)
Comment 39 Tomás Cohen Arazi 2021-07-11 23:40:50 UTC Comment hidden (obsolete)
Comment 40 Tomás Cohen Arazi 2021-07-11 23:41:00 UTC Comment hidden (obsolete)
Comment 41 Tomás Cohen Arazi 2021-07-11 23:41:12 UTC Comment hidden (obsolete)
Comment 42 Tomás Cohen Arazi 2021-07-11 23:41:22 UTC Comment hidden (obsolete)
Comment 43 Tomás Cohen Arazi 2021-07-11 23:41:31 UTC Comment hidden (obsolete)
Comment 44 Tomás Cohen Arazi 2021-07-11 23:41:39 UTC Comment hidden (obsolete)
Comment 45 Tomás Cohen Arazi 2021-07-11 23:42:28 UTC
Rebased.

Please y'all: help making this happen :-D Patches accepted
Comment 46 Tomás Cohen Arazi 2021-07-12 11:38:08 UTC
This needs more work due to lots of recent changes (and a few rebase issues).
Comment 47 Tomás Cohen Arazi 2021-07-12 21:19:53 UTC Comment hidden (obsolete)
Comment 48 Tomás Cohen Arazi 2021-07-12 21:19:58 UTC Comment hidden (obsolete)
Comment 49 Tomás Cohen Arazi 2021-07-12 21:20:03 UTC Comment hidden (obsolete)
Comment 50 Tomás Cohen Arazi 2021-07-12 21:20:08 UTC Comment hidden (obsolete)
Comment 51 Tomás Cohen Arazi 2021-07-12 21:20:14 UTC Comment hidden (obsolete)
Comment 52 Tomás Cohen Arazi 2021-07-12 21:20:20 UTC Comment hidden (obsolete)
Comment 53 Tomás Cohen Arazi 2021-07-12 21:20:25 UTC Comment hidden (obsolete)
Comment 54 Tomás Cohen Arazi 2021-07-12 21:22:39 UTC
Ok, this now looks much better. No more compilation issues and many rebase problems fixed.

I took the approach of using the view table (reserves) for Letters.pm so we deal with a possible migration path for existing letters in a separate bug.

There are still bugs you will notice if you run:

git diff origin/master --name-only | grep -e '\.t$' | xargs prove

Help is welcome, but things look much better. A lot has changed recently!
Comment 55 Tomás Cohen Arazi 2021-07-20 16:35:07 UTC Comment hidden (obsolete)
Comment 56 Tomás Cohen Arazi 2021-07-20 16:35:14 UTC Comment hidden (obsolete)
Comment 57 Tomás Cohen Arazi 2021-07-20 16:35:21 UTC Comment hidden (obsolete)
Comment 58 Tomás Cohen Arazi 2021-07-20 16:35:28 UTC Comment hidden (obsolete)
Comment 59 Tomás Cohen Arazi 2021-07-20 16:35:35 UTC Comment hidden (obsolete)
Comment 60 Tomás Cohen Arazi 2021-07-20 16:35:43 UTC Comment hidden (obsolete)
Comment 61 Tomás Cohen Arazi 2021-07-20 16:35:50 UTC Comment hidden (obsolete)
Comment 62 Tomás Cohen Arazi 2021-07-20 16:36:14 UTC Comment hidden (obsolete)
Comment 63 Tomás Cohen Arazi 2021-07-20 16:40:02 UTC
Ok, rebased after the EXPORT bug was pushed.

Letters now will use the new holds table. See the TODO.

There are still bugs you will notice if you run:

git diff origin/master --name-only | grep -e '\.t$' | xargs prove

Help is welcome, but things look much better. A lot has changed recently!

TODO:
- t/db_dependent/Reserves.t failure
- t/db_dependent/Circulation.t failure
- Add some warning in about.pl about wrong letters (maybe?)
Comment 64 Tomás Cohen Arazi 2021-07-20 20:29:46 UTC
(In reply to Tomás Cohen Arazi from comment #63)
> Ok, rebased after the EXPORT bug was pushed.
> 
> Letters now will use the new holds table. See the TODO.
> 
> There are still bugs you will notice if you run:
> 
> git diff origin/master --name-only | grep -e '\.t$' | xargs prove
> 
> Help is welcome, but things look much better. A lot has changed recently!
> 
> TODO:
> - t/db_dependent/Reserves.t failure
> - t/db_dependent/Circulation.t failure
> - Add some warning in about.pl about wrong letters (maybe?)

Another TODO: rewrite C4::HoldsQueue in terms of the 'holds' table. It felt like  just translating into the new table columns was the same amount of work as moving it into using Koha::Holds directly, and more thought is needed as there are a bunch of possible optimizations that could be made in the area.
Comment 65 Jonathan Druart 2021-07-21 08:12:08 UTC
(In reply to Tomás Cohen Arazi from comment #63)

> - t/db_dependent/Reserves.t failure

There are 2 things:
1.
1107                 item_id          => $item->biblionumber,
Key must be biblio_id

2.
The $title_level_target_query query in _Findgroupreserve is now returning the "reserved" hold. On master it's not matching any rows and the third query ($query) is hit and returned the different holds.

I think it's coming from:
-        JOIN hold_fill_targets USING (reserve_id)
vs
+        JOIN hold_fill_targets ON (
+                holds.biblio_id=hold_fill_targets.biblionumber
+            AND holds.patron_id=hold_fill_targets.borrowernumber)


> - t/db_dependent/Circulation.t failure

    #          got: 'on_reserve'
    #     expected: 'too_soon'
Related to hold's status as well so may be fixed if the previous test is corrected.

> - Add some warning in about.pl about wrong letters (maybe?)

After we moved the marcxml out of biblioitems we added a warning on the report list view. Maybe we should do the same for the notice templates?

  commit f22d2e7200ee8b35aff66b26acc3e2daa49f9f0d
  Bug 17898: Automagically convert SQL reports


Questions:
* Shouldn't *_date DB fields be *_on?
* item_level => item_level_request
I think we agreed on "item_level_request", why did you change it?
* Shouldn't holds.id be holds.hold_id? I cannot remember when/where but I think we agreed we shouldn't use "id" (to prevent wrong id to be returned on JOIN).
Comment 66 Victor Grousset/tuxayo 2021-07-21 13:44:03 UTC
Hi, just letting know that I'm investigating the «No reserves HOLD letter transported by print» warning/error. Just the message, not the failure of the test.(maybe linked, I didn't looked at all)

So this is weird, it finds a template for
- reserves&HOLD&email
- reserves&HOLD&sms

but not for reserves&HOLD&print

When the letter table contains only
- reserves&HOLD&email
- reserves&HOLD&print

So it shouldn't for sms but should for print

The same query (from Notice/Templates.pm PoV) on the previous test 


The warning/error comes from
"Itemnumber should not be removed when the waiting status is revert"
"Itemnumber should be set on hold confirmation"

More precisely the ModReserveAffect() call before the assertion of the above tests.


After `cpanm DBIx::QueryLog` adding `DBIx::QueryLog` before the call to the ORM.
I can see that the params are correct. The query is:
SELECT COUNT( * ) FROM `letter` `me` WHERE ( ( ( `branchcode` = 'Rvfd7OyOU8' OR `branchcode` = '' ) AND `code` = 'HOLD' AND `lang` = 'default' AND `message_transport_type` = 'print' AND `module` = 'reserves' ) );

Which yields 1 when ran in mariadb client. (right after interupting the tests with die: after ModReserveAffect() )

But when using dbh() with the same query right before and after the DBIx one is done (there are two ->count calls). Is yields 0.

That's what I've found so far.
Comment 67 Victor Grousset/tuxayo 2021-07-21 13:45:46 UTC
I missed the two previous messages. At the first glance it seems the «No reserves HOLD letter transported by print» isn't related to the other stuff.
Comment 68 Tomás Cohen Arazi 2021-07-22 12:16:32 UTC
(In reply to Jonathan Druart from comment #65)
> (In reply to Tomás Cohen Arazi from comment #63)
> 
> > - t/db_dependent/Reserves.t failure
> 
> There are 2 things:
> 1.
> 1107                 item_id          => $item->biblionumber,
> Key must be biblio_id
> 
> 2.
> The $title_level_target_query query in _Findgroupreserve is now returning
> the "reserved" hold. On master it's not matching any rows and the third
> query ($query) is hit and returned the different holds.
> 
> I think it's coming from:
> -        JOIN hold_fill_targets USING (reserve_id)
> vs
> +        JOIN hold_fill_targets ON (
> +                holds.biblio_id=hold_fill_targets.biblionumber
> +            AND holds.patron_id=hold_fill_targets.borrowernumber)

Thanks! That solved Reserves.t. Will ask Agustin about that change, because we merged many commits in our branch into this commit (a year ago).

> > - t/db_dependent/Circulation.t failure
> 
>     #          got: 'on_reserve'
>     #     expected: 'too_soon'
> Related to hold's status as well so may be fixed if the previous test is
> corrected.

This might be something else because the problem stands. Will submit the version with the fix for review.

> > - Add some warning in about.pl about wrong letters (maybe?)
> 
> After we moved the marcxml out of biblioitems we added a warning on the
> report list view. Maybe we should do the same for the notice templates?
> 
>   commit f22d2e7200ee8b35aff66b26acc3e2daa49f9f0d
>   Bug 17898: Automagically convert SQL reports

Good idea. If there are chances this dev gets pushed to master, this is something I will work on.

> Questions:
> * Shouldn't *_date DB fields be *_on?

Probably, Some will be _on, some others _until. I didn't think about it and went into doing the API-way. I didn't find a written Coding guideline about this, though. No problem for me with changing this. It will require a bunch of new mappings only (Koha::Hold->to_api_mapping).

> * item_level => item_level_request

Currently it is:
- 'item_level_hold' in (old)reserves
- 'item_level_request' in hold_fill_target and tmp_holdsqueue
- 'item_level' on the API (voted)

> I think we agreed on "item_level_request", why did you change it?

As with dates, I preferred to directly use the API version. Can be revisited in a follow-up patch along with the other changes if QA requires it.

> * Shouldn't holds.id be holds.hold_id? I cannot remember when/where but I
> think we agreed we shouldn't use "id" (to prevent wrong id to be returned on
> JOIN).

That's correct, and it is in the guidelines. Follow-up coming.

Thanks for the review!
Comment 69 Martin Renvoize 2021-07-22 14:12:35 UTC
I can't find a guideline for DB field names (other than the tablename_id one.. which I wasn't aware of before.. I far prefer just 'id' for primary key and 'tablename_id' for relations.. as for me that makes it much clearer (and it makes dbic much happier when auto-building schema's.. you don't end up with a relation that has the same name as the field accessor).

I kinda like the idea of sticking to the API field guidelines for new tables where we can.. but I'm not hugely worried.. I just think it's clearer if we can be consistent as much as possible: https://wiki.koha-community.org/wiki/Coding_Guidelines_-_API#SWAGGER1.3.4:_mapping

So.. my vote would be for 'action_date' as opposed to 'action_on'.. and perhaps adding a DB Guidelines to say 'Where creating new, DB fields should match the API field name guidelines'
Comment 70 Jonathan Druart 2021-07-22 15:25:48 UTC
> my vote would be for 'action_date' as opposed to 'action_on'

Well, ok but we need to be consistent over the years :D

I am pretty sure we all agreed on *_on when we discussed it and we have been enforcing this rule for a couple of years already :)
Comment 71 Jonathan Druart 2021-07-22 15:32:41 UTC
From 2015, bug 10459 from comment 30.
Comment 72 Jonathan Druart 2021-07-22 15:33:47 UTC
% git grep -c '_on`' installer/data/mysql/kohastructure.sql
installer/data/mysql/kohastructure.sql:29
% git grep -c '_date`' installer/data/mysql/kohastructure.sql
installer/data/mysql/kohastructure.sql:4
Comment 73 Jonathan Druart 2021-07-22 15:36:25 UTC
(In reply to Jonathan Druart from comment #72)
> % git grep -c '_on`' installer/data/mysql/kohastructure.sql
> installer/data/mysql/kohastructure.sql:29
> % git grep -c '_date`' installer/data/mysql/kohastructure.sql
> installer/data/mysql/kohastructure.sql:4

git grep -c 'date`' installer/data/mysql/kohastructure.sql                                                                                         [17:35:36]
installer/data/mysql/kohastructure.sql:65

...
Comment 74 Tomás Cohen Arazi 2021-07-23 04:04:07 UTC
Created attachment 123071 [details] [review]
Bug 25260: Atomic update

Sponsored-by: ByWater Solutions
Sponsored-by: Theke Solutions

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 75 Tomás Cohen Arazi 2021-07-23 04:04:13 UTC
Created attachment 123072 [details] [review]
Bug 25260: Add a 'holds' table

This patch introduces a new table for holds. It also creates views for
reseves and oldreserves to retain the current behaviour. This views will
not be used in the codebase, but kept for backwards compatibility in
reports.

Sponsored-by: ByWater Solutions
Sponsored-by: Theke Solutions

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 76 Tomás Cohen Arazi 2021-07-23 04:04:19 UTC
Created attachment 123073 [details] [review]
Bug 25260: Adapt Koha::Hold(s)

The new table structure aligns the 'holds' table with the terminology
that's been agreed, and also makes some design changes, like having an
ENUM status field, that cover all the current options for the hold lifecycle.

This requires changing the Koha::Hold(s) classes to this new situation.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 77 Tomás Cohen Arazi 2021-07-23 04:04:25 UTC
Created attachment 123074 [details] [review]
Bug 25260: Schema files

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 78 Tomás Cohen Arazi 2021-07-23 04:04:30 UTC
Created attachment 123075 [details] [review]
Bug 25260: Sane TestBuilder defaults

This patch sets some sane defaults for TestBuilder.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 79 Tomás Cohen Arazi 2021-07-23 04:04:36 UTC
Created attachment 123076 [details] [review]
Bug 25260: Adapt all the things

This patch adapts many pm and test files to work with unified holds
table.

To test:

1. Remove all previous holds in reserves and old_reserves tables.
2. Create 6 holds. Cancel 2 of them, fulfill one, set one in waiting and
   another in transit.
CHECK => "select reserve_id, found from reserves" query in mysql should return 3 rows, where in found
column should find 'W', 'T' and another with null values.
      => "select reserve_id, found from old_reserves" query in mysql should return 3 rows, where in found column you should find 2 with 'C', and one with 'F' values.
3. in mysql change reserve_id to the value of a hold in reserve table.
4. apply patches
5. updatedatabase
SUCCESS => you should get the following warning "There were 1 old reserves that could not be moved, please check '_old_reserves'"
        => "select id, status, completed from holds" query in mysql should return 5 rows.
          => The hold that had null value in found column, now should have 'placed' status and 0 in completed.
          => The one that had 'W' in found column, now should have 'waiting' status and 0 in completed.
          => The one that had 'T' in found column, now should have 'in_transit' status and 0 in completed.
          => The one that had 'F' in found column, now should have 'fulfilled' status and 1 in completed.
          => The unmodified cancelled hold, now should have 'cancelled' status and 1 in completed.
        => "select * from _old_reserves" should give you one row with the modified hold.
6. in staff interface create, cancel, modify priorities, etc and everything should work exactly as before.

 # Run every test that has reserve and hold word wihtin
 # check that you are in main koha directory
 # check you have correctly setted KOHA_INTRANET_URL and KOHA_OPAC_URL environment variables
 # check plack is up and running.. tests will use selenium
 # prepare yourself a mug of coffe/te or take a nap.. this will take a while
7. prove $(grep -ilr -e reserve -e hold t|grep "\.t$")

 # finally
8. Sign off

Sponsored-by: ByWater Solutions
Sponsored-by: Theke Solutions

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 80 Tomás Cohen Arazi 2021-07-23 04:04:42 UTC
Created attachment 123077 [details] [review]
Bug 25260: (QA follow-up) Fix any bad dates prior to migration

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 81 Tomás Cohen Arazi 2021-07-23 04:04:48 UTC
Created attachment 123078 [details] [review]
Bug 25260: ILSDI is tied to old DB structure for holds

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 82 Tomás Cohen Arazi 2021-07-23 04:04:54 UTC
Created attachment 123079 [details] [review]
Bug 25260: (QA follow-up) Rename id => hold_id

The guidelines clearly specify this is how the primary key needs to be
named.

https://wiki.koha-community.org/wiki/Coding_Guidelines#SQL7:_Primary_keys

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 83 Tomás Cohen Arazi 2021-07-23 04:05:00 UTC
Created attachment 123080 [details] [review]
Bug 25260: (follow-up) Remove warning

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 84 Tomás Cohen Arazi 2021-07-23 04:06:23 UTC
The following failures remain:
- t/db_dependent/Circulation.t
- t/db_dependent/Reserves.t

I'm too sleepy to continue.
Bug 28744 is related to id => hold_id.
Comment 85 Victor Grousset/tuxayo 2021-07-24 08:52:23 UTC
updatedatabase blew up
> DBD::mysql::db do failed: Unknown column 'hold_id' in 'field list

https://paste.chapril.org/?7d7a8d5b83ead5ef#4gdtAfiFa4deck4s21h8MzVsRY18oniZ7kwxYCSUivcZ

It happens when I start my instance on master code, apply these patches + the dependency bug 28744.

It works when I start my instance with all the patches already applied.

It's a koha-testing-docker install. I don't see in the start logs a mention of the atomic updates applied but it definitely does.
Comment 86 Victor Grousset/tuxayo 2021-07-26 18:08:14 UTC
Quick finding: On master on the relevant tests if we change:

ModReserveAffect( $item->itemnumber, $patron->borrowernumber,1 );
↓↓↓
ModReserveAffect( $item->itemnumber, $patron->borrowernumber );

There will be the same warning «No reserves HOLD letter transported by print»
(Same as when the patches are applied.) I hope this helps.

Of course the patches need the 3rd param gone, otherwise the test fail, it has been changed for a reason ^^"
Comment 87 Victor Grousset/tuxayo 2021-07-26 18:11:06 UTC
Here are the calls.

https://gitlab.com/koha-community/Koha/-/blob/9d4a4e7ca20fcc1da75708fb2ae55308fb49c77c/t/db_dependent/Reserves.t#L952

https://gitlab.com/koha-community/Koha/-/blob/9d4a4e7ca20fcc1da75708fb2ae55308fb49c77c/t/db_dependent/Reserves.t#L980

---

Also, minor stuff, in the patches, the comment «# Mark it waiting» is removed from the 1st call but not from the 2nd. Likely needs to be removed also.
Comment 88 Tomás Cohen Arazi 2021-07-28 15:21:18 UTC
Created attachment 123257 [details] [review]
Bug 25260: Adapt all the things

This patch adapts many pm and test files to work with unified holds
table.

To test:

1. Remove all previous holds in reserves and old_reserves tables.
2. Create 6 holds. Cancel 2 of them, fulfill one, set one in waiting and
   another in transit.
CHECK => "select reserve_id, found from reserves" query in mysql should return 3 rows, where in found
column should find 'W', 'T' and another with null values.
      => "select reserve_id, found from old_reserves" query in mysql should return 3 rows, where in found column you should find 2 with 'C', and one with 'F' values.
3. in mysql change reserve_id to the value of a hold in reserve table.
4. apply patches
5. updatedatabase
SUCCESS => you should get the following warning "There were 1 old reserves that could not be moved, please check '_old_reserves'"
        => "select id, status, completed from holds" query in mysql should return 5 rows.
          => The hold that had null value in found column, now should have 'placed' status and 0 in completed.
          => The one that had 'W' in found column, now should have 'waiting' status and 0 in completed.
          => The one that had 'T' in found column, now should have 'in_transit' status and 0 in completed.
          => The one that had 'F' in found column, now should have 'fulfilled' status and 1 in completed.
          => The unmodified cancelled hold, now should have 'cancelled' status and 1 in completed.
        => "select * from _old_reserves" should give you one row with the modified hold.
6. in staff interface create, cancel, modify priorities, etc and everything should work exactly as before.

 # Run every test that has reserve and hold word wihtin
 # check that you are in main koha directory
 # check you have correctly setted KOHA_INTRANET_URL and KOHA_OPAC_URL environment variables
 # check plack is up and running.. tests will use selenium
 # prepare yourself a mug of coffe/te or take a nap.. this will take a while
7. prove $(grep -ilr -e reserve -e hold t|grep "\.t$")

 # finally
8. Sign off

Sponsored-by: ByWater Solutions
Sponsored-by: Theke Solutions

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 89 Tomás Cohen Arazi 2021-07-28 15:21:26 UTC
Created attachment 123258 [details] [review]
Bug 25260: (QA follow-up) Fix any bad dates prior to migration

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 90 Tomás Cohen Arazi 2021-07-28 15:21:34 UTC
Created attachment 123259 [details] [review]
Bug 25260: ILSDI is tied to old DB structure for holds

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 91 Tomás Cohen Arazi 2021-07-28 15:21:44 UTC
Created attachment 123260 [details] [review]
Bug 25260: (QA follow-up) Rename id => hold_id

The guidelines clearly specify this is how the primary key needs to be
named.

https://wiki.koha-community.org/wiki/Coding_Guidelines#SQL7:_Primary_keys

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 92 Tomás Cohen Arazi 2021-07-28 15:21:52 UTC
Created attachment 123261 [details] [review]
Bug 25260: (follow-up) Remove warning

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 93 Owen Leonard 2021-07-28 16:03:36 UTC
I get an error from pendingreserves.pl:

DBIx::Class::ResultSource::_resolve_join(): No such relationship itembib on Hold at /kohadevbox/koha/Koha/Objects.pm line 444
 at /usr/share/perl5/DBIx/Class/Exception.pm line 77

When checking in an item which has a hold, the modal dialog has incomplete information:

Hold for:

A patron from library -

Transfer to: 

The hold isn't marked as "Waiting" even though it's at its destination library.
Comment 94 Tomás Cohen Arazi 2021-08-02 18:32:46 UTC
Created attachment 123396 [details] [review]
Bug 25260: Fix pendingreserves.pl
Comment 95 Tomás Cohen Arazi 2021-09-20 12:24:33 UTC
(In reply to Owen Leonard from comment #93)
> I get an error from pendingreserves.pl:

Thanks for taking the time to test!