[Bug 33608] New: Allow to get statistics about found books
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Bug ID: 33608 Summary: Allow to get statistics about found books Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Reports Assignee: koha-bugs@lists.koha-community.org Reporter: m.de.rooy@rijksmuseum.nl QA Contact: testopia@bugs.koha-community.org -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |m.de.rooy@rijksmuseum.nl |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |33636 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33636 [Bug 33636] Move C4::Stats to Koha namespace -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=30928 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Summary|Allow to get statistics |Allow to get statistics |about found books |about found/recovered books -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #1 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Hi Marcel, can you explain this a bit more? Is this about logging when an item's lost status is lifted? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #2 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Small patch Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #3 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 150840 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150840&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 150841 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150841&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #5 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 150842 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150842&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #6 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 150843 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150843&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #7 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 150844 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150844&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #8 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 150845 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150845&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #9 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 150846 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=150846&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Andrew Fuerste-Henry <andrewfh@dubcolib.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrewfh@dubcolib.org --- Comment #10 from Andrew Fuerste-Henry <andrewfh@dubcolib.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #11 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150840|0 |1 is obsolete| | --- Comment #12 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151051 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151051&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150841|0 |1 is obsolete| | --- Comment #13 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151052 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151052&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150842|0 |1 is obsolete| | --- Comment #14 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151053 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151053&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150843|0 |1 is obsolete| | --- Comment #15 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151054 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151054&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150844|0 |1 is obsolete| | --- Comment #16 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151055 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151055&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150845|0 |1 is obsolete| | --- Comment #17 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151056 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151056&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #150846|0 |1 is obsolete| | --- Comment #18 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151057 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151057&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #19 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Rebased on top of 30928. More laborious than hoped for ;) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #20 from Owen Leonard <oleonard@myacpl.org> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #21 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #22 from Owen Leonard <oleonard@myacpl.org> --- 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
-- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #23 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #24 from Owen Leonard <oleonard@myacpl.org> --- 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 | +---------------------+--------+--------+------------+-----------+ -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #25 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart+koha@gmail. | |com --- Comment #26 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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'. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #27 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 151913 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151913&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=33879 --- Comment #28 from 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151051|0 |1 is obsolete| | Attachment #151052|0 |1 is obsolete| | Attachment #151053|0 |1 is obsolete| | Attachment #151054|0 |1 is obsolete| | Attachment #151055|0 |1 is obsolete| | Attachment #151056|0 |1 is obsolete| | Attachment #151057|0 |1 is obsolete| | Attachment #151913|0 |1 is obsolete| | --- Comment #29 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 151915 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151915&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #30 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 151916 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151916&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #31 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 151917 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151917&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #32 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 151918 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151918&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #33 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 151919 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151919&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #34 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 151920 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151920&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #35 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 151921 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151921&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #36 from Owen Leonard <oleonard@myacpl.org> --- Created attachment 151922 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=151922&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151915|0 |1 is obsolete| | --- Comment #37 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 153653 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153653&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151916|0 |1 is obsolete| | --- Comment #38 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 153654 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153654&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151917|0 |1 is obsolete| | --- Comment #39 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 153655 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153655&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151918|0 |1 is obsolete| | --- Comment #40 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 153656 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153656&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151919|0 |1 is obsolete| | --- Comment #41 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 153657 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153657&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151920|0 |1 is obsolete| | --- Comment #42 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 153658 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153658&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151921|0 |1 is obsolete| | --- Comment #43 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 153659 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153659&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #151922|0 |1 is obsolete| | --- Comment #44 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 153660 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153660&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #45 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 153661 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153661&action=edit Bug 33608: (QA follow-up) Tidy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA CC| |martin.renvoize@ptfs-europe | |.com --- Comment #46 from 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.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@ptfs-europe |y.org |.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |BLOCKED --- Comment #47 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Will address the >0 concern and look at test coverage -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #48 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=34308 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #49 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |34316 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34316 [Bug 34316] account->add_credit adds tries to add wrong statistic type -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153653|0 |1 is obsolete| | --- Comment #50 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153698 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153698&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153654|0 |1 is obsolete| | --- Comment #51 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153699 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153699&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153655|0 |1 is obsolete| | --- Comment #52 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153700 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153700&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153656|0 |1 is obsolete| | --- Comment #53 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153701 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153701&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153657|0 |1 is obsolete| | --- Comment #54 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153702 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153702&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153658|0 |1 is obsolete| | --- Comment #55 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153703 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153703&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153659|0 |1 is obsolete| | --- Comment #56 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153704 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153704&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153660|0 |1 is obsolete| | --- Comment #57 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153705 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153705&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153661|0 |1 is obsolete| | --- Comment #58 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153706 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153706&action=edit Bug 33608: (QA follow-up) Tidy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #59 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153707 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153707&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #60 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- # 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #61 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #62 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Sorry, am going to still replace the added test of params->{amount} by exists on the 7th patch. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #63 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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'; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #64 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- # 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #65 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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, } ); } -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #66 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- The spaces before => are now no longer the same. Weird. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #67 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- I recommend to go with the tidy patch of Martin. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153703|0 |1 is obsolete| | --- Comment #68 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153708 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153708&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153704|0 |1 is obsolete| | --- Comment #69 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153709 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153709&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153705|0 |1 is obsolete| | --- Comment #70 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153710 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153710&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153706|0 |1 is obsolete| | --- Comment #71 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153711 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153711&action=edit Bug 33608: (QA follow-up) Tidy Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153707|0 |1 is obsolete| | --- Comment #72 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153712 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153712&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153712|0 |1 is obsolete| | --- Comment #73 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 153713 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153713&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Katie Bliss <kebliss@dmpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |kebliss@dmpl.org -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153713|0 |1 is obsolete| | --- Comment #74 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 153765 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=153765&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #75 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Still applies, no warns from qa tools? Tomas: Why does this stay in the queue ? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #76 from Andrew Fuerste-Henry <andrewfh@dubcolib.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |rel_23_11_candidate -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #77 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #78 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |34628 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34628 [Bug 34628] Lost item statistics are not copied to pseudonymized transactions -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #79 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #80 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #81 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #82 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #83 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Most time spent here is tidy again and again.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153698|0 |1 is obsolete| | Attachment #153711|0 |1 is obsolete| | --- Comment #84 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157440 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157440&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153699|0 |1 is obsolete| | --- Comment #85 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157441 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157441&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153700|0 |1 is obsolete| | --- Comment #86 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157442 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157442&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153701|0 |1 is obsolete| | --- Comment #87 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157443 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157443&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153702|0 |1 is obsolete| | --- Comment #88 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157444 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157444&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153708|0 |1 is obsolete| | --- Comment #89 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157445 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157445&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153709|0 |1 is obsolete| | --- Comment #90 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157446 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157446&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153710|0 |1 is obsolete| | --- Comment #91 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157447 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157447&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #153765|0 |1 is obsolete| | --- Comment #92 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157448 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157448&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #93 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Created attachment 157449 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=157449&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #94 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA --- Comment #95 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- Gonna move this back to PQA given the nature of the changes. It is mostly tide here and there. Change POD etc. Nothing fundamental. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #96 from Jonathan Druart <jonathan.druart+koha@gmail.com> --- (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... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to master Version(s)| |23.11.00 released in| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #97 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Pushed to master for 23.11. Nice work everyone, thanks! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |34611 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=34611 [Bug 34611] Add a script for pseudonymizing existing data -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #98 from Tomás Cohen Arazi <tomascohen@gmail.com> --- This doesn't seem to have added tests for ->store()... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #99 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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*' ); + }; -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=33608 --- Comment #100 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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? -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org