Bug 12556

Summary: SelfCheck machine starts the hold instantly with an email sent out
Product: Koha Reporter: Jonathan Field <jonathan.field>
Component: SIP2Assignee: Joonas Kylmälä <joonas.kylmala>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: baselibrary.consortium, benjamin.rokseth, colin.campbell, f.demians, jdemuth, jonathan.druart, jonathan.field, joonas.kylmala, katrin.fischer, lucas, martin.renvoize, olli-antti.kivilahti, sally.healey, timothy_alexis.vass
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
This new system preference HoldsNeedProcessingSIP allows the libraries to prepare the items in peace for the next patron before a notification about a waiting hold is sent to the patron. Without this system preference in some cases the item might not have been ready for pick up by the time patron came to the library to checkout the item they had on hold. This feature works only with SIP2 return machines for the time being.
Version(s) released in:
20.11.00
Bug Depends on:    
Bug Blocks: 25260, 27012, 27030, 32529    
Attachments: Bug 12556: Refactor hold transfer status setting to its own method
Bug 12556: Remove unused HTML
Bug 12556: Reuse code from Koha::Hold instead of duplicating it
Bug 12556: reserves/request.pl: Reuse code from Koha::Hold
Bug 12556: Simplify and correct logic in holds-table.inc template
Bug 12556: Add new "in processing" state to holds
Bug 12556: Add new "in processing" state to holds
Bug 12556: Add new "in processing" state to holds
Bug 12556: Refactor hold transfer status setting to its own method
Bug 12556: Remove unused HTML
Bug 12556: Reuse code from Koha::Hold instead of duplicating it
Bug 12556: reserves/request.pl: Reuse code from Koha::Hold
Bug 12556: Simplify and correct logic in holds-table.inc template
Bug 12556: Add new "in processing" state to holds
Bug 12556: Add new "in processing" state to holds
Bug 12556: Refactor hold transfer status setting to its own method
Bug 12556: Refactor hold transfer status setting to its own method
Bug 12556: Remove unused HTML
Bug 12556: Reuse code from Koha::Hold instead of duplicating it
Bug 12556: reserves/request.pl: Reuse code from Koha::Hold
Bug 12556: Simplify and correct logic in holds-table.inc template
Bug 12556: Add new "in processing" state to holds
Bug 12556: Refactor hold transfer status setting to its own method
Bug 12556: Remove unused HTML
Bug 12556: Reuse code from Koha::Hold instead of duplicating it
Bug 12556: reserves/request.pl: Reuse code from Koha::Hold
Bug 12556: Simplify and correct logic in holds-table.inc template
Bug 12556: Add new "in processing" state to holds
Bug 12556: Refactor hold transfer status setting to its own method
Bug 12556: Remove unused HTML
Bug 12556: Refactor hold transfer status setting to its own method
Bug 12556: Remove unused HTML
Bug 12556: Reuse code from Koha::Hold instead of duplicating it
Bug 12556: reserves/request.pl: Reuse code from Koha::Hold
Bug 12556: Simplify and correct logic in holds-table.inc template
Bug 12556: Add new "in processing" state to holds
Bug 12556: Refactor hold transfer status setting to its own method
Bug 12556: Remove unused HTML
Bug 12556: Reuse code from Koha::Hold instead of duplicating it
Bug 12556: reserves/request.pl: Reuse code from Koha::Hold
Bug 12556: Simplify and correct logic in holds-table.inc template
Bug 12556: Add new "in processing" state to holds
Bug 12556: Refactor hold transfer status setting to its own method
Bug 12556: Remove unused HTML
Bug 12556: Reuse code from Koha::Hold instead of duplicating it
Bug 12556: reserves/request.pl: Reuse code from Koha::Hold
Bug 12556: Simplify and correct logic in holds-table.inc template
Bug 12556: Add new "in processing" state to holds
Bug 12556: (QA follow-up) Fix QA Script failures
Bug 12556: (follow-up) Fix failing test
Bug 12556: (follow-up) Fix failing test
Bug 12556: (QA follow-up) Fix QA Script failures
Bug 12556: Add missig POD for ->set_processing

Description Jonathan Field 2014-07-11 09:54:52 UTC
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.
Comment 1 Olli-Antti Kivilahti 2015-04-24 11:02:25 UTC
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.
Comment 2 Benjamin Rokseth 2017-01-10 19:08:25 UTC
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!
Comment 3 Katrin Fischer 2017-01-11 07:16:55 UTC
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.
Comment 4 Olli-Antti Kivilahti 2017-01-12 15:18:26 UTC
(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.
Comment 5 Joonas Kylmälä 2020-07-22 15:50:12 UTC
Created attachment 107181 [details] [review]
Bug 12556: Refactor hold transfer status setting to its own method
Comment 6 Joonas Kylmälä 2020-07-22 15:50:15 UTC
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.
Comment 7 Joonas Kylmälä 2020-07-22 15:50:19 UTC
Created attachment 107183 [details] [review]
Bug 12556: Reuse code from Koha::Hold instead of duplicating it
Comment 8 Joonas Kylmälä 2020-07-22 15:50:23 UTC
Created attachment 107184 [details] [review]
Bug 12556: reserves/request.pl: Reuse code from Koha::Hold
Comment 9 Joonas Kylmälä 2020-07-22 15:50:26 UTC
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).
Comment 10 Joonas Kylmälä 2020-07-22 15:50:29 UTC
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
Comment 11 Joonas Kylmälä 2020-08-20 07:08:58 UTC
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
Comment 12 Joonas Kylmälä 2020-08-26 09:52:00 UTC
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
Comment 13 Joonas Kylmälä 2020-08-26 12:21:27 UTC
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.
Comment 14 Joonas Kylmälä 2020-09-11 10:57:42 UTC
Created attachment 109924 [details] [review]
Bug 12556: Refactor hold transfer status setting to its own method
Comment 15 Joonas Kylmälä 2020-09-11 10:57:46 UTC
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.
Comment 16 Joonas Kylmälä 2020-09-11 10:57:51 UTC
Created attachment 109926 [details] [review]
Bug 12556: Reuse code from Koha::Hold instead of duplicating it
Comment 17 Joonas Kylmälä 2020-09-11 10:57:59 UTC
Created attachment 109927 [details] [review]
Bug 12556: reserves/request.pl: Reuse code from Koha::Hold
Comment 18 Joonas Kylmälä 2020-09-11 10:58:05 UTC
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).
Comment 19 Joonas Kylmälä 2020-09-11 10:58:11 UTC
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
Comment 20 Joonas Kylmälä 2020-09-11 10:59:20 UTC
Rebased and fixed the issue with another patron being able to checkout in processing hold via SIP machine. Ready for sign-off again.
Comment 21 Joonas Kylmälä 2020-09-25 07:51:47 UTC
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
Comment 22 Joonas Kylmälä 2020-09-25 07:52:29 UTC
Rebased.
Comment 23 Timothy Alexis Vass 2020-09-25 11:21:19 UTC
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
Comment 24 Joonas Kylmälä 2020-09-25 11:32:02 UTC
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?
Comment 25 Timothy Alexis Vass 2020-09-30 07:43:09 UTC
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>
Comment 26 Timothy Alexis Vass 2020-09-30 07:59:05 UTC
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>
Comment 27 Timothy Alexis Vass 2020-09-30 07:59:12 UTC
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>
Comment 28 Timothy Alexis Vass 2020-09-30 07:59:18 UTC
Created attachment 110945 [details] [review]
Bug 12556: Reuse code from Koha::Hold instead of duplicating it

Signed-off-by: John Doe <you@example.com>
Comment 29 Timothy Alexis Vass 2020-09-30 07:59:27 UTC
Created attachment 110946 [details] [review]
Bug 12556: reserves/request.pl: Reuse code from Koha::Hold

Signed-off-by: John Doe <you@example.com>
Comment 30 Timothy Alexis Vass 2020-09-30 07:59:33 UTC
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>
Comment 31 Timothy Alexis Vass 2020-09-30 07:59:40 UTC
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>
Comment 32 Timothy Alexis Vass 2020-09-30 08:08:32 UTC
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>
Comment 33 Timothy Alexis Vass 2020-09-30 08:08:37 UTC
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>
Comment 34 Timothy Alexis Vass 2020-09-30 08:08:44 UTC
Created attachment 110951 [details] [review]
Bug 12556: Reuse code from Koha::Hold instead of duplicating it

Signed-off-by: John Doe <you@example.com>
Comment 35 Timothy Alexis Vass 2020-09-30 08:08:50 UTC
Created attachment 110952 [details] [review]
Bug 12556: reserves/request.pl: Reuse code from Koha::Hold

Signed-off-by: John Doe <you@example.com>
Comment 36 Timothy Alexis Vass 2020-09-30 08:08:56 UTC
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>
Comment 37 Timothy Alexis Vass 2020-09-30 08:09:02 UTC
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>
Comment 38 Timothy Alexis Vass 2020-09-30 08:11:07 UTC
Created attachment 110955 [details] [review]
Bug 12556: Refactor hold transfer status setting to its own method

Signed-off-by: John Doe <you@example.com>
Comment 39 Timothy Alexis Vass 2020-09-30 08:11:13 UTC
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>
Comment 40 Timothy Alexis Vass 2020-09-30 08:31:33 UTC
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>
Comment 41 Timothy Alexis Vass 2020-09-30 08:31:40 UTC
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>
Comment 42 Timothy Alexis Vass 2020-09-30 08:31:45 UTC
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>
Comment 43 Timothy Alexis Vass 2020-09-30 08:31:52 UTC
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>
Comment 44 Timothy Alexis Vass 2020-09-30 08:31:58 UTC
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>
Comment 45 Timothy Alexis Vass 2020-09-30 08:32:06 UTC
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>
Comment 46 Martin Renvoize 2020-10-09 13:29:02 UTC
Sorry Joonas, this one now has some conflicts.. code looks good.. if you rebase it I'll QA it asap.
Comment 47 Joonas Kylmälä 2020-10-09 14:30:30 UTC
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>
Comment 48 Joonas Kylmälä 2020-10-09 14:30:35 UTC
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>
Comment 49 Joonas Kylmälä 2020-10-09 14:30:40 UTC
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>
Comment 50 Joonas Kylmälä 2020-10-09 14:30:45 UTC
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>
Comment 51 Joonas Kylmälä 2020-10-09 14:30:51 UTC
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>
Comment 52 Joonas Kylmälä 2020-10-09 14:30:58 UTC
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>
Comment 53 Martin Renvoize 2020-10-12 10:38:54 UTC
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>
Comment 54 Martin Renvoize 2020-10-12 10:39:00 UTC
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>
Comment 55 Martin Renvoize 2020-10-12 10:39:09 UTC
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>
Comment 56 Martin Renvoize 2020-10-12 10:39:15 UTC
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>
Comment 57 Martin Renvoize 2020-10-12 10:39:21 UTC
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>
Comment 58 Martin Renvoize 2020-10-12 10:39:30 UTC
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>
Comment 59 Martin Renvoize 2020-10-12 10:39:37 UTC
Created attachment 111500 [details] [review]
Bug 12556: (QA follow-up) Fix QA Script failures

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 60 Martin Renvoize 2020-10-12 10:41:48 UTC
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.
Comment 61 Joonas Kylmälä 2020-10-12 10:58:03 UTC
Created attachment 111509 [details] [review]
Bug 12556: (follow-up) Fix failing test

The method call set_waiting(1) was renamed to be set_transfer()
Comment 62 Joonas Kylmälä 2020-10-12 11:06:19 UTC
(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)?
Comment 63 Martin Renvoize 2020-10-12 13:02:36 UTC
(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 :)
Comment 64 Martin Renvoize 2020-10-12 13:04:59 UTC
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>
Comment 65 Martin Renvoize 2020-10-12 13:05:46 UTC
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
Comment 66 Jonathan Druart 2020-10-14 09:45:41 UTC
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
Comment 67 Jonathan Druart 2020-10-14 12:33:36 UTC
Created attachment 111610 [details] [review]
Bug 12556: Add missig POD for ->set_processing
Comment 68 Jonathan Druart 2020-10-14 12:53:25 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 69 Lucas Gass 2020-11-13 14:51:54 UTC
enhancement will not be backported to 20.05.x
Comment 70 Frédéric Demians 2023-08-11 06:55:41 UTC
It would be nice to have this functionnality with more granularity, not with a global system preference, but with a SIPconfig.xml parameter.