Bug 23059 - reserves_stats.pl: Simplify reserve status handling
Summary: reserves_stats.pl: Simplify reserve status handling
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Emily Lamancusa
URL:
Keywords:
Depends on:
Blocks: 34988
  Show dependency treegraph
 
Reported: 2019-06-05 20:26 UTC by Andrew Fuerste-Henry
Modified: 2023-10-26 06:11 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00


Attachments
Bug 23059: reserve_stats.pl: Simplify reservestatus (5.58 KB, patch)
2023-05-23 15:06 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23059: (QA follow-up) Put status strings in a span (1.30 KB, patch)
2023-06-26 12:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 23059: reserve_stats.pl: Simplify reservestatus (5.59 KB, patch)
2023-06-28 15:23 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23059: (QA follow-up) Put status strings in a span (1.31 KB, patch)
2023-06-28 15:23 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 23059: reserve_stats.pl: Simplify reservestatus (5.67 KB, patch)
2023-10-04 14:13 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 23059: (QA follow-up) Put status strings in a span (1.38 KB, patch)
2023-10-04 14:13 UTC, Emily Lamancusa
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2019-06-05 20:26:23 UTC
This bug is sort of a catch-all for changes that need to be made to the holds statistic wizard, reserves_stats.pl

Two big issues:
- By default, the wizard sets a value for items.homebranch and items.holdingbranch. That means it unexpectedly cuts out all bib-level holds and gives no clear indication that it's done so. You *can* change home and holding branch to not limit, but it's not intuitive.

- The wizard splits holds into 5 statuses (Placed, Processed, Pending, Satisfied, and Cancelled) that aren't used anywhere else in Koha. These statuses aren't explained without diving into the code and don't match how we otherwise divide and talk about holds.
Comment 1 Ron Houk 2019-06-05 20:33:09 UTC
I have run into issues with reserves_stats.pl mentioned by Andrew where no bib level holds were showing up, and I didn't understand why.  I think it might be nice to make the defaults be so that all holds show up and then allow users to filter later on.

It might also be nice to have an explanation of the different statuses in the documentation.
Comment 2 Marcel de Rooy 2023-05-22 11:57:47 UTC
When you do not confirm a hold, Koha stored it without itemnumber in old holds when directly checking out to same patron.
These filled holds are not taken into consideration in statistics.
See further 33791/33792.
Comment 3 Marcel de Rooy 2023-05-22 12:47:37 UTC
(In reply to Andrew Fuerste-Henry from comment #0)

> Two big issues:
> - By default, the wizard sets a value for items.homebranch and
> items.holdingbranch. That means it unexpectedly cuts out all bib-level holds
> and gives no clear indication that it's done so. You *can* change home and
> holding branch to not limit, but it's not intuitive.

I submitted a tiny fix for this problem on bug 33792 with a possible major impact on your results :)
Comment 4 Marcel de Rooy 2023-05-22 12:49:39 UTC
(In reply to Andrew Fuerste-Henry from comment #0)
> 
> - The wizard splits holds into 5 statuses (Placed, Processed, Pending,
> Satisfied, and Cancelled) that aren't used anywhere else in Koha. These
> statuses aren't explained without diving into the code and don't match how
> we otherwise divide and talk about holds.

Yes this is weird code. Will try to improve that a bit. But it wont be the major overhaul you requested. But small steps may bring us further too.
Comment 5 Marcel de Rooy 2023-05-23 15:06:37 UTC
Created attachment 151579 [details] [review]
Bug 23059: reserve_stats.pl: Simplify reservestatus

This patch does the following:
[1] Go back to four statuses: Cancelled, Filled, Waiting or Placed.
    Placed is used as collective name for all other statuses: pending
    (placed), processing (found==P) or transit (found==T). Placed before
    anyway.
[2] Allow translation of these statuses in the template. Remove the sub
    reservestatushuman.
[3] The output of changeifreservestatus is considerably shorter and less
    ugly in the constructed sql statement.

Test plan:
[1] Use reservestatus as row, as column and only as filter (clicking few
    statuses).
[2] Verify that the shown statistics meet your expectations.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2023-05-30 12:30:13 UTC
NOTE for interested ones:

The former code
-                   when priority>0 then 1
-                   when priority=0 then
-                       (case
-                          when found='f' then 4
-                          when found='w' then
-                          (case
-                           when cancellationdate is null then 3
-                           else 5
-                           end )
-                          else 2
-                        end )
made a lot of cancelled holds count under processing status 2 ! The only ones that were counted were those which were already waiting.
This bug is resolved now too.
Comment 7 Marcel de Rooy 2023-05-30 12:37:52 UTC
Most cancelled holds look like:
found, cancellationdate
| NULL  | 2022-05-16       |

Only cancelled waits look like:
found, cancellationdate
| W     | 2022-06-16       |
Comment 8 Marcel de Rooy 2023-06-26 12:01:38 UTC
Created attachment 152679 [details] [review]
Bug 23059: (QA follow-up) Put status strings in a span

No test plan.
Read the patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Owen Leonard 2023-06-28 15:23:44 UTC
Created attachment 152824 [details] [review]
Bug 23059: reserve_stats.pl: Simplify reservestatus

This patch does the following:
[1] Go back to four statuses: Cancelled, Filled, Waiting or Placed.
    Placed is used as collective name for all other statuses: pending
    (placed), processing (found==P) or transit (found==T). Placed before
    anyway.
[2] Allow translation of these statuses in the template. Remove the sub
    reservestatushuman.
[3] The output of changeifreservestatus is considerably shorter and less
    ugly in the constructed sql statement.

Test plan:
[1] Use reservestatus as row, as column and only as filter (clicking few
    statuses).
[2] Verify that the shown statistics meet your expectations.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 10 Owen Leonard 2023-06-28 15:23:46 UTC
Created attachment 152825 [details] [review]
Bug 23059: (QA follow-up) Put status strings in a span

No test plan.
Read the patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 11 Marcel de Rooy 2023-06-29 05:39:19 UTC
(In reply to Owen Leonard from comment #10)

> Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Thx !
Comment 12 Jonathan Druart 2023-09-01 10:02:42 UTC
Why not using reservestatus for 

+                <input type="checkbox" name="filter_reservestatus_or_C" value="C" /> Cancelled
+                <input type="checkbox" name="filter_reservestatus_or_F" value="F" /> Filled
+                <input type="checkbox" name="filter_reservestatus_or_P" value="P" /> Placed
+                <input type="checkbox" name="filter_reservestatus_or_W" value="W" /> Waiting

?
Comment 13 Marcel de Rooy 2023-09-02 12:54:59 UTC
(In reply to Jonathan Druart from comment #12)
Thanks for QA.

> Why not using reservestatus for 
> 
> +                <input type="checkbox" name="filter_reservestatus_or_C"
> value="C" /> Cancelled
> +                <input type="checkbox" name="filter_reservestatus_or_F"
> value="F" /> Filled
> +                <input type="checkbox" name="filter_reservestatus_or_P"
> value="P" /> Placed
> +                <input type="checkbox" name="filter_reservestatus_or_W"
> value="W" /> Waiting
> 
> ?

For several reasons:
[1] Completely improving this part of code is not in scope. Look at the changes here. They improve and resolve a bug. Further work can be done, but is not required here.
[2] The reservestatus refers to the routine in C4/Reserves. Which has some interesting comments:
## FIXME: I don't think this does what it thinks it does.
## It only ever checks the first reserve result, even though
## multiple reserves for that bib can have the itemnumber set
## the sub is only used once in the codebase.
It does not know Cancelled or Filled which are important in reporting here.
So in short, that routine needs attention. Cannot be forced here.
[3] This script needs a solution in SQL not in Perl. We cannot call Perl for each row in the query and impact performance negatively. Implementing this needs much more changes than the current scope requires.

Please have another look.
Comment 14 Emily Lamancusa 2023-10-04 14:13:24 UTC
Created attachment 156548 [details] [review]
Bug 23059: reserve_stats.pl: Simplify reservestatus

This patch does the following:
[1] Go back to four statuses: Cancelled, Filled, Waiting or Placed.
    Placed is used as collective name for all other statuses: pending
    (placed), processing (found==P) or transit (found==T). Placed before
    anyway.
[2] Allow translation of these statuses in the template. Remove the sub
    reservestatushuman.
[3] The output of changeifreservestatus is considerably shorter and less
    ugly in the constructed sql statement.

Test plan:
[1] Use reservestatus as row, as column and only as filter (clicking few
    statuses).
[2] Verify that the shown statistics meet your expectations.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 15 Emily Lamancusa 2023-10-04 14:13:26 UTC
Created attachment 156549 [details] [review]
Bug 23059: (QA follow-up) Put status strings in a span

No test plan.
Read the patch.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 16 Emily Lamancusa 2023-10-04 14:25:07 UTC
This is a nice improvement to clarity and consistency. Works as intended and QA tool is happy.

One thing that would be nice to see as a follow-up would be the ability for libraries that have in-processing and in-transit statuses to be able to separate those out from "Placed". But that requires extra processing to determine whether those statuses are relevant to the library, so it's a bit out of scope for this bug, and doesn't need to block the alignment of the report with the universally-relevant hold statuses.
Comment 17 Marcel de Rooy 2023-10-05 06:10:29 UTC
(In reply to Emily Lamancusa from comment #15)
> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>

Thx Emily !
Comment 18 Marcel de Rooy 2023-10-05 06:13:25 UTC
(In reply to Emily Lamancusa from comment #16)
> This is a nice improvement to clarity and consistency. Works as intended and
> QA tool is happy.
> 
> One thing that would be nice to see as a follow-up would be the ability for
> libraries that have in-processing and in-transit statuses to be able to
> separate those out from "Placed". But that requires extra processing to
> determine whether those statuses are relevant to the library, so it's a bit
> out of scope for this bug, and doesn't need to block the alignment of the
> report with the universally-relevant hold statuses.

Opened bug 34988
Comment 19 Tomás Cohen Arazi 2023-10-20 17:46:04 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 20 Fridolin Somers 2023-10-26 06:11:42 UTC
Enhancement not pushed to 23.05.x