This bug report requires the removal of SQL code out of the administrative script aqplan.pl and placed into a perl module in C4 directory. Instead of placing the SQL code directly into a subroutine in a C4 module the database query will be re-written as a DBIx query.
Created attachment 61006 [details] [review] Bug 18212 - Created a subroutine GetAuthValuesForCategory() in C4/Acqusition.pm perl module. I've removed one sql select query from aqplan.pl and rewritten it in DBIx query format in the new GetAuthValuesForCategory subroutine. aqplan.pl is successfully calling and running this DBIx query
Created attachment 61007 [details] [review] Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files The new subroutines in the perl modules contain POD
Created attachment 61008 [details] [review] Bug 18212 - Added a successful unit test for GetAuthValues() function
Created attachment 61009 [details] [review] Bug 18212 - Four successfully transferred sql queries to Koha::AuthorisedValue, Koha::ItemType, Koha::Libraries from aqplan.pl administrative script All these queries have been placed inside subroutines in the perl modules outlined above, each subroutine has POD, and a successful unit test
Test plan: 1. Restart memcached: sudo service memcached restart 2. Drop and recreate the Koha instance database 3. Go through the web installer, selecting all data to be installed in step 3 4. After the web installer is finished log in with the Koha database administrator credentials 5. Create yourself a patron account 6. Set the patron account to have superlibrarian privileges 7. Log out and back in as your newly created patron 8. Navigate to the Acquisitions interface 9. Create a budget named 'testbudget' 10. Create a fund named 'testfund' 11. Navigate to the 'Administrator' interface 12. Write in the URL 'cgi-bin/koha/admin/aqplan.pl' 13. There should be no records showing 14. If you click on Planning->Plan by Months testfund will be displayed 15. Repeat step 14 but click on Planning->Plan by libraries and Planning->Plan by Itemtypes, Both of these pages should also show 'testfund' 16. In your terminal navigate from your Koha instance root directory to t/db_dependent 17. Enter koha shell: sudo koha-shell <instancename> 18. Run AuthorisedValues unit tests: prove -v AuthorisedValues.t 19. The tests should run successful 20. Exit the koha shell 21. cd into the 'koha' directory from where you are 22. Enter the koha shell and run the ItemTypes.t and Libraries.t unit tests by writing in: prove -v filename.t (both of which should be successful)
Amended Test plan: 1. Apply all the patches attached to this bug report 2. Restart memcached: sudo service memcached restart 3. Drop and recreate the Koha instance database 4. Go through the web installer, selecting all data to be installed in step 3 5. After the web installer is finished log in with the Koha database administrator credentials 6. Create yourself a patron account 7. Set the patron account to have superlibrarian privileges 8. Log out and back in as your newly created patron 9. Navigate to the Acquisitions interface 10. Create a budget named 'testbudget' 11. Create a fund named 'testfund' 12. Navigate to the 'Administrator' interface 13. Write in the URL 'cgi-bin/koha/admin/aqplan.pl' 14. There should be no records showing 15. If you click on Planning->Plan by Months testfund will be displayed 16. Repeat step 14 but click on Planning->Plan by libraries and Planning->Plan by Itemtypes, Both of these pages should also show 'testfund' 17. In your terminal navigate from your Koha instance root directory to t/db_dependent 18. Enter koha shell: sudo koha-shell <instancename> 19. Run AuthorisedValues unit tests: prove -v AuthorisedValues.t 20. The tests should run successful 21. Exit the koha shell 22. cd into the 'koha' directory from where you are 23. Enter the koha shell and run the ItemTypes.t and Libraries.t unit tests by writing in: prove -v filename.t (both of which should be successful)
Patch Bug 18212 - Four successfully transferred sql queries... does not apply: Wende an: Bug 18212 - Four successfully transferred sql queries to Koha::AuthorisedValue, Koha::ItemType, Koha::Libraries from aqplan.pl administrative script /home/marc/koha/.git/rebase-apply/patch:316: trailing whitespace. my $dbh = C4::Context->dbh; fatal: sha1 information is lacking or useless (Koha/ItemType.pm). Dem Repository fehlen notwendige Blobs um auf einen 3-Wege-Merge zurückzufallen. Kann nicht zu 3-Wege-Merge zurückfallen.
Created attachment 62584 [details] [review] Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files The new subroutines in the perl modules contain POD
Created attachment 62585 [details] [review] Bug 18212 - Four successfully transferred sql queries to Koha::AuthorisedValue, Koha::ItemType, Koha::Libraries from aqplan.pl administrative script
(In reply to Marc Veron on comment 7) Hi Marc I have gone through and fixed the patches and rebased them down to 2 patches. It should now apply successfully for you on a clean branch.
Created attachment 62587 [details] [review] Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files The new subroutines in the perl modules contain POD Tested both patches together, works as expected Amended with blank line preceding a =cut to make qa tools pass for first patch Signed-off-by: Marc Véron <veron@veron.ch>
Comment on attachment 62587 [details] [review] Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files Obsoleting because there are problems with next patch after my amendement.
Comment on attachment 62584 [details] [review] Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files Restoring previous patch. It does not pass qa tools because of a fail in Koha/ItemType.pm (Apparent command =cut not preceded by blank lins)
Created attachment 62588 [details] [review] Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files The new subroutines in the perl modules contain POD There is a minor error in qa tools that can be fixed in next patch. Followed test plan (comment #6) with both patches together, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Sorr,, Alex, I wanted to rescue a minor issue in the first patch (qa tools complaining about missing blank line preceding a =cut to make), but it did not work out. Reset to your patches. Do you have qa tools installed?
Hi Marc I do have qa tools installed. Could you please clarify what the issue with these patches are? I have just run the qa tool over the last 2 patches and all tests were successful.
(In reply to Alex Buckley from comment #16) > Hi Marc > > I do have qa tools installed. > > Could you please clarify what the issue with these patches are? I have just > run the qa tool over the last 2 patches and all tests were successful. Just re-tested. QA tools complain about 2 minor things: - Apply 1st patch - Run QA tools (koha-qa.pl -v 2 -c 1) Result: (...) FAIL Koha/ItemType.pm (...) FAIL pod in file Koha/ItemType.pm Apparent command =cut not preceded by blank line -Apply 2nd patch - QA tools FAIL Koha/AuthorisedValue.pm (...) FAIL pod in file Koha/AuthorisedValue.pm Apparent command =cut not preceded by blank line Setting back to FQA. Will be happy to sign of as soon as QA tools pass :-)
Created attachment 66567 [details] [review] Bug 18212 - Followup: Fixed POD in AuthorisedValue.pm and ItemType.pm Passes QA test now, and all unit tests pass Test plan: 1. Apply all the patches attached to this bug report 2. Restart memcached: sudo service memcached restart 3. Drop and recreate the Koha instance database 4. Go through the web installer, selecting all data to be installed in step 3 5. After the web installer is finished log in with the Koha database administrator credentials 6. Create yourself a patron account 7. Set the patron account to have superlibrarian privileges 8. Log out and back in as your newly created patron 9. Navigate to the Acquisitions interface 10. Create a budget named 'testbudget' 11. Create a fund named 'testfund' 12. Navigate to the 'Administrator' interface 13. Write in the URL 'cgi-bin/koha/admin/aqplan.pl' 14. There should be no records showing 15. If you click on Planning->Plan by Months testfund will be displayed 16. Repeat step 14 but click on Planning->Plan by libraries and Planning->Plan by Itemtypes, Both of these pages should also show 'testfund' 17. In your terminal navigate from your Koha instance root directory to t/db_dependent 18. Enter koha shell: sudo koha-shell <instancename> 19. Run AuthorisedValues unit tests: prove -v AuthorisedValues.t 20. The tests should run successful 21. Exit the koha shell 22. cd into the 'koha' directory from where you are 23. Enter the koha shell and run the ItemTypes.t and Libraries.t unit tests by writing in: prove -v filename.t (both of which should be successful) Sponsored-by: Catalyst IT
Created attachment 66570 [details] [review] Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files The new subroutines in the perl modules contain POD Test plan: 1. Apply all the patches attached to this bug report 2. Restart memcached: sudo service memcached restart 3. Drop and recreate the Koha instance database 4. Go through the web installer, selecting all data to be installed in step 3 5. After the web installer is finished log in with the Koha database administrator credentials 6. Create yourself a patron account 7. Set the patron account to have superlibrarian privileges 8. Log out and back in as your newly created patron 9. Navigate to the Acquisitions interface 10. Create a budget named 'testbudget' 11. Create a fund named 'testfund' 12. Navigate to the 'Administrator' interface 13. Write in the URL 'cgi-bin/koha/admin/aqplan.pl' 14. There should be no records showing 15. If you click on Planning->Plan by Months testfund will be displayed 16. Repeat step 14 but click on Planning->Plan by libraries and Planning->Plan by Itemtypes, Both of these pages should also show 'testfund' 17. In your terminal navigate from your Koha instance root directory to t/db_dependent 18. Enter koha shell: sudo koha-shell <instancename> 19. Run AuthorisedValues unit tests: prove -v AuthorisedValues.t 20. The tests should run successful 21. Exit the koha shell 22. cd into the 'koha' directory from where you are 23. Enter the koha shell and run the ItemTypes.t and Libraries.t unit tests by writing in: prove -v filename.t (both of which should be successful) Sponsored-by: Catalyst IT
Created attachment 66571 [details] [review] Bug 18212 - Followup: Fixed POD in AuthorisedValue.pm and ItemType.pm Passes QA test now, and all unit tests pass Test plan: 1. Apply all the patches attached to this bug report 2. Restart memcached: sudo service memcached restart 3. Drop and recreate the Koha instance database 4. Go through the web installer, selecting all data to be installed in step 3 5. After the web installer is finished log in with the Koha database administrator credentials 6. Create yourself a patron account 7. Set the patron account to have superlibrarian privileges 8. Log out and back in as your newly created patron 9. Navigate to the Acquisitions interface 10. Create a budget named 'testbudget' 11. Create a fund named 'testfund' 12. Navigate to the 'Administrator' interface 13. Write in the URL 'cgi-bin/koha/admin/aqplan.pl' 14. There should be no records showing 15. If you click on Planning->Plan by Months testfund will be displayed 16. Repeat step 14 but click on Planning->Plan by libraries and Planning->Plan by Itemtypes, Both of these pages should also show 'testfund' 17. In your terminal navigate from your Koha instance root directory to t/db_dependent 18. Enter koha shell: sudo koha-shell <instancename> 19. Run AuthorisedValues unit tests: prove -v AuthorisedValues.t 20. The tests should run successful 21. Exit the koha shell 22. cd into the 'koha' directory from where you are 23. Enter the koha shell and run the ItemTypes.t and Libraries.t unit tests by writing in: prove -v filename.t (both of which should be successful) Sponsored-by: Catalyst IT
QA test pass now with these two patches: OK C4/Acquisition.pm OK critic OK forbidden patterns OK git manipulation OK pod OK spelling OK valid OK Koha/AuthorisedValue.pm OK critic OK forbidden patterns OK git manipulation OK pod OK spelling OK valid OK Koha/ItemType.pm OK critic OK forbidden patterns OK git manipulation OK pod OK spelling OK valid OK Koha/Libraries.pm OK critic OK forbidden patterns OK git manipulation OK pod OK spelling OK valid OK admin/aqplan.pl OK critic OK forbidden patterns OK git manipulation OK pod OK spelling OK valid OK t/db_dependent/AuthorisedValues.t OK critic OK forbidden patterns OK git manipulation OK pod OK spelling OK valid OK t/db_dependent/Koha/ItemTypes.t OK critic OK forbidden patterns OK git manipulation OK pod OK spelling OK valid OK t/db_dependent/Koha/Libraries.t OK critic OK forbidden patterns OK git manipulation OK pod OK spelling OK valid
Comment on attachment 66570 [details] [review] Bug 18212 - Moved all sql queries out aqplan.pl into Koha::Libraries.pm, Koha::ItemType.pm, Koha::AuthorisedValue.pm files Review of attachment 66570 [details] [review]: ----------------------------------------------------------------- ::: C4/Acquisition.pm @@ +3055,5 @@ > + }); > + return $authvalues; > +} > + > + You don't need this, you use Koha::AuthorisedValues module directly ::: Koha/AuthorisedValue.pm @@ +174,5 @@ > + order_by => [qw/ lib /] > + }); > + return @Getauthorised_values; > +}; > + You don't need this, you use Koha::AuthorisedValues module directly @@ +187,5 @@ > + Koha::AuthorisedValues->new()->search({ > + category => { 'like','%a%'}, > + }); > + return @distinctauthorised_values; > +}; You don't need this, you use Koha::AuthorisedValues module directly ::: Koha/ItemType.pm @@ +116,5 @@ > + my $sth = $dbh->prepare($query); > + $sth->execute(); > + return $sth; > +} > + You don't need this, just use Koha::ItemTypes->search_with_localization ::: Koha/Libraries.pm @@ +65,5 @@ > + my $sth = $dbh->prepare($query); > + $sth->execute(); > + return $sth; > +} > + Use Koha::Libraries->search for getting all libraries. ::: admin/aqplan.pl @@ +110,4 @@ > my $budgets_ref = GetBudgetHierarchy( $budget_period_id, $show_mine?$template->{VARS}->{'USER_INFO'}->{'branchcode'}:'', $show_mine?$template->{VARS}->{'USER_INFO'}->{'borrowernumber'}:'' ); > > # build categories list > +my @DistinctAuthValues = Koha::AuthorisedValue->GetDistinctCat(); should be something like: my $acq_categories = Koha::AuthorisedValueCategories->search( { category => { like => 'A%' } } ); @@ +116,5 @@ > my @category_list; > > # a hash, to check that some hardcoded categories exist. > my %categories; > +while ( my ($category) = each @DistinctAuthValues ) { while ( my $category = $acq_categories->next ) { @@ +191,4 @@ > } > # ------------------------------------------------------------ > if ( $authcat =~ m/^Asort/ ) { > + my @AuthValues = Koha::AuthorisedValue->GetAuthValues($authcat); my $AuthValues = Koha::AuthorisedValues->search ( { category => $authcat } ); @@ +225,4 @@ > } > > elsif ( $authcat eq 'ITEMTYPES' ) { > + my $sth = Koha::ItemType->GetItemTypes($dbh); Koha::ItemTypes->search_with_localization @@ +237,4 @@ > > } elsif ( $authcat eq 'BRANCHES' ) { > > + my $sth = Koha::Libraries->GetBranches($dbh); Koha::Libraries->search @@ +249,3 @@ > } elsif ($authcat) { > + > + my $sth = Koha::AuthorisedValue->GetAuthValues($authcat,$dbh); Koha::AuthorisedValues->search ( { category => $authcat } ); ::: t/db_dependent/AuthorisedValues.t @@ +100,5 @@ > + { > + order_by => [qw/ lib /] > + }); > +}; > + If you want to test some subroutine, you can't just redefine it in the test. You need to use it from the place where it is defined... but you do not need this sub routine at all, so no need to add this test @@ +111,5 @@ > + Koha::AuthorisedValues->new()->search({ > + category => { 'like','%a%'}, > + }); > +} > + If you want to test some subroutine, you can't just redefine it in the test. You need to use it from the place where it is defined... but you do not need this sub routine at all, so no need to add this test ::: t/db_dependent/Koha/ItemTypes.t @@ +126,5 @@ > + my $sth = $dbh->prepare($query); > + $sth->execute(); > + return $sth->rows; > +} > + If you want to test some subroutine, you can't just redefine it in the test. You need to use it from the place where it is defined... but you do not need this sub routine at all, so no need to add this test ::: t/db_dependent/Koha/Libraries.t @@ +92,5 @@ > + my $sth = $dbh->prepare($query); > + $sth->execute(); > + return $sth->rows; > +} > + If you want to test some subroutine, you can't just redefine it in the test. You need to use it from the place where it is defined... but you do not need this sub routine at all, so no need to add this test