Hi Marcel, can you explain this a bit more? Is this about logging when an item's lost status is lifted?

(In reply to Katrin Fischer from comment #1)
> Hi Marcel, can you explain this a bit more? Is this about logging when an
> item's lost status is lifted?

Hi Katrin. Yes, this is about logging the switch to/from a lost status. We can currently retrieve info about setting to lost with the lost date in items, but we cannot see (easier) when we recovered a book and lifted the lost status within a given period.

Item_found and item_lost don't fit very well with the other types of actions recorded in statistics, conceptually. Everything else is an action that someone took -- checkout, checkin, payment, writeoff, etc. Item_found and item_lost are changes to the state of the item as a result of one of those actions. So this feels like it's adding confusion to the nature of the data recorded in this table.

(In reply to Andrew Fuerste-Henry from comment #10)
> Item_found and item_lost don't fit very well with the other types of actions
> recorded in statistics, conceptually. Everything else is an action that
> someone took -- checkout, checkin, payment, writeoff, etc. Item_found and
> item_lost are changes to the state of the item as a result of one of those
> actions. So this feels like it's adding confusion to the nature of the data
> recorded in this table.

Thx for taking a look.
Found and lost represent the result of actions like a checkin or an item edit. These actions do change state of the items just like the other ones mentioned. I do not really see why this would create confusion. Since the table has a very generic name (statistics), and the data stored fits fine in the current table design, I opted for this solution. All data is about item status changes. The one thing we are missing the most is the number of found/recovered items.

Created attachment 151051 [details] [review]
Bug 33608: Add two new circulation types in Statistics module

These are: item_lost and item_found. Speak for themself.

Test plan:
Run t/db_dependent/Stats.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 151052 [details] [review]
Bug 33608: Add UpdateStats to item->store

Test plan:
Run t/db_dependent/Koha/Item.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 151053 [details] [review]
Bug 33608: Rearranging Stats.t, removing useless t version

Subtest, modules, license.
The t/Stats.t is as good as empty, can be removed.

Test plan:
Run t/db_dependent/Stats.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 151054 [details] [review]
Bug 33608: Correct indentation in Stats.t

Test plan:
Run t/db_dependent/Stats.t
Run git diff -w HEAD~1.. t/db_dependent/Stats.t, proving that we
only did whitespace changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 151055 [details] [review]
Bug 33608: Redirect UpdateStats to Koha namespace

Moving code to Koha::Statistic->new and ->insert.
Polishing code a bit further in next patch.

Test plan:
Run t/db_dependent/Stats.t
Run t/db_dependent/Koha/Statistics.t
Run t/db_dependent/Koha/Pseudonymization.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 151056 [details] [review]
Bug 33608: Polish Koha::Statistic further

Adding exceptions, removing croaks.
No exception in new for unknown hash keys, store will catch that.
Prepare switching amount parameter to value (db column name).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 151057 [details] [review]
Bug 33608: Move older Stats tests to Koha/Statistics.t

Test plan:
Run t/db_dependent/Koha/Statistics.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Rebased on top of 30928. More laborious than hoped for ;)

When I run t/db_dependent/Stats.t I get an error:

Cannot detect source of 't/db_dependent/Stats.t'! at /usr/share/perl/5.32/TAP/Parser/IteratorFactory.pm line 256.

Am I doing something wrong?

(In reply to Owen Leonard from comment #20)
> When I run t/db_dependent/Stats.t I get an error:
> 
> Cannot detect source of 't/db_dependent/Stats.t'! at
> /usr/share/perl/5.32/TAP/Parser/IteratorFactory.pm line 256.
> 
> Am I doing something wrong?

It seems so.
I could reproduce it:

root@master:/usr/share/koha# prove t/db_dependent/Koha/Statistics.t!
Cannot detect source of 't/db_dependent/Koha/Statistics.t!'! at /usr/share/perl/5.32/TAP/Parser/IteratorFactory.pm line 256.

This error signals running prove on a file that does not exist.

Solution: Remove exclamation mark after .t

I did not run the command with an extra exclamation mark.

> Cannot detect source of 't/db_dependent/Stats.t'!

The exclamation mark is added by the error message.

I guess I assumed that Stats.t existed because the test plan said so:

> Test plan:
> Run t/db_dependent/Stats.t

(In reply to Owen Leonard from comment #22)
> I did not run the command with an extra exclamation mark.
> 
> > Cannot detect source of 't/db_dependent/Stats.t'!
> 
> The exclamation mark is added by the error message.
> 
> I guess I assumed that Stats.t existed because the test plan said so:
> 
> > Test plan:
> > Run t/db_dependent/Stats.t

 t/db_dependent/Koha/Statistics.t | 144 +++++++++++++++++++++-----
 t/db_dependent/Stats.t           | 168 -------------------------------
 2 files changed, 119 insertions(+), 193 deletions(-)
 delete mode 100755 t/db_dependent/Stats.t

They are merged in the last patch.

The interface may be assumed to be "intranet" but should it log that information anyway for consistency?

+---------------------+--------+--------+------------+-----------+
| datetime            | branch | value  | type       | interface |
+---------------------+--------+--------+------------+-----------+
| 2023-05-30 17:16:18 | CPL    | 0.0000 | issue      | intranet  |
| 2023-05-30 17:16:27 | CPL    |   NULL | item_lost  | NULL      |
| 2023-05-30 17:18:45 | CPL    |   NULL | item_found | NULL      |
| 2023-05-30 17:18:45 | CPL    |   NULL | return     | intranet  |
+---------------------+--------+--------+------------+-----------+

(In reply to Owen Leonard from comment #24)
> The interface may be assumed to be "intranet" but should it log that
> information anyway for consistency?
> 
> +---------------------+--------+--------+------------+-----------+
> | datetime            | branch | value  | type       | interface |
> +---------------------+--------+--------+------------+-----------+
> | 2023-05-30 17:16:18 | CPL    | 0.0000 | issue      | intranet  |
> | 2023-05-30 17:16:27 | CPL    |   NULL | item_lost  | NULL      |
> | 2023-05-30 17:18:45 | CPL    |   NULL | item_found | NULL      |
> | 2023-05-30 17:18:45 | CPL    |   NULL | return     | intranet  |
> +---------------------+--------+--------+------------+-----------+

Good point.
This was actually developed before the interface column was added. But I suppose that it should default to context->interface.
Adding a follow-up.

Catching a C4::Auth problem on the way:

This line in cookie_auth may bite us (and possibly many other callers):
    C4::Context->interface($session->param('interface'));
It overwrites the interface set by get_template_and_user. Here from additem.

Here i am on intranet but i logged in via opac
| 2023-06-01 11:44:01 | CPL    |   NULL | item_lost  |       |          1 | BKS      | NULL     |           NULL | NULL  | NULL         | opac      |

Here again on intranet with intranet cookie:
| 2023-06-01 11:58:11 | CPL    |   NULL | item_found |       |          1 | BKS      | NULL     |           NULL | NULL  | NULL         | intranet  |

Will open another report for this small Auth 'bug'.

Created attachment 151913 [details] [review]
Bug 33608: (follow-up) Default value for interface column

We should default to C4::Context->interface.

Test plan:
Login via intranet (see Bugzilla report).
Edit an item. Set to lost.
Check entry in statistics table on interface value.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

(In reply to Marcel de Rooy from comment #26)

> Will open another report for this small Auth 'bug'.

Opened byg 33879

Created attachment 151915 [details] [review]
Bug 33608: Add two new circulation types in Statistics module

These are: item_lost and item_found. Speak for themself.

Test plan:
Run t/db_dependent/Stats.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 151916 [details] [review]
Bug 33608: Add UpdateStats to item->store

Test plan:
Run t/db_dependent/Koha/Item.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 151917 [details] [review]
Bug 33608: Rearranging Stats.t, removing useless t version

Subtest, modules, license.
The t/Stats.t is as good as empty, can be removed.

Test plan:
Run t/db_dependent/Stats.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 151918 [details] [review]
Bug 33608: Correct indentation in Stats.t

Test plan:
Run t/db_dependent/Stats.t
Run git diff -w HEAD~1.. t/db_dependent/Stats.t, proving that we
only did whitespace changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 151919 [details] [review]
Bug 33608: Redirect UpdateStats to Koha namespace

Moving code to Koha::Statistic->new and ->insert.
Polishing code a bit further in next patch.

Test plan:
Run t/db_dependent/Stats.t
Run t/db_dependent/Koha/Statistics.t
Run t/db_dependent/Koha/Pseudonymization.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 151920 [details] [review]
Bug 33608: Polish Koha::Statistic further

Adding exceptions, removing croaks.
No exception in new for unknown hash keys, store will catch that.
Prepare switching amount parameter to value (db column name).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 151921 [details] [review]
Bug 33608: Move older Stats tests to Koha/Statistics.t

Test plan:
Run t/db_dependent/Koha/Statistics.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 151922 [details] [review]
Bug 33608: (follow-up) Default value for interface column

We should default to C4::Context->interface.

Test plan:
Login via intranet (see Bugzilla report).
Edit an item. Set to lost.
Check entry in statistics table on interface value.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Created attachment 153653 [details] [review]
Bug 33608: Add two new circulation types in Statistics module

These are: item_lost and item_found. Speak for themself.

Test plan:
Run t/db_dependent/Stats.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153654 [details] [review]
Bug 33608: Add UpdateStats to item->store

Test plan:
Run t/db_dependent/Koha/Item.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153655 [details] [review]
Bug 33608: Rearranging Stats.t, removing useless t version

Subtest, modules, license.
The t/Stats.t is as good as empty, can be removed.

Test plan:
Run t/db_dependent/Stats.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153656 [details] [review]
Bug 33608: Correct indentation in Stats.t

Test plan:
Run t/db_dependent/Stats.t
Run git diff -w HEAD~1.. t/db_dependent/Stats.t, proving that we
only did whitespace changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153657 [details] [review]
Bug 33608: Redirect UpdateStats to Koha namespace

Moving code to Koha::Statistic->new and ->insert.
Polishing code a bit further in next patch.

Test plan:
Run t/db_dependent/Stats.t
Run t/db_dependent/Koha/Statistics.t
Run t/db_dependent/Koha/Pseudonymization.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153658 [details] [review]
Bug 33608: Polish Koha::Statistic further

Adding exceptions, removing croaks.
No exception in new for unknown hash keys, store will catch that.
Prepare switching amount parameter to value (db column name).

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153659 [details] [review]
Bug 33608: Move older Stats tests to Koha/Statistics.t

Test plan:
Run t/db_dependent/Koha/Statistics.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153660 [details] [review]
Bug 33608: (follow-up) Default value for interface column

We should default to C4::Context->interface.

Test plan:
Login via intranet (see Bugzilla report).
Edit an item. Set to lost.
Check entry in statistics table on interface value.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153661 [details] [review]
Bug 33608: (QA follow-up) Tidy

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

QA scripts happy and the code all works as described.  Two points to mention for RM however.

1) There's a small change of logic here regarding itemlost triggers. We now trigger on change from any lost value (any non zero) to not lost.. We used to only trigger for positive lost values to not lost. (I don't think this is an issue and couldn't find anything in the documentation to say we should be treating negatives differently however)
2) I feel like the code here is inconsistent with other Koha::Object based modules.. however, I can understand this as a stepping stone move..

Will address the >0 concern and look at test coverage

(In reply to Martin Renvoize from comment #46)

> 2) I feel like the code here is inconsistent with other Koha::Object based
> modules.. however, I can understand this as a stepping stone move..

Lets do that on 33636

(In reply to Martin Renvoize from comment #46)
> 1) There's a small change of logic here regarding itemlost triggers. We now
> trigger on change from any lost value (any non zero) to not lost.. We used
> to only trigger for positive lost values to not lost. (I don't think this is
> an issue and couldn't find anything in the documentation to say we should be
> treating negatives differently however)

Opened 34308 to look at the negatives.

Created attachment 153698 [details] [review]
Bug 33608: Add two new circulation types in Statistics module

These are: item_lost and item_found. Speak for themself.

Test plan:
Run t/db_dependent/Stats.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153699 [details] [review]
Bug 33608: Add UpdateStats to item->store

Test plan:
Run t/db_dependent/Koha/Item.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153700 [details] [review]
Bug 33608: Rearranging Stats.t, removing useless t version

Subtest, modules, license.
The t/Stats.t is as good as empty, can be removed.

Test plan:
Run t/db_dependent/Stats.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153701 [details] [review]
Bug 33608: Correct indentation in Stats.t

Test plan:
Run t/db_dependent/Stats.t
Run git diff -w HEAD~1.. t/db_dependent/Stats.t, proving that we
only did whitespace changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153702 [details] [review]
Bug 33608: Redirect UpdateStats to Koha namespace

Moving code to Koha::Statistic->new and ->insert.
Polishing code a bit further in next patch.

Test plan:
Run t/db_dependent/Stats.t
Run t/db_dependent/Koha/Statistics.t
Run t/db_dependent/Koha/Pseudonymization.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153703 [details] [review]
Bug 33608: Polish Koha::Statistic further

Adding exceptions, removing croaks.
No exception in new for unknown hash keys, store will catch that.
Prepare switching amount parameter to value (db column name).

Test plan:
Run t/db_dependent/Stats.t
Run t/db_dependent/Koha/Items.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153704 [details] [review]
Bug 33608: Move older Stats tests to Koha/Statistics.t

Test plan:
Run t/db_dependent/Koha/Statistics.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153705 [details] [review]
Bug 33608: (follow-up) Default value for interface column

We should default to C4::Context->interface.

Test plan:
Login via intranet (see Bugzilla report).
Edit an item. Set to lost.
Check entry in statistics table on interface value.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153706 [details] [review]
Bug 33608: (QA follow-up) Tidy

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153707 [details] [review]
Bug 33608: (QA follow-up) Restore older gt zero tests

See the Bugzilla report. I have been asked to restore the former
tests although I definitely think that they are wrong. Will address
that on bug 34308 separately.

This currently has the side-effect of negative lost values being
interpreted as 'found'. (Do not use negative lost values!)
The added subtest still reflects that now. Added a few TODOs.

Test plan:
[1] Prove t/db_dependent/Koha/Items.t

    # Subtest: itemlost / statistics
        1..5
        ok 1 - No statistics added
        ok 2 - statistics added
        ok 3 - No statistics added, already lost
        ok 4 - statistics added
        ok 5 - No statistics added, already *found*
    ok 8 - itemlost / statistics

 OK     Koha/Item.pm
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   OK     pod coverage
   SKIP   spelling
   OK     tidiness
   OK     valid

 OK     t/db_dependent/Koha/Items.t
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   SKIP   spelling
   OK     tidiness
   OK     valid

Items.t actually revealed two bugs obscuring each other:

[1] Bug 34316 about not rethrowing an exception

[2] Bug 33608: Polish Koha::Statistic further (9.68 KB, patch)
Former version contained a bug too when checking the amount parameter needed for account statistics (amount becomes value in table).
Gave it attention too on the following patch Move older Stats tests to Koha/Statistics.t when running the corresponding test.

First concern resolved. Martin: could you have another look?

Sorry, am going to still replace the added test of params->{amount} by exists on the 7th patch.

diff --git a/Koha/Statistic.pm b/Koha/Statistic.pm
index 2aa4044ce9..479c6472a2 100644
--- a/Koha/Statistic.pm
+++ b/Koha/Statistic.pm
@@ -73,7 +73,7 @@ sub new {
     Koha::Exceptions::BadParameter->throw( parameter => $params ) if !$params || ref($params) ne 'HASH';
     Koha::Exceptions::WrongParameter->throw( name => 'type', value => $params->{type} ) if !$params->{type};

-    if ( $params->{amount} ) {
+    if ( exists $params->{amount} ) {
         $params->{value} //= delete $params->{amount}; # legacy amount parameter supported
     }

diff --git a/t/db_dependent/Koha/Statistics.t b/t/db_dependent/Koha/Statistics.t
index 999d1b8567..a7f2c20d4e 100755
--- a/t/db_dependent/Koha/Statistics.t
+++ b/t/db_dependent/Koha/Statistics.t
@@ -73,7 +73,7 @@ subtest 'Basic Koha object tests' => sub {
 };

 subtest 'Test exceptions in ->new' => sub {
-    plan tests => 5;
+    plan tests => 6;
     $schema->storage->txn_begin;

     throws_ok { Koha::Statistic->new } 'Koha::Exceptions::BadParameter', '->new called without params';
@@ -93,6 +93,8 @@ subtest 'Test exceptions in ->new' => sub {
     $params->{type} = 'payment';
     delete $params->{amount};
     throws_ok { Koha::Statistic->new($params) } 'Koha::Exceptions::MissingParameter', '->new called for accounts without amount';
+    $params->{amount} = 0;
+    lives_ok { Koha::Statistic->new($params) } '->new accepts zero amount';
     $params->{type} = 'issue';
     delete $params->{itemnumber};
     throws_ok { Koha::Statistic->new($params) } 'Koha::Exceptions::MissingParameter', '->new called for circulation without itemnumber';

# Subtest: Test exceptions in ->new
    1..6
    ok 1 - ->new called without params
    ok 2 - ->new called without type
    ok 3 - ->new called with wrong type
    ok 4 - ->new called for accounts without amount
    ok 5 - ->new accepts zero amount
    ok 6 - ->new called for circulation without itemnumber

Tomas, this is weirdL

 WARN   Koha/Statistic.pm
   WARN   tidiness
                The file is less tidy than before (bad/messy lines before: 1, now: 12)

File has been tidied. But if I tidy it again, it generates this diff:

diff --git a/Koha/Statistic.pm b/Koha/Statistic.pm
index 217f378ab0..e37d1b88f7 100644
--- a/Koha/Statistic.pm
+++ b/Koha/Statistic.pm
@@ -27,9 +27,9 @@ use Koha::PseudonymizedTransaction;

 use base qw(Koha::Object);

-our @allowed_accounts_types     = qw( writeoff payment );
-our @allowed_circulation_types  = qw( renew issue localuse return onsite_checkout recall item_found item_lost );
-our @mandatory_accounts_keys    = qw( type branch borrowernumber value ); # note that amount is mapped to value
+our @allowed_accounts_types    = qw( writeoff payment );
+our @allowed_circulation_types = qw( renew issue localuse return onsite_checkout recall item_found item_lost );
+our @mandatory_accounts_keys = qw( type branch borrowernumber value );    # note that amount is mapped to value
 our @mandatory_circulation_keys = qw( type branch borrowernumber itemnumber ccode itemtype );

 =head1 NAME
@@ -103,18 +103,18 @@ sub new {

     return $class->SUPER::new(
         {
-            datetime       => Koha::Database->new->schema->storage->datetime_parser->format_datetime(dt_from_string),
-            branch         => $params->{branch},
-            type           => $params->{type},
-            value          => _key_or_default( $params, 'value', 0 ),
-            other          => _key_or_default( $params, 'other', q{} ),
-            itemnumber     => $params->{itemnumber},
-            itemtype       => _key_or_default( $params, 'itemtype', q{} ),
-            location       => $params->{location},
+            datetime   => Koha::Database->new->schema->storage->datetime_parser->format_datetime(dt_from_string),
+            branch     => $params->{branch},
+            type       => $params->{type},
+            value      => _key_or_default( $params, 'value', 0 ),
+            other      => _key_or_default( $params, 'other', q{} ),
+            itemnumber => $params->{itemnumber},
+            itemtype   => _key_or_default( $params, 'itemtype', q{} ),
+            location   => $params->{location},
             borrowernumber => $params->{borrowernumber},                 # no longer sending empty string (changed 2023)
             categorycode   => $params->{categorycode},
             ccode          => _key_or_default( $params, 'ccode', q{} ),
-            interface      => $params->{interface} // C4::Context->interface,
+            interface => $params->{interface} // C4::Context->interface,
         }
     );
 }

The spaces before => are now no longer the same. Weird.

I recommend to go with the tidy patch of Martin.

Created attachment 153708 [details] [review]
Bug 33608: Polish Koha::Statistic further

Adding exceptions, removing croaks.
No exception in new for unknown hash keys, store will catch that.
Prepare switching amount parameter to value (db column name).

Test plan:
Run t/db_dependent/Stats.t
Run t/db_dependent/Koha/Items.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153709 [details] [review]
Bug 33608: Move older Stats tests to Koha/Statistics.t

Test plan:
Run t/db_dependent/Koha/Statistics.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153710 [details] [review]
Bug 33608: (follow-up) Default value for interface column

We should default to C4::Context->interface.

Test plan:
Login via intranet (see Bugzilla report).
Edit an item. Set to lost.
Check entry in statistics table on interface value.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153711 [details] [review]
Bug 33608: (QA follow-up) Tidy

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 153712 [details] [review]
Bug 33608: (QA follow-up) Restore older gt zero tests

See the Bugzilla report. I have been asked to restore the former
tests although I definitely think that they are wrong. Will address
that on bug 34308 separately.

This currently has the side-effect of negative lost values being
interpreted as 'found'. (Do not use negative lost values!)
The added subtest still reflects that now. Added a few TODOs.

Test plan:
[1] Prove t/db_dependent/Koha/Items.t

Created attachment 153713 [details] [review]
Bug 33608: (QA follow-up) Restore older gt zero tests

See the Bugzilla report. I have been asked to restore the former
tests although I definitely think that they are wrong. Will address
that on bug 34308 separately.

This currently has the side-effect of negative lost values being
interpreted as 'found'. (Do not use negative lost values!)
The added subtest still reflects that now. Added a few TODOs.

Test plan:
[1] Prove t/db_dependent/Koha/Items.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 153765 [details] [review]
Bug 33608: (QA follow-up) Restore older gt zero tests

See the Bugzilla report. I have been asked to restore the former
tests although I definitely think that they are wrong. Will address
that on bug 34308 separately.

This currently has the side-effect of negative lost values being
interpreted as 'found'. (Do not use negative lost values!)
The added subtest still reflects that now. Added a few TODOs.

Test plan:
[1] Prove t/db_dependent/Koha/Items.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Still applies, no warns from qa tools? 
Tomas: Why does this stay in the queue ?

If we're recording this in statistics, I'd love to see it included in what we also enter in pseudonymized_transactions. Otherwise this data is lost when/if libraries delete their old statistics lines.

(In reply to Marcel de Rooy from comment #75)
> Still applies, no warns from qa tools? 
> Tomas: Why does this stay in the queue ?

I'm on vacation after KohaCon. Well be back to the keyboard really soon.

(In reply to Andrew Fuerste-Henry from comment #76)
> If we're recording this in statistics, I'd love to see it included in what
> we also enter in pseudonymized_transactions. Otherwise this data is lost
> when/if libraries delete their old statistics lines.

Opened bug 34628. Note that the pseudonymization now depends on having a borrowernumber which is not available here.

Please continue discussion on the new report.

I generally like the cleanup being made in the area. Great job!

I have some QA remarks that I consider blockers. Sorted randomly.

1) Changes to POD in Koha::Statistic are wrong in terms of consistency:

-=head2 Class methods
+=head2 METHODS
...
-=head2 Internal methods
+=head2 INTERNAL METHODS

Please keep the 'style' we picked some years ago.

2) Koha::Statistic->_key_or_default feels bad. I'd prefer a ternary operator.

3) Koha::Statistic->insert means introducing a new pattern, and I don't think we need it. I'd ask you to file a new bug for proposing that pattern and have devs discuss it there, so this enhancement is not blocked.

4) C4/Stats.pm =>

+use Koha::Statistic;

^^ it should use the plural class.

5) Probably for Martin's follow-up: if we are tidying an entire hash structure, I usually suggest we sort keys on the same run. I can do it inline, but thought it was worth mentioning explicitly.

6) This would benefit from some patch squashing.

I'm happy to push this ASAP once the issues are solved.

Thx for your comments. I hope to return to this one soon if time allows. This probably means considerable delay since it will need another QA round and the queues are filled up already..

(In reply to Marcel de Rooy from comment #80)
> Thx for your comments. I hope to return to this one soon if time allows.
> This probably means considerable delay since it will need another QA round
> and the queues are filled up already..

I will QA myself if Martin doesn't.

Jonathan: There is still in an issue in running tests with qa tools. If you run qa tools here on the full set of patches (will submit a new batch in a minute), it tries to prove a deleted (moved) test script. Which doesnt work.

Most time spent here is tidy again and again..

Created attachment 157440 [details] [review]
Bug 33608: Add two new circulation types in Statistics module

These are: item_lost and item_found. Speak for themself.

Test plan:
Run t/db_dependent/Stats.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 157441 [details] [review]
Bug 33608: Add UpdateStats to item->store

Test plan:
Run t/db_dependent/Koha/Item.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 157442 [details] [review]
Bug 33608: Rearranging Stats.t, removing useless t version

Subtest, modules, license.
The t/Stats.t is as good as empty, can be removed.

Test plan:
Run t/db_dependent/Stats.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 157443 [details] [review]
Bug 33608: Correct indentation in Stats.t

Test plan:
Run t/db_dependent/Stats.t
Run git diff -w HEAD~1.. t/db_dependent/Stats.t, proving that we
only did whitespace changes.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 157444 [details] [review]
Bug 33608: Redirect UpdateStats to Koha namespace

Moving code to Koha::Statistic->new and ->insert.
Polishing code a bit further in next patch.

Test plan:
Run t/db_dependent/Stats.t
Run t/db_dependent/Koha/Statistics.t
Run t/db_dependent/Koha/Pseudonymization.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

[EDIT] POD headers, use plural module and tidy Koha::Statistic

Created attachment 157445 [details] [review]
Bug 33608: Polish Koha::Statistic further

Adding exceptions, removing croaks.
No exception in new for unknown hash keys, store will catch that.
Prepare switching amount parameter to value (db column name).

Test plan:
Run t/db_dependent/Stats.t
Run t/db_dependent/Koha/Items.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

[EDIT] Fixed the mandatory check in Stats.t. Removed key_or_default.
       Additional tidy.

Created attachment 157446 [details] [review]
Bug 33608: Move older Stats tests to Koha/Statistics.t

Test plan:
Run t/db_dependent/Koha/Statistics.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

[EDIT} Tidied inline.

Created attachment 157447 [details] [review]
Bug 33608: (follow-up) Default value for interface column

We should default to C4::Context->interface.

Test plan:
Login via intranet (see Bugzilla report).
Edit an item. Set to lost.
Check entry in statistics table on interface value.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 157448 [details] [review]
Bug 33608: (QA follow-up) Restore older gt zero tests

See the Bugzilla report. I have been asked to restore the former
tests although I definitely think that they are wrong. Will address
that on bug 34308 separately.

This currently has the side-effect of negative lost values being
interpreted as 'found'. (Do not use negative lost values!)
The added subtest still reflects that now. Added a few TODOs.

Test plan:
[1] Prove t/db_dependent/Koha/Items.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Created attachment 157449 [details] [review]
Bug 33608: (QA follow-up) Remove ->insert method

Might come back on the subject when resuming with bug 33636.

Test plan:
Run t/db_dependent/Koha/Statistics.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Tidy Koha/Item.pm bit further. Ordered hash keys too.

DONE 1) Changes to POD in Koha::Statistic are wrong in terms of consistency:
=> OK Surrendering. No problem. But I do think that these headers should be all in uc. Much nicer :)

DONE 2) Koha::Statistic->_key_or_default feels bad. I'd prefer a ternary operator. REMOVED

DONE 3) Koha::Statistic->insert means introducing a new pattern, and I don't think we need it. I'd ask you to file a new bug for proposing that pattern and have devs discuss it there, so this enhancement is not blocked.
=> Removed the method for now. Will resume this on 33636 probably.

DONE 4) C4/Stats.pm =>
+use Koha::Statistic;
^^ it should use the plural class.
=> Fixed in patch 5.

DONE 5) Probably for Martin's follow-up: if we are tidying an entire hash structure, I usually suggest we sort keys on the same run. I can do it inline, but thought it was worth mentioning explicitly.
=> Fixed two hashes that have been tidied.

DONE 6) This would benefit from some patch squashing.
=> Did some inline tidying removing the need for a tidy patch.

Gonna move this back to PQA given the nature of the changes. It is mostly tide here and there. Change POD etc. Nothing fundamental.

(In reply to Marcel de Rooy from comment #82)
> Jonathan: There is still in an issue in running tests with qa tools. If you
> run qa tools here on the full set of patches (will submit a new batch in a
> minute), it tries to prove a deleted (moved) test script. Which doesnt work.

I've quickly tried to fix it, but I only managed to generate worst situations.

You could revert the last commit "Issue #75 - Prevent to run tests twice on the same files", but it's not ideal.
I said it already, but we need a rewrite of the QA tool, how it deals with moved and deleted files is wrong. But that's not for now, and not for here...

Pushed to master for 23.11.

Nice work everyone, thanks!

This doesn't seem to have added tests for ->store()...

(In reply to Tomás Cohen Arazi from comment #98)
> This doesn't seem to have added tests for ->store()...

Please clarify what you exactly miss?

I am seeing:

+    subtest 'itemlost / statistics' => sub {    # TODO BZ 34308 (gt zero checks)
+        plan tests => 5;
+
+        my $item = $builder->build_sample_item;
+        $item->itemlost(-1)->store;             # weird value; >0 test not triggered ?
+        is( Koha::Statistics->search( { itemnumber => $item->id } )->count, 0, 'No statistics added' );
+        $item->itemlost(1)->store;
+        is( Koha::Statistics->search( { itemnumber => $item->id } )->count, 1, 'statistics added' );
+        $item->itemlost(2)->store;
+        is( Koha::Statistics->search( { itemnumber => $item->id } )->count, 1, 'No statistics added, already lost' );
+        $item->itemlost(-1)->store;             # weird value; <=0 test triggered ?
+        is( Koha::Statistics->search( { itemnumber => $item->id } )->count, 2, 'statistics added' );
+        $item->itemlost(-2)->store;             # weird value, but no status change
+        is( Koha::Statistics->search( { itemnumber => $item->id } )->count, 2, 'No statistics added, already *found*' );
+    };

Or are you thinking about those lines:

+    Koha::PseudonymizedTransaction->new_from_statistic($self)->store
+        if C4::Context->preference('Pseudonymization')
+        && $self->borrowernumber    # Not a real transaction if the patron does not exist
+                                    # For instance can be a transfer, or hold trigger
+        && grep { $_ eq $self->type } qw(renew issue return onsite_checkout);

Could mock new_from_statistic to see if it is really called?

Note btw:
git grep new_from_statistic
Koha/PseudonymizedTransaction.pm:=head3 new_from_statistic
Koha/PseudonymizedTransaction.pm:sub new_from_statistic {
Koha/Statistic.pm:    Koha::PseudonymizedTransaction->new_from_statistic($self)->store
=> Does not give results in t?