Bug 33285 - It should be possible to specify the separator used in runreport.pl
Summary: It should be possible to specify the separator used in runreport.pl
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Command-line Utilities (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 33050
  Show dependency treegraph
 
Reported: 2023-03-20 20:08 UTC by Nick Clemens
Modified: 2023-12-28 20:44 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.05,22.05.12,21.11.20


Attachments
Bug 33285: Allow specifying the delimeter for runreport.pl (1.71 KB, patch)
2023-03-20 20:12 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 33284: Unit test (4.70 KB, patch)
2023-03-27 13:33 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 33284: Set renewer_id appropriately for anonymize settings (1.08 KB, patch)
2023-03-27 13:33 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 33284: Set renewer_id for sets of checkouts (1.17 KB, patch)
2023-03-27 13:33 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 33285: Allow specifying the delimeter for runreport.pl (1.77 KB, patch)
2023-03-27 13:46 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 33285: Allow specifying the delimeter for runreport.pl (1.83 KB, patch)
2023-03-31 22:22 UTC, Fridolin Somers
Details | Diff | Splinter Review
Bug 33285: (QA follow-up) add POD and fix some code style (1.30 KB, patch)
2023-03-31 22:22 UTC, Fridolin Somers
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2023-03-20 20:08:46 UTC

    
Comment 1 Nick Clemens 2023-03-20 20:12:51 UTC
Created attachment 148435 [details] [review]
Bug 33285: Allow specifying the delimeter for runreport.pl

To test:
1 - Write a report in koha
2 - perl misc/cronjobs/runreport.pl --format csv 1 (or correct report number)
3 - Note you get commas
4 - Apply patch
5 - Repeat #2 - no change
6 - perl misc/cronjobs/runreport.pl --format csv --separator "|" 1
7 - Now it is pipe delimited
8 - perl misc/cronjobs/runreport.pl --format tsv --separator "|" 1
9 - Error is reported, you cannot set separator unless csv
10 - Try other separators
Comment 2 Andrew Fuerste-Henry 2023-03-27 13:33:21 UTC
Created attachment 148769 [details] [review]
Bug 33284: Unit test

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 3 Andrew Fuerste-Henry 2023-03-27 13:33:23 UTC
Created attachment 148770 [details] [review]
Bug 33284: Set renewer_id appropriately for anonymize settings

This patch adds a trigger into the anonymize function of
Koha::Old::Checkout such that we pass along the anonymize action to the
renewals record.

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 4 Andrew Fuerste-Henry 2023-03-27 13:33:25 UTC
Created attachment 148771 [details] [review]
Bug 33284: Set renewer_id for sets of checkouts

This patch adds a trigger into the anonymize function of
Koha::Old::Checkouts such that we pass along the anonymize action to
the renewals record.

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 5 Andrew Fuerste-Henry 2023-03-27 13:36:05 UTC
(sorry, mistyped a bug number)
Comment 6 Andrew Fuerste-Henry 2023-03-27 13:36:36 UTC
Comment on attachment 148769 [details] [review]
Bug 33284: Unit test

>From 7885567b7214e27baeef1e951456b6c78833ecb4 Mon Sep 17 00:00:00 2001
>From: Martin Renvoize <martin.renvoize@ptfs-europe.com>
>Date: Tue, 21 Mar 2023 15:01:08 +0000
>Subject: [PATCH] Bug 33284: Unit test
>
>Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
>
>https://bugs.koha-community.org/show_bug.cgi?id=33285
>---
> t/db_dependent/Koha/Old/Checkout.t  | 37 ++++++++++++++++++++++++-----
> t/db_dependent/Koha/Old/Checkouts.t | 30 ++++++++++++++++++++++-
> 2 files changed, 60 insertions(+), 7 deletions(-)
>
>diff --git a/t/db_dependent/Koha/Old/Checkout.t b/t/db_dependent/Koha/Old/Checkout.t
>index 7cf520e96f..ade2a1ff2b 100755
>--- a/t/db_dependent/Koha/Old/Checkout.t
>+++ b/t/db_dependent/Koha/Old/Checkout.t
>@@ -30,7 +30,7 @@ my $builder = t::lib::TestBuilder->new;
> 
> subtest 'anonymize() tests' => sub {
> 
>-    plan tests => 8;
>+    plan tests => 9;
> 
>     $schema->storage->txn_begin;
> 
>@@ -50,6 +50,25 @@ subtest 'anonymize() tests' => sub {
>             value => { borrowernumber => $patron->id }
>         }
>     );
>+    my $renewal_1 = $builder->build_object(
>+        {
>+            class => 'Koha::Checkouts::Renewals',
>+            value => {
>+                checkout_id => $checkout_2->id,
>+                interface   => 'opac',
>+                renewer_id  => $patron->id
>+            }
>+        }
>+    );
>+    my $renewal_2 = $builder->build_object(
>+        {
>+            class => 'Koha::Checkouts::Renewals',
>+            value => {
>+                checkout_id => $checkout_2->id,
>+                interface   => 'intranet'
>+            }
>+        }
>+    );
> 
>     is( $patron->old_checkouts->count, 2, 'Patron has 2 completed checkouts' );
> 
>@@ -63,20 +82,26 @@ subtest 'anonymize() tests' => sub {
>     is( $@->syspref, 'AnonymousPatron', 'syspref parameter is correctly passed' );
>     is( $patron->old_checkouts->count, 2, 'No changes, patron has 2 linked completed checkouts' );
> 
>-    is( $checkout_1->borrowernumber, $patron->id,
>-        'Anonymized hold not linked to patron' );
>     is( $checkout_2->borrowernumber, $patron->id,
>-        'Not anonymized hold still linked to patron' );
>+        'Checkout to anonymize still linked to patron' );
>+    is( $checkout_2->renewals->count, 2, 'Checkout 2 has 2 renewals' );
> 
>     my $anonymous_patron =
>       $builder->build_object( { class => 'Koha::Patrons' } );
>     t::lib::Mocks::mock_preference( 'AnonymousPatron', $anonymous_patron->id );
> 
>-    # anonymize second hold
>+    # anonymize second checkout
>     $checkout_2->anonymize;
>     $checkout_2->discard_changes;
>     is( $checkout_2->borrowernumber, $anonymous_patron->id,
>-        'Anonymized hold linked to anonymouspatron' );
>+        'Anonymized checkout linked to anonymouspatron' );
>+    is(
>+        $checkout_2->renewals->search(
>+            { renewer_id => $anonymous_patron->id }
>+        )->count,
>+        1,
>+        'OPAC renewal was anonymized'
>+    );
> 
>     $schema->storage->txn_rollback;
> };
>diff --git a/t/db_dependent/Koha/Old/Checkouts.t b/t/db_dependent/Koha/Old/Checkouts.t
>index 2c7c52ad62..7254bcba8e 100755
>--- a/t/db_dependent/Koha/Old/Checkouts.t
>+++ b/t/db_dependent/Koha/Old/Checkouts.t
>@@ -32,7 +32,7 @@ my $builder = t::lib::TestBuilder->new;
> 
> subtest 'anonymize() tests' => sub {
> 
>-    plan tests => 10;
>+    plan tests => 13;
> 
>     $schema->storage->txn_begin;
> 
>@@ -89,8 +89,28 @@ subtest 'anonymize() tests' => sub {
>             }
>         }
>     );
>+    my $renewal_1 = $builder->build_object(
>+        {
>+            class => 'Koha::Checkouts::Renewals',
>+            value => {
>+                checkout_id => $checkout_4->id,
>+                interface   => 'opac',
>+                renewer_id  => $patron->id
>+            }
>+        }
>+    );
>+    my $renewal_2 = $builder->build_object(
>+        {
>+            class => 'Koha::Checkouts::Renewals',
>+            value => {
>+                checkout_id => $checkout_4->id,
>+                interface   => 'intranet'
>+            }
>+        }
>+    );
> 
>     is( $patron->old_checkouts->count, 4, 'Patron has 4 completed checkouts' );
>+    is( $checkout_4->renewals->count, 2, 'Checkout 4 has 2 renewals' );
> 
>     # filter them so only the older two are part of the resultset
>     my $checkouts = $patron->old_checkouts->filter_by_last_update(
>@@ -112,6 +132,14 @@ subtest 'anonymize() tests' => sub {
>     is( $anonymized_count, 2, 'update() tells 2 rows were updated' );
> 
>     is( $patron->old_checkouts->count, 2, 'Patron has 2 completed checkouts' );
>+    is( $checkout_4->renewals->count, 2, 'Checkout 4 still has 2 renewals' );
>+    is(
>+        $checkout_4->renewals->search(
>+            { renewer_id => $anonymous_patron->id }
>+        )->count,
>+        1,
>+        'OPAC renewal was anonymized'
>+    );
> 
>     $schema->storage->txn_rollback;
> };
>-- 
>2.30.2
Comment 7 Andrew Fuerste-Henry 2023-03-27 13:46:25 UTC
Created attachment 148775 [details] [review]
Bug 33285: Allow specifying the delimeter for runreport.pl

To test:
1 - Write a report in koha
2 - perl misc/cronjobs/runreport.pl --format csv 1 (or correct report number)
3 - Note you get commas
4 - Apply patch
5 - Repeat #2 - no change
6 - perl misc/cronjobs/runreport.pl --format csv --separator "|" 1
7 - Now it is pipe delimited
8 - perl misc/cronjobs/runreport.pl --format tsv --separator "|" 1
9 - Error is reported, you cannot set separator unless csv
10 - Try other separators

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 8 Fridolin Somers 2023-03-31 22:22:46 UTC
Created attachment 149058 [details] [review]
Bug 33285: Allow specifying the delimeter for runreport.pl

To test:
1 - Write a report in koha
2 - perl misc/cronjobs/runreport.pl --format csv 1 (or correct report number)
3 - Note you get commas
4 - Apply patch
5 - Repeat #2 - no change
6 - perl misc/cronjobs/runreport.pl --format csv --separator "|" 1
7 - Now it is pipe delimited
8 - perl misc/cronjobs/runreport.pl --format tsv --separator "|" 1
9 - Error is reported, you cannot set separator unless csv
10 - Try other separators

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 9 Fridolin Somers 2023-03-31 22:22:49 UTC
Created attachment 149059 [details] [review]
Bug 33285: (QA follow-up) add POD and fix some code style

Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com>
Comment 10 Fridolin Somers 2023-03-31 22:24:37 UTC
Strange we can not use "\t" :
  perl misc/cronjobs/runreport.pl --format csv --separator "\t" 1
Generates with "\" as separator.

We have format tsv for that so no issue.

Great option ;)
Comment 11 Tomás Cohen Arazi 2023-04-06 12:30:39 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 12 Jacob O'Mara 2023-04-16 17:24:42 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 13 Lucas Gass 2023-04-24 17:48:21 UTC
Backported to 22.05.x for upcoming 22.05.12
Comment 14 Arthur Suzuki 2023-05-03 19:15:24 UTC
applied to 21.11.x for 21.11.20