The patch for bug 9362 (which I did accept into master) introduces a MySQLism: Using HAVING without GROUP BY is a MySQLism, so at some point the query will have to become: SELECT issues.*, items.itype as itemtype, items.homebranch, TO_DAYS( date_due )-TO_DAYS( NOW() ) as days_until_due, branches.branchemail FROM issues LEFT JOIN items USING (itemnumber) LEFT OUTER JOIN branches USING (branchcode) WHERE returndate is NULL AND (TO_DAYS( date_due )-TO_DAYS( NOW() )) > 0 AND (TO_DAYS( date_due )-TO_DAYS( NOW() )) < ?; This is a pity, as it would be nice to be able to use "days_until_due" as an alias to make the query more readable, but it simply isn't portable.
Created attachment 20311 [details] [review] Bug 10606: Remove MySQLism in GetUpcomingDueIssues Removing this mysqlism as Galen proposed. Sponsored-by: Universidad Nacional de Cordoba
Created attachment 20333 [details] [review] Bug 10606: Remove MySQLism in GetUpcomingDueIssues To test: [1] Arrange to have at least one loan in your test database due one day from now. [2] Run misc/cronjobs/advance_notices.pl -c -n -v -m=2 and note the number of loans reported. [3] Apply the patch. [4] Run misc/cronjobs/advance_notices.pl -c -n -v -m=2 again and verify that the number of loans reported remains the same. Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Galen Charlton <gmc@esilibrary.com>
Tomás, It seems that TO_DAYS is a mysqlism. It is not in the postgres documentation http://www.postgresql.org/docs/9.2/static/functions-datetime.html
Created attachment 20403 [details] [review] Bug 10606: Remove MySQLism in GetUpcomingDueIssues To test: [1] Arrange to have at least one loan in your test database due one day from now. [2] Run misc/cronjobs/advance_notices.pl -c -n -v -m=2 and note the number of loans reported. [3] Apply the patch. [4] Run misc/cronjobs/advance_notices.pl -c -n -v -m=2 again and verify that the number of loans reported remains the same. Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Galen Charlton <gmc@esilibrary.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Kyle, did you seen my comment ? postgres=# select TO_DAYS( NOW() ); ERREUR: la fonction to_days(timestamp with time zone) n'existe pas LIGNE 1 : select TO_DAYS( NOW() ); In english: The do_days function does not exist.
Created attachment 20410 [details] [review] Bug 10606: Remove MySQLism in GetUpcomingDueIssues To test: [1] Arrange to have at least one loan in your test database due one day from now. [2] Run misc/cronjobs/advance_notices.pl -c -n -v -m=2 and note the number of loans reported. [3] Apply the patch. [4] Run misc/cronjobs/advance_notices.pl -c -n -v -m=2 again and verify that the number of loans reported remains the same. Sponsored-by: Universidad Nacional de Cordoba
New version passes tests form Bug 10719 too.
Created attachment 20422 [details] [review] Bug 10606: Remove MySQLism in GetUpcomingDueIssues To test: [1] Arrange to have at least one loan in your test database due one day from now. [2] Run misc/cronjobs/advance_notices.pl -c -n -v -m=2 and note the number of loans reported. [3] Apply the patch. [4] Run misc/cronjobs/advance_notices.pl -c -n -v -m=2 again and verify that the number of loans reported remains the same. Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com>
Created attachment 20610 [details] [review] [PASSED QA] Bug 10606: Remove MySQLism in GetUpcomingDueIssues To test: [1] Arrange to have at least one loan in your test database due one day from now. [2] Run misc/cronjobs/advance_notices.pl -c -n -v -m=2 and note the number of loans reported. [3] Apply the patch. [4] Run misc/cronjobs/advance_notices.pl -c -n -v -m=2 again and verify that the number of loans reported remains the same. Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> All tests and QA script pass. Also tested with unit tests from bug 10719.
The bug 10719 tests do not passed when using MariaDB: # Failed test 'Bug 9362: Only one item due in more than 2 days (3 days in advance)' # at t/db_dependent/Circulation.t line 321. # got: '0' # expected: '1' not ok 35 - Bug 9362: Only one item due in more than 2 days (4 days in advance) # Failed test 'Bug 9362: Only one item due in more than 2 days (4 days in advance)' # at t/db_dependent/Circulation.t line 321. # got: '0' # expected: '1' not ok 36 - Bug 9362: Only one item due in more than 2 days (5 days in advance) # Failed test 'Bug 9362: Only one item due in more than 2 days (5 days in advance)' # at t/db_dependent/Circulation.t line 321. # got: '0' # expected: '1' # Looks like you failed 3 tests of 36. The DATE() function returns a date, but it looks like DATEDIFF(DATE( date_due ),DATE( NOW() )) works but " DATE( date_due )-DATE( NOW() )" does not.
(In reply to Galen Charlton from comment #10) > The DATE() function returns a date, but it looks like DATEDIFF(DATE( > date_due ),DATE( NOW() )) works but " DATE( date_due )-DATE( NOW() )" does > not. I confirm they pass with MySQL and they don't pass with MariaDB. Using MariaDB: MariaDB [koha]> select to_days("2013-08-21 23:59:00")-to_days(NOW()); +-----------------------------------------------+ | to_days("2013-08-21 23:59:00")-to_days(NOW()) | +-----------------------------------------------+ | -6 | +-----------------------------------------------+ MariaDB [koha]> select date("2013-08-21 23:59:00")-date(NOW()); +-----------------------------------------+ | date("2013-08-21 23:59:00")-date(NOW()) | +-----------------------------------------+ | -6 | +-----------------------------------------+ Really odd...
Not sure what was wrong, but I have just retested to apply this patch and prove Circulation.t, and it returns green. Needs QA.
Created attachment 35154 [details] [review] Bug 10606: Remove MySQLism in GetUpcomingDueIssues To test: [1] Arrange to have at least one loan in your test database due one day from now. [2] Run misc/cronjobs/advance_notices.pl -c -n -v -m=2 and note the number of loans reported. [3] Apply the patch. [4] Run misc/cronjobs/advance_notices.pl -c -n -v -m=2 again and verify that the number of loans reported remains the same. Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> All tests and QA script pass. Also tested with unit tests from bug 10719.
Created attachment 35332 [details] [review] [PASSED QA] Bug 10606: Remove MySQLism in GetUpcomingDueIssues To test: [1] Arrange to have at least one loan in your test database due one day from now. [2] Run misc/cronjobs/advance_notices.pl -c -n -v -m=2 and note the number of loans reported. [3] Apply the patch. [4] Run misc/cronjobs/advance_notices.pl -c -n -v -m=2 again and verify that the number of loans reported remains the same. Sponsored-by: Universidad Nacional de Cordoba Signed-off-by: Jonathan Druart <jonathan.druart@biblibre.com> Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de> All tests and QA script pass. Also tested with unit tests from bug 10719. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Kyle, did you test with MariaDB?
Patch pushed to master.
I reverted the patch. There's no point breaking functionalities for something that should be handled in the ORM. Even having an IF switch for each engine...