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.
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.
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.
(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 :)
(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.
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>
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.
Most cancelled holds look like: found, cancellationdate | NULL | 2022-05-16 | Only cancelled waits look like: found, cancellationdate | W | 2022-06-16 |
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>
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>
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>
(In reply to Owen Leonard from comment #10) > Signed-off-by: Owen Leonard <oleonard@myacpl.org> Thx !
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 ?
(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.
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>
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>
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.
(In reply to Emily Lamancusa from comment #15) > Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Thx Emily !
(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
Pushed to master for 23.11. Nice work everyone, thanks!
Enhancement not pushed to 23.05.x