Hello :) I purpose to add an email notification to patron when a booking is cancelled.
Created attachment 167010 [details] [review] Bug 36915: Send email notification when a booking is cancelled When a booking is cancelled, patron received an email based on specific letter. Test plan: 1) Create a letter with code "BOOKING_CANCELLATION" on "Notices and slips" tool. 2) Add booking or go on item already booked in advance. 3) Cancel it 4) Verify in message_queue table directly or go on patron page and click on "Notices" tab section. Sponsored by: Association de Gestion des Œuvres Sociales d'Inria (AGOS)
Shouldn't this be in $booking->delete instead?
Created attachment 167013 [details] [review] Bug 36915: Send email notification when a booking is cancelled When a booking is cancelled, patron received an email based on specific letter. Test plan: 1) Create a letter with code "BOOKING_CANCELLATION" on "Notices and slips" tool. 2) Add booking or go on item already booked in advance. 3) Cancel it 4) Verify in message_queue table directly or go on patron page and click on "Notices" tab section. Sponsored by: Association de Gestion des Œuvres Sociales d'Inria (AGOS)
(In reply to Jonathan Druart from comment #2) > Shouldn't this be in $booking->delete instead? Thanks for your feedback, you're right.. I was wrong :)
And please provide tests ;)
I like this, and it was indeed on my enhancements list ;) Couple of comments, however.. 1) Could we pass the bookings object into GetPreparedLetter instead of passing various individual pieces. (Bug 19966 adds this functionality to GetPreperaredLetter, you'd want to pass the object and then call delete at the end of the method after GetPreparredLetter has returned) 2) Could we add a sample notice template too.
Created attachment 168149 [details] [review] Bug 36915: Send email notification when a booking is cancelled When a booking is cancelled, patron received an email based on specific letter. Test plan: 1) Create a letter with code "BOOKING_CANCELLATION" on "Notices and slips" tool. 2) Add booking or go on item already booked in advance. 3) Cancel it 4) Verify in message_queue table directly or go on patron page and click on "Notices" tab section. Sponsored by: Association de Gestion des Œuvres Sociales d'Inria (AGOS) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 168150 [details] [review] Bug 36915: (follow-up) Pass object to GetPreparedLetter This patch updates the logic to pass the object to be deleted through to GetPreparedLetter. This way, as we add fields and accessors to the object in the future, they will automatically become available to the notices.
I added the suggested follow-up to utilise the 'objects' option in GetPreparedLetter. Still needs unit tests.. I'll try to take a look at adding those when I can. I'm also working on a default notice template to ship. I do wonder if we really want to tie this to 'delete'.. I'd be tempted to introduce a 'cancel' method to be more explicit. Then if we later introduce database cleanup or other workflows that include deletions we can call the standard delete method without having to work around the cancellation message handling.
Created attachment 168155 [details] [review] Bug 36915: (follow-up) Basic sample notice
I've Failed QA because when you preview the notice, there is a huge amount of white space before the actual message. I'm not sure whether this is because of the sample notice, or something else. Either way, I don't know how to fix this (otherwise I would have added a follow-up). Suggestion: - Change the title of the notice to "A library booking was cancelled" - In the notice change "You library" to "Your library" Do we do database updates for new sample notices? Testing notes (using KTD): 1. Apply the patches. 2. Reset everything (so the sample notice is available): reset_all 3. Add an email address to the patron you will make the booking for (for example, Mary Burton, mary@example.com) 4. Add a booking: . Find a record . Go to Items > Priority . Change "Bookable" to "Yes" then click "Update" . Use the "Place booking" action for the record . Enter the details to create a booking (using the patron you added an email address to) 5. Cancel a booking: . Go to the record . Select the Bookings section . Click "Cancel" for the booking you made in step 4 . Click "Confirm" in the pop-up window to cancel the booking 6. Check that a notice was generated: . Go to the notices section for the patron OR . Check the message queue in the database: - koha-mysql kohadev - select * from message_queue;
Speaking to ByWater and their partners using Bookings, they would indeed want to keep the booking around for reports down the line. I think we should add a 'status' field to bookings to track that they have been placed, cancelled, fulfilled and add a 'cancel' method to set it with an optional 'delete' that can be passed would you want to remove the booking rather than just set to cancelled. I'll look into the notice issue highlighted by David too.
Created attachment 170058 [details] [review] Bug 36915: (follow-up) Make basic sample notice html and add breaks
Making the sample notice html by setting is_html to true fixes the whitespace problem. I also added David's suggestions to the sample notice while I was at it.
Created attachment 170077 [details] [review] Bug 36915: Send email notification when a booking is cancelled When a booking is cancelled, patron received an email based on specific letter. Test plan: 1) Create a letter with code "BOOKING_CANCELLATION" on "Notices and slips" tool. 2) Add booking or go on item already booked in advance. 3) Cancel it 4) Verify in message_queue table directly or go on patron page and click on "Notices" tab section. Sponsored by: Association de Gestion des Œuvres Sociales d'Inria (AGOS) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com>
Created attachment 170078 [details] [review] Bug 36915: (follow-up) Pass object to GetPreparedLetter This patch updates the logic to pass the object to be deleted through to GetPreparedLetter. This way, as we add fields and accessors to the object in the future, they will automatically become available to the notices. Signed-off-by: David Nind <david@davidnind.com>
Created attachment 170079 [details] [review] Bug 36915: (follow-up) Basic sample notice Signed-off-by: David Nind <david@davidnind.com>
Created attachment 170080 [details] [review] Bug 36915: (follow-up) Make basic sample notice html and add breaks Signed-off-by: David Nind <david@davidnind.com>
Blockers: Unit tests still needed Questions: Should this notice always be sent? Hold cancellations have a 'reason' and nothing is sent when the reason is skipped. I think we should have something similar here - suppose the library just wants to move the booking to a new edition etc. Should there be a messaging preference? Patrons may not want to receive these notices, are they mandatory? Suggestion: "Your library" in notice is somewhat unclear - is this notice coming from the library who owns the material, or the patron's library? We should use a template "branchname" and display the actual library
Created attachment 170099 [details] [review] Bug 36915: (follow-up) Include branchname in sample notice
That last patch adds the missing branchname of the pickup library to the notice. Other sample notices include a lot of fields of the library object, not sure whether we should do this here as well or the branchname is sufficient.
Created attachment 170155 [details] [review] Bug 36915: (follow-up) Add tests
Questions: > Should this notice always be sent? Hold cancellations have a 'reason' and nothing is sent when the reason is skipped. I think we should have something similar here - suppose the library just wants to move the booking to a new edition etc. I'd like to get this thing through and move the cancellation via a cancel method into two follow-up bugs. 1. Add a status column to the bookings table. 2. Move the the notification from delete into cancel and also change the status to 'cancelled' (Child of 1). > Should there be a messaging preference? Patrons may not want to receive these notices, are they mandatory? This should probably also be it's own bug (Child or 2).
Created attachment 170203 [details] [review] Bug 36915: Send email notification when a booking is cancelled When a booking is cancelled, patron received an email based on specific letter. Test plan: 1) Create a letter with code "BOOKING_CANCELLATION" on "Notices and slips" tool. 2) Add booking or go on item already booked in advance. 3) Cancel it 4) Verify in message_queue table directly or go on patron page and click on "Notices" tab section. Sponsored by: Association de Gestion des Œuvres Sociales d'Inria (AGOS) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 170204 [details] [review] Bug 36915: (follow-up) Pass object to GetPreparedLetter This patch updates the logic to pass the object to be deleted through to GetPreparedLetter. This way, as we add fields and accessors to the object in the future, they will automatically become available to the notices. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 170205 [details] [review] Bug 36915: (follow-up) Basic sample notice Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 170206 [details] [review] Bug 36915: (follow-up) Make basic sample notice html and add breaks Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 170207 [details] [review] Bug 36915: (follow-up) Include branchname in sample notice Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 170208 [details] [review] Bug 36915: (follow-up) Add tests Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Created attachment 170385 [details] [review] Bug 36915: Send email notification when a booking is cancelled When a booking is cancelled, patron received an email based on specific letter. Test plan: 1) Create a letter with code "BOOKING_CANCELLATION" on "Notices and slips" tool. 2) Add booking or go on item already booked in advance. 3) Cancel it 4) Verify in message_queue table directly or go on patron page and click on "Notices" tab section. Sponsored by: Association de Gestion des Œuvres Sociales d'Inria (AGOS) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 170386 [details] [review] Bug 36915: (follow-up) Pass object to GetPreparedLetter This patch updates the logic to pass the object to be deleted through to GetPreparedLetter. This way, as we add fields and accessors to the object in the future, they will automatically become available to the notices. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 170387 [details] [review] Bug 36915: (follow-up) Basic sample notice Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 170388 [details] [review] Bug 36915: (follow-up) Make basic sample notice html and add breaks Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 170389 [details] [review] Bug 36915: (follow-up) Include branchname in sample notice Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 170390 [details] [review] Bug 36915: (follow-up) Add tests Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 170391 [details] [review] Bug 36915: (QA follow-up) Passing less to encourage traversal I generally push for contextual use of accessors in TT notices and try to reduce extra lookups. In this case we can access the pickup library branchname directly using the pickup_library relation available to us in the booking object we pass. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
More great teamwork and it's great to see bookings edging forward now. All works really nicely now, no regressions and well tested. Passing QA
Are there any plans to make this a patron messaging preference? It seems at the moment this feature can only be turned on/off by adding or deleting the notice.
Yes, there are plans, but we wanted to move the control aspect into follow-up bugs.
(In reply to Paul Derscheid from comment #39) > Yes, there are plans, but we wanted to move the control aspect into > follow-up bugs. Is it an option to have it for this release? I think than we could also do a database update to add the notice by default, as we'd not turn it on for the patrons automatically. Makes it cleaner. Also update path: If we introduce the messaging preference later, we will need to deal with creating "checkboxes" if the notice exists, assuming the library is using it not to deactivate it by accident. Also checking where the notice exists and matching that to the home libraries of the patrons... messier to do later.
Pushed for 24.11! Well done everyone, thank you!
(In reply to Katrin Fischer from comment #40) > (In reply to Paul Derscheid from comment #39) > > Yes, there are plans, but we wanted to move the control aspect into > > follow-up bugs. > > Is it an option to have it for this release? > > I think than we could also do a database update to add the notice by > default, as we'd not turn it on for the patrons automatically. Makes it > cleaner. > > Also update path: > > If we introduce the messaging preference later, we will need to deal with > creating "checkboxes" if the notice exists, assuming the library is using it > not to deactivate it by accident. Also checking where the notice exists and > matching that to the home libraries of the patrons... messier to do later. Never got a reply :( This will make an update later on harder.