Bug 5462

Summary: Fix variable names so we dont break template::toolkit
Product: Koha Reporter: Chris Cormack <chris>
Component: Architecture, internals, and plumbingAssignee: Galen Charlton <gmcharlt>
Status: CLOSED FIXED QA Contact: Bugs List <koha-bugs>
Severity: enhancement    
Priority: PATCH-Sent (DO NOT USE) CC: colin.campbell, followingthepath, koha.sekjal, m.de.rooy, nengard, ztajoli
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on:    
Bug Blocks: 5917    
Attachments: Proposed correction
Correction for MARCdetail.pl
follow up patch
Fix for patron messaging attributes
Update for IT sql definition
Update of the italian sql files The bug is about strings that are used as variables by Template::Toolkit The strings need to be like valid perl variable names. To fix the bug we need to fix also .sql files. Others .sql files are OK. Italian .sql file need

Description Chris Cormack 2010-11-30 21:10:40 UTC
Variables for template toolkit need to be valid perl variable names.

Some of our template variables aren't. We should fix them even if we weren't changing to template::toolkit but we definitely need to before we can move.
Comment 1 Chris Cormack 2010-11-30 21:31:31 UTC
new/awaiting_qa/5462 has the fixes please test
Comment 2 Colin Campbell 2011-02-03 10:22:30 UTC
Branch bug_5462_signoff on git://github.com/colinsc/koha.git has been rebased against current master and tested for completeness
Comment 3 Chris Cormack 2011-02-03 19:58:39 UTC
Pushed, please test
Comment 4 Owen Leonard 2011-02-07 17:27:03 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2011-02-08 08:09:15 UTC
Follow up patch pushed
Comment 6 Marcel de Rooy 2011-02-09 13:14:06 UTC
Current master does not show marc details for a record in staff client. All tabs have disappeared. (Click on MARC tab in normal view for one record.) 

Problem seems to be connected with one of the patches connected to this report.

Commit 8fd29f3d62d1aa5d7e728032ea00bd98f3aef52d for this report makes the marc details disappear..

Few commits before the patch, at e29aca325d199468673365ee7540760b38630833, I am still seeing marc details.

Apparently, some work on the templates should be adjusted?
Comment 7 Owen Leonard 2011-02-09 13:46:56 UTC Comment hidden (obsolete)
Comment 8 Owen Leonard 2011-02-11 19:32:12 UTC
*** Bug 5744 has been marked as a duplicate of this bug. ***
Comment 9 Chris Cormack 2011-02-13 21:36:36 UTC
Signed off patch does not apply

Applying: Follow-up fix for Bug 5462, Fix variable names so we dont break template::toolkit
Using index info to reconstruct a base tree...
error: patch failed: koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/MARCdetail.tmpl:45
error: koha-tmpl/intranet-tmpl/prog/en/modules/catalogue/MARCdetail.tmpl: patch does not apply
Did you hand edit your patch?
It does not apply to blobs recorded in its index.
Cannot fall back to three-way merge.
Patch failed at 0001 Follow-up fix for Bug 5462, Fix variable names so we dont break template::toolkit
Comment 10 Marcel de Rooy 2011-02-14 08:12:58 UTC
I recreated the patch, but the referred blobs are still the same. And the patches still apply. I tested it again with latest commit mentioned below.

Are you sure that you tested this patch with a clean working dir? What could be the reason for not matching blobs?

[marcel@RKM004 testclone]$ git apply ../0001-5462.patch
[marcel@RKM004 testclone]$ git reset --hard
HEAD is now at f6be5ef Bug 5751 : Follow up patch, removing unused variable and fixing copyright
[marcel@RKM004 testclone]$ git am -3 ../0001-5462.patch
Applying: Follow-up fix for Bug 5462, Fix variable names so we dont break template::toolkit
Comment 11 Nicole C. Engard 2011-02-16 13:52:52 UTC Comment hidden (obsolete)
Comment 12 Chris Cormack 2011-02-16 20:29:01 UTC
Follow up patch pushed
Comment 13 MJ Ray (software.coop) 2011-03-04 10:26:52 UTC
This bug is mentioned in:
Follow-up correction for Bug 5462 - Fix	variable names for template::toolkit http://lists.koha-community.org/pipermail/koha-patches/2011-February/013775.html
Follow-up fix for Bug 5462,	Fix variable names so we dont break template::toolkit http://lists.koha-community.org/pipermail/koha-patches/2011-February/013796.html
Follow-up fix for Bug 5462, Fix variable names so we dont break template::toolkit http://lists.koha-community.org/pipermail/koha-patches/2011-February/013797.html
Follow up on Bug 5462: fixing	variable names breaks messaging preference form http://lists.koha-community.org/pipermail/koha-patches/2011-February/013873.html
Follow-up fix for Bug 5462, Fix variable names so we dont break template::toolkit http://lists.koha-community.org/pipermail/koha-patches/2011-February/013848.html
Comment 14 Owen Leonard 2011-03-30 20:16:12 UTC
I discovered a new instance of this bug. In Messaging.pm sub GetMessagingOptions queries the message_attributes table for message_attributes.message_name and assigns a series of variables based on that name. This means we end up with variables named things like "Item due," and thus the template has checks for things like <!-- TMPL_IF NAME="Item Due" --> and <!-- TMPL_ELSIF NAME="Item Check-in" -->
Comment 15 Owen Leonard 2011-03-31 16:21:01 UTC Comment hidden (obsolete)
Comment 16 MJ Ray (software.coop) 2011-04-11 08:32:25 UTC
This bug is mentioned in:
Incremental fix for Bug 5462,	Fix variable names so we dont break template::toolkit http://lists.koha-community.org/pipermail/koha-patches/2011-March/014402.html
Comment 17 Zeno Tajoli 2011-05-12 19:55:34 UTC
This bug is partial done in 3.6.x tree.
To do:
Insert this line at C4/Members/Messaging.pm,line 212:

$row->{'message_name'} =~ s/\W/\_/g;

With this last commit the bug is done.
Or, more exactly, all patch here present will be commit into the master.
Comment 18 Chris Cormack 2011-05-12 22:19:30 UTC
Zeno, the message names have now been fixed in the database.

So that line should not be needed, ie

insert into `message_attributes`
(`message_attribute_id`, message_name, `takes_days`)
values
(1, 'Item_Due', 0),
(2, 'Advance_Notice', 1),
(4, 'Hold_Filled', 0),
(5, 'Item_Check_in', 0),
(6, 'Item_Checkout', 0);

$DBversion = "3.03.00.051";
if (C4::Context->preference("Version") < TransformToNum($DBversion)) {
    print "Upgrade to $DBversion done (Remove spaces and dashes from message_attribute names)\n";
    $dbh->do("UPDATE message_attributes SET message_name = 'Item_Due' WHERE message_name='Item Due'");
    $dbh->do("UPDATE message_attributes SET message_name = 'Advance_Notice' WHERE message_name='Advance Notice'");
    $dbh->do("UPDATE message_attributes SET message_name = 'Hold_Filled' WHERE message_name='Hold Filled'");
    $dbh->do("UPDATE message_attributes SET message_name = 'Item_Check_in' WHERE message_name='Item Check-in'");
    $dbh->do("UPDATE message_attributes SET message_name = 'Item_Checkout' WHERE message_name='Item Checkout'");
    SetVersion ($DBversion);
}

So i think we can mark this resolved fixed ... do you agree?
Comment 19 Zeno Tajoli 2011-05-13 08:13:59 UTC Comment hidden (obsolete)
Comment 20 Zeno Tajoli 2011-05-13 08:16:22 UTC
To cloese the patch, we need to to update also italian .sql file
installer/data/mysql/it-IT/necessari/sample_notices_message_attributes.sql

I have insert a patch for it here (as attach) and I send it to koha-patches.
The patch is for 3.6 and 3.4 also.
Comment 21 Zeno Tajoli 2011-05-13 08:17:25 UTC
Link to koha-patches:
http://lists.koha-community.org/pipermail/koha-patches/2011-May/015047.html
Comment 22 Chris Cormack 2011-05-30 08:26:12 UTC
Created attachment 4299 [details] [review]
Update of the italian sql files The bug is about strings that are used as variables by Template::Toolkit The strings need to be like valid perl variable names. To fix the bug we need to fix also .sql files. Others .sql files are OK. Italian .sql file need

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 23 Chris Cormack 2011-05-30 08:26:54 UTC
Signed off patch attached
Comment 24 Ian Walls 2011-07-14 22:32:28 UTC
Easy patch to read, names consistent.  Marking as Passed QA
Comment 25 Chris Cormack 2011-07-14 23:11:37 UTC
Pushed, please test