The average checkout report reports/issues_avg_stats.pl always uses biblioitems.itemtype, regardless to item-level_itypes system preference.
Created attachment 40003 [details] [review] Bug 13452: 'Average loan time' report to obey item-level_itypes preference Fixes the 'Average loan time' report so that the system preference setting item-level_itypes is taken into account. Before the patch, the report was always using the biblio level itemtype. With the patch, it will depend on the pref setting. To test: - Run various reports, compare results with both possible pref settings - Try limiting your search in various ways, especially on a specific itemtype - Make sure you switch checkboxes in testing, using itemtype as the row or as the column Notes: - Filtering on a specific itemtype does not work without the patch as it fixes a bug there. - Fixes headings and breadcrumbs so that the name of the report is the same as on the reports start page
Created attachment 40004 [details] [review] Bug 13452: 'Average loan time' report to obey item-level_itypes preference Fixes the 'Average loan time' report so that the system preference setting item-level_itypes is taken into account. Before the patch, the report was always using the biblio level itemtype. With the patch, it will depend on the pref setting. To test: - Run various reports, compare results with both possible pref settings - Try limiting your search in various ways, especially on a specific itemtype - Make sure you switch checkboxes in testing, using itemtype as the row or as the column Notes: - Filtering on a specific itemtype on master was not working if the itemtype was chosen as column. The patch should fix that. - Fixes headings and breadcrumbs so that the name of the report is the same as on the reports start page
Comment on attachment 40004 [details] [review] Bug 13452: 'Average loan time' report to obey item-level_itypes preference Review of attachment 40004 [details] [review]: ----------------------------------------------------------------- ::: reports/issues_avg_stats.pl @@ +407,4 @@ > if (( @colfilter ) and ($colfilter[1])){ > $sth2->execute("'".$colfilter[0]."'","'".$colfilter[1]."'"); > } elsif ($colfilter[0]) { > + $sth2->execute; Perhaps a shallow reading of the changes doesn't make this obvious, but why did this change?
Comment on attachment 40004 [details] [review] Bug 13452: 'Average loan time' report to obey item-level_itypes preference Review of attachment 40004 [details] [review]: ----------------------------------------------------------------- ::: reports/issues_avg_stats.pl @@ +407,4 @@ > if (( @colfilter ) and ($colfilter[1])){ > $sth2->execute("'".$colfilter[0]."'","'".$colfilter[1]."'"); > } elsif ($colfilter[0]) { > + $sth2->execute; Because without this change there was always an error in the logs - without any of the other changes of the patch. I needed to change it to get it working first... that's how I remember. The answer should be further above in the code... this whole script should be rewritten to create the SQL in cleaner ways.
Created attachment 40798 [details] [review] Bug 13452: 'Average loan time' report to obey item-level_itypes preference Fixes the 'Average loan time' report so that the system preference setting item-level_itypes is taken into account. Before the patch, the report was always using the biblio level itemtype. With the patch, it will depend on the pref setting. To test: - Run various reports, compare results with both possible pref settings - Try limiting your search in various ways, especially on a specific itemtype - Make sure you switch checkboxes in testing, using itemtype as the row or as the column Notes: - Filtering on a specific itemtype on master was not working if the itemtype was chosen as column. The patch should fix that. - Fixes headings and breadcrumbs so that the name of the report is the same as on the reports start page Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com>
I won't use the splinter review, it will be confusing. 1/ There are 2 different ways to do the same thing: 247 if ( $line=~/itemtype/ ) { $line = $itype; } 257 $linefilter[0] = @$filters[9] if ($line =~ /itemtype/ ) ; and 263 if ( $column=~/itemtype/ ) { $column = $itype; } 274 $colfilter[0] = @$filters[9] if ($column =~ $itype ) ; 2/ Please use sql placeholders for "issuingrules.itemtype=$itype" (x3) 3/ if (( @colfilter ) and ($colfilter[1])){ $sth2->execute("'".$colfilter[0]."'","'".$colfilter[1]."'"); } elsif ($colfilter[0]) { $sth2->execute; } else { $sth2->execute; } Could be : if (( @colfilter ) and ($colfilter[1])){ $sth2->execute("'".$colfilter[0]."'","'".$colfilter[1]."'"); } else { $sth2->execute; }
Created attachment 43053 [details] [review] Bug 13452: Fix obvious issues in issues_avg_stats.pl - Remove joins with issuingrules which are not needed - Fix the SQL query so that $weight is now the value of COUNT(*) instead of a timestamp - QA fix: '$column eq $itype' instead of '$column =~ $itype' (same for $line)
QA fix #2 not needed due to the removal of joins QA fix #3: the whole 'if' block is unnecessary, and wrong, since there is no placeholders in query. Replaced by a simple $sth2->execute;
Created attachment 48987 [details] [review] Bug 13452: Fix obvious issues in issues_avg_stats.pl - Remove joins with issuingrules which are not needed - Fix the SQL query so that $weight is now the value of COUNT(*) instead of a timestamp - QA fix: '$column eq $itype' instead of '$column =~ $itype' (same for $line) Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
On reports/issues_avg_stats.pl, select row=patron category and column=item type DBD::mysql::st execute failed: You have an error in your SQL syntax
(In reply to Jonathan Druart from comment #10) > On reports/issues_avg_stats.pl, select row=patron category and column=item > type > > DBD::mysql::st execute failed: You have an error in your SQL syntax I tried just now and saw no errors. Generated SQL: SELECT borrowers.categorycode, items.itype, issuedate, returndate, COUNT(*) FROM `old_issues`,borrowers,biblioitems LEFT JOIN items ON (biblioitems.biblioitemnumber=items.biblioitemnumber) WHERE old_issues.itemnumber=items.itemnumber AND old_issues.borrowernumber=borrowers.borrowernumber group by borrowers.categorycode, items.itype, issuedate, returndate order by borrowers.categorycode, items.itype I tried to change the value of item-level_itypes syspref, but it works as well. Could you give it another try ?
Still the same. Actually $itype is set to undef because not defined in the same scope. If you are not using plack it should work correctly, with plack it will fail.
Created attachment 56246 [details] [review] Bug 13452: Fix for plack
Created attachment 63322 [details] [review] [SIGNED-OFF] Bug 13452: 'Average loan time' report to obey item-level_itypes preference Fixes the 'Average loan time' report so that the system preference setting item-level_itypes is taken into account. Before the patch, the report was always using the biblio level itemtype. With the patch, it will depend on the pref setting. To test: - Run various reports, compare results with both possible pref settings - Try limiting your search in various ways, especially on a specific itemtype - Make sure you switch checkboxes in testing, using itemtype as the row or as the column Notes: - Filtering on a specific itemtype on master was not working if the itemtype was chosen as column. The patch should fix that. - Fixes headings and breadcrumbs so that the name of the report is the same as on the reports start page Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 63323 [details] [review] [SIGNED-OFF] Bug 13452: Fix obvious issues in issues_avg_stats.pl - Remove joins with issuingrules which are not needed - Fix the SQL query so that $weight is now the value of COUNT(*) instead of a timestamp - QA fix: '$column eq $itype' instead of '$column =~ $itype' (same for $line) Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Created attachment 63324 [details] [review] [SIGNED-OFF] Bug 13452: Fix for plack Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
String changes.
Created attachment 64304 [details] [review] Bug 13452: 'Average loan time' report to obey item-level_itypes preference Fixes the 'Average loan time' report so that the system preference setting item-level_itypes is taken into account. Before the patch, the report was always using the biblio level itemtype. With the patch, it will depend on the pref setting. To test: - Run various reports, compare results with both possible pref settings - Try limiting your search in various ways, especially on a specific itemtype - Make sure you switch checkboxes in testing, using itemtype as the row or as the column Notes: - Filtering on a specific itemtype on master was not working if the itemtype was chosen as column. The patch should fix that. - Fixes headings and breadcrumbs so that the name of the report is the same as on the reports start page Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 64305 [details] [review] Bug 13452: Fix for plack Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 64306 [details] [review] Bug 13452: 'Average loan time' report to obey item-level_itypes preference Fixes the 'Average loan time' report so that the system preference setting item-level_itypes is taken into account. Before the patch, the report was always using the biblio level itemtype. With the patch, it will depend on the pref setting. To test: - Run various reports, compare results with both possible pref settings - Try limiting your search in various ways, especially on a specific itemtype - Make sure you switch checkboxes in testing, using itemtype as the row or as the column Notes: - Filtering on a specific itemtype on master was not working if the itemtype was chosen as column. The patch should fix that. - Fixes headings and breadcrumbs so that the name of the report is the same as on the reports start page Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 64307 [details] [review] Bug 13452: 'Average loan time' report to obey item-level_itypes preference Fixes the 'Average loan time' report so that the system preference setting item-level_itypes is taken into account. Before the patch, the report was always using the biblio level itemtype. With the patch, it will depend on the pref setting. To test: - Run various reports, compare results with both possible pref settings - Try limiting your search in various ways, especially on a specific itemtype - Make sure you switch checkboxes in testing, using itemtype as the row or as the column Notes: - Filtering on a specific itemtype on master was not working if the itemtype was chosen as column. The patch should fix that. - Fixes headings and breadcrumbs so that the name of the report is the same as on the reports start page Signed-off-by: Indranil Das Gupta (L2C2 Technologies) <indradg@gmail.com> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 64308 [details] [review] Bug 13452: Fix obvious issues in issues_avg_stats.pl - Remove joins with issuingrules which are not needed - Fix the SQL query so that $weight is now the value of COUNT(*) instead of a timestamp - QA fix: '$column eq $itype' instead of '$column =~ $itype' (same for $line) Signed-off-by: Srdjan <srdjan@catalyst.net.nz> Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 64309 [details] [review] Bug 13452: Fix for plack Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 17.11, thanks to everybody involved!
Pushed to 17.05.x, will be in 17.05.01
(In reply to Fridolin SOMERS from comment #25) > Pushed to 17.05.x, will be in 17.05.01 Oh 17.05.02 now
These patches have been pushed to 16.11.x and will be in 16.11.10.
Pushed to 16.05.x, for 16.05.15 release