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.
Our WIP branch can be found here: https://gitlab.com/thekesolutions/Koha/-/blob/holds_table
Created attachment 103984 [details] [review] Bug 25260: Add a 'holds' table Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 103985 [details] [review] Bug 25260: Adapt Koha::Hold(s) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 103986 [details] [review] Bug 25260: Atomic update
Created attachment 103987 [details] [review] Bug 25260: Schema updates [DO NOT PUSH]
Created attachment 103988 [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$")
Created attachment 103989 [details] [review] Bug 25260: Add a 'holds' table Sponsored-by: ByWater Solutions Sponsored-by: Theke Solutions
Created attachment 103990 [details] [review] Bug 25260: Adapt Koha::Hold(s) Sponsored-by: ByWater Solutions Sponsored-by: Theke Solutions
Created attachment 103991 [details] [review] Bug 25260: Atomic update Sponsored-by: ByWater Solutions Sponsored-by: Theke Solutions
Created attachment 103992 [details] [review] Bug 25260: Schema updates [DO NOT PUSH] Sponsored-by: ByWater Solutions Sponsored-by: Theke Solutions
Created attachment 103993 [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
Created attachment 104053 [details] [review] Bug 25260: Add a 'holds' table Sponsored-by: ByWater Solutions Sponsored-by: Theke Solutions
Created attachment 104054 [details] [review] Bug 25260: Adapt Koha::Hold(s) Sponsored-by: ByWater Solutions Sponsored-by: Theke Solutions
Created attachment 104055 [details] [review] Bug 25260: Atomic update Sponsored-by: ByWater Solutions Sponsored-by: Theke Solutions
Created attachment 104056 [details] [review] Bug 25260: Schema updates [DO NOT PUSH] Sponsored-by: ByWater Solutions Sponsored-by: Theke Solutions
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
*** Bug 23133 has been marked as a duplicate of this bug. ***
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
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).
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)
(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.
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
Things to fix: - New status: 'processing' (bug 12556) - New column: cancellation_reason (bug 25534)
New column: non_priority (bug 22789)
Remote branch updated. Some things need fixing, help welcome.
I'll jump on to taking a look at this asap.. love the direction it takes.
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?
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>
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>
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>
Created attachment 117748 [details] [review] Bug 25260: Schema files Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 117749 [details] [review] Bug 25260: Sane TestBuilder defaults Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
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.
Any QA people interested here.. this is a significant cleanup piece and would be great to see it progress.
Does not apply. Looks like lots of changes in the Schema patch do not belong here too. Please cleanup.
Created attachment 122761 [details] [review] Bug 25260: Atomic update Sponsored-by: ByWater Solutions Sponsored-by: Theke Solutions Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122762 [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>
Created attachment 122763 [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>
Created attachment 122764 [details] [review] Bug 25260: Schema files Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122765 [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>
Created attachment 122766 [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>
Created attachment 122767 [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>
Rebased. Please y'all: help making this happen :-D Patches accepted
This needs more work due to lots of recent changes (and a few rebase issues).
Created attachment 122787 [details] [review] Bug 25260: Atomic update Sponsored-by: ByWater Solutions Sponsored-by: Theke Solutions Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122788 [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>
Created attachment 122789 [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>
Created attachment 122790 [details] [review] Bug 25260: Schema files Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122791 [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>
Created attachment 122792 [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>
Created attachment 122793 [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>
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!
Created attachment 122976 [details] [review] Bug 25260: Atomic update Sponsored-by: ByWater Solutions Sponsored-by: Theke Solutions Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122977 [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>
Created attachment 122978 [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>
Created attachment 122979 [details] [review] Bug 25260: Schema files Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 122980 [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>
Created attachment 122981 [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>
Created attachment 122982 [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>
Created attachment 122983 [details] [review] Bug 25260: ILSDI is tied to old DB structure for holds Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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?)
(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.
(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).
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.
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.
(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!
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'
> 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 :)
From 2015, bug 10459 from comment 30.
% 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
(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 ...
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>
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>
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>
Created attachment 123074 [details] [review] Bug 25260: Schema files Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
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>
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>
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>
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>
Created attachment 123080 [details] [review] Bug 25260: (follow-up) Remove warning Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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.
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.
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 ^^"
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.
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>
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>
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>
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>
Created attachment 123261 [details] [review] Bug 25260: (follow-up) Remove warning Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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.
Created attachment 123396 [details] [review] Bug 25260: Fix pendingreserves.pl
(In reply to Owen Leonard from comment #93) > I get an error from pendingreserves.pl: Thanks for taking the time to test!
Tried to test this, but patches do not apply. Received this error: Applying: Bug 25260: Atomic update Using index info to reconstruct a base tree... M C4/Installer.pm Falling back to patching base and 3-way merge... Auto-merging C4/Installer.pm CONFLICT (content): Merge conflict in C4/Installer.pm error: Failed to merge in the changes. Patch failed at 0001 Bug 25260: Atomic update hint: Use 'git am --show-current-patch=diff' to see the failed patch When you have resolved this problem run "git bz apply --continue". If you would prefer to skip this patch, instead run "git bz apply --skip". To restore the original branch and stop patching run "git bz apply --abort". Patch left in /tmp/Bug-25260-Atomic-update-pnuVUG.patch
Tried a rebase, failed at Applying: Bug 25260: Add a 'holds' table In kohastructure.sql, it's too hard with the column renames and reorder to propagate the changes of main/master's reserves table to holds. Maybe with git imerge it would help since it finds the conflicts cases one by one instead of having all of them fused together.