Bug 13452 - Average checkout report always uses biblioitems.itemtype
Summary: Average checkout report always uses biblioitems.itemtype
Status: Pushed to Stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Julian Maurice
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-12-12 16:40 UTC by Fridolin SOMERS
Modified: 2017-07-31 11:26 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 13452: 'Average loan time' report to obey item-level_itypes preference (9.09 KB, patch)
2015-06-09 00:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13452: 'Average loan time' report to obey item-level_itypes preference (9.12 KB, patch)
2015-06-09 00:19 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 13452: 'Average loan time' report to obey item-level_itypes preference (9.19 KB, patch)
2015-07-05 21:55 UTC, Indranil Das Gupta
Details | Diff | Splinter Review
Bug 13452: Fix obvious issues in issues_avg_stats.pl (4.41 KB, patch)
2015-10-02 09:39 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13452: Fix obvious issues in issues_avg_stats.pl (4.45 KB, patch)
2016-03-11 01:46 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Bug 13452: Fix for plack (1.06 KB, patch)
2016-10-12 10:40 UTC, Julian Maurice
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13452: 'Average loan time' report to obey item-level_itypes preference (9.28 KB, patch)
2017-05-10 09:06 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13452: Fix obvious issues in issues_avg_stats.pl (4.52 KB, patch)
2017-05-10 09:06 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13452: Fix for plack (1.12 KB, patch)
2017-05-10 09:06 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 13452: 'Average loan time' report to obey item-level_itypes preference (9.32 KB, patch)
2017-06-15 10:50 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13452: Fix for plack (1.17 KB, patch)
2017-06-15 10:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13452: 'Average loan time' report to obey item-level_itypes preference (9.32 KB, patch)
2017-06-15 10:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13452: 'Average loan time' report to obey item-level_itypes preference (9.32 KB, patch)
2017-06-15 10:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13452: Fix obvious issues in issues_avg_stats.pl (4.56 KB, patch)
2017-06-15 10:53 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13452: Fix for plack (1.17 KB, patch)
2017-06-15 10:53 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin SOMERS 2014-12-12 16:40:51 UTC
The average checkout report reports/issues_avg_stats.pl always uses biblioitems.itemtype, regardless to item-level_itypes system preference.
Comment 1 Katrin Fischer 2015-06-09 00:10:38 UTC Comment hidden (obsolete)
Comment 2 Katrin Fischer 2015-06-09 00:19:02 UTC Comment hidden (obsolete)
Comment 3 M. Tompsett 2015-07-05 04:31:10 UTC
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 4 Katrin Fischer 2015-07-05 11:36:27 UTC
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.
Comment 5 Indranil Das Gupta 2015-07-05 21:55:00 UTC
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>
Comment 6 Jonathan Druart 2015-07-09 11:41:56 UTC
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;
     }
Comment 7 Julian Maurice 2015-10-02 09:39:07 UTC Comment hidden (obsolete)
Comment 8 Julian Maurice 2015-10-02 09:41:30 UTC
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;
Comment 9 Srdjan Jankovic 2016-03-11 01:46:44 UTC
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>
Comment 10 Jonathan Druart 2016-03-14 15:43:46 UTC
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
Comment 11 Julian Maurice 2016-08-19 13:55:40 UTC
(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 ?
Comment 12 Jonathan Druart 2016-08-19 14:40:27 UTC
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.
Comment 13 Julian Maurice 2016-10-12 10:40:12 UTC
Created attachment 56246 [details] [review]
Bug 13452: Fix for plack
Comment 14 Josef Moravec 2017-05-10 09:06:33 UTC
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>
Comment 15 Josef Moravec 2017-05-10 09:06:50 UTC
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>
Comment 16 Josef Moravec 2017-05-10 09:06:58 UTC
Created attachment 63324 [details] [review]
[SIGNED-OFF] Bug 13452: Fix for plack

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 17 Marcel de Rooy 2017-05-19 09:15:22 UTC
String changes.
Comment 18 Kyle M Hall 2017-06-15 10:50:44 UTC
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>
Comment 19 Kyle M Hall 2017-06-15 10:51:12 UTC
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>
Comment 20 Kyle M Hall 2017-06-15 10:51:52 UTC
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>
Comment 21 Kyle M Hall 2017-06-15 10:53:03 UTC
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>
Comment 22 Kyle M Hall 2017-06-15 10:53:24 UTC
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>
Comment 23 Kyle M Hall 2017-06-15 10:53:54 UTC
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>
Comment 24 Jonathan Druart 2017-06-15 19:07:28 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 25 Fridolin SOMERS 2017-06-27 14:17:03 UTC
Pushed to 17.05.x, will be in 17.05.01
Comment 26 Fridolin SOMERS 2017-06-27 14:18:59 UTC
(In reply to Fridolin SOMERS from comment #25)
> Pushed to 17.05.x, will be in 17.05.01
Oh 17.05.02 now
Comment 27 Katrin Fischer 2017-06-27 21:26:22 UTC
These patches have been pushed to 16.11.x and will be in 16.11.10.
Comment 28 Mason James 2017-07-31 11:26:19 UTC
Pushed to 16.05.x, for 16.05.15 release