Bug 12344 - Multi transport types for issues and checkins
Summary: Multi transport types for issues and checkins
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Notices (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Benjamin Rokseth
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-06-02 11:30 UTC by Benjamin Rokseth
Modified: 2015-12-03 22:02 UTC (History)
4 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
This small patch corrects the order of generating notices for issues and returns (checkout/checkin) so that borrower's notices are rendered correctly (for sms,email,etc.) (3.71 KB, patch)
2014-06-02 12:37 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 12344 - Multi transport types for issues and checkins (3.76 KB, patch)
2014-06-02 12:38 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 12344 - Multi transport types for issues and checkins (3.82 KB, patch)
2015-03-03 14:28 UTC, Sophie MEYNIEUX
Details | Diff | Splinter Review
Bug 12344 - Multi transport types for issues and checkins (3.83 KB, patch)
2015-03-05 11:34 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
QA followup: skip message type if template is missing (1.27 KB, patch)
2015-03-05 11:37 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 12344 - Multi transport types for issues and checkins (3.83 KB, patch)
2015-03-05 12:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 12344: QA followup: use next instead of return (1.31 KB, patch)
2015-03-05 12:48 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Rokseth 2014-06-02 11:30:44 UTC
This bug relates to bug #9016 - multi transport types already pushed to master (3.16).

If a patron has enabled notice for checkin/checkout other than email, the message transport type (e.g. sms) is ignored in the notice generation.

The affected method is C4::Circulation::SendCirculationAlert

Generation of notice (C4::Letters::GetPreparedLetter) is done before the check for borrower's chosen transport types
Comment 1 Benjamin Rokseth 2014-06-02 12:37:02 UTC Comment hidden (obsolete)
Comment 2 Benjamin Rokseth 2014-06-02 12:38:39 UTC Comment hidden (obsolete)
Comment 3 Sophie MEYNIEUX 2015-03-03 14:28:58 UTC Comment hidden (obsolete)
Comment 4 Sophie MEYNIEUX 2015-03-03 14:31:14 UTC
Works as described :
- without patch, letter model used is the one for email
- with patch, letter model used is the one for sms
Comment 5 Jonathan Druart 2015-03-04 15:46:33 UTC
I think I found an issue, to reproduce:
Enable sms + email for checkout, check an item out => no letter is generated.

Ok it's a configuration problem, but I think it could easily be fixed if you next instead of return if the template does not exist.
Comment 6 Benjamin Rokseth 2015-03-05 11:06:37 UTC
good catch, Jonathan! If a patron enables multiple message types, and a message type is missing a template, it would return before processing the remaining types.

A simple fix, as suggested, is to use next instead of return
Comment 7 Benjamin Rokseth 2015-03-05 11:34:56 UTC Comment hidden (obsolete)
Comment 8 Benjamin Rokseth 2015-03-05 11:37:15 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-03-05 12:48:28 UTC
Created attachment 36576 [details] [review]
Bug 12344 - Multi transport types for issues and checkins

This small patch corrects the order of generating notices for issues and returns (checkout/checkin) so that borrower's notices are rendered correctly (for sms,email,etc.)

Test plan:

1) Edit SMSSendDriver syspref to use driver 'Test'
2) Edit CHECKOUT template for sms to 'SMS test'
3) select SMS for test patron's messaging prefs for item checkout
4) checkout an item
5) check the table message_queue, verify that template sms is
   not used (message content is not 'SMS test')

6) apply patch, make new checkout
7) check that message_queue table now has a correctly generated
   notice with 'SMS test'

For a real world test use a real SMS::Send driver and run the
cronjob process-message-queue.pl to send messages immediately.

Signed-off-by: Sophie Meynieux <sophie.meynieux@biblibre.com>

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 10 Jonathan Druart 2015-03-05 12:48:37 UTC
Created attachment 36577 [details] [review]
Bug 12344: QA followup: use next instead of return

Use next instead of return when generating templates.
In case patron has enabled a message type that misses a template,
next message type will be attempted instead of returning at once.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Comment 11 Tomás Cohen Arazi 2015-03-30 16:49:37 UTC
Patches pushed to master.

Thanks Benjamin!
Comment 12 Chris Cormack 2015-03-31 05:31:45 UTC
Pushed to 3.18.x will be in 3.18.6

Just a note Benjamin, you might want to fix your git.author and git.email settings in your vagrant instance.

Ive fixed the author in the 3.18.x branch, but its still wrong in master.
Comment 13 Benjamin Rokseth 2015-04-08 12:49:36 UTC
Ah, I see. It was an old vagrant instance used on the first patch. How do you suggest I change? New patch and obsolete?