Bug 17509 - Notify patrons to return items requested on hold by another person
Summary: Notify patrons to return items requested on hold by another person
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature with 12 votes (vote)
Assignee: Radek Šiman (R-Bit Technology, s.r.o.)
QA Contact: Testopia
URL:
Keywords:
: 13846 (view as bug list)
Depends on:
Blocks:
 
Reported: 2016-10-27 10:29 UTC by Radek Šiman (R-Bit Technology, s.r.o.)
Modified: 2023-07-05 13:44 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 17509: Added NotifyToReturnItemWhenHoldIsPlaced syspref (3.39 KB, patch)
2016-11-03 22:19 UTC, Radek Šiman (R-Bit Technology, s.r.o.)
Details | Diff | Splinter Review
Bug 17509: Added system preference to restrict recipients based on library (3.98 KB, patch)
2016-11-03 22:19 UTC, Radek Šiman (R-Bit Technology, s.r.o.)
Details | Diff | Splinter Review
Bug 17509: Improved system preferences for item return notification (4.43 KB, patch)
2016-11-03 22:20 UTC, Radek Šiman (R-Bit Technology, s.r.o.)
Details | Diff | Splinter Review
Bug 17509: renamed atomic file to suit conventions (1.34 KB, patch)
2016-11-03 22:20 UTC, Radek Šiman (R-Bit Technology, s.r.o.)
Details | Diff | Splinter Review
Bug 17509: Removed old name of atomic update file (1.35 KB, patch)
2016-11-03 22:20 UTC, Radek Šiman (R-Bit Technology, s.r.o.)
Details | Diff | Splinter Review
Bug 17509: Added notification letter HOLDPLACED_CONTACT (31.85 KB, patch)
2016-11-03 22:20 UTC, Radek Šiman (R-Bit Technology, s.r.o.)
Details | Diff | Splinter Review
Bug 17509: notify patrons to return an item whenever a hold is placed on it (4.84 KB, patch)
2016-11-03 22:20 UTC, Radek Šiman (R-Bit Technology, s.r.o.)
Details | Diff | Splinter Review
Bug 17509: Fixed invalid patterns (1.43 KB, patch)
2016-11-03 22:21 UTC, Radek Šiman (R-Bit Technology, s.r.o.)
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: Added NotifyToReturnItemWhenHoldIsPlaced syspref (3.46 KB, patch)
2016-11-04 09:20 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: Added system preference to restrict recipients based on library (4.05 KB, patch)
2016-11-04 09:20 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: Improved system preferences for item return notification (4.50 KB, patch)
2016-11-04 09:20 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: renamed atomic file to suit conventions (1.41 KB, patch)
2016-11-04 09:21 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: Removed old name of atomic update file (1.41 KB, patch)
2016-11-04 09:21 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: Added notification letter HOLDPLACED_CONTACT (31.93 KB, patch)
2016-11-04 09:21 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: notify patrons to return an item whenever a hold is placed on it (4.88 KB, patch)
2016-11-04 09:21 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: Fixed invalid patterns (1.33 KB, patch)
2016-11-04 09:21 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: Added tests (5.72 KB, patch)
2017-05-08 20:53 UTC, Radek Šiman (R-Bit Technology, s.r.o.)
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: Added NotifyToReturnItemWhenHoldIsPlaced syspref (3.42 KB, patch)
2017-05-09 06:26 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: Added system preference to restrict recipients based on library (4.02 KB, patch)
2017-05-09 06:27 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: Improved system preferences for item return notification (4.47 KB, patch)
2017-05-09 06:27 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: renamed atomic file to suit conventions (1.41 KB, patch)
2017-05-09 06:27 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: Removed old name of atomic update file (1.41 KB, patch)
2017-05-09 06:27 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: Added notification letter HOLDPLACED_CONTACT (32.04 KB, patch)
2017-05-09 06:27 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: notify patrons to return an item whenever a hold is placed on it (4.88 KB, patch)
2017-05-09 06:27 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: Fixed invalid patterns (1.33 KB, patch)
2017-05-09 06:27 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 17509: Added tests (5.80 KB, patch)
2017-05-09 06:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: (followup) Logic moved from C4 to Koha namespace (8.28 KB, patch)
2017-10-11 23:08 UTC, Radek Šiman (R-Bit Technology, s.r.o.)
Details | Diff | Splinter Review
Bug 17509: Added NotifyToReturnItemWhenHoldIsPlaced syspref (3.41 KB, patch)
2018-10-12 08:26 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: Added system preference to restrict recipients based on library (4.00 KB, patch)
2018-10-12 08:26 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: Improved system preferences for item return notification (4.46 KB, patch)
2018-10-12 08:26 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: renamed atomic file to suit conventions (1.40 KB, patch)
2018-10-12 08:26 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: Removed old name of atomic update file (1.40 KB, patch)
2018-10-12 08:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: Added notification letter HOLDPLACED_CONTACT (32.73 KB, patch)
2018-10-12 08:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: notify patrons to return an item whenever a hold is placed on it (4.86 KB, patch)
2018-10-12 08:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: Fixed invalid patterns (1.32 KB, patch)
2018-10-12 08:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: Added tests (5.79 KB, patch)
2018-10-12 08:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: (follow-up) Logic moved from C4 to Koha namespace (8.33 KB, patch)
2018-10-12 08:27 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: Added NotifyToReturnItemWhenHoldIsPlaced syspref (4.36 KB, patch)
2019-03-08 10:12 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: Added notification letter HOLDPLACED_CONTACT (32.83 KB, patch)
2019-03-08 10:12 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: notify patrons to return an item whenever a hold is placed on it (4.91 KB, patch)
2019-03-08 10:12 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: Added tests (5.80 KB, patch)
2019-03-08 10:12 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17509: (follow-up) Logic moved from C4 to Koha namespace (8.35 KB, patch)
2019-03-08 10:12 UTC, Josef Moravec
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Radek Šiman (R-Bit Technology, s.r.o.) 2016-10-27 10:29:58 UTC
Create a new notification allowing to ask patrons to return a book which has just been put on hold by somebody else.
Comment 1 Radek Šiman (R-Bit Technology, s.r.o.) 2016-11-03 22:19:38 UTC
Created attachment 57143 [details] [review]
Bug 17509: Added NotifyToReturnItemWhenHoldIsPlaced syspref

This system preference will allow to ask patrons to return items
whenever a new hold is placed on them

Test plan:
1) Open System preferences / Circulation / Holds Policy
2) Change value from Enable to Don't enable and vice versa
3) Check NotifyToReturnItemWhenHoldIsPlaced is changed in database
Comment 2 Radek Šiman (R-Bit Technology, s.r.o.) 2016-11-03 22:19:57 UTC
Created attachment 57144 [details] [review]
Bug 17509: Added system preference to restrict recipients based on library

Test plan:
1) apply patch
2) go to System preferences / Circulation / Holds Policy
3) change which patrons will be contacted
4) compare your change against database value
NotifyToReturnItemFromLibrary
Comment 3 Radek Šiman (R-Bit Technology, s.r.o.) 2016-11-03 22:20:08 UTC
Created attachment 57145 [details] [review]
Bug 17509: Improved system preferences for item return notification

Test plan:
1) apply patch
2) go to System preferences / Circulation / Holds Policy
3) change NotifyToReturn* variables as you wish
4) verify your changes in database
Comment 4 Radek Šiman (R-Bit Technology, s.r.o.) 2016-11-03 22:20:20 UTC
Created attachment 57146 [details] [review]
Bug 17509: renamed atomic file to suit conventions
Comment 5 Radek Šiman (R-Bit Technology, s.r.o.) 2016-11-03 22:20:31 UTC
Created attachment 57147 [details] [review]
Bug 17509: Removed old name of atomic update file
Comment 6 Radek Šiman (R-Bit Technology, s.r.o.) 2016-11-03 22:20:46 UTC
Created attachment 57148 [details] [review]
Bug 17509: Added notification letter HOLDPLACED_CONTACT
Comment 7 Radek Šiman (R-Bit Technology, s.r.o.) 2016-11-03 22:20:59 UTC
Created attachment 57149 [details] [review]
Bug 17509: notify patrons to return an item whenever a hold is placed on it

Test plan:
1) apply patch
2) run database update
3) enable sending an email to the patrons to return an item whenever a
   hold request is placed on it
4) place a hold
5) patrons having checked-out the item on hold are notified by email
   (see HOLDPLACED_CONTACT letter)
Comment 8 Radek Šiman (R-Bit Technology, s.r.o.) 2016-11-03 22:21:13 UTC
Created attachment 57150 [details] [review]
Bug 17509: Fixed invalid patterns
Comment 9 Josef Moravec 2016-11-04 09:20:38 UTC
Created attachment 57157 [details] [review]
[SIGNED-OFF] Bug 17509: Added NotifyToReturnItemWhenHoldIsPlaced syspref

This system preference will allow to ask patrons to return items
whenever a new hold is placed on them

Test plan:
1) Open System preferences / Circulation / Holds Policy
2) Change value from Enable to Don't enable and vice versa
3) Check NotifyToReturnItemWhenHoldIsPlaced is changed in database

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 10 Josef Moravec 2016-11-04 09:20:53 UTC
Created attachment 57159 [details] [review]
[SIGNED-OFF] Bug 17509: Added system preference to restrict recipients based on library

Test plan:
1) apply patch
2) go to System preferences / Circulation / Holds Policy
3) change which patrons will be contacted
4) compare your change against database value
NotifyToReturnItemFromLibrary

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 11 Josef Moravec 2016-11-04 09:20:57 UTC
Created attachment 57160 [details] [review]
[SIGNED-OFF] Bug 17509: Improved system preferences for item return notification

Test plan:
1) apply patch
2) go to System preferences / Circulation / Holds Policy
3) change NotifyToReturn* variables as you wish
4) verify your changes in database

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 12 Josef Moravec 2016-11-04 09:21:02 UTC
Created attachment 57161 [details] [review]
[SIGNED-OFF] Bug 17509: renamed atomic file to suit conventions

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 13 Josef Moravec 2016-11-04 09:21:06 UTC
Created attachment 57162 [details] [review]
[SIGNED-OFF] Bug 17509: Removed old name of atomic update file

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 14 Josef Moravec 2016-11-04 09:21:11 UTC
Created attachment 57163 [details] [review]
[SIGNED-OFF] Bug 17509: Added notification letter HOLDPLACED_CONTACT

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 15 Josef Moravec 2016-11-04 09:21:15 UTC
Created attachment 57164 [details] [review]
[SIGNED-OFF] Bug 17509: notify patrons to return an item whenever a hold is placed on it

Test plan:
1) apply patch
2) run database update
3) enable sending an email to the patrons to return an item whenever a
   hold request is placed on it
4) place a hold
5) patrons having checked-out the item on hold are notified by email
   (see HOLDPLACED_CONTACT letter)

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 16 Josef Moravec 2016-11-04 09:21:19 UTC
Created attachment 57165 [details] [review]
[SIGNED-OFF] Bug 17509: Fixed invalid patterns

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 17 Jonathan Druart 2016-12-14 11:21:02 UTC
2 quick comments:

1/ Maybe it would be better to have only 1 syspref, or at least squash them into the same syspref block (in the .pref file)
2/ It would be great to move GetBorrowersToSatisfyHold to the Koha namespace and return objects instead.

I don't think they are blocker, but recommended.

I am failing it because you do not provide tests for the changes you made in subroutines.
Comment 18 Radek Šiman (R-Bit Technology, s.r.o.) 2016-12-14 14:55:30 UTC
Jonathan, could you please tell me more regarding your syspref suggestion? I thought it's been already squashed into one single section having 2 sysprefs. Have you meant to do it another way? This is current layout:
http://prntscr.com/djd0f0
Comment 19 Jonathan Druart 2016-12-14 16:37:22 UTC
(In reply to Radek Šiman (R-Bit Technology, s.r.o.) from comment #18)
> Jonathan, could you please tell me more regarding your syspref suggestion? I
> thought it's been already squashed into one single section having 2
> sysprefs. Have you meant to do it another way? This is current layout:
> http://prntscr.com/djd0f0

Indeed, I wrongly wrote the diff. You can forget this point.
Comment 20 Katrin Fischer 2016-12-27 09:57:01 UTC
*** Bug 13846 has been marked as a duplicate of this bug. ***
Comment 21 Pierre Genty 2017-03-23 09:23:11 UTC
Hi, 
We are really interested in this new feature. Is someone still working on it ?
Comment 22 Radek Šiman (R-Bit Technology, s.r.o.) 2017-03-23 09:31:01 UTC
Hi, yes, it's still in development, but I'm currently working on another important issue within payments, so this one has to wait a bit. We need this kind of notification too.
Comment 23 Radek Šiman (R-Bit Technology, s.r.o.) 2017-05-08 20:53:29 UTC
Created attachment 63258 [details] [review]
Bug 17509: Added tests

prove t/db_dependent/Reserves/Notification.t
Comment 24 Josef Moravec 2017-05-09 06:26:45 UTC
Created attachment 63269 [details] [review]
[SIGNED-OFF] Bug 17509: Added NotifyToReturnItemWhenHoldIsPlaced syspref

This system preference will allow to ask patrons to return items
whenever a new hold is placed on them

Test plan:
1) Open System preferences / Circulation / Holds Policy
2) Change value from Enable to Don't enable and vice versa
3) Check NotifyToReturnItemWhenHoldIsPlaced is changed in database

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 25 Josef Moravec 2017-05-09 06:27:04 UTC
Created attachment 63270 [details] [review]
[SIGNED-OFF] Bug 17509: Added system preference to restrict recipients based on library

Test plan:
1) apply patch
2) go to System preferences / Circulation / Holds Policy
3) change which patrons will be contacted
4) compare your change against database value
NotifyToReturnItemFromLibrary

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 26 Josef Moravec 2017-05-09 06:27:14 UTC
Created attachment 63271 [details] [review]
[SIGNED-OFF] Bug 17509: Improved system preferences for item return notification

Test plan:
1) apply patch
2) go to System preferences / Circulation / Holds Policy
3) change NotifyToReturn* variables as you wish
4) verify your changes in database

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 27 Josef Moravec 2017-05-09 06:27:20 UTC
Created attachment 63272 [details] [review]
[SIGNED-OFF] Bug 17509: renamed atomic file to suit conventions

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 28 Josef Moravec 2017-05-09 06:27:25 UTC
Created attachment 63273 [details] [review]
[SIGNED-OFF] Bug 17509: Removed old name of atomic update file

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 29 Josef Moravec 2017-05-09 06:27:30 UTC
Created attachment 63274 [details] [review]
[SIGNED-OFF] Bug 17509: Added notification letter HOLDPLACED_CONTACT

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 30 Josef Moravec 2017-05-09 06:27:35 UTC
Created attachment 63275 [details] [review]
[SIGNED-OFF] Bug 17509: notify patrons to return an item whenever a hold is placed on it

Test plan:
1) apply patch
2) run database update
3) enable sending an email to the patrons to return an item whenever a
   hold request is placed on it
4) place a hold
5) patrons having checked-out the item on hold are notified by email
   (see HOLDPLACED_CONTACT letter)

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 31 Josef Moravec 2017-05-09 06:27:40 UTC
Created attachment 63276 [details] [review]
[SIGNED-OFF] Bug 17509: Fixed invalid patterns

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 32 Josef Moravec 2017-05-09 06:27:45 UTC
Created attachment 63277 [details] [review]
[SIGNED-OFF] Bug 17509: Added tests

prove t/db_dependent/Reserves/Notification.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Test pass
Comment 33 Josef Moravec 2017-05-09 06:28:58 UTC
I tested the last patch and rebased the whole patchset on master.
Comment 34 David Cook 2017-06-13 01:50:35 UTC
In other systems, I think they refer to this as "recalling" an item. We might want to use similar terminology for the sake of users.
Comment 35 Katrin Fischer 2017-06-13 05:52:01 UTC
I think a recall goes usually further than this, possibly shortening the due date and removing the option to renew etc. - I think this is just a friendly email so not quite a recall yet.
Comment 36 Radek Šiman (R-Bit Technology, s.r.o.) 2017-06-13 06:19:58 UTC
This feature is meant exactly as Katrin said: it kindly asks the user to return the item ASAP only.
Comment 37 Liz Rea 2017-07-26 23:29:45 UTC
Let's start here with recalls, and build the other stuff in.
Comment 38 Radek Šiman (R-Bit Technology, s.r.o.) 2017-07-27 07:22:01 UTC
I'd like to see this patch to be published as is. The other stuff should be developed in a separately filed bug.
Comment 39 Liz Rea 2017-07-27 21:06:18 UTC
That's what I mean, let's get this in and build the rest of recalls off of it somewhere else.
Comment 40 Nick Clemens 2017-07-31 15:23:41 UTC
I like this one a lot, but a few issues:
1 - GetMember has been removed should use Koha::Patrons
2 - The sub should not go into C4:Reserves but should be a class method in Koha::Holds
you can get patrons with something like (adding conditions per the pref) 
Koha::Patrons->search({'item.biblionumber'=>$hold->biblionumber,issue_id=>{'!=' => undef} },{ join => {'issues' => 'item'} })
Comment 41 Radek Šiman (R-Bit Technology, s.r.o.) 2017-10-11 23:08:05 UTC
Created attachment 67982 [details] [review]
Bug 17509: (followup) Logic moved from C4 to Koha namespace

This patch simplifies logic by using Koha objects. Automated test has
been moved from t/db_dependent/Reserves to t/db_dependent/Holds.

Test plan:
1) apply patch
2) run database update
3) enable sending an email to the patrons to return an item whenever a
   hold request is placed on it (NotifyToReturnItemFromLibrary)
4) place a hold
5) patrons having checked-out the item on hold are notified by email
   (see HOLDPLACED_CONTACT letter)
Comment 42 Alex Buckley 2017-10-21 06:54:19 UTC
Hi 

Can you please rebase against master, a conflict is thrown when applying these patches on a clean, up to date branch.

Specifically the patch causing the issue is '[SIGNED-OFF] Bug 17509: Added notification letter HOLDPLACED_CONTACT' and the file with the conflict is in installer/data/mysql/uk-UA/mandatory/sample_notices.sql

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 17509: Added NotifyToReturnItemWhenHoldIsPlaced syspref
Applying: Bug 17509: Added system preference to restrict recipients based on library
Applying: Bug 17509: Improved system preferences for item return notification
Applying: Bug 17509: renamed atomic file to suit conventions
Applying: Bug 17509: Removed old name of atomic update file
Applying: Bug 17509: Added notification letter HOLDPLACED_CONTACT
Using index info to reconstruct a base tree...
M	installer/data/mysql/uk-UA/mandatory/sample_notices.sql
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/uk-UA/mandatory/sample_notices.sql
CONFLICT (content): Merge conflict in installer/data/mysql/uk-UA/mandatory/sample_notices.sql
Failed to merge in the changes.
Patch failed at 0001 Bug 17509: Added notification letter HOLDPLACED_CONTACT
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.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/SIGNED-OFF-Bug-17509-Added-notification-letter-HOL-iFsWW_.patch
Comment 43 Josef Moravec 2018-10-12 08:26:43 UTC
Created attachment 80452 [details] [review]
Bug 17509: Added NotifyToReturnItemWhenHoldIsPlaced syspref

This system preference will allow to ask patrons to return items
whenever a new hold is placed on them

Test plan:
1) Open System preferences / Circulation / Holds Policy
2) Change value from Enable to Don't enable and vice versa
3) Check NotifyToReturnItemWhenHoldIsPlaced is changed in database

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 44 Josef Moravec 2018-10-12 08:26:49 UTC
Created attachment 80453 [details] [review]
Bug 17509: Added system preference to restrict recipients based on library

Test plan:
1) apply patch
2) go to System preferences / Circulation / Holds Policy
3) change which patrons will be contacted
4) compare your change against database value
NotifyToReturnItemFromLibrary

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 45 Josef Moravec 2018-10-12 08:26:54 UTC
Created attachment 80454 [details] [review]
Bug 17509: Improved system preferences for item return notification

Test plan:
1) apply patch
2) go to System preferences / Circulation / Holds Policy
3) change NotifyToReturn* variables as you wish
4) verify your changes in database

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 46 Josef Moravec 2018-10-12 08:26:59 UTC
Created attachment 80455 [details] [review]
Bug 17509: renamed atomic file to suit conventions

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 47 Josef Moravec 2018-10-12 08:27:04 UTC
Created attachment 80456 [details] [review]
Bug 17509: Removed old name of atomic update file

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 48 Josef Moravec 2018-10-12 08:27:10 UTC
Created attachment 80457 [details] [review]
Bug 17509: Added notification letter HOLDPLACED_CONTACT

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 49 Josef Moravec 2018-10-12 08:27:15 UTC
Created attachment 80458 [details] [review]
Bug 17509: notify patrons to return an item whenever a hold is placed on it

Test plan:
1) apply patch
2) run database update
3) enable sending an email to the patrons to return an item whenever a
   hold request is placed on it
4) place a hold
5) patrons having checked-out the item on hold are notified by email
   (see HOLDPLACED_CONTACT letter)

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 50 Josef Moravec 2018-10-12 08:27:21 UTC
Created attachment 80459 [details] [review]
Bug 17509: Fixed invalid patterns

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 51 Josef Moravec 2018-10-12 08:27:26 UTC
Created attachment 80460 [details] [review]
Bug 17509: Added tests

prove t/db_dependent/Reserves/Notification.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Test pass
Comment 52 Josef Moravec 2018-10-12 08:27:31 UTC
Created attachment 80461 [details] [review]
Bug 17509: (follow-up) Logic moved from C4 to Koha namespace

This patch simplifies logic by using Koha objects. Automated test has
been moved from t/db_dependent/Reserves to t/db_dependent/Holds.

Test plan:
1) apply patch
2) run database update
3) enable sending an email to the patrons to return an item whenever a
   hold request is placed on it (NotifyToReturnItemFromLibrary)
4) place a hold
5) patrons having checked-out the item on hold are notified by email
   (see HOLDPLACED_CONTACT letter)

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 53 Josef Moravec 2018-10-12 08:28:43 UTC
Patchset rebased on master, still works as advertised.

Setting back to signed off
Comment 54 Jonathan Druart 2018-10-16 19:05:51 UTC
Quick code review:

1. 
+            my $library = Koha::Libraries->find($borrower->branchcode)->unblessed;

can be replaced by $patron->library

2. "lang" param must be passed to GetPreparedLetter to make the notice translatable

3. Use "patron" instead of "borrower", especially for Koha::

4. +                        to_address             => $borrower->email,
To not enqueue with to_address, it must depend on AutoEmailPrimaryAddress. It is correctly handled when sent.

5. We will need native English speaker for "NotifyToReturnItemFromLibrary" (name, description, options)

6. Code in borrowers_to_satisfy could be simplify (no blocker, I can have a look later)

7. New template notices must use the TT syntax (I would recommend to provide the notice for 'en' only and wait for the QA stamp to provide the changes to other files)

8. Changes to AddReserve is not covered by tests. Maybe it should be moved to Koha::Hold?
Comment 55 Josef Moravec 2019-03-08 10:12:34 UTC
Created attachment 86365 [details] [review]
Bug 17509: Added NotifyToReturnItemWhenHoldIsPlaced syspref

This system preference will allow to ask patrons to return items
whenever a new hold is placed on them

Test plan:
1) Open System preferences / Circulation / Holds Policy
2) Change value from Enable to Don't enable and vice versa
3) Check NotifyToReturnItemWhenHoldIsPlaced is changed in database
4) change which patrons will be contacted
5) compare your change against database value NotifyToReturnItemFromLibrary

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 56 Josef Moravec 2019-03-08 10:12:39 UTC
Created attachment 86366 [details] [review]
Bug 17509: Added notification letter HOLDPLACED_CONTACT

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 57 Josef Moravec 2019-03-08 10:12:44 UTC
Created attachment 86367 [details] [review]
Bug 17509: notify patrons to return an item whenever a hold is placed on it

Test plan:
1) apply patch
2) run database update
3) enable sending an email to the patrons to return an item whenever a
   hold request is placed on it
4) place a hold
5) patrons having checked-out the item on hold are notified by email
   (see HOLDPLACED_CONTACT letter)

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 58 Josef Moravec 2019-03-08 10:12:48 UTC
Created attachment 86368 [details] [review]
Bug 17509: Added tests

prove t/db_dependent/Reserves/Notification.t

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Test pass
Comment 59 Josef Moravec 2019-03-08 10:12:52 UTC
Created attachment 86369 [details] [review]
Bug 17509: (follow-up) Logic moved from C4 to Koha namespace

This patch simplifies logic by using Koha objects. Automated test has
been moved from t/db_dependent/Reserves to t/db_dependent/Holds.

Test plan:
1) apply patch
2) run database update
3) enable sending an email to the patrons to return an item whenever a
   hold request is placed on it (NotifyToReturnItemFromLibrary)
4) place a hold
5) patrons having checked-out the item on hold are notified by email
   (see HOLDPLACED_CONTACT letter)

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 60 Josef Moravec 2019-03-18 10:33:10 UTC
Comment on attachment 86369 [details] [review]
Bug 17509: (follow-up) Logic moved from C4 to Koha namespace

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

::: C4/Reserves.pm
@@ +268,2 @@
>          foreach my $borrower (@borrowers) {
> +            if ( !$borrower->email ) {

should be ->first_valid_email_address

@@ +292,3 @@
>                          message_transport_type => 'email',
>                          from_address           => $admin_email_address,
> +                        to_address             => $borrower->email,

should be ->first_valid_email_address

::: Koha/Hold.pm
@@ +330,5 @@
> +Returns Koha::Patron objects able to return item to satisfy this Hold
> +
> +=cut
> +
> +sub borrowers_to_satisfy {

Should be called "patrons_..."
Comment 61 Katrin Fischer 2019-04-03 21:46:57 UTC
> > +sub borrowers_to_satisfy {
> 
> Should be called "patrons_..."

Maybe the naming should be changed to include something about holds?
Comment 62 Josef Moravec 2019-04-04 09:52:20 UTC
(In reply to Katrin Fischer from comment #61)
> > > +sub borrowers_to_satisfy {
> > 
> > Should be called "patrons_..."
> 
> Maybe the naming should be changed to include something about holds?

I agree
Comment 63 Aleisha Amohia 2022-06-09 20:55:56 UTC
Is this still relevant/needed now that recalls has been pushed?
Comment 64 Katrin Fischer 2022-06-10 05:58:30 UTC
I think it's a different feature and would still be nice to have. With recalls you _have_ to return the item, this is more a 'nicely asking'.
Comment 65 Michal Denar 2022-06-10 06:19:04 UTC
Hi, Aleisha and Katrin,
The feature of alerting the reader that there is a reservation for a borrowed book is great. However, in practice, we have run into one detail that readers perceive negatively. If an email arrives every time someone makes a hold on a title/unit, that can be a lot of repeated notifications in some cases. Would it perhaps only send one, the first time a hold is made?
Comment 66 Katrin Fischer 2022-06-10 06:21:38 UTC
Hi Michal,

you make a good point. Just noting that neither Aleisha nor I are developers of this patch and it could be an abandoned development.
Comment 67 Michal Denar 2022-06-10 06:39:05 UTC
I asked Radek to comment on this patch.
Comment 68 Radek Šiman (R-Bit Technology, s.r.o.) 2022-06-10 10:30:43 UTC
(In reply to Michal Denar from comment #67)
> I asked Radek to comment on this patch.

Hi girls and boys, the libraries we support like this feature, but until it notifies the user during the loan only once, it's kind of unusable. 

Thinking about use cases maybe I've found a way to solve this issue. We could try to find the last HOLDPLACED_CONTACT message's time_queued in the message_queue table between issuedate and recently created hold's reservedate. If there is a message, we shouldn't send another one. In the case of no results, new HOLDPLACED_CONTACT message will be queued.

Is is good or bad idea?
Comment 69 Katrin Fischer 2023-07-05 13:44:47 UTC
(In reply to Radek Šiman (R-Bit Technology, s.r.o.) from comment #68)
> (In reply to Michal Denar from comment #67)
> > I asked Radek to comment on this patch.
> 
> Hi girls and boys, the libraries we support like this feature, but until it
> notifies the user during the loan only once, it's kind of unusable. 
> 
> Thinking about use cases maybe I've found a way to solve this issue. We
> could try to find the last HOLDPLACED_CONTACT message's time_queued in the
> message_queue table between issuedate and recently created hold's
> reservedate. If there is a message, we shouldn't send another one. In the
> case of no results, new HOLDPLACED_CONTACT message will be queued.
> 
> Is is good or bad idea?

Sorry for not replying earlier to this. We just had another library asking about this feature.

I think you are right that we don't want to email for every hold placed. If it's a high demand item, that could cause quite a bit of trouble.

Maybe a different idea: What if we create a flag in the issues table that is set to the date the last HOLDPLACED_CONTACT was sent. This way we could also enhance the feature in the future to say another reminder is ok after X days or similar.

And we don't have to rely on the message_queue, that might get cleaned or be otherwise unreliable.

Another question would be to whom we send the email if there are multiple checked out copies. I'd say if it's an item level hold, we should only email the issuer of this item (thinking of items for serial issues etc) and if it's a record level hold we email all patrons that have items checked out? An alternative could be to email up to x users that had the item for the longest time at this point.