Bug 10606 - MySQLism in GetUpcomingDueIssues
Summary: MySQLism in GetUpcomingDueIssues
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Galen Charlton
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-17 15:56 UTC by Galen Charlton
Modified: 2015-12-03 22:11 UTC (History)
2 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 10606: Remove MySQLism in GetUpcomingDueIssues (897 bytes, patch)
2013-08-13 13:59 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 10606: Remove MySQLism in GetUpcomingDueIssues (1.22 KB, patch)
2013-08-14 14:25 UTC, Galen Charlton
Details | Diff | Splinter Review
Bug 10606: Remove MySQLism in GetUpcomingDueIssues (1.27 KB, patch)
2013-08-16 12:35 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 10606: Remove MySQLism in GetUpcomingDueIssues (1.52 KB, patch)
2013-08-16 13:53 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 10606: Remove MySQLism in GetUpcomingDueIssues (1.58 KB, patch)
2013-08-16 15:22 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 10606: Remove MySQLism in GetUpcomingDueIssues (1.72 KB, patch)
2013-08-25 21:25 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 10606: Remove MySQLism in GetUpcomingDueIssues (1.71 KB, patch)
2015-01-13 12:50 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 10606: Remove MySQLism in GetUpcomingDueIssues (1.78 KB, patch)
2015-01-16 15:18 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Galen Charlton 2013-07-17 15:56:01 UTC
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.
Comment 1 Tomás Cohen Arazi 2013-08-13 13:59:00 UTC Comment hidden (obsolete)
Comment 2 Galen Charlton 2013-08-14 14:25:00 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2013-08-16 11:55:21 UTC
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
Comment 4 Kyle M Hall 2013-08-16 12:35:01 UTC Comment hidden (obsolete)
Comment 5 Jonathan Druart 2013-08-16 13:23:26 UTC
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.
Comment 6 Tomás Cohen Arazi 2013-08-16 13:53:55 UTC Comment hidden (obsolete)
Comment 7 Tomás Cohen Arazi 2013-08-16 13:55:24 UTC
New version passes tests form Bug 10719 too.
Comment 8 Jonathan Druart 2013-08-16 15:22:16 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2013-08-25 21:25:10 UTC Comment hidden (obsolete)
Comment 10 Galen Charlton 2013-08-26 15:10:15 UTC
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.
Comment 11 Jonathan Druart 2013-08-27 08:09:08 UTC
(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...
Comment 12 Jonathan Druart 2015-01-13 12:50:31 UTC
Not sure what was wrong, but I have just retested to apply this patch and prove Circulation.t, and it returns green.
Needs QA.
Comment 13 Jonathan Druart 2015-01-13 12:50:43 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2015-01-16 15:18:49 UTC
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>
Comment 15 Jonathan Druart 2015-01-16 15:26:05 UTC
Kyle, did you test with MariaDB?
Comment 16 Tomás Cohen Arazi 2015-01-21 14:20:10 UTC
Patch pushed to master.
Comment 17 Tomás Cohen Arazi 2015-03-30 16:16:24 UTC
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...