Bug 21281 - Label Template - Creation not working
Summary: Label Template - Creation not working
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Label/patron card printing (show other bugs)
Version: master
Hardware: PC Linux
: P5 - low blocker (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-08-28 12:45 UTC by Henrique
Modified: 2018-09-21 13:33 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 21281: Surround creator_templates.rows with backquotes (4.18 KB, patch)
2018-08-29 20:12 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 21281: Correct SQL statement in C4/Creators/Template (1.79 KB, patch)
2018-09-05 17:28 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 21281: Correct t/db_dependent/Creators/Lib.t failures (3.11 KB, patch)
2018-09-05 18:44 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 21281: Surround creator_templates.rows with backquotes (4.24 KB, patch)
2018-09-14 16:50 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 21281: Correct t/db_dependent/Creators/Lib.t failures (3.17 KB, patch)
2018-09-14 16:51 UTC, Chris Cormack
Details | Diff | Splinter Review
Bug 21281: Surround creator_templates.rows with backquotes (4.32 KB, patch)
2018-09-21 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21281: Correct t/db_dependent/Creators/Lib.t failures (3.25 KB, patch)
2018-09-21 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 21281: (QA follow-up) Introduce _add_backtics (4.52 KB, patch)
2018-09-21 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Henrique 2018-08-28 12:45:44 UTC
Hi, 

Koha version:	18.05.02.000
mysql Ver 15.1 Distrib 10.3.8-MariaDB

create a "Label Template" and save.
(any configuration ....)

There is no error message.

But it shows a message indicating that "there is no template"

It looks like Koha is using some reserved words ( rows ) during the creation of a new label template...

As i looked the sql commands that koha is using to create the template i figure out that its using "rows" - a new reserved word ...

Could you verify it, please
 tia Henrique
Comment 1 Henrique 2018-08-28 14:00:39 UTC
... more info ...

the SQL command is

INSERT INTO creator_templates (creator, rows, template_code, profile_id, page_height, label_width, left_text_margin, units, cols, label_height, col_gap, row_gap, template_desc, page_width, top_margin, top_text_margin, left_margin) VALUES ('Labels','0','DEFAULT TEMPLATE','0','0','0','0','POINT','0','0','0','0','Default description','0','0','0','0')


it does NOT work on 
10.3.8-MariaDB-1:10.3.8+maria~stretch-log - mariadb.org binary distribution


It WORKs on
10.0.30-MariaDB-0+deb8u2



ROWS is a reserved word ....

tia,
Henrique
Comment 2 Chris Nighswonger 2018-08-28 14:55:56 UTC
The solution is to quote the keyword in question with backticks per the MariaDB knowledge base: "Reserved words cannot be used as Identifiers, unless they are quoted."[1]

[1]https://mariadb.com/kb/en/library/reserved-words/
Comment 3 Henrique 2018-08-28 16:38:24 UTC
Hi Chris,

yes, i know that. backticks is the solution ...

but i dont want to mess with koha code ....

koha uses some record object / classes / model ...

...not so easy to change

i think koha should code backticks on all sql statments to solve this sittuation and prevent any others in the future ....

tia
Comment 4 Jonathan Druart 2018-08-29 20:12:55 UTC
Created attachment 78292 [details] [review]
Bug 21281: Surround creator_templates.rows with backquotes

'rows' is a reserved word since MariaDB 10.2.4 and MySQL
https://mariadb.com/kb/en/library/mariadb-1024-release-notes/
https://dev.mysql.com/doc/refman/8.0/en/keywords.html

Test plan:
Do not apply this patch and make sure you recreate the reported issue
Apply this patch and confirm that it is now fixed.

QA will take care for the changes in installer and test files
Comment 5 M. Tompsett 2018-09-05 15:52:49 UTC
Binding to UNIX socket file "/var/run/koha/kohadev/plack.sock"
Setting gid to "1001 1001"
Setting uid to "1001"
Starman: Accepting connections at unix://localhost:/var/run/koha/kohadev/plack.sock/
DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'rows) VALUES ('0','Labels','0','0','0','DEFAULT TEMPLATE','0','0','0','POINT','0' at line 1 [for Statement "INSERT INTO creator_templates (top_text_margin, creator, label_height, label_width, profile_id, template_code, page_height, row_gap, left_text_margin, units, page_width, left_margin, col_gap, top_margin, cols, template_desc, rows) VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?);" with ParamValues: 0=0, 1='Labels', 2=0, 3=0, 4="0", 5="DEFAULT TEMPLATE", 6=0, 7=0, 8=0, 9="POINT", 10=0, 11=0, 12=0, 13=0, 14=0, 15="bleep!", 16=0] at /home/vagrant/kohaclone/C4/Creators/Template.pm line 199.
Database returned the following error: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'rows) VALUES ('0','Labels','0','0','0','DEFAULT TEMPLATE','0','0','0','POINT','0' at line 1 at /home/vagrant/kohaclone/C4/Creators/Template.pm line 201.

-- this is after applying the patch.
Comment 6 M. Tompsett 2018-09-05 17:28:58 UTC
Created attachment 78448 [details] [review]
Bug 21281: Correct SQL statement in C4/Creators/Template

TEST PLAN
---------
1) Start a stretch kohgadevbox
2) Upgrade your MariaDB to 10.3 whatever
   (Google for instructions)
3) reset_all
   -- ignore the zebra problem.
   -- maybe include a sudo systemctl restart daemon-reload
      or whatever might have been mentioned in the output.
4) log in as admin/admin to staff client
5) Tool -> Label Creator -> New -> Layout Template
6) Change the description to something know like 'foobar'.
7) Save
   -- doesn't appear.
8) cat /var/log/koha/kohadev/plack-err.log
   -- mentions of errors like in comment #5
9) apply this patch
10) echo | sudo tee /var/log/koha/kohadev/plack-err.log
    -- this will make checking the log file easier.
11) restart_all
   -- maybe include a sudo systemctl restart daemon-reload
      or whatever might have been mentioned in the output.
12) reload the staff client page.
13) repeat steps 5-7
    -- template shows up this time.
14) cat /var/log/koha/kohadev/plack-err.log
    -- no errors as before.
15) run koha qa test tools
    -- passes
Comment 7 M. Tompsett 2018-09-05 17:53:47 UTC
Comment on attachment 78448 [details] [review]
Bug 21281: Correct SQL statement in C4/Creators/Template

busted prove case.
Comment 8 M. Tompsett 2018-09-05 18:18:45 UTC
prove -v -r t/db_dependent/Creators/

current patch breaks. Patch needs tweaking still.
Comment 9 M. Tompsett 2018-09-05 18:44:43 UTC
Created attachment 78449 [details] [review]
Bug 21281: Correct t/db_dependent/Creators/Lib.t failures

TEST PLAN
---------
1) apply first patch
2) kshell kohadev
3) prove -r -v t/db_dependent/Creators/
   -- failures
4) apply this patch
5) repeat 2,3
   -- no failures
6) run koha qa test tools
Comment 10 Michal Denar 2018-09-07 11:04:16 UTC
Hi,
If I did folow tets plan, I get erroe after first patch:

not ok 334 - get_batch_summary() raises warning if something went wrong with the sql request execution

#   Failed test 'get_batch_summary() raises warning if something went wrong with the sql request execution'
#   at t/db_dependent/Creators/Lib.t line 766.
# found warning: DBD::mysql::st execute failed: Unknown column 'count(batch_id) as _item_count' in 'field list' [for Statement "SELECT `batch_id`, `count(batch_id) as _item_count` FROM creator_batches GROUP BY batch_id"] at /home/vagrant/kohaclone/C4/Creators/Lib.pm line 286.
# found warning: Database returned the following error on attempted SELECT: something went wrong at /home/vagrant/kohaclone/C4/Creators/Lib.pm line 288.
# expected to find warning: Database returned the following error on attempted SELECT: something went wrong
ok 335 - $batches return -1
DBD::mysql::st execute failed: Unknown column 'count(batch_id) as _item_count' in 'field list' [for Statement "SELECT `batch_id`, `count(batch_id) as _item_count` FROM creator_batches WHERE 1  AND creator = ?  GROUP BY batch_id" with ParamValues: 0='Labels'] at /home/vagrant/kohaclone/C4/Creators/Lib.pm line 286.
Database returned the following error on attempted SELECT: Unknown column 'count(batch_id) as _item_count' in 'field list' at /home/vagrant/kohaclone/C4/Creators/Lib.pm line 288.
ok 336 - There are 2 batches
Can't use string ("-1") as an ARRAY ref while "strict refs" in use at t/db_dependent/Creators/Lib.t line 785.
# Looks like your test exited with 255 just after 336.
Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 310/645 subtests

Test Summary Report
-------------------
t/db_dependent/Creators/Lib.t (Wstat: 65280 Tests: 336 Failed: 1)
  Failed test:  334
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 645 tests but ran 336.
Files=1, Tests=336,  2 wallclock secs ( 0.13 usr  0.04 sys +  1.82 cusr  0.13 csys =  2.12 CPU)
Result: FAIL



If I did apply both patch together, first test was without errors.

I'm new in testing, maybe I did some mistake.

Mike
Comment 11 Michal Denar 2018-09-07 11:09:16 UTC
After aplly of second patch I didi this error:

Cannot detect source of '--no'! at /usr/share/perl/5.20/TAP/Parser/IteratorFactory.pm line 261.
	TAP::Parser::IteratorFactory::detect_source(TAP::Parser::IteratorFactory=HASH(0x12b1dd8), TAP::Parser::Source=HASH(0x12b1e98)) called at /usr/share/perl/5.20/TAP/Parser/IteratorFactory.pm line 211
	TAP::Parser::IteratorFactory::make_iterator(TAP::Parser::IteratorFactory=HASH(0x12b1dd8), TAP::Parser::Source=HASH(0x12b1e98)) called at /usr/share/perl/5.20/TAP/Parser.pm line 470
	TAP::Parser::_initialize(TAP::Parser=HASH(0x135d2d0), HASH(0x1051b70)) called at /usr/share/perl/5.20/TAP/Object.pm line 55
	TAP::Object::new("TAP::Parser", HASH(0x1051b70)) called at /usr/share/perl/5.20/TAP/Object.pm line 130
	TAP::Object::_construct(TAP::Harness=HASH(0xee3e20), "TAP::Parser", HASH(0x1051b70)) called at /usr/share/perl/5.20/TAP/Harness.pm line 793
	TAP::Harness::make_parser(TAP::Harness=HASH(0xee3e20), TAP::Parser::Scheduler::Job=HASH(0x10519c0)) called at /usr/share/perl/5.20/TAP/Harness.pm line 592
	TAP::Harness::_aggregate_single(TAP::Harness=HASH(0xee3e20), TAP::Parser::Aggregator=HASH(0xffb4a8), TAP::Parser::Scheduler=HASH(0xf036e0)) called at /usr/share/perl/5.20/TAP/Harness.pm line 684
	TAP::Harness::aggregate_tests(TAP::Harness=HASH(0xee3e20), TAP::Parser::Aggregator=HASH(0xffb4a8), "t/db_dependent/Creators/Lib.t", "--no", "failures") called at /usr/share/perl/5.20/TAP/Harness.pm line 499
	TAP::Harness::__ANON__() called at /usr/share/perl/5.20/TAP/Harness.pm line 512
	TAP::Harness::runtests(TAP::Harness=HASH(0xee3e20), "t/db_dependent/Creators/Lib.t", "--no", "failures") called at /usr/share/perl/5.20/App/Prove.pm line 553
	App::Prove::_runtests(App::Prove=HASH(0x96b720), HASH(0xebfc00), "TAP::Harness", "t/db_dependent/Creators/Lib.t", "--no", "failures") called at /usr/share/perl/5.20/App/Prove.pm line 511
	App::Prove::run(App::Prove=HASH(0x96b720)) called at /usr/bin/prove line 13

Mike
Comment 12 M. Tompsett 2018-09-07 17:19:57 UTC
Michal Denar, this is what I would expect you to see.
Install a kohadevbox, vagrant up and then vagrant ssh.
Get into the kohaclone directory.
git checkout -b bug_21281 origin/master
And then the following output should be practically identical.
I did [SNIP] the excessive passing parts.

vagrant@kohadevbox:kohaclone(bug_21281)$ git reset --hard origin/master
HEAD is now at a9e859942c Bug 19550: (QA follow-up) Add missing [% USE %]
vagrant@kohadevbox:kohaclone(bug_21281)$ git bz apply 21281
Bug 21281 - Label Template - Creation not working

78292 - Bug 21281: Surround creator_templates.rows with backquotes
78449 - Bug 21281: Correct t/db_dependent/Creators/Lib.t failures

Apply? [(y)es, (n)o, (i)nteractive] i
[Comment out second line, so only first one applies]
Applying: Bug 21281: Surround creator_templates.rows with backquotes
vagrant@kohadevbox:kohaclone(bug_21281)$ kshell kohadev
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove -r -v t/db_dependent/Creators/
t/db_dependent/Creators/Lib.t ..
1..645
ok 1 - use C4::Creators::Lib;
[SNIP]
ok 86 - creator          is good
DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'rows = '7'' at line 1 [for Statement "SELECT * FROM creator_templates WHERE 1  AND rows = ? " with ParamValues: 0=7] at /home/vagrant/kohaclone/C4/Creators/Lib.pm line 179.
Database returned the following error: You have an error in your SQL syntax; check the manual that corresponds to your MariaDB server version for the right syntax to use near 'rows = '7'' at line 1 at /home/vagrant/kohaclone/C4/Creators/Lib.pm line 181.
ok 87 - There is 1 template matching
Can't use string ("-1") as an ARRAY ref while "strict refs" in use at t/db_dependent/Creators/Lib.t line 376.
# Looks like your test exited with 11 just after 87.
Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 558/645 subtests

Test Summary Report
-------------------
t/db_dependent/Creators/Lib.t (Wstat: 2816 Tests: 87 Failed: 0)
  Non-zero exit status: 11
  Parse errors: Bad plan.  You planned 645 tests but ran 87.
Files=1, Tests=87,  2 wallclock secs ( 0.02 usr  0.01 sys +  1.06 cusr  0.33 csys =  1.42 CPU)
Result: FAIL
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ exit
exit
vagrant@kohadevbox:kohaclone(bug_21281)$ git bz apply 21281
Bug 21281 - Label Template - Creation not working

78292 - Bug 21281: Surround creator_templates.rows with backquotes
78449 - Bug 21281: Correct t/db_dependent/Creators/Lib.t failures

Apply? [(y)es, (n)o, (i)nteractive] i
[Comment out first one, so both will be applied]
Applying: Bug 21281: Correct t/db_dependent/Creators/Lib.t failures
vagrant@kohadevbox:kohaclone(bug_21281)$ kshell kohadev
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove -r -v t/db_dependent/Creators/
t/db_dependent/Creators/Lib.t ..
1..645
ok 1 - use C4::Creators::Lib;
[SNIP]
ok 645 - categorycode is good
ok
All tests successful.
Files=1, Tests=645,  2 wallclock secs ( 0.06 usr  0.05 sys +  1.04 cusr  0.39 csys =  1.54 CPU)
Result: PASS
kohadev-koha@kohadevbox:/home/vagrant/kohaclone$
Comment 13 Chris Cormack 2018-09-14 16:50:54 UTC
Created attachment 78668 [details] [review]
Bug 21281: Surround creator_templates.rows with backquotes

'rows' is a reserved word since MariaDB 10.2.4 and MySQL
https://mariadb.com/kb/en/library/mariadb-1024-release-notes/
https://dev.mysql.com/doc/refman/8.0/en/keywords.html

Test plan:
Do not apply this patch and make sure you recreate the reported issue
Apply this patch and confirm that it is now fixed.

QA will take care for the changes in installer and test files

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 14 Chris Cormack 2018-09-14 16:51:13 UTC
Created attachment 78669 [details] [review]
Bug 21281: Correct t/db_dependent/Creators/Lib.t failures

TEST PLAN
---------
1) apply first patch
2) kshell kohadev
3) prove -r -v t/db_dependent/Creators/
   -- failures
4) apply this patch
5) repeat 2,3
   -- no failures
6) run koha qa test tools

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>
Comment 15 Marcel de Rooy 2018-09-21 07:15:37 UTC
-    my $query = "SELECT `$params[0]` FROM $params[1]";
+    my $fields_list = $params[0];

If I am not mistaken, params[0] is a single field, not a field list. So the new variable name is confusing.

If it would be a list of fields, this code does not look good:
+    if (index($fields_list, ' ')==-1 && index($fields_list,',')==-1 && $fields_list ne '*') {
+        $fields_list = "`$fields_list`";
+    }
If you would pass 'rows, something', it would not add backtics.
Comment 16 Marcel de Rooy 2018-09-21 08:12:03 UTC
Working on a follow-up..
Comment 17 Marcel de Rooy 2018-09-21 08:31:38 UTC
Created attachment 79192 [details] [review]
Bug 21281: Surround creator_templates.rows with backquotes

'rows' is a reserved word since MariaDB 10.2.4 and MySQL
https://mariadb.com/kb/en/library/mariadb-1024-release-notes/
https://dev.mysql.com/doc/refman/8.0/en/keywords.html

Test plan:
Do not apply this patch and make sure you recreate the reported issue
Apply this patch and confirm that it is now fixed.

QA will take care for the changes in installer and test files

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 18 Marcel de Rooy 2018-09-21 08:31:43 UTC
Created attachment 79193 [details] [review]
Bug 21281: Correct t/db_dependent/Creators/Lib.t failures

TEST PLAN
---------
1) apply first patch
2) kshell kohadev
3) prove -r -v t/db_dependent/Creators/
   -- failures
4) apply this patch
5) repeat 2,3
   -- no failures
6) run koha qa test tools

Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 19 Marcel de Rooy 2018-09-21 08:31:48 UTC
Created attachment 79194 [details] [review]
Bug 21281: (QA follow-up) Introduce _add_backtics

Added a (initially trivial) test in Creators/Lib.t too.
Sometimes you start something, but you end somewhere else ;)

The test seems a bit slower now (regex, lookahead, etc). But this should
not hurt label creators in daily use.
Advantages: code is even more solid, consolidated in one place and can be
tested on its own.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 20 Marcel de Rooy 2018-09-21 08:33:20 UTC
> QA will take care for the changes in installer and test files

Sorry. Same lack amount of time.
Warm invitation to add them now :)
Comment 21 Marcel de Rooy 2018-09-21 08:34:44 UTC
(In reply to Marcel de Rooy from comment #19)
> The test seems a bit slower now (regex, lookahead, etc). But this should

Not even lookahead btw
Comment 22 Jonathan Druart 2018-09-21 12:31:01 UTC
(In reply to Marcel de Rooy from comment #20)
> > QA will take care for the changes in installer and test files
> 
> Sorry. Same lack amount of time.
> Warm invitation to add them now :)

It was part of the test plan: the changes to .sql and .t cannot be tested by everybody. Nothing is missing.

Thanks for the follow-up
Comment 23 M. Tompsett 2018-09-21 13:33:33 UTC
Comment on attachment 79194 [details] [review]
Bug 21281: (QA follow-up) Introduce _add_backtics

Review of attachment 79194 [details] [review]:
-----------------------------------------------------------------

Thanks for this. I didn't get around to writing something similar.