Bug 12555 - The DelItem subroutine should be called on canceling a receipt
Summary: The DelItem subroutine should be called on canceling a receipt
Status: In Discussion
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 11744 12583
Blocks: 5334
  Show dependency treegraph
 
Reported: 2014-07-11 09:48 UTC by Jonathan Druart
Modified: 2020-03-26 14:47 UTC (History)
4 users (show)

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


Attachments
Bug 12555: Call DelItem on cancelling a receipt (2.07 KB, patch)
2014-07-16 13:13 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12555: Call DelItem on cancelling a receipt (2.12 KB, patch)
2014-08-08 03:10 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 12555: Add a FK on aqorders_items.itemnumber (3.23 KB, patch)
2014-08-20 10:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12555: Call DelItem on cancelling a receipt (2.12 KB, patch)
2014-11-06 08:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12555: Add a FK on aqorders_items.itemnumber (2.61 KB, patch)
2014-11-06 08:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12555: Call DelItem on cancelling a receipt (2.24 KB, patch)
2014-11-28 15:09 UTC, Paola Rossi
Details | Diff | Splinter Review
Bug 12555: Add a FK on aqorders_items.itemnumber (2.70 KB, patch)
2014-11-28 15:09 UTC, Paola Rossi
Details | Diff | Splinter Review
[PASSED QA] Bug 12555: Call DelItem on cancelling a receipt (2.24 KB, patch)
2014-12-05 15:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 12555: Call DelItem on cancelling a receipt (2.24 KB, patch)
2014-12-05 15:55 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 12555: Add a FK on aqorders_items.itemnumber (2.70 KB, patch)
2014-12-05 15:55 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2014-07-11 09:48:30 UTC
Bug 11744 needs some additional work.
The routine _cancel_items_receipt should call C4::Items::DelItem.
Comment 1 Jonathan Druart 2014-07-16 13:13:45 UTC Comment hidden (obsolete)
Comment 2 Chris Cormack 2014-08-08 03:10:14 UTC Comment hidden (obsolete)
Comment 3 Katrin Fischer 2014-08-17 19:47:26 UTC
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.
Comment 4 Jonathan Druart 2014-08-19 15:49:31 UTC
(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?
Comment 5 Jonathan Druart 2014-08-20 10:05:19 UTC Comment hidden (obsolete)
Comment 6 Liz Rea 2014-11-06 06:44:22 UTC
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
Comment 7 Jonathan Druart 2014-11-06 08:49:27 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2014-11-06 08:49:29 UTC Comment hidden (obsolete)
Comment 9 Paola Rossi 2014-11-20 17:32:13 UTC
>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.
Comment 10 Jonathan Druart 2014-11-21 10:22:08 UTC
(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?
Comment 11 Paola Rossi 2014-11-28 15:09:13 UTC Comment hidden (obsolete)
Comment 12 Paola Rossi 2014-11-28 15:09:44 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2014-12-05 15:51:28 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2014-12-05 15:55:04 UTC
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>
Comment 15 Kyle M Hall 2014-12-05 15:55:10 UTC
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>
Comment 16 Katrin Fischer 2014-12-06 08:12:16 UTC
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.
Comment 17 Jonathan Druart 2014-12-08 09:41:35 UTC
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?
Comment 18 Katrin Fischer 2014-12-08 09:58:32 UTC
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.
Comment 19 Katrin Fischer 2014-12-08 09:59:23 UTC
I think for me it would make sense to either delete both - the item and the linking, or keep both.
Comment 20 Jonathan Druart 2015-09-22 10:43:13 UTC
What can I do to unblock this one?
Comment 21 Katrin Fischer 2017-08-15 19:46:44 UTC
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.