Description
Jonathan Druart
2012-11-07 08:50:27 UTC
Created attachment 13276 [details] [review] Bug 9016: Multi transport types for notices The goal of the patch: This patch allows to choose one (or more) message transport type for one overdue. Currently it is just possible to choose a letter. To prevent too many letters, we create an association table between overduerules and message_transport_types and change the primary key for the letter table. Like this, we are able to have an unique letter code for several transport types. The idea of this patch is to have a sms driver to send messages with a transport type defined as "sms". This patch: - adds a new jquery plugin : insertatcaret - adds datatable on the tools/letter.pl page - adds a new column letter.message_transport_type - adds a new table overduerules_transport_types : association table between overduerules and message_transport_types. - modifies the primary key for the letter table (become module, code, branchcode, message_transport_type) - rewrites a big part of the tools/overduerules.tt code The migration problem: A new updatedatabase entry changes the DB structures changes and set all the letter.message_transport_type to 'email'. The email type is the default type everywhere in the code. To test: 1/ Check that all your previous overdue rules are preserved 2/ Check that the tables on the tools/overduerules.pl page are filled with the email transport type (default type). 3/ On the tools/letters.pl check that your previous letters still exist. 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. We are currently working on this patch, please do not consider this submission for the moment Created attachment 20814 [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'. Created attachment 20815 [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. Created attachment 20816 [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. (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. Created attachment 20916 [details] [review] Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref 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". Created attachment 21164 [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'. Created attachment 21165 [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. Created attachment 21166 [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. Created attachment 21167 [details] [review] Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref 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". Created attachment 21636 [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'. Created attachment 21637 [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. Created attachment 21638 [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. Created attachment 21639 [details] [review] Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref Patch applied cleanly, go forth and signoff ----------------- -- 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. (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. Created attachment 22814 [details] [review] Bug 9016: Fix unit tests The unit tests should insert overdue rules. + Fix the doc for GetOverdueMessageTransportTypes Sponsored by Rennes 2 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 Oups, I tested with french translation. I suppose that's why it did not work... Back to need sign off. Mathieu 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 Note : my SMS template was just the text "SMS" 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. (In reply to Jonathan Druart from comment #27) Test plan is right. Please see the test plan from the active patches, not obsolete ones. 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 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! Created attachment 23272 [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'. Created attachment 23273 [details] [review] Bug 9016: Fix unit tests The unit tests should insert overdue rules. + Fix the doc for GetOverdueMessageTransportTypes (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. Created attachment 23289 [details] [review] [SIGNED-OFF] 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> Created attachment 23290 [details] [review] [SIGNED-OFF] 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> Created attachment 23291 [details] [review] [SIGNED-OFF] Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> Created attachment 23292 [details] [review] [SIGNED-OFF] 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> Created attachment 23293 [details] [review] [SIGNED-OFF] 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> Changes ok. Please rebase and address this qa script error: FAIL C4/Overdues.pm FAIL pod *** ERROR: Spurious text after =cut in file C4/Overdues.pm Created attachment 23562 [details] [review] Bug 9016: QA Followup Fix tab characters and POD. 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. 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 (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? Works well here! Created attachment 23955 [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> Created attachment 23956 [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> Created attachment 23957 [details] [review] Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> Created attachment 23958 [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> Created attachment 23959 [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> Created attachment 23960 [details] [review] Bug 9016: QA Followup Fix tab characters and POD. Last patches fix conflicts with commit b753678c3dff4e0fd2fde008b3c43f0796fcfb05 Bug 11208: regression test for failing SMS messages 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 Created attachment 24571 [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> Created attachment 24572 [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> Created attachment 24573 [details] [review] Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> Created attachment 24574 [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> Created attachment 24575 [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> Created attachment 24576 [details] [review] Bug 9016: QA Followup Fix tab characters and POD. (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. 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. Created attachment 25006 [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. (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! Created attachment 25123 [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> Created attachment 25124 [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> Created attachment 25125 [details] [review] Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> Created attachment 25126 [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> Created attachment 25127 [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> Created attachment 25128 [details] [review] Bug 9016: QA Followup Fix tab characters and POD. Created attachment 25129 [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. Created attachment 25130 [details] [review] Bug 9016 [QA Followup] - Add insertAtCaret library to about page CONFLICT (content): Merge conflict in koha-tmpl/intranet-tmpl/prog/en/modules/tools/letter.tt Created attachment 25415 [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> Created attachment 25416 [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> Created attachment 25417 [details] [review] Bug 9016: FIX GetMessageTransportTypes returns an arrayref, not hashref Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> Created attachment 25418 [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> Created attachment 25419 [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> Created attachment 25420 [details] [review] Bug 9016: QA Followup Fix tab characters and POD. Created attachment 25421 [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. Created attachment 25422 [details] [review] Bug 9016 [QA Followup] - Add insertAtCaret library to about page Will have a look again. Will probably not finish this today however,, 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.. (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. (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.. Thanks for the feedback Mr. de Rooy. From now on I will attach a testing report for all of my sign offs. (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 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. (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. Created attachment 25606 [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. 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 (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. 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? (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. Created attachment 25899 [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. (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. Created attachment 26182 [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. (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. Created attachment 26209 [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. Created attachment 26210 [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 <kivilahtio@ProBook6570b> Yes sir works great! Created attachment 26211 [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 <kivilahtio@ProBook6570b> Yes sir works great! Created attachment 26216 [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! Created attachment 26226 [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. Created attachment 26233 [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. Check out the awesome documentation we wrapped up here http://wiki.koha-community.org/wiki/Message_transport_types Still applies. I will try to pick up now where I left this.. 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? (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. (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? Created attachment 26648 [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> Created attachment 26649 [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> Created attachment 26650 [details] [review] Bug 9016: Improve number of characters display when mtt=sms Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> (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. Created attachment 26656 [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> Created attachment 26657 [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> Created attachment 26658 [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> Created attachment 26659 [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> Created attachment 26660 [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> Created attachment 26661 [details] [review] Bug 9016: QA Followup Fix tab characters and POD. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 26662 [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> Created attachment 26663 [details] [review] Bug 9016 [QA Followup] - Add insertAtCaret library to about page Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Created attachment 26664 [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> Created attachment 26665 [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> Created attachment 26666 [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> Created attachment 26667 [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> Created attachment 26668 [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> Created attachment 26670 [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> Created attachment 26672 [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> Created attachment 26673 [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> 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! (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. 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? 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. 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> Last patches fix conflict with bug 11497. Pushed to master. Thanks, Jonathan! 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 (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. (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 (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. (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.] If feed is not implemented, shouldn't we remove it from the sample data and maybe even from existing installations as a transport type? (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. 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. (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 Bug 12298 created. |