Bug 15084

Summary: Move the currency related code to Koha::Acquisition::Currenc[y|ies]
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: brendan, colin.campbell, josef.moravec, katrin.fischer, kyle, m.de.rooy
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16017
Change sponsored?: --- Patch complexity: ---
Who signed the patch off: Text to go in the release notes:
Version(s) released in:
Bug Depends on: 15083    
Bug Blocks: 15449, 15323, 15962, 15987, 16053, 16089, 16227, 19328    
Attachments: Bug 15084: Add Koha::Acquisition::Currenc[y|ies] classes
Bug 15084: Make sure the previous active currency is marked as inactive
Bug 15084: use Koha::Acquisition::Currenc[y|ies] in admin/currency
Bug 15084: DB Changes - add currency.archived and FK on aqorders.currency
Bug 15084: Replace C4::Budgets::GetCurrencies with Koha::Acquisition::Currencies->search
Bug 15084: Remove C4::Budgets::ConvertCurrency
Bug 15084: Add tests for Koha::Acquisition::Currenc[y|ies]
Bug 15084: Add Koha::Acquisition::Currenc[y|ies] classes
Bug 15084: Make sure the previous active currency is marked as inactive
Bug 15084: use Koha::Acquisition::Currenc[y|ies] in admin/currency
Bug 15084: DB Changes - add currency.archived and FK on aqorders.currency
Bug 15084: Replace C4::Budgets::GetCurrencies with Koha::Acquisition::Currencies->search
Bug 15084: Remove C4::Budgets::ConvertCurrency
Bug 15084: Add tests for Koha::Acquisition::Currenc[y|ies]
Bug 15084: DB Changes - add currency.archived and FK on aqorders.currency
Bug 15084: DBIx::Class - Shema changes
Bug 15084: Add Koha::Acquisition::Currenc[y|ies] classes
Bug 15084: Make sure the previous active currency is marked as inactive
Bug 15084: use Koha::Acquisition::Currenc[y|ies] in admin/currency
Bug 15084: Replace C4::Budgets::GetCurrencies with Koha::Acquisition::Currencies->search
Bug 15084: Remove C4::Budgets::ConvertCurrency
Bug 15084: Add tests for Koha::Acquisition::Currenc[y|ies]
Bug 15084: DB Changes - add currency.archived and FK on aqorders.currency
Bug 15084: DBIx::Class - Shema changes
Bug 15084: Add Koha::Acquisition::Currenc[y|ies] classes
Bug 15084: Make sure the previous active currency is marked as inactive
Bug 15084: use Koha::Acquisition::Currenc[y|ies] in admin/currency
Bug 15084: Replace C4::Budgets::GetCurrencies with Koha::Acquisition::Currencies->search
Bug 15084: Remove C4::Budgets::ConvertCurrency
Bug 15084: Add tests for Koha::Acquisition::Currenc[y|ies]
Bug 15084: DB Changes - add currency.archived and FK on aqorders.currency
Bug 15084: DBIx::Class - Shema changes
Bug 15084 [QA Followup] - Clean up test currencies after unit tests
Bug 15084: Add Koha::Acquisition::Currenc[y|ies] classes
Bug 15084: Make sure the previous active currency is marked as inactive
Bug 15084: use Koha::Acquisition::Currenc[y|ies] in admin/currency
Bug 15084: Replace C4::Budgets::GetCurrencies with Koha::Acquisition::Currencies->search
Bug 15084: Remove C4::Budgets::ConvertCurrency
Bug 15084: Add tests for Koha::Acquisition::Currenc[y|ies]
Bug 15084: DB Changes - add currency.archived and FK on aqorders.currency
Bug 15084: DBIx::Class - Shema changes
Bug 15084 [QA Followup] - Clean up test currencies after unit tests
Bug 15084: Fix conflict with bug 15446 (type vs _type)
Bug 15084 [QA Followup] - Update Schema files
Bug 15084 [QA Followup] - Fix new uses of GetCurrency
Bug 15084: typo currency vs currency_code
Bug 15084: [QA Follow-up] Update kohastructure
Bug 15084: [QA Follow-up] Correct field length of currency

Description Jonathan Druart 2015-10-29 14:49:45 UTC

    
Comment 1 Jonathan Druart 2015-10-29 15:47:26 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-10-29 15:47:29 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-10-29 15:47:33 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2015-10-29 15:47:35 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2015-10-29 15:47:38 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-10-29 15:47:42 UTC Comment hidden (obsolete)
Comment 7 Jonathan Druart 2015-10-29 15:47:45 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2015-12-15 10:27:24 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-12-15 10:27:27 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-12-15 10:27:30 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2015-12-15 10:27:33 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2015-12-15 10:27:36 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2015-12-15 10:27:39 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2015-12-15 10:27:43 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2015-12-15 10:28:36 UTC
Rebased against master
Comment 16 Josef Moravec 2015-12-16 13:21:58 UTC
Jonathan, there is a typo in atomic update sql file, 

update table aqorders set currency=NULL where currency="";

should be

update aqorders set currency=NULL where currency="";

So id did the database update manually.
y
On the currency management page I can't see if the currency is archived (although the column in table is here)

On editing previous order, I can see the right price, but in currency select dropdown there is current active currency selected.

Everything else works as expected.
Comment 17 Jonathan Druart 2015-12-17 10:58:47 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2015-12-17 10:58:55 UTC Comment hidden (obsolete)
Comment 19 Jonathan Druart 2015-12-17 11:00:22 UTC
(In reply to Josef Moravec from comment #16)
> Jonathan, there is a typo in atomic update sql file, 
> 
> update table aqorders set currency=NULL where currency="";
> 
> should be
> 
> update aqorders set currency=NULL where currency="";

Thanks for the catch!

> On the currency management page I can't see if the currency is archived
> (although the column in table is here)
> 
> On editing previous order, I can see the right price, but in currency select
> dropdown there is current active currency selected.

This is because the Schema file (
Koha/Schema/Result/Currency.pm) was not updated.
When you test a patch modifying the DB structure, make sure the schema is updated too (using misc/devel/update_dbix_class_files.pl).
I have added the schema change in the last patch.
Comment 20 Josef Moravec 2015-12-18 10:18:13 UTC Comment hidden (obsolete)
Comment 21 Josef Moravec 2015-12-18 10:18:25 UTC Comment hidden (obsolete)
Comment 22 Josef Moravec 2015-12-18 10:18:33 UTC Comment hidden (obsolete)
Comment 23 Josef Moravec 2015-12-18 10:18:42 UTC Comment hidden (obsolete)
Comment 24 Josef Moravec 2015-12-18 10:18:49 UTC Comment hidden (obsolete)
Comment 25 Josef Moravec 2015-12-18 10:18:56 UTC Comment hidden (obsolete)
Comment 26 Josef Moravec 2015-12-18 10:19:04 UTC Comment hidden (obsolete)
Comment 27 Josef Moravec 2015-12-18 10:19:09 UTC Comment hidden (obsolete)
Comment 28 Kyle M Hall 2016-01-22 18:38:26 UTC Comment hidden (obsolete)
Comment 29 Kyle M Hall 2016-01-22 18:38:43 UTC Comment hidden (obsolete)
Comment 30 Kyle M Hall 2016-01-22 18:38:51 UTC Comment hidden (obsolete)
Comment 31 Kyle M Hall 2016-01-22 18:38:57 UTC Comment hidden (obsolete)
Comment 32 Kyle M Hall 2016-01-22 18:39:03 UTC Comment hidden (obsolete)
Comment 33 Kyle M Hall 2016-01-22 18:39:10 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2016-01-22 18:39:15 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2016-01-22 18:39:21 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2016-01-22 18:39:27 UTC Comment hidden (obsolete)
Comment 37 Brendan Gallagher 2016-03-02 22:48:54 UTC
Didn't apply cleanly - I know because of the moving target that master is.  Please rebase.  Once it's done - leave me an @later in #koha and I'll jump right on it.  Thank you so much for the time and bother of rebasing.

Brendan
Comment 38 Jonathan Druart 2016-03-03 08:42:00 UTC
Created attachment 48594 [details] [review]
Bug 15084: Add Koha::Acquisition::Currenc[y|ies] classes

The first step of this patch set is to move the business logic code from
admin/currencies.pl to Koha::Acquisition::Currenc[y|ies].

Then a foreign key will be created on aqorders.currency as we want to
assure data integrity. Note that a aqbooksellers.currency also exists
but is never used. It could be removed in another bug report.
This update has to care about possible breaking relation. For instance
an old order has been made using a currency which is now deleted. To be
sure the update process won't break a new column currency.archived is
created and leave open the door for a further improvement (marked a
currency as archived from the interface: this won't be provided by this
enhancement).
These archived currencies won't appear on the interface for newly
created items (order/suggestion).

Once this is done it becomes easy to remove the subroutine from
C4::Budgets: GetCurrencies and GetCurrency.
ConvertCurrency will also be removed but is not used anymore.

Note that none of these subroutines were covered by tests. Now they are.

Test plan:
0/ Don't apply this patch
1/ Create a temporary currency and an order using it
2/ Remove the currency from your CLI
3/ Apply the patch and execute the DB entry
4/ Note that the currency is inserted and marked as archived.
5/ Edit the previous order and confirm that the correct currency is
still selected.
It won't never be displayed anywhere else.
6/ Test the admin script: on admin/currency.pl add/remove/edit
currencies.
You could not have 2 active currencies at the same time.
7/ Then on the different scripts of the acquisition module, focus on the
currencies values and create a new order, receive it.
Create/edit a suggestion

Other changes must be checked by the QAer.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 39 Jonathan Druart 2016-03-03 08:42:04 UTC
Created attachment 48595 [details] [review]
Bug 15084: Make sure the previous active currency is marked as inactive

On inserting an active currency, others should be marked as inactive.
We can only have 1 active currency at the same time.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 40 Jonathan Druart 2016-03-03 08:42:08 UTC
Created attachment 48596 [details] [review]
Bug 15084: use Koha::Acquisition::Currenc[y|ies] in admin/currency

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 41 Jonathan Druart 2016-03-03 08:42:12 UTC
Created attachment 48597 [details] [review]
Bug 15084: Replace C4::Budgets::GetCurrencies with Koha::Acquisition::Currencies->search

Most part of the code here is unnecessary complex. We should selected
the currency if it is selected, that's all :)

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 42 Jonathan Druart 2016-03-03 08:42:15 UTC
Created attachment 48598 [details] [review]
Bug 15084: Remove C4::Budgets::ConvertCurrency

This subroutine is unused and can be removed.
There is no trace left of currency in C4::Budgets.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 43 Jonathan Druart 2016-03-03 08:42:19 UTC
Created attachment 48599 [details] [review]
Bug 15084: Add tests for Koha::Acquisition::Currenc[y|ies]

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 44 Jonathan Druart 2016-03-03 08:42:22 UTC
Created attachment 48600 [details] [review]
Bug 15084: DB Changes - add currency.archived and FK on aqorders.currency

See the comment for details.

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 45 Jonathan Druart 2016-03-03 08:42:25 UTC
Created attachment 48601 [details] [review]
Bug 15084: DBIx::Class - Shema changes

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 46 Jonathan Druart 2016-03-03 08:42:28 UTC
Created attachment 48602 [details] [review]
Bug 15084 [QA Followup] - Clean up test currencies after unit tests

Test currency left behind will cuase next run of the unit test to fail
as well as leaving db cruft.

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 47 Jonathan Druart 2016-03-03 08:42:32 UTC
Created attachment 48603 [details] [review]
Bug 15084: Fix conflict with bug 15446 (type vs _type)
Comment 48 Jonathan Druart 2016-03-03 08:42:50 UTC
Patches rebased.
Comment 49 Brendan Gallagher 2016-03-03 21:16:42 UTC
Pushed to MAster - Should be in the May 2016 Release.  Thanks!  (Let me know if I missed something here - but I'm pretty sure we are good to go)
Comment 50 Marcel de Rooy 2016-03-04 16:29:18 UTC
Are we still missing some things here?
I see some problems related to aqorder and currency when running TestBuilder.t
Looking further, I also saw a foreign key for currency in aqorders that does not appear in kohastructure (although that is not the solution yet).
Also we need some DBIx updates for Koha/Schema/Result; is that a RM action?
Comment 51 Kyle M Hall 2016-03-04 19:42:13 UTC
Created attachment 48703 [details] [review]
Bug 15084 [QA Followup] - Update Schema files

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 52 Kyle M Hall 2016-03-07 13:33:44 UTC
Created attachment 48747 [details] [review]
Bug 15084 [QA Followup] - Fix new uses of GetCurrency

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 53 Jonathan Druart 2016-03-07 14:17:17 UTC
(In reply to Kyle M Hall from comment #52)
> Created attachment 48747 [details] [review] [review]
> Bug 15084 [QA Followup] - Fix new uses of GetCurrency
> 
> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Hum, bug 15987?
Comment 54 Jonathan Druart 2016-03-08 10:13:53 UTC
@RM: The schema has not been regenerated.
Comment 55 Jonathan Druart 2016-03-08 10:19:34 UTC
Created attachment 48772 [details] [review]
Bug 15084: typo currency vs currency_code

The DB column is currency.currency not currency.currency_code.

Test plan:
Delete a currency
Without this patch, you will get a warning in the logs:
No method currency_code! at
/home/koha/src/koha-tmpl/intranet-tmpl/prog/en/modules/admin/currency.tt
line 148.
And the currency won't be deleted.

With this patch, it will!

It also changes the value of the delete op.
Comment 56 Marcel de Rooy 2016-03-08 13:55:08 UTC
Created attachment 48783 [details] [review]
Bug 15084: [QA Follow-up] Update kohastructure

Adds two db revs to kohastructure
Comment 57 Marcel de Rooy 2016-03-08 16:05:17 UTC
Aarrggg
This was hard to find:
The foreign key constraint (in AQorder) will sometimes fail on you:
Why?
aqorder:
| currency                | varchar(3)    | YES  | MUL | NULL              |
currency:
| currency  | varchar(10) | NO   | PRI |                   |                

Length is not the SAME !!
Comment 58 Kyle M Hall 2016-03-08 16:27:23 UTC
(In reply to Marcel de Rooy from comment #57)
> Aarrggg
> This was hard to find:
> The foreign key constraint (in AQorder) will sometimes fail on you:
> Why?
> aqorder:
> | currency                | varchar(3)    | YES  | MUL | NULL              |
> currency:
> | currency  | varchar(10) | NO   | PRI |                   |                
> 
> Length is not the SAME !!

So I take it we need a dbrev to update aqorder.currency to varchar(10) as well?
Comment 59 Katrin Fischer 2016-03-08 19:54:15 UTC
Ithink it would be the easiest fix that way.
Comment 60 Brendan Gallagher 2016-03-08 22:05:32 UTC
Pushed the last two patches (48772 and 48783)  Thanks
Comment 61 Marcel de Rooy 2016-03-09 12:46:22 UTC
Created attachment 48858 [details] [review]
Bug 15084: [QA Follow-up] Correct field length of currency

Bug 15084 added a FK constraint while the fields in the database are not
in sync as to length. This will produce errors when using currency codes
longer than three characters. Probably you won't, but nobody stopped
users from entering EURO or DOLLAR etc. Not to speak about TestBuilder
too.

This patch corrects the database revision for aqorders in updatedatabase,
because we need to change the field length before adding the FK constraint.
It also updates other currency fields < 10 chars (via atomicupdate).
RM: So please add that dbrev too in updatedatabase.

Note that another report should deal with adding missing constraints on
the currency code in suggestions and aqbooksellers.
Also note that the aqorder fields listprice and invoiceprice refer to
currency. Imo these are very poor names for currency codes; you should
never call something a price when you mean a currency code!

Similar changes are applied to kohastructure.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested the db revisions.
Comment 62 Marcel de Rooy 2016-03-09 12:49:49 UTC
(In reply to Brendan Gallagher from comment #60)
> Pushed the last two patches (48772 and 48783)  Thanks

Thanks.
Don't forget the DBIx updates.
I added a follow-up for what I discovered after the first kohastructure discrepancy, namely the currency length problem (3 vs 10 chars).
I took the liberty to correct the dbrev for the currency constraint in aqorders directly.
Also added an atomicupdate for a subsequent dbrev.
Comment 63 Marcel de Rooy 2016-03-09 12:56:29 UTC
(In reply to Marcel de Rooy from comment #61)
> Note that another report should deal with adding missing constraints on
> the currency code in suggestions and aqbooksellers.
> Also note that the aqorder fields listprice and invoiceprice refer to
> currency. Imo these are very poor names for currency codes; you should
> never call something a price when you mean a currency code!

Opened bug 16017 for Jonathan :)
Comment 64 Owen Leonard 2016-03-09 13:16:29 UTC
I tried to apply this in order to test Bug 15962 but there is a conflict:

CONFLICT (add/add): Merge conflict in Koha/Acquisition/Currencies.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 15084: Add Koha::Acquisition::Currenc[y|ies] classes
Comment 65 Marcel de Rooy 2016-03-09 13:18:19 UTC
Resetting status
Comment 66 Owen Leonard 2016-03-09 13:24:15 UTC
(In reply to Marcel de Rooy from comment #65)
> Resetting status

Sorry, I assumed that since the status of this bug was "Passed QA" that the patches which git-bz would grab would be patches which were relevant to testing.
Comment 67 Brendan Gallagher 2016-03-09 15:31:00 UTC
(In reply to Marcel de Rooy from comment #62)
> (In reply to Brendan Gallagher from comment #60)
> > Pushed the last two patches (48772 and 48783)  Thanks
> 
> Thanks.
> Don't forget the DBIx updates.
> I added a follow-up for what I discovered after the first kohastructure
> discrepancy, namely the currency length problem (3 vs 10 chars).
> I took the liberty to correct the dbrev for the currency constraint in
> aqorders directly.
> Also added an atomicupdate for a subsequent dbrev.


Thanks!  ok pushed 48858 and updated the Schema.  I also saw some spots that the Schema needs some fixing from another bug.  (adding that to my list to fix up - we've still got some time).  Brendan
Comment 68 Katrin Fischer 2016-06-19 22:16:52 UTC
*** Bug 4338 has been marked as a duplicate of this bug. ***