Description
Radek Šiman (R-Bit Technology, s.r.o.)
2016-10-27 10:29:58 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 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 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 Created attachment 57146 [details] [review] Bug 17509: renamed atomic file to suit conventions Created attachment 57147 [details] [review] Bug 17509: Removed old name of atomic update file Created attachment 57148 [details] [review] Bug 17509: Added notification letter HOLDPLACED_CONTACT 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) Created attachment 57150 [details] [review] Bug 17509: Fixed invalid patterns 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> 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> 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> Created attachment 57161 [details] [review] [SIGNED-OFF] Bug 17509: renamed atomic file to suit conventions Signed-off-by: Josef Moravec <josef.moravec@gmail.com> 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> Created attachment 57163 [details] [review] [SIGNED-OFF] Bug 17509: Added notification letter HOLDPLACED_CONTACT Signed-off-by: Josef Moravec <josef.moravec@gmail.com> 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> Created attachment 57165 [details] [review] [SIGNED-OFF] Bug 17509: Fixed invalid patterns Signed-off-by: Josef Moravec <josef.moravec@gmail.com> 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. 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 (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. *** Bug 13846 has been marked as a duplicate of this bug. *** Hi, We are really interested in this new feature. Is someone still working on it ? 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. Created attachment 63258 [details] [review] Bug 17509: Added tests prove t/db_dependent/Reserves/Notification.t 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> 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> 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> Created attachment 63272 [details] [review] [SIGNED-OFF] Bug 17509: renamed atomic file to suit conventions Signed-off-by: Josef Moravec <josef.moravec@gmail.com> 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> Created attachment 63274 [details] [review] [SIGNED-OFF] Bug 17509: Added notification letter HOLDPLACED_CONTACT Signed-off-by: Josef Moravec <josef.moravec@gmail.com> 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> Created attachment 63276 [details] [review] [SIGNED-OFF] Bug 17509: Fixed invalid patterns Signed-off-by: Josef Moravec <josef.moravec@gmail.com> 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 I tested the last patch and rebased the whole patchset on master. 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. 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. This feature is meant exactly as Katrin said: it kindly asks the user to return the item ASAP only. Let's start here with recalls, and build the other stuff in. I'd like to see this patch to be published as is. The other stuff should be developed in a separately filed bug. That's what I mean, let's get this in and build the rest of recalls off of it somewhere else. 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'} }) 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) 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 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> 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> 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> Created attachment 80455 [details] [review] Bug 17509: renamed atomic file to suit conventions Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 80456 [details] [review] Bug 17509: Removed old name of atomic update file Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Created attachment 80457 [details] [review] Bug 17509: Added notification letter HOLDPLACED_CONTACT Signed-off-by: Josef Moravec <josef.moravec@gmail.com> 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> Created attachment 80459 [details] [review] Bug 17509: Fixed invalid patterns Signed-off-by: Josef Moravec <josef.moravec@gmail.com> 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 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> Patchset rebased on master, still works as advertised. Setting back to signed off 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? 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> Created attachment 86366 [details] [review] Bug 17509: Added notification letter HOLDPLACED_CONTACT Signed-off-by: Josef Moravec <josef.moravec@gmail.com> 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> 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 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 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_..."
> > +sub borrowers_to_satisfy {
>
> Should be called "patrons_..."
Maybe the naming should be changed to include something about holds?
(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 Is this still relevant/needed now that recalls has been pushed? 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'. 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? 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. I asked Radek to comment on this patch. (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? (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. |