Bug 25260 - Merge 'reserves' and 'old_reserves' into a new 'holds' table
Summary: Merge 'reserves' and 'old_reserves' into a new 'holds' table
Status: Signed Off
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
Blocks:
  Show dependency treegraph
 
Reported: 2020-04-23 01:37 UTC by Tomás Cohen Arazi
Modified: 2021-04-01 12:45 UTC (History)
10 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Large patch
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

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
Created attachment 104053 [details] [review]
Bug 25260: Add a 'holds' table

Sponsored-by: ByWater Solutions
Sponsored-by: Theke Solutions
Comment 13 Tomás Cohen Arazi 2020-04-30 20:52:24 UTC
Created attachment 104054 [details] [review]
Bug 25260: Adapt Koha::Hold(s)

Sponsored-by: ByWater Solutions
Sponsored-by: Theke Solutions
Comment 14 Tomás Cohen Arazi 2020-04-30 20:52:31 UTC
Created attachment 104055 [details] [review]
Bug 25260: Atomic update

Sponsored-by: ByWater Solutions
Sponsored-by: Theke Solutions
Comment 15 Tomás Cohen Arazi 2020-04-30 20:52:38 UTC
Created attachment 104056 [details] [review]
Bug 25260: Schema updates [DO NOT PUSH]

Sponsored-by: ByWater Solutions
Sponsored-by: Theke Solutions
Comment 16 Tomás Cohen Arazi 2020-04-30 20:52:47 UTC
Created attachment 104057 [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
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
Created attachment 117745 [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 29 Martin Renvoize 2021-03-04 16:18:38 UTC
Created attachment 117746 [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 30 Martin Renvoize 2021-03-04 16:18:41 UTC
Created attachment 117747 [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 31 Martin Renvoize 2021-03-04 16:18:46 UTC
Created attachment 117748 [details] [review]
Bug 25260: Schema files

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 32 Martin Renvoize 2021-03-04 16:18:50 UTC
Created attachment 117749 [details] [review]
Bug 25260: Sane TestBuilder defaults

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Martin Renvoize 2021-03-04 16:18:54 UTC
Created attachment 117750 [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 34 Martin Renvoize 2021-03-04 16:18:58 UTC
Created attachment 117751 [details] [review]
Bug 25260: (QA follow-up) Fix any bad dates prior to migration

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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.