Bug 12167 - datetime type casting issue regarding bug 7567
Summary: datetime type casting issue regarding bug 7567
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Tools (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Mark Tompsett
QA Contact: Testopia
URL:
Keywords:
Depends on: 7567
Blocks:
  Show dependency treegraph
 
Reported: 2014-05-01 00:06 UTC by Mark Tompsett
Modified: 2022-06-06 20:24 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 12167 - Correct faulty test logic (1.44 KB, patch)
2014-05-01 03:17 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12167 - Patch disappearing news problem (1.73 KB, patch)
2014-05-01 03:17 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12167 - Invalid results check for opac_news test (1.87 KB, patch)
2014-07-28 01:50 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12167 - Bad expected results and condition check for GetNewsToDisplay (2.04 KB, patch)
2014-07-28 01:50 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12167 - Clean up GetNewsToDisplay (2.36 KB, patch)
2014-07-28 01:51 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12167 - Invalid results check for opac_news test (1.95 KB, patch)
2014-07-28 01:55 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12167 - Bad expected results and condition check for GetNewsToDisplay (1.92 KB, patch)
2014-07-28 01:55 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12167 - Clean up GetNewsToDisplay (2.36 KB, patch)
2014-07-28 01:55 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12167: Invalid results check for opac_news test (2.03 KB, patch)
2014-08-07 21:15 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12167: Bad expected results and condition check for GetNewsToDisplay (2.00 KB, patch)
2014-08-07 21:15 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 12167: Clean up GetNewsToDisplay (2.52 KB, patch)
2014-08-07 21:15 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 12167 - Made SQL even more ANSI SQL. (4.61 KB, patch)
2014-08-09 00:33 UTC, Mark Tompsett
Details | Diff | Splinter Review
[SIGNED-oFF] Bug 12167: Made SQL even more ANSI SQL. (4.74 KB, patch)
2014-08-10 16:23 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[PASSED QA] Bug 12167: Invalid results check for opac_news test (2.48 KB, patch)
2014-08-17 07:58 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12167: Bad expected results and condition check for GetNewsToDisplay (2.05 KB, patch)
2014-08-17 07:58 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12167: Clean up GetNewsToDisplay (2.57 KB, patch)
2014-08-17 07:58 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 12167: Made SQL even more ANSI SQL. (4.79 KB, patch)
2014-08-17 07:58 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 12167: Invalid results check for opac_news test (7.90 KB, patch)
2014-08-25 13:55 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12167: Invalid results check for opac_news test (2.47 KB, patch)
2015-04-30 14:27 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12167: Bad expected results and condition check for GetNewsToDisplay (2.04 KB, patch)
2015-04-30 14:28 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12167: Clean up GetNewsToDisplay (2.58 KB, patch)
2015-04-30 14:28 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 12167: Made SQL even more ANSI SQL. (4.89 KB, patch)
2015-04-30 14:28 UTC, Mark Tompsett
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Tompsett 2014-05-01 00:06:37 UTC
I just upgraded the mysql server on our live and test systems running Ubuntu, and bug 7567 stopped displaying news in the staff client. I was able to track it down to a type casting issue. A patch follows.
Comment 1 Mark Tompsett 2014-05-01 03:17:08 UTC Comment hidden (obsolete)
Comment 2 Mark Tompsett 2014-05-01 03:17:13 UTC Comment hidden (obsolete)
Comment 3 Bernardo Gonzalez Kriegel 2014-07-26 23:12:50 UTC
Tested on Ubuntu 12.04 and 14.04

Patch 1: for me is correct, but it pass before AND after.
Why it should fail?

Patch 2: why CAST? Usually I use date(timestamp) for this.

Anyway, on current master (and both Ubuntu vers) my news display normally.
Comment 4 Mark Tompsett 2014-07-27 01:08:21 UTC
Patch 1 - patch 1 the logic is wrong, this is a required fixed. It's broken logic that works without really testing what it should be.

Patch 2 - Since MySQL has updated again, it has since corrected itself on Ubuntu, but I haven't retested recently. So it may not be necessary. I'll retest this later.
Comment 5 Bernardo Gonzalez Kriegel 2014-07-27 01:41:10 UTC
(In reply to M. Tompsett from comment #4)
> Patch 1 - patch 1 the logic is wrong, this is a required fixed. It's broken
> logic that works without really testing what it should be.

I agree, and your solution is correct. 
My only point from your test plan is that it can't fail

> Patch 2 - Since MySQL has updated again, it has since corrected itself on
> Ubuntu, but I haven't retested recently. So it may not be necessary. I'll
> retest this later.

Again, I think is correct, or better than current code
I only mention that my news are displayed, and curious about the use of CAST.
Comment 6 Mark Tompsett 2014-07-27 03:04:32 UTC
CAST is ANSI SQL. I was trying to make sure to avoid MySQLisms.
Comment 7 Bernardo Gonzalez Kriegel 2014-07-27 20:37:48 UTC
Ok then.

Would you mind to change test plan on both patches?
I can sign them, but only as they change for good some
code. But I see nothing as described in current test plan.
Comment 8 Mark Tompsett 2014-07-28 01:50:44 UTC Comment hidden (obsolete)
Comment 9 Mark Tompsett 2014-07-28 01:50:52 UTC Comment hidden (obsolete)
Comment 10 Mark Tompsett 2014-07-28 01:51:06 UTC Comment hidden (obsolete)
Comment 11 Mark Tompsett 2014-07-28 01:55:19 UTC Comment hidden (obsolete)
Comment 12 Mark Tompsett 2014-07-28 01:55:23 UTC Comment hidden (obsolete)
Comment 13 Mark Tompsett 2014-07-28 01:55:28 UTC Comment hidden (obsolete)
Comment 14 Bernardo Gonzalez Kriegel 2014-08-07 21:15:37 UTC Comment hidden (obsolete)
Comment 15 Bernardo Gonzalez Kriegel 2014-08-07 21:15:45 UTC Comment hidden (obsolete)
Comment 16 Bernardo Gonzalez Kriegel 2014-08-07 21:15:52 UTC Comment hidden (obsolete)
Comment 17 Ludwin Hernández Vásquez 2014-08-08 17:50:09 UTC
It seems to be that the function your working with not avoid mysqlism.

I propose a bit of code that work with mysql and postgresql:

sub GetNewsToDisplay {
    my ($lang,$branch) = @_;
    my $dbh = C4::Context->dbh;
    # SELECT *,DATE_FORMAT(timestamp, '%d/%m/%Y') AS newdate
    my $query = q{
     SELECT *,opac_news.timestamp AS newdate
     FROM   opac_news
     WHERE   (
        expirationdate >= current_date
        OR    expirationdate IS NULL
        #OR    expirationdate = '00-00-0000' not proper for postgresql               
     )
     AND  CAST(opac_news.timestamp AS DATE) <= current_date
     AND   (lang = '' OR lang = ?)
     AND   (branchcode IS NULL OR branchcode = ?)
     ORDER BY number
    }; # expirationdate field is NOT in ISO format?
       # timestamp has HH:mm:ss, CURRENT_DATE generates 00:00:00
       #           by adding 1, that captures today correctly.
    my $sth = $dbh->prepare($query);
    $lang = $lang // q{};
    $sth->execute($lang,$branch);
    my @results;
    while ( my $row = $sth->fetchrow_hashref ){
        $row->{newdate} = format_date($row->{newdate});
        push @results, $row;
    }
    return \@results;
}

functions to substitute or remove
*OR expirationdate = '00-00-0000' not proper for postgresql           
*CURRENT_DATE() equivalent in postgresql current_date

kind regards! 

Ludwin Hernández Vásquez Universidad de El Salvador
Comment 18 Mark Tompsett 2014-08-08 23:50:52 UTC
(In reply to Ludwin Hernández Vásquez from comment #17)
> It seems to be that the function your working with not avoid mysqlism.
> 
> I propose a bit of code that work with mysql and postgresql:
[SNIP]
>      SELECT *,opac_news.timestamp AS newdate
>      FROM   opac_news
>      WHERE   (
>         expirationdate >= current_date
>         OR    expirationdate IS NULL
>         #OR    expirationdate = '00-00-0000' not proper for postgresql      
> 
>      )
>      AND  CAST(opac_news.timestamp AS DATE) <= current_date
>      AND   (lang = '' OR lang = ?)
>      AND   (branchcode IS NULL OR branchcode = ?)
>      ORDER BY number
>     }; # expirationdate field is NOT in ISO format?
>        # timestamp has HH:mm:ss, CURRENT_DATE generates 00:00:00
>        #           by adding 1, that captures today correctly.

[SNIP and REORDER]
> functions to substitute or remove
> *CURRENT_DATE() equivalent in postgresql current_date
1) current_date and CURRENT_DATE are equivalent in postgresql, as far as I can tell (http://www.postgresql.org/docs/8.1/static/functions-datetime.html -- See Table 9-26), though perhaps some SQLs may be case sensitive. It functions in postgresql just fine, as far as I can tell. Though, it does barf on CURRENT_DATE() with the ()'s, so I'll remove that.


> *OR expirationdate = '00-00-0000' not proper for postgresql

Okay, after an hour or two of trying to get postgresql installed and trying to mimick this type of query, I see the problem.

This would potentially need another patch to actually correct any historical mysql news by updating expirationdate=NULL where expirationdate='0000-00-00' in order to safely remove this. Though, I am surprised it functioned with '00-00-0000'. Anyways, I guess I'll add a follow up patch.
Comment 19 Mark Tompsett 2014-08-09 00:33:04 UTC Comment hidden (obsolete)
Comment 20 Bernardo Gonzalez Kriegel 2014-08-10 16:23:46 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2014-08-17 07:58:43 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2014-08-17 07:58:46 UTC Comment hidden (obsolete)
Comment 23 Katrin Fischer 2014-08-17 07:58:50 UTC Comment hidden (obsolete)
Comment 24 Katrin Fischer 2014-08-17 07:58:55 UTC Comment hidden (obsolete)
Comment 25 Tomás Cohen Arazi 2014-08-25 13:27:58 UTC
Hi, this patches don't apply anymore.

On rebasing, could we squash them, and explain better its purpose? It seems to me that it is a refactoring patch, so I expect to have regression tests for all stuff that gets touched.
Comment 26 Mark Tompsett 2014-08-25 13:55:27 UTC Comment hidden (obsolete)
Comment 27 Mark Tompsett 2014-08-26 16:24:39 UTC
Are per Tomas' suggestion, I fought a learning curve, and think I have DBIx code for GetNewsToDisplay. Still testing my code.
Comment 28 Mark Tompsett 2015-04-30 14:27:58 UTC
Created attachment 38721 [details] [review]
Bug 12167: Invalid results check for opac_news test

C4/NewsChannels.pm has a function GetNewsToDisplay at the end
of the file. It returns an array reference (i.e. one result).
t/db_dependent/NewsChannels.t has a test which expects two
values. This is likely a cut and paste error on my part.
Notice get_opac_news returns an array of two things.

TEST PLAN
---------
1) prove -v t/db_dependent/NewsChannels.t
   -- magically the wrong condition still passes.
2) apply this first patch only.
3) prove -v t/db_dependent/NewsChannels.t
   -- notice the second result value is UNDEFINED.
   -- notice the first result is an array reference.
   -- and the last test passes, because unless somehow the
      array reference was less than 2, it will pass.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Tested news expiration and publication dates:
- publication date today, expiration date tomorrow: shows
- publication date tomorrow, expiration date later: doesn't show
- publication date yesterday, expiration date today: shows
- publication date some time ago, expiration date yesterday: doesn't show
- publication date today, expiration date empty: shows

Passes all tests and QA script, no problems found.
Comment 29 Mark Tompsett 2015-04-30 14:28:05 UTC
Created attachment 38722 [details] [review]
Bug 12167: Bad expected results and condition check for GetNewsToDisplay

This corrected the results expected to a single array reference.
It then determines that it is defined and an array reference.
And lastly, correct the logic that was supposed to check for
valid results.

TEST PLAN
---------
1) Apply patch 1 and 2.
2) prove -v t/db_dependent/NewsChannels.t
   -- all tests should pass
   -- note the test changes to verify correctly what should
      be tested for.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 30 Mark Tompsett 2015-04-30 14:28:11 UTC
Created attachment 38723 [details] [review]
Bug 12167: Clean up GetNewsToDisplay

This bug started out as a result of busting while in transitions
of MySQL were happening in Ubuntu. However, I figured this fix
should remove backtick MySQLisms as per
http://wiki.koha-community.org/wiki/Coding_Guidelines#SQL6:_Backticks
And clean up GetNewsToDisplay a bit.

TEST PLAN
---------
1) News should function fine before applying any patches.
2) Apply all patches.
3) prove -v t/db_dependent/NewsChannels.t
   -- expecting all happy results.
   -- note the removal of backticks and the assumed typecasting
      of a timestamp to date.
   -- specified opac_news.timestamp to make sure that it would
      more likely parse as a fieldname and not as a keyword.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

All patches applied, test pass, cleaner code, no regressions found,
no koha-qa errors

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 31 Mark Tompsett 2015-04-30 14:28:16 UTC
Created attachment 38724 [details] [review]
Bug 12167: Made SQL even more ANSI SQL.

The comparison against '00-00-0000' is not possible under
PostgreSQL. By providing a database upgrade which replaces all
'0000-00-00' dates with NULL, this portion of the SQL query can
be removed.

Additionally, MySQL can handle CURRENT_DATE(), but PostgreSQL
barfs horribly. By removing the ()'s, it functions in both.

The NewsChannels.t specifically added an explanationdate IS NULL
case to catch what was not tested before.

TEST PLAN
---------
1) Apply patch.
2) ./installer/data/mysql/updatedatabase.pl
3) prove -v t/db_dependent/NewsChannels.t
   -- This will confirm that the modified GetNewsToDisplay
      function has not broken.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

Update database runs without problem, test pass, no koha-qa errors.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 32 Mark Tompsett 2015-04-30 14:37:23 UTC
I rebased this, even though I am still working on something based on comment #27.
Comment 33 Jonathan Druart 2021-08-19 08:34:44 UTC
No longer valid after bug 22544.