|
Description
Mark Tompsett
2014-05-01 00:06:37 UTC
Created attachment 27825 [details] [review] Bug 12167 - Correct faulty test logic GetNewsToDisplay returns a reference to an array of hashes. Of course the hash >= 2. This patch properly checks the scalar size of the array. TEST PLAN --------- 1) prove -v t/db_dependent/NewsChannels.t -- should "work" 2) apply the patch 3) prove -v t/db_dependent/NewsChannels.t -- should "fail" This is in preparation for the next patch. NOTE: I believe this may only work under Ubuntu 12.04 Created attachment 27826 [details] [review] Bug 12167 - Patch disappearing news problem This doesn't duplicate on Debian, but it does under Ubuntu. TEST PLAN --------- 1) Update your git master to current. 2) Log into your staff client. 3) The news may have disappeared. -- Make sure to have news entered. The problem portion is the timestamp and date+1 logic don't seem to function properly. 4) Apply the first patch. 5) prove -v t/db_dependent/NewsChannels.t -- It should "fail". 6) Apply the second patch. 7) The news should reappear. -- There is no apparent change under Debian. 8) prove -v t/db_dependent/NewsChannels.t -- It should now "work". 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. 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. (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. CAST is ANSI SQL. I was trying to make sure to avoid MySQLisms. 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. Created attachment 30171 [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. Created attachment 30172 [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. Created attachment 30173 [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. Created attachment 30174 [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. Created attachment 30175 [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. Created attachment 30176 [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. Created attachment 30599 [details] [review] [SIGNED-OFF] 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> Created attachment 30600 [details] [review] [SIGNED-OFF] 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> Created attachment 30601 [details] [review] [SIGNED-OFF] 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 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
(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. Created attachment 30642 [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. Created attachment 30663 [details] [review] [SIGNED-oFF] 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. Created attachment 30882 [details] [review] [PASSED QA] 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. Created attachment 30883 [details] [review] [PASSED QA] 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> Created attachment 30884 [details] [review] [PASSED QA] 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> Created attachment 30885 [details] [review] [PASSED QA] 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> 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. Created attachment 31139 [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> 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> 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 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. Are per Tomas' suggestion, I fought a learning curve, and think I have DBIx code for GetNewsToDisplay. Still testing my code. 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. 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> 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> 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> I rebased this, even though I am still working on something based on comment #27. No longer valid after bug 22544. |