Bug 11744 - cancel receipt doesn't work for one item
Summary: cancel receipt doesn't work for one item
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Target Milestone: ---
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 12555
  Show dependency treegraph
 
Reported: 2014-02-11 21:10 UTC by Marjorie Barry-Vila
Modified: 2015-06-04 23:33 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
cancel receipt (5.23 MB, application/x-shockwave-flash)
2014-02-11 21:10 UTC, Marjorie Barry-Vila
Details
Bug 11744: Add regression tests (2.93 KB, patch)
2014-02-19 12:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11744: Cancel a receipt does not delete items created on receiving (3.40 KB, patch)
2014-02-19 12:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11744: Add regression tests (3.02 KB, patch)
2014-02-26 20:13 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 11744: Cancel a receipt does not delete items created on receiving (3.46 KB, patch)
2014-02-26 20:13 UTC, sandboxes@biblibre.com
Details | Diff | Splinter Review
Bug 11744: A receipt should not be cancelled if holds exist (2.83 KB, patch)
2014-03-19 12:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11744: Add regression tests (3.18 KB, patch)
2014-07-11 10:16 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 11744: Cancel a receipt does not delete items created on receiving (3.53 KB, patch)
2014-07-11 12:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11744: A receipt should not be cancelled if holds exist (2.87 KB, patch)
2014-07-11 12:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
[PASSED QA] Bug 11744: Add regression tests (3.26 KB, patch)
2014-07-11 12:15 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 Marjorie Barry-Vila 2014-02-11 21:10:35 UTC
Created attachment 25201 [details]
cancel receipt

Hi,

My test plan:
-AcqCreateItem = receiving an order
-order 1 item
-receive the order, item is created
-cancel receipt
-the order is pending but the item is not deleted

If i receive a second time the order, an other item is created.

Someone can test it?


It's difficult to speak in english for me so i make a video without sound...

Thanks,

Marjorie
Comment 1 Jacek Ablewicz 2014-02-19 10:54:45 UTC
(In reply to Marjorie Barry-Vila from comment #0)

> Someone can test it?

Yes, I can confirm there is a problem with CancelReceipt(). It will delete (one or more, it doesn't matter) items associated with received order only if the given order is a partial receive (and the parent order is not fully received yet).

Plus, in those rare cases when it is actually deleting the items, it's currently not doing a very good job - it is removing item records directly from mysql table, but it does NOT flag biblio record for Zebra reindexing after the deletion.

Until recently, it also used to delete the items if given order was a transfer to another basket - but it was deleting the order record itself too, which was not all that good (see bug 11549).
Comment 2 Jonathan Druart 2014-02-19 12:38:46 UTC
I reproduced against master.
I will provide a patch soon.
Comment 3 Jonathan Druart 2014-02-19 12:46:33 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2014-02-19 12:46:38 UTC Comment hidden (obsolete)
Comment 5 Marjorie Barry-Vila 2014-02-24 13:36:02 UTC
Hi,
I tested the patch on 3.12.7 and it works fine.

Regards,

Marjorie
Comment 6 Jonathan Druart 2014-02-24 13:47:18 UTC
(In reply to Marjorie Barry-Vila from comment #5)
Hi Marjorie,
Could you test this patch against the master branch using a sandbox?
Comment 7 Marjorie Barry-Vila 2014-02-24 14:40:37 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to Marjorie Barry-Vila from comment #5)
> Hi Marjorie,
> Could you test this patch against the master branch using a sandbox?


It's the first time i am testing a patch in a sandbox.
I tested on a sandbox (http://pro.test6.biblibre.com) and it works fine too.


Marjorie
Comment 8 sandboxes@biblibre.com 2014-02-26 20:13:12 UTC
Patch tested with a sandbox, by marjorie barry-vila <marjorie.barry-vila@ccsr.qc.ca>
Comment 9 sandboxes@biblibre.com 2014-02-26 20:13:53 UTC Comment hidden (obsolete)
Comment 10 sandboxes@biblibre.com 2014-02-26 20:13:56 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2014-03-11 11:06:26 UTC
I think I found a problem with the way the item is deleted - it seems it's missing some of the checks it shoudl do before deleting.

What I did:

- received 1 out of 2 items ordered
- placed a hold on the record
- undid the receipt
- item was deleted, hold didn't show up any longer... but:
- repeated the receipt - the hold showed up again as it hadn't been cancelled before, but was invisible from the GUI.

Next I tried with a checkout:

- checked out the created item
- undid the receipt
- the item was not deleted (which is good), but there was also no warning

I wonder if this is more like a missing feature than a real bug, as it might need some more thought to make it completely work. My thought is, that some of the same checks for deleting an order with items (when using AcqCreateItem = on order is used) might also apply here. I can think of holds and issues for now. For a hold we could raise a warning, that it will be cancelled if there ar no other items on the record. For an issue the item should be left and maybe undoing the receipt be denied? Not sure what would happen if you undid it and then later received it again - would it create a second item?

I also tested for AcqCreateItem = on order that the items would not be touched, which is good :) But something for another bug - maybe we shoudl put a note, that the changes made to the items on receive won't be undone by undoing the receipt?
Comment 12 Jonathan Druart 2014-03-19 10:24:28 UTC
(In reply to Katrin Fischer from comment #11)
Katrin,
I agree that some tests are missing but in my opinion it is outside the scope of this bug. Don't you think?
Comment 13 Katrin Fischer 2014-03-19 10:29:11 UTC
The problem I see here is that right now the feature is not there, but no damage is done. The item is not deleted and you can do it manually, the data will remain consistent. With the patch, you can delete the item, but the hold is not cancelled and invisible from the interface - you will never know that there is a lost hold on a record with no items - I think that is a data inconsistency we should avoid.
Comment 14 Jonathan Druart 2014-03-19 12:05:09 UTC Comment hidden (obsolete)
Comment 15 Katrin Fischer 2014-03-31 19:48:49 UTC
Hi Jonathan, 

I have tested this again and it works as described. But I am a bit worried still - before the patch you could undo a receipt any time, being aware that the items would not be deleted. Now you can undo and the items will be deleted, which is nice. But if there is a hold on the record, you can't undo until you cancel he hold. 

This could be a problem for a very popular book with multiple holds and multiple copies outside of the order. Then you'd have to cancel all of them, to reenter them again after undoing the receipt.

Could showing an alert: 'There is at least 1 hold on this record, please delete items manually' be an option?

Or a check on remaining items? If no items remain you shouldn't be able to undo the receipt without fixing it. If there is an item and it's not an item level hold on your ordered item, you could.

I am not feeling very sure about this, maybe we could get the bug reporter's (Marjorie?) and some others opinion on this?
Comment 16 Jonathan Druart 2014-04-01 07:43:29 UTC
(In reply to Katrin Fischer from comment #15)
> Could showing an alert: 'There is at least 1 hold on this record, please
> delete items manually' be an option?

Did you see I added a 'title' on the "Can't cancel receipt" span?
It displays "Cannot cancel receipt of this order line because at least one reservation exists on the records." on mouse over.
Comment 17 Marjorie Barry-Vila 2014-04-02 15:07:33 UTC
(In reply to Katrin Fischer from comment #15)
> Hi Jonathan, 
> 
> I have tested this again and it works as described. But I am a bit worried
> still - before the patch you could undo a receipt any time, being aware that
> the items would not be deleted. Now you can undo and the items will be
> deleted, which is nice. But if there is a hold on the record, you can't undo
> until you cancel he hold. 
> 
> This could be a problem for a very popular book with multiple holds and
> multiple copies outside of the order. Then you'd have to cancel all of them,
> to reenter them again after undoing the receipt.
> 
> Could showing an alert: 'There is at least 1 hold on this record, please
> delete items manually' be an option?
> 
> Or a check on remaining items? If no items remain you shouldn't be able to
> undo the receipt without fixing it. If there is an item and it's not an item
> level hold on your ordered item, you could.
> 
> I am not feeling very sure about this, maybe we could get the bug reporter's
> (Marjorie?) and some others opinion on this?

Hi Katrin,
I tested it with new patch of Jonathan and for me it works.
Test plan:
-create one item available
-order one item on the same record
-receive item ordered
-do a reservation on item available
-you can undo the receipt 

It is normal for me to delete a hold of a item which is not realy received. But if a hold is on a item already received in a other basket, you do not have to delete the hold.

I am sorry, my english is bad. Do you understand the same thing that me?

Marjorie
Comment 18 Kyle M Hall 2014-04-11 11:14:04 UTC
(In reply to Jacek Ablewicz from comment #1)
> (In reply to Marjorie Barry-Vila from comment #0)
> 
> > Someone can test it?
> 
> Yes, I can confirm there is a problem with CancelReceipt(). It will delete
> (one or more, it doesn't matter) items associated with received order only
> if the given order is a partial receive (and the parent order is not fully
> received yet).
> 
> Plus, in those rare cases when it is actually deleting the items, it's
> currently not doing a very good job - it is removing item records directly
> from mysql table, but it does NOT flag biblio record for Zebra reindexing
> after the deletion.
> 
> Until recently, it also used to delete the items if given order was a
> transfer to another basket - but it was deleting the order record itself
> too, which was not all that good (see bug 11549).

This patch set does not appear to update zebra on deletion. Also, I wonder if it would be best to call DelItem from the item deletion to take care of that. Using DelItem would also cause the item to be moved to deleteditems rather than being irrevocably destroyed. If these are not issues or I am missing something, please reset the status.

Other than that everything looks good!
Comment 19 Katrin Fischer 2014-04-11 11:16:18 UTC
Hi Kyle, thx for taking a look at this - I won't be able to do much QA before the beginning of next week. Woudl DelItem also help with the hold issues?
Comment 20 Jonathan Druart 2014-04-11 11:43:39 UTC
(In reply to Kyle M Hall from comment #18)
> This patch set does not appear to update zebra on deletion. Also, I wonder
> if it would be best to call DelItem from the item deletion to take care of
> that. Using DelItem would also cause the item to be moved to deleteditems
> rather than being irrevocably destroyed. If these are not issues or I am
> missing something, please reset the status.

I am not sure this should be done on this report. It is a major one and I tried to change the code as few as possible.
Note that I totally agree with your remark, we should call DelItem here.
Comment 21 Jonathan Druart 2014-07-11 09:39:49 UTC
QAers, could you reconsider this bug please?
This major is blocked for 3 months.
Comment 22 Jonathan Druart 2014-07-11 10:16:48 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2014-07-11 10:18:34 UTC
The UT patch is rebased (conflict with commit Bug 11699: ModReceiveOrder should take a hashref in parameter),
Comment 24 Kyle M Hall 2014-07-11 12:14:40 UTC
Created attachment 29626 [details] [review]
[PASSED QA] Bug 11744: Cancel a receipt does not delete items created on receiving

If items is created on receiving, cancel a receipt should delete them.
The code only manage the case if the order is a child of another order
(partial).

To reproduce:
1/ Set AcqCreateItem to receiving
2/ Order one or more item(s)
3/ Receive the order and verify the item is created
4/ Cancel the receipt
5/ The item is not deleted

Test plan:
1/ Apply this patch and do again previous steps. The item should not be
deleted at step 5.
2/ Set AcqCreateItem to ordering and verify the item is not deleted.

Signed-off-by: marjorie barry-vila <marjorie.barry-vila@ccsr.qc.ca>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Kyle M Hall 2014-07-11 12:14:58 UTC
Created attachment 29627 [details] [review]
[PASSED QA] Bug 11744: A receipt should not be cancelled if holds exist

Test plan:
1/ Set acqCreateItem pref to on receiving
2/ Receive 1 of 2 items ordered
3/ Place a hold on the record
4/ Verify you are not able to cancel the receipt.

This is not the best way to do that but it is the easy one.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 26 Kyle M Hall 2014-07-11 12:15:03 UTC
Created attachment 29628 [details] [review]
[PASSED QA] Bug 11744: Add regression tests

Before the patch:
 prove t/db_dependent/Acquisition/CancelReceipt.t
returns 1 failure.
After the patch, the 2 tests pass.

Signed-off-by: marjorie barry-vila <marjorie.barry-vila@ccsr.qc.ca>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 27 Katrin Fischer 2014-07-11 12:18:14 UTC
I lost track of this a bit, should we have a new bug report for comment 18-20?
Comment 28 Jonathan Druart 2014-07-11 13:02:02 UTC
(In reply to Katrin Fischer from comment #27)
> I lost track of this a bit, should we have a new bug report for comment
> 18-20?

Bug 12555 already opened and linked ;)
Comment 29 Katrin Fischer 2014-07-11 13:08:45 UTC
Ah! Thx!
Comment 30 Tomás Cohen Arazi 2014-07-14 14:33:25 UTC
Patches pushed to master.

Thanks Jonathan!
Comment 31 Jonathan Druart 2014-07-16 13:14:20 UTC
(In reply to Jonathan Druart from comment #28)
> (In reply to Katrin Fischer from comment #27)
> > I lost track of this a bit, should we have a new bug report for comment
> > 18-20?
> 
> Bug 12555 already opened and linked ;)

Patch submitted.