Description
Jonathan Druart
2015-10-29 14:49:45 UTC
Created attachment 44162 [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. Created attachment 44163 [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. Created attachment 44164 [details] [review] Bug 15084: use Koha::Acquisition::Currenc[y|ies] in admin/currency Created attachment 44165 [details] [review] Bug 15084: DB Changes - add currency.archived and FK on aqorders.currency See the comment for details. Created attachment 44166 [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 :) Created attachment 44167 [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. Created attachment 44168 [details] [review] Bug 15084: Add tests for Koha::Acquisition::Currenc[y|ies] Created attachment 45690 [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. Created attachment 45691 [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. Created attachment 45692 [details] [review] Bug 15084: use Koha::Acquisition::Currenc[y|ies] in admin/currency Created attachment 45693 [details] [review] Bug 15084: DB Changes - add currency.archived and FK on aqorders.currency See the comment for details. Created attachment 45694 [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 :) Created attachment 45695 [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. Created attachment 45696 [details] [review] Bug 15084: Add tests for Koha::Acquisition::Currenc[y|ies] Rebased against master 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. Created attachment 45759 [details] [review] Bug 15084: DB Changes - add currency.archived and FK on aqorders.currency See the comment for details. Created attachment 45761 [details] [review] Bug 15084: DBIx::Class - Shema changes (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. Created attachment 45807 [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> Created attachment 45808 [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> Created attachment 45809 [details] [review] Bug 15084: use Koha::Acquisition::Currenc[y|ies] in admin/currency Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 45810 [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> Created attachment 45811 [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> Created attachment 45812 [details] [review] Bug 15084: Add tests for Koha::Acquisition::Currenc[y|ies] Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 45813 [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> Created attachment 45814 [details] [review] Bug 15084: DBIx::Class - Shema changes Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 47213 [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> Created attachment 47214 [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> Created attachment 47215 [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> Created attachment 47216 [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> Created attachment 47217 [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> Created attachment 47218 [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> Created attachment 47219 [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> Created attachment 47220 [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> Created attachment 47221 [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> 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 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> 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> 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> 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> 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> 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> 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> 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> 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> Created attachment 48603 [details] [review] Bug 15084: Fix conflict with bug 15446 (type vs _type) Patches rebased. 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) 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? Created attachment 48703 [details] [review] Bug 15084 [QA Followup] - Update Schema files Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Created attachment 48747 [details] [review] Bug 15084 [QA Followup] - Fix new uses of GetCurrency Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> (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? @RM: The schema has not been regenerated. 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. Created attachment 48783 [details] [review] Bug 15084: [QA Follow-up] Update kohastructure Adds two db revs to kohastructure 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 !! (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? Ithink it would be the easiest fix that way. Pushed the last two patches (48772 and 48783) Thanks 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. (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. (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 :) 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 Resetting status (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. (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 *** Bug 4338 has been marked as a duplicate of this bug. *** |