| 
 
        
          Description
        
        
          Fridolin Somers
        
        
        
        
          2014-12-12 16:40:51 UTC
        
       
    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  |