|
Description
Aleisha Amohia
2020-02-24 20:04:23 UTC
Created attachment 99540 [details] [review] Bug 24718: Introducing HourBasedHolds system preference Sponsored-by: Waikato Institute of Technology Created attachment 99541 [details] [review] Bug 24718: Get reservedate and expirationdate to use datetime Sponsored-by: Waikato Institute of Technology Created attachment 99542 [details] [review] Bug 24718: Get templates working with HourBasedHolds Sponsored-by: Waikato Institute of Technology Created attachment 99543 [details] [review] Bug 24718: Get OPAC working with datetimepicker and HourBasedHolds Sponsored-by: Waikato Institute of Technology Created attachment 99544 [details] [review] Bug 24718: Adding notice to be sent when cancelling holds Sponsored-by: Waikato Institute of Technology Created attachment 99545 [details] [review] Bug 24718: Send notice when cancelling a hold from intranet Sponsored-by: Waikato Institute of Technology Hi Aleisha, just reading the commit subjects I wonder if the notice for cancelling holds could be a separate feature? (In reply to Katrin Fischer from comment #7) > Hi Aleisha, just reading the commit subjects I wonder if the notice for > cancelling holds could be a separate feature? Not really, the sending of notices was part of the feature we wrote for this library and depends on a syspref that this feature introduces. This will be explain in the test plan I'm currently writing Ok thx - I was thinking of something like bug 20453. Hmm I see. I guess could obselete the final two patches here and put them on Bug 24053 so that the notice is always sent, without depending on a syspref If it goes the direction of the other bug, I think not having it necessarily depend on hourly loans would be nice. Maybe we could also do it later. I am adding a see also for now and will let you think it over to what works best here (don't know much about the feature written yet!) :) The library wanted the ability to specify hours when reserving items for patrons, and be able to cancel those reserves - a process of 'booking' something for a few hours, with the ability to reject (cancel) the booking. The patron would then be notified if the reserve was rejected/cancelled by the library (this part is handled in Bug 20453). The cancel_expired_holds.pl cronjob can be run hourly to work with this feature. The library only needed the hour-based holds functionality on the staff side as their patrons can't placed holds via the OPAC (RequestOnOpac disabled) but I have put the functionality on the OPAC side for the option. Test plan for Bug 24718: hour-based holds You will need two borrowers (Borrower A and B) and a record with items (Biblio A) 1) Update database, restart memcached 2) Enable the following sysprefs: RequestOnOpac, AllowHoldDateInFuture, OPACAllowHoldDateInFuture, HourBasedHolds 3) Log into the OPAC as Borrower A 4) Find Biblio A 5) Click 'Place a reserve' 6) Click 'Show more options' 7) Try entering dates in the 'starts on' field and 'not needed after' field. Notice you can also enter a time. Leave the start date empty and choose a specific end date/time. Confirm the hold 8) Confirm the hold is saved with the reserve date set to today/now and the expiry date is saved as specified. Notice the time selection is shown. 9) Log into the staff interface 10) Find Biblio A and place a hold for Borrower B 11) Try entering dates in the 'starts on' field and 'expires on' field. Notice you can also enter a time. Specify a future start date. Confirm the hold 12) Confirm your selected date and time shows in the confirmed hold. View all of the pages that show holds and confirm they all show the date and time correctly. Sponsored-by: Waikato Institute of Technology Tests coming Created attachment 99548 [details] [review] Bug 24718: Ensuring tests pass Confirm these tests pass before and after applying patches t/db_dependent/Holds.t t/db_dependent/Circulation.t t/db_dependent/Circulation/issue.t t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Holds/LocalHoldsPriority.t t/db_dependent/Holds/HoldItemtypeLimit.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/HoldsQueue.t t/db_dependent/Reserves/CancelExpiredReserves.t (In reply to Aleisha Amohia from comment #12) > The library wanted the ability to specify hours when reserving items for > patrons, and be able to cancel those reserves - a process of 'booking' > something for a few hours, with the ability to reject (cancel) the booking. > The patron would then be notified if the reserve was rejected/cancelled by > the library (this part is handled in Bug 20453). The cancel_expired_holds.pl > cronjob can be run hourly to work with this feature. > > The library only needed the hour-based holds functionality on the staff side > as their patrons can't placed holds via the OPAC (RequestOnOpac disabled) > but I have put the functionality on the OPAC side for the option. > > Test plan for Bug 24718: hour-based holds > > You will need two borrowers (Borrower A and B) and a record with items > (Biblio A) > > 1) Update database, restart memcached Remember to update dbix files! > 2) Enable the following sysprefs: RequestOnOpac, AllowHoldDateInFuture, > OPACAllowHoldDateInFuture, HourBasedHolds > 3) Log into the OPAC as Borrower A > 4) Find Biblio A > 5) Click 'Place a reserve' > 6) Click 'Show more options' > 7) Try entering dates in the 'starts on' field and 'not needed after' field. > Notice you can also enter a time. Leave the start date empty and choose a > specific end date/time. Confirm the hold > 8) Confirm the hold is saved with the reserve date set to today/now and the > expiry date is saved as specified. Notice the time selection is shown. > 9) Log into the staff interface > 10) Find Biblio A and place a hold for Borrower B > 11) Try entering dates in the 'starts on' field and 'expires on' field. > Notice you can also enter a time. Specify a future start date. Confirm the > hold > 12) Confirm your selected date and time shows in the confirmed hold. View > all of the pages that show holds and confirm they all show the date and time > correctly. > > Sponsored-by: Waikato Institute of Technology Hi Aleisha. Everything seems to work as expected according to the test plan. There is one failed test though (after the patch is applied): root@40629b80bd10:koha(bz24718)$ prove t/db_dependent/Holds.t t/db_dependent/Holds.t .. 1/61 # Failed test 'holds_placed_today should return a valid borrowernumber' # at t/db_dependent/Holds.t line 104. # got: '247' # expected: '246' t/db_dependent/Holds.t .. 61/61 # Looks like you failed 1 test of 61. t/db_dependent/Holds.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/61 subtests Test Summary Report ------------------- t/db_dependent/Holds.t (Wstat: 256 Tests: 61 Failed: 1) Failed test: 8 Non-zero exit status: 1 Files=1, Tests=61, 7 wallclock secs ( 0.04 usr 0.00 sys + 5.63 cusr 0.52 csys = 6.19 CPU) Result: FAIL In addition, there is also an error happening after the patches are applied when you go the patrons details page and the holds tab: > DataTables warning: table id=holds-table - Ajax error. For more information > about this error, please see http://datatables.net/tn/7 I've reported this error before on bug 24769 - it only seems to happen for me when testing ILS-DI bugs, but is now happening for this bug as well. Created attachment 103521 [details] [review] Bug 24718: (follow-up) Removing old code This solves the JS error Created attachment 103522 [details] [review] Bug 24718: (follow-up) Updated schema file For sandboxes Created attachment 103549 [details] [review] Bug 24718: Introducing HourBasedHolds system preference Sponsored-by: Waikato Institute of Technology Signed-off-by: David Nind <david@davidnind.com> Created attachment 103550 [details] [review] Bug 24718: Get reservedate and expirationdate to use datetime Sponsored-by: Waikato Institute of Technology Signed-off-by: David Nind <david@davidnind.com> Created attachment 103551 [details] [review] Bug 24718: Get templates working with HourBasedHolds Sponsored-by: Waikato Institute of Technology Signed-off-by: David Nind <david@davidnind.com> Created attachment 103552 [details] [review] Bug 24718: Get OPAC working with datetimepicker and HourBasedHolds Sponsored-by: Waikato Institute of Technology Signed-off-by: David Nind <david@davidnind.com> Created attachment 103553 [details] [review] Bug 24718: Ensuring tests pass Confirm these tests pass before and after applying patches t/db_dependent/Holds.t t/db_dependent/Circulation.t t/db_dependent/Circulation/issue.t t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Holds/LocalHoldsPriority.t t/db_dependent/Holds/HoldItemtypeLimit.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/HoldsQueue.t t/db_dependent/Reserves/CancelExpiredReserves.t Signed-off-by: David Nind <david@davidnind.com> Created attachment 103554 [details] [review] Bug 24718: (follow-up) Removing old code This solves the JS error Signed-off-by: David Nind <david@davidnind.com> Created attachment 103555 [details] [review] Bug 24718: (follow-up) Updated schema file For sandboxes Signed-off-by: David Nind <david@davidnind.com> Created attachment 106270 [details] [review] Bug 24718: Get reservedate and expirationdate to use datetime Sponsored-by: Waikato Institute of Technology Signed-off-by: David Nind <david@davidnind.com> Created attachment 106271 [details] [review] Bug 24718: Get templates working with HourBasedHolds Sponsored-by: Waikato Institute of Technology Signed-off-by: David Nind <david@davidnind.com> Created attachment 106272 [details] [review] Bug 24718: Get OPAC working with datetimepicker and HourBasedHolds Sponsored-by: Waikato Institute of Technology Signed-off-by: David Nind <david@davidnind.com> Created attachment 106273 [details] [review] Bug 24718: Ensuring tests pass Confirm these tests pass before and after applying patches t/db_dependent/Holds.t t/db_dependent/Circulation.t t/db_dependent/Circulation/issue.t t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Holds/LocalHoldsPriority.t t/db_dependent/Holds/HoldItemtypeLimit.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/HoldsQueue.t t/db_dependent/Reserves/CancelExpiredReserves.t Signed-off-by: David Nind <david@davidnind.com> Created attachment 106274 [details] [review] Bug 24718: (follow-up) Removing old code This solves the JS error Signed-off-by: David Nind <david@davidnind.com> Created attachment 106275 [details] [review] Bug 24718: (follow-up) Updated schema file For sandboxes Signed-off-by: David Nind <david@davidnind.com> Hi Aleisha, The comment on the syspref says jobs will be run hourly - probably should say something like: NOTE: This system preference requires the misc/cronjobs/holds/ cronjobs. Ask your system administrator to schedule them. When confirming a hold the max pickup delay is added for days correctly, however, the time part is set to 00:00:00 - I thin it should be preserved from original setting or set to 23:59:59 t/db_dependent/api/v1/holds.t fails - should we accept a simple date and use 23:59:59 if time not specified? I am not sure how it is handled for checkouts Created attachment 110084 [details] [review] Bug 24718: Introducing HourBasedHolds system preference Sponsored-by: Waikato Institute of Technology Signed-off-by: David Nind <david@davidnind.com> Created attachment 110085 [details] [review] Bug 24718: Get reservedate and expirationdate to use datetime Sponsored-by: Waikato Institute of Technology Signed-off-by: David Nind <david@davidnind.com> Created attachment 110086 [details] [review] Bug 24718: Get templates working with HourBasedHolds Sponsored-by: Waikato Institute of Technology Signed-off-by: David Nind <david@davidnind.com> Created attachment 110087 [details] [review] Bug 24718: Get OPAC working with datetimepicker and HourBasedHolds Sponsored-by: Waikato Institute of Technology Signed-off-by: David Nind <david@davidnind.com> Created attachment 110088 [details] [review] Bug 24718: Ensuring tests pass Confirm these tests pass before and after applying patches t/db_dependent/Holds.t t/db_dependent/Circulation.t t/db_dependent/Circulation/issue.t t/db_dependent/Holds/HoldFulfillmentPolicy.t t/db_dependent/Holds/LocalHoldsPriority.t t/db_dependent/Holds/HoldItemtypeLimit.t t/db_dependent/Holds/RevertWaitingStatus.t t/db_dependent/HoldsQueue.t t/db_dependent/Reserves/CancelExpiredReserves.t Signed-off-by: David Nind <david@davidnind.com> Created attachment 110089 [details] [review] Bug 24718: (follow-up) Removing old code This solves the JS error Signed-off-by: David Nind <david@davidnind.com> Created attachment 110090 [details] [review] Bug 24718: (follow-up) Updated schema file For sandboxes Signed-off-by: David Nind <david@davidnind.com> Created attachment 110091 [details] [review] Bug 24718: (follow-up) Update syspref description, fix expdate time when setting waiting (In reply to Nick Clemens from comment #32) > Hi Aleisha, > > The comment on the syspref says jobs will be run hourly - probably should > say something like: > NOTE: This system preference requires the misc/cronjobs/holds/ cronjobs. Ask > your system administrator to schedule them. Fixed > > When confirming a hold the max pickup delay is added for days correctly, > however, the time part is set to 00:00:00 - I thin it should be preserved > from original setting or set to 23:59:59 Fixed (note, this is for when setting a hold as waiting). It now preserves minutes and seconds > > t/db_dependent/api/v1/holds.t fails - should we accept a simple date and use > 23:59:59 if time not specified? I am not sure how it is handled for checkouts The same tests in this test file fail for me, before and after the patches, so I can't test this one. Can you please paste the output for what fails and be more specific when you say 'should we accept a simple date'? Thanks! (In reply to Aleisha Amohia from comment #41) > > t/db_dependent/api/v1/holds.t fails - should we accept a simple date and use > > 23:59:59 if time not specified? I am not sure how it is handled for checkouts > > The same tests in this test file fail for me, before and after the patches, > so I can't test this one. Can you please paste the output for what fails and > be more specific when you say 'should we accept a simple date'? > > Thanks! I filed bug 26463 - the tests fail for me if AllowHoldPolicyOverride is set, otherwise they pass before these patches. I will attach a list of failures I see after the patches. By 'simple date' I meant we should not expect/require the time part of the date, but fall back to a default. Though I am not sure where I saw that as needed It may be the problem is in api/v1/swagger/definitions/hold.json where hold_date and expiration_date are defined as "format": "date" and may need to be "date-time" with these patches? Created attachment 110098 [details]
Test failure after patches
(In reply to Nick Clemens from comment #42) > It may be the problem is in api/v1/swagger/definitions/hold.json where > hold_date and expiration_date are defined as "format": "date" and may need > to be "date-time" with these patches? Thank you for the suggestion, I'll try this. Created attachment 110137 [details] [review] Bug 24718: (follow-up) Attempting to fix Koha REST holds Nick, I didn't really know what to do other than remove dateonly flags from all the date options. I don't think it worked. Feel free to amend that last patch on my behalf. Created attachment 110161 [details] [review] Bug 24718: (follow-up) Attempting to fix Koha REST holds (In reply to Aleisha Amohia from comment #46) > Nick, I didn't really know what to do other than remove dateonly flags from > all the date options. I don't think it worked. Feel free to amend that last > patch on my behalf. Patch amended. Just needed to set the format to date-time everywhere - still one failure, but it looks like that comes from C4::Reserves::AddReserve - the time part is set based on the HourBasedHolds pref, so we need to mock that or change the assumptions I tihnk Created attachment 110303 [details] [review] Bug 24718: (follow-up) Attempting to fix Koha REST holds Without having looked at the patchset and how the API is to be used, it feels like we should be adding a new (optional) field for either date-time case, or the time portion. Depending on the use we would make of it. We could also leave the route alone and have a special route for hourly holds /holds/hourly This looks interesting.. could we get a rebase? Created attachment 126559 [details] [review] Bug 24718: Introducing HourBasedHolds system preference Sponsored-by: Waikato Institute of Technology Signed-off-by: David Nind <david@davidnind.com> Created attachment 126560 [details] [review] Bug 24718: Modify reserves.reservedate and expirationdate to use datetime Created attachment 126561 [details] [review] Bug 24718: Updated schema files Created attachment 126562 [details] [review] Bug 24718: Hour based holds on staff client Tests in t/db_dependent/Holds.t Created attachment 126563 [details] [review] Bug 24718: Hour based holds on OPAC Created attachment 126564 [details] [review] Bug 24718: Holds API fixes for datetime I have rebased the patches and fixed the API test that was failing. Note I removed the functionality to place hour-based holds on the OPAC because it should use the new flatpickr plugin which still needs work Bug 29212 . That can be added later. Created attachment 148293 [details] [review] Bug 24718: Introducing HourBasedHolds system preference Sponsored-by: Waikato Institute of Technology Signed-off-by: David Nind <david@davidnind.com> Created attachment 148294 [details] [review] Bug 24718: Modify reserves.reservedate and expirationdate to use datetime Created attachment 148295 [details] [review] Bug 24718: Hour based holds on staff client Tests in t/db_dependent/Holds.t Created attachment 148296 [details] [review] Bug 24718: Hour based holds on OPAC Created attachment 148297 [details] [review] Bug 24718: Holds API fixes for datetime Created attachment 148298 [details] [review] Bug 24718: Updated schema files There is a minor merge conflict in t/db_dependent/Holds.t while trying to apply patch "Bug 24718: Hour based holds on staff client". I was able to resolve this, but Koha now dies on Internal Server Error. Looking from locks there are these lines: Global symbol "$expdate" requires explicit package name (did you forget to declare "my $expdate"?) at ../Koha/C4/Reserves.pm line 216." (and several others) Type of arg 1 to List::MoreUtils::XS::any must be block or sub {} (not reference constructor) at ../Koha/C4/Reserves.pm line 1848, near "$ignore_borrowers ;" Setting this status as "Patch doesn't apply". We now have the base for a booking feature in Koha. It could make sense to build this functionality on top of that now. Created attachment 162474 [details] [review] Bug 24718: Introducing HourBasedHolds system preference Sponsored-by: Waikato Institute of Technology Signed-off-by: David Nind <david@davidnind.com> Created attachment 162475 [details] [review] Bug 24718: Modify reserves.reservedate and expirationdate to use datetime Created attachment 162476 [details] [review] Bug 24718: Hour based holds on staff client Tests in t/db_dependent/Holds.t Created attachment 162477 [details] [review] Bug 24718: Hour based holds on OPAC Created attachment 162478 [details] [review] Bug 24718: Holds API fixes for datetime Created attachment 162479 [details] [review] Bug 24718: Updated schema files (In reply to Katrin Fischer from comment #66) > We now have the base for a booking feature in Koha. It could make sense to > build this functionality on top of that now. I've rebased this for main because one of our library's uses it in production, bu I absolutely agree. Setting to In Discussion until someone has capacity to take that on. |