Created attachment 95977 [details] [review] Bug 24152: Add method Koha::Objects->filter_by_last_update
Created attachment 95978 [details] [review] Bug 24152: Add a warning about the delete of statistics's table entries
Created attachment 95979 [details] [review] Bug 24152: Add the ability to purge pseudonymized tables anonymized_* tables have been added by bug 24151, this patch adds the usual way to purge data from them. The cleanup_database.pl script has been adjusted to take new parameters that will help to delete pseudonymized data. Test plan: Call the scrip with the new parameter to remove pseudonymized data * --pseudo-patrons DAYS will remove entries from anonymized_borrowers older than DAYS day * --pseudo-patrons can be used without the parameter DAYS but with -- pseudo-patrons-from and/or --pseudo-patrons-to instead, to provide a range of date Same for --pseudo-transactions, see the POD of the script. You can use the patch from bug 24153 to make the tests easier, data will not be deleted if the new --confirm flag is not passed. Sponsored-by: Association KohaLa - https://koha-fr.org/
I'd like to see tests for the new `filter_by_last_update` method added to Koha::Objects (and really, I'd love to see that in it's own mini bug for fast QA and push in the future.. smaller bugs in a tree means others can benefit from such nice little additions more quickly)
Created attachment 97253 [details] [review] Bug 24152: Add method Koha::Objects->filter_by_last_update
Created attachment 97254 [details] [review] Bug 24152: Add a warning about the delete of statistics's table entries
Created attachment 97255 [details] [review] Bug 24152: Add the ability to purge pseudonymized tables anonymized_* tables have been added by bug 24151, this patch adds the usual way to purge data from them. The cleanup_database.pl script has been adjusted to take new parameters that will help to delete pseudonymized data. Test plan: Call the scrip with the new parameter to remove pseudonymized data * --pseudo-transactions DAYS will remove entries from pseudonymized_transactions older than DAYS day * --pseudo-transactions can be used without the parameter DAYS but with -- pseudo-transactions-from and/or --pseudo-transactions-to instead, to provide a range of date You can use the patch from bug 24153 to make the tests easier, data will not be deleted if the new --confirm flag is not passed. Sponsored-by: Association KohaLa - https://koha-fr.org/
I've tested for Association KohaLa. The data in the tables pseudonymized_transactions and pseudonymized_borrower_attributes are correctly deleted when using --pseudo-transactions flag for cleanup_database.pl I tried it with a number of days and also with the choice of dates : --pseudo-transactions --pseudo-transactions-from YYYY-MM-DD --pseudo-transactions-to YYYY-MM-DD and both work correctly. Thanks! Sonia BOUIS
Created attachment 101297 [details] [review] Bug 24152: Add method Koha::Objects->filter_by_last_update Signed-off-by: Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 101298 [details] [review] Bug 24152: Add a warning about the delete of statistics's table entries Signed-off-by: Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 101299 [details] [review] Bug 24152: Add the ability to purge pseudonymized tables anonymized_* tables have been added by bug 24151, this patch adds the usual way to purge data from them. The cleanup_database.pl script has been adjusted to take new parameters that will help to delete pseudonymized data. Test plan: Call the scrip with the new parameter to remove pseudonymized data * --pseudo-transactions DAYS will remove entries from pseudonymized_transactions older than DAYS day * --pseudo-transactions can be used without the parameter DAYS but with -- pseudo-transactions-from and/or --pseudo-transactions-to instead, to provide a range of date You can use the patch from bug 24153 to make the tests easier, data will not be deleted if the new --confirm flag is not passed. Sponsored-by: Association KohaLa - https://koha-fr.org/ Signed-off-by: Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Changing status to BLOCKED because of dependency on bug 24151, which does not apply at the moment.
Created attachment 105817 [details] [review] Bug 24152: Add method Koha::Objects->filter_by_last_update Signed-off-by: Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 105818 [details] [review] Bug 24152: Add a warning about the delete of statistics's table entries Signed-off-by: Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
Created attachment 105819 [details] [review] Bug 24152: Add the ability to purge pseudonymized tables anonymized_* tables have been added by bug 24151, this patch adds the usual way to purge data from them. The cleanup_database.pl script has been adjusted to take new parameters that will help to delete pseudonymized data. Test plan: Call the scrip with the new parameter to remove pseudonymized data * --pseudo-transactions DAYS will remove entries from pseudonymized_transactions older than DAYS day * --pseudo-transactions can be used without the parameter DAYS but with -- pseudo-transactions-from and/or --pseudo-transactions-to instead, to provide a range of date You can use the patch from bug 24153 to make the tests easier, data will not be deleted if the new --confirm flag is not passed. Sponsored-by: Association KohaLa - https://koha-fr.org/ Signed-off-by: Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr>
QAing
Created attachment 106051 [details] [review] Bug 24152: Add method Koha::Objects->filter_by_last_update Signed-off-by: Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 106052 [details] [review] Bug 24152: Add a warning about the delete of statistics's table entries Signed-off-by: Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 106053 [details] [review] Bug 24152: Add the ability to purge pseudonymized tables anonymized_* tables have been added by bug 24151, this patch adds the usual way to purge data from them. The cleanup_database.pl script has been adjusted to take new parameters that will help to delete pseudonymized data. Test plan: Call the scrip with the new parameter to remove pseudonymized data * --pseudo-transactions DAYS will remove entries from pseudonymized_transactions older than DAYS day * --pseudo-transactions can be used without the parameter DAYS but with -- pseudo-transactions-from and/or --pseudo-transactions-to instead, to provide a range of date You can use the patch from bug 24153 to make the tests easier, data will not be deleted if the new --confirm flag is not passed. Sponsored-by: Association KohaLa - https://koha-fr.org/ Signed-off-by: Signed-off-by: Sonia Bouis <sonia.bouis@univ-lyon3.fr> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 106054 [details] [review] Bug 24152: (QA follow-up) Add tests for alternative from and to pars Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Looks good to me, minor points: + || defined $pPseudoTransactions ) { print "You did not specify any cleanup work for the script to do.\n\n"; => Looks like you forgot the alternative parameters here with from and to ? Follow-up added. Use of uninitialized value $pPseudoTransactions in concatenation (.) or string at misc/cronjobs/cleanup_database.pl line 462. Purging pseudonymized transactions older than days. => Since we have three pars, removed the one parameter from the message. Looks like it is not possible to purge with days 0 ? Is the time stripped in the dtf parsing?
(In reply to Marcel de Rooy from comment #21) > Looks good to me, minor points: > > + || defined $pPseudoTransactions > ) { > print "You did not specify any cleanup work for the script to do.\n\n"; > => Looks like you forgot the alternative parameters here with from and to ? > Follow-up added. > > Use of uninitialized value $pPseudoTransactions in concatenation (.) or > string at misc/cronjobs/cleanup_database.pl line 462. > Purging pseudonymized transactions older than days. > => Since we have three pars, removed the one parameter from the message. > > Looks like it is not possible to purge with days 0 ? Is the time stripped in > the dtf parsing? I think it's because of the follow-up, before it was "if defined $pPseudoTransactions". I will add another follow-up, can you double check?
Created attachment 106196 [details] [review] Bug 24152: Allow purge with 0 day
Well no, it was not the follow-up :)
(In reply to Jonathan Druart from comment #23) > Well no, it was not the follow-up :) - ( $pPseudoTransactions ? ( days => $pPseudoTransactions ) : () ), - ( $pPseudoTransactionsFrom ? ( from => $pPseudoTransactionsFrom ) : () ), - ( $pPseudoTransactionsTo ? ( to => $pPseudoTransactionsTo ) : () ), + ( defined $pPseudoTransactions ? ( days => $pPseudoTransactions ) : () ), + ( $pPseudoTransactionsFrom ? ( from => $pPseudoTransactionsFrom ) : () ), + ( $pPseudoTransactionsTo ? ( to => $pPseudoTransactionsTo ) : () ), Well I did not test, but the change with defined explains it for me.
(In reply to Marcel de Rooy from comment #25) > (In reply to Jonathan Druart from comment #23) > > Well no, it was not the follow-up :) > > - ( $pPseudoTransactions ? ( days => $pPseudoTransactions > ) : () ), > - ( $pPseudoTransactionsFrom ? ( from => $pPseudoTransactionsFrom > ) : () ), > - ( $pPseudoTransactionsTo ? ( to => $pPseudoTransactionsTo > ) : () ), > + ( defined $pPseudoTransactions ? ( days => > $pPseudoTransactions ) : () ), > + ( $pPseudoTransactionsFrom ? ( from => > $pPseudoTransactionsFrom ) : () ), > + ( $pPseudoTransactionsTo ? ( to => > $pPseudoTransactionsTo ) : () ), > > > Well I did not test, but the change with defined explains it for me. Hmm. I tried it now but filter_last_update works with < and truncates to days. So what actually happens, is that I am purging records before today. Which is a bit counterintuitive.
Indeed, the script does not allow to remove data from today. It is the current behaviour for other data this script purges. I am going to open a separate bug report.
(In reply to Jonathan Druart from comment #27) > Indeed, the script does not allow to remove data from today. It is the > current behaviour for other data this script purges. I am going to open a > separate bug report. See bug 26026.
Pushed to master for 20.11, thanks to everybody involved!
Not backporting enhancement to 20.05.x series.