I can't seem to add a new subscription without filling in every. single. box. As an example, I created a subscription with no vendor, a biblio, no special anything, and I got an error Can't call method "set_additional_fields" on an undefined value at /kohadevbox/koha/serials/subscription-add.pl line 401 there are also sql errors when you don't fill in various boxes: Incorrect integer value: '' for column 'aqbooksellerid' at row 1 if you don't give a vendor Incorrect integer value: '' for column 'innerloop1' at row 1 if you don't use a prediction pattern that fills all boxes x y z My version of sql is 5.5.62 and this was in a docker test setup.
This is probably to do with strict mode, as KohaDevBox doesn't seem to manifest this behaviour. Off of blocker because most people have already turned off strict mode if they are using mysql, but it's still pretty major because we can't add subscriptions in strict mode and that seems wrong.
Created attachment 89391 [details] [review] Bug 22812: Fix new subscription with strict sql modes With sql_strict_modes turned on, if you try to create a new subscription you will get: Incorrect integer value: '' for column '$INT_COLUMN' at row 1
Note that now there is the following error in the log: DBD::mysql::st execute failed: Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause [for Statement "SELECT count(routingid) routingids FROM subscription LEFT JOIN subscriptionroutinglist but does not explode the interface.
Created attachment 89392 [details] [review] Bug 22812: Add tests for checkrouting
Created attachment 89393 [details] [review] Bug 22812: Fix GROUP BY in checkrouting On the way, let fix this error DBD::mysql::st execute failed: Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause [for Statement "SELECT count(routingid) routingids FROM subscription LEFT JOIN subscriptionroutinglist Test plan: Create a routing list, search for subscriptions In the action list you should see "Edit routing list (1)" or "New routing list"
Created attachment 89454 [details] [review] Bug 22812: Fix new subscription with strict sql modes With sql_strict_modes turned on, if you try to create a new subscription you will get: Incorrect integer value: '' for column '$INT_COLUMN' at row 1 Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 89455 [details] [review] Bug 22812: Add tests for checkrouting Signed-off-by: Liz Rea <wizzyrea@gmail.com>
Created attachment 89456 [details] [review] Bug 22812: Fix GROUP BY in checkrouting On the way, let fix this error DBD::mysql::st execute failed: Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause [for Statement "SELECT count(routingid) routingids FROM subscription LEFT JOIN subscriptionroutinglist Test plan: Create a routing list, search for subscriptions In the action list you should see "Edit routing list (1)" or "New routing list" Signed-off-by: Liz Rea <wizzyrea@gmail.com>
+ $_ ||= undef # Set to undef for integer values, not empty string + for ( + $aqbooksellerid, $lastvalue1, $innerloop1, $lastvalue2, + $innerloop2, $lastvalue3, $innerloop3, + ); This also sets 0 back to undef. The comment is very confusing about what it does. Fortunately, integer values like 1 are not set to undef.
SELECT count(routingid) routingids FROM subscription LEFT JOIN subscriptionroutinglist ON subscription.subscriptionid = subscriptionroutinglist.subscriptionid order by ranking asc; does not raise a warning with me on mariadb with sql mode: STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION Do I overlook a setting?
Created attachment 89535 [details] [review] Bug 22812: Fix new subscription with strict sql modes With sql_strict_modes turned on, if you try to create a new subscription you will get: Incorrect integer value: '' for column '$INT_COLUMN' at row 1 Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 89536 [details] [review] Bug 22812: Add tests for checkrouting Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 89537 [details] [review] Bug 22812: Fix GROUP BY in checkrouting On the way, let fix this error DBD::mysql::st execute failed: Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause [for Statement "SELECT count(routingid) routingids FROM subscription LEFT JOIN subscriptionroutinglist Test plan: Create a routing list, search for subscriptions In the action list you should see "Edit routing list (1)" or "New routing list" Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
QA Comment: Bug apparently solved. Some minor questions, no blockers.
(In reply to Marcel de Rooy from comment #9) > + $_ ||= undef # Set to undef for integer values, not empty string > + for ( > + $aqbooksellerid, $lastvalue1, $innerloop1, $lastvalue2, > + $innerloop2, $lastvalue3, $innerloop3, > + ); > > This also sets 0 back to undef. You are right, and I think it should be followed-up before pushed. I initially write it for aqbooksellerid, and so wanted to catch 0. I reused the same logic for the other errors (I was not expecting to have them initially).
(In reply to Marcel de Rooy from comment #10) > SELECT count(routingid) routingids FROM subscription LEFT JOIN > subscriptionroutinglist ON subscription.subscriptionid = > subscriptionroutinglist.subscriptionid order by ranking asc; > > does not raise a warning with me on mariadb with sql mode: > STRICT_TRANS_TABLES,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER, > NO_ENGINE_SUBSTITUTION > > Do I overlook a setting? MariaDB [koha_kohadev]> SET sql_mode = 'ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE,ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION'; Query OK, 0 rows affected (0.01 sec) MariaDB [koha_kohadev]> SELECT count(routingid) routingids FROM subscription LEFT JOIN subscriptionroutinglist ON subscription.subscriptionid = subscriptionroutinglist.subscriptionid WHERE subscription.subscriptionid = 1 ORDER BY ranking ASC; ERROR 1140 (42000): Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause
Created attachment 89561 [details] [review] Bug 22812: Use Koha::Subscription in NewSubscription Starting to write tests I realize that we are going to add too much specific logic which is already handled in Koha::Object->store. The easiest and safe way is to use it :)
Maybe I went a bit too far with this last patch, but it's definitely the way to go. Sending back to NSO as I rewrote the original approach.
Created attachment 89570 [details] [review] Bug 22812: Fix new subscription with strict sql modes With sql_strict_modes turned on, if you try to create a new subscription you will get: Incorrect integer value: '' for column '$INT_COLUMN' at row 1 Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 89571 [details] [review] Bug 22812: Add tests for checkrouting Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 89572 [details] [review] Bug 22812: Fix GROUP BY in checkrouting On the way, let fix this error DBD::mysql::st execute failed: Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause [for Statement "SELECT count(routingid) routingids FROM subscription LEFT JOIN subscriptionroutinglist Test plan: Create a routing list, search for subscriptions In the action list you should see "Edit routing list (1)" or "New routing list" Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 89573 [details] [review] Bug 22812: Use Koha::Subscription in NewSubscription Starting to write tests I realize that we are going to add too much specific logic which is already handled in Koha::Object->store. The easiest and safe way is to use it :) Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com>
Created attachment 89590 [details] [review] Bug 22812: Fix new subscription with strict sql modes With sql_strict_modes turned on, if you try to create a new subscription you will get: Incorrect integer value: '' for column '$INT_COLUMN' at row 1 Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 89591 [details] [review] Bug 22812: Add tests for checkrouting Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 89592 [details] [review] Bug 22812: Fix GROUP BY in checkrouting On the way, let fix this error DBD::mysql::st execute failed: Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with no GROUP columns is illegal if there is no GROUP BY clause [for Statement "SELECT count(routingid) routingids FROM subscription LEFT JOIN subscriptionroutinglist Test plan: Create a routing list, search for subscriptions In the action list you should see "Edit routing list (1)" or "New routing list" Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 89593 [details] [review] Bug 22812: Use Koha::Subscription in NewSubscription Starting to write tests I realize that we are going to add too much specific logic which is already handled in Koha::Object->store. The easiest and safe way is to use it :) Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Michal Denar <black23@gmail.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
(In reply to Jonathan Druart from comment #16) > MariaDB [koha_kohadev]> SET sql_mode = > 'ONLY_FULL_GROUP_BY,STRICT_TRANS_TABLES,NO_ZERO_IN_DATE,NO_ZERO_DATE, > ERROR_FOR_DIVISION_BY_ZERO,NO_AUTO_CREATE_USER,NO_ENGINE_SUBSTITUTION'; > Query OK, 0 rows affected (0.01 sec) > > MariaDB [koha_kohadev]> SELECT count(routingid) routingids FROM subscription > LEFT JOIN subscriptionroutinglist ON subscription.subscriptionid = > subscriptionroutinglist.subscriptionid WHERE subscription.subscriptionid = 1 > ORDER BY ranking ASC; > ERROR 1140 (42000): Mixing of GROUP columns (MIN(),MAX(),COUNT(),...) with > no GROUP columns is illegal if there is no GROUP BY clause OK Confirmed
Awesome work all! Pushed to master for 19.05
Pushed to 18.11.x for 18.11.06