Bug 14876

Summary: Show number of holds per record on the search results
Product: Koha Reporter: Kyle M Hall <kyle.m.hall>
Component: Hold requestsAssignee: Nick Clemens <nick>
Status: RESOLVED FIXED QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: amy, gmcharlt, jdemuth, jonathan.druart, josef.moravec, katrin.fischer, kyle, lisetteslatah, m.de.rooy, martin.renvoize, nick, nicole, sally.healey, tomascohen, veron
Version: master   
Hardware: All   
OS: All   
Whiteboard:
Change sponsored?: Sponsored Patch complexity: Trivial patch
Bot Control: --- When did the bot last check this:
Who signed the patch off: Text to go in the release notes:
Attachments: Bug 14876 - Show number of holds on the staff side next to 'holds' link
[SIGNED-OFF] Bug 14876 - Show number of holds on the staff side next to 'holds' link
Bug 14876 - (QA Followup) Fix pod
Bug 14876 - Show number of holds per record on the search results
Bug 14876 - Show number of holds per record on the search results
Bug 14876 - Show number of holds per record on the search results
Bug 14876 (QA Followup) Don't show (0) as holds counts when no holds
Bug 14876 - Show number of holds per record on the search results
Bug 14876 - Show number of holds per record on the search results
Bug 14876 - Show number of holds per record on the search results

Description Kyle M Hall 2015-09-22 15:02:37 UTC
Librarians have expressed that it would be useful to have the number of holds a record has on it display in the search results, preferably next to the "Holds" link for each record ( e.g. "Holds (3)" ).
Comment 1 Kyle M Hall 2015-09-22 15:06:39 UTC
Idea:

1) Create new module Koha::Template::Plugin::Holds with subroutine count( $biblionumber )
2) Modify koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt to call this plugin to display the count of holds per record
Comment 2 Nick Clemens 2016-07-14 17:55:54 UTC Comment hidden (obsolete)
Comment 3 Liz Rea 2016-07-18 04:03:03 UTC
Created attachment 53465 [details] [review]
[SIGNED-OFF] Bug 14876 - Show number of holds on the staff side next to 'holds' link

This patch adds a a Holds plugin for the templates to count existing
holds on a record.

We then use this in
koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/results.tt
to display the current number of holds on a record.

To test:
1 - Search in staff
2 - Note holds link has no extra information
3 - Apply patch
4 - prove -v t/db_dependent/Template/Plugin/Holds.t
5 - Do search in staff that returns records with some holds (i.e. place
        some holds)
6 - Note the number of holds now displays next to the text of the link:
    Holds (#)

Signed-off-by: Liz Rea <liz@catalyst.net.nz>
Comment 4 Jonathan Druart 2016-07-25 11:17:38 UTC
QA comments:
1/ POD of the plugin is wrong (c/p and not updated from another one).
2/ Since this plugin is only used once, why did you choose to use a TT plugin?
Comment 5 Nick Clemens 2016-08-18 16:12:21 UTC
Created attachment 54578 [details] [review]
Bug 14876 - (QA Followup) Fix pod
Comment 6 Nick Clemens 2016-08-18 16:19:28 UTC
(In reply to Jonathan Druart from comment #4)
> 2/ Since this plugin is only used once, why did you choose to use a TT
> plugin?

If we don't use this we'd have to iterate through the search results and counts the holds for each before display, this way we only check for those that are being currently shown.

We could also alter detail.tt to use this function
Comment 7 Jonathan Druart 2016-08-18 17:07:02 UTC
(In reply to Nick Clemens from comment #6)
> (In reply to Jonathan Druart from comment #4)
> > 2/ Since this plugin is only used once, why did you choose to use a TT
> > plugin?
> 
> If we don't use this we'd have to iterate through the search results and
> counts the holds for each before display, this way we only check for those
> that are being currently shown.

I am not sure I am following you, the results returned by searchResults are always displayed, isn't?

Since we already iterate in catalogue/search.pl it's not a big deal to do this job over there. I don't think it's the job of a plugin to calculate the number of holds for a given record.

Feel free to ask for another QA point of view ;)
Comment 8 Kyle M Hall 2016-08-22 10:21:38 UTC
I also think the plugin is a good solution. It keeps the code DRY. There is a distinct likelihood that we will want to display the holds count in other places in the future. The plugin means we only need to modify half the files we would need to otherwise.
Comment 9 Jonathan Druart 2016-08-22 11:27:17 UTC
CCing Katrin, Tomas, Marcel and Martin.

It's not clear to me when we need a TT plugin.
Sometimes it's obvious (get branch names, get patron category description, filter (json, date, price), pref values, etc.)
But sometimes it's not: Borrowers.IsDebarred, Borrowers.HasOverdues.
Holds.count is part of this second group.

I don't think plugins should be a lazy solution for developers.

We need to define when we can use a plugin, otherwise any methods from Koha modules could have their plugin wrapper.

And we will have to maintain:
  Koha::Holds->count({biblionumber => $biblionumber });
  Koha::Template::Plugin::Holds->count($biblionumber);
and
  Koha::REST::V1::Hold->count($biblionumber); # does not exist (yet)

(I don't switch to in discussion otherwise you will hate me)
Comment 10 Marcel de Rooy 2016-08-22 11:46:58 UTC
From catalogue/detail.pl:

#we only need to pass the number of holds to the template
my $holds = C4::Reserves::GetReservesFromBiblionumber({ biblionumber => $biblionumber, all_dates => 1 });
$template->param( holdcount => scalar ( @$holds ) );
Comment 11 Jonathan Druart 2016-08-22 11:59:13 UTC
I was also wondering why
  my $nb_holds = Koha::Holds->count({ biblionumber => $biblionumber })->count;
and not something like
  my $nb_holds = Koha::Biblios->find(biblionumber)->get_holds->count
Comment 12 Marcel de Rooy 2016-08-22 12:07:40 UTC
(In reply to Jonathan Druart from comment #11)
> and not something like
>   my $nb_holds = Koha::Biblios->find(biblionumber)->get_holds->count

Hm. Does not really look nice to me.
Apart from looking up the biblio record without using it..
Comment 13 Jonathan Druart 2016-09-07 10:05:24 UTC
ping Tomas, Katrin and Martin!
Comment 14 Nick Clemens 2016-09-28 15:31:29 UTC
Created attachment 55889 [details] [review]
Bug 14876 - Show number of holds per record on the search results

This patch edits C4/Search.pm to pass back the count of holds along with
other data for display in results.tt

To test:
1 - Search in staff
2 - Note holds link has no extra information
3 - Apply patch
4 - Do search in staff that returns records with some holds (i.e. place
                some holds)
5 - Note the number of holds now displays next to the text of the link:
    Holds (#)
Comment 15 Nick Clemens 2016-09-28 15:32:17 UTC
Updated patch to remove use of tt plugin
Comment 16 Marc Véron 2016-09-28 16:01:27 UTC
Created attachment 55892 [details] [review]
Bug 14876 - Show number of holds per record on the search results

This patch edits C4/Search.pm to pass back the count of holds along with
other data for display in results.tt

To test:
1 - Search in staff
2 - Note holds link has no extra information
3 - Apply patch
4 - Do search in staff that returns records with some holds (i.e. place
                some holds)
5 - Note the number of holds now displays next to the text of the link:
    Holds (#)

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 17 Jonathan Druart 2016-09-29 08:18:10 UTC
Nick,
Since this is only used on the search result on the staff interface, I'd prefer to get the number of holds from catalogue/search.pl (unless you plan to display this info somewhere else).
Comment 18 Nick Clemens 2016-09-29 09:55:38 UTC
Created attachment 55910 [details] [review]
Bug 14876 - Show number of holds per record on the search results

This patch edits catalogue/search.pl to pass back the count of holds along with
other data for display in results.tt

To test:
1 - Search in staff
2 - Note holds link has no extra information
3 - Apply patch
4 - Do search in staff that returns records with some holds (i.e. place
    some holds)
5 - Note the number of holds now displays next to the text of the link:
    Holds (#)

Signed-off-by: Marc Véron <veron@veron.ch>
Comment 19 Jonathan Druart 2016-10-03 11:49:22 UTC
Nick, the parenthesis are displayed even if there are no holds on the record.
Comment 20 Nick Clemens 2016-10-10 08:05:54 UTC
I prefer to show 'Holds (0)' when there are no holds, this is how it is currently done on the details page. 

If we want to display nothing then we should alter the details page to be 'Holds' instead of 'Holds (0)'

Happy to do it either way, but want it to be consistent.
Comment 21 Nick Clemens 2016-10-10 08:22:58 UTC
Created attachment 56082 [details] [review]
Bug 14876 (QA Followup)     Don't show (0) as holds counts when no holds
Comment 22 Jonathan Druart 2016-10-24 13:47:00 UTC
So it seems that you now use something that I did not want but was already pushed (and I QAed)
You created Koha::Template::Plugin::Hold->acount when Koha::Template::Plugin::Biblio->HoldsCount already existed...
Comment 23 Jonathan Druart 2016-10-24 13:49:57 UTC
But you are using the plugin for biblio-view-menu.inc but for not results.tt.
I am at lost here....
Comment 24 Jonathan Druart 2016-10-24 13:50:30 UTC
(In reply to Jonathan Druart from comment #23)
> But you are using the plugin for biblio-view-menu.inc but for not results.tt.
> I am at lost here....

s/but for not/but not for
Comment 25 Nick Clemens 2016-10-31 14:42:00 UTC
Created attachment 57000 [details] [review]
Bug 14876 - Show number of holds per record on the search results

This patch sues the existing plugin
Koha::Template::Plugin::Biblio->HoldsCount

To be consistent with current behaviour 'Holds (0)' is shown when
no holds have been placed on an item

To test:
1 - Search in staff
2 - Note holds link has no extra information
3 - Apply patch
4 - prove -v t/db_dependent/Template/Plugin/Holds.t
5 - Do search in staff that returns records with some holds (i.e. place
                some holds)
6 - Note the number of holds now displays next to the text of the link:
    Holds (#)
Comment 26 Kyle M Hall 2016-12-07 15:47:28 UTC
Created attachment 58013 [details] [review]
Bug 14876 - Show number of holds per record on the search results

This patch sues the existing plugin
Koha::Template::Plugin::Biblio->HoldsCount

To be consistent with current behaviour 'Holds (0)' is shown when
no holds have been placed on an item

To test:
1 - Search in staff
2 - Note holds link has no extra information
3 - Apply patch
4 - prove -v t/db_dependent/Template/Plugin/Holds.t
5 - Do search in staff that returns records with some holds (i.e. place
                some holds)
6 - Note the number of holds now displays next to the text of the link:
    Holds (#)

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>
Comment 27 Jonathan Druart 2016-12-13 12:26:43 UTC
Created attachment 58153 [details] [review]
Bug 14876 - Show number of holds per record on the search results

This patch sues the existing plugin
Koha::Template::Plugin::Biblio->HoldsCount

To be consistent with current behaviour 'Holds (0)' is shown when
no holds have been placed on an item

To test:
1 - Search in staff
2 - Note holds link has no extra information
3 - Apply patch
4 - prove -v t/db_dependent/Template/Plugin/Holds.t
5 - Do search in staff that returns records with some holds (i.e. place
                some holds)
6 - Note the number of holds now displays next to the text of the link:
    Holds (#)

Signed-off-by: Barbara Johnson <barbara.johnson@bedfordtx.gov>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 28 Kyle M Hall 2016-12-16 11:37:30 UTC
Pushed to master for 17.05, thanks Nick!
Comment 29 Katrin Fischer 2016-12-18 20:34:36 UTC
This won't get backported to 16.11.x as it is an enhancement.