Currently when a reserved book is returned on a SelfCheck machine the hold starts the hold instantly with an email sent out. It would be preferable if the hold started once it had been taken out of the SelfCheck bins and checked in by reservations staff at the counter. Current suggestion: Possible time delay on the message - may help if after 8 at night and 9 the following morning if delayed until following morning still a workaround if looked at account on OPAC they would see a hold e.g. so theoretically it could still be in the bin - it is difficult to find a correct time delay because of the number of variables - a bit more of a task to put in functionality change Possible Ideal Process: Requested item is returned via SelfService machine - for the patron the process remains the same (they are directed to put it in the RED slot) Instead of an email being sent immediately at that point to the patron next in line for the request, the item would have a new circulation status e.g. ‘In SelfService’ this removes the item from the patron’s record but does not trigger the next hold until it is checked-in by a member of the library, at which point the item moves onto the next circulation status that it would have had before going into ‘In SelfService’ (e.g. ‘On hold’ or ‘in transit’ etc.) There could be different variations of this process based on what is easiest to incorporate into Koha e.g. the item could possibly go into a transit status to a new Shelving Location or Branch - again ‘SelfService etc.’ - before returning to the correct next circulation status as described before - once it is checked-in.
Hi! We experimented by disabling the hold catching when checking in Items with the SIP Server. This was because our borrowers would get the Reservation notice and come looking for the Item while the Item was still being processed. Then we started to get issues with Items (that had reservations) being in Available-state between getting checked in with the SIP server and waiting for the librarian to check them in using Koha. We reverted back to the way things are, because we get less issues this way. Our librarians know where to look for the reserved Item if on the rare occasion a borrower is fast enough to intercept the hold before we place it to the self pickup shelf. However if the limbo-available-state between check-in can be fixed, (this might be more trouble than it's worth) I guess this would make sense.
I notice this bug is rather old, but we actually suffered badly from this, as we have several fully automated branches that are open 24/7 without staff. Self service machines with reservation boxes often resulted in things checked in and patrons then turning up to collect their long-awaited material which is locked up in reservation boxes until next day. Our solution, by any mean not ideal, was to have separated branches for automated selfcheck machines, a SIP translation proxy that fixes the items that are NOT reserved so they can be put directly on the shelf, and finally a cronjob to clean up the branchtransfer mess. We (Oslo Public Library) would gladly cooperate on this if any good ideas turn up!
I think the nicest thing to do might be a separate status or a flag? We always get asked how to identify books on hold that have been returned, so they can be pulled off the cart. Or even that an email is sent to the circulation desk so they can go and pick it up from the return station. Currently you can't really make that happen and it would be nice if we could.
(In reply to Benjamin Rokseth from comment #2) > I notice this bug is rather old, but we actually suffered badly from this, > as we have several fully automated branches that are open 24/7 without staff. > They get better with age, like good wine and other stuff. Sorry to hear about your misery. We can completely relate to that :) > Our solution, by any mean not ideal, was to have separated branches for > automated selfcheck machines, a SIP translation proxy that fixes the items > that are NOT reserved so they can be put directly on the shelf, and finally > a cronjob to clean up the branchtransfer mess. > Our self-service libraries are separate branches and strangely I haven't heard of this problem manifesting yet for us, but I know sooner or later someone will notice. > We (Oslo Public Library) would gladly cooperate on this if any good ideas > turn up! You could attach the opening hours of the pickup library in the hold notification? Maybe this would help? Tinker a bit with the Letters-module. The C4::Letters::GetPreparedLetter() is very very hairy tho. Also you would need to add the library opening hours somewhere. Easiest thing to do is to make a YAML-config syspref with those opening hours per branch. Super fast GUI-replacement and will get this problem solved in no time. No need for DB or GUI code. >Instead of an email being sent immediately at that point to the patron next in >line for the request, the item would have a new circulation status e.g. ‘In >SelfService’ this removes the item from the patron’s record but does not >trigger the next hold until it is checked-in by a member of the library, at >which point the item moves onto the next circulation status that it would have >had before going into ‘In SelfService’ (e.g. ‘On hold’ or ‘in transit’ etc.) >However if the limbo-available-state between check-in can be fixed, (this >might be more trouble than it's worth) I guess this would make sense. Maybe this idea is a good solution? Put all SIP2-server checkouts to a Limbo-state Skip catching holds and transfers when put to a 'Limbo'-status? But relinguish ties to the previous borrower. Show in OPAC/Intra the status as Limbo, this is more brittle to program. Shouldn't be too difficult to test and program in.
Created attachment 107181 [details] [review] Bug 12556: Refactor hold transfer status setting to its own method
Created attachment 107182 [details] [review] Bug 12556: Remove unused HTML Koha::Hold::is_at_destination() already checks for found = 'W' so the ELSE code path which would print "Item waiting to be pulled from [...]" never gets executed.
Created attachment 107183 [details] [review] Bug 12556: Reuse code from Koha::Hold instead of duplicating it
Created attachment 107184 [details] [review] Bug 12556: reserves/request.pl: Reuse code from Koha::Hold
Created attachment 107185 [details] [review] Bug 12556: Simplify and correct logic in holds-table.inc template The states is_waiting and is_found are already implied by is_at_destination. Also we would never get the 'transfered' html class since transfer's status is T (this might not have been true in the past but now is).
Created attachment 107186 [details] [review] Bug 12556: Add new "in processing" state to holds This adds new syspref, HoldsNeedProcessingSIP, which controls whether a hold that is related to item will be filled automatically or not. If the user has enabled the syspref then instead of fulfilling the hold automatically the hold will go to "in processing" state. To test: 1. Checkout a book to patron A 2. Place a bib level hold to the book for B 3. Patron A returns the book via SIP, to simulate this use: ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --item <ItemBarcode> -m checkin 4. Notice that no notification is generated for Patron B about hold and that the hold status in intranet and opac is "In Processing". 5. Notice that patron A (or other patrons) cannot checkout a book that is in processing, because it is considered to be attached to the holdee (similarly to the waiting state): ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --patron <PatronABarcode> --item <ItemBarcode> -m checkout
Created attachment 108711 [details] [review] Bug 12556: Add new "in processing" state to holds This adds new syspref, HoldsNeedProcessingSIP, which controls whether a hold that is related to item will be filled automatically or not. If the user has enabled the syspref then instead of fulfilling the hold automatically the hold will go to "in processing" state. To test: 1. Checkout a book to patron A 2. Place a bib level hold to the book for B 3. Patron A returns the book via SIP, to simulate this use: ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --item <ItemBarcode> -m checkin 4. Notice that no notification is generated for Patron B about hold and that the hold status in intranet and opac is "In Processing". 5. Notice that patron A (or other patrons) cannot checkout a book that is in processing, because it is considered to be attached to the holdee (similarly to the waiting state): ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --patron <PatronABarcode> --item <ItemBarcode> -m checkout
Created attachment 109140 [details] [review] Bug 12556: Add new "in processing" state to holds This adds new syspref, HoldsNeedProcessingSIP, which controls whether a hold that is related to item will be filled automatically or not. If the user has enabled the syspref then instead of fulfilling the hold automatically the hold will go to "in processing" state. To test: 1. Checkout a book to patron A 2. Place a bib level hold to the book for B 3. Patron A returns the book via SIP, to simulate this use: ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --item <ItemBarcode> -m checkin 4. Notice that no notification is generated for Patron B about hold and that the hold status in intranet and opac is "In Processing". 5. Notice that patron A (or other patrons) cannot checkout a book that is in processing, because it is considered to be attached to the holdee (similarly to the waiting state): ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --patron <PatronABarcode> --item <ItemBarcode> -m checkout
One of our customers was testing the proposed patches and found out that if the setting AllowItemsOnHoldCheckoutSIP is enabled then In processing state holds can be checked out. They shouldn't be able to be since the item for the patron's hold is already attached for the patron in the same way it is with Waiting state.
Created attachment 109924 [details] [review] Bug 12556: Refactor hold transfer status setting to its own method
Created attachment 109925 [details] [review] Bug 12556: Remove unused HTML Koha::Hold::is_at_destination() already checks for found = 'W' so the ELSE code path which would print "Item waiting to be pulled from [...]" never gets executed.
Created attachment 109926 [details] [review] Bug 12556: Reuse code from Koha::Hold instead of duplicating it
Created attachment 109927 [details] [review] Bug 12556: reserves/request.pl: Reuse code from Koha::Hold
Created attachment 109928 [details] [review] Bug 12556: Simplify and correct logic in holds-table.inc template The states is_waiting and is_found are already implied by is_at_destination. Also we would never get the 'transfered' html class since transfer's status is T (this might not have been true in the past but now is).
Created attachment 109929 [details] [review] Bug 12556: Add new "in processing" state to holds This adds new syspref, HoldsNeedProcessingSIP, which controls whether a hold that is related to item will be filled automatically or not. If the user has enabled the syspref then instead of fulfilling the hold automatically the hold will go to "in processing" state. To test: 1. Checkout a book to patron A 2. Place a bib level hold to the book for B 3. Patron A returns the book via SIP, to simulate this use: ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --item <ItemBarcode> -m checkin 4. Notice that no notification is generated for Patron B about hold and that the hold status in intranet and opac is "In Processing". 5. Notice that patron A (or other patrons) cannot checkout a book that is in processing, because it is considered to be attached to the holdee (similarly to the waiting state): ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --patron <PatronABarcode> --item <ItemBarcode> -m checkout
Rebased and fixed the issue with another patron being able to checkout in processing hold via SIP machine. Ready for sign-off again.
Created attachment 110719 [details] [review] Bug 12556: Add new "in processing" state to holds This adds new syspref, HoldsNeedProcessingSIP, which controls whether a hold that is related to item will be filled automatically or not. If the user has enabled the syspref then instead of fulfilling the hold automatically the hold will go to "in processing" state. To test: 1. Checkout a book to patron A 2. Place a bib level hold to the book for B 3. Patron A returns the book via SIP, to simulate this use: ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --item <ItemBarcode> -m checkin 4. Notice that no notification is generated for Patron B about hold and that the hold status in intranet and opac is "In Processing". 5. Notice that patron A (or other patrons) cannot checkout a book that is in processing, because it is considered to be attached to the holdee (similarly to the waiting state): ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --patron <PatronABarcode> --item <ItemBarcode> -m checkout
Rebased.
Steps 1, 2 and 3 worked fine: ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --item 3999900000001 -m checkin Attempting socket connection to 127.0.0.1:6001...connected! SEND: 9300CNkoha|COkoha|CPCPL| READ: 941 Trying 'checkin' SEND: 09N20200925 11172320200925 111723APCPL|AOCPL|AB3999900000001|ACkoha|BIN| READ: 101YNY20200925 111725AOCPL|AB3999900000001|AQCPL|AJE Street shuffle :|CY23529000120056|CTCPL|DAJohnny Bryan ((212) 555-1212)|CV01| Then, when reloading the page 'cgi-bin/koha/reserve/request.pl?biblionumbers=1/' There is an error beginning with: The method Koha::Hold->is_in_processing is not covered by tests! Trace begun at /kohadevbox/koha/Koha/Object.pm line 780 Koha::Object::AUTOLOAD('Koha::Hold=HASH(0x55d06d6dbac0)') called at /kohadevbox/koha/reserve/request.pl line 668
Timothy, strange, I'm not able to reproduce the issue by going to that page. Can you do koha-plack --restart <instance> and see if the issue goes away?
Created attachment 110942 [details] [review] Bug 12556: Refactor hold transfer status setting to its own method Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Created attachment 110943 [details] [review] Bug 12556: Refactor hold transfer status setting to its own method Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Created attachment 110944 [details] [review] Bug 12556: Remove unused HTML Koha::Hold::is_at_destination() already checks for found = 'W' so the ELSE code path which would print "Item waiting to be pulled from [...]" never gets executed. Signed-off-by: John Doe <you@example.com>
Created attachment 110945 [details] [review] Bug 12556: Reuse code from Koha::Hold instead of duplicating it Signed-off-by: John Doe <you@example.com>
Created attachment 110946 [details] [review] Bug 12556: reserves/request.pl: Reuse code from Koha::Hold Signed-off-by: John Doe <you@example.com>
Created attachment 110947 [details] [review] Bug 12556: Simplify and correct logic in holds-table.inc template The states is_waiting and is_found are already implied by is_at_destination. Also we would never get the 'transfered' html class since transfer's status is T (this might not have been true in the past but now is). Signed-off-by: John Doe <you@example.com>
Created attachment 110948 [details] [review] Bug 12556: Add new "in processing" state to holds This adds new syspref, HoldsNeedProcessingSIP, which controls whether a hold that is related to item will be filled automatically or not. If the user has enabled the syspref then instead of fulfilling the hold automatically the hold will go to "in processing" state. To test: 1. Checkout a book to patron A 2. Place a bib level hold to the book for B 3. Patron A returns the book via SIP, to simulate this use: ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --item <ItemBarcode> -m checkin 4. Notice that no notification is generated for Patron B about hold and that the hold status in intranet and opac is "In Processing". 5. Notice that patron A (or other patrons) cannot checkout a book that is in processing, because it is considered to be attached to the holdee (similarly to the waiting state): ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --patron <PatronABarcode> --item <ItemBarcode> -m checkout Signed-off-by: John Doe <you@example.com>
Created attachment 110949 [details] [review] Bug 12556: Refactor hold transfer status setting to its own method Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Created attachment 110950 [details] [review] Bug 12556: Remove unused HTML Koha::Hold::is_at_destination() already checks for found = 'W' so the ELSE code path which would print "Item waiting to be pulled from [...]" never gets executed. Signed-off-by: John Doe <you@example.com>
Created attachment 110951 [details] [review] Bug 12556: Reuse code from Koha::Hold instead of duplicating it Signed-off-by: John Doe <you@example.com>
Created attachment 110952 [details] [review] Bug 12556: reserves/request.pl: Reuse code from Koha::Hold Signed-off-by: John Doe <you@example.com>
Created attachment 110953 [details] [review] Bug 12556: Simplify and correct logic in holds-table.inc template The states is_waiting and is_found are already implied by is_at_destination. Also we would never get the 'transfered' html class since transfer's status is T (this might not have been true in the past but now is). Signed-off-by: John Doe <you@example.com>
Created attachment 110954 [details] [review] Bug 12556: Add new "in processing" state to holds This adds new syspref, HoldsNeedProcessingSIP, which controls whether a hold that is related to item will be filled automatically or not. If the user has enabled the syspref then instead of fulfilling the hold automatically the hold will go to "in processing" state. To test: 1. Checkout a book to patron A 2. Place a bib level hold to the book for B 3. Patron A returns the book via SIP, to simulate this use: ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --item <ItemBarcode> -m checkin 4. Notice that no notification is generated for Patron B about hold and that the hold status in intranet and opac is "In Processing". 5. Notice that patron A (or other patrons) cannot checkout a book that is in processing, because it is considered to be attached to the holdee (similarly to the waiting state): ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --patron <PatronABarcode> --item <ItemBarcode> -m checkout Signed-off-by: John Doe <you@example.com>
Created attachment 110955 [details] [review] Bug 12556: Refactor hold transfer status setting to its own method Signed-off-by: John Doe <you@example.com>
Created attachment 110956 [details] [review] Bug 12556: Remove unused HTML Koha::Hold::is_at_destination() already checks for found = 'W' so the ELSE code path which would print "Item waiting to be pulled from [...]" never gets executed. Signed-off-by: John Doe <you@example.com>
Created attachment 110961 [details] [review] Bug 12556: Refactor hold transfer status setting to its own method Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Created attachment 110962 [details] [review] Bug 12556: Remove unused HTML Koha::Hold::is_at_destination() already checks for found = 'W' so the ELSE code path which would print "Item waiting to be pulled from [...]" never gets executed. Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Created attachment 110963 [details] [review] Bug 12556: Reuse code from Koha::Hold instead of duplicating it Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Created attachment 110964 [details] [review] Bug 12556: reserves/request.pl: Reuse code from Koha::Hold Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Created attachment 110965 [details] [review] Bug 12556: Simplify and correct logic in holds-table.inc template The states is_waiting and is_found are already implied by is_at_destination. Also we would never get the 'transfered' html class since transfer's status is T (this might not have been true in the past but now is). Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Created attachment 110966 [details] [review] Bug 12556: Add new "in processing" state to holds This adds new syspref, HoldsNeedProcessingSIP, which controls whether a hold that is related to item will be filled automatically or not. If the user has enabled the syspref then instead of fulfilling the hold automatically the hold will go to "in processing" state. To test: 1. Checkout a book to patron A 2. Place a bib level hold to the book for B 3. Patron A returns the book via SIP, to simulate this use: ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --item <ItemBarcode> -m checkin 4. Notice that no notification is generated for Patron B about hold and that the hold status in intranet and opac is "In Processing". 5. Notice that patron A (or other patrons) cannot checkout a book that is in processing, because it is considered to be attached to the holdee (similarly to the waiting state): ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --patron <PatronABarcode> --item <ItemBarcode> -m checkout Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Sorry Joonas, this one now has some conflicts.. code looks good.. if you rebase it I'll QA it asap.
Created attachment 111405 [details] [review] Bug 12556: Refactor hold transfer status setting to its own method Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Created attachment 111406 [details] [review] Bug 12556: Remove unused HTML Koha::Hold::is_at_destination() already checks for found = 'W' so the ELSE code path which would print "Item waiting to be pulled from [...]" never gets executed. Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Created attachment 111407 [details] [review] Bug 12556: Reuse code from Koha::Hold instead of duplicating it Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Created attachment 111408 [details] [review] Bug 12556: reserves/request.pl: Reuse code from Koha::Hold Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Created attachment 111409 [details] [review] Bug 12556: Simplify and correct logic in holds-table.inc template The states is_waiting and is_found are already implied by is_at_destination. Also we would never get the 'transfered' html class since transfer's status is T (this might not have been true in the past but now is). Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se>
Created attachment 111410 [details] [review] Bug 12556: Add new "in processing" state to holds This adds new syspref, HoldsNeedProcessingSIP, which controls whether a hold that is related to item will be filled automatically or not. If the user has enabled the syspref then instead of fulfilling the hold automatically the hold will go to "in processing" state. To test: 1. Checkout a book to patron A 2. Place a bib level hold to the book for B 3. Patron A returns the book via SIP, to simulate this use: ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --item <ItemBarcode> -m checkin 4. Notice that no notification is generated for Patron B about hold and that the hold status in intranet and opac is "In Processing". 5. Notice that patron A (or other patrons) cannot checkout a book that is in processing, because it is considered to be attached to the holdee (similarly to the waiting state): ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --patron <PatronABarcode> --item <ItemBarcode> -m checkout Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Created attachment 111494 [details] [review] Bug 12556: Refactor hold transfer status setting to its own method Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 111495 [details] [review] Bug 12556: Remove unused HTML Koha::Hold::is_at_destination() already checks for found = 'W' so the ELSE code path which would print "Item waiting to be pulled from [...]" never gets executed. Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 111496 [details] [review] Bug 12556: Reuse code from Koha::Hold instead of duplicating it Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 111497 [details] [review] Bug 12556: reserves/request.pl: Reuse code from Koha::Hold Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 111498 [details] [review] Bug 12556: Simplify and correct logic in holds-table.inc template The states is_waiting and is_found are already implied by is_at_destination. Also we would never get the 'transfered' html class since transfer's status is T (this might not have been true in the past but now is). Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 111499 [details] [review] Bug 12556: Add new "in processing" state to holds This adds new syspref, HoldsNeedProcessingSIP, which controls whether a hold that is related to item will be filled automatically or not. If the user has enabled the syspref then instead of fulfilling the hold automatically the hold will go to "in processing" state. To test: 1. Checkout a book to patron A 2. Place a bib level hold to the book for B 3. Patron A returns the book via SIP, to simulate this use: ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --item <ItemBarcode> -m checkin 4. Notice that no notification is generated for Patron B about hold and that the hold status in intranet and opac is "In Processing". 5. Notice that patron A (or other patrons) cannot checkout a book that is in processing, because it is considered to be attached to the holdee (similarly to the waiting state): ./misc/sip_cli_emulator.pl -su koha -sp koha -l CPL -a 127.0.0.1 -p 6001 --patron <PatronABarcode> --item <ItemBarcode> -m checkout Signed-off-by: Timothy Alexis Vass <timothy_alexis.vass@ub.lu.se> Rebased-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 111500 [details] [review] Bug 12556: (QA follow-up) Fix QA Script failures Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Looks good :) However, sorry Joonas, but t/db_dependent/api/v1/holds.t is failing.. and ideally I'd love to see some basic tests for the new set_* methods introduced in Koha::Hold. They're trivial methods, but having a unit test that proves they correctly set the three fields for each of the methods will help prevent future regressions. Failing QA, but hopefully not for long.
Created attachment 111509 [details] [review] Bug 12556: (follow-up) Fix failing test The method call set_waiting(1) was renamed to be set_transfer()
(In reply to Martin Renvoize from comment #60) > However, sorry Joonas, but t/db_dependent/api/v1/holds.t is failing.. and > ideally I'd love to see some basic tests for the new set_* methods > introduced in Koha::Hold. They're trivial methods, but having a unit test > that proves they correctly set the three fields for each of the methods will > help prevent future regressions. thanks, fixed the api test. I added the same test to set_processing and set_transfer methods by calling $hold->suspend_hold as there was originally with the set_waiting method. That should check what the function already does (just set the found status). Do you think it is still required to explicitly check the found status for these (even though it indirectly is checked by suspend_hold call)?
(In reply to Joonas Kylmälä from comment #62) > (In reply to Martin Renvoize from comment #60) > > However, sorry Joonas, but t/db_dependent/api/v1/holds.t is failing.. and > > ideally I'd love to see some basic tests for the new set_* methods > > introduced in Koha::Hold. They're trivial methods, but having a unit test > > that proves they correctly set the three fields for each of the methods will > > help prevent future regressions. > > thanks, fixed the api test. I added the same test to set_processing and > set_transfer methods by calling $hold->suspend_hold as there was originally > with the set_waiting method. That should check what the function already > does (just set the found status). Do you think it is still required to > explicitly check the found status for these (even though it indirectly is > checked by suspend_hold call)? Fair point.. they are tested indirectly so they are covered. Lets go with it as is and we can continue to follow through further cleanups at a later date :)
Created attachment 111516 [details] [review] Bug 12556: (follow-up) Fix failing test The method call set_waiting(1) was renamed to be set_transfer() Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
No regressions found, QA scripts are happy. Glad to see a bit of tidying up going on in here prior to working in the new functionality. Passing QA
Created attachment 111585 [details] [review] Bug 12556: (QA follow-up) Fix QA Script failures Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> JD amended patch, fix: FAIL C4/RotatingCollections.pm FAIL pod *** WARNING: line containing nothing but whitespace in paragraph in file C4/RotatingCollections.pm
Created attachment 111610 [details] [review] Bug 12556: Add missig POD for ->set_processing
Pushed to master for 20.11, thanks to everybody involved!
enhancement will not be backported to 20.05.x
It would be nice to have this functionnality with more granularity, not with a global system preference, but with a SIPconfig.xml parameter.