Bug 19618

Summary: Add 'Club Holds' feature
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Hold requestsAssignee: Agustín Moyano <agustinmoyano>
Status: CLOSED FIXED QA Contact: Kyle M Hall <kyle>
Severity: new feature    
Priority: P5 - low CC: 1joynelson, andrewfh, gmcharlt, jonathan.druart, josef.moravec, katrin.fischer, kyle, liz, magnus, margaret, martin.renvoize, tomascohen
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This new feature adds the ability for clubs to place a hold for bibs. When such a hold is placed, in the background a hold will be automatically placed for each member of the group in random order.
Version(s) released in:
19.11.00
Bug Depends on:    
Bug Blocks: 23710, 23857, 24410, 24546, 24587, 27330    
Attachments: Bug 19618: Add club_holds and club_holds_to_patron_holds tables
Bug 19618: [DO NOT PUSH] (follow-up) Schema structure
Bug 19618: Add ability to place holds for members of a club in intranet
Bug 19618: Add api endpoint for club holds
Bug 19618: Add tests
Bug 19618: Add club_holds and club_holds_to_patron_holds tables
Bug 19618: [DO NOT PUSH] (follow-up) Schema structure
Bug 19618: Add ability to place holds for members of a club in intranet
Bug 19618: Add api endpoint for club holds
Bug 19618: Add tests
Bug 19618: (QA follow-up) POD Fix
Bug 19618: Add club_holds and club_holds_to_patron_holds tables
Bug 19618: [DO NOT PUSH] (follow-up) Schema structure
Bug 19618: Add ability to place holds for members of a club in intranet
Bug 19618: Add api endpoint for club holds
Bug 19618: Add tests
Bug 19618: (QA follow-up) POD Fix

Description Kyle M Hall 2017-11-13 16:03:36 UTC
1) Add a pulldown and button labeled "Add randomized holds" to the holds page for each bib
Comment 1 Magnus Enger 2017-11-27 08:57:11 UTC
Not sure I understand this one. Is it similar/related to Bug 8809 - Cascading Holds?
Comment 2 Agustín Moyano 2019-07-10 13:59:26 UTC
This bug is intended to add the ability for clubs to place a hold for bibs

This means that when a club places a hold, what really happens is that a hold will automatically be placed for each member in random order.
Comment 3 Agustín Moyano 2019-09-10 07:11:35 UTC
Created attachment 92674 [details] [review]
Bug 19618: Add club_holds and club_holds_to_patron_holds tables

This patch adds 2 new tables
1. club_holds
2. club_holds_to_patron_holds

They are ment to keep info about hold requests made in name of a club

To test:
1) apply this patch
2) perl installer/data/mysql/updatedatabase.pl
SUCCESS => 2 new tables were created
Comment 4 Agustín Moyano 2019-09-10 07:11:39 UTC
Created attachment 92675 [details] [review]
Bug 19618: [DO NOT PUSH] (follow-up) Schema structure
Comment 5 Agustín Moyano 2019-09-10 07:11:43 UTC
Created attachment 92676 [details] [review]
Bug 19618: Add ability to place holds for members of a club in intranet

This patch adds the ability to place a hold for each member of a club in random order.

To test:
1) apply this patch
2) create 2 clubs, (club names should have some part in common, and some part different for each other)
3) in one of them add at least 6 members
4) enter patron clubs management and click on "Actions" button
SUCCESS.1 => club with members has a new action called "search to hold"
5) click on search to hold
SUCCESS.2 => in the list of bilios there appears an action called "Place hold for <club name>"
6) click on "Place hold for <club name>"
SUCCESS.3 => a new window appears where you can select pickup location (defaults to club's library, if any), and the list of members.
7) go back to the list of bilios in the catalog and click on "Forget <club name>"
8) click on "Holds" action of any biblio
SUCCESS.4 => a search box appears with two tabs: Patrons and Clubs
9) click on Clubs tab and search by the common part of clubs names
SUCCESS.5 => a list of clubs that matches the search appears. If you click on any of them, the same page as SUCCESS.3 appears.
10) go back to the search box in SUCCESS.4 and search by the different part of the name.
SUCCESS.6 => because there is only one club that matches search criteria, the same page as SUCCESS.3 appears;
11) Sign off
Comment 6 Agustín Moyano 2019-09-10 07:11:46 UTC
Created attachment 92677 [details] [review]
Bug 19618: Add api endpoint for club holds

This patch adds an endpoint in thi api in /api/v1/clubs/{club_id}/holds whith the verb POST that maps to Koha::REST::V1::Clubs::Holds#add controller.
Classes for club_holds and club_holds_to_patron_holds new tables where also added.

To test:
1) Reach SUCCESS.3 test point of previous patch with club that has no enrollments
2) Click on "Place Hold"
SUCCESS => an alert should appear that you cannot place hold on a club without patrons
3) Reach SUCCESS.3 test point of previous patch with club that has enrollments
4) Click on "Place Hold"
SUCCESS => holds priority list should appear with holds for every patron in club
5) Repeat steps 3 and 4.
SUCCESS => new holds should appear in different order
6) Sign off
Comment 7 Agustín Moyano 2019-09-10 07:11:50 UTC
Created attachment 92678 [details] [review]
Bug 19618: Add tests

This patch adds tests for new features

To test:
1) prove t/db_dependent/Koha/Club/Enrollment.t
2) prove t/db_dependent/Koha/Club/Hold.t
3) prove t/db_dependent/api/v1/clubs_holds.t
4) Sign off
Comment 8 Tomás Cohen Arazi 2019-09-11 14:37:38 UTC
Agustín, you need to add the proposed endpoints here:
https://wiki.koha-community.org/wiki/REST_api_RFCs

I suggest you copy and paste another RFC. As this is using new tables, you probably don't need to propose attribute name mappings.

My 2 cents.
Comment 9 Martin Renvoize 2019-09-12 15:58:58 UTC
Created attachment 92746 [details] [review]
Bug 19618: Add club_holds and club_holds_to_patron_holds tables

This patch adds 2 new tables
1. club_holds
2. club_holds_to_patron_holds

They are ment to keep info about hold requests made in name of a club

To test:
1) apply this patch
2) perl installer/data/mysql/updatedatabase.pl
SUCCESS => 2 new tables were created

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2019-09-12 15:59:02 UTC
Created attachment 92747 [details] [review]
Bug 19618: [DO NOT PUSH] (follow-up) Schema structure

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2019-09-12 15:59:06 UTC
Created attachment 92748 [details] [review]
Bug 19618: Add ability to place holds for members of a club in intranet

This patch adds the ability to place a hold for each member of a club in random order.

To test:
1) apply this patch
2) create 2 clubs, (club names should have some part in common, and some part different for each other)
3) in one of them add at least 6 members
4) enter patron clubs management and click on "Actions" button
SUCCESS.1 => club with members has a new action called "search to hold"
5) click on search to hold
SUCCESS.2 => in the list of bilios there appears an action called "Place hold for <club name>"
6) click on "Place hold for <club name>"
SUCCESS.3 => a new window appears where you can select pickup location (defaults to club's library, if any), and the list of members.
7) go back to the list of bilios in the catalog and click on "Forget <club name>"
8) click on "Holds" action of any biblio
SUCCESS.4 => a search box appears with two tabs: Patrons and Clubs
9) click on Clubs tab and search by the common part of clubs names
SUCCESS.5 => a list of clubs that matches the search appears. If you click on any of them, the same page as SUCCESS.3 appears.
10) go back to the search box in SUCCESS.4 and search by the different part of the name.
SUCCESS.6 => because there is only one club that matches search criteria, the same page as SUCCESS.3 appears;
11) Sign off

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2019-09-12 15:59:11 UTC
Created attachment 92749 [details] [review]
Bug 19618: Add api endpoint for club holds

This patch adds an endpoint in thi api in /api/v1/clubs/{club_id}/holds whith the verb POST that maps to Koha::REST::V1::Clubs::Holds#add controller.
Classes for club_holds and club_holds_to_patron_holds new tables where also added.

To test:
1) Reach SUCCESS.3 test point of previous patch with club that has no enrollments
2) Click on "Place Hold"
SUCCESS => an alert should appear that you cannot place hold on a club without patrons
3) Reach SUCCESS.3 test point of previous patch with club that has enrollments
4) Click on "Place Hold"
SUCCESS => holds priority list should appear with holds for every patron in club
5) Repeat steps 3 and 4.
SUCCESS => new holds should appear in different order
6) Sign off

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2019-09-12 15:59:14 UTC
Created attachment 92750 [details] [review]
Bug 19618: Add tests

This patch adds tests for new features

To test:
1) prove t/db_dependent/Koha/Club/Enrollment.t
2) prove t/db_dependent/Koha/Club/Hold.t
3) prove t/db_dependent/api/v1/clubs_holds.t
4) Sign off

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 14 Martin Renvoize 2019-09-12 15:59:19 UTC
Created attachment 92751 [details] [review]
Bug 19618: (QA follow-up) POD Fix

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize 2019-09-12 16:04:15 UTC
This all seems to work well enough to me.. Signing off..

Some QA notes though.. What happens if the club is disbanded (deleted).. should the holds also be removed.. I don't see anything here doing that?
Comment 16 Martin Renvoize 2019-09-12 16:04:46 UTC
Also.. was this sponsored by anyone?
Comment 17 Tomás Cohen Arazi 2019-09-12 16:10:25 UTC
(In reply to Martin Renvoize from comment #16)
> Also.. was this sponsored by anyone?

Southeast Kansas Library - SEKLS
Comment 18 Agustín Moyano 2019-09-12 17:33:42 UTC
(In reply to Martin Renvoize from comment #15)
> This all seems to work well enough to me.. Signing off..
> 
> Some QA notes though.. What happens if the club is disbanded (deleted)..
> should the holds also be removed.. I don't see anything here doing that?

Hi Martin, thanks for taking the time.

I really wouldn't touch holds that were already placed, even if club was deleted. 

Patrons may depend on that hold.
Comment 19 Kyle M Hall 2019-09-30 13:39:35 UTC
Created attachment 93253 [details] [review]
Bug 19618: Add club_holds and club_holds_to_patron_holds tables

This patch adds 2 new tables
1. club_holds
2. club_holds_to_patron_holds

They are ment to keep info about hold requests made in name of a club

To test:
1) apply this patch
2) perl installer/data/mysql/updatedatabase.pl
SUCCESS => 2 new tables were created

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Sponsored-by: Southeast Kansas Library - SEKLS

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall 2019-09-30 13:39:49 UTC
Created attachment 93254 [details] [review]
Bug 19618: [DO NOT PUSH] (follow-up) Schema structure

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Sponsored-by: Southeast Kansas Library - SEKLS

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Kyle M Hall 2019-09-30 13:39:52 UTC
Created attachment 93255 [details] [review]
Bug 19618: Add ability to place holds for members of a club in intranet

This patch adds the ability to place a hold for each member of a club in random order.

To test:
1) apply this patch
2) create 2 clubs, (club names should have some part in common, and some part different for each other)
3) in one of them add at least 6 members
4) enter patron clubs management and click on "Actions" button
SUCCESS.1 => club with members has a new action called "search to hold"
5) click on search to hold
SUCCESS.2 => in the list of bilios there appears an action called "Place hold for <club name>"
6) click on "Place hold for <club name>"
SUCCESS.3 => a new window appears where you can select pickup location (defaults to club's library, if any), and the list of members.
7) go back to the list of bilios in the catalog and click on "Forget <club name>"
8) click on "Holds" action of any biblio
SUCCESS.4 => a search box appears with two tabs: Patrons and Clubs
9) click on Clubs tab and search by the common part of clubs names
SUCCESS.5 => a list of clubs that matches the search appears. If you click on any of them, the same page as SUCCESS.3 appears.
10) go back to the search box in SUCCESS.4 and search by the different part of the name.
SUCCESS.6 => because there is only one club that matches search criteria, the same page as SUCCESS.3 appears;
11) Sign off

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Sponsored-by: Southeast Kansas Library - SEKLS

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 22 Kyle M Hall 2019-09-30 13:39:55 UTC
Created attachment 93256 [details] [review]
Bug 19618: Add api endpoint for club holds

This patch adds an endpoint in thi api in /api/v1/clubs/{club_id}/holds whith the verb POST that maps to Koha::REST::V1::Clubs::Holds#add controller.
Classes for club_holds and club_holds_to_patron_holds new tables where also added.

To test:
1) Reach SUCCESS.3 test point of previous patch with club that has no enrollments
2) Click on "Place Hold"
SUCCESS => an alert should appear that you cannot place hold on a club without patrons
3) Reach SUCCESS.3 test point of previous patch with club that has enrollments
4) Click on "Place Hold"
SUCCESS => holds priority list should appear with holds for every patron in club
5) Repeat steps 3 and 4.
SUCCESS => new holds should appear in different order
6) Sign off

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Sponsored-by: Southeast Kansas Library - SEKLS

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 23 Kyle M Hall 2019-09-30 13:39:58 UTC
Created attachment 93257 [details] [review]
Bug 19618: Add tests

This patch adds tests for new features

To test:
1) prove t/db_dependent/Koha/Club/Enrollment.t
2) prove t/db_dependent/Koha/Club/Hold.t
3) prove t/db_dependent/api/v1/clubs_holds.t
4) Sign off

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Sponsored-by: Southeast Kansas Library - SEKLS

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 24 Kyle M Hall 2019-09-30 13:40:01 UTC
Created attachment 93258 [details] [review]
Bug 19618: (QA follow-up) POD Fix

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Sponsored-by: Southeast Kansas Library - SEKLS

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 25 Martin Renvoize 2019-10-01 07:14:01 UTC
Nice work!

Pushed to master for 19.11.00
Comment 26 Jonathan Druart 2020-01-13 10:16:46 UTC
-                    <form action="placerequest.pl" method="post" name="form" id="hold-request-form">
+                    <form action="/api/v1/holds" method="post" name="form" id="hold-request-form">


Was this change expected?

Either it was and we should remove placerequest.pl as it was the only occurrence, or we should restore the call.
Comment 27 Jonathan Druart 2020-01-13 10:22:30 UTC
At first glance I'd say there is a item_type (expected by Koha::REST::V1::Holds) vs itemtype (sent by request.tt) issue
Also reserve_date vs hold_date

Either I am missing something or this patchset was not ready to be pushed.
Comment 28 Tomás Cohen Arazi 2020-01-13 12:02:26 UTC
(In reply to Jonathan Druart from comment #27)
> At first glance I'd say there is a item_type (expected by
> Koha::REST::V1::Holds) vs itemtype (sent by request.tt) issue
> Also reserve_date vs hold_date
> 
> Either I am missing something or this patchset was not ready to be pushed.

Can you file a bug for that?
Comment 29 Tomás Cohen Arazi 2020-01-13 12:07:30 UTC
(In reply to Tomás Cohen Arazi from comment #28)
> (In reply to Jonathan Druart from comment #27)
> > At first glance I'd say there is a item_type (expected by
> > Koha::REST::V1::Holds) vs itemtype (sent by request.tt) issue
> > Also reserve_date vs hold_date
> > 
> > Either I am missing something or this patchset was not ready to be pushed.
> 
> Can you file a bug for that?

We need to restore the call to placerequest.pl
Comment 30 Jonathan Druart 2020-01-13 12:50:18 UTC
I still do not manage to find regression, and found:

1043                 if($('.datepickerfrom_hidden').length)
1044                     data.hold_date = $('.datepickerfrom_hidden').val()||null;
1045                 if($('input[name="itemtype"]').length) {
1046                     data.item_type = $('input[name="itemtype"]').val()||null;
1047                 }

Are we safe?
Comment 31 Tomás Cohen Arazi 2020-01-13 13:14:52 UTC
(In reply to Jonathan Druart from comment #30)
> I still do not manage to find regression, and found:
> 
> 1043                 if($('.datepickerfrom_hidden').length)
> 1044                     data.hold_date =
> $('.datepickerfrom_hidden').val()||null;
> 1045                 if($('input[name="itemtype"]').length) {
> 1046                     data.item_type =
> $('input[name="itemtype"]').val()||null;
> 1047                 }
> 
> Are we safe?

My only concern is not about attribute mappings, but on the multi_hold use case.
Comment 32 Jonathan Druart 2020-01-13 13:36:40 UTC
See bug 24410.
Comment 33 Jonathan Druart 2020-02-26 16:03:47 UTC
Can you explain what is the purpose of the new table?

club_holds_to_patron_holds.error_code seems super odd, it takes the return of CanBookBeReserved and put it in DB??

How is this table populated? Only from an API endpoint that is not used from Koha?
Comment 34 Agustín Moyano 2020-02-26 16:50:31 UTC
Hi Jonathan, I'll respond between lines:

(In reply to Jonathan Druart from comment #33)
> Can you explain what is the purpose of the new table?

This bug adds 2 new tables:

club_holds and club_holds_to_patron_holds

club_holds has the information of a hold request or error for a certain club.

 
> club_holds_to_patron_holds.error_code seems super odd, it takes the return
> of CanBookBeReserved and put it in DB??

Think of club_holds_to_patron_holds table more like a log.. it has the information of which holds where generated from a certain club hold, and in case of error, it has the information of the error message, error code and patron.
 
So, if someone, someday wants to create a report on how many holds where made through club holds, they have the info.

> How is this table populated? Only from an API endpoint that is not used from
> Koha?

It's populated in the API endpoint, but it's used in koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt line 150

<form action="/api/v1/clubs/[% club.id | html %]/holds" method="post" name="form" id="club-request-form">
Comment 35 Jonathan Druart 2020-02-26 17:04:39 UTC
Thanks Agustin, it's a bit more clear.

More questions then :)
Is there a plan to have a UI view of club_holds_to_patron_holds?
If not, what's the point of it? Could not we replace it with entries in action_logs?

The enum on error_code is weird, and will lead to issues: everytime a new status is returned by the subroutine we will need to adjust the enum. It seems that it should be a simple varchar.
By the way what is the difference between error_code and error_message? (Documentation is missing in kohastructure.sql :)
Comment 36 Agustín Moyano 2020-02-26 17:57:34 UTC
(In reply to Jonathan Druart from comment #35)
> Thanks Agustin, it's a bit more clear.
> 
> More questions then :)
> Is there a plan to have a UI view of club_holds_to_patron_holds?

I don't remember exactly what we planned for this table, but I do remember that we created them for traceability. 

But it's not a bad idea to have a "club holds history" in a near future.

> If not, what's the point of it? Could not we replace it with entries in
> action_logs?

I didn't know action_logs table existed.. I should study it.

> The enum on error_code is weird, and will lead to issues: everytime a new
> status is returned by the subroutine we will need to adjust the enum. It
> seems that it should be a simple varchar.

You're right.. It's a bad design

> By the way what is the difference between error_code and error_message?
> (Documentation is missing in kohastructure.sql :)

error_code holds the result of C4::Reserves::CanItemBeReserved or C4::Reserves::CanBookBeReserved depending if you are trying to hold on a item or a biblio.

error_message says "Could not create hold for Patron(".$patron_id.")" if C4::Reserves::AddReserve fails.. 

maybe we can have only one column of error messages.
Comment 37 Martin Renvoize 2020-02-26 22:18:06 UTC
(In reply to Agustín Moyano from comment #36)
> (In reply to Jonathan Druart from comment #35)
> > Thanks Agustin, it's a bit more clear.
> > 
> > More questions then :)
> > Is there a plan to have a UI view of club_holds_to_patron_holds?
> 
> I don't remember exactly what we planned for this table, but I do remember
> that we created them for traceability. 
> 
> But it's not a bad idea to have a "club holds history" in a near future.
> 
> > If not, what's the point of it? Could not we replace it with entries in
> > action_logs?
> 
> I didn't know action_logs table existed.. I should study it.
> 
> > The enum on error_code is weird, and will lead to issues: everytime a new
> > status is returned by the subroutine we will need to adjust the enum. It
> > seems that it should be a simple varchar.
> 
> You're right.. It's a bad design
> 
> > By the way what is the difference between error_code and error_message?
> > (Documentation is missing in kohastructure.sql :)
> 
> error_code holds the result of C4::Reserves::CanItemBeReserved or
> C4::Reserves::CanBookBeReserved depending if you are trying to hold on a
> item or a biblio.
> 
> error_message says "Could not create hold for Patron(".$patron_id.")" if
> C4::Reserves::AddReserve fails.. 
> 
> maybe we can have only one column of error messages.

This has a translation code smell to me.. adding such a message in the database means it's not translatable.. we should stick to codes and then use the templates to allow translation.
Comment 38 Jonathan Druart 2020-08-27 09:03:09 UTC
How do we add a hold for club at item level?
It's all what I see on the form: https://snipboard.io/cOIK8q.jpg

ie. how do I fill club_holds.item_id with something?
Comment 39 Andrew Fuerste-Henry 2021-01-04 15:19:27 UTC
This wasn't ever intended to place item-level holds. We're placing a bunch of bib-level holds for our club patrons in a random order, with the expectation that the bib will contain multiple copies of the same title and everyone will get a copy.