Created attachment 23381 [details] [review] Add the "Orders by budget" report. This patch adds the "Orders by budget" report to the reports page. This report can generate the list of all orders on a specific budget. The "All budgets" and "All active budgets" options are also available to have different views of all your orders. A "[i]" is also added before the names of inactive budgets in the budget selection box. Sponsored-by: CCSR ( http://www.ccsr.qc.ca )
Don't forget to provide unit tests for new routines.
Created attachment 25397 [details] [review] New patch version with unit tests. Here is a new version of the patch with unit tests for the added subroutines.
Merge conflict in t/db_dependent/Acquisition.t
Created attachment 25913 [details] [review] New version of the patch, fixes merge conflict and a small bug. Here is a new version of the patch, fixing the merge conflict and a small bug we found with the biblio titles.
Created attachment 25915 [details] [review] [Signed-off]Add the "Orders by budget" report. http://bugs.koha-community.org/show_bug.cgi?id=11371 How I tested: Go to Home > Reports > Orders by budget Select one or all budgets Output to Scrren or CSV file Set a budget to inactive Verify that this budget is marked wit [i] in drop down list (Filters) Patch behaves as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Comment on attachment 25915 [details] [review] [Signed-off]Add the "Orders by budget" report. Review of attachment 25915 [details] [review]: ----------------------------------------------------------------- Hi Frederick, I know this has been in the loop for a while already, but I found some things that I feel should be addressed. I hope my comments are understandable. I. The QA script points out some things: QA SCRIPT testing 1 commit(s) (applied to 7408fa5 'Bug 8168: (follow-up) Use semicolon a') OK C4/Budgets.pm OK pod OK forbidden patterns OK valid OK critic FAIL reports/orders_by_budget.pl OK pod FAIL forbidden patterns forbidden pattern: Koha is now under the GPLv3 license (line 10) FAIL valid Scalar value @filtered_budgets[0] better written as $filtered_budgets[0] FAIL critic I/O layer ":utf8" used at line 154, column 9. Use ":encoding(UTF-8)" to get strict validation. FAIL t/db_dependent/Acquisition.t OK pod FAIL forbidden patterns forbidden pattern: tab char (line 135) forbidden pattern: tab char (line 136) forbidden pattern: tab char (line 134) forbidden pattern: tab char (line 137) OK valid OK critic FAIL t/db_dependent/Budgets.t OK pod FAIL forbidden patterns forbidden pattern: tab char (line 68) forbidden pattern: tab char (line 67) forbidden pattern: tab char (line 58) forbidden pattern: tab char (line 49) forbidden pattern: tab char (line 50) forbidden pattern: tab char (line 57) OK valid OK critic FAIL koha-tmpl/intranet-tmpl/prog/en/modules/reports/orders_by_budget.tt FAIL forbidden patterns forbidden pattern: tab char (line 36) forbidden pattern: tab char (line 99) forbidden pattern: tab char (line 88) forbidden pattern: tab char (line 90) forbidden pattern: tab char (line 95) forbidden pattern: tab char (line 98) forbidden pattern: tab char (line 100) forbidden pattern: tab char (line 58) forbidden pattern: tab char (line 101) forbidden pattern: tab char (line 15) forbidden pattern: tab char (line 62) forbidden pattern: tab char (line 61) forbidden pattern: tab char (line 77) forbidden pattern: tab char (line 83) forbidden pattern: tab char (line 78) forbidden pattern: tab char (line 57) forbidden pattern: tab char (line 96) forbidden pattern: tab char (line 14) forbidden pattern: tab char (line 82) forbidden pattern: tab char (line 107) forbidden pattern: tab char (line 80) forbidden pattern: tab char (line 89) forbidden pattern: tab char (line 59) OK tt_valid OK valid_template FAIL koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt FAIL forbidden patterns forbidden pattern: tab char (line 65) OK tt_valid OK valid_template Please fix those. II. Testing the interface 1) I think the budgets should be funds in the interface, or fund (budget) for the pull down list. 2) There is a small display issue with the legend 'Filters' on the first fieldset. The legend shows inside the elemend, instead of above. 3) The list could be improved, showing only funds with orders. But that would be an enhancement. 4) Lots of the date and currency formatting could be handled in the template. But it works and dateformat is honored. III Code review Generally I think there is a bit of confusion about the terminology, visible in the interface and in the naming and documentation of the new routines. The hierarchies in Koha speak are something like: basket group - basket - order/order line budget - fund - child funds I have tried to point those out and also found a few other things: ::: C4/Budgets.pm @@ +476,4 @@ > } > > # ------------------------------------------------------------------- > +sub GetBudgetPeriodDescription { 1) Please add complete documentation to the new routine - I know Budget.pm is a bit sparse on that, but it's still worth not making it worse :) (minor) 2) Please also document the return values (see also below) (minor) @@ +725,5 @@ > +=head2 GetBudgetReport > + > + &GetBudgetReport(); > + > +Get one specific budget for reports without cancelled baskets. 3) Documentation could be a bit more clear: Get all orders for a specific fund, without cancelled orders. @@ +739,5 @@ > + FROM aqbudgets b > + INNER JOIN aqorders o > + ON b.budget_id = o.budget_id > + WHERE b.budget_id=? > + AND (o.datecancellationprinted IS NULL OR o.datecancellationprinted='0000-00-00') 4) The SQL could make use of the new orderstatus column in aqorders to filter out the cancelled orders now. (minor) @@ +755,5 @@ > +=head2 GetBudgetsReport > + > + &GetBudgetsReport(); > + > +Get all budgets for reports without cancelled baskets. 5) This seems to be mostly a copy of GetActiveBudgetsReport but for the Active bit. Why not move those into one and use a parameter? Suggestion for improving the description: Get all but cancelled orders for all funds. @@ +779,5 @@ > + } > + return @results; > +} > + > +=head2 GetActiveBudgets 6) I think the name doesn't fit the use of the routine. GetActiveBudgets will give you inactive budgets when you call it with parameter 0? ::: koha-tmpl/intranet-tmpl/prog/en/modules/reports/orders_by_budget.tt @@ +35,5 @@ > + <th>Basket</th> > + <th>Basket by</th> > + <th>Title</th> > + <th>Currency</th> > + <th>Vendor Price</th> 7) Tiny thing: capitalization (trivial) ::: koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt @@ +61,4 @@ > <h2>Other</h2> > <ul> > <li><a href="/cgi-bin/koha/reports/itemslost.pl">Items lost</a></li> > + <li><a href="/cgi-bin/koha/reports/orders_by_budget.pl">Orders by budget</a></li> 8) Should be orders by fund. ::: reports/orders_by_budget.pl @@ +174,5 @@ > + print '"Total RRP"' . $sep; > + print '"Total cost"' . $sep; > + print '"Entry date"' . $sep; > + print '"Date received"' . $sep; > + print '"Notes"' . "\n"; 9) These headers won't be translatable. It's an old problem we have solved recently using a TT include. Take a look at the other CSV export options in acquisitions especially. Could be an enhancement. @@ +204,5 @@ > + } > +} > +else { > + # Set file export choices > + my $CGIextChoice = CGI::scrolling_list( 10) CGI::scrolling_list shouldn't be used in new scripts, as we are trying to get rid of it totally - see bug 766. @@ +212,5 @@ > + -size => 1, > + -multiple => 0 > + ); > + > + my $CGIsepChoice = GetDelimiterChoices; 11) I am not sure there, but avoiding CGIsepChoice might also be preferrable.
Created attachment 30545 [details] [review] Bug 11731 - Fixed QA warnings and merge conflicts
Created attachment 30546 [details] [review] Bug 11371 - Follow-up to Comment 6 'Budgets' is now displayed as 'funds' in the Koha UI. Fixed the legend display issue. Improved POD. Use of the orderstatus column to filter canceled orders. GetActiveBudgetsReports has been merged with GetBudgetsReports( [$activity] ). GetActiveBudgets renamed to GetBudgetsByActivity Create templates for .CSV file generation. Removed CGI ui components. new file: koha-tmpl/intranet-tmpl/prog/en/includes/csv_headers/reports/orders_by_budget.tt new file: koha-tmpl/intranet-tmpl/prog/en/modules/reports/csv/orders_by_budget.tt
Conflict in 3 files. Unable to resolve the conflict.
Created attachment 35385 [details] [review] This patch adds the "Orders by budget" report to the reports page. This report can generate the list of all orders on a specific budget. The "All budgets" and "All active budgets" options are also available to have different views of all your orders. A "[i]" is also added before the names of inactive budgets in the budget selection box. How to test: Go to Home > Reports > Orders by budget Select one or all budgets Output to Scrren or CSV file Set a budget to inactive Verify that this budget is marked wit [i] in drop down list (Filters) Sponsored-by: ccsr Bug 11371 - Add a new report : Orders by budget
(In reply to Ahmed Sfayhi from comment #10) > > A "[i]" is also added before the names of inactive budgets in the budget > selection box. > That "[i]" is not translatable.
CONFLICT (content): Merge conflict in t/db_dependent/Budgets.t CONFLICT (content): Merge conflict in t/db_dependent/Acquisition.t CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/reports/reports-home.tt
Created attachment 36620 [details] [review] Add a new report : Orders by budget This patch adds the "Orders by budget" report to the reports page. This report can generate the list of all orders on a specific budget. The "All budgets" and "All active budgets" options are also available to have different views of all your orders. A "[i]" is also added before the names of inactive budgets in the budget selection box. How to test: Go to Home > Reports > Orders by budget Select one or all budgets Output to Scrren or CSV file Set a budget to inactive Verify that this budget is marked wit [i] in drop down list (Filters) Sponsored-by: ccsr
Conflits fixes and old patchs squashed
It works great, but failed QA tests: Processing files before patches |========================>| 8 / 8 (100.00%) Processing files after patches |========================>| 8 / 8 (100.00%) FAIL C4/Budgets.pm OK critic OK forbidden patterns FAIL pod *** ERROR: Spurious text after =cut in file C4/Budgets.pm OK valid FAIL reports/orders_by_budget.pl OK critic OK forbidden patterns OK pod FAIL valid Possible attempt to separate words with commas
Created attachment 36707 [details] [review] QA fix
Created attachment 37010 [details] [review] Add a new report : Orders by budget This patch adds the "Orders by budget" report to the reports page. This report can generate the list of all orders on a specific budget. The "All budgets" and "All active budgets" options are also available to have different views of all your orders. A "[i]" is also added before the names of inactive budgets in the budget selection box. How to test: Go to Home > Reports > Orders by budget Select one or all budgets Output to Scrren or CSV file Set a budget to inactive Verify that this budget is marked wit [i] in drop down list (Filters) Sponsored-by: ccsr http://bugs.koha-community.org/show_bug.cgi?id=11371 Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>
Created attachment 37011 [details] [review] QA fix http://bugs.koha-community.org/show_bug.cgi?id=11371 Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>
QA comment: 1/ Please rename the script according to previous change (budget vs fund). 2/ Add basket name in the basket column 3/ Get rid of the warning: FAIL reports/orders_by_budget.pl FAIL valid Possible attempt to put comments in qw() list Possible attempt to separate words with commas The no warnings qw() is not a correct solution (and does not work) 4/ default separator should be chosen (syspref "delimiter") 5/ Add DataTables on the table could be useful and easy. 6/ Both tests fail: t/db_dependent/Budgets.t .. 58/120 DBIx::Class::Row::new(): No such column 'budget_description' on Koha::Schema::Result::Aqbudgetperiod at C4/Budgets.pm line 144
Created attachment 41463 [details] [review] Add a new report : Orders by budget This patch adds the "Orders by budget" report to the reports page. This report can generate the list of all orders on a specific budget. The "All budgets" and "All active budgets" options are also available to have different views of all your orders. A "[i]" is also added before the names of inactive budgets in the budget selection box. How to test: Go to Home > Reports > Orders by budget Select one or all budgets Output to Scrren or CSV file Set a budget to inactive Verify that this budget is marked wit [i] in drop down list (Filters) Sponsored-by: ccsr http://bugs.koha-community.org/show_bug.cgi?id=11371 Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>
Created attachment 41464 [details] [review] Bug11371 - QA fix http://bugs.koha-community.org/show_bug.cgi?id=11371 Signed-off-by: Nicole Engard <nengard@bywatersolutions.com>
Fixed Conflicts in first patch and updated QA fix
Everything seems ok except the csv file where some columns don't have the right datas : column "basket by" contains the name of the basket column "biblionumber" contains librarian id column title contains biblionumber currency contains Title Vendor price contains Currency etc.
Created attachment 45243 [details] [review] Bug 11371 - Correct right data in each column and correct format date and tidying up in template file
Created attachment 45244 [details] [review] Bug 11371 - Correct right data in each column and correct format date and tidying up in template file
Created attachment 45245 [details] [review] Bug 11371 - fix problem with right data in each column in csv file fix problem with right data in each column correct format date tidying up in template file
Applying the patches does some auto-merging: Auto-merging t/db_dependent/Budgets.t Auto-merging C4/Budgets.pm After that, t/db_dependent/Budgets.t fails (it passed OK without patches): Test Summary Report ------------------- t/db_dependent/Budgets.t (Wstat: 65280 Tests: 76 Failed: 1) Failed test: 76 Non-zero exit status: 255 Parse errors: Bad plan. You planned 122 tests but ran 76
Created attachment 45288 [details] [review] Bug 11371 - QA fix 2
I fixed all tests for this patch, but a failed QA on the master before application of the patch.
Sorry, t/db_dependent/Budgets.t fails with: Parse errors: Bad plan. You planned 122 tests but ran 126.
Created attachment 45338 [details] [review] Bug 11371 - QA fix, Parse errors: Bad plan
Created attachment 45887 [details] [review] Bug 11371 - Add a new report : Orders by budget This patch adds the "Orders by budget" report to the reports page. This report can generate the list of all orders on a specific budget. The "All budgets" and "All active budgets" options are also available to have different views of all your orders. A "[i]" is also added before the names of inactive budgets in the budget selection box. How to test: Go to Home > Reports > Orders by budget Select one or all budgets Output to Scrren or CSV file Set a budget to inactive Verify that this budget is marked wit [i] in drop down list (Filters) Sponsored-by: ccsr http://bugs.koha-community.org/show_bug.cgi?id=11371 Author: bouzid <bouzid.fergani@inlibro.com>
In this patch, I fix many problems, most important is note not appear.
Created attachment 45890 [details] [review] Bug 11371 - Add a new report : Orders by budget This patch adds the "Orders by budget" report to the reports page. This report can generate the list of all orders on a specific budget. The "All budgets" and "All active budgets" options are also available to have different views of all your orders. A "[i]" is also added before the names of inactive budgets in the budget selection box. How to test: Go to Home > Reports > Orders by budget Select one or all budgets Output to Scrren or CSV file Set a budget to inactive Verify that this budget is marked wit [i] in drop down list (Filters) Sponsored-by: ccsr http://bugs.koha-community.org/show_bug.cgi?id=11371 Author: bouzid <bouzid.fergani@inlibro.com>
Without patch, t/db_dependent/Acquisition.t runs OK With patch there is an error: (...) DBIx::Class::ResultSet::create(): No such column budget_description on Koha::Schema::Result::Aqbudgetperiod at /usr/share/kohaclone/C4/Budgets.pm line 143 (...) (Wstat: 65280 Tests: 10 Failed: 0) Non-zero exit status: 255 Parse errors: Bad plan. You planned 87 tests but ran 10. Note: The field budget_description was dropped in installer/data/mysql/updatedatabase.pl line 4277
Created attachment 45981 [details] [review] Bug 11371 - Add a new report : Orders by budget This patch adds the "Orders by budget" report to the reports page. This report can generate the list of all orders on a specific budget. The "All budgets" and "All active budgets" options are also available to have different views of all your orders. A "[i]" is also added before the names of inactive budgets in the budget selection box. How to test: apply patch run updatedatabase.pl Go to Home > Reports > Orders by budget Select one or all budgets Output to Scrren or CSV file Set a budget to inactive Verify that this budget is marked wit [i] in drop down list (Filters) Sponsored-by: ccsr http://bugs.koha-community.org/show_bug.cgi?id=11371 Author: bouzid <bouzid.fergani@inlibro.com>
Created attachment 45982 [details] [review] Bug 11371 - Add a new report : Orders by budget This patch adds the "Orders by budget" report to the reports page. This report can generate the list of all orders on a specific budget. The "All budgets" and "All active budgets" options are also available to have different views of all your orders. A "[i]" is also added before the names of inactive budgets in the budget selection box. How to test: apply patch run updatedatabase.pl Go to Home > Reports > Orders by budget Select one or all budgets Output to Scrren or CSV file Set a budget to inactive Verify that this budget is marked wit [i] in drop down list (Filters) Sponsored-by: ccsr http://bugs.koha-community.org/show_bug.cgi?id=11371 Author: bouzid <bouzid.fergani@inlibro.com> Followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch>
Comment on attachment 45982 [details] [review] Bug 11371 - Add a new report : Orders by budget Review of attachment 45982 [details] [review]: ----------------------------------------------------------------- I am not convinced by the new subroutines you add with this patch. There are usually too specific and could be done with existing subroutines from C4::Acquisition or C4::Budgets The main problem is that there is not real test coverage for them. ::: C4/Budgets.pm @@ +473,5 @@ > + Description is fetched from aqbudgetperiods, id is compared aqbudgets. (INNER JOIN on budget_period_id) > + > +=cut > + > +sub GetBudgetPeriodDescription { This subroutine is too specific. It could be replaced with my $budget_period = GetBudgetPeriod( $budget->{budget_period_id} ) my $budget_period_description = $budget_period->{budget_period_description}
Comment on attachment 45982 [details] [review] Bug 11371 - Add a new report : Orders by budget Review of attachment 45982 [details] [review]: ----------------------------------------------------------------- I feel we are getting closer with this, even if it might not feel like it. Can you please take a look at my new comments and the one Jonathan has made? There is one additional thing not noted in the splitter comments: The new report should also be added to the reports module navigation (I think it's reports-menu.inc), that shows up on the left side once you have opened a reports page. ::: koha-tmpl/intranet-tmpl/prog/en/modules/reports/orders_by_budget.tt @@ +38,5 @@ > + <thead> > + <tr> > + <th>Fund</th> > + <th>Basket</th> > + <th>Basket Name</th> Please fix capitalization in the list of column names. We have agreed that only the first word should be capitalized. @@ +42,5 @@ > + <th>Basket Name</th> > + <th>Basket By</th> > + <th>Title</th> > + <th>Currency</th> > + <th>Vendor Price</th> Please use list price to match acquisition terminology. ::: reports/orders_by_fund.pl @@ +26,5 @@ > +=cut > + > +use strict; > +use warnings; > + Please use Modern::Perl. @@ +111,5 @@ > + > + # Format the dates and currencies correctly > + $order->{'datereceived'} = Koha::DateUtils::output_pref(Koha::DateUtils::dt_from_string($order->{'datereceived'})); > + $order->{'entrydate'} = Koha::DateUtils::output_pref(Koha::DateUtils::dt_from_string($order->{'entrydate'})); > + $order->{'listprice'} = sprintf( "%.2f", $order->{'listprice'} ); It would be nicer to handle the display on template level with the new Price TT plugin. @@ +221,5 @@ > + foreach my $thisbudget ( sort {$a->{budget_name} cmp $b->{budget_name}} @non_active_budgets ) { > + my $budget_period_desc = C4::Budgets::GetBudgetPeriodDescription($thisbudget->{budget_id}); > + my %row = ( > + value => $thisbudget->{budget_id}, > + description => "[i] ". $thisbudget->{budget_name}, The [i] is a translation issue. I think as it's in the template now it's not translatable at all, but I would also prefer another way of displaying it, as [i] is not very clear and will probably cause confusion for translators. Looking in other spots of the acquisition module I have some suggestions: - Don't show the inactive funds to begin with, but only, when a checkbox is checked (see neworderempty.pl) and then put (inactive) behind the inactiv fund name. - Use a hierarchical display for more than one budget and it's funds like on histsearch.pl ::: t/db_dependent/Budgets.t @@ +2,2 @@ > use Modern::Perl; > +use Test::More ; Please don't remove the number of tests to run.
... even if it might not _look_ like it...
Created attachment 50230 [details] [review] Bug 11371 - Add a new report : Orders by budget
Created attachment 50231 [details] [review] Bug 11371 - Add a new report : Orders by budget Use subroutine GetBudgetHierarchy fir return all budgets Delete subroutine GetBudgetPeriodDescription and theire tests Use Price TT plugin Correct name of column and capitalization the first letter Add checkbox for show inactive budgets, default the drop down list containt a active budget Not use [i] for inactive budgets, i add (inactive) at the end of inactive budget Add vendor note in the list of show attribute Test case: Go to Home > Reports > Orders by budget Select one or all budgets You can show the inactive budget, default the drop down list containt a active budget Choose output to screen ou csv file
Created attachment 50233 [details] [review] Bug 11371 - Add option show or no inactive budget and more options Use subroutine GetBudgetHierarchy fir return all budgets Delete subroutine GetBudgetPeriodDescription and theire tests Use Price TT plugin Correct name of column and capitalization the first letter Add checkbox for show inactive budgets, default the drop down list containt a active budget Not use [i] for inactive budgets, i add (inactive) at the end of inactive budget Add vendor note in the list of show attribute Test case: Go to Home > Reports > Orders by budget Select one or all budgets You can show the inactive budget, default the drop down list containt a active budget Choose output to screen ou csv file
Patch does not apply. Applying: Bug 11371 - Add a new report : Orders by budget fatal: sha1 information is lacking or useless (C4/Budgets.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge.
Created attachment 50466 [details] [review] Bug 11371 - Add a new report : Orders by fund with more options Add option show or no inactive budget and more options Use subroutine GetBudgetHierarchy for return all budgets Delete subroutine GetBudgetPeriodDescription and theire tests Use Price TT plugin Correct name of column and capitalization the first letter Add checkbox for show inactive budgets, default the drop down list containt a active budget Not use [i] for inactive budgets, i add (inactive) at the end of inactive budget Add vendor note in the list of show attribute Test case: Go to Home > Reports > Orders by fund Select one or all budgets You can show the inactive budget, default the drop down list containt a active budget Choose output to screen ou csv file
Created attachment 50468 [details] [review] Bug 11371 - Add a new report : Orders by fund with more options Add option show or no inactive budget and more options Use subroutine GetBudgetHierarchy for return all budgets Delete subroutine GetBudgetPeriodDescription and theire tests Use Price TT plugin Correct name of column and capitalization the first letter Add checkbox for show inactive budgets, default the drop down list containt a active budget Not use [i] for inactive budgets, i add (inactive) at the end of inactive budget Add vendor note in the list of show attribute Test case: Go to Home > Reports > Orders by fund Select one or all budgets You can show the inactive budget, default the drop down list containt a active budget Choose output to screen ou csv file Works as expected. QA tools OK with Bug 16104 applied. Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 50769 [details] [review] Bug 11371 - Add a new report : Orders by fund with more options Add option show or no inactive budget and more options Use subroutine GetBudgetHierarchy for return all budgets Delete subroutine GetBudgetPeriodDescription and theire tests Use Price TT plugin Correct name of column and capitalization the first letter Add checkbox for show inactive budgets, default the drop down list containt a active budget Not use [i] for inactive budgets, i add (inactive) at the end of inactive budget Add vendor note in the list of show attribute Test case: Go to Home > Reports > Orders by fund Select one or all budgets You can show the inactive budget, default the drop down list containt a active budget Choose output to screen ou csv file Works as expected. QA tools OK with Bug 16104 applied. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> - changed 'Fund (budget):' back to 'Fund:', as the budget no longer shows in the pull down. - Fixed number of tests in Budgets.t - Removed &GetBudgetPeriodDescription
Pushed to master for the May 2016 release, thanks Bouzid!
You are welcome Kyle.
The tests in t/db_dependent/Acquisition.t are broken, see bug 16419.