Bug 11744 needs some additional work. The routine _cancel_items_receipt should call C4::Items::DelItem.
Created attachment 29752 [details] [review] Bug 12555: Call DelItem on cancelling a receipt On cancelling a receipt, the items should be deleted using the C4::Items::DelItem subroutine in order to update zebra and sent the items in the deleteditems table. The entries in aqorders_items will automatically deleted by the DBMS (on delete cascade). Test plan: 0/ Set the AcqCreateItem pref to "on receiving" 1/ Create an order with some items 2/ Receive the items 3/ Note the itemnumber in the items table 4/ Cancel the receipt 5/ Verify the itemnumbers are present in the deleteditems table and the items have been deleted.
Created attachment 30613 [details] [review] Bug 12555: Call DelItem on cancelling a receipt On cancelling a receipt, the items should be deleted using the C4::Items::DelItem subroutine in order to update zebra and sent the items in the deleteditems table. The entries in aqorders_items will automatically deleted by the DBMS (on delete cascade). Test plan: 0/ Set the AcqCreateItem pref to "on receiving" 1/ Create an order with some items 2/ Receive the items 3/ Note the itemnumber in the items table 4/ Cancel the receipt 5/ Verify the itemnumbers are present in the deleteditems table and the items have been deleted. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
It worked nicely in my tests, but makes one of the tests fail in my database: $ perl t/db_dependent/Acquisition/CancelReceipt.t 1..2 not ok 1 - Create items on receiving: 0 item exist after cancelling a receipt # Failed test 'Create items on receiving: 0 item exist after cancelling a receipt' # at t/db_dependent/Acquisition/CancelReceipt.t line 70. # got: '1' # expected: '0' DBD::mysql::st execute failed: Duplicate entry '4126' for key 'PRIMARY' at /home/katrin/kohaclone/C4/Acquisition.pm line 1317. DBD::mysql::st execute failed: Duplicate entry '4126' for key 'PRIMARY' at /home/katrin/kohaclone/C4/Acquisition.pm line 1317. # Looks like you planned 2 tests but ran 1. # Looks like you failed 1 test of 1 run. # Looks like your test exited with 255 just after 1.
(In reply to Jonathan Druart from comment #1) > The entries in aqorders_items will automatically deleted by the DBMS (on > delete cascade). Actually it's not true at the moment, aqorders_items.itemnumber does not have a foreign key on items.itemnumber. But it should, isn't it?
Created attachment 31008 [details] [review] Bug 12555: Add a FK on aqorders_items.itemnumber The previous patch assumed that deleting an item will delete the entry in aqorders_items. But the FK did not exist. Looking at the code, we never join aqorders_items and deleteditems, so I think we can add this constraint. Before adding this constraint, we need to delete all entries in aqorders_items if the item has already been deleted.
Applying: Bug 12555: Add a FK on aqorders_items.itemnumber fatal: sha1 information is lacking or useless (installer/data/mysql/kohastructure.sql). Repository lacks necessary blobs to fall back on 3-way merge. Sorry Jonathan :/ Liz
Created attachment 33290 [details] [review] Bug 12555: Call DelItem on cancelling a receipt On cancelling a receipt, the items should be deleted using the C4::Items::DelItem subroutine in order to update zebra and sent the items in the deleteditems table. The entries in aqorders_items will automatically deleted by the DBMS (on delete cascade). Test plan: 0/ Set the AcqCreateItem pref to "on receiving" 1/ Create an order with some items 2/ Receive the items 3/ Note the itemnumber in the items table 4/ Cancel the receipt 5/ Verify the itemnumbers are present in the deleteditems table and the items have been deleted. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Created attachment 33291 [details] [review] Bug 12555: Add a FK on aqorders_items.itemnumber The previous patch assumed that deleting an item will delete the entry in aqorders_items. But the FK did not exist. Looking at the code, we never join aqorders_items and deleteditems, so I think we can add this constraint. Before adding this constraint, we need to delete all entries in aqorders_items if the item has already been deleted.
>Before adding this constraint, we need to delete all entries in aqorders_items if the item has already been deleted. Before applying, : 0/ Set the AcqCreateItem pref to "on receiving" 1/ Create an order with 1 item 2/ On Catalog "Delete the items" of the biblio record. In the DB I saw an aqorders_items's record and a deleteditems's record with the same itemnumber entry. Then I've applied the patches against master 3.17.00.057 HEAD 13229. And I upgraded the DB as required. [In the DB the aqorders_items record was deleted as required.] Then I tried to create an order with an item, but no item was created. So I pass the patch to "Failed QA" status.
(In reply to Paola Rossi from comment #9) > Then I tried to create an order with an item, but no item was created. If AcqCreateItem is set to "on receiving", it's normal that no item is created on ordering. Did you mean the item is not created on receiving neither?
Created attachment 34016 [details] [review] Bug 12555: Call DelItem on cancelling a receipt I've tried to test the patches againt master 3.17.00.060 HEAD 13353. Before applying, : 0/ Set the AcqCreateItem pref to "on receiving" 1/ Create an order with 1 item 2/ On Catalog "Delete the items" of the biblio record. In the DB I saw an aqorders_items's record and a deleteditems's record with the same itemnumber entry. Then I've applied the patches against master 3.17.00.060 HEAD 13353. I upgraded the DB as required. [In the DB the aqorders_items record was deleted as required.] Everything is OK as required by the test plan. So I pass the patches to "Signed Off" status.
Created attachment 34017 [details] [review] Bug 12555: Add a FK on aqorders_items.itemnumber
Created attachment 34157 [details] [review] [PASSED QA] Bug 12555: Call DelItem on cancelling a receipt On cancelling a receipt, the items should be deleted using the C4::Items::DelItem subroutine in order to update zebra and sent the items in the deleteditems table. The entries in aqorders_items will automatically deleted by the DBMS (on delete cascade). Test plan: 0/ Set the AcqCreateItem pref to "on receiving" 1/ Create an order with some items 2/ Receive the items 3/ Note the itemnumber in the items table 4/ Cancel the receipt 5/ Verify the itemnumbers are present in the deleteditems table and the items have been deleted. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 34158 [details] [review] [PASSED QA] Bug 12555: Call DelItem on cancelling a receipt On cancelling a receipt, the items should be deleted using the C4::Items::DelItem subroutine in order to update zebra and sent the items in the deleteditems table. The entries in aqorders_items will automatically deleted by the DBMS (on delete cascade). Test plan: 0/ Set the AcqCreateItem pref to "on receiving" 1/ Create an order with some items 2/ Receive the items 3/ Note the itemnumber in the items table 4/ Cancel the receipt 5/ Verify the itemnumbers are present in the deleteditems table and the items have been deleted. Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz> Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 34159 [details] [review] [PASSED QA] Bug 12555: Add a FK on aqorders_items.itemnumber The previous patch assumed that deleting an item will delete the entry in aqorders_items. But the FK did not exist. Looking at the code, we never join aqorders_items and deleteditems, so I think we can add this constraint. Before adding this constraint, we need to delete all entries in aqorders_items if the item has already been deleted. Signed-off-by: Paola Rossi <paola.rossi@cineca.it> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
I am not sure about deleting in aqorders_items when the items are deleted/the order is cancelled. We keep the record, the items and even the cancelled order - so not sure we should delete what ties them together. For library statistics often the items that were deleted/removed from the collection are queried/counted - so the library would want to differentiate between an item that really existed in the library or didn't.
The aqorders_items entries are already removed on canceling a receipt. Before this patch, the items were deleted too. With the patch, they are moved to deleteditems. I am not sure to understand what is the info you loose, could you please detail?
I am afraid I won't be able to tell which deleted items were non-existant in the library - belonged to a cancelled order - and which items were removed for other reasons (damaged, outdated). Biblionumber might not be enough, as you might have cancelled an order and at a later point in time the existing items get thrown out because the book gets outdated/replaces by a newer edition.
I think for me it would make sense to either delete both - the item and the linking, or keep both.
What can I do to unblock this one?
I think this is one of the places, where our split tables create problems again as there is no clean way of doing what I think would be right with FKs. Scenario: Items are created on order. Order is created with items, order is cancelled. The items are deleted. But what is next? 1) Items are moved to deleteditems Pro: - More information is kept. - Usual way of doing things in Koha. Contra: - If the aqorders_items link is removed, you won't be able to exclude these items from statistics effectively (no way to tell they never were in the library. This especially creates a problem for German libraries that have to keep an 'inventory book' - being able to tell when which item was bought and when it was removed from the library etc. 2) Items are really deleted Pro: - Items never really existed, deleting them makes sense. - They won't get counted for statistics. Contra: - Unusual as Koha usually moves to deleteditems. At the moment I think I'd prefer 2 as the ideal solution 1) with keeping the link seems not easily/cleanly achievable without merging our items tables.