Bug 33608 - Allow to get statistics about found/recovered books
Summary: Allow to get statistics about found/recovered books
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Reports (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Marcel de Rooy
QA Contact: Martin Renvoize
URL:
Keywords: rel_23_11_candidate
Depends on: 34316
Blocks: 33636 34611 34628
  Show dependency treegraph
 
Reported: 2023-04-25 14:51 UTC by Marcel de Rooy
Modified: 2023-10-26 06:18 UTC (History)
5 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:
23.11.00


Attachments
Bug 33608: Add two new circulation types in Statistics module (1.30 KB, patch)
2023-05-09 11:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Add UpdateStats to item->store (2.30 KB, patch)
2023-05-09 11:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Rearranging Stats.t, removing useless t version (2.60 KB, patch)
2023-05-09 11:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Correct indentation in Stats.t (11.65 KB, patch)
2023-05-09 11:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Redirect UpdateStats to Koha namespace (12.82 KB, patch)
2023-05-09 11:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Polish Koha::Statistic further (8.79 KB, patch)
2023-05-09 11:24 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Move older Stats tests to Koha/Statistics.t (13.49 KB, patch)
2023-05-09 11:25 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Add two new circulation types in Statistics module (1.31 KB, patch)
2023-05-11 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Add UpdateStats to item->store (2.30 KB, patch)
2023-05-11 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Rearranging Stats.t, removing useless t version (2.60 KB, patch)
2023-05-11 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Correct indentation in Stats.t (12.24 KB, patch)
2023-05-11 08:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Redirect UpdateStats to Koha namespace (13.23 KB, patch)
2023-05-11 08:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Polish Koha::Statistic further (8.99 KB, patch)
2023-05-11 08:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Move older Stats tests to Koha/Statistics.t (14.09 KB, patch)
2023-05-11 08:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: (follow-up) Default value for interface column (1.10 KB, patch)
2023-06-01 12:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Add two new circulation types in Statistics module (1.32 KB, patch)
2023-06-01 12:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 33608: Add UpdateStats to item->store (2.31 KB, patch)
2023-06-01 12:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 33608: Rearranging Stats.t, removing useless t version (2.61 KB, patch)
2023-06-01 12:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 33608: Correct indentation in Stats.t (12.25 KB, patch)
2023-06-01 12:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 33608: Redirect UpdateStats to Koha namespace (13.24 KB, patch)
2023-06-01 12:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 33608: Polish Koha::Statistic further (9.00 KB, patch)
2023-06-01 12:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 33608: Move older Stats tests to Koha/Statistics.t (14.10 KB, patch)
2023-06-01 12:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 33608: (follow-up) Default value for interface column (1.11 KB, patch)
2023-06-01 12:24 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 33608: Add two new circulation types in Statistics module (1.39 KB, patch)
2023-07-19 14:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33608: Add UpdateStats to item->store (2.38 KB, patch)
2023-07-19 14:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33608: Rearranging Stats.t, removing useless t version (2.68 KB, patch)
2023-07-19 14:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33608: Correct indentation in Stats.t (12.31 KB, patch)
2023-07-19 14:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33608: Redirect UpdateStats to Koha namespace (13.31 KB, patch)
2023-07-19 14:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33608: Polish Koha::Statistic further (9.06 KB, patch)
2023-07-19 14:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33608: Move older Stats tests to Koha/Statistics.t (14.16 KB, patch)
2023-07-19 14:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33608: (follow-up) Default value for interface column (1.17 KB, patch)
2023-07-19 14:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33608: (QA follow-up) Tidy (14.81 KB, patch)
2023-07-19 14:11 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33608: Add two new circulation types in Statistics module (1.42 KB, patch)
2023-07-20 08:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Add UpdateStats to item->store (2.42 KB, patch)
2023-07-20 08:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Rearranging Stats.t, removing useless t version (2.72 KB, patch)
2023-07-20 08:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Correct indentation in Stats.t (12.35 KB, patch)
2023-07-20 08:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Redirect UpdateStats to Koha namespace (13.35 KB, patch)
2023-07-20 08:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Polish Koha::Statistic further (9.68 KB, patch)
2023-07-20 08:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Move older Stats tests to Koha/Statistics.t (14.92 KB, patch)
2023-07-20 08:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: (follow-up) Default value for interface column (1.21 KB, patch)
2023-07-20 08:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: (QA follow-up) Tidy (14.89 KB, patch)
2023-07-20 08:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: (QA follow-up) Restore older gt zero tests (4.18 KB, patch)
2023-07-20 08:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Polish Koha::Statistic further (9.68 KB, patch)
2023-07-20 09:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Move older Stats tests to Koha/Statistics.t (15.03 KB, patch)
2023-07-20 09:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: (follow-up) Default value for interface column (1.21 KB, patch)
2023-07-20 09:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: (QA follow-up) Tidy (15.00 KB, patch)
2023-07-20 09:33 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: (QA follow-up) Restore older gt zero tests (4.18 KB, patch)
2023-07-20 09:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: (QA follow-up) Restore older gt zero tests (4.24 KB, patch)
2023-07-20 09:34 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: (QA follow-up) Restore older gt zero tests (4.26 KB, patch)
2023-07-21 09:44 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 33608: Add two new circulation types in Statistics module (1.42 KB, patch)
2023-10-19 14:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Add UpdateStats to item->store (2.42 KB, patch)
2023-10-19 14:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Rearranging Stats.t, removing useless t version (2.72 KB, patch)
2023-10-19 14:09 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Correct indentation in Stats.t (12.35 KB, patch)
2023-10-19 14:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Redirect UpdateStats to Koha namespace (13.30 KB, patch)
2023-10-19 14:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Polish Koha::Statistic further (11.14 KB, patch)
2023-10-19 14:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: Move older Stats tests to Koha/Statistics.t (14.97 KB, patch)
2023-10-19 14:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: (follow-up) Default value for interface column (1.35 KB, patch)
2023-10-19 14:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: (QA follow-up) Restore older gt zero tests (4.59 KB, patch)
2023-10-19 14:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33608: (QA follow-up) Remove ->insert method (4.53 KB, patch)
2023-10-19 14:10 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Marcel de Rooy 2023-04-25 14:51:05 UTC

    
Comment 1 Katrin Fischer 2023-05-01 13:09:00 UTC
Hi Marcel, can you explain this a bit more? Is this about logging when an item's lost status is lifted?
Comment 2 Marcel de Rooy 2023-05-02 14:52:45 UTC
(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.
Comment 3 Marcel de Rooy 2023-05-09 11:24:48 UTC Comment hidden (obsolete)
Comment 4 Marcel de Rooy 2023-05-09 11:24:50 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2023-05-09 11:24:53 UTC Comment hidden (obsolete)
Comment 6 Marcel de Rooy 2023-05-09 11:24:55 UTC Comment hidden (obsolete)
Comment 7 Marcel de Rooy 2023-05-09 11:24:57 UTC Comment hidden (obsolete)
Comment 8 Marcel de Rooy 2023-05-09 11:24:59 UTC Comment hidden (obsolete)
Comment 9 Marcel de Rooy 2023-05-09 11:25:01 UTC Comment hidden (obsolete)
Comment 10 Andrew Fuerste-Henry 2023-05-09 21:36:20 UTC
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.
Comment 11 Marcel de Rooy 2023-05-10 11:20:22 UTC
(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.
Comment 12 Marcel de Rooy 2023-05-11 08:31:50 UTC
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>
Comment 13 Marcel de Rooy 2023-05-11 08:31:53 UTC
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>
Comment 14 Marcel de Rooy 2023-05-11 08:31:55 UTC
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>
Comment 15 Marcel de Rooy 2023-05-11 08:31:57 UTC
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>
Comment 16 Marcel de Rooy 2023-05-11 08:32:00 UTC
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>
Comment 17 Marcel de Rooy 2023-05-11 08:32:02 UTC
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>
Comment 18 Marcel de Rooy 2023-05-11 08:32:04 UTC
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>
Comment 19 Marcel de Rooy 2023-05-11 08:42:24 UTC
Rebased on top of 30928. More laborious than hoped for ;)
Comment 20 Owen Leonard 2023-05-26 11:55:19 UTC
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?
Comment 21 Marcel de Rooy 2023-05-26 12:23:10 UTC
(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
Comment 22 Owen Leonard 2023-05-26 18:04:26 UTC
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
Comment 23 Marcel de Rooy 2023-05-26 18:31:42 UTC
(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.
Comment 24 Owen Leonard 2023-05-30 17:20:57 UTC
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  |
+---------------------+--------+--------+------------+-----------+
Comment 25 Marcel de Rooy 2023-06-01 11:34:28 UTC
(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.
Comment 26 Marcel de Rooy 2023-06-01 12:01:23 UTC
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'.
Comment 27 Marcel de Rooy 2023-06-01 12:10:27 UTC
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>
Comment 28 Marcel de Rooy 2023-06-01 12:12:59 UTC
(In reply to Marcel de Rooy from comment #26)

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

Opened byg 33879
Comment 29 Owen Leonard 2023-06-01 12:24:41 UTC
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>
Comment 30 Owen Leonard 2023-06-01 12:24:43 UTC
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>
Comment 31 Owen Leonard 2023-06-01 12:24:46 UTC
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>
Comment 32 Owen Leonard 2023-06-01 12:24:48 UTC
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>
Comment 33 Owen Leonard 2023-06-01 12:24:51 UTC
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>
Comment 34 Owen Leonard 2023-06-01 12:24:53 UTC
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>
Comment 35 Owen Leonard 2023-06-01 12:24:56 UTC
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>
Comment 36 Owen Leonard 2023-06-01 12:24:59 UTC
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>
Comment 37 Martin Renvoize 2023-07-19 14:11:04 UTC
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>
Comment 38 Martin Renvoize 2023-07-19 14:11:07 UTC
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>
Comment 39 Martin Renvoize 2023-07-19 14:11:09 UTC
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>
Comment 40 Martin Renvoize 2023-07-19 14:11:12 UTC
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>
Comment 41 Martin Renvoize 2023-07-19 14:11:15 UTC
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>
Comment 42 Martin Renvoize 2023-07-19 14:11:17 UTC
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>
Comment 43 Martin Renvoize 2023-07-19 14:11:20 UTC
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>
Comment 44 Martin Renvoize 2023-07-19 14:11:22 UTC
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>
Comment 45 Martin Renvoize 2023-07-19 14:11:25 UTC
Created attachment 153661 [details] [review]
Bug 33608: (QA follow-up) Tidy

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 46 Martin Renvoize 2023-07-19 14:14:48 UTC
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..
Comment 47 Marcel de Rooy 2023-07-19 14:23:45 UTC
Will address the >0 concern and look at test coverage
Comment 48 Marcel de Rooy 2023-07-19 14:24:56 UTC
(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
Comment 49 Marcel de Rooy 2023-07-19 14:50:46 UTC
(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.
Comment 50 Marcel de Rooy 2023-07-20 08:50:58 UTC
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>
Comment 51 Marcel de Rooy 2023-07-20 08:51:01 UTC
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>
Comment 52 Marcel de Rooy 2023-07-20 08:51:04 UTC
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>
Comment 53 Marcel de Rooy 2023-07-20 08:51:07 UTC
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>
Comment 54 Marcel de Rooy 2023-07-20 08:51:10 UTC
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>
Comment 55 Marcel de Rooy 2023-07-20 08:51:13 UTC
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>
Comment 56 Marcel de Rooy 2023-07-20 08:51:16 UTC
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>
Comment 57 Marcel de Rooy 2023-07-20 08:51:19 UTC
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>
Comment 58 Marcel de Rooy 2023-07-20 08:51:22 UTC
Created attachment 153706 [details] [review]
Bug 33608: (QA follow-up) Tidy

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 59 Marcel de Rooy 2023-07-20 08:51:25 UTC
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
Comment 60 Marcel de Rooy 2023-07-20 08:53:21 UTC
    # 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
Comment 61 Marcel de Rooy 2023-07-20 08:58:38 UTC
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?
Comment 62 Marcel de Rooy 2023-07-20 09:15:48 UTC
Sorry, am going to still replace the added test of params->{amount} by exists on the 7th patch.
Comment 63 Marcel de Rooy 2023-07-20 09:20:36 UTC
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';
Comment 64 Marcel de Rooy 2023-07-20 09:20:54 UTC
# 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
Comment 65 Marcel de Rooy 2023-07-20 09:30:44 UTC
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,
         }
     );
 }
Comment 66 Marcel de Rooy 2023-07-20 09:31:21 UTC
The spaces before => are now no longer the same. Weird.
Comment 67 Marcel de Rooy 2023-07-20 09:31:42 UTC
I recommend to go with the tidy patch of Martin.
Comment 68 Marcel de Rooy 2023-07-20 09:33:28 UTC
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>
Comment 69 Marcel de Rooy 2023-07-20 09:33:31 UTC
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>
Comment 70 Marcel de Rooy 2023-07-20 09:33:34 UTC
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>
Comment 71 Marcel de Rooy 2023-07-20 09:33:37 UTC
Created attachment 153711 [details] [review]
Bug 33608: (QA follow-up) Tidy

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 72 Marcel de Rooy 2023-07-20 09:34:13 UTC
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
Comment 73 Marcel de Rooy 2023-07-20 09:34:43 UTC
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>
Comment 74 Martin Renvoize 2023-07-21 09:44:55 UTC
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>
Comment 75 Marcel de Rooy 2023-08-25 07:02:31 UTC
Still applies, no warns from qa tools? 
Tomas: Why does this stay in the queue ?
Comment 76 Andrew Fuerste-Henry 2023-08-25 15:49:14 UTC
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.
Comment 77 Tomás Cohen Arazi 2023-08-26 22:52:21 UTC
(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.
Comment 78 Marcel de Rooy 2023-08-28 08:40:15 UTC
(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.
Comment 79 Tomás Cohen Arazi 2023-08-29 14:33:44 UTC
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.
Comment 80 Marcel de Rooy 2023-08-29 14:47:54 UTC
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..
Comment 81 Tomás Cohen Arazi 2023-08-29 14:55:11 UTC
(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.
Comment 82 Marcel de Rooy 2023-10-19 13:58:24 UTC
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.
Comment 83 Marcel de Rooy 2023-10-19 13:59:20 UTC
Most time spent here is tidy again and again..
Comment 84 Marcel de Rooy 2023-10-19 14:09:44 UTC
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>
Comment 85 Marcel de Rooy 2023-10-19 14:09:47 UTC
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>
Comment 86 Marcel de Rooy 2023-10-19 14:09:50 UTC
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>
Comment 87 Marcel de Rooy 2023-10-19 14:10:03 UTC
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>
Comment 88 Marcel de Rooy 2023-10-19 14:10:17 UTC
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
Comment 89 Marcel de Rooy 2023-10-19 14:10:19 UTC
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.
Comment 90 Marcel de Rooy 2023-10-19 14:10:22 UTC
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.
Comment 91 Marcel de Rooy 2023-10-19 14:10:25 UTC
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>
Comment 92 Marcel de Rooy 2023-10-19 14:10:28 UTC
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>
Comment 93 Marcel de Rooy 2023-10-19 14:10:30 UTC
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.
Comment 94 Marcel de Rooy 2023-10-19 14:12:00 UTC
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.
Comment 95 Marcel de Rooy 2023-10-19 14:14:37 UTC
Gonna move this back to PQA given the nature of the changes. It is mostly tide here and there. Change POD etc. Nothing fundamental.
Comment 96 Jonathan Druart 2023-10-20 08:21:50 UTC
(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...
Comment 97 Tomás Cohen Arazi 2023-10-25 14:09:53 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 98 Tomás Cohen Arazi 2023-10-25 16:57:35 UTC
This doesn't seem to have added tests for ->store()...
Comment 99 Marcel de Rooy 2023-10-26 06:10:34 UTC
(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*' );
+    };
Comment 100 Marcel de Rooy 2023-10-26 06:18:03 UTC
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?