Bug 13409 - Add ability to purge old special holidays using cleanup_database.pl
Summary: Add ability to purge old special holidays using cleanup_database.pl
Status: RESOLVED DUPLICATE of bug 16672
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 10744
  Show dependency treegraph
 
Reported: 2014-12-08 15:32 UTC by Kyle M Hall
Modified: 2017-07-25 14:32 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 13409 - Add ability to purge old special holidays using cleanup_database.pl (3.90 KB, patch)
2014-12-08 15:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13409 - Add ability to purge old special holidays using cleanup_database.pl v2 (4.34 KB, patch)
2015-10-19 11:47 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13409 - Add ability to purge old special holidays using cleanup_database.pl (5.15 KB, patch)
2015-10-19 18:26 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13409 [QA Followup] - Remove MySQLism (2.11 KB, patch)
2015-10-19 18:27 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 13409 - Add ability to purge old special holidays using cleanup_database.pl (5.22 KB, patch)
2015-10-21 13:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13409 [QA Followup] - Remove MySQLism (2.18 KB, patch)
2015-10-21 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13409: Remove MySQLism - Replace DATE with CAST AS DATE (958 bytes, patch)
2015-10-21 13:32 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2014-12-08 15:32:28 UTC
Some libraries have expressed interested in being able to purge old holidays from the database.
Comment 1 Kyle M Hall 2014-12-08 15:34:16 UTC Comment hidden (obsolete)
Comment 2 Aleisha Amohia 2015-04-24 03:34:05 UTC
Patch seems to work cleanly, signing off.
Comment 3 Jonathan Druart 2015-05-24 14:09:11 UTC
Comment on attachment 34177 [details] [review]
Bug 13409 - Add ability to purge old special holidays using cleanup_database.pl

Review of attachment 34177 [details] [review]:
-----------------------------------------------------------------

::: misc/cronjobs/cleanup_database.pl
@@ +340,5 @@
> +sub PurgeHolidays {
> +    $sth = $dbh->prepare(
> +        q{
> +            DELETE FROM special_holidays
> +            WHERE DATE( CONCAT( year, '-', month, '-', day ) ) < DATE_SUB( CURDATE(), INTERVAL ? DAY )

It would be good to have a DB agnostic way to day that.
For instance "DATE_SUB( CURDATE(), INTERVAL ? DAY )" could be done in Perl, don't you think?
Comment 4 Olli-Antti Kivilahti 2015-10-19 11:47:39 UTC Comment hidden (obsolete)
Comment 5 Olli-Antti Kivilahti 2015-10-19 11:50:40 UTC
Hi there!

I accidentally implemented this simple fix and found Kyle's patch later.
Sharing my solution regardless.

My solution actually could be tested if somebody decided to write tests for it.

Not implementing this with DBIx because it is very counterproductive and I don't feel like spending a day doing an 2-hour job.

Failed QA for both commits due to a lack of tests.
Comment 6 Kyle M Hall 2015-10-19 13:41:13 UTC
Thanks Olli! I do like your version ( except for the odd default value ; ). However it does break from the standard way of doing things in cleanup_database. That being said, if you *were* to add a unit test I'd say that would be a worthwhile improvement and I'd be more than happy to deprecate my version and sign off on yours!
Comment 7 Kyle M Hall 2015-10-19 18:26:59 UTC
Created attachment 43608 [details] [review]
Bug 13409 - Add ability to purge old special holidays using cleanup_database.pl

Some libraries have expressed interested in being able to purge old
holidays from the database.

Test Plan:
1) Create some special holidays in the past
2) Use the new --holidays DAYS option to test deleting special holidays
3) Verify only special holidays past the given amount of days were deleted
Comment 8 Kyle M Hall 2015-10-19 18:27:05 UTC
Created attachment 43609 [details] [review]
Bug 13409 [QA Followup] - Remove MySQLism
Comment 9 Jonathan Druart 2015-10-21 13:31:59 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2015-10-21 13:32:02 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2015-10-21 13:32:08 UTC
Created attachment 43695 [details] [review]
Bug 13409: Remove MySQLism - Replace DATE with CAST AS DATE

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 12 Olli-Antti Kivilahti 2015-10-21 13:44:20 UTC
(In reply to Kyle M Hall from comment #6)
> Thanks Olli! I do like your version ( except for the odd default value ; ).
Heh, you should see the odd default values in acquisitions.module.
(if don't know what to do order, order one piece of this Biblio)

> However it does break from the standard way of doing things in
> cleanup_database.
Erm... The cleanup_database is from the 1990's based on the programming patterns. Why do you need to follow such a standard? Is it too much to ask to expose business logic inside the core API rather than access DB directly from the Controller/script?
This is the root of many issues in Koha and we need to break that crappy cycle of untestable/automatable features.

> That being said, if you *were* to add a unit test I'd say
> that would be a worthwhile improvement and I'd be more than happy to
> deprecate my version and sign off on yours!

Well now nobody can test it, unless they refactor the feature by moving the business logic inside the API.
This modification would need to pass yet another community process.
Comment 13 Olli-Antti Kivilahti 2015-10-21 13:48:54 UTC
I could still fail QA due to missing tests.
But back then nobody wrote test, so according to the spirit of cleanup_database.pl this behaviour is acceptable.
Comment 14 Jonathan Druart 2015-10-21 13:51:38 UTC
Ok, removing my signoff, obsoleting patches.
Please (someone) move the sql to a pm and write tests.

Marked as Failed QA.

(Everybody's happy?)
Comment 15 Olli-Antti Kivilahti 2015-10-22 08:11:05 UTC
No.
I think everybody is really pissed off :)

But I guess we first need to get really pissed and then start thinking on how we can improve ourselves/statuq quo.
Comment 16 Olli-Antti Kivilahti 2015-11-05 09:19:59 UTC
Hi!

I just implemented Bug 10744.
This created special holidays for Koha::Calendar to use with the test cases.
Oh boy how much fun it would be to be able to remove special holidays calling an internal module instead of writing direct SQL-code in the test script.

I wonder where could we find one patch which would introduce such an internal API call?

You guys have so many years of Koha development under your hood, I am sure you both know what is best for Koha, and won't fail to keep developing Koha, just as it was developed 10 years ago.
Comment 17 Katrin Fischer 2016-12-27 15:42:20 UTC
Can we get this moving again?
Comment 18 Fridolin Somers 2017-07-25 14:32:33 UTC

*** This bug has been marked as a duplicate of bug 16672 ***