Bug 5680

Summary: When deleting an order, delete attached items, and if applicable biblio
Product: Koha Reporter: Chris Cormack <chris>
Component: AcquisitionsAssignee: Christophe Croullebois <christophe.croullebois>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: enhancement    
Priority: PATCH-Sent (DO NOT USE) CC: katrin.fischer, koha.sekjal, m.de.rooy, magnus, mathsabypro, paul.poulain
Version: 3.4   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=7162
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 8149, 5574    
Attachments: Patch
new ment
this one is ok
screenshot of basket with test cases
The lat one
screenshot with new error messages
Bug 5680: Order cancelling improvement : delete attached items & biblio if avalaible
Bug 5680: Follow Up for Order cancelling improvement : the same for parcel.pl
screenshot of messages on basket summary page
screenshot of messages on order receive page
[SIGNED-OFF] Bug 5680: Follow Up for Order cancelling improvement : the same for parcel.pl
Passed QA patch
Passed QA followup patch

Description Chris Cormack 2011-02-02 01:25:48 UTC
- all items attached to the order are deleted
    - if there is no more items, then the biblio is deleted as well
Comment 1 Chris Cormack 2011-02-02 01:27:39 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2011-03-13 22:50:43 UTC
Works as advertised.

Tested:
- Deleting from order with no remaining items and remaining items
- Cancelling order from shipment with no remaining items and remaining items

The biblio record was only deleted, when no other items were left.
The bibio record remained and only the ordered items were deleted
when there were other items attached to the record.
Comment 3 Paul Poulain 2011-03-14 13:54:38 UTC
Katrin, chris & me had a strong discussion (by mail) about the proposed behaviour. Chris says Koha should not delete the biblio silently when orders are cancelled (ie: if the librarian has modified the biblio, the deletion can be annoying), I said it can, because it delete only biblio without anymore items, so there's no need to keep the biblio (ie: the booksellers said "can't give you the book" no need to keep it in the catalogue")

Can you give your opinion on this question functionnally speaking ?
Comment 4 Katrin Fischer 2011-03-14 14:04:01 UTC
In most cases you will want to delete the biblio after the order was cancelled/deleted. If you don't do that automatically a lot of biblios without items will be created, because people forget to search and delete them. 

But I can see there might be circumstances where you want to keep the record to order it from another vendor, order it later, keep it with a note, that it's out of print etc.

I think I would like to see a prompt asking the librarian for confirmation before the record is deleted.
Comment 5 Marcel de Rooy 2011-03-14 14:09:47 UTC
I would prefer:
Do not delete biblio's when there are items. Warn instead. (Note that we have AcqCreateItem set to cataloging. Normally, there should not be items.) 
If no items, delete on confirmation. (This could be the first confirmation; in that case the confirmation for deleting an order should be clear about that.) 

If supplier1 does not have the book, could supplier2 have it? In that case it could be helpful not to delete right-away..
Comment 6 Paul Poulain 2011-03-14 14:19:09 UTC
Katrin, I think we (chris & me) can agree on this:
I think I would like to see a prompt asking the librarian for confirmation
before the record is deleted.

Marcel:
I don't think the biblio is deleted if you don't have AcqCreateItem on ordering
Comment 7 Katrin Fischer 2011-03-14 14:23:37 UTC
I also believe it only deletes the items attached to the order. I had tested with a record that had different items, created from acq and cataloging. Only the acq items were deleted.
Comment 8 Chris Cormack 2011-04-02 08:21:52 UTC
Yep, all this needs is a follow up patch to check for reserves on the biblio, or subscriptions on the biblio. And then warn the user if this is the case.

The scenario of wanting to order the biblio from another supplier was not the one I was concerned about. Its more if a user has placed a hold on it, the hold with either be left dangling or deleted without the user being notified.

So not just a confirmation prompt, I want it to check and make sure the biblio has nothing attached to it (another order, a serial subscription a hold etc) and then tell the librarian this.
Comment 9 Paul Poulain 2011-05-20 10:34:13 UTC
Christophe will work on a patch for that: when the user clicks on "delete order", an AJAX will return the question that must be asked.

2 possibilities :
POSSIBILITY 1 :
* no more items on the biblio
* no other order (usefull if AcqCreateItems is set to "recieving" or "cataloguing" !)
 => ask "delete order only / delete order and biblio / cancel"

POSSIBILITY 2:
 otherwise
=> ask "delete order / cancel"
Comment 10 Katrin Fischer 2011-05-20 10:44:12 UTC
+1
Comment 11 Christophe Croullebois 2011-05-25 07:17:38 UTC Comment hidden (obsolete)
Comment 12 Christophe Croullebois 2011-05-25 07:51:23 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2011-06-15 12:55:44 UTC
Created attachment 4480 [details]
screenshot of basket with test cases

1) TYPO: Wording of confirmation message and link
Currently: Are you sure you want to delete this order with his biblio ?
I am not sure if we should use 'biblio' or better say record. Also 'his' should be 'its'. Suggestion: Are you sure you want to delete this catalog record and order? Both links have long descriptions and are close to each other - but I have no idea, how to improve that for a clearer display.

2) OK: Link to delete order and biblio is only shown, when there are no additional items on the record. Deleting the order only deletes the on order items.

3) PROBLEM: Deleting a record from the order silently deletes subscription attached to that biblio. You can loose a lot of data this way.

4) PROBLEM: One big reason to create items on orders is to enable holds on these items. If you delete a order and biblio with holds on the on order items, the hold is silently deleted.

5) OK: Deleting order, items and biblio with no holds, subscriptions and other items attached to the record. Works.
Comment 14 Christophe Croullebois 2011-09-16 10:24:16 UTC
Katrin,
1) TYPO: OK, right, sry for my poor english
3) PROBLEM: right I have forforgoten this, I propose to include a test to see if subscription exists. In this case no second link.
4) PROBLEM: it's more complex, I  propose to test if there are holds on items in order, and if so, to use notices for emailing to borrower the hold cancelation.
I think this option needs to speak about, what is your opinion about my suggest ?
Because I think even if we keep biblio we can't keep ghost items, so its better to advise the borrower as we can do when the order is receveid.
Waiting suggests.
Comment 15 Katrin Fischer 2011-09-19 10:48:16 UTC
Hi Christophe, 

thx for commenting :)

4) Showing no link is ok for me. I am trying to imagine a good way to show the reason why the link is not available, but I have no good idea right now. If you don't show the link for existing subscriptiosn I will sign-off.

5) I like the idea for hold cancellation messages. But to sign-off on this patch I think it would be ok if you check for existing holds, give an alert and refuse to delete the order in that case. So the librarian can cancel them manually and inform the borrower if they want to.
Comment 16 Katrin Fischer 2011-10-07 05:54:28 UTC
Hi Christophe, can you update on the status?
Comment 17 Christophe Croullebois 2011-10-09 18:54:49 UTC Comment hidden (obsolete)
Comment 18 Christophe Croullebois 2011-10-09 19:38:17 UTC
So Katrin,
I hope this one is ok, must be tested deeply, because multiple cases.
I have taken care of the difference between an hold on catalog record and an hold on a specific item.
In some cases you can't delete an order, in other cases you can't delete the order and catalog's record but the order can be deleted.
On mouse over you have explanations with count.
More, some lines of warnings with count are shown depending of the case.
The ergonomy can be discussed, I have chosen these viewing options to have a technical structure steady.
Good tests.
Waiting your comments.
Comment 19 Katrin Fischer 2011-10-09 22:01:27 UTC
Created attachment 5814 [details]
screenshot with new error messages

Testing...
Comment 20 Katrin Fischer 2011-10-09 22:45:08 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2011-10-09 22:48:20 UTC
Thx Christophe for your work on this! I think we are almost there!
Comment 22 Christophe Croullebois 2011-10-11 07:32:33 UTC
Katrin,
many thx for your tests.
Oups, I have forgotten the basket summary page...
I'll work on a follow up.
Right, it's better with your changes, visualy it's clearer.
So  with your help, quite to have something.
Comment 23 Christophe Croullebois 2011-10-15 20:50:17 UTC Comment hidden (obsolete)
Comment 24 Christophe Croullebois 2011-10-15 20:58:42 UTC
Hello Katrin,
I hope this part is ok.
I have created one more column in parcel.pl.
Of course it was not the same script, so I hope I haven't forgotten something.
Good tests.
Thx
Comment 25 Katrin Fischer 2011-10-17 05:50:22 UTC
Created attachment 5914 [details]
screenshot of messages on basket summary page
Comment 26 Katrin Fischer 2011-10-17 05:53:33 UTC
Created attachment 5915 [details]
screenshot of messages on order receive page
Comment 27 Katrin Fischer 2011-10-17 06:09:50 UTC Comment hidden (obsolete)
Comment 28 Ian Walls 2011-10-18 20:38:36 UTC
Created attachment 5964 [details] [review]
Passed QA patch

Rebased on current HEAD (minor conflict with new Analytics subroutine export)
Comment 29 Ian Walls 2011-10-18 20:39:34 UTC
Created attachment 5965 [details] [review]
Passed QA followup patch

Followup patch passed QA; minor change to template language.  Was showing items remaining, but talking about holds remaining, so I made that consistent.
Comment 30 Ian Walls 2011-10-18 20:41:06 UTC
I've tested and reviewed these patches, and found them sound.  Some minor updates to apply to current HEAD, and fix a small language issue in a tooltip.

Some new C4::Items subroutines here... could be very helpful in our de-nesting efforts be removing the need to include other modules than C4::Items.  Will investigate in the future.

Passed QA
Comment 31 Chris Cormack 2011-10-19 03:59:48 UTC
Pushed, thank you Katrin and Ian for the good testing, and Christophe for all your hard work writing the card.