Bug 19259 - Overdue rules do not save (delay field should only accept numbers)
Summary: Overdue rules do not save (delay field should only accept numbers)
Status: Pushed to Master
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords: Academy
Depends on:
Blocks:
 
Reported: 2017-09-06 00:00 UTC by Aleisha Amohia
Modified: 2017-10-11 16:54 UTC (History)
3 users (show)

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


Attachments
Bug 19259: Enforce type of the delay value when editing overdue rules - template side only (1.57 KB, patch)
2017-09-27 16:11 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 19259: Enforce type of the delay value when editing overdue rules - template side only (1.62 KB, patch)
2017-09-29 18:18 UTC, David Bourgault
Details | Diff | Splinter Review
Bug 19259: Enforce type of the delay value when editing overdue rules - template side only (1.68 KB, patch)
2017-10-11 00:24 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Aleisha Amohia 2017-09-06 00:00:16 UTC
I am unable to save any overdue rules.

To reproduce:
Go to Tools -> Overdue notice/status triggers
Attempt to save some rules/make changes
Click Save changes button
Notice your rules were not saved. Check the overduerules table in mysql for confirmation
Comment 1 Marc Véron 2017-09-06 05:54:32 UTC
Confirmed.

In my tests it saves columns Delay, Letter and Restrict.
It does NOT save columns Email and Print (Phone and SMS not tested).

While saving I get the following in plack-error.log:

DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`overduerules_transport_types`, CONSTRAINT `overduerules_fk` FOREIGN KEY (`overduerules_id`) REFERENCES `old_overduerules` (`overduerules_id`) ON DELETE CASCADE ON UPDATE CASCAD) [for Statement "
        INSERT INTO overduerules_transport_types(
            overduerules_id, letternumber, message_transport_type
        ) VALUES (
            (SELECT overduerules_id FROM overduerules WHERE branchcode = ? AND categorycode = ?), ?, ?
        )
    " with ParamValues: 0='', 1='K', 2=1, 3="email"] at /home/marc/koha/tools/overduerules.pl line 198.
Comment 2 Jonathan Druart 2017-09-06 17:09:32 UTC
It works for me on master with a fresh install.

old_overduerules sounds related to 3.23.00.007

What returns "show create table overduerules_transport_types"?
Comment 3 Marc Véron 2017-09-06 19:40:53 UTC
(In reply to Jonathan Druart from comment #2)
> It works for me on master with a fresh install.
> 
> old_overduerules sounds related to 3.23.00.007
> 
> What returns "show create table overduerules_transport_types"?

mysql> show create table overduerules_transport_types;
+------------------------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| Table                        | Create Table                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                         |
+------------------------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| overduerules_transport_types | CREATE TABLE `overduerules_transport_types` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `letternumber` int(1) NOT NULL DEFAULT '1',
  `message_transport_type` varchar(20) COLLATE utf8_unicode_ci NOT NULL DEFAULT 'email',
  `overduerules_id` int(11) NOT NULL,
  PRIMARY KEY (`id`),
  KEY `overduerules_fk` (`overduerules_id`),
  KEY `mtt_fk` (`message_transport_type`),
  CONSTRAINT `mtt_fk` FOREIGN KEY (`message_transport_type`) REFERENCES `message_transport_types` (`message_transport_type`) ON DELETE CASCADE ON UPDATE CASCADE,
  CONSTRAINT `overduerules_fk` FOREIGN KEY (`overduerules_id`) REFERENCES `old_overduerules` (`overduerules_id`) ON DELETE CASCADE ON UPDATE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci |
+------------------------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)
Comment 4 Marc Véron 2017-09-06 20:42:56 UTC
Reading:
https://dev.mysql.com/doc/refman/5.5/en/rename-table.html
-------
RENAME TABLE changes internally generated foreign key constraint names and user-defined foreign key constraint names that contain the string “tbl_name_ibfk_” to reflect the new table name. InnoDB interprets foreign key constraint names that contain the string “tbl_name_ibfk_” as internally generated names.

Foreign key constraint names that point to the renamed table are automatically updated unless there is a conflict, in which case, the statement fails with an error. A conflict occurs if the renamed constraint name already exists. In such cases, you must drop and re-create the foreign keys in order for them to function properly. 
-------

Line 11590 in updatedatabase.pl renames table overduerules to old_overduerules, then it creates a new table overduerules.

The renaming action changed / renamed REFERENCES in constraints of overduerules_transport_types (maybe in other tables as well?).

That would explain that the error can not be reproduced on a fresh install (i.e. an install later than 3.23.00.007).
Comment 5 Aleisha Amohia 2017-09-06 20:44:59 UTC
Sorry I should clarify:
When I first filed this bug I thought the whole feature was broken, but I was wrong. The overdue rule does not save when you put letters in the delay field (rather than numbers) and there is no error message. Otherwise I am actually able to save overdue rules.
Comment 6 Jonathan Druart 2017-09-07 15:45:51 UTC
Marc,

% git checkout 0aa6ba1599bb0de923619a365f529119dc5c67a6 # DBrev 3.23.00.006
% reset_all

> SHOW CREATE TABLE overduerules_transport_types;
| overduerules_transport_types | CREATE TABLE `overduerules_transport_types` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `branchcode` varchar(10) COLLATE utf8_unicode_ci NOT NULL DEFAULT '',
  `categorycode` varchar(10) COLLATE utf8_unicode_ci NOT NULL DEFAULT '',
  `letternumber` int(1) NOT NULL DEFAULT '1',
  `message_transport_type` varchar(20) COLLATE utf8_unicode_ci NOT NULL DEFAULT 'email',
  PRIMARY KEY (`id`),
  KEY `overduerules_fk` (`branchcode`,`categorycode`),
  KEY `mtt_fk` (`message_transport_type`),
  CONSTRAINT `mtt_fk` FOREIGN KEY (`message_transport_type`) REFERENCES `message_transport_types` (`message_transport_type`) ON DELETE CASCADE ON UPDATE CASCADE,
  CONSTRAINT `overduerules_fk` FOREIGN KEY (`branchcode`, `categorycode`) REFERENCES `overduerules` (`branchcode`, `categorycode`) ON DELETE CASCADE ON UPDATE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci |

% git checkout master
% restart_all
% sudo koha-shell kohadev
% perl installer/data/mysql/updatedatabase.pl

> SHOW CREATE TABLE overduerules_transport_types;
| overduerules_transport_types | CREATE TABLE `overduerules_transport_types` (
  `id` int(11) NOT NULL AUTO_INCREMENT,
  `letternumber` int(1) NOT NULL DEFAULT '1',
  `message_transport_type` varchar(20) COLLATE utf8_unicode_ci NOT NULL DEFAULT 'email',
  `overduerules_id` int(11) NOT NULL,
  PRIMARY KEY (`id`),
  KEY `mtt_fk` (`message_transport_type`),
  KEY `overduerules_transport_types_fk` (`overduerules_id`),
  CONSTRAINT `mtt_fk` FOREIGN KEY (`message_transport_type`) REFERENCES `message_transport_types` (`message_transport_type`) ON DELETE CASCADE ON UPDATE CASCADE,
  CONSTRAINT `overduerules_transport_types_fk` FOREIGN KEY (`overduerules_id`) REFERENCES `overduerules` (`overduerules_id`) ON DELETE CASCADE ON UPDATE CASCADE
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci |


Without data it seems that the update works fine.

Maybe you got some errors during the upgrade? Is it a DB in production?
Comment 7 Jonathan Druart 2017-09-07 15:51:09 UTC
Seems ok with data too
Comment 8 Marc Véron 2017-09-07 16:14:25 UTC
(In reply to Jonathan Druart from comment #6)
(...)
> 
> Without data it seems that the update works fine.
> 
> Maybe you got some errors during the upgrade? Is it a DB in production?

Thanks for testing.

It's not a DB for production.
It's on a VM I installed a couple of years ago; best possible that while upgrading something went wrong.

Question: Is there any script that can be used to check database structure integrity? (E.g. Typo3 CMS has such script in it's install section)

...and let's not forget the problem reported by Aleisha (see comment 5)
Comment 9 Marc Véron 2017-09-08 09:32:19 UTC
- The problem described in comment #1 was related to a local database issue. I fixed the database structure, and I can no longer reproduce the problem (not saving Email and Print flags)

- It is possible to save changes with the exception of comment #5: 
  It does not save when you put letters in the delay field (rather than numbers)

- Changing importance to "normal"

- Changing
Comment 10 Marc Véron 2017-09-08 09:33:35 UTC
- Changing .... the title for better description
Comment 11 Marc Véron 2017-09-08 09:59:59 UTC
(In reply to Marc Véron from comment #4)
> Reading:
> https://dev.mysql.com/doc/refman/5.5/en/rename-table.html
> -------
> RENAME TABLE changes internally generated foreign key constraint names and
> user-defined foreign key constraint names that contain the string
> “tbl_name_ibfk_” to reflect the new table name. InnoDB interprets foreign
> key constraint names that contain the string “tbl_name_ibfk_” as internally
> generated names.
> 
> Foreign key constraint names that point to the renamed table are
> automatically updated unless there is a conflict, in which case, the
> statement fails with an error. A conflict occurs if the renamed constraint
> name already exists. In such cases, you must drop and re-create the foreign
> keys in order for them to function properly. 
> -------
> 
> Line 11590 in updatedatabase.pl renames table overduerules to
> old_overduerules, then it creates a new table overduerules.
> 
> The renaming action changed / renamed REFERENCES in constraints of
> overduerules_transport_types (maybe in other tables as well?).
> 
> That would explain that the error can not be reproduced on a fresh install
> (i.e. an install later than 3.23.00.007).

For the records: The database issue could be fixed with following hack:

mysql> show create table overduerules_transport_types;
(Verify that you see REFERENCES `old_overduerules`)

mysql> RENAME TABLE overduerules TO old_overduerules;
mysql> RENAME TABLE old_overduerules TO overduerules;

mysql> show create table overduerules_transport_types;
(Verify that REFERENCES has changed to REFERENCES `overduerules`)
Comment 12 Jonathan Druart 2017-09-27 16:11:13 UTC
Created attachment 67400 [details] [review]
Bug 19259: Enforce type of the delay value when editing overdue rules - template side only

Overdue rules do not save when a delay that is not an integer > 0,
without any warnings.
This patch forces the user to select a number > 0.

Test plan:
Without this patch applied, enter non-integer value or negative or 0
=> The rule will not be inserted without any warnings.
With this patch applied you will get an message from the browser and the
form will not save
Comment 13 David Bourgault 2017-09-29 18:18:27 UTC
Created attachment 67482 [details] [review]
Bug 19259: Enforce type of the delay value when editing overdue rules - template side only

Overdue rules do not save when a delay that is not an integer > 0,
without any warnings.
This patch forces the user to select a number > 0.

Test plan:
Without this patch applied, enter non-integer value or negative or 0
=> The rule will not be inserted without any warnings.
With this patch applied you will get an message from the browser and the
form will not save

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>
Comment 14 Nick Clemens 2017-10-11 00:24:02 UTC
Created attachment 67921 [details] [review]
Bug 19259: Enforce type of the delay value when editing overdue rules - template side only

Overdue rules do not save when a delay that is not an integer > 0,
without any warnings.
This patch forces the user to select a number > 0.

Test plan:
Without this patch applied, enter non-integer value or negative or 0
=> The rule will not be inserted without any warnings.
With this patch applied you will get an message from the browser and the
form will not save

Signed-off-by: David Bourgault <david.bourgault@inlibro.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Jonathan Druart 2017-10-11 16:54:18 UTC
Pushed to master for 17.11, thanks to everybody involved!