|
Description
Kyle M Hall (khall)
2017-11-13 16:03:36 UTC
Not sure I understand this one. Is it similar/related to Bug 8809 - Cascading Holds? 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. 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 Created attachment 92675 [details] [review] Bug 19618: [DO NOT PUSH] (follow-up) Schema structure 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 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 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 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. 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> Created attachment 92747 [details] [review] Bug 19618: [DO NOT PUSH] (follow-up) Schema structure Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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> 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> 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> Created attachment 92751 [details] [review] Bug 19618: (QA follow-up) POD Fix Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> 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? Also.. was this sponsored by anyone? (In reply to Martin Renvoize from comment #16) > Also.. was this sponsored by anyone? Southeast Kansas Library - SEKLS (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. 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> 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> 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> 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> 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> 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> Nice work! Pushed to master for 19.11.00 - <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. 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. (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? (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 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?
(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. See bug 24410. 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? 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"> 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 :) (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. (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. 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? 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. |
