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 (vote)
Assignee: Radek Šiman (R-Bit Technology, s.r.o.)
QA Contact: Testopia
URL:
Whiteboard:
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: 2018-10-16 19:05 UTC (History)
11 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


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

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.genty02 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?