Bug 14518 - Deleting records with a hold (and no item) does not work
Summary: Deleting records with a hold (and no item) does not work
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Blou
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-10 13:50 UTC by Blou
Modified: 2017-06-14 22:11 UTC (History)
7 users (show)

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


Attachments
Bug 14518 - Allow deleting notices with reservations (and no item) after confirmation (1.57 KB, patch)
2015-07-10 13:59 UTC, Blou
Details | Diff | Splinter Review
[SIGNED OFF] Bug 14518 - Allow deleting recordss with reservations (and no item) after confirmation (1.63 KB, patch)
2015-10-02 20:56 UTC, Nick Clemens
Details | Diff | Splinter Review
[PASSED QA] Bug 14518 - Allow deleting records with holds (and no item) after confirmation (2.18 KB, patch)
2015-10-03 17:53 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Blou 2015-07-10 13:50:12 UTC
There's a popup asking for confirmation, but the Yes is ignored.

The change to the code is very simple.  In intranet-tmpl/prog/en/includes/cat-toolbar.inc:

54         else if ( holdcount > 0 ) {
55             is_confirmed = confirm( _("%s holds(s) for this ...
56         } else {
57             is_confirmed = confirm(_("Are you sure you want ...
58         }
59         if (is_confirmed) {
60             if ( count > 0 || holdcount > 0 ){    <<== but we just confirmed!!
61                 return false;                     <<== Nooooooo!!!
62             } else {
63                 window.location="/cgi-bin/koha/cataloguing/addbiblio.pl?op=delete&amp;biblionumber=[% biblionumber %]";
64             }
 
Our proposed fix is just to remove that extra if.
Comment 1 Blou 2015-07-10 13:59:27 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2015-07-12 21:55:27 UTC
Hi Blou,

please be careful - notices are letters in Koha, but I think you are talking about the bibliographic records?

From reading the problem description I think we got a different problem here - Koha should not allow to place holds on records without items ever. So maybe we need to find out why this happens first.
Comment 3 Blou 2015-07-13 12:13:14 UTC
Well, the library seems to use it, so in that sense, I think Koha should allow it.  :)

I think the usage is that they publicize future arrivals before getting them.  They created the records, did their orders, told their "clients" and allowed them to reserve them.  It does make sense to me as a user, but I'm no librarian.
Comment 4 Katrin Fischer 2015-07-13 12:15:34 UTC
It should not be possible - because the hold might never be filled. I think they are misusing a bug - and that could turn into a big problem for them on updating.

For me it's not possible to place holds for records without items from the OPAC - I have no idea, how they are doing it.
Comment 5 Blou 2015-07-13 13:34:29 UTC
I clarified with Francois:  This happened due to a migration to Koha.  So you're right, it shouldn't happen in Koha.

THAT said, we can argue about handling such case or not, but Koha is clearly misbehaving on the prompt: it asks if we want to do it anyway, then it totally ignores the result.
Comment 6 Katrin Fischer 2015-07-13 13:47:34 UTC
Yep - but we need a new test plan - I think the only way you can do it is make an item... place a hold and then delete the item using SQL or something like that :)
Comment 7 Maxime Beaulieu 2015-07-21 20:00:11 UTC
(In reply to Katrin Fischer from comment #6)
> Yep - but we need a new test plan

I. Setup
Create the following if needed:
1) A patron for placing holds and checkouts
2) A test bibliographic record
3) An item for the record. Set the barcode to "DELETEME"

II. Corrupting the Database
1) Checkout the item "DELETEME"
2) Place a record level hold on the test record (next available item)
3) Checkin the item "DELETEME"
4) MySQL: DELETE FROM items WHERE barcode="DELETEME";

III. Deleting the record.
1) On the test record's details page: Edit -> Delete record
2) Click 'yes' in the prompt
3) Nothing happens.

IV. Fixing the bug
1) Apply the patch
2) Repeat step III to delete the record
3) Record is deleted
Comment 8 Aleisha Amohia 2015-07-22 05:16:37 UTC
Hi I wasn't able to reproduce this bug, I was able to delete the record just fine
Comment 9 Maxime Beaulieu 2015-07-22 12:34:28 UTC
(In reply to Aleisha Amohia from comment #8)
> Hi I wasn't able to reproduce this bug, I was able to delete the record just
> fine

Hi, did you complete step II correctly?
After this step. The record should have a hold placed on it, but no items.

When placing the hold for the record, did you check the "Place a hold on the next available item" checkbox?

The item needs to be deleted directy in the database using a mysql query ( see III.4 )
Comment 10 Aleisha Amohia 2015-07-24 04:09:12 UTC
I probably messed up somewhere, I'll retest now!
Comment 11 Aleisha Amohia 2015-07-27 04:18:18 UTC
I tested again and still couldn't reproduce the bug. Can someone else test this please? I must be testing wrong somewhere but I'm sure I'm following the instructions correctly
Comment 12 Nick Clemens 2015-10-02 20:56:39 UTC Comment hidden (obsolete)
Comment 13 Katrin Fischer 2015-10-03 17:53:15 UTC
Please always make sure that the commit message reflects the changes made to the patch - in this case the updated test plan. I have amended the commit message in QA.
Comment 14 Katrin Fischer 2015-10-03 17:53:49 UTC
Created attachment 43082 [details] [review]
[PASSED QA] Bug 14518 - Allow deleting records with holds (and no item) after confirmation

Cleaning up records with no item but holds is impossible since
the code does not consider the result from the confirmation popup.

This situation happens to our client in their (annual) cleanup.

I. Setup
Create the following if needed:
1) A patron for placing holds and checkouts
2) A test bibliographic record
3) An item for the record. Set the barcode to "DELETEME"

II. Corrupting the Database
1) Checkout the item "DELETEME"
2) Place a record level hold on the test record (next available item)
3) Checkin the item "DELETEME"
4) MySQL: DELETE FROM items WHERE barcode="DELETEME";

III. Deleting the record.
1) On the test record's details page: Edit -> Delete record
2) Click 'yes' in the prompt
3) Nothing happens.

IV. Fixing the bug
1) Apply the patch
2) Repeat step III to delete the record
3) Record is deleted

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Note: It's not possible in Koha to place a hold on a
record with items, so the effect described here is more
likely to be the result of a migration issue or another
bug. But doesn't hurt to provide a way out :)
Comment 15 Tomás Cohen Arazi 2015-10-05 15:41:07 UTC
Patch pushed to master.

Thanks Remi and congrats for your commit!