Bug 19287

Summary: Add ability to mark an item 'Lost' from 'Holds to pull' list
Product: Koha Reporter: Séverine Queune <severine.queune>
Component: Hold requestsAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: new feature    
Priority: P5 - low CC: black23, gmcharlt, jonathan.druart, katrin.fischer, martin.renvoize, niamh.walker-headon, nicolas.legrand, roch.damour, sally.healey, sandboxes
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=12363
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19974
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20844
Change sponsored?: Sponsored Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 19974    
Bug Blocks: 26203, 21869, 22476, 26183    
Attachments: Bug 19287: Just a bit of cleanup
Bug 19287: Add new pref CanMarkHoldsAwaitingPickupAsLost
Bug 19287: Add ability to mark an item ‘Lost’ from ‘Holds awaiting pickup’ list
Bug 19287: Refactor the template to avoid c/p code
Bug 19287: Add some feedback messages to the librarian
Bug 19287: Just a bit of cleanup
Bug 19287: Add new pref CanMarkHoldsAwaitingPickupAsLost
Bug 19287: Add ability to mark an item "Lost" from "Holds awaiting pickup" list
Bug 19287: Refactor the template to avoid c/p code
Bug 19287: Add some feedback messages to the librarian
Bug 19287: Do not use 'yes' and 'no' for choices
Bug 19287: Add ability to mark an item ‘Lost’ from ‘Holds to pull’ list
Bug 19287: Add ability to mark an item 'Lost' from 'Holds to pull' list
Bug 19287: Fix conflict with bug 19304
Bug 19287: Add ability to mark an item 'Lost' from 'Holds to pull' list
Bug 19287: Fix conflict with bug 19304
Bug 19287: Add ability to mark an item 'Lost' from 'Holds to pull' list
Bug 19287: Fix conflict with bug 19304
Bug 19287: Fix conflict with bug 19974
Bug 19287: Use the notice defined for the homebranch of the patron
Bug 19287: Add CANCEL_HOLD_ON_LOST to installer files for all languages
Bug 19287: Add column action to columns settings
Bug 19287: Update UpdateItemWhenLostFromHoldList description
Bug 19287: Do not display buttons for biblio level holds
Bug 19287: Shorten button text
Bug 19287: Add ability to mark an item 'Lost' from 'Holds to pull' list
Bug 19287: Fix conflict with bug 19304
Bug 19287: Fix conflict with bug 19974
Bug 19287: Use the notice defined for the homebranch of the patron
Bug 19287: Add CANCEL_HOLD_ON_LOST to installer files for all languages
Bug 19287: Add column action to columns settings
Bug 19287: Update UpdateItemWhenLostFromHoldList description
Bug 19287: Do not display buttons for biblio level holds
Bug 19287: Shorten button text
Bug 19287: (QA follow-up) Fix preference description
Bug 19287: Add CANCEL_HOLD_ON_LOST to installer files for all languages
Bug 19287: (follow-up) Use the notice defined for the library of the patron

Description Séverine Queune 2017-09-11 15:04:59 UTC
The BULAC (http://www.bulac.fr/) would like to mark items not found on shelves after patrons’ holds as ‘Lost’ directly from the “Circulation > Holds awaiting pickup” list.

We could have a new button on the “Action” columns of the 2 tabs,  producing 3 automatic actions :
  - cancel the hold
  - notify the patron if the hold was supposed to be available (the notice should be set on the “Tools > Notices and Slips” menu)
  - set the field “items.notforloan” or “items.itemlost” (define in a system preference) from “0” to a value from the same ‘authorized values list’ and set in another system preference

As long as “Change sponsored?” is set on “Seeking developer”, anyone wanting to quote us for the development of such a feature or just ask for more information is welcome to write to us: <sigb@bulac.fr>.
Comment 1 Jonathan Druart 2017-10-17 20:51:49 UTC
Created attachment 68226 [details] [review]
Bug 19287: Just a bit of cleanup

Let's add a "op" variable to know what the script is supposed to do and
make it less obscure.
Comment 2 Jonathan Druart 2017-10-17 20:51:54 UTC
Created attachment 68227 [details] [review]
Bug 19287: Add new pref CanMarkHoldsAwaitingPickupAsLost

This patch adds a new pref to control the behaviour of this patch set.
Comment 3 Jonathan Druart 2017-10-17 20:51:58 UTC
Created attachment 68228 [details] [review]
Bug 19287: Add ability to mark an item ‘Lost’ from ‘Holds awaiting pickup’ list

This is the main patch of this new enhancement.
The goal to this enhancement is to add the ability to mark items as lost
from the ‘Holds awaiting pickup’ page.
A new pref is added to control the behaviour, default is off
To enable it, you need to set the new pref CanMarkHoldsAwaitingPickupAsLost to one of these two values:
- "Allow to mark items as lost"
=> The item will be marked as lost and the hold will be cancelled
- "Allow to mark items as lost and notify the patron"
=> Same as previously but the patron will be notified as well.

The notification is done using a new notice template (code=CANCEL_HOLD_ON_LOST).
Feel free to suggest another default wording.

Test plan:
You need to clearly know how this page is working currently to make sure
this patch does not break existing behaviours
1/ Check 2 items out
2/ Add a hold on one of the items
3/ Add 2 holds on the other item
4/ Return the item to mark the hold as waiting
5/ From here you will be able to see entries in the "Circulation › Holds
awaiting pickup" page
6/ I suggest to make a backup of this table, to avoid to repeat the
previous steps. You will need to test the following steps using the
different values of the pref
7/ Cancel the hold
8/ Mark the items as lost
=> You will receive feedback messages depending on different situations:
* The CANCEL_HOLD_ON_LOST does not exist
* The patron does not have an email address (but the notice has been
enqueued! It is the current behaviour with other notices)
* The notice has been enqueued
* The hold has been cancelled

QA Notes:
1/ C4::Circulation::LostItem is usually called with the second parameter
set, which will mark the issue as returned. It is weird that this
behaviour cannot be controlled by a pref.
2/ From which library do we want to pick the notice? This patch use
reserves.branchcode
Comment 4 Jonathan Druart 2017-10-17 20:52:03 UTC
Created attachment 68229 [details] [review]
Bug 19287: Refactor the template to avoid c/p code

The two tabs were using the same code.
The only change I found is the subject of the mailto link, I am not sure
it is relevant to have two different subjects (?)
There was also a phone number info displayed on only one table.
Comment 5 Jonathan Druart 2017-10-17 20:52:07 UTC
Created attachment 68230 [details] [review]
Bug 19287: Add some feedback messages to the librarian

Be kind with librarians and give them feedback messages.
Comment 6 Jonathan Druart 2017-10-17 20:54:44 UTC
(In reply to Jonathan Druart from comment #3)
> Created attachment 68228 [details] [review] [review]
> QA Notes:
> 1/ C4::Circulation::LostItem is usually called with the second parameter
> set, which will mark the issue as returned. It is weird that this
> behaviour cannot be controlled by a pref.

This patch set may need to be rebased on top of bug 12363, to make this behaviour  configurable.
Comment 7 Séverine Queune 2017-10-18 14:21:46 UTC
I tried several times to apply this patch but it never worked.
I first thought it could come from the sandbox but no problem to apply and test bug 12363.
Sorry :/
Comment 8 Jonathan Druart 2017-10-18 14:36:21 UTC
Created attachment 68251 [details] [review]
Bug 19287: Just a bit of cleanup

Let's add a "op" variable to know what the script is supposed to do and
make it less obscure.
Comment 9 Jonathan Druart 2017-10-18 14:36:25 UTC
Created attachment 68252 [details] [review]
Bug 19287: Add new pref CanMarkHoldsAwaitingPickupAsLost

This patch adds a new pref to control the behaviour of this patch set.
Comment 10 Jonathan Druart 2017-10-18 14:36:29 UTC
Created attachment 68253 [details] [review]
Bug 19287: Add ability to mark an item "Lost" from "Holds awaiting pickup" list

This is the main patch of this new enhancement.
The goal to this enhancement is to add the ability to mark items as lost
from the "Holds awaiting pickup" page.
A new pref is added to control the behaviour, default is off
To enable it, you need to set the new pref CanMarkHoldsAwaitingPickupAsLost to one of these two values:
- "Allow to mark items as lost"
=> The item will be marked as lost and the hold will be cancelled
- "Allow to mark items as lost and notify the patron"
=> Same as previously but the patron will be notified as well.

The notification is done using a new notice template (code=CANCEL_HOLD_ON_LOST).
Feel free to suggest another default wording.

Test plan:
You need to clearly know how this page is working currently to make sure
this patch does not break existing behaviours
1/ Check 2 items out
2/ Add a hold on one of the items
3/ Add 2 holds on the other item
4/ Return the item to mark the hold as waiting
5/ From here you will be able to see entries in the "Circulation > Holds
awaiting pickup" page
6/ I suggest to make a backup of this table, to avoid to repeat the
previous steps. You will need to test the following steps using the
different values of the pref
7/ Cancel the hold
8/ Mark the items as lost
=> You will receive feedback messages depending on different situations:
* The CANCEL_HOLD_ON_LOST does not exist
* The patron does not have an email address (but the notice has been
enqueued! It is the current behaviour with other notices)
* The notice has been enqueued
* The hold has been cancelled

QA Notes:
1/ C4::Circulation::LostItem is usually called with the second parameter
set, which will mark the issue as returned. It is weird that this
behaviour cannot be controlled by a pref.
2/ From which library do we want to pick the notice? This patch use
reserves.branchcode
Comment 11 Jonathan Druart 2017-10-18 14:36:33 UTC
Created attachment 68254 [details] [review]
Bug 19287: Refactor the template to avoid c/p code

The two tabs were using the same code.
The only change I found is the subject of the mailto link, I am not sure
it is relevant to have two different subjects (?)
There was also a phone number info displayed on only one table.
Comment 12 Jonathan Druart 2017-10-18 14:36:36 UTC
Created attachment 68255 [details] [review]
Bug 19287: Add some feedback messages to the librarian

Be kind with librarians and give them feedback messages.
Comment 13 Jonathan Druart 2017-10-18 14:37:43 UTC
(In reply to Séverine Queune from comment #7)
> I tried several times to apply this patch but it never worked.
> I first thought it could come from the sandbox but no problem to apply and
> test bug 12363.
> Sorry :/

For the record: "git bz" complained with
UnicodeEncodeError: 'ascii' codec can't encode character u'\u2018' in position 56: ordinal not in range(128)

Because of the report title, it contained "U+2018: LEFT SINGLE QUOTATION MARK"
Comment 14 Séverine Queune 2017-10-18 16:11:29 UTC
Thanks Jonathan for the very quick rebase !

About the tests (only on the 'Holds waiting' tab) :

There is no button displayed on the (circ/waitingreserves.pl) page when the syspref is set to "Allow to mark item as lost".

Using "Allow to mark item as lost and notify patron", behaviours depending on the different parameters (notice existing or not, email stored or not) are as detailed on point 8 of the plan test.
Items are still available after cancelling order.
Is there a syspref you are working on to define which authorized value we want to set to items.itemlost or items.notforloan ?

I set the syspref "ReservesMaxPickUpDelay" to "1 day" and keep several items available on the hold list.
I hope I could test tomorrow the behaviour on the "Holds waiting over x days" tab and change the status of the ticket.

Just a question : is it normal that when I created the notice CANCEL_HOLD_ON_LOST (this code is ok for me), I choose the 'Holds' Koha module, and after saving it, the display is 'Reserves' module ?
Comment 15 Jonathan Druart 2017-10-18 23:01:29 UTC
Created attachment 68284 [details] [review]
Bug 19287: Do not use 'yes' and 'no' for choices

If the pref value is 'yes' or 'no', it will actually be '1' or '0' in
DB, and the comparaison will then fail.
To avoid that I think it is better to rename the value.
Comment 16 Jonathan Druart 2017-10-18 23:06:29 UTC
(In reply to Séverine Queune from comment #14)
> Thanks Jonathan for the very quick rebase !
> 
> About the tests (only on the 'Holds waiting' tab) :
> 
> There is no button displayed on the (circ/waitingreserves.pl) page when the
> syspref is set to "Allow to mark item as lost".

Indeed, I picked 'yes' and 'no' as syspref value, it was not a good idea.
That should be fixed with the last patch.

> Using "Allow to mark item as lost and notify patron", behaviours depending
> on the different parameters (notice existing or not, email stored or not)
> are as detailed on point 8 of the plan test.
> Items are still available after cancelling order.

Which means it is working as expected so far, right? :)

> Is there a syspref you are working on to define which authorized value we
> want to set to items.itemlost or items.notforloan ?

I think there is a patch somewhere, maybe lost in the bug tracker to allow that, but I am not sure.
I'd say it should be discussed and fixed on a separate bug report as it will impact other areas.

> I set the syspref "ReservesMaxPickUpDelay" to "1 day" and keep several items
> available on the hold list.
> I hope I could test tomorrow the behaviour on the "Holds waiting over x
> days" tab and change the status of the ticket.
> 
> Just a question : is it normal that when I created the notice
> CANCEL_HOLD_ON_LOST (this code is ok for me), I choose the 'Holds' Koha
> module, and after saving it, the display is 'Reserves' module ?

You are right, there is a small ergonomic issue. Actually "reserve" is the code for notices of the hold module. You can open a new bug report for that :)
Comment 17 Sally 2017-10-19 12:13:33 UTC
Hi Jonathan,

How does this interact with system preference ExpireReservesMaxPickUpDelayCharge/WaitingHoldCancelationFee ?

The plan says:

=> The item will be marked as lost and the hold will be cancelled

If the WaitingHoldCancelationFee is automatically added, I would be concerned about:

Patron visits library to collect item.
Item is missing.
Staff marks item as lost.
Patron is charged WaitingHoldCancelationFee - even though they tried to collect the item.

Also, this might be out of the scope of this bug (maybe a new bug?) but in a similar scenario:

Patron visits library to collect item.
Item is missing.
Staff marks item as lost.
Patron still wants the item, so a new hold needs to be placed.  

Is it possible to retain the original hold so another copy can fill the request instead of cancelling it?
Comment 18 Séverine Queune 2017-10-20 10:24:41 UTC
I now have a button "Mark item as lost", thanks !

Behaviour is correct also for the "Holds waiting over" tab, except for the last and, to me, the main part of this patch : items are supposed to be marked as lost and not be available, but they are not.

This is one of the enhancement we ask for, as you can see on the third point of the description of this ticket :
>- "set the field “items.notforloan” or “items.itemlost” (define in a system
> preference) from “0” to a value from the same ‘authorized values list’ and set
> in another system preference").

Dis I misunderstood something in you're comment #16 ?

I open ticket #19500 about the name of the module.
Comment 19 Jonathan Druart 2017-10-24 16:15:54 UTC
Hi Sally and Séverine,

(In reply to Sally Healey from comment #17)
> How does this interact with system preference
> ExpireReservesMaxPickUpDelayCharge/WaitingHoldCancelationFee ?

The pref ExpireReservesMaxPickUpDelayCharge is only used by the cronjob cancel_expired_holds.pl. No cancellation fees are charged from this form.

> Also, this might be out of the scope of this bug (maybe a new bug?) but in a
> similar scenario:
> 
> Patron visits library to collect item.
> Item is missing.
> Staff marks item as lost.
> Patron still wants the item, so a new hold needs to be placed.  
> 
> Is it possible to retain the original hold so another copy can fill the
> request instead of cancelling it?

I would consider this as another enhancement indeed. However I do not think it is linked directly to this one particularly.


(In reply to Séverine Queune from comment #18)
> I now have a button "Mark item as lost", thanks !
> 
> Behaviour is correct also for the "Holds waiting over" tab, except for the
> last and, to me, the main part of this patch : items are supposed to be
> marked as lost and not be available, but they are not.
> 
> This is one of the enhancement we ask for, as you can see on the third point
> of the description of this ticket :
> >- "set the field “items.notforloan” or “items.itemlost” (define in a system
> > preference) from “0” to a value from the same ‘authorized values list’ and set
> > in another system preference").
> 
> Dis I misunderstood something in you're comment #16 ?

Nope, I completely forgot that part sorry. I will try to provide a fix soon.

> I open ticket #19500 about the name of the module.

Thanks!
Comment 20 Jonathan Druart 2017-10-24 16:27:43 UTC
(In reply to Séverine Queune from comment #0)
>   - set the field “items.notforloan” or “items.itemlost” (define in a system
> preference) from “0” to a value from the same ‘authorized values list’ and
> set in another system preference

I do not know what is better here. Do we need 2 more sysprefs? Only 1 with a YAML syntax?
Or we can also display the LOST authorised values on the "Holds awaiting pickup" page.
Comment 21 Sally 2017-10-25 13:31:44 UTC
(In reply to Jonathan Druart from comment #19)
> Hi Sally and Séverine,
> 
> (In reply to Sally Healey from comment #17)
> > How does this interact with system preference
> > ExpireReservesMaxPickUpDelayCharge/WaitingHoldCancelationFee ?
> 
> The pref ExpireReservesMaxPickUpDelayCharge is only used by the cronjob
> cancel_expired_holds.pl. No cancellation fees are charged from this form.

Hi Jonathan,

That's great to hear, thanks!

We don't use the cronjob cancel_expired_holds.pl but checking items in via circulation and manually cancelling the hold does charge the expired fee (I mentioned it a little on bug 19486: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19486 )
Comment 22 Séverine Queune 2017-10-25 19:21:50 UTC
(In reply to Jonathan Druart from comment #20)
> (In reply to Séverine Queune from comment #0)
> >   - set the field “items.notforloan” or “items.itemlost” (define in a system
> > preference) from “0” to a value from the same ‘authorized values list’ and
> > set in another system preference
> 
> I do not know what is better here. Do we need 2 more sysprefs? Only 1 with a
> YAML syntax?
> Or we can also display the LOST authorised values on the "Holds awaiting
> pickup" page.

Nicolas agreeds with the yaml syntax syspref !
Can you tell us more about the behaviour you project before starting to develop it (to be sure) ?
Comment 23 Jonathan Druart 2017-10-26 16:50:48 UTC
Suggestion 1:
Bug 11629 adds UpdateNotForLoanStatusOnCheckin and bug 14576 adds UpdateitemLocationOnCheckin.
Both have the same behaviour: it will update the NotForLoan or Location value of the checked in item following the rules defined in the sysprefs.
For instance: 
   NEW: FIC
   FIC: GEN
will modified the value of location to FIC if was NEW, and to GEN if was FIC.

I am not sure it makes sense for us here, but we could imagine a new pref UpdateItemWhenLost which could be filled with:
  itemlost: 1
  notforloan: 2
which will work for any fields of the items table.

Suggestion 2:
We already have a DefaultLongOverdueLostValue, would it make sense to use it here too?

Suggestion 3:
We add 2 other sysprefs: UpdateNotForLoanStatusWhenLost and UpdateItemLostWhenLost (weird name)

Suggestion 4:
We display the different LOST authorised values on the "Holds awaiting pickup" page, like it is on the "Items details" page for Lost, Damaged and Withdrawn.

Vote by text message, send 1, 2, 3 or 4 to 0424242
Comment 24 Katrin Fischer 2017-10-26 18:59:25 UTC
Hi all, trying to understand this bug a bit better. Could you explain the use case for your library a bit? What is the additional not-for-loan used for? Why is it needed in addition to the lost setting?
Comment 25 Nicolas Legrand 2017-11-06 09:45:02 UTC
Hey Katrin,

we use on shelf holds to pick documents from our stacks. Sometimes, the document which is in the catalogue is no more in stacks. When we cancel the hold, it is cancelled silently. Our patrons don't understand why it disappeared and may ask it again.

So, we want our patrons to be informed, and we want to mark the item as lost so it cannot be asked again and again. Those who fetch the books are not always those who edit the catalogue, we would like them to modify the item without having to open the cataloguing module.

Is it more clear? Best regards,
Comment 26 Séverine Queune 2017-11-06 10:22:33 UTC
(In reply to Jonathan Druart from comment #23)
> Suggestion 1:
> Bug 11629 adds UpdateNotForLoanStatusOnCheckin and bug 14576 adds
> UpdateitemLocationOnCheckin.
> Both have the same behaviour: it will update the NotForLoan or Location
> value of the checked in item following the rules defined in the sysprefs.
> For instance: 
>    NEW: FIC
>    FIC: GEN
> will modified the value of location to FIC if was NEW, and to GEN if was FIC.
>
> I am not sure it makes sense for us here, but we could imagine a new pref
> UpdateItemWhenLost which could be filled with:
>   itemlost: 1
>   notforloan: 2
> which will work for any fields of the items table.
> 
> Suggestion 2:
> We already have a DefaultLongOverdueLostValue, would it make sense to use it
> here too?
> 
> Suggestion 3:
> We add 2 other sysprefs: UpdateNotForLoanStatusWhenLost and
> UpdateItemLostWhenLost (weird name)
> 
> Suggestion 4:
> We display the different LOST authorised values on the "Holds awaiting
> pickup" page, like it is on the "Items details" page for Lost, Damaged and
> Withdrawn.
> 
> Vote by text message, send 1, 2, 3 or 4 to 0424242


Sorry for the delay...
We vote suggestion 1.
Comment 27 Séverine Queune 2017-11-06 10:31:42 UTC
(In reply to Katrin Fischer from comment #24)
> Hi all, trying to understand this bug a bit better. Could you explain the
> use case for your library a bit? What is the additional not-for-loan used
> for? Why is it needed in addition to the lost setting?

Hi Katrin,

Some libraries interested with this enhancement may prefer using 'notforloan' instead of the 'itemlost' field (the one we'll use), so we asked for these 2 options to feet the different local usages.
We understand it adds a difficulty for this development.
Comment 28 Jonathan Druart 2017-11-06 18:55:07 UTC
(In reply to Séverine Queune from comment #26)
> We vote suggestion 1.

Sally and Katrin, would you also agree with a new "UpdateItemWhenLost" pref?
Comment 29 Katrin Fischer 2017-11-27 14:21:24 UTC
I like 1) and 4).
1) would allow to set different values depending on the problem that appeared when trying to pull the book.
3) / 4) would be more automated as the values would be configured, which is also nice and wouldn't clutter up the interface.

I don't have a clear preference between those 3 at the moment.

I don't like reusing the long overdue pref 2) as it's often a specific lost value not used otherwise. Often libaries distinguish between not returned by user (lost) and not found on the shelf/misplaced items (missing) or move to different lost status depending on the time it's been not found / times it has been searched for.
Comment 30 Jonathan Druart 2017-12-06 15:53:41 UTC
Ok, so let's say we go with a new pref "UpdateItemWhenLost".
It will get used when an item is marked as lost. It will work for notforloan and other values, but for "itemlost" the behaviour will be weird:
If you mark an item as lost from the batch item tool or the item list view, you will select a value for itemlost already.

So we could restrict the pref to the "Holds awaiting pickup" page (pref UpdateItemWhenMarkedLostFromHoldsAwaitingPickup ??)

Another idea/remark?
Comment 31 Katrin Fischer 2017-12-06 16:41:28 UTC
Ok, still not quite understanding it :)

Why the holds awaiting pickup list? That would mean, you got the book for the patron, you checked it in, you sent the email notice to the patron and THEN you notice it's actually not there... ? Wouldn't it make more sense to this from the holds queue or holds to pull pages that you use when looking for the item?
Comment 32 Séverine Queune 2017-12-12 09:47:00 UTC
(In reply to Katrin Fischer from comment #31)
> Ok, still not quite understanding it :)
> 
> Why the holds awaiting pickup list? That would mean, you got the book for
> the patron, you checked it in, you sent the email notice to the patron and
> THEN you notice it's actually not there... ? Wouldn't it make more sense to
> this from the holds queue or holds to pull pages that you use when looking
> for the item?

You're certainly right Katrin, it would be more logic to have this option on "Holds to pull".
We made a mistake when we though of this enhancement because we have lots of local developments and one the consequence is that the 2 lists "Holds queue" and "Holds to pull" are always empty.
Sorry about that Jonathan, will it be ok for you ?
Comment 33 Jonathan Druart 2017-12-12 17:21:24 UTC
Created attachment 69746 [details] [review]
Bug 19287: Add ability to mark an item ‘Lost’ from ‘Holds to pull’ list

The goal to this enhancement is to add the ability to cancel a hold and
mark items as lost from the ‘Holds to pull’ page.
A new pref is added to control the behaviour, default is off

To mark items as lost you need to set the new pref CanMarkHoldsToPullAsLost to one of these two values:
- "Allow to mark items as lost"
=> The item will be marked as lost and the hold will be cancelled
- "Allow to mark items as lost and notify the patron"
=> Same as previously but the patron will be notified as well.

The notification is done using a new notice template (code=CANCEL_HOLD_ON_LOST).
Feel free to suggest another default wording.

To update the values of the items when one of these 2 buttons are used, you need to
fill the UpdateItemWhenLostFromHoldList with something like: 'itemlost: 1'

Test plan:
1/ Place holds on some items
2/ Go to Home › Circulation › Holds to pull
From here you can cancel the hold or
3/ Mark the items as lost
=> You will receive feedback messages depending on different situations:
* The CANCEL_HOLD_ON_LOST does not exist
* The patron does not have an email address (but the notice has been
enqueued! It is the current behaviour with other notices)
* The notice has been enqueued
* The hold has been cancelled

QA Notes:
From which library do we want to pick the notice? This patch use
reserves.branchcode
Comment 34 Jonathan Druart 2017-12-12 17:24:04 UTC
I am not convinced by this patch, but you can try it and let me know how do you feel about it.
There are now 2 prefs CanMarkHoldsToPullAsLost and UpdateItemWhenLostFromHoldList.
Comment 35 Séverine Queune 2018-01-17 13:34:32 UTC
Did I miss a dependency ? The patch doesn’t seem to apply.
Comment 36 Jonathan Druart 2018-01-18 18:10:38 UTC
Created attachment 70705 [details] [review]
Bug 19287: Add ability to mark an item 'Lost' from 'Holds to pull' list

The goal to this enhancement is to add the ability to cancel a hold and
mark items as lost from the 'Holds to pull' page.
A new pref is added to control the behaviour, default is off

To mark items as lost you need to set the new pref CanMarkHoldsToPullAsLost to one of these two values:
- "Allow to mark items as lost"
=> The item will be marked as lost and the hold will be cancelled
- "Allow to mark items as lost and notify the patron"
=> Same as previously but the patron will be notified as well.

The notification is done using a new notice template (code=CANCEL_HOLD_ON_LOST).
Feel free to suggest another default wording.

To update the values of the items when one of these 2 buttons are used, you need to
fill the UpdateItemWhenLostFromHoldList with something like: 'itemlost: 1'

Test plan:
1/ Place holds on some items
2/ Go to Home > Circulation > Holds to pull
From here you can cancel the hold or
3/ Mark the items as lost
=> You will receive feedback messages depending on different situations:
* The CANCEL_HOLD_ON_LOST does not exist
* The patron does not have an email address (but the notice has been
enqueued! It is the current behaviour with other notices)
* The notice has been enqueued
* The hold has been cancelled

QA Notes:
From which library do we want to pick the notice? This patch use
reserves.branchcode
Comment 37 Jonathan Druart 2018-01-18 18:11:10 UTC
(In reply to Séverine Queune from comment #35)
> Did I miss a dependency ? The patch doesn’t seem to apply.

It applied correctly here, could you test again with this rebased version?
Comment 38 Séverine Queune 2018-01-19 15:29:37 UTC
(In reply to Jonathan Druart from comment #37)
> (In reply to Séverine Queune from comment #35)
> > Did I miss a dependency ? The patch doesn’t seem to apply.
> 
> It applied correctly here, could you test again with this rebased version?

New patch applies correctly.

- with "Allow and notify" option, I get this sotware error :
Undefined subroutine &C4::Members::GetNoticeEmailAddress called at /home/koha/src/circ/pendingreserves.pl line 95.

- "Mark as lost" works well : hold is cancelled, the fields set in the syspref are correctly updated (and when I choose itemlost is changed, itemlost_on is "timestamped" (can I say it ?)).

Cancellation is blocked with this 2 options if hold is placed on biblio.

- with the syspref set to "Do not allow", I only see the usual option "Cancel hold".



(In reply to Katrin Fischer from comment #31)
> Why the holds awaiting pickup list? That would mean, you got the book for
> the patron, you checked it in, you sent the email notice to the patron and
> THEN you notice it's actually not there... ? Wouldn't it make more sense to
> this from the holds queue or holds to pull pages that you use when looking
> for the item?

That was definetly a good idea to use the "Holds to pull" list, thank you Katrin !
Comment 39 Jonathan Druart 2018-01-19 16:41:58 UTC
Created attachment 70764 [details] [review]
Bug 19287: Fix conflict with bug 19304
Comment 40 Jonathan Druart 2018-01-19 16:42:39 UTC
(In reply to Séverine Queune from comment #38)
> (In reply to Jonathan Druart from comment #37)
> > (In reply to Séverine Queune from comment #35)
> - with "Allow and notify" option, I get this sotware error :
> Undefined subroutine &C4::Members::GetNoticeEmailAddress called at
> /home/koha/src/circ/pendingreserves.pl line 95.

Yes, conflict with bug 19287, should be ok now.
Comment 41 Séverine Queune 2018-01-24 16:56:14 UTC
(In reply to Jonathan Druart from comment #40)
> (In reply to Séverine Queune from comment #38)
> > (In reply to Jonathan Druart from comment #37)
> > > (In reply to Séverine Queune from comment #35)
> > - with "Allow and notify" option, I get this sotware error :
> > Undefined subroutine &C4::Members::GetNoticeEmailAddress called at
> > /home/koha/src/circ/pendingreserves.pl line 95.
> 
> Yes, conflict with bug 19287, should be ok now.

Cancellation still works with the "Allow" only option.

I have this (normal) Software error when I used the "Allow and notify" option :
> The method 0 is not covered by tests!
Email is "pending" on the patron account, but I can't be sure that, in a functional instance, cancellation is made because I can't refresh the error page, and when I go back to the "Holds to pull" page or the record, hold is still on the item.

I will see tomorrow with Nicolas, maybe he could install this patch on our Koha-Test and we could see if everything works as expected.

Is that ok with you ?
Comment 42 Biblibre Sandboxes 2018-01-25 16:28:11 UTC
Patch tested with a sandbox, by Séverine QUEUNE <severine.queune@bulac.fr>
Comment 43 Biblibre Sandboxes 2018-01-25 16:28:46 UTC
Created attachment 70918 [details] [review]
Bug 19287: Add ability to mark an item 'Lost' from 'Holds to pull' list

The goal to this enhancement is to add the ability to cancel a hold and
mark items as lost from the 'Holds to pull' page.
A new pref is added to control the behaviour, default is off

To mark items as lost you need to set the new pref CanMarkHoldsToPullAsLost to one of these two values:
- "Allow to mark items as lost"
=> The item will be marked as lost and the hold will be cancelled
- "Allow to mark items as lost and notify the patron"
=> Same as previously but the patron will be notified as well.

The notification is done using a new notice template (code=CANCEL_HOLD_ON_LOST).
Feel free to suggest another default wording.

To update the values of the items when one of these 2 buttons are used, you need to
fill the UpdateItemWhenLostFromHoldList with something like: 'itemlost: 1'

Test plan:
1/ Place holds on some items
2/ Go to Home > Circulation > Holds to pull
From here you can cancel the hold or
3/ Mark the items as lost
=> You will receive feedback messages depending on different situations:
* The CANCEL_HOLD_ON_LOST does not exist
* The patron does not have an email address (but the notice has been
enqueued! It is the current behaviour with other notices)
* The notice has been enqueued
* The hold has been cancelled

QA Notes:
From which library do we want to pick the notice? This patch use
reserves.branchcode

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 44 Biblibre Sandboxes 2018-01-25 16:28:49 UTC
Created attachment 70919 [details] [review]
Bug 19287: Fix conflict with bug 19304

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 45 Katrin Fischer 2018-04-08 21:21:08 UTC
Comment on attachment 70918 [details] [review]
Bug 19287: Add ability to mark an item 'Lost' from 'Holds to pull' list

Review of attachment 70918 [details] [review]:
-----------------------------------------------------------------

Hi Jonathan, 

I'll add a see also to bug 19974 - might need a follow-up depending on it.

>QA Notes:
>From which library do we want to pick the notice? This patch use
>reserves.branchcode
_send_message_by_email will be using the homebranch of the library for getting the right return-path and reply-to addresses for the email. I think instead of the pick-up library i'd use the homebranch of the patron to make it all match.

1) Please add the new notice to the different installers for new installations.
2) New action column is not in column visibility settings. I think for print it might make sense that someone wants to hide it from display.

::: installer/data/mysql/atomicupdate/bug_19287.sql
@@ +1,3 @@
> +INSERT IGNORE INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES ('CanMarkHoldsToPullAsLost','do_not_allow','do_not_allow|allow|allow_and_notify','Add a button to the "Holds to pull" screen to mark an item as lost and notify the patron.','Choice');
> +
> +INSERT IGNORE INTO letter(module, code, branchcode, name, is_html, title, content, message_transport_type, lang) VALUES ('reserves', 'CANCEL_HOLD_ON_LOST', '', 'Hold has been cancelled', 0, "Hold has been cancelled", "Dear [% borrower.firstname %] [% borrower.surname %],\n\nWe regret to inform you that the following item cannot longer be placed on hold, it has been marked as lost\n\nTitle: [% biblio.title %]\nAuthor: [% biblio.author %]\nCopy: [% item.copynumber %]\nLocation: [% branch.branchname %]", 'email', 'default');

3) I think maybe:
We regret to inform you, that the following item can not be provided due to it being missing. Your hold was cancelled.

::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref
@@ +712,5 @@
> +        -
> +            - Update item's values when marked as lost from the hold to pull screen
> +            - pref: UpdateItemWhenLostFromHoldList
> +              type: textarea
> +            - This is a list of values to update an item when it is marked as lost from the holds to pull screen.

4) I think it would be good to give an example here for the syntax. I am not sure how to set it and to test it. And I think have all the text above the input field.

::: koha-tmpl/intranet-tmpl/prog/en/modules/circ/pendingreserves.tt
@@ +34,5 @@
> +                    <span>There is no notice template with a code 'CANCEL_HOLD_ON_LOST' defined in your system.</span>
> +                [% CASE 'hold_cancelled' %]
> +                    <span>The hold has been correctly cancelled.</span>
> +                [% CASE 'hold_placed_at_biblio_level' %]
> +                    <span>The hold has been placed at biblio level. It is not possible to mark it as lost.</span>

5) Maybe we should not display the button in this case?
The hold has been placed on biblio level. It is not possible to determine the item to mark as lost. (the it in the second sentence refers to hold for me, and you can't mark holds lost)

@@ +125,5 @@
> +                    <input type="hidden" name="op" value="mark_as_lost" />
> +                    <input type="submit" value="Mark item as lost" />
> +                [% ELSIF Koha.Preference('CanMarkHoldsToPullAsLost') == 'allow_and_notify' %]
> +                    <input type="hidden" name="op" value="mark_as_lost_and_notify" />
> +                    <input type="submit" value="Mark item as lost and notify the patron" />

6) Mark lost and notify patron (shorter for the button)
Comment 46 Jonathan Druart 2018-04-09 18:11:15 UTC
Created attachment 73881 [details] [review]
Bug 19287: Add ability to mark an item 'Lost' from 'Holds to pull' list

The goal to this enhancement is to add the ability to cancel a hold and
mark items as lost from the 'Holds to pull' page.
A new pref is added to control the behaviour, default is off

To mark items as lost you need to set the new pref CanMarkHoldsToPullAsLost to one of these two values:
- "Allow to mark items as lost"
=> The item will be marked as lost and the hold will be cancelled
- "Allow to mark items as lost and notify the patron"
=> Same as previously but the patron will be notified as well.

The notification is done using a new notice template (code=CANCEL_HOLD_ON_LOST).
Feel free to suggest another default wording.

To update the values of the items when one of these 2 buttons are used, you need to
fill the UpdateItemWhenLostFromHoldList with something like: 'itemlost: 1'

Test plan:
1/ Place holds on some items
2/ Go to Home > Circulation > Holds to pull
From here you can cancel the hold or
3/ Mark the items as lost
=> You will receive feedback messages depending on different situations:
* The CANCEL_HOLD_ON_LOST does not exist
* The patron does not have an email address (but the notice has been
enqueued! It is the current behaviour with other notices)
* The notice has been enqueued
* The hold has been cancelled

QA Notes:
From which library do we want to pick the notice? This patch use
reserves.branchcode

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 47 Jonathan Druart 2018-04-09 18:11:21 UTC
Created attachment 73882 [details] [review]
Bug 19287: Fix conflict with bug 19304

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Comment 48 Jonathan Druart 2018-04-09 18:11:26 UTC
Created attachment 73883 [details] [review]
Bug 19287: Fix conflict with bug 19974

Note that these patches should be pushed along with bug 19984 to avoid
regressions (if the syspref is changed between the 2 update DB entries)
Comment 49 Jonathan Druart 2018-04-09 18:11:30 UTC
Created attachment 73884 [details] [review]
Bug 19287: Use the notice defined for the homebranch of the patron
Comment 50 Jonathan Druart 2018-04-09 18:11:35 UTC
Created attachment 73885 [details] [review]
Bug 19287: Add CANCEL_HOLD_ON_LOST to installer files for all languages
Comment 51 Jonathan Druart 2018-04-09 18:11:39 UTC
Created attachment 73886 [details] [review]
Bug 19287: Add column action to columns settings
Comment 52 Jonathan Druart 2018-04-09 18:11:43 UTC
Created attachment 73887 [details] [review]
Bug 19287: Update UpdateItemWhenLostFromHoldList description
Comment 53 Jonathan Druart 2018-04-09 18:11:48 UTC
Created attachment 73888 [details] [review]
Bug 19287: Do not display buttons for biblio level holds
Comment 54 Jonathan Druart 2018-04-09 18:11:52 UTC
Created attachment 73889 [details] [review]
Bug 19287: Shorten button text
Comment 55 Jonathan Druart 2018-04-09 18:12:50 UTC
(In reply to Katrin Fischer from comment #45)
> Comment on attachment 70918 [details] [review] [review]
> Bug 19287: Add ability to mark an item 'Lost' from 'Holds to pull' list
> 
> Review of attachment 70918 [details] [review] [review]:

Thanks Katrin!
All your remarks have been taken into account in different patch to ease next reviews.
They can be squashed if needed.
Comment 56 Katrin Fischer 2018-04-10 05:41:29 UTC
The third patch won't apply, can you please take a look?

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 19287: Add ability to mark an item 'Lost' from 'Holds to pull' list
Applying: Bug 19287: Fix conflict with bug 19304
Applying: Bug 19287: Fix conflict with bug 19974
error: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref).
error: could not build fake ancestor
Patch failed at 0001 Bug 19287: Fix conflict with bug 19974
The copy of the patch that failed is found in: .git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-19287-Fix-conflict-with-bug-19974-Ayu3UB.patch
Comment 57 Jonathan Druart 2018-04-11 15:12:00 UTC
Dependency was missing
Comment 58 Katrin Fischer 2018-04-12 10:22:52 UTC
Blocked by dependency.
Comment 59 Katrin Fischer 2018-04-22 18:44:31 UTC
Created attachment 74696 [details] [review]
Bug 19287: Add ability to mark an item 'Lost' from 'Holds to pull' list

The goal to this enhancement is to add the ability to cancel a hold and
mark items as lost from the 'Holds to pull' page.
A new pref is added to control the behaviour, default is off

To mark items as lost you need to set the new pref CanMarkHoldsToPullAsLost to one of these two values:
- "Allow to mark items as lost"
=> The item will be marked as lost and the hold will be cancelled
- "Allow to mark items as lost and notify the patron"
=> Same as previously but the patron will be notified as well.

The notification is done using a new notice template (code=CANCEL_HOLD_ON_LOST).
Feel free to suggest another default wording.

To update the values of the items when one of these 2 buttons are used, you need to
fill the UpdateItemWhenLostFromHoldList with something like: 'itemlost: 1'

Test plan:
1/ Place holds on some items
2/ Go to Home > Circulation > Holds to pull
From here you can cancel the hold or
3/ Mark the items as lost
=> You will receive feedback messages depending on different situations:
* The CANCEL_HOLD_ON_LOST does not exist
* The patron does not have an email address (but the notice has been
enqueued! It is the current behaviour with other notices)
* The notice has been enqueued
* The hold has been cancelled

QA Notes:
From which library do we want to pick the notice? This patch use
reserves.branchcode

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 60 Katrin Fischer 2018-04-22 18:44:36 UTC
Created attachment 74697 [details] [review]
Bug 19287: Fix conflict with bug 19304

Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>
Signed-off-by: Séverine QUEUNE <severine.queune@bulac.fr>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 61 Katrin Fischer 2018-04-22 18:44:41 UTC
Created attachment 74698 [details] [review]
Bug 19287: Fix conflict with bug 19974

Note that these patches should be pushed along with bug 19984 to avoid
regressions (if the syspref is changed between the 2 update DB entries)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 62 Katrin Fischer 2018-04-22 18:44:46 UTC
Created attachment 74699 [details] [review]
Bug 19287: Use the notice defined for the homebranch of the patron

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 63 Katrin Fischer 2018-04-22 18:44:51 UTC
Created attachment 74700 [details] [review]
Bug 19287: Add CANCEL_HOLD_ON_LOST to installer files for all languages

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 64 Katrin Fischer 2018-04-22 18:44:55 UTC
Created attachment 74701 [details] [review]
Bug 19287: Add column action to columns settings

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 65 Katrin Fischer 2018-04-22 18:45:00 UTC
Created attachment 74702 [details] [review]
Bug 19287: Update UpdateItemWhenLostFromHoldList description

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 66 Katrin Fischer 2018-04-22 18:45:05 UTC
Created attachment 74703 [details] [review]
Bug 19287: Do not display buttons for biblio level holds

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 67 Katrin Fischer 2018-04-22 18:45:10 UTC
Created attachment 74704 [details] [review]
Bug 19287: Shorten button text

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 68 Katrin Fischer 2018-04-22 18:45:15 UTC
Created attachment 74705 [details] [review]
Bug 19287: (QA follow-up) Fix preference description

The last part of the preference description didn't show.
Moved behind the other text and rephrased.
Comment 69 Katrin Fischer 2018-04-22 18:46:30 UTC
I's probalby not even show the hint about the biblio leve hold, but that's just a matter of taste :)
Comment 70 Jonathan Druart 2018-04-23 15:56:32 UTC
Created attachment 74749 [details] [review]
Bug 19287: Add CANCEL_HOLD_ON_LOST to installer files for all languages

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 71 Jonathan Druart 2018-04-23 17:29:04 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 72 Jonathan Druart 2018-05-24 13:10:00 UTC
Created attachment 75535 [details] [review]
Bug 19287: (follow-up) Use the notice defined for the library of the patron
Comment 73 Jonathan Druart 2018-05-24 13:11:07 UTC
(In reply to Jonathan Druart from comment #72)
> Created attachment 75535 [details] [review] [review]
> Bug 19287: (follow-up) Use the notice defined for the library of the patron

Fix the bug caught by Séverine on bug 20767 comment 4.
Comment 74 Jonathan Druart 2018-05-24 16:43:07 UTC
Last patch pushed to master for 18.05
Comment 75 Nick Clemens 2018-11-20 16:33:35 UTC
update systempreferences set options="batchmod|moredetail|cronjob|additem|pendingreserves", value="batchmod|moredetail|cronjob|additem|pendingreserves" where variable="MarkLostItemsAsReturned";

This update is wrong - we should not be setting the options for all installs, and they should be comma separated

Filed bug 21869