Bug 8991 - Add a script to delete old orders
Summary: Add a script to delete old orders
Status: CLOSED WONTFIX
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-31 09:30 UTC by Jonathan Druart
Modified: 2016-06-21 21:36 UTC (History)
9 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 8991: New cronjobs script: purge_orders (3.79 KB, patch)
2012-10-31 09:34 UTC, Jonathan Druart
Details | Diff | Splinter Review
[Signed off] Bug 8991: New cronjobs script: purge_orders (3.88 KB, patch)
2013-02-15 15:39 UTC, Melia Meggs
Details | Diff | Splinter Review
Bug 8991: New cronjobs script: purge_orders (4.03 KB, patch)
2013-06-03 07:13 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 8991: New cronjobs script: purge_orders (6.04 KB, patch)
2013-06-13 14:29 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 Jonathan Druart 2012-10-31 09:30:18 UTC
We need a script to delete old orders
Comment 1 Jonathan Druart 2012-10-31 09:34:28 UTC Comment hidden (obsolete)
Comment 2 claire.hernandez@biblibre.com 2012-11-07 13:29:49 UTC
We are currently working on this patch, please do not consider this submission for the moment
Comment 3 Jonathan Druart 2013-02-14 09:23:19 UTC
Switch back to needs signoff.

This script permits to delete orders which have been received before a given date.
See the help for more details.
Comment 4 Melia Meggs 2013-02-15 15:35:24 UTC
Before running the purge_orders script, I checked what orders I had.

mysql> SELECT * FROM aqorders WHERE datereceived IS NOT NULL\G;

I found 4 orders with the following datereceived:
2012-10-13
2012-10-13
2012-11-29
2012-11-30


Then I ran purge_orders.pl -until 2012-11-01

mysql> SELECT * FROM aqorders WHERE datereceived IS NOT NULL\G;

Now I found 2 orders with the following datereceived:
2012-11-29
2012-11-30


So it would appear that everything with datereceived before 2012-11-01 has been correctly purged, and I'm signing off.
Comment 5 Melia Meggs 2013-02-15 15:39:04 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2013-06-03 07:00:09 UTC
I had a warning:
Use of uninitialized value in concatenation (.) or string at misc/cronjobs/purge_orders.pl line 67.

Probably related to a deleted biblio (order with biblionumber null)? You could add a check.
Comment 7 Marcel de Rooy 2013-06-03 07:11:48 UTC
QA Comment:
Code (on itself) looks good to me. No qa-tools complaints.
Also the need for a job to optionally delete acquisition records is recognized.

I only wonder if you should only delete from aqorders and if receiveddate should be the date to trigger the action.
The following thoughts are not blocking this patch, but just raise a discussion about it:
Why not delete baskets? Now we can be stuck with empty baskets. Would you really want to delete orders from an open basket or a basket that has not been received fully?
Personally, I would go for a job that deletes inactive baskets (closed and all received) with creationdate as trigger. E.g. I could annually delete all inactive baskets, created more than one year ago or something.

In the hope that these considerations may stimulate further improvements and since it only is one new script, passing qa.
Comment 8 Marcel de Rooy 2013-06-03 07:13:02 UTC Comment hidden (obsolete)
Comment 9 Galen Charlton 2013-06-03 15:17:27 UTC
(In reply to comment #7)
> Personally, I would go for a job that deletes inactive baskets (closed and
> all received) with creationdate as trigger. E.g. I could annually delete all
> inactive baskets, created more than one year ago or something.

Libraries I'm familiar with, if they purged old orders at all (and IMO, one should think twice about doing so anyway because of the potential value of data-mining past acquisitions activity), would do so only in units of fiscal years.  I.e., one might choose to delete all closed orders for the 2011 fiscal year, but depending on how the library handles ordering and receiving at the beginning and end of the fiscal year, that may not actually map cleanly to a specific cutoff date for receiving.

I agree that if one is deleting from acqorders, one should also delete the relevant baskets.

> In the hope that these considerations may stimulate further improvements and
> since it only is one new script, passing qa.

I have some reservations about the existence of what could be a pretty major foot-gun.  For example, imagine what would happen if one's fingers slipped when entering the date; if you blew away closed orders from the current fiscal year, thereby throwing your fund balances out of whack, there's no recourse except to restore from backup.

The original bug report said:

> We need a script to delete old orders

To which I ask: why?  Is there a specific way in which the old orders are getting in the way?  Is there a regulatory requirement that's satisfied by purging old orders?

I'm not trying to be difficult, but I do think that sometimes it's better for this sort of maintenance to be done via SQL rather than a script.  At least when you're staring at the 'mysql>' prompt, you likely know that you're about to do something potentially dangerous.

I suggest that the purge job should produce an output file with the complete contents of the rows it deletes.

All of that said, ultimately, somebody who runs a script called 'purge_orders.pl' presumably will have an idea of what they're in for, so I will push this patch in a day or two, though with a follow-up to add a dry run mode.  However, I would appreciate it if the original patch submitter could respond to Marcel's and my questions and comments.
Comment 10 Galen Charlton 2013-06-03 15:29:08 UTC
Also, I question the notion of this script existing as a _cronjob_.  Is the idea really to run it automatically?

I think misc/maintenance is a better home for this script.
Comment 11 Jonathan Druart 2013-06-13 14:26:35 UTC
(In reply to comment #7)

Hi Marcel and Galen,

> I only wonder if you should only delete from aqorders and if receiveddate
> should be the date to trigger the action.

It is the request from our customer.

> Why not delete baskets? Now we can be stuck with empty baskets. Would you
> really want to delete orders from an open basket or a basket that has not
> been received fully?

I will provide a new patch in order to delete empty closed baskets.

> Personally, I would go for a job that deletes inactive baskets (closed and
> all received) with creationdate as trigger. E.g. I could annually delete all
> inactive baskets, created more than one year ago or something.

It could be a great improvement. 

(In reply to comment #9)
> I agree that if one is deleting from acqorders, one should also delete the
> relevant baskets.

Yep, will be done.

> The original bug report said:
> > We need a script to delete old orders
> To which I ask: why?  Is there a specific way in which the old orders are
> getting in the way?  Is there a regulatory requirement that's satisfied by
> purging old orders?

It is a need for some customers to delete old orders. I suppose it is to avoid to have too many orders. After a time, it becomes useless to keep all of them.

> I'm not trying to be difficult, but I do think that sometimes it's better
> for this sort of maintenance to be done via SQL rather than a script.  At
> least when you're staring at the 'mysql>' prompt, you likely know that
> you're about to do something potentially dangerous.

Yes but with a cronjob, it is easily to call a script rather than execute a sql query.

> I suggest that the purge job should produce an output file with the complete
> contents of the rows it deletes.

The following patch will add a 'confirm' flag. Maybe it fixes this problem?

> All of that said, ultimately, somebody who runs a script called
> 'purge_orders.pl' presumably will have an idea of what they're in for, so I
> will push this patch in a day or two, though with a follow-up to add a dry
> run mode.  However, I would appreciate it if the original patch submitter
> could respond to Marcel's and my questions and comments.

I hope I answered to all of your questions.

(In reply to comment #10)
> Also, I question the notion of this script existing as a _cronjob_.  Is the
> idea really to run it automatically?
> 
> I think misc/maintenance is a better home for this script.

The idea is to run the job all nights (or weeks/months/...), so yes it is a cronjob.
Comment 12 Jonathan Druart 2013-06-13 14:29:26 UTC
Created attachment 18941 [details] [review]
Bug 8991: New cronjobs script: purge_orders

The script takes a '-until' parameter and deletes all orders
received before this date.
If baskets exist without order, they will delete too.
If the script is called without the confirm flag, nothing is done and
the output contains all actions to do.

Execute
  purge_orders.pl -h
for more details.
Comment 13 Jonathan Druart 2013-06-13 14:30:10 UTC
Since I completely rewrite this patch, I switch back to "Needs Signoff".
Comment 14 Katrin Fischer 2013-06-16 08:07:17 UTC
This script is potentially dangerous as it deletes data in an unrecoverable way.

Maybe we should have a very clear warning about the consequences of this in the script before you can run it.  

Deleting orders and baskets has some consequences like losing the acquisition history shown in the items tab in staff detail page. 

Also if you are not careful with the date, you will mess up the amounts in your funds and budgets for a fiscal year. Really bad.

I never had this request from one of our libraries and I can't help but wonder why you wouldn't want to keep the data. I wonder if it's display problems being the reason for this request or some law requirement?
Comment 15 Jonathan Druart 2013-06-17 09:28:02 UTC
(In reply to comment #14)
> This script is potentially dangerous as it deletes data in an unrecoverable
> way.
> 
> Maybe we should have a very clear warning about the consequences of this in
> the script before you can run it.  
> 
> Deleting orders and baskets has some consequences like losing the
> acquisition history shown in the items tab in staff detail page. 
> 
> Also if you are not careful with the date, you will mess up the amounts in
> your funds and budgets for a fiscal year. Really bad.

A confirm flag is needed for deleting records. Without it, a list of orders and baskets is displayed.

> I never had this request from one of our libraries and I can't help but
> wonder why you wouldn't want to keep the data. I wonder if it's display
> problems being the reason for this request or some law requirement?

I don't know, I asked Mathieu Saby to explain why this script is useful for them.
Comment 16 Mathieu Saby 2013-06-17 09:40:04 UTC
Hello
It was made for us, but before my arrival in Rennes 2, so I don't have all the informations. I will ask our team.

Mathieu
Comment 17 I'm just a bot 2013-10-24 19:26:09 UTC
Patch applied cleanly, go forth and signoff
Comment 18 Marcel de Rooy 2014-02-07 21:06:04 UTC
Note: get_baskets_to_delete will not return all deletable baskets in dry-run mode. Not sure if you should really need that, but the information now is not accurate..
Comment 19 Marcel de Rooy 2014-02-10 09:57:08 UTC
(In reply to M. de Rooy from comment #18)
> Note: get_baskets_to_delete will not return all deletable baskets in dry-run
> mode. Not sure if you should really need that, but the information now is
> not accurate..

I reread one of my earlier comments on this report.
I still prefer to delete in terms of completed baskets than completed orders. Actually the Koha basket is "the order" and the Koha order is "an order line".
If all order lines have been processed and pass the date filter, the basket could be deleted via DelBasket with a cascading effect to aqorders and aqorders_items.

I agree with Galen for the folder. I would also suggest to move it to misc/maintenance. No problem to run a cron job from there, but most people will probably run this at most annually and manually :)

In order to meet the concerns raised about deleting valuable acquisition history, what do you think about adding a few lines that copy aqbasket, aqorders and aqorders_items for the selected baskets into some [temporary] archive tables and dumping these tables to file? The archive and sqldump steps could be parametrized.
Comment 20 David Cook 2014-03-26 07:30:35 UTC
I agree with Galen and Katrin. I don't think that many libraries would find this script useful. 

In fact, from a regulatory financial perspective, it could be extremely problematic. Libraries aren't always known for keeping the best accounting records, so acquisitions data is often the only record. If it disappears, there could be trouble as one of the key components of the audit trail vanishes.

From an archival/records management perspective, it doesn't make sense to erase these records. These data are records. If they aren't 'active' or relevant anymore, they should be exported into some type of "semi-active" or "inactive" state. As Galen suggests, an output of the purge should be kept.

Going back to the regulatory perspective, when the time comes to officially dispose of the records, the archived output can be deleted.

I think that probably makes the most sense. Of course, I might be biased, as I'm the one saying it :p.

I could maybe see this being set up as a cronjob on an instance by instance basis, but definitely not setup as a default cronjob. I imagine there would be quite a few furious acquisitions librarians otherwise.
Comment 21 Paola Rossi 2014-06-05 15:57:58 UTC
I apply against master 3.17.00.007

in comment 16:

"A confirm flag is needed for deleting records. Without it, a list of orders and baskets is displayed."

Without "-c" option set, the script makes some anticipations.
The script counts orders to delete (completely received orders) and baskets to delete (basket without orders). But, counting the baskets, the script doesn't count the baskets whose orders, being completely received, are all eraseable.
So after the first run without "-c" option set, the following execution with "-c" set will be able to delete some baskets: this is unexpected!

NB1. The script doesn't delete orders received _before_ some date, but _until_ some date. The day in the "-until" option is included.
NB2. There is no way to ask for deleting only orders and not the baskets. There is no way to ask for deleting only empty baskets.

I pass the patch to "Failed QA" status.