Bug 24152

Summary: Add the ability to purge pseudonymized data
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Command-line UtilitiesAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: axelle.clarisse, julian.maurice, koha, lucas, m.de.rooy, martin.renvoize, robin
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=26026
Change sponsored?: Sponsored Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00
Bug Depends on: 19008, 24151    
Bug Blocks: 24153    
Attachments: Bug 24152: Add method Koha::Objects->filter_by_last_update
Bug 24152: Add a warning about the delete of statistics's table entries
Bug 24152: Add the ability to purge pseudonymized tables
Bug 24152: Add method Koha::Objects->filter_by_last_update
Bug 24152: Add a warning about the delete of statistics's table entries
Bug 24152: Add the ability to purge pseudonymized tables
Bug 24152: Add method Koha::Objects->filter_by_last_update
Bug 24152: Add a warning about the delete of statistics's table entries
Bug 24152: Add the ability to purge pseudonymized tables
Bug 24152: Add method Koha::Objects->filter_by_last_update
Bug 24152: Add a warning about the delete of statistics's table entries
Bug 24152: Add the ability to purge pseudonymized tables
Bug 24152: Add method Koha::Objects->filter_by_last_update
Bug 24152: Add a warning about the delete of statistics's table entries
Bug 24152: Add the ability to purge pseudonymized tables
Bug 24152: (QA follow-up) Add tests for alternative from and to pars
Bug 24152: Allow purge with 0 day

Description Jonathan Druart 2019-12-03 15:48:17 UTC

    
Comment 1 Jonathan Druart 2019-12-03 18:12:42 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2019-12-03 18:12:45 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2019-12-03 18:12:48 UTC Comment hidden (obsolete)
Comment 4 Martin Renvoize 2020-01-03 11:29:05 UTC
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)
Comment 5 Jonathan Druart 2020-01-11 18:14:10 UTC
Created attachment 97253 [details] [review]
Bug 24152: Add method Koha::Objects->filter_by_last_update
Comment 6 Jonathan Druart 2020-01-11 18:14:13 UTC
Created attachment 97254 [details] [review]
Bug 24152: Add a warning about the delete of statistics's table entries
Comment 7 Jonathan Druart 2020-01-11 18:14:16 UTC
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/
Comment 8 Koha Team University Lyon 3 2020-03-20 14:31:10 UTC
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
Comment 9 Jonathan Druart 2020-03-20 16:02:54 UTC
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>
Comment 10 Jonathan Druart 2020-03-20 16:02:57 UTC
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>
Comment 11 Jonathan Druart 2020-03-20 16:03:01 UTC
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>
Comment 12 Julian Maurice 2020-06-12 13:45:04 UTC
Changing status to BLOCKED because of dependency on bug 24151, which does not apply at the moment.
Comment 13 Jonathan Druart 2020-06-12 14:08:58 UTC
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>
Comment 14 Jonathan Druart 2020-06-12 14:09:02 UTC
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>
Comment 15 Jonathan Druart 2020-06-12 14:09:05 UTC
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>
Comment 16 Marcel de Rooy 2020-06-19 07:38:58 UTC
QAing
Comment 17 Marcel de Rooy 2020-06-19 08:05:47 UTC
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>
Comment 18 Marcel de Rooy 2020-06-19 08:05:51 UTC
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>
Comment 19 Marcel de Rooy 2020-06-19 08:05:55 UTC
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>
Comment 20 Marcel de Rooy 2020-06-19 08:06:00 UTC
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>
Comment 21 Marcel de Rooy 2020-06-19 08:06:41 UTC
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?
Comment 22 Jonathan Druart 2020-06-23 10:00:05 UTC
(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?
Comment 23 Jonathan Druart 2020-06-23 10:03:32 UTC
Created attachment 106196 [details] [review]
Bug 24152: Allow purge with 0 day
Comment 24 Jonathan Druart 2020-06-23 10:03:57 UTC
Well no, it was not the follow-up :)
Comment 25 Marcel de Rooy 2020-06-24 12:40:48 UTC
(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.
Comment 26 Marcel de Rooy 2020-07-02 07:51:01 UTC
(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.
Comment 27 Jonathan Druart 2020-07-20 12:21:55 UTC
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.
Comment 28 Jonathan Druart 2020-07-20 12:24:58 UTC
(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.
Comment 29 Jonathan Druart 2020-07-20 15:48:20 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 30 Lucas Gass 2020-07-25 18:10:44 UTC
Not backporting enhancement to 20.05.x series.