Bug 13943 - Cancelling the deletion process of an item deletes item anyway
Summary: Cancelling the deletion process of an item deletes item anyway
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
: 14032 (view as bug list)
Depends on:
Blocks:
 
Reported: 2015-04-02 11:54 UTC by Katrin Fischer
Modified: 2016-06-21 21:38 UTC (History)
8 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:


Attachments
Bug 13943: Prevent the deletion of items if the user cancels (5.12 KB, patch)
2015-07-13 16:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13943: Prevent the deletion of items if the user cancels (7.20 KB, patch)
2015-07-14 08:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13943: Prevent the deletion of items if the user cancels (7.26 KB, patch)
2015-07-19 02:35 UTC, Nick Clemens
Details | Diff | Splinter Review
[PASSED QA] Bug 13943: Prevent the deletion of items if the user cancels (7.34 KB, patch)
2015-09-01 22:01 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 Katrin Fischer 2015-04-02 11:54:55 UTC
This is kind of an odd bug, as we have not been able to reproduce the problem consistently.

- search for a record with items
- go to the detail page
- edit > edit items
- click the delete link at the beginning of the items table on top
- in the alert window 'do you really want...' click 'cancel'
- the page reloads and the item is deleted anyway

A library reported the problem to us and after I couldn't reproduce it, we did a screenshare to see if we could find what we were doing differently. The first item we deleted showed the wrong behaviour, after that we were not able to reproduce it agan.

Koha version is 3.18.5.1
Comment 1 David Cook 2015-04-21 06:03:57 UTC
*** Bug 14032 has been marked as a duplicate of this bug. ***
Comment 2 David Cook 2015-04-21 06:06:17 UTC
I've definitely experienced this as well, although in my case the problem was with Chrome and IE while it worked in Firefox.

After I reported the bug and gave up on it, I tried again in Chrome and IE... and found that they were working as expected! No buggy behaviour!

So I don't know... my boss suggested that we make the confirm_deletion function more simple or remove it all together in favour of the following inline code:

onclick="return confirm('Are you sure you want to delete this item?')"

or

onclick="return confirm(_('Are you sure you want to delete this item?'))"

Perhaps that's a thought... when I tested the first option, it worked fine.
Comment 3 Fridolin SOMERS 2015-04-21 07:36:24 UTC
(In reply to David Cook from comment #2)

> So I don't know... my boss suggested that we make the confirm_deletion
> function more simple or remove it all together in favour of the following
> inline code:
> 
> onclick="return confirm('Are you sure you want to delete this item?')"
> 
> or
> 
> onclick="return confirm(_('Are you sure you want to delete this item?'))"
> 
> Perhaps that's a thought... when I tested the first option, it worked fine.

Be careful :
1/ JavaScript in HTML attributes is not translatable
2/ Always use double quotes inside _() function because translation may add a single quote
Comment 4 David Cook 2015-04-21 23:29:22 UTC
(In reply to Fridolin SOMERS from comment #3)
> Be careful :
> 1/ JavaScript in HTML attributes is not translatable
> 2/ Always use double quotes inside _() function because translation may add
> a single quote

Good to know :). I don't have any plans to work on this anyway, but good to know :).
Comment 5 David Cook 2015-04-22 02:11:17 UTC
So Liz Rea is a genius! She pointed out that the problem only arises if the screen is using the result browser!

For instance:

http://192.168.1.61:2112/cgi-bin/koha/cataloguing/additem.pl?biblionumber=1&searchid=scs_1429666452994

Deletes item after "Cancel".

http://192.168.1.61:2112/cgi-bin/koha/cataloguing/additem.pl?biblionumber=1

Doesn't delete item after "Cancel".

--

The cause is in "koha-tmpl/intranet-tmpl/js/browser.js":

$('a[href*="biblionumber="]').click(function (ev) {
                        ev.preventDefault();
                        window.location = $(this).attr('href') + '&searchid=' + me.searchid;
                    });

Basically, this is rewriting the URL for all the other links on the page, so that when you click... "Edit", "Delete", "Normal", etc. you still have that searchid parameter.

Now... it seems to me that the thing to do is either be more sophisticated with the Javascript magic (i.e. parse the href, find the query string, add the key=value appropriately, then set the href for the element before it browser acts on it), or... handle it server-side.
Comment 6 David Cook 2015-04-22 02:12:23 UTC
Follow-up... that bit of Javascript will do a window.location regardless of the outcome of the "confirm_deletion(); return false;" onclick stuff. :(
Comment 7 David Cook 2015-04-22 02:13:21 UTC
(In reply to David Cook from comment #5)
> Basically, this is rewriting the URL 

Well, rewriting the URL and redirecting to it...
Comment 8 Katrin Fischer 2015-05-06 13:25:39 UTC
This is growing into a bigger problem - we also got a report of the same phenomenon in acquisitions now when cancelling an order.
Comment 9 David Cook 2015-05-07 00:53:34 UTC
(In reply to Katrin Fischer from comment #8)
> This is growing into a bigger problem - we also got a report of the same
> phenomenon in acquisitions now when cancelling an order.

Interesting... does the result browser Javascript get called there? That's what's causing the problem elsewhere.
Comment 10 Jonathan Druart 2015-07-13 15:13:00 UTC
Can we get a step-by-step plan to reproduce the issue please?
Does it happen with the "en" templates?
What versions of chromium, all?
Is there a JS error?
Comment 11 Katrin Fischer 2015-07-13 15:26:59 UTC
Hi Joubu,

try the following:
- Do a search with mulitple results
- Go to the detail page (make sure results browser shows up!)
- Use the "Edit items" link from the toolbar
- Delete item
- Choose "cancel"
- Watch item getting deleted

I just tested this in Firefox - hope it's going to happen consequently now at least...
Comment 12 Jonathan Druart 2015-07-13 16:38:18 UTC Comment hidden (obsolete)
Comment 13 David Cook 2015-07-13 23:28:04 UTC
Jonathan, have you tested this? 

At a glance, it looks like the main delete button will work because the href attribute points to "/cgi-bin/koha/mainpage.pl" instead of "/cgi-bin/koha/cataloguing/additem.pl?op=delitem&biblionumber=[% biblionumber %]&itemnumber=[% item_loo.itemnumber %]", but I don't know why you'd point at "/cgi-bin/koha/mainpage.pl"... I think that would be a surprising result when successfully deleting an item, no?

I imagine the inline delete link will work since browser.js should try to bind the problematic click handler using the selector $('a[href*="biblionumber="]') before the inline links get created. 

Anyway... just some thoughts...
Comment 14 Jonathan Druart 2015-07-14 08:42:30 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2015-07-14 08:44:27 UTC
(In reply to David Cook from comment #13)
> Jonathan, have you tested this? 
> 
> At a glance, it looks like the main delete button will work because the href
> attribute points to "/cgi-bin/koha/mainpage.pl" instead of
> "/cgi-bin/koha/cataloguing/additem.pl?op=delitem&biblionumber=[%
> biblionumber %]&itemnumber=[% item_loo.itemnumber %]", but I don't know
> why you'd point at "/cgi-bin/koha/mainpage.pl"... I think that would be a
> surprising result when successfully deleting an item, no?

Sorry, it was a stupid test.
I was happy, it works (the deletion did not work when I canceled the deletion).
So I tested, but not completely.

> I imagine the inline delete link will work since browser.js should try to
> bind the problematic click handler using the selector
> $('a[href*="biblionumber="]') before the inline links get created. 
> 
> Anyway... just some thoughts...

Thanks for that, I did not catch that yesterday! It was very useful.
The last patch should work as expected.
Comment 16 Nick Clemens 2015-07-19 02:35:10 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2015-09-01 22:01:38 UTC
Created attachment 42184 [details] [review]
[PASSED QA] Bug 13943: Prevent the deletion of items if the user cancels

On the edit items page, there is some weird JS code: if the user clicks
on the delete link and then cancel, the item is deleted anyway.

It's caused by the following JS code in browser.js
  $('a[href*="biblionumber="]').click(function (ev) {
      ev.preventDefault();
      window.location = $(this).attr('href') + '&searchid=' + me.searchid;
  });

Test plan:
- Do a search with multiple results
- Go to the detail page (make sure results browser shows up!)
- Use the "Edit items" link from the toolbar
- Delete an item (try both way)
- Choose "cancel"
- Delete an item (try both way) and confirm the deletion
You should see the browser after the deletion and the item should have
been deleted correctly.
- Edit an item (try both way)
You should see the browser (did not work before this patch)

Note: Before this patch, the 2 first columns didn't contain the
edit/delete item links, now it's only the 1st one.

Signed-off-by: Nick Clemens <nick@quecheelibrary.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Much better!
Comment 18 Tomás Cohen Arazi 2015-09-02 17:47:45 UTC
Patch pushed to master.

Thanks Jonathan!
Comment 19 Chris Cormack 2015-09-22 01:35:26 UTC
pushed to 3.20.x will be in 3.20.4
Comment 20 Liz Rea 2015-09-22 02:26:19 UTC
Pushed to 3.18.x will be in 3.18.11