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
(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).
I reproduced against master. I will provide a patch soon.
Created attachment 25444 [details] [review] 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.
Created attachment 25445 [details] [review] 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.
Hi, I tested the patch on 3.12.7 and it works fine. Regards, Marjorie
(In reply to Marjorie Barry-Vila from comment #5) Hi Marjorie, Could you test this patch against the master branch using a sandbox?
(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
Patch tested with a sandbox, by marjorie barry-vila <marjorie.barry-vila@ccsr.qc.ca>
Created attachment 25682 [details] [review] 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>
Created attachment 25683 [details] [review] 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>
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?
(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?
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.
Created attachment 26447 [details] [review] 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.
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?
(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.
(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
(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!
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?
(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.
QAers, could you reconsider this bug please? This major is blocked for 3 months.
Created attachment 29622 [details] [review] 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>
The UT patch is rebased (conflict with commit Bug 11699: ModReceiveOrder should take a hashref in parameter),
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>
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>
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>
I lost track of this a bit, should we have a new bug report for comment 18-20?
(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 ;)
Ah! Thx!
Patches pushed to master. Thanks Jonathan!
(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.