Bug 9016 - Multi transport types for notices
Summary: Multi transport types for notices
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL: http://wiki.koha-community.org/wiki/M...
Keywords:
Depends on:
Blocks: 10832 10833 10845 11208 11867 11868 27534
  Show dependency treegraph
 
Reported: 2012-11-07 08:50 UTC by Jonathan Druart
Modified: 2021-01-27 08:45 UTC (History)
9 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 9016: Multi transport types for notices (69.28 KB, patch)
2012-11-07 08:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: DB changes: new table overduerules_transport_types (5.85 KB, patch)
2013-09-06 09:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Create a message for each transport type. (37.93 KB, patch)
2013-09-06 09:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Define different transport types for overdues (23.22 KB, patch)
2013-09-06 09:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref (732 bytes, patch)
2013-09-09 14:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: DB changes: new table overduerules_transport_types (5.85 KB, patch)
2013-09-18 11:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Create a message for each transport type. (37.93 KB, patch)
2013-09-18 11:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Define different transport types for overdues (23.22 KB, patch)
2013-09-18 11:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref (732 bytes, patch)
2013-09-18 11:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: DB changes: new table overduerules_transport_types (5.85 KB, patch)
2013-09-30 07:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Create a message for each transport type. (37.93 KB, patch)
2013-09-30 07:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Define different transport types for overdues (23.22 KB, patch)
2013-09-30 07:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref (732 bytes, patch)
2013-09-30 07:46 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Fix unit tests (2.11 KB, patch)
2013-11-08 12:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: DB changes: new table overduerules_transport_types (5.25 KB, patch)
2013-12-03 17:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Fix unit tests (2.38 KB, patch)
2013-12-03 17:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9016: Create a message for each transport type. (38.01 KB, patch)
2013-12-04 13:39 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9016: Define different transport types for overdues (23.29 KB, patch)
2013-12-04 13:39 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref (807 bytes, patch)
2013-12-04 13:39 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9016: DB changes: new table overduerules_transport_types (5.33 KB, patch)
2013-12-04 13:39 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9016: Fix unit tests (2.45 KB, patch)
2013-12-04 13:39 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 9016: QA Followup (3.27 KB, patch)
2013-12-16 08:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Create a message for each transport type. (38.24 KB, patch)
2014-01-06 15:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Define different transport types for overdues (23.28 KB, patch)
2014-01-06 15:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref (798 bytes, patch)
2014-01-06 15:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: DB changes: new table overduerules_transport_types (5.32 KB, patch)
2014-01-06 15:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Fix unit tests (2.44 KB, patch)
2014-01-06 15:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: QA Followup (3.27 KB, patch)
2014-01-06 15:25 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Create a message for each transport type. (38.24 KB, patch)
2014-01-20 10:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Define different transport types for overdues (23.28 KB, patch)
2014-01-20 10:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref (798 bytes, patch)
2014-01-20 10:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: DB changes: new table overduerules_transport_types (5.29 KB, patch)
2014-01-20 10:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Fix unit tests (2.44 KB, patch)
2014-01-20 10:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: QA Followup (3.27 KB, patch)
2014-01-20 10:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Fix some translate issues (3.35 KB, patch)
2014-02-03 13:17 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Create a message for each transport type. (38.26 KB, patch)
2014-02-07 13:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9016: Define different transport types for overdues (23.28 KB, patch)
2014-02-07 13:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref (815 bytes, patch)
2014-02-07 13:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9016: DB changes: new table overduerules_transport_types (5.36 KB, patch)
2014-02-07 13:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9016: Fix unit tests (2.44 KB, patch)
2014-02-07 13:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9016: QA Followup (3.28 KB, patch)
2014-02-07 13:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9016: Fix some translate issues (3.35 KB, patch)
2014-02-07 13:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9016 [QA Followup] - Add insertAtCaret library to about page (1.21 KB, patch)
2014-02-07 13:56 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9016: Create a message for each transport type. (37.65 KB, patch)
2014-02-19 08:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Define different transport types for overdues (23.28 KB, patch)
2014-02-19 08:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref (798 bytes, patch)
2014-02-19 08:33 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: DB changes: new table overduerules_transport_types (5.36 KB, patch)
2014-02-19 08:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Fix unit tests (2.44 KB, patch)
2014-02-19 08:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: QA Followup (3.29 KB, patch)
2014-02-19 08:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Fix some translate issues (3.35 KB, patch)
2014-02-19 08:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016 [QA Followup] - Add insertAtCaret library to about page (1.19 KB, patch)
2014-02-19 08:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: (follow-up) qa fixes (3.02 KB, patch)
2014-02-25 09:05 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: FIX GetLetters should return all letters (1.65 KB, patch)
2014-03-06 14:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: FIX GetLetters should return all letters (1.97 KB, patch)
2014-03-12 14:35 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 9016: FIX additional mtt's name should be displayed in header (1.10 KB, patch)
2014-03-12 17:47 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: FIX additional mtt's name should be displayed in header (1.17 KB, patch)
2014-03-12 17:49 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 9016: FIX additional mtt's name should be displayed in header (1.17 KB, patch)
2014-03-12 17:54 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 9016: FIX additional mtt's name should be displayed in header (1.17 KB, patch)
2014-03-12 18:04 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 9016: FIX letter name is not displayed on editing a letter (2.39 KB, patch)
2014-03-13 08:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: FIX letter name is not displayed on editing a letter (2.69 KB, patch)
2014-03-13 09:50 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 9016: (followup) FIX letter name is not displayed on editing a letter (986 bytes, patch)
2014-03-27 15:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Add some validation on the letter form (3.75 KB, patch)
2014-03-27 15:09 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Improve number of characters display when mtt=sms (1.91 KB, patch)
2014-03-27 15:19 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Create a message for each transport type. (37.74 KB, patch)
2014-03-28 09:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016: Define different transport types for overdues (23.38 KB, patch)
2014-03-28 09:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref (914 bytes, patch)
2014-03-28 09:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016: DB changes: new table overduerules_transport_types (5.42 KB, patch)
2014-03-28 09:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016: Fix unit tests (2.54 KB, patch)
2014-03-28 09:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016: QA Followup (3.37 KB, patch)
2014-03-28 09:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016: Fix some translate issues (3.44 KB, patch)
2014-03-28 09:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016 [QA Followup] - Add insertAtCaret library to about page (1.30 KB, patch)
2014-03-28 09:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016: (follow-up) qa fixes (3.10 KB, patch)
2014-03-28 09:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016: FIX GetLetters should return all letters (2.07 KB, patch)
2014-03-28 09:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016: FIX additional mtt's name should be displayed in header (1.27 KB, patch)
2014-03-28 09:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016: FIX letter name is not displayed on editing a letter (2.79 KB, patch)
2014-03-28 09:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016: (followup) FIX letter name is not displayed on editing a letter (1.06 KB, patch)
2014-03-28 09:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016: Add some validation on the letter form (3.85 KB, patch)
2014-03-28 09:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016: Improve number of characters display when mtt=sms (2.00 KB, patch)
2014-03-28 09:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016: [QA Follow-up] Remove warning from content.length test (1.06 KB, patch)
2014-03-28 09:11 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 9016: Create a message for each transport type. (37.78 KB, patch)
2014-04-30 07:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Define different transport types for overdues (23.34 KB, patch)
2014-04-30 07:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref (856 bytes, patch)
2014-04-30 07:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: DB changes: new table overduerules_transport_types (5.34 KB, patch)
2014-04-30 07:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Fix unit tests (2.50 KB, patch)
2014-04-30 07:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: QA Followup (3.35 KB, patch)
2014-04-30 07:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Fix some translate issues (3.41 KB, patch)
2014-04-30 07:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016 [QA Followup] - Add insertAtCaret library to about page (1.25 KB, patch)
2014-04-30 07:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: (follow-up) qa fixes (3.04 KB, patch)
2014-04-30 07:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: FIX GetLetters should return all letters (2.03 KB, patch)
2014-04-30 07:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: FIX additional mtt's name should be displayed in header (1.24 KB, patch)
2014-04-30 07:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: FIX letter name is not displayed on editing a letter (2.76 KB, patch)
2014-04-30 07:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: (followup) FIX letter name is not displayed on editing a letter (1.02 KB, patch)
2014-04-30 07:27 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Add some validation on the letter form (3.81 KB, patch)
2014-04-30 07:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: Improve number of characters display when mtt=sms (1.96 KB, patch)
2014-04-30 07:28 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 9016: [QA Follow-up] Remove warning from content.length test (1.02 KB, patch)
2014-04-30 07:28 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 2012-11-07 08:50:27 UTC

    
Comment 1 Jonathan Druart 2012-11-07 08:51:29 UTC Comment hidden (obsolete)
Comment 2 claire.hernandez@biblibre.com 2012-11-07 13:31:03 UTC
We are currently working on this patch, please do not consider this submission for the moment
Comment 3 Jonathan Druart 2013-09-06 09:05:45 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2013-09-06 09:05:51 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2013-09-06 09:05:56 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2013-09-06 09:10:07 UTC
(In reply to Jonathan Druart from comment #1)
> 4/ Now check the features :) The patch allows you to sent letters to the
> queue with an email or sms format.
> So try to create letters with 1 or both of these transport types and
> check the corresponding checkbox on the overdues management page.
> Play with the script misc/cronjobs/overdue_notices.pl in order to see
> yours notices in the message_queue table.

Will be done on bug 10832.
Comment 7 Jonathan Druart 2013-09-09 14:53:34 UTC Comment hidden (obsolete)
Comment 8 I'm just a bot 2013-09-18 10:40:25 UTC
Applying: Bug 9016: DB changes: new table overduerules_transport_types
Using index info to reconstruct a base tree...
M	installer/data/mysql/kohastructure.sql
M	installer/data/mysql/updatedatabase.pl
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Auto-merging installer/data/mysql/kohastructure.sql
Patch failed at 0001 Bug 9016: DB changes: new table overduerules_transport_types
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 9 Jonathan Druart 2013-09-18 11:38:43 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2013-09-18 11:38:47 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2013-09-18 11:38:52 UTC Comment hidden (obsolete)
Comment 12 Jonathan Druart 2013-09-18 11:38:56 UTC Comment hidden (obsolete)
Comment 13 I'm just a bot 2013-09-29 06:00:56 UTC
Applying: Bug 9016: DB changes: new table overduerules_transport_types
Using index info to reconstruct a base tree...
M	installer/data/mysql/updatedatabase.pl
Falling back to patching base and 3-way merge...
Auto-merging installer/data/mysql/updatedatabase.pl
CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl
Patch failed at 0001 Bug 9016: DB changes: new table overduerules_transport_types
The copy of the patch that failed is found in:
   /home/christopher/git/koha/.git/rebase-apply/patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Comment 14 Jonathan Druart 2013-09-30 07:46:08 UTC Comment hidden (obsolete)
Comment 15 Jonathan Druart 2013-09-30 07:46:13 UTC Comment hidden (obsolete)
Comment 16 Jonathan Druart 2013-09-30 07:46:18 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2013-09-30 07:46:23 UTC Comment hidden (obsolete)
Comment 18 I'm just a bot 2013-10-27 06:32:23 UTC
Patch applied cleanly, go forth and signoff
Comment 19 Olli-Antti Kivilahti 2013-11-07 17:21:26 UTC
-----------------
--  Letters.t  --
-----------------

All test complete! Awsum!

------------------
--  Overdues.t  --
------------------
"my" variable $mtts masks earlier declaration in same scope
+++++
line 50: my $mtts = C4::Overdues::GetOverdueMessageTransportTypes('', 'PT', 1);
Should be
line 50: $mtts = C4::Overdues::GetOverdueMessageTransportTypes('', 'PT', 1);



DBD::mysql::db do failed: Cannot add or update a child row: a foreign key constraint fails (`kohadata`.`overduerules_transport_types`, CONSTRAINT `overduerules_fk` FOREIGN KEY (`branchcode`, `categorycode`) REFERENCES `overduerules` (`branchcode`, `categorycode`) ON DELETE CASCADE ON) at /home/kivilahtio/koha/kohaclone/t/db_dependent/Overdues.t line 23.
+++++
koha.overduerules-table is missing the branchcode and patron.categorycode definitions you are trying to link to with 
line 24: INSERT INTO overduerules_transport_types( branchcode, categorycode, letternumber, message_transport_type ) VALUES
    ('CPL', 'PT', 1, 'email'),

After adding a definition via tools/overduerules.pl for branch CPL for patron PT, the given SQL element works (not the whole clause).
Should be included in the test case.


-------------------
--  Overdues.pm  --
-------------------
at GetOverdueMessageTransportTypes()
"return a arrayref with message_transport_type for given branchcode, categorycode and letternumber(1,2 or 3)"
Should be 
"return a arrayref with all message_transport_types for given branchcode, categorycode and letternumber(1,2 or 3)"

Should GetOverdueMessageTransportTypes() also take into account the default values? When library branch specific rules are not set then the function could return the default values instead?


-----------------------------
--  tools/overduerules.pl  --
-----------------------------

Adding definitions seems to work pretty well. Will test more tomorrow. Good work with this one!


Failed QA because Overdues.t needs improvement.
Comment 20 Jonathan Druart 2013-11-08 12:45:07 UTC
(In reply to Olli-Antti Kivilahti from comment #19)
> DBD::mysql::db do failed: Cannot add or update a child row: a foreign key
> constraint fails (`kohadata`.`overduerules_transport_types`, CONSTRAINT
> `overduerules_fk` FOREIGN KEY (`branchcode`, `categorycode`) REFERENCES
> `overduerules` (`branchcode`, `categorycode`) ON DELETE CASCADE ON) at
> /home/kivilahtio/koha/kohaclone/t/db_dependent/Overdues.t line 23.

Yes, I forgot to insert the overdue rules. It will be fixed with a followup.

> at GetOverdueMessageTransportTypes()
> "return a arrayref with message_transport_type for given branchcode,
> categorycode and letternumber(1,2 or 3)"
> Should be 
> "return a arrayref with all message_transport_types for given branchcode,
> categorycode and letternumber(1,2 or 3)"

Will be fixed too.

> Should GetOverdueMessageTransportTypes() also take into account the default
> values? When library branch specific rules are not set then the function
> could return the default values instead?

No I don't think. It is the current behavior: if no specific rule is defined for a library, no data is displayed on the interface when this library is selected.
Comment 21 Jonathan Druart 2013-11-08 12:45:26 UTC Comment hidden (obsolete)
Comment 22 Mathieu Saby 2013-11-29 21:46:45 UTC
Sponsored by Rennes 2
Comment 23 Mathieu Saby 2013-11-30 21:56:42 UTC
Hi

I made a fresh install, create some overdue rules, and apply the patch.

I edited manually updatedatabase, to replace "13.00.XXX" with "15.00.XXX", then I ran updatedatabase.

Some pages appears to be broken:

cgi-bin/koha/tools/letter.pl?op=add_form&branchcode=&module=circulation&code=ODUE
has lost the overdue template (the textarea is void)
and the Koha Module (now "Catalogue" instead of "Circulation")

http://pro.koha-msaby/cgi-bin/koha/tools/overduerules.pl
has lost all the rules I have defined before the patch.
and cannot retrieve any notice template in the dropdown list.

I don't think it is linked with my manual operation on updatedatabase (the changes were made in db structures).

So I must fail QA.

Mathieu
Comment 24 Mathieu Saby 2013-12-01 12:16:58 UTC
Oups, I tested with french translation. I suppose that's why it did not work...
Back to need sign off.

Mathieu
Comment 25 Mathieu Saby 2013-12-01 13:30:54 UTC
I set some overdue books for a patron.
I ran overdue_notices.pl -n
I applied the patch
 ran overdue_notices.pl -n again
I had the same results as before the patch.
=> OK

In the list of notices, I was able to define a new template for ODUE notices sent by SMS.
=> OK

In Overdue rules, I was able to check one or serveral boxes (email/feed/phone/print/sms) for each patron category, and for letters 1,2,3
I submit the changes, and checked it was saved
=> OK

But...
I checked only the box "sms" for all my patron categories, and for the 3 letters.
I ran overdue_notices.pl -n
The results were the same as before : the original notice (made for email) was used, not the new notice I had defined for SMS.
I ran overdue_notices.pl and check message_queue table
The results were the same as before : the original notice (made for email) was used, not the new notice I had defined for SMS.

=> KO
Maybe I'm doing something wrong?

And, additional questions:
Are "sms" and "phone" are supposed to be 2 different message carrier?
And what about "feed" type ? I don't see how overdue messages can be sent by "feeds".
So, if feed and phone type should be suppressed, could you provide an other patch for doing so ? (I don't consider it's part of this bug...).

Failing QA for the "sms" notice template not being used in overdue_notices.pl.
Could you take a look at that?

Mathieu
Comment 26 Mathieu Saby 2013-12-01 13:33:23 UTC
Note : my SMS template was just the text "SMS"
Comment 27 Jonathan Druart 2013-12-02 12:27:04 UTC
In fact the test plan is wrong. The 4th step is not for this patch.
The changes for overdue_notices is made by bug 10832.
Comment 28 Jonathan Druart 2013-12-02 12:28:53 UTC
(In reply to Jonathan Druart from comment #27)
Test plan is right. Please see the test plan from the active patches, not obsolete ones.
Comment 29 Mathieu Saby 2013-12-02 12:32:10 UTC
I'll give a look later, but if the behavior of overdue_notices.pl is changed by an other patch, I suppose I will be able to sign off this one.

Mathieu
Comment 30 Olli-Antti Kivilahti 2013-12-03 15:45:04 UTC
Testing using the latest master

---------------------
+ updatedatabase.pl +
---------------------
Having a problem updating these changes to my DB from updatedatabase.pl 
+$DBversion = "3.13.00.XXX";
+if ( C4::Context->preference("Version") < TransformToNum($DBversion) ) {
Disabling this if-clause enables the DB modifications which work ok.

-------------------
+ perl Overdues.t +
-------------------
ok 1 - use C4::Overdues;
ok 2 - C4::Overdues->can('GetOverdueMessageTransportTypes')
DBD::mysql::db do failed: Duplicate entry 'CPL-PT' for key 'PRIMARY' at Overdues.t line 23.
DBD::mysql::db do failed: Duplicate entry 'CPL-PT' for key 'PRIMARY' at Overdues.t line 23.
# Tests were run but no plan was declared and done_testing() was not seen.
# Looks like your test exited with 255 just after 2.

Caused by me having the records in my db already, problematic since test plan encourages to have existing data in db. A check should be in place to make sure no duplicate entries are INSERTed or maybe use a internal function to update the db?
Otherwise tests run ok.



Sorry for taking so long to get back at this.
So close Joubu! Lemme sign this off!
Comment 31 Jonathan Druart 2013-12-03 17:05:04 UTC Comment hidden (obsolete)
Comment 32 Jonathan Druart 2013-12-03 17:05:34 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2013-12-03 17:07:28 UTC
(In reply to Olli-Antti Kivilahti from comment #30)

> ---------------------
> + updatedatabase.pl +
> ---------------------

Yes, since 3.14 is out, new update db should be marked 3.15.00.XXX

> -------------------
> + perl Overdues.t +
> -------------------
> ok 1 - use C4::Overdues;
> ok 2 - C4::Overdues->can('GetOverdueMessageTransportTypes')
> DBD::mysql::db do failed: Duplicate entry 'CPL-PT' for key 'PRIMARY' at
> Overdues.t line 23.
> DBD::mysql::db do failed: Duplicate entry 'CPL-PT' for key 'PRIMARY' at
> Overdues.t line 23.
> # Tests were run but no plan was declared and done_testing() was not seen.
> # Looks like your test exited with 255 just after 2.

Last patch removes data in the overduerules table too.
Comment 34 Olli-Antti Kivilahti 2013-12-04 13:39:13 UTC Comment hidden (obsolete)
Comment 35 Olli-Antti Kivilahti 2013-12-04 13:39:22 UTC Comment hidden (obsolete)
Comment 36 Olli-Antti Kivilahti 2013-12-04 13:39:28 UTC Comment hidden (obsolete)
Comment 37 Olli-Antti Kivilahti 2013-12-04 13:39:34 UTC Comment hidden (obsolete)
Comment 38 Olli-Antti Kivilahti 2013-12-04 13:39:38 UTC Comment hidden (obsolete)
Comment 39 Olli-Antti Kivilahti 2013-12-04 13:40:23 UTC
Changes ok.
Comment 40 Kyle M Hall 2013-12-13 20:17:16 UTC
Please rebase and address this qa script error:
FAIL   C4/Overdues.pm
   FAIL   pod
                *** ERROR: Spurious text after =cut  in file C4/Overdues.pm
Comment 41 Jonathan Druart 2013-12-16 08:49:35 UTC Comment hidden (obsolete)
Comment 42 Jonathan Druart 2013-12-16 08:51:55 UTC
Kyle,

Patches apply cleanly (only a conflict in updatedb.pl).
You got a conflict because of the previous patches was not obsoleted.
I provided a QA followup for QA issues.

Switch back to Signed Off.
Comment 43 Kyle M Hall 2013-12-20 19:45:34 UTC
Not sure if this is a problem with the patch or just me:

Applying: Bug 9016: QA Followup
fatal: sha1 information is lacking or useless (C4/Letters.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 9016: QA Followup
Comment 44 Jonathan Druart 2013-12-23 10:04:06 UTC
(In reply to Kyle M Hall from comment #43)
> Not sure if this is a problem with the patch or just me:
> 
> Applying: Bug 9016: QA Followup
> fatal: sha1 information is lacking or useless (C4/Letters.pm).
> Repository lacks necessary blobs to fall back on 3-way merge.
> Cannot fall back to three-way merge.
> Patch failed at 0001 Bug 9016: QA Followup

Same as comment 42, against master all patches apply cleanly (except a conflict on updatedb.pl).
Could you retry please?
Comment 45 Olli-Antti Kivilahti 2014-01-03 13:59:59 UTC
Works well here!
Comment 46 Jonathan Druart 2014-01-06 15:24:53 UTC Comment hidden (obsolete)
Comment 47 Jonathan Druart 2014-01-06 15:25:02 UTC Comment hidden (obsolete)
Comment 48 Jonathan Druart 2014-01-06 15:25:08 UTC Comment hidden (obsolete)
Comment 49 Jonathan Druart 2014-01-06 15:25:14 UTC Comment hidden (obsolete)
Comment 50 Jonathan Druart 2014-01-06 15:25:22 UTC Comment hidden (obsolete)
Comment 51 Jonathan Druart 2014-01-06 15:25:28 UTC Comment hidden (obsolete)
Comment 52 Jonathan Druart 2014-01-06 15:26:21 UTC
Last patches fix conflicts with commit b753678c3dff4e0fd2fde008b3c43f0796fcfb05 Bug 11208: regression test for failing SMS messages
Comment 53 Kyle M Hall 2014-01-17 18:34:56 UTC
I'm having issues with the final patch. Is this a problem with the patch or just me?

Applying: Bug 9016: DB changes: new table overduerules_transport_types
Applying: Bug 9016: Fix unit tests
Applying: Bug 9016: QA Followup
fatal: sha1 information is lacking or useless (C4/Letters.pm).
Repository lacks necessary blobs to fall back on 3-way merge.
Cannot fall back to three-way merge.
Patch failed at 0001 Bug 9016: QA Followup
Comment 54 Jonathan Druart 2014-01-20 10:00:29 UTC Comment hidden (obsolete)
Comment 55 Jonathan Druart 2014-01-20 10:00:38 UTC Comment hidden (obsolete)
Comment 56 Jonathan Druart 2014-01-20 10:00:45 UTC Comment hidden (obsolete)
Comment 57 Jonathan Druart 2014-01-20 10:00:51 UTC Comment hidden (obsolete)
Comment 58 Jonathan Druart 2014-01-20 10:00:58 UTC Comment hidden (obsolete)
Comment 59 Jonathan Druart 2014-01-20 10:01:04 UTC Comment hidden (obsolete)
Comment 60 Jonathan Druart 2014-01-20 10:02:07 UTC
(In reply to Kyle M Hall from comment #53)
> I'm having issues with the final patch. Is this a problem with the patch or
> just me?

I don't know, I just got a conflict on updatedb.pl
I rebased patches, maybe it could fix your problem.
Comment 61 Katrin Fischer 2014-02-02 19:07:18 UTC
Starting with a code review on the squashed patch set:

- Adds 2 new routines: 
  * GetMessageTransportTypes
  * GetOverdueMessageTransportTypes
  
- Database changes:
  * Adds a new table overduerules_transport_types
  * Adds a new column to letter - messgage_transport_type, email is default
  * Changes the primary key by adding message_transport_type to it:
      module code branchcode message_transport_type
  * Adds entries to overduerules_transport_types defaulting to email

- New Javascript Library
  TODO: Add to the about page?

- TODO: Translatability - I think the message transport types on the overdue 
  notice triggers page are not translatable:

+              [% FOREACH mtt IN message_transport_types %]
+                <th scpre="col">[% mtt %]</th>

I will continue testing.
Comment 62 Jonathan Druart 2014-02-03 13:17:36 UTC Comment hidden (obsolete)
Comment 63 Jonathan Druart 2014-02-03 13:18:58 UTC
(In reply to Katrin Fischer from comment #61)
> - TODO: Translatability - I think the message transport types on the overdue 
>   notice triggers page are not translatable:
> 
> +              [% FOREACH mtt IN message_transport_types %]
> +                <th scpre="col">[% mtt %]</th>

Last patch fixes that. Thanks for the catch!
Comment 64 Kyle M Hall 2014-02-07 13:56:07 UTC Comment hidden (obsolete)
Comment 65 Kyle M Hall 2014-02-07 13:56:28 UTC Comment hidden (obsolete)
Comment 66 Kyle M Hall 2014-02-07 13:56:32 UTC Comment hidden (obsolete)
Comment 67 Kyle M Hall 2014-02-07 13:56:36 UTC Comment hidden (obsolete)
Comment 68 Kyle M Hall 2014-02-07 13:56:41 UTC Comment hidden (obsolete)
Comment 69 Kyle M Hall 2014-02-07 13:56:45 UTC Comment hidden (obsolete)
Comment 70 Kyle M Hall 2014-02-07 13:56:49 UTC Comment hidden (obsolete)
Comment 71 Kyle M Hall 2014-02-07 13:56:55 UTC Comment hidden (obsolete)
Comment 72 Marcel de Rooy 2014-02-18 16:08:06 UTC
CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt
Comment 73 Jonathan Druart 2014-02-19 08:33:36 UTC Comment hidden (obsolete)
Comment 74 Jonathan Druart 2014-02-19 08:33:46 UTC Comment hidden (obsolete)
Comment 75 Jonathan Druart 2014-02-19 08:33:56 UTC Comment hidden (obsolete)
Comment 76 Jonathan Druart 2014-02-19 08:34:07 UTC Comment hidden (obsolete)
Comment 77 Jonathan Druart 2014-02-19 08:34:13 UTC Comment hidden (obsolete)
Comment 78 Jonathan Druart 2014-02-19 08:34:19 UTC Comment hidden (obsolete)
Comment 79 Jonathan Druart 2014-02-19 08:34:25 UTC Comment hidden (obsolete)
Comment 80 Jonathan Druart 2014-02-19 08:34:31 UTC Comment hidden (obsolete)
Comment 81 Marcel de Rooy 2014-02-19 15:27:21 UTC
Will have a look again. Will probably not finish this today however,,
Comment 82 Marcel de Rooy 2014-02-19 15:39:50 UTC
Olli,
Since this is a larger set of patches, it could be helpful while reviewing it to know what you exactly tested when signing off?
Is it possible to elaborate on that?
Thanks in advance..
Comment 83 Olli-Antti Kivilahti 2014-02-24 09:33:05 UTC
(In reply to M. de Rooy from comment #82)
> Olli,
> Since this is a larger set of patches, it could be helpful while reviewing
> it to know what you exactly tested when signing off?
> Is it possible to elaborate on that?
> Thanks in advance..

Wow, this is a big guestion.

Basically I followed the test plan for this bug and for each dependant bug.
Verifying from the SQL-client that data is preserved and the described database changes happen.
I didn't do any Perl::Critic runs, as I am not very familiar with those and I get pretty uncontrolled results with it :)

We are using this featureset in our semi-production server and have uncovered some bugs and subsequently reported them in other bug entries (which Joubu has promptly fixed).

Unfortunately I didn't keep track of the test paths I took, but I can guarantee I didn't reach a 100% test coverage. Maybe 70% for this patch + the critical path.

Tho I would applaud a unified Koha testing methodology to be adopted.
Comment 84 Marcel de Rooy 2014-02-24 13:23:10 UTC
(In reply to Olli-Antti Kivilahti from comment #83)
Thanks for responding.

> Wow, this is a big guestion.
Really? Everything you could add, is helpful. Perhaps something for a next time: If you include more information in your signoff comment about what you tested especially when testing larger patches, your signoff gets even more weight.

> Basically I followed the test plan for this bug and for each dependant bug.
Always feel free to leave the test plan too. This could reveal the bugs where the author was not thinking of..
Comment 85 Olli-Antti Kivilahti 2014-02-24 13:38:47 UTC
Thanks for the feedback Mr. de Rooy.

From now on I will attach a testing report for all of my sign offs.
Comment 86 Marcel de Rooy 2014-02-24 13:47:11 UTC
(In reply to Olli-Antti Kivilahti from comment #85)
> Thanks for the feedback Mr. de Rooy.
You are welcome. 
> From now on I will attach a testing report for all of my sign offs.
olli++

Marcel
Comment 87 Marcel de Rooy 2014-02-24 14:33:04 UTC
First QA comment:

  * General: Feature looks good (although not complete yet). Seems to be a welcome addition.
    At this moment in the process, it is not clear what is used (probably only email) and what is not. That makes a push not very obvious?

  * Patch 1
  * Functionality: I can imagine that I have filled in multiple transport types but want to activate or deactivate some of them. Would you need some status on each of them? 
  * t/db_dependent/Letters.t passed
    Just noting that the value 'feed' is not added in the test. Seems no problem..
  * POD of GetMessageTransportTypes says: returns a list of hashes?? But it returns an arrayref! (See also Patch 3, yes.)
  * BTW why not use my $mtts = $dbh->selectcol_arrayref.. instead of the more complex map { } .. in GetMessageTransportTypes?
  * Just noting that we still have lots of SQL code in tools/letter.pl. Too bad you didn't move it ;) No blocker for me.
  * Textual: You must specify a title and a content -> Please specify title and content [a content sounds funny to me as non-native speaker]
  * IMPORTANT: The Insert button does no longer surround the fieldname with << and >>. 
    This line (among others) got killed: myQuery.value = chaineSql.substring(0, startPos) +'<<'+ chaineAj+'>>' + chaineSql.substring(endPos, chaineSql.length);
    Note that this does not build up confidence in the test stage. (Did you perhaps repair this on another report already?) 
    When resolving bugs crosses the boundaries of a patch set, it could be hard to perform QA. (I simply cannot qa 30 patches at 8 reports at one time [numbers at random, no offense]..)

  * Patch 2
  * Test t/db_dependent/Overdues.t passed
  * Functionality clash? For Overdue notice (phone notice): I can select all transport types. Confusing..
  * From commit: [Currently, only email, sms and print are relevant.] Note that you could hide what is not relevant now?
  * Glancing through tools/overduerules.pl: If you move First, Second, etc. from template to script, you do not translate them anymore?
    Note that I would not disapprove the idea of more flexibility there..

  * Patch 4
    The dbrev prints: Upgrade done (Bug 9016: Adds the association table overduerules_transport_types)
    The db rev does a lot more than that (or more important things..)
    It also adds e.g. message_transport_type to letter which is closer to the 'general theme'.
    Please change the print message to a more generic one. 
  * Other patches: no specific comments at this time.

Changing status for now.
Comment 88 Jonathan Druart 2014-02-25 09:03:12 UTC
(In reply to M. de Rooy from comment #87)
> First QA comment:

\o/ Thanks Marcel!

>   * General: Feature looks good (although not complete yet). Seems to be a
> welcome addition.
>     At this moment in the process, it is not clear what is used (probably
> only email) and what is not. That makes a push not very obvious?

I tried to explain the complete feature in a previous email to koha-devel: all patches should be reviewed together and pushed together, in order to keep thing consistent (for the review, not the same day :))

>   * Patch 1
>   * Functionality: I can imagine that I have filled in multiple transport
> types but want to activate or deactivate some of them. Would you need some
> status on each of them? 

I am sorry but I don't understand what you mean here.

>   * POD of GetMessageTransportTypes says: returns a list of hashes?? But it
> returns an arrayref! (See also Patch 3, yes.)
>   * BTW why not use my $mtts = $dbh->selectcol_arrayref.. instead of the
> more complex map { } .. in GetMessageTransportTypes?

Yes, will be fixed.

>   * Just noting that we still have lots of SQL code in tools/letter.pl. Too
> bad you didn't move it ;) No blocker for me.

Not too much at the same time :)

>   * Textual: You must specify a title and a content -> Please specify title
> and content [a content sounds funny to me as non-native speaker]
>   * IMPORTANT: The Insert button does no longer surround the fieldname with
> << and >>. 

Good catch, will be fixed!

>     When resolving bugs crosses the boundaries of a patch set, it could be
> hard to perform QA. (I simply cannot qa 30 patches at 8 reports at one time
> [numbers at random, no offense]..)

I tried to fix issues on the same report, all linked reports do something different.
Except if it is a bug on master, I open a new report.

>   * Patch 2
>   * Functionality clash? For Overdue notice (phone notice): I can select all
> transport types. Confusing..

I don't find this notice in sql files. Normally, with this feature, all notices created for a specific transport type, should be moved "into" the "main" notice. For instance, "Overdue notice (phone notice)" should be moved into the "phone" template for the "Overdue" notice.

>   * From commit: [Currently, only email, sms and print are relevant.] Note
> that you could hide what is not relevant now?

Maybe someone else would like to implement phone, etc.
So I did not restrict the mtts to display on the interface.

>   * Glancing through tools/overduerules.pl: If you move First, Second, etc.
> from template to script, you do not translate them anymore?

Yes, they are translatable:
    var tab_map = { "1" : _("First"), "2" : _("Second"), "3" : _("Third")};

>   * Patch 4
>     The dbrev prints: Upgrade done (Bug 9016: Adds the association table
> overduerules_transport_types)
>     The db rev does a lot more than that (or more important things..)
>     It also adds e.g. message_transport_type to letter which is closer to
> the 'general theme'.
>     Please change the print message to a more generic one. 

Will be fixed.
Comment 89 Jonathan Druart 2014-02-25 09:05:16 UTC Comment hidden (obsolete)
Comment 90 Katrin Fischer 2014-02-25 09:11:45 UTC
I think phone notifications are already implemented to some degree at least least for one vendor - bug 4246. I noticed a few phone related notices in one of my installations, but I seem not to have them at all my dev installations?

Found this:
http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=installer/data/mysql/atomicupdate/Bug-4246-Talking-Tech-itiva-phone-notifications.pl;hb=4677bb52616794457f616571479aab6425be75f9
Comment 91 Marcel de Rooy 2014-02-26 12:17:34 UTC
(In reply to Jonathan Druart from comment #88)

Thanks for your quick response and changes.

> >     At this moment in the process, it is not clear what is used (probably
> > only email) and what is not. That makes a push not very obvious?
> I tried to explain the complete feature in a previous email to koha-devel:
> all patches should be reviewed together and pushed together, in order to
> keep thing consistent (for the review, not the same day :))
Sorry, I have seen your email passing by, but it got lost :) Imo it would be easier to have all information on the report. (Perhaps a document attachment explaining some thing or so..)

> >   * Functionality: I can imagine that I have filled in multiple transport
> > types but want to activate or deactivate some of them. Would you need some
> > status on each of them? 
> I am sorry but I don't understand what you mean here.
Now you see the 4 or 5 types (and that could grow..?) But I miss a field like Status that activates or deactivates the transport type for that notice.
We could also use that status somehow to warn people that activating a certain type is not useful or even possible when there is no code to support it yet.

> >   * Functionality clash? For Overdue notice (phone notice): I can select all
> > transport types. Confusing..
> I don't find this notice in sql files. Normally, with this feature, all
> notices created for a specific transport type, should be moved "into" the
> "main" notice. For instance, "Overdue notice (phone notice)" should be moved
> into the "phone" template for the "Overdue" notice.
Yes, that sounds more logical to me. This notice must have come from some earlier test perhaps..

> >   * From commit: [Currently, only email, sms and print are relevant.] Note
> > that you could hide what is not relevant now?
> Maybe someone else would like to implement phone, etc.
> So I did not restrict the mtts to display on the interface.
OK Same comment as above actually: What about not allowing to enable the types that are not yet supported? 

I do not change the status for now. Appreciate feedback on the questions raised.
Comment 92 Jonathan Druart 2014-02-26 13:00:33 UTC
If some transport type are not relevant for someone, he cans completely remove the mtt in the message_transport_types table.

I don't see a proper way to implement what you propose. For instance, on tools/overduerules.pl, all notices and mtt are listed. We could imagine a tricky js stuff for disabling/enabling mtt function of the notice selected. But is it really useful?
Comment 93 Jonathan Druart 2014-02-27 14:11:20 UTC
I submitted 2 new patches on bug 11867 and bug 11868, related to this feature.
Comment 94 Olli-Antti Kivilahti 2014-03-03 10:42:56 UTC
(In reply to Jonathan Druart from comment #92)
> If some transport type are not relevant for someone, he cans completely
> remove the mtt in the message_transport_types table.
> 
> I don't see a proper way to implement what you propose. For instance, on
> tools/overduerules.pl, all notices and mtt are listed. We could imagine a
> tricky js stuff for disabling/enabling mtt function of the notice selected.
> But is it really useful?

Well this feature probably should be documented in the manual, but I don't think a feature is needed to add/remove $mtt's.
If in the manual we had the information just discussed about adding/removing $mtt's via SQL, that would be enough.
Also descriptions about which $mtt's are in use and instructions on writing your own cronjobs to handle them is a good solution. Atleast for us the print notice sending process is pretty complex and making a suitable UI for defining that process in a unified way might be quite difficult.
Comment 95 Jonathan Druart 2014-03-06 14:40:13 UTC Comment hidden (obsolete)
Comment 96 Olli-Antti Kivilahti 2014-03-12 13:54:12 UTC
(In reply to Jonathan Druart from comment #95)
> Created attachment 25899 [details] [review] [review]
> Bug 9016: FIX GetLetters should return all letters
> 
> GetLetters only returns letters with a mtt = email. It should return all
> letter codes in the DB.
> 
> The message_transport_type parameter is never used.
> 
> To reproduce the issue:
> Create a notice with a sms template and no email template.
> Go on the overdue rules configucation page.
> The notice does not appear in the notice list.

Hi there Joubu!
Design fault here. The letter/template is identified by module and code.
You can have duplicate codes.

        $letters{ $letter->{'code'} } = $letter->{'name'};

This overwrites the code when there are overlapping codes in modules.
Comment 97 Olli-Antti Kivilahti 2014-03-12 14:35:22 UTC Comment hidden (obsolete)
Comment 98 Olli-Antti Kivilahti 2014-03-12 16:04:42 UTC
(In reply to Olli-Antti Kivilahti from comment #96)
> Hi there Joubu!
> Design fault here. The letter/template is identified by module and code.
> You can have duplicate codes.
> 
>         $letters{ $letter->{'code'} } = $letter->{'name'};
> 
> This overwrites the code when there are overlapping codes in modules.

Sorry this doesn't apply here, I made a new bug 11931 for this finding.
Comment 99 Jonathan Druart 2014-03-12 17:47:46 UTC Comment hidden (obsolete)
Comment 100 Olli-Antti Kivilahti 2014-03-12 17:49:33 UTC Comment hidden (obsolete)
Comment 101 Olli-Antti Kivilahti 2014-03-12 17:54:23 UTC Comment hidden (obsolete)
Comment 102 Olli-Antti Kivilahti 2014-03-12 18:04:34 UTC Comment hidden (obsolete)
Comment 103 Jonathan Druart 2014-03-13 08:32:06 UTC Comment hidden (obsolete)
Comment 104 Olli-Antti Kivilahti 2014-03-13 09:50:37 UTC Comment hidden (obsolete)
Comment 105 Olli-Antti Kivilahti 2014-03-13 10:19:09 UTC
Check out the awesome documentation we wrapped up here
http://wiki.koha-community.org/wiki/Message_transport_types
Comment 106 Marcel de Rooy 2014-03-27 10:43:18 UTC
Still applies. I will try to pick up now where I left this..
Comment 107 Marcel de Rooy 2014-03-27 14:06:54 UTC
Follow-up QA Comment:
Good/promising feature. Looks good, but with some comments:

1) This patch series provides some groundwork for subsequent changes in follow-up reports.
This means that not everything this patch seems to offer now, is actually used.
For instance, you can edit sms notices now or click phone in overdue rules, but this will not produce any results.
Given that I am told that these changes are still coming in the follow-up reports, this should just be considered as temporary confusion.
I note however that marking these options as not-in-use-yet (somehow..) and releasing them further on in the process would have had my (personal) preference. 

2) At the last transport_type of tools/letter.pl a counter is displayed like: Message body: 11/160.
It was not evident at the start for me that it meant 11 chars from max 160.
No blocker, but could be improved later?

3) When adding a new notice the first time, I was forced to add subject and contents for each transport type (Which I did not really like).
BUT then I tried to reproduce this once again, and I couldn't. (Caching problem?)
So no blocker.
But I discovered one small thing: If I enter code and name on the top, and do not fill anything further. The program seems to accept that and save, but actually it just ignores what I typed. Maybe we should catch that situation. No blocker btw.
And another more annoying one (maybe more theoretical indeed): Fill in code, name and the SECOND mtt. Now it will save one letter (including the name). But it will not pickup the name again when editing this notice. (One of the last patches dealt with something similar.) 

4) In this report overdue notices are still handled by transport type email only.
The commit message in one of the patches suggests otherwise.
As I understand, the next patch series (10832) takes care of it.

5) On Overdue rules, I can enter a delay value and select a notice without selecting a transport type. The action is saved, but reads to me that Koha will not do anything with it. Should we warn the user here? 
No blocker for now. But could be improved.

6) From a commit message: "if you insert a new line in your message_transport_types table, the overdues configuration table did not displayed an header."
How would you add such a type? Just via sql? I understood that the interface will not offer such a functionality.
I think that it is great that the interface immediately shows me overdue rules and notices for the new type. But on the other hand, we could discuss how sensible it really is to do so. No blocker, but an interesting discussion :)

7) Just noting that tools/overduerules.pl contains sql code and you also add some. (Admin scripts are marked as an exception...)

Changed tests pass. No complaints from qa tools.
I am inclined to pass QA, but do you have a quick fix for some of the stuff under point 3?
Comment 108 Jonathan Druart 2014-03-27 14:30:07 UTC
(In reply to M. de Rooy from comment #107)
> Follow-up QA Comment:

Thanks!

> Good/promising feature. Looks good, but with some comments:
> 
> 1) This patch series provides some groundwork for subsequent changes in
> follow-up reports.
> This means that not everything this patch seems to offer now, is actually
> used.

Actually it does not make sense to push this patch set alone. All patches from linked reports should be pushed at the same time.

> 2) At the last transport_type of tools/letter.pl a counter is displayed
> like: Message body: 11/160.
> It was not evident at the start for me that it meant 11 chars from max 160.
> No blocker, but could be improved later?

Hum, what do you propose?

> 3) When adding a new notice the first time, I was forced to add subject and
> contents for each transport type (Which I did not really like).
> BUT then I tried to reproduce this once again, and I couldn't. (Caching
> problem?)
> So no blocker.
> But I discovered one small thing: If I enter code and name on the top, and
> do not fill anything further. The program seems to accept that and save, but
> actually it just ignores what I typed. Maybe we should catch that situation.
> No blocker btw.
> And another more annoying one (maybe more theoretical indeed): Fill in code,
> name and the SECOND mtt. Now it will save one letter (including the name).
> But it will not pickup the name again when editing this notice. (One of the
> last patches dealt with something similar.) 

I will try to have a look at this too.

> 4) In this report overdue notices are still handled by transport type email
> only.
> The commit message in one of the patches suggests otherwise.
> As I understand, the next patch series (10832) takes care of it.

Same as 1). Patches from bug 10832 should be pushed at the same time as this one.

> 5) On Overdue rules, I can enter a delay value and select a notice without
> selecting a transport type. The action is saved, but reads to me that Koha
> will not do anything with it. Should we warn the user here? 
> No blocker for now. But could be improved.

I will try to provide a followup.

> 6) From a commit message: "if you insert a new line in your
> message_transport_types table, the overdues configuration table did not
> displayed an header."

Yes, Olli found this problem on testing the patch. I agreed with him, it is an issue and fixed it. It was in order to avoid further issues.

> How would you add such a type? Just via sql? I understood that the interface
> will not offer such a functionality.
> I think that it is great that the interface immediately shows me overdue
> rules and notices for the new type. But on the other hand, we could discuss
> how sensible it really is to do so. No blocker, but an interesting
> discussion :)

I think it is more flexible to display all mtt existing in DB rather than dealing with "authorized" mtt in templates.

> 7) Just noting that tools/overduerules.pl contains sql code and you also add
> some. (Admin scripts are marked as an exception...)

Hum, yes... I didn't try to refactor this code.
Comment 109 Marcel de Rooy 2014-03-27 14:38:41 UTC
(In reply to Jonathan Druart from comment #108)
> > 2) At the last transport_type of tools/letter.pl a counter is displayed
> > like: Message body: 11/160.
> > It was not evident at the start for me that it meant 11 chars from max 160.
> > No blocker, but could be improved later?
> 
> Hum, what do you propose?
I have no perfect solution at hand, but maybe just adding 11/160 characters would already be more clear?
Comment 110 Jonathan Druart 2014-03-27 15:09:52 UTC Comment hidden (obsolete)
Comment 111 Jonathan Druart 2014-03-27 15:09:59 UTC Comment hidden (obsolete)
Comment 112 Jonathan Druart 2014-03-27 15:19:29 UTC Comment hidden (obsolete)
Comment 113 Jonathan Druart 2014-03-27 15:23:43 UTC
(In reply to M. de Rooy from comment #107)
> 3) When adding a new notice the first time, I was forced to add subject and
> contents for each transport type (Which I did not really like).
> BUT then I tried to reproduce this once again, and I couldn't. (Caching
> problem?)
> So no blocker.
> But I discovered one small thing: If I enter code and name on the top, and
> do not fill anything further. The program seems to accept that and save, but
> actually it just ignores what I typed. Maybe we should catch that situation.
> No blocker btw.
> And another more annoying one (maybe more theoretical indeed): Fill in code,
> name and the SECOND mtt. Now it will save one letter (including the name).
> But it will not pickup the name again when editing this notice. (One of the
> last patches dealt with something similar.) 

Done.

> 5) On Overdue rules, I can enter a delay value and select a notice without
> selecting a transport type. The action is saved, but reads to me that Koha
> will not do anything with it. Should we warn the user here? 
> No blocker for now. But could be improved.

Not trivial. Javascript code to write will be horrible...
In my opinion, it's a lot of ugly code for a small added value.

(In reply to M. de Rooy from comment #109)
> I have no perfect solution at hand, but maybe just adding 11/160 characters
> would already be more clear?

Done.
Comment 114 Marcel de Rooy 2014-03-28 09:10:15 UTC Comment hidden (obsolete)
Comment 115 Marcel de Rooy 2014-03-28 09:10:24 UTC Comment hidden (obsolete)
Comment 116 Marcel de Rooy 2014-03-28 09:10:30 UTC Comment hidden (obsolete)
Comment 117 Marcel de Rooy 2014-03-28 09:10:35 UTC Comment hidden (obsolete)
Comment 118 Marcel de Rooy 2014-03-28 09:10:41 UTC Comment hidden (obsolete)
Comment 119 Marcel de Rooy 2014-03-28 09:10:46 UTC Comment hidden (obsolete)
Comment 120 Marcel de Rooy 2014-03-28 09:10:51 UTC Comment hidden (obsolete)
Comment 121 Marcel de Rooy 2014-03-28 09:10:56 UTC Comment hidden (obsolete)
Comment 122 Marcel de Rooy 2014-03-28 09:11:02 UTC Comment hidden (obsolete)
Comment 123 Marcel de Rooy 2014-03-28 09:11:08 UTC Comment hidden (obsolete)
Comment 124 Marcel de Rooy 2014-03-28 09:11:14 UTC Comment hidden (obsolete)
Comment 125 Marcel de Rooy 2014-03-28 09:11:19 UTC Comment hidden (obsolete)
Comment 126 Marcel de Rooy 2014-03-28 09:11:24 UTC Comment hidden (obsolete)
Comment 127 Marcel de Rooy 2014-03-28 09:11:29 UTC Comment hidden (obsolete)
Comment 128 Marcel de Rooy 2014-03-28 09:11:35 UTC Comment hidden (obsolete)
Comment 129 Marcel de Rooy 2014-03-28 09:11:40 UTC Comment hidden (obsolete)
Comment 130 Katrin Fischer 2014-04-22 15:14:44 UTC
Hi Marcel - do you plan to continue on the other patches? I am not sure in which state that is and if you already started. Could you give a quick update? Thx!
Comment 131 Marcel de Rooy 2014-04-23 11:57:15 UTC
(In reply to Katrin Fischer from comment #130)
> Hi Marcel - do you plan to continue on the other patches? I am not sure in
> which state that is and if you already started. Could you give a quick
> update? Thx!

I plan to continue with some of these reports, but I did not start yet. And I certainly have no monopoly on them. It could be advantageous if some other people also look at this line of development.
Comment 132 Katrin Fischer 2014-04-23 12:00:27 UTC
As feature freeze will be upon us quite soon I was wondering if we can figure out a way to get this QA'd in time.

I am currently still working on some other big patches, but maybe someone else will be able to help you with those before I can?
Comment 133 Jonathan Druart 2014-04-29 07:55:12 UTC
Note for RM, and others:
To complete the feature (MTT), bug 11867 and bug 11209 should reach the "Passed QA" status.
They are bugfixes and quite easy to test.
Comment 134 Jonathan Druart 2014-04-30 07:27:12 UTC
Created attachment 27794 [details] [review]
Bug 9016: Create a message for each transport type.

This patch adds:
- a new jquery plugin : insertatcaret.
- the ability to define a notice template for each transport type.
- a new routine C4::Letters::GetMessageTransportTypes.

Test plan:
- Go on tools/letter.pl and check that all existing notices are still
  there.
- Modify one. A new empty message is present for sms, print, etc. The
  email message is filled with the existant value.
- Add a message for sms for example (don't forget the subject) and save.
- edit again and verify the sms message has been saved.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 135 Jonathan Druart 2014-04-30 07:27:17 UTC
Created attachment 27795 [details] [review]
Bug 9016: Define different transport types for overdues

This patch adds:
- a new routine C4::Overdues::GetOverdueMessageTransportTypes.
- the ability to define one or more transport types for an overdue
  level.

Test plan:
- go on tools/overduerules.pl.
- verify that previous rules still exist and that the 'email' checkboxes
  is checked if a rule is defined.
- redefined rules at different levels and check the transport type
  needed. Currently, only email, sms and print are relevant
- Save the changes.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 136 Jonathan Druart 2014-04-30 07:27:20 UTC
Created attachment 27796 [details] [review]
Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 137 Jonathan Druart 2014-04-30 07:27:24 UTC
Created attachment 27797 [details] [review]
Bug 9016: DB changes: new table overduerules_transport_types

This patch adds:
- a new table overduerules_transport_types.
- a new column letter.message_transport_type.
- a new primary key for letter.
- fill the new table with existing values.

Test plan:
After applying this patch and executing the updatedatabase entry, verify
that the overduerules_transport_types table contains a row for each
entry in the overduerules table.
The message_transport_type column should contain 'email'.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 138 Jonathan Druart 2014-04-30 07:27:28 UTC
Created attachment 27798 [details] [review]
Bug 9016: Fix unit tests

The unit tests should insert overdue rules.
+ Fix the doc for GetOverdueMessageTransportTypes

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 139 Jonathan Druart 2014-04-30 07:27:32 UTC
Created attachment 27799 [details] [review]
Bug 9016: QA Followup

Fix tab characters and POD.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 140 Jonathan Druart 2014-04-30 07:27:36 UTC
Created attachment 27800 [details] [review]
Bug 9016: Fix some translate issues

On tools/letter.pl and tools/overduerules.pl, some strings were not
translatable. This patch should fix that.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 141 Jonathan Druart 2014-04-30 07:27:39 UTC
Created attachment 27801 [details] [review]
Bug 9016 [QA Followup] - Add insertAtCaret library to about page

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 142 Jonathan Druart 2014-04-30 07:27:43 UTC
Created attachment 27802 [details] [review]
Bug 9016: (follow-up) qa fixes

* Fixes POD of GetMessageTransportTypes.
* Removes the useless map in GetMessageTransportTypes.
* Textual: "You must specify a title and a content" -> "Please specify title and content".
* Reintroduces << and >> around the field name.
* Change message for the update DB entry.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 143 Jonathan Druart 2014-04-30 07:27:47 UTC
Created attachment 27803 [details] [review]
Bug 9016: FIX GetLetters should return all letters

GetLetters only returns letters with a mtt = email. It should return all
letter codes in the DB.

The message_transport_type parameter is never used.

To reproduce the issue:
Create a notice with a sms template and no email template.
Go on the overdue rules configucation page.
The notice does not appear in the notice list.

Signed-off-by: Olli-Antti Kivilahti <kivilahtio@ProBook6570b>

---------------
Testing report:
---------------
Testing this subroutine from a test stub. Calling the method without arguments
and with argument 'circulation' and 'circulat'.
Works as supposed to.
Related Bug 11931 discovered but not within the scope of this featureset.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 144 Jonathan Druart 2014-04-30 07:27:51 UTC
Created attachment 27804 [details] [review]
Bug 9016: FIX additional mtt's name should be displayed in header

if you insert a new line in your message_transport_types table, the
overdues configuration table did not displayed an header.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Yes sir works great!

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 145 Jonathan Druart 2014-04-30 07:27:55 UTC
Created attachment 27805 [details] [review]
Bug 9016: FIX letter name is not displayed on editing a letter

On editing a letter, the letter name is not displayed. So if you save
it, the letter name is erased with an empty string.

Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
Tested by setting the "Name"-field in letter.pl, saving, re-editing and verifying
that the email is present in the letter's "Name"-field.
The letter "Name" is replicated for each template in the DB, but that doesn't
incur any performance penalty.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 146 Jonathan Druart 2014-04-30 07:27:58 UTC
Created attachment 27806 [details] [review]
Bug 9016: (followup) FIX letter name is not displayed on editing a letter

If the name is not in email, the name is not correctly retrieved.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 147 Jonathan Druart 2014-04-30 07:28:02 UTC
Created attachment 27807 [details] [review]
Bug 9016: Add some validation on the letter form

1/ Code and Name are mandatory.
2/ At least 1 template should be filled on saving.

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 148 Jonathan Druart 2014-04-30 07:28:06 UTC
Created attachment 27808 [details] [review]
Bug 9016: Improve number of characters display when mtt=sms

Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 149 Jonathan Druart 2014-04-30 07:28:10 UTC
Created attachment 27809 [details] [review]
Bug 9016: [QA Follow-up] Remove warning from content.length test

If the letter.content is undef, the content.length test in the letter
template will trigger warning: Argument "" isn't numeric in numeric gt (>).
This patch eliminates that warning.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 150 Jonathan Druart 2014-04-30 07:28:39 UTC
Last patches fix conflict with bug 11497.
Comment 151 Galen Charlton 2014-05-02 21:19:48 UTC
Pushed to master.  Thanks, Jonathan!
Comment 152 Nicole C. Engard 2014-05-12 14:34:02 UTC
I'm writing up the documentation for this and don't see any explanation of how the 'feed' is supposed to work. I know how to document all other new values, but what is 'feed' and how do I access it?

Thanks
Nicole
Comment 153 Jonathan Druart 2014-05-12 14:43:42 UTC
(In reply to Nicole C. Engard from comment #152)
> I'm writing up the documentation for this and don't see any explanation of
> how the 'feed' is supposed to work. I know how to document all other new
> values, but what is 'feed' and how do I access it?

Hello Nicole,
The feed entry was not introduced by this patch. The entry in the message_transport_types seems to be there for a long time.
I think it is (was?) used to generated RSS feeds. But I don't know how it works and if it works.
Comment 154 Nicole C. Engard 2014-05-13 18:35:18 UTC
(In reply to Jonathan Druart from comment #153)
> Hello Nicole,
> The feed entry was not introduced by this patch. The entry in the
> message_transport_types seems to be there for a long time.
> I think it is (was?) used to generated RSS feeds. But I don't know how it
> works and if it works.

Then we might want to hide those options - it seems very misleading to have options that do nothing on these pages in the release.

Nicole
Comment 155 Jonathan Druart 2014-05-14 07:56:32 UTC
(In reply to Nicole C. Engard from comment #154)
> (In reply to Jonathan Druart from comment #153)
> > Hello Nicole,
> > The feed entry was not introduced by this patch. The entry in the
> > message_transport_types seems to be there for a long time.
> > I think it is (was?) used to generated RSS feeds. But I don't know how it
> > works and if it works.
> 
> Then we might want to hide those options - it seems very misleading to have
> options that do nothing on these pages in the release.
> 
> Nicole

Sorry but which pages are you talking about?
The message transport types displayed on the "Notices and slips" pages can be managed deleting/adding values in the message_transport_types tables.
Comment 156 Marcel de Rooy 2014-05-14 11:26:53 UTC
(In reply to Nicole C. Engard from comment #154)
> Then we might want to hide those options - it seems very misleading to have
> options that do nothing on these pages in the release.

Sounds like the discussion around comment91:
[Now you see the 4 or 5 types (and that could grow..?) But I miss a field like Status that activates or deactivates the transport type for that notice.
We could also use that status somehow to warn people that activating a certain type is not useful or even possible when there is no code to support it yet.]

And comment107:
[This means that not everything this patch seems to offer now, is actually used.
For instance, you can edit sms notices now or click phone in overdue rules, but this will not produce any results.
Given that I am told that these changes are still coming in the follow-up reports, this should just be considered as temporary confusion.
I note however that marking these options as not-in-use-yet (somehow..) and releasing them further on in the process would have had my (personal) preference.]
Comment 157 Katrin Fischer 2014-05-18 10:42:20 UTC
If feed is not implemented, shouldn't we remove it from the sample data and maybe even from existing installations as a transport type?
Comment 158 Jonathan Druart 2014-05-21 06:58:38 UTC
(In reply to Katrin Fischer from comment #157)
> If feed is not implemented, shouldn't we remove it from the sample data and
> maybe even from existing installations as a transport type?

How to be sure nobody uses it?
Does someone remember who/why introduced it into Koha? I didn't find a clear explanation using git log.
Comment 159 Katrin Fischer 2014-05-21 07:35:24 UTC
My understanding is there are no configuration options for it and it as hidden from the display until the newer changes. I think if a library built functionality for it, it's not in current Koha and they could still add back the entry in the database easily.
Comment 160 Mathieu Saby 2014-05-21 07:53:05 UTC
(In reply to Katrin Fischer from comment #159)
> My understanding is there are no configuration options for it and it as
> hidden from the display until the newer changes. I think if a library built
> functionality for it, it's not in current Koha and they could still add back
> the entry in the database easily.

Hello
This puzzled me when I tested last year when I was working in Rennes. I made an enquiry in git history, which I don't remember the result... but I wrote it in the Mantis ticket we used to track this ENH. MAybe it can help.
For me it looked like a mistake.

Mathieu Saby
Comment 161 Jonathan Druart 2014-05-21 07:59:06 UTC
Bug 12298 created.