Bug 15962 - Currency deletion doesn't correctly identify currencies in use
Summary: Currency deletion doesn't correctly identify currencies in use
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL: /cgi-bin/koha/admin/currency.pl?op=de...
Keywords:
Depends on: 15084
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-02 17:29 UTC by Owen Leonard
Modified: 2016-12-05 21:25 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 15962: Add messages on the currency admin page (1.75 KB, patch)
2016-03-08 10:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 15962: Block the currency deletion if used (3.32 KB, patch)
2016-03-08 10:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15962: Add messages on the currency admin page (1.85 KB, patch)
2016-03-11 16:33 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 15962: Block the currency deletion if used (3.43 KB, patch)
2016-03-11 16:33 UTC, Owen Leonard
Details | Diff | Splinter Review
[PASSED QA] Bug 15962: Block the currency deletion if used (3.44 KB, patch)
2016-03-21 06:31 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 15962: Add messages on the currency admin page (1.87 KB, patch)
2016-03-21 06:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 15962: Block the currency deletion if used (3.44 KB, patch)
2016-03-21 06:33 UTC, Katrin Fischer
Details | Diff | Splinter Review
[3.22.x] Bug 15962: Block the currency deletion if used (2.93 KB, patch)
2016-04-14 12:41 UTC, Julian Maurice
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Owen Leonard 2016-03-02 17:29:33 UTC
currency.pl tries to see if a currency is active before deleting it:

    my $total_row = $dbh->selectrow_hashref(
        'select count(*) as total from aqbooksellers where currency=?',
        {}, $curr );

However it doesn't look like the currency column in aqbooksellers is used (anymore?). It looks to me like there are two columns to check: List prices aqbooksellers.listprice) and invoice price (aqbooksellers.invoiceprice).

If you try to delete a currency which is in use as a list or invoice price in a vendor record the process will fail silently:

currency.pl: DBD::mysql::db do failed: Cannot delete or update a parent row: a foreign key constraint fails (`koha`.`aqbasket`, CONSTRAINT `aqbasket_ibfk_3` FOREIGN KEY (`basketgroupid`) REFERENCES `aqbasketgroups` (`id`) ON UPDATE CASCADE)
Comment 1 Katrin Fischer 2016-03-03 06:40:58 UTC
Nice catch! I think you are right about currency being no longer in use - I had to track down the deletion problem a little while ago.
Comment 2 Jonathan Druart 2016-03-08 10:51:20 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2016-03-08 10:51:28 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-03-08 10:52:14 UTC
You will need the last patch on bug 15084 (typo currency vs currency_code) to test this patch.
Comment 5 Owen Leonard 2016-03-09 13:27:36 UTC
In testing the "active" status for a currency keeps getting unset:

1. Edit active currency.
2. Save without making changes (the "active" checkbox is checked).
3. The currency is saved without the "active" status and I get a warning, "No active currency is defined."
Comment 6 Jonathan Druart 2016-03-11 11:25:41 UTC
(In reply to Owen Leonard from comment #5)
> In testing the "active" status for a currency keeps getting unset:
> 
> 1. Edit active currency.
> 2. Save without making changes (the "active" checkbox is checked).
> 3. The currency is saved without the "active" status and I get a warning,
> "No active currency is defined."

Confirmed, but on master :)
Comment 7 Jonathan Druart 2016-03-11 11:59:29 UTC
(In reply to Jonathan Druart from comment #6)
> (In reply to Owen Leonard from comment #5)
> > In testing the "active" status for a currency keeps getting unset:
> > 
> > 1. Edit active currency.
> > 2. Save without making changes (the "active" checkbox is checked).
> > 3. The currency is saved without the "active" status and I get a warning,
> > "No active currency is defined."
> 
> Confirmed, but on master :)

Fixed on bug 16053.
Comment 8 Owen Leonard 2016-03-11 16:33:38 UTC Comment hidden (obsolete)
Comment 9 Owen Leonard 2016-03-11 16:33:44 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2016-03-21 06:31:34 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2016-03-21 06:33:40 UTC
Created attachment 49375 [details] [review]
[PASSED QA] Bug 15962: Add messages on the currency admin page

Bug 15084 rewrote the currency admin page but forgot to add the messages
block.

Test plan:
Add/update/delete vendors.
You should get information messages.

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2016-03-21 06:33:43 UTC
Created attachment 49376 [details] [review]
[PASSED QA] Bug 15962: Block the currency deletion if used

A currency should not be deleted if used by a vendor or a basket.

Test plan:
1/ Add a new currency
2/ Create a vendor using this currency
3/ Create a basket using this currency
4/ Try to delete the currency
5/ Delete the basket
6/ Try to delete the currency
7/ Delete the vendor
8/ Delete the currency

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Brendan Gallagher 2016-03-21 22:56:38 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 14 Julian Maurice 2016-04-14 12:41:27 UTC
Created attachment 50232 [details] [review]
[3.22.x] Bug 15962: Block the currency deletion if used

Patch for 3.22.x
Comment 15 Julian Maurice 2016-04-14 12:41:58 UTC
Patch pushed to 3.22.x, will be in 3.22.6
Comment 16 Frédéric Demians 2016-04-27 15:33:39 UTC
Pushed to 3.22.x, will be in 3.20.11.