Bug 5790 - Deleting a biblio should alert/fail if there are existent holds
Summary: Deleting a biblio should alert/fail if there are existent holds
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: 3.10
Hardware: All All
: P5 - low normal (vote)
Target Milestone: ---
Assignee: Kyle M Hall
QA Contact: Jonathan Druart
URL:
Whiteboard:
Keywords:
: 1959 (view as bug list)
Depends on:
Blocks: 8149 9565
  Show dependency treegraph
 
Reported: 2011-02-21 22:18 UTC by Liz Rea
Modified: 2014-05-26 21:04 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
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
Bug 5790 - Prevent deletion of records with holds (8.88 KB, patch)
2013-01-30 17:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5790 - Prevent deletion of records with holds (9.01 KB, patch)
2013-01-30 23:22 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 5790 - Prevent deletion of records with holds - QA Followup (1.37 KB, patch)
2013-01-31 14:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5790 - Prevent deletion of records with holds (10.19 KB, patch)
2013-01-31 14:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5790 - Prevent deletion of records with holds (10.06 KB, patch)
2013-01-31 14:39 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5790 - Prevent deletion of records with holds (9.99 KB, patch)
2013-02-04 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5790 - Prevent deletion of records with holds (9.87 KB, patch)
2013-02-04 20:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5790 - Prevent deletion of records with holds (9.87 KB, patch)
2013-02-04 20:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5790 - Prevent deletion of records with holds (10.15 KB, patch)
2013-02-04 20:30 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 5790 - Prevent deletion of records with holds - QA Followup (1.54 KB, patch)
2013-02-05 13:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5790 - Prevent deletion of records with holds - QA Followup (1.63 KB, patch)
2013-02-06 21:34 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 5790 - Prevent deletion of records with holds (10.21 KB, patch)
2013-02-07 10:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5790 - Prevent deletion of records with holds - QA Followup (1.70 KB, patch)
2013-02-07 10:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 5790 - Prevent deletion of records with holds [ 3.10.x ] (10.37 KB, patch)
2013-02-11 14:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 5790 - Prevent deletion of records with holds - QA Followup [ 3.10.x ] (1.69 KB, patch)
2013-02-11 14:26 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 Liz Rea 2011-02-21 22:18:33 UTC
Currently Koha will delete a biblio record if there are existent serial subscriptions and patron holds. It would be better if it either reported them out and asked for confirmation (It's ok that we delete these, right?) or failed outright with an error (Hey, there's still stuff here! Delete X holds or subscriptions and try again).
Comment 1 Liz Rea 2011-02-22 15:43:26 UTC
Here is a report that will show you bibs that have no items, but have orphan holds. 

To delete the holds (or notify patrons that the hold is being deleted due to no items in the collection), you will need to create a dummy item to see the holds.

SELECT DISTINCT biblio.biblionumber, biblio.title FROM biblio LEFT JOIN items
ON
biblio.biblionumber = items.biblionumber LEFT JOIN reserves ON
biblio.biblionumber = reserves.biblionumber WHERE items.itemnumber IS NULL and
reserves.borrowernumber IS NOT NULL
Comment 2 Liz Rea 2011-02-22 15:43:47 UTC
btw that report was written by Jared Camins of ByWater Solutions.
Comment 3 Nicole C. Engard 2011-06-09 16:18:05 UTC
Any ideas on how to fix this issue?  Seems like it would be a big deal to most libraries.

Nicole
Comment 4 Liz Rea 2011-08-05 18:44:30 UTC
as extra gravy, it would be cool if when you deleted a bib that has holds on it, an email was sent to the patrons with holds saying "You had placed a hold on <title>. This item has been removed from the catalog, and so we are canceling your hold. Please contact your local library for help getting this item either from our system or other methods of ILL."
Comment 5 Chris Cormack 2011-08-25 23:42:26 UTC
I am working on a fix for this, implementation will be

Koha will refuse to delete a biblio with holds or a serial subscription attached, and report that you need to deal with those first.

Then the library can decide what they want to do.
Comment 6 Chris Cormack 2011-08-25 23:49:52 UTC
Currently, we just delete the subscriptions silently

    # We delete attached subscriptions
    my $subscriptions = &C4::Serials::GetFullSubscriptionsFromBiblionumber($biblionumber);
    foreach my $subscription (@$subscriptions) {
        &C4::Serials::DelSubscription( $subscription->{subscriptionid} );
    }

It doesnt check for holds at all.
Comment 7 Katrin Fischer 2012-02-19 12:14:52 UTC
This is still an issue on current master.
Comment 8 Katrin Fischer 2012-02-19 12:15:51 UTC
*** Bug 1959 has been marked as a duplicate of this bug. ***
Comment 9 Gaetan Boisson 2012-09-13 16:08:41 UTC
This is still an issue it seems.

Also i think if what we do now is to delete the subscription silently, it's not so bad : what should happen in my opinion is that the user whould be warned that his action will also affect X subscirptions. But if that is what they want to do then it's a lot more simple to just delete the biblio than to go through all the subsriptions first, and then back to the biblio.

So i would say, on the subscription side, just add a warning.

On the holds side, well that's a bit more complicated ;)
Comment 10 Kyle M Hall 2013-01-30 17:18:23 UTC Comment hidden (obsolete)
Comment 11 Liz Rea 2013-01-30 23:22:34 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2013-01-31 09:47:40 UTC
QA comments:
The patch works great but I have 3 small remarks:
1/ the cat-toolbar include file is included in 2 others tt files (catalogue/labeledMARCdetail.tt and catalogue/imageviewer.tt)
2/ instead of adding a new id "disabled2", it would be better to create a class ?
3/ qa tools complains:
 * koha-tmpl/intranet-tmpl/prog/en/includes/cat-toolbar.inc                 FAIL
    forbidden patterns          FAIL
      The patch introduces a forbidden pattern: simple-quote string (62)
      The patch introduces a forbidden pattern: tabulation character (36)
      The patch introduces a forbidden pattern: simple-quote string (37)
      The patch introduces a forbidden pattern: simple-quote string (60)
      The patch introduces a forbidden pattern: simple-quote string (35)
      The patch introduces a forbidden pattern: simple-quote string (39)
  Not blocker, it is not introduced with this patch.

Marked as Failed QA for 1.
Comment 13 Jonathan Druart 2013-01-31 09:49:56 UTC
(In reply to comment #12)
> QA comments:
> 2/ instead of adding a new id "disabled2", it would be better to create a
> class ?

Hum... forget that. It does not work
Comment 14 Kyle M Hall 2013-01-31 13:50:58 UTC
(In reply to comment #13)
> (In reply to comment #12)
> > QA comments:
> > 2/ instead of adding a new id "disabled2", it would be better to create a
> > class ?
> 
> Hum... forget that. It does not work

Yeah, I spent a good amount of time trying to just use a class, but no luck. That was the only way to accomplish the correct behavior.
Comment 15 Kyle M Hall 2013-01-31 14:27:28 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2013-01-31 14:35:29 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2013-01-31 14:39:24 UTC Comment hidden (obsolete)
Comment 18 Kyle M Hall 2013-01-31 14:40:06 UTC
Removed Liz's SO, as the patch has been modified. Liz, could you retest this new patch?
Comment 19 Katrin Fischer 2013-02-01 10:41:39 UTC
Hi Kyle,

I think we will also want to check for existing orders (checkouts, holds, subscriptions, orders). In acquisitions you can delete records when none of those conditions is met - maybe the code could be reused?

Haven't looked at the code, just wanted to leave a short note.
Comment 20 Kyle M Hall 2013-02-01 11:47:33 UTC
(In reply to comment #19)
> Hi Kyle,
> 
> I think we will also want to check for existing orders (checkouts, holds,
> subscriptions, orders). In acquisitions you can delete records when none of
> those conditions is met - maybe the code could be reused?
> 
> Haven't looked at the code, just wanted to leave a short note.

This code probably wouldn't help too much for those cases, but they would not be difficult to implement anyway.
Comment 21 Gaetan Boisson 2013-02-01 11:57:48 UTC
Yeah, this looks like a nice fix!

As Katrin said the problem exists in a few other places.

This bug describes the problem with orders in acquisitions :
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=5781

Ideally checkouts and subscriptions should also be dealt with.
Comment 22 Liz Rea 2013-02-02 00:22:53 UTC
Gah!
Comment 23 Kyle M Hall 2013-02-04 13:09:43 UTC Comment hidden (obsolete)
Comment 24 Kyle M Hall 2013-02-04 20:24:13 UTC Comment hidden (obsolete)
Comment 25 Kyle M Hall 2013-02-04 20:25:36 UTC Comment hidden (obsolete)
Comment 26 Liz Rea 2013-02-04 20:30:21 UTC Comment hidden (obsolete)
Comment 27 Jonathan Druart 2013-02-05 09:04:35 UTC
I got an error :

Undefined subroutine &main::GetReservesFromBiblionumber called at /home/koha/src/catalogue/moredetail.pl line 218.

+ Please fix the simple-quote string:
 * koha-tmpl/intranet-tmpl/prog/en/includes/cat-toolbar.inc         FAIL
    forbidden patterns          FAIL
      The patch introduces a forbidden pattern: simple-quote string (43)


Marked as Failed QA.
Comment 28 Kyle M Hall 2013-02-05 13:51:45 UTC Comment hidden (obsolete)
Comment 29 Liz Rea 2013-02-06 21:34:00 UTC Comment hidden (obsolete)
Comment 30 Jonathan Druart 2013-02-07 10:50:27 UTC
Now the feature is integrated on all pages where the catalogue toolbar is displayed.

Marked as Passed QA.
Comment 31 Jonathan Druart 2013-02-07 10:50:51 UTC Comment hidden (obsolete)
Comment 32 Jonathan Druart 2013-02-07 10:50:59 UTC Comment hidden (obsolete)
Comment 33 Jared Camins-Esakov 2013-02-07 12:34:10 UTC
This patch has been pushed to master. The patch does not address the problem with subscriptions, so I created bug 9565. A fix for that bug would be welcomed.
Comment 34 Chris Cormack 2013-02-08 23:46:51 UTC
Does not apply to 3.10.x please submit a new patch for 3.10.x if it is needed
Comment 35 Kyle M Hall 2013-02-11 14:25:35 UTC
Created attachment 15242 [details] [review]
Bug 5790 - Prevent deletion of records with holds [ 3.10.x ]

Test Plan:
1) Apply patch
2) Create a record
3) Create an item for the record
3) Place a hold on the bib
4) Attempt to 'Delete all items', you should recieve
   an error message stating to delete all holds before
   deleting all items.

Also, it is possible to get into a situation where a record has
holds but no items. In this situation, it is not possible to
view/delete the holds without adding an item back to the record.
In this case, attempting to delete the bib causes a warning, but
does not prevent deletion.

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Passes tests - do note that it was a design decision to leave the delete links clickable even though they are grey.
The reasoning is that librarians will want to be able to know *why* they cannot delete a given item or bib - I like this.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 36 Kyle M Hall 2013-02-11 14:26:06 UTC
Created attachment 15243 [details] [review]
Bug 5790 - Prevent deletion of records with holds - QA Followup [ 3.10.x ]

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
fixes qa concerns, feature still works

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 37 Chris Cormack 2013-02-12 09:54:29 UTC
Just updating for myself, so I push it to stable.
Comment 38 Chris Cormack 2013-04-24 08:42:39 UTC
Pushed to 3.10.x will be in 3.10.6