Bug 11867 - Multi transport types: Manage *_PHONE notices
Summary: Multi transport types: Manage *_PHONE notices
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 9016 10845
Blocks: 12752 16217
  Show dependency treegraph
 
Reported: 2014-02-27 14:02 UTC by Jonathan Druart
Modified: 2016-04-06 16:01 UTC (History)
5 users (show)

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


Attachments
Bug 11867: MTT: Manage *_PHONE notices (5.31 KB, patch)
2014-02-27 14:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11867: MTT: Manage *_PHONE notices (5.24 KB, patch)
2014-03-03 11:48 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 11867: MTT: Manage *_PHONE notices (5.48 KB, patch)
2014-04-30 16:00 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 11867: FIX message_transport_type parameter for Itiva cronjob (1.16 KB, patch)
2014-05-01 13:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11867: MTT: Manage *_PHONE notices (5.17 KB, patch)
2014-05-01 13:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11867: MTT: Manage *_PHONE notices (5.19 KB, patch)
2014-05-01 13:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 11867: Change the sql query in order to win beauty prices (1.87 KB, patch)
2014-05-02 15:08 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 Jonathan Druart 2014-02-27 14:02:11 UTC
The *_PHONE notices (HOLD_PHONE, PREDUE_PHONE and OVERDUE_PHONE) should be "merged" into the main code (i.e. HOLD, PREDUE and OVERDUE).
Comment 1 Jonathan Druart 2014-02-27 14:09:26 UTC Comment hidden (obsolete)
Comment 2 Olli-Antti Kivilahti 2014-03-03 11:00:24 UTC
-- Merge issues:   --
---------------------
git pull
git bz apply [9016 10832 10833 10845 11208]
So far so good!

So then we get the merge error :(
http://pastebin.com/PrxXt4t4
Comment 3 Jonathan Druart 2014-03-03 11:48:22 UTC Comment hidden (obsolete)
Comment 4 Olli-Antti Kivilahti 2014-03-03 13:30:35 UTC
Merging ok! Maybe someone with TalkingTechItiva in use might be better suited to sign this task off.
Comment 5 Katrin Fischer 2014-04-29 07:50:12 UTC
Can we get a sign off on this please?
Comment 6 Bernardo Gonzalez Kriegel 2014-04-29 20:11:56 UTC
Problems to test: Bug 9016 gives merge errors, 
also on updatedatbase.pl (this patch),
don't we need
if ( CheckVersion($DBversion) ) {
instead of
if ( C4::Context->preference("Version") < TransformToNum($DBversion) ) {
?
Comment 7 Jonathan Druart 2014-04-30 07:29:43 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #6)
Bug 9016 rebased against master.
This patch applies cleanly on top of bug 9016 and bug 10845.
Comment 8 Bernardo Gonzalez Kriegel 2014-04-30 16:00:17 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2014-05-01 13:31:25 UTC
I'm seeing this code in the Itiva cronjob:

        my $letter = C4::Letters::GetPreparedLetter(
            module      => $module,
            letter_code => $code,
            tables      => {
                borrowers   => $issues->{'borrowernumber'},
                biblio      => $issues->{'biblionumber'},
                biblioitems => $issues->{'biblionumber'},
                message_transport_type => 'phone',
            },
            message_transport_type => 'phone',
        );

The duplicated mtt seems odd to me. Is that correct?
Comment 10 Jonathan Druart 2014-05-01 13:39:29 UTC
(In reply to M. de Rooy from comment #9)
> The duplicated mtt seems odd to me. Is that correct?

No, it seems that the one introduced by bug 10845 is wrong. I will provide follow-ups.
Comment 11 Marcel de Rooy 2014-05-01 13:42:03 UTC
Please add the bug number too in the print message of updatedatabase
Comment 12 Jonathan Druart 2014-05-01 13:43:20 UTC Comment hidden (obsolete)
Comment 13 Jonathan Druart 2014-05-01 13:46:18 UTC Comment hidden (obsolete)
Comment 14 Jonathan Druart 2014-05-01 13:46:51 UTC
Patch rebased on top of bug 9016 and bug 10845.
Comment 15 Jonathan Druart 2014-05-01 13:49:33 UTC
Created attachment 27876 [details] [review]
Bug 11867: MTT: Manage *_PHONE notices

The *_PHONE notices (HOLD_PHONE, PREDUE_PHONE and OVERDUE_PHONE) should
be "merged" into the main code (i.e. HOLD, PREDUE and OVERDUE).

Test plan:
1/ Make sure you have HOLD_PHONE, PREDUE_PHONE and OVERDUE_PHONE notices
2/ Execute the update DB entry
3/ Verify the 3 notices have been merged into "phone" template of the
HOLD, PREDUE and OVERDUE notices
4/ Verify there is no regression in the Talking Tech feature (how?)

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
No koha-qa errors

Verified that notices are merged
TalkingTech_itiva_outbound.pl runs without problem... but can't produce
any output, may be not correctly configured (my setup), no warnings
nor log messages
Comment 16 Jonathan Druart 2014-05-01 13:50:06 UTC
Last patch adds the bug number in the updatedb entry.
Comment 17 Marcel de Rooy 2014-05-01 17:56:51 UTC
The sql construct
name=(SELECT name FROM ( SELECT name FROM letter  WHERE code= ... LIMIT 1) AS t)
does probably not win beauty prices :)

You could try something with joining letter in the update statement. But on the other hand why would you? You do not need to change the name for the Phone notices. That name should be fine. Note that the SELECT .. LIMIT 1 will not always provide the best choice; you are not even sure which one he picks.

Is it an idea to remove name from the UPDATE?
Comment 18 Jonathan Druart 2014-05-02 15:08:12 UTC
Created attachment 27920 [details] [review]
Bug 11867: Change the sql query in order to win beauty prices
Comment 19 Jonathan Druart 2014-05-02 15:08:59 UTC
(In reply to M. de Rooy from comment #17)
> The sql construct
> name=(SELECT name FROM ( SELECT name FROM letter  WHERE code= ... LIMIT 1)
> AS t)
> does probably not win beauty prices :)

Erk

> Is it an idea to remove name from the UPDATE?

The name should be the same for all letters with the same code.
Comment 20 Galen Charlton 2014-05-02 19:25:22 UTC
I will also be including this in part of the overall MTT push to master.
Comment 21 Galen Charlton 2014-05-02 21:21:37 UTC
Pushed to master.  Thanks, Jonathan!