https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Patch complexity|--- |Small patch Status|NEW |Needs Signoff -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #2 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197711 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197711&action=edit Bug 42418: Wire Koha::Checkin into AddReturn Every call to AddReturn now creates a Koha::Checkin record capturing the full context and outcomes of the return scan: - user_id and desk_id from C4::Context->userenv - library_id from the branch parameter - checkout_id from the issue being returned (old_issues) - transfer_id from $messages->{WasTransfered} - hold_id from $messages->{ResFound}{reserve_id} The checkin record is also passed to the after_circ_action plugin hook, so plugins can access it directly instead of reconstructing context from side effects. AddReturn now returns a fifth value: the Koha::Checkin object. Existing callers are unaffected — Perl silently ignores extra return values. Assisted-by: Sonnet 4.6 (Anthropic) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Lisette Scheer <lisette@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |lisette@bywatersolutions.co | |m -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197710|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #4 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197715 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197715&action=edit Bug 42418: Add checkins table and Koha::Checkin object Introduces checkins as a first-class resource with its own table, tracking the full context and outcomes of a return scan: - who: user_id (staff member), desk_id (optional) - what: item_id - where: library_id - when: timestamp - outcomes: checkout_id (returned), transfer_id (triggered), hold_id (filled) New files: - Koha::Checkin / Koha::Checkins (Koha::Object classes) - Koha::Schema::Result::Checkin (DBIC, generated via dbic) - atomicupdate/bug_42418.pl + kohastructure.sql Test plan: 1. Apply patch 2. Run updatedatabase.pl 3. Verify checkins table exists with correct structure 4. Sign off :-D Assisted-by: Sonnet 4.6 (Anthropic) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |tomascohen@gmail.com --- Comment #6 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Lisette Scheer from comment #3)

Would be helpful to have a clear path to deleted_items so when an item is deleted the link isn't lost.

The deleteditems pattern is a known anti-pattern we'd like to move away from. I'm not sure we want to add a FK here perpetuating it. Happy to discuss it in a broader context.

Also might be worth adding a local_use column for flagging if a checkin was a local use. Would make tracking that more effectively, especially if there could be a checkbox or something so local use could be specifically set.

Done! Thanks for bringing that up! I totally missed it, and fits perfectly within the reasons we need a proper checkins table :-D

I know an adjustment to me when I came to Koha was that my old ils had a separate tab for local_use checkins and things like holds we'd pull and check in to fill the hold wouldn't count as local use since it wasn't really used locally, just pulled by staff to fill the hold.

This sounds great. File a bug depending on this one so we discuss it in context! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197716|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #8 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197723 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197723&action=edit Bug 42418: Wire Koha::Checkin into AddReturn Every call to AddReturn now creates a Koha::Checkin record capturing the full context and outcomes of the return scan: - user_id and desk_id from C4::Context->userenv - library_id from the branch parameter - checkout_id from the issue being returned (old_issues) - transfer_id from $messages->{WasTransfered} - hold_id from $messages->{ResFound}{reserve_id} The checkin record is also passed to the after_circ_action plugin hook, so plugins can access it directly instead of reconstructing context from side effects. AddReturn now returns a fifth value: the Koha::Checkin object. Existing callers are unaffected — Perl silently ignores extra return values. Assisted-by: Sonnet 4.6 (Anthropic) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@openfifth.c | |o.uk -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #11 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Katrin Fischer from comment #10)

Can you explain a bit how this will affect old_issues or if there are plans for old_issues down the road?

Another solution could have been to add additional columns to old issues as that represents a checkin currently. I guess you had reasons not to go this route, but am a bit worried about the duplicated data.

I always think of `old_issues` as the `issues` table with information about a special status of the checkout: returned (which is inferred from a return date being set). With that in mind my only plan is to merge those tables in the mid (?) term. But -regardless of what we do with those tables- this development highlights that the checkin action can (or not) trigger some status change for a checkout, and many more things. All of those nicely handled and tested here :-). Taking accountlines as an example, I will propose we ditch the old_issues link and replace with a checkin link. That way we actually gain tracking possibilities for (say) refunding lost fines even when there might not be a linked checkout, and things like that. The link to old checkouts could be reconstructed through the checkin link. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197722|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #13 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197863 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197863&action=edit Bug 42418: Add checkins table and Koha::Checkin object Introduces checkins as a first-class resource with its own table, tracking the full context and outcomes of a return scan: - who: user_id (staff member), desk_id (optional) - what: item_id - where: library_id - when: timestamp - outcomes: checkout_id (returned), transfer_id (triggered), hold_id (filled) New files: - Koha::Checkin / Koha::Checkins (Koha::Object classes) - Koha::Schema::Result::Checkin (DBIC, generated via dbic) - atomicupdate/bug_42418.pl + kohastructure.sql Test plan: 1. Apply patch 2. Run updatedatabase.pl 3. Verify checkins table exists with correct structure 4. Sign off :-D Assisted-by: Sonnet 4.6 (Anthropic) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #15 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 197865 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=197865&action=edit Bug 42418: (follow-up) Make checkins.user_id nullable SIP and CLI-driven returns have no userenv, so user_id must accept NULL. The FK action is also changed from CASCADE to SET NULL so deleting a staff patron preserves the checkin history. Changes: - kohastructure.sql: user_id DEFAULT NULL, FK ON DELETE SET NULL - atomicupdate: same - DBIC schema regenerated - Test updated to expect SET NULL behavior -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #17 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- This looks to be a solid concept to me! It will definitely be useful to know when an item was scanned in with or without a return being processed. The conversation has drifted a bit, so here are my two bits; If we created a new table for all of the $table/old_$table combos, we could we could recreate the existing current/old tables as views to avoid the need to rewrite reports. It looks like DBIC also supports views, but I haven't looked into it deeply. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #197864|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #18 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198164 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198164&action=edit Bug 42418: DB changes Adds the checkins table to track individual checkin events with references to the item, staff member, library, desk, checkout, transfer, hold, recall, restriction, and return claim involved. Includes backfill of historical checkin records from old_issues. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #20 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198166 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198166&action=edit Bug 42418: Add Koha::Checkin and Koha::Checkins classes Koha::Object-based classes for the checkins table, with tests. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #22 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198168 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198168&action=edit Bug 42418: Wire Koha::Checkin into AddReturn Creates a Koha::Checkin record during AddReturn, capturing the checkin context (item, staff, library, desk, checkout, transfer, hold, recall, restriction, claim). The checkin_id is passed to the after_circ_action hook payload for plugin consumption. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198165|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198167|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #23 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198169 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198169&action=edit Bug 42418: DB changes Adds the checkins table to track individual checkin events with references to the item, staff member, library, desk, checkout, transfer, hold, recall, restriction, and return claim involved. Includes backfill of historical checkin records from old_issues. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #25 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198171 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198171&action=edit Bug 42418: Add Koha::Checkin and Koha::Checkins classes Koha::Object-based classes for the checkins table, with tests. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #27 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198173 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198173&action=edit Bug 42418: Wire Koha::Checkin into AddReturn Creates a Koha::Checkin record during AddReturn, capturing the checkin context (item, staff, library, desk, checkout, transfer, hold, recall, restriction, claim). The checkin_id is passed to the after_circ_action hook payload for plugin consumption. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Kyle M Hall (khall) <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #29 from Kyle M Hall (khall) <kyle@bywatersolutions.com> --- prove t/db_dependent/Circulation.t t/db_dependent/Circulation.t .. 30/87 # Transfer limit: Holding -> Return # Transfer limit: Return -> Home # Transfer limit: Return -> Holding # Looks like you planned 3 tests but ran 2. # Failed test 'AddReturn + TransferLimits' # at t/db_dependent/Circulation.t line 3217. Broken FK constraint # Failed test 'no warnings' # at /usr/share/perl/5.36/Test/Builder.pm line 193. # There were 1 warning(s) # Previous test 2 'Item cannot be returned if there is a transfer to do (item is floating)' # DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`checkins`, CONSTRAINT `checkins_ibfk_2` FOREIGN KEY (`user_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/Koha/Object.pm line 174 # at /kohadevbox/koha/Koha/Object.pm line 178. # Koha::Object::catch {...} (DBIx::Class::Exception=HASH(0xaaaad545efd8)) called at /usr/share/perl5/Try/Tiny.pm line 123 # Try::Tiny::try(CODE(0xaaaad5327910), Try::Tiny::Catch=REF(0xaaaad57ae988)) called at /kohadevbox/koha/Koha/Object.pm line 183 # Koha::Object::store(Koha::Checkin=HASH(0xaaaacf983888)) called at /kohadevbox/koha/C4/Circulation.pm line 2842 # C4::Circulation::AddReturn("rDenWVmihu00aHTJS", "UihEz4gC") called at t/db_dependent/Circulation.t line 3215 # main::__ANON__() called at /usr/share/perl/5.36/Test/Builder.pm line 374 # eval {...} called at /usr/share/perl/5.36/Test/Builder.pm line 374 # Test::Builder::subtest(Test::Builder=HASH(0xaaaac96e2c20), "AddReturn + TransferLimits", CODE(0xaaaad5381bd8)) called at /usr/share/perl/5.36/Test/More.pm line 809 # Test::More::subtest("AddReturn + TransferLimits", CODE(0xaaaad5381bd8)) called at t/db_dependent/Circulation.t line 3217 # # Looks like your test exited with 11 just after 34. t/db_dependent/Circulation.t .. Dubious, test returned 11 (wstat 2816, 0xb00) Failed 55/87 subtests Test Summary Report ------------------- t/db_dependent/Circulation.t (Wstat: 2816 (exited 11) Tests: 34 Failed: 2) Failed tests: 33-34 Non-zero exit status: 11 Parse errors: Bad plan. You planned 87 tests but ran 34. Files=1, Tests=34, 10 wallclock secs ( 0.03 usr 0.01 sys + 7.42 cusr 1.34 csys = 8.80 CPU) Result: FAIL -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #31 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- (In reply to Kyle M Hall (khall) from comment #29)

# DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`checkins`, CONSTRAINT `checkins_ibfk_2` FOREIGN KEY (`user_id`) REFERENCES `borrowers` (`borrowernumber`) ON DELETE SET NULL ON UPDATE CASCADE) at /kohadevbox/koha/Koha/Object.pm line 174

That's because the mocked userenv (I reused) is pointing to a non-existent patron. I'll provide a fix, but the fallback to `borrowernumber=51` (the default) is a ticking bomb I'd say. Gonna file a separate (follow-up) bug report to make `mock_userenv` create a patron if not passed. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198169|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198171|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198173|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #33 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198556 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198556&action=edit Bug 42418: Add Koha::Schema::Result::Checkin DBIC Result class for the new checkins table. Includes relationships to items, borrowers, branches, desks, old_issues, branchtransfers, reserves, recalls, borrower_debarments, and return_claims. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #35 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198558 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198558&action=edit Bug 42418: DBIC schema changes [DO NOT PUSH] Auto-generated has_many relationships added to related Result classes for the new checkins table. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #37 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 198560 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198560&action=edit Bug 42418: (follow-up) Fix tests using invalid mock_userenv The checkins table has a FK on user_id pointing to borrowers. Several mock_userenv calls only passed branchcode, which defaults borrowernumber to the hardcoded value 51 — a non-existent patron. This causes FK violations when AddReturn creates a checkin record. This patch fixes the affected calls in Circulation.t and Branch.t by passing a valid patron to mock_userenv. It also adjusts the stock rotation transfer test to account for the new 5th return value from AddReturn (the Koha::Checkin object), and adds tests for the three checkin record user_id scenarios (valid patron, missing number, explicit undef number). A global fix to mock_userenv (auto-generating a patron when none is passed) would address the ~120 remaining calls across the test suite, but the risk surface warrants a separate bug. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Andrew Fuerste-Henry <andrew@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |andrew@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|24401 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24401 [Bug 24401] Add POST /checkins endpoint with availability check and confirmation flow -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 David Nind <david@davidnind.com> 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=42418 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198556|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198558|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198560|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198632|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #41 from David Nind <david@davidnind.com> --- Created attachment 198678 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198678&action=edit Bug 42418: Add Koha::Schema::Result::Checkin DBIC Result class for the new checkins table. Includes relationships to items, borrowers, branches, desks, old_issues, branchtransfers, reserves, recalls, borrower_debarments, and return_claims. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #43 from David Nind <david@davidnind.com> --- Created attachment 198680 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198680&action=edit Bug 42418: DBIC schema changes [DO NOT PUSH] Auto-generated has_many relationships added to related Result classes for the new checkins table. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #45 from David Nind <david@davidnind.com> --- Created attachment 198682 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198682&action=edit Bug 42418: (follow-up) Fix tests using invalid mock_userenv The checkins table has a FK on user_id pointing to borrowers. Several mock_userenv calls only passed branchcode, which defaults borrowernumber to the hardcoded value 51 — a non-existent patron. This causes FK violations when AddReturn creates a checkin record. This patch fixes the affected calls in Circulation.t and Branch.t by passing a valid patron to mock_userenv. It also adjusts the stock rotation transfer test to account for the new 5th return value from AddReturn (the Koha::Checkin object), and adds tests for the three checkin record user_id scenarios (valid patron, missing number, explicit undef number). A global fix to mock_userenv (auto-generating a patron when none is passed) would address the ~120 remaining calls across the test suite, but the risk surface warrants a separate bug. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #47 from David Nind <david@davidnind.com> --- Created attachment 198684 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=198684&action=edit Bug 42418: (follow-up) Fix Patrons.t for checkin record creation AddReturn now creates a checkin record which requires a valid library_id. The anonymize_last_borrowers subtests call AddReturn without a userenv set, causing a NOT NULL constraint violation. This adds mock_userenv before the AddReturn calls. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Patrons.t => SUCCESS: All 52 tests pass! 3. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #198677|0 |1 is obsolete| | Attachment #198678|0 |1 is obsolete| | Attachment #198679|0 |1 is obsolete| | Attachment #198680|0 |1 is obsolete| | Attachment #198681|0 |1 is obsolete| | Attachment #198682|0 |1 is obsolete| | Attachment #198683|0 |1 is obsolete| | Attachment #198684|0 |1 is obsolete| | Attachment #201487|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #50 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201931 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201931&action=edit Bug 42418: Add Koha::Schema::Result::Checkin DBIC Result class for the new checkins table. Includes relationships to items, borrowers, branches, desks, old_issues, branchtransfers, reserves, recalls, borrower_debarments, and return_claims. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #52 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201933 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201933&action=edit Bug 42418: DBIC schema changes [DO NOT PUSH] Auto-generated has_many relationships added to related Result classes for the new checkins table. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #54 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201935 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201935&action=edit Bug 42418: (follow-up) Fix tests using invalid mock_userenv The checkins table has a FK on user_id pointing to borrowers. Several mock_userenv calls only passed branchcode, which defaults borrowernumber to the hardcoded value 51 — a non-existent patron. This causes FK violations when AddReturn creates a checkin record. This patch fixes the affected calls in Circulation.t and Branch.t by passing a valid patron to mock_userenv. It also adjusts the stock rotation transfer test to account for the new 5th return value from AddReturn (the Koha::Checkin object), and adds tests for the three checkin record user_id scenarios (valid patron, missing number, explicit undef number). A global fix to mock_userenv (auto-generating a patron when none is passed) would address the ~120 remaining calls across the test suite, but the risk surface warrants a separate bug. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #56 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201937 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201937&action=edit Bug 42418: (follow-up) Fix Patrons.t for checkin record creation AddReturn now creates a checkin record which requires a valid library_id. The anonymize_last_borrowers subtests call AddReturn without a userenv set, causing a NOT NULL constraint violation. This adds mock_userenv before the AddReturn calls. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Patrons.t => SUCCESS: All 52 tests pass! 3. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #57 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201943 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201943&action=edit Bug 42418: DB changes Adds the checkins table to track individual checkin events with references to the item, staff member, library, desk, checkout, transfer, hold, recall, restriction, and return claim involved. Includes backfill of historical checkin records from old_issues. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #59 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201945 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201945&action=edit Bug 42418: Add Koha::Checkin and Koha::Checkins classes Koha::Object-based classes for the checkins table, with tests. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #61 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201947 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201947&action=edit Bug 42418: Wire Koha::Checkin into AddReturn Creates a Koha::Checkin record during AddReturn, capturing the checkin context (item, staff, library, desk, checkout, transfer, hold, recall, restriction, claim). The checkin_id is passed to the after_circ_action hook payload for plugin consumption. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #63 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 201949 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=201949&action=edit Bug 42418: (follow-up) Skip old_issues with no library on backfill Both checkin_library and branchcode are nullable in old_issues. The COALESCE can return NULL, which violates the NOT NULL constraint on checkins.library_id. Filter out those rows. Signed-off-by: David Nind <david@davidnind.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #201943|0 |1 is obsolete| | Attachment #201944|0 |1 is obsolete| | Attachment #201945|0 |1 is obsolete| | Attachment #201946|0 |1 is obsolete| | Attachment #201947|0 |1 is obsolete| | Attachment #201948|0 |1 is obsolete| | Attachment #201949|0 |1 is obsolete| | Attachment #201950|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #66 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 204154 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204154&action=edit Bug 42418: Add Koha::Schema::Result::Checkin DBIC Result class for the new checkins table. Includes relationships to items, borrowers, branches, desks, old_issues, branchtransfers, reserves, recalls, borrower_debarments, and return_claims. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #68 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 204156 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204156&action=edit Bug 42418: DBIC schema changes [DO NOT PUSH] Auto-generated has_many relationships added to related Result classes for the new checkins table. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #70 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 204158 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204158&action=edit Bug 42418: (follow-up) Fix tests using invalid mock_userenv The checkins table has a FK on user_id pointing to borrowers. Several mock_userenv calls only passed branchcode, which defaults borrowernumber to the hardcoded value 51 — a non-existent patron. This causes FK violations when AddReturn creates a checkin record. This patch fixes the affected calls in Circulation.t and Branch.t by passing a valid patron to mock_userenv. It also adjusts the stock rotation transfer test to account for the new 5th return value from AddReturn (the Koha::Checkin object), and adds tests for the three checkin record user_id scenarios (valid patron, missing number, explicit undef number). A global fix to mock_userenv (auto-generating a patron when none is passed) would address the ~120 remaining calls across the test suite, but the risk surface warrants a separate bug. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #72 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 204160 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204160&action=edit Bug 42418: (follow-up) Fix Patrons.t for checkin record creation AddReturn now creates a checkin record which requires a valid library_id. The anonymize_last_borrowers subtests call AddReturn without a userenv set, causing a NOT NULL constraint violation. This adds mock_userenv before the AddReturn calls. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Patrons.t => SUCCESS: All 52 tests pass! 3. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #204153|0 |1 is obsolete| | Attachment #204154|0 |1 is obsolete| | Attachment #204155|0 |1 is obsolete| | Attachment #204156|0 |1 is obsolete| | Attachment #204157|0 |1 is obsolete| | Attachment #204158|0 |1 is obsolete| | Attachment #204159|0 |1 is obsolete| | Attachment #204160|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #74 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204905 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204905&action=edit Bug 42418: Add Koha::Schema::Result::Checkin DBIC Result class for the new checkins table. Includes relationships to items, borrowers, branches, desks, old_issues, branchtransfers, reserves, recalls, borrower_debarments, and return_claims. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #76 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204907 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204907&action=edit Bug 42418: DBIC schema changes [DO NOT PUSH] Auto-generated has_many relationships added to related Result classes for the new checkins table. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #78 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204909 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204909&action=edit Bug 42418: (follow-up) Fix tests using invalid mock_userenv The checkins table has a FK on user_id pointing to borrowers. Several mock_userenv calls only passed branchcode, which defaults borrowernumber to the hardcoded value 51 — a non-existent patron. This causes FK violations when AddReturn creates a checkin record. This patch fixes the affected calls in Circulation.t and Branch.t by passing a valid patron to mock_userenv. It also adjusts the stock rotation transfer test to account for the new 5th return value from AddReturn (the Koha::Checkin object), and adds tests for the three checkin record user_id scenarios (valid patron, missing number, explicit undef number). A global fix to mock_userenv (auto-generating a patron when none is passed) would address the ~120 remaining calls across the test suite, but the risk surface warrants a separate bug. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #80 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204911 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204911&action=edit Bug 42418: (follow-up) Fix Patrons.t for checkin record creation AddReturn now creates a checkin record which requires a valid library_id. The anonymize_last_borrowers subtests call AddReturn without a userenv set, causing a NOT NULL constraint violation. This adds mock_userenv before the AddReturn calls. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Patrons.t => SUCCESS: All 52 tests pass! 3. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #82 from Martin Renvoize (ashimema) <martin.renvoize@openfifth.co.uk> --- Created attachment 204913 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=204913&action=edit Bug 42418: (QA follow-up) Fix DataCorrupted return arity and checkin timestamp population Two fixes to how AddReturn manages the Koha::Checkin audit record it creates for every scan: - The checkin record's DB-computed defaults (e.g. timestamp, which defaults to current_timestamp()) were never refreshed from storage after the initial INSERT, so the in-memory object kept holding the literal SQL placeholder DBIx::Class leaves behind instead of the real value. Add ->discard_changes after ->store to fix this. - AddReturn's DataCorrupted error path (fired when MarkIssueReturned dies) still returned the pre-Bug-42418 4-tuple instead of including $checkin_record, and never attached its own messages to it. This crashed the REST controller's to_api($checkin) call on undef, masking the real error behind a generic 500 and leaving the audit row without any explanation of what happened. Add a 'data_corrupted' message case to _attach_messages_to_checkin for this. Test plan: 1. prove t/db_dependent/Circulation.t t/db_dependent/Koha/Checkin.t => SUCCESS: all pass. Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #204904|0 |1 is obsolete| | Attachment #204905|0 |1 is obsolete| | Attachment #204906|0 |1 is obsolete| | Attachment #204907|0 |1 is obsolete| | Attachment #204908|0 |1 is obsolete| | Attachment #204909|0 |1 is obsolete| | Attachment #204910|0 |1 is obsolete| | Attachment #204911|0 |1 is obsolete| | Attachment #204912|0 |1 is obsolete| | Attachment #204913|0 |1 is obsolete| | Attachment #204914|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #85 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 205061 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205061&action=edit Bug 42418: Add Koha::Schema::Result::Checkin DBIC Result class for the new checkins table. Includes relationships to items, borrowers, branches, desks, old_issues, branchtransfers, reserves, recalls, borrower_debarments, and return_claims. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #87 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 205063 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205063&action=edit Bug 42418: DBIC schema changes [DO NOT PUSH] Auto-generated has_many relationships added to related Result classes for the new checkins table. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #89 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 205065 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205065&action=edit Bug 42418: (follow-up) Fix tests using invalid mock_userenv The checkins table has a FK on user_id pointing to borrowers. Several mock_userenv calls only passed branchcode, which defaults borrowernumber to the hardcoded value 51 — a non-existent patron. This causes FK violations when AddReturn creates a checkin record. This patch fixes the affected calls in Circulation.t and Branch.t by passing a valid patron to mock_userenv. It also adjusts the stock rotation transfer test to account for the new 5th return value from AddReturn (the Koha::Checkin object), and adds tests for the three checkin record user_id scenarios (valid patron, missing number, explicit undef number). A global fix to mock_userenv (auto-generating a patron when none is passed) would address the ~120 remaining calls across the test suite, but the risk surface warrants a separate bug. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #91 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 205067 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205067&action=edit Bug 42418: (follow-up) Fix Patrons.t for checkin record creation AddReturn now creates a checkin record which requires a valid library_id. The anonymize_last_borrowers subtests call AddReturn without a userenv set, causing a NOT NULL constraint violation. This adds mock_userenv before the AddReturn calls. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Patrons.t => SUCCESS: All 52 tests pass! 3. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #93 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 205069 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=205069&action=edit Bug 42418: (QA follow-up) Fix DataCorrupted return arity and checkin timestamp population Two fixes to how AddReturn manages the Koha::Checkin audit record it creates for every scan: - The checkin record's DB-computed defaults (e.g. timestamp, which defaults to current_timestamp()) were never refreshed from storage after the initial INSERT, so the in-memory object kept holding the literal SQL placeholder DBIx::Class leaves behind instead of the real value. Add ->discard_changes after ->store to fix this. - AddReturn's DataCorrupted error path (fired when MarkIssueReturned dies) still returned the pre-Bug-42418 4-tuple instead of including $checkin_record, and never attached its own messages to it. This crashed the REST controller's to_api($checkin) call on undef, masking the real error behind a generic 500 and leaving the audit row without any explanation of what happened. Add a 'data_corrupted' message case to _attach_messages_to_checkin for this. Test plan: 1. prove t/db_dependent/Circulation.t t/db_dependent/Koha/Checkin.t => SUCCESS: all pass. Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Bug 42418 depends on bug 41728, which changed state. Bug 41728 Summary: Add `Koha::Item::Availability::Checkin` to centralize logic https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=41728 What |Removed |Added ---------------------------------------------------------------------------- Status|Needs documenting |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #205060|0 |1 is obsolete| | Attachment #205061|0 |1 is obsolete| | Attachment #205062|0 |1 is obsolete| | Attachment #205063|0 |1 is obsolete| | Attachment #205064|0 |1 is obsolete| | Attachment #205065|0 |1 is obsolete| | Attachment #205066|0 |1 is obsolete| | Attachment #205067|0 |1 is obsolete| | Attachment #205068|0 |1 is obsolete| | Attachment #205069|0 |1 is obsolete| | Attachment #205070|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #96 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 208539 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208539&action=edit Bug 42418: Add Koha::Schema::Result::Checkin DBIC Result class for the new checkins table. Includes relationships to items, borrowers, branches, desks, old_issues, branchtransfers, reserves, recalls, borrower_debarments, and return_claims. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #98 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 208541 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208541&action=edit Bug 42418: DBIC schema changes [DO NOT PUSH] Auto-generated has_many relationships added to related Result classes for the new checkins table. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #100 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 208543 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208543&action=edit Bug 42418: (follow-up) Fix tests using invalid mock_userenv The checkins table has a FK on user_id pointing to borrowers. Several mock_userenv calls only passed branchcode, which defaults borrowernumber to the hardcoded value 51 — a non-existent patron. This causes FK violations when AddReturn creates a checkin record. This patch fixes the affected calls in Circulation.t and Branch.t by passing a valid patron to mock_userenv. It also adjusts the stock rotation transfer test to account for the new 5th return value from AddReturn (the Koha::Checkin object), and adds tests for the three checkin record user_id scenarios (valid patron, missing number, explicit undef number). A global fix to mock_userenv (auto-generating a patron when none is passed) would address the ~120 remaining calls across the test suite, but the risk surface warrants a separate bug. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #102 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 208545 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208545&action=edit Bug 42418: (follow-up) Fix Patrons.t for checkin record creation AddReturn now creates a checkin record which requires a valid library_id. The anonymize_last_borrowers subtests call AddReturn without a userenv set, causing a NOT NULL constraint violation. This adds mock_userenv before the AddReturn calls. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Patrons.t => SUCCESS: All 52 tests pass! 3. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #104 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 208547 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208547&action=edit Bug 42418: (QA follow-up) Fix DataCorrupted return arity and checkin timestamp population Two fixes to how AddReturn manages the Koha::Checkin audit record it creates for every scan: - The checkin record's DB-computed defaults (e.g. timestamp, which defaults to current_timestamp()) were never refreshed from storage after the initial INSERT, so the in-memory object kept holding the literal SQL placeholder DBIx::Class leaves behind instead of the real value. Add ->discard_changes after ->store to fix this. - AddReturn's DataCorrupted error path (fired when MarkIssueReturned dies) still returned the pre-Bug-42418 4-tuple instead of including $checkin_record, and never attached its own messages to it. This crashed the REST controller's to_api($checkin) call on undef, masking the real error behind a generic 500 and leaving the audit row without any explanation of what happened. Add a 'data_corrupted' message case to _attach_messages_to_checkin for this. Test plan: 1. prove t/db_dependent/Circulation.t t/db_dependent/Koha/Checkin.t => SUCCESS: all pass. Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Passed QA --- Comment #106 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Rebased on main: regenerated the [DO NOT PUSH] DBIC schema commit (it had lost Borrower->patron_account_link from bug 39658) and adjusted test counts and contexts. No functional changes. Restoring PQA. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #208538|0 |1 is obsolete| | Attachment #208539|0 |1 is obsolete| | Attachment #208540|0 |1 is obsolete| | Attachment #208541|0 |1 is obsolete| | Attachment #208542|0 |1 is obsolete| | Attachment #208543|0 |1 is obsolete| | Attachment #208544|0 |1 is obsolete| | Attachment #208545|0 |1 is obsolete| | Attachment #208546|0 |1 is obsolete| | Attachment #208547|0 |1 is obsolete| | Attachment #208548|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #108 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 208995 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208995&action=edit Bug 42418: Add Koha::Schema::Result::Checkin DBIC Result class for the new checkins table. Includes relationships to items, borrowers, branches, desks, old_issues, branchtransfers, reserves, recalls, borrower_debarments, and return_claims. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #110 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 208997 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208997&action=edit Bug 42418: DBIC schema changes [DO NOT PUSH] Auto-generated has_many relationships added to related Result classes for the new checkins table. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #112 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 208999 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=208999&action=edit Bug 42418: (follow-up) Fix tests using invalid mock_userenv The checkins table has a FK on user_id pointing to borrowers. Several mock_userenv calls only passed branchcode, which defaults borrowernumber to the hardcoded value 51 — a non-existent patron. This causes FK violations when AddReturn creates a checkin record. This patch fixes the affected calls in Circulation.t and Branch.t by passing a valid patron to mock_userenv. It also adjusts the stock rotation transfer test to account for the new 5th return value from AddReturn (the Koha::Checkin object), and adds tests for the three checkin record user_id scenarios (valid patron, missing number, explicit undef number). A global fix to mock_userenv (auto-generating a patron when none is passed) would address the ~120 remaining calls across the test suite, but the risk surface warrants a separate bug. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #114 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209001 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209001&action=edit Bug 42418: (follow-up) Fix Patrons.t for checkin record creation AddReturn now creates a checkin record which requires a valid library_id. The anonymize_last_borrowers subtests call AddReturn without a userenv set, causing a NOT NULL constraint violation. This adds mock_userenv before the AddReturn calls. Test plan: 1. Apply patch 2. Run: $ ktd --shell k$ prove t/db_dependent/Koha/Patrons.t => SUCCESS: All 52 tests pass! 3. Sign off :-D Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #116 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209003 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209003&action=edit Bug 42418: (QA follow-up) Fix DataCorrupted return arity and checkin timestamp population Two fixes to how AddReturn manages the Koha::Checkin audit record it creates for every scan: - The checkin record's DB-computed defaults (e.g. timestamp, which defaults to current_timestamp()) were never refreshed from storage after the initial INSERT, so the in-memory object kept holding the literal SQL placeholder DBIx::Class leaves behind instead of the real value. Add ->discard_changes after ->store to fix this. - AddReturn's DataCorrupted error path (fired when MarkIssueReturned dies) still returned the pre-Bug-42418 4-tuple instead of including $checkin_record, and never attached its own messages to it. This crashed the REST controller's to_api($checkin) call on undef, masking the real error behind a generic 500 and leaving the audit row without any explanation of what happened. Add a 'data_corrupted' message case to _attach_messages_to_checkin for this. Test plan: 1. prove t/db_dependent/Circulation.t t/db_dependent/Koha/Checkin.t => SUCCESS: all pass. Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #118 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209005 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209005&action=edit Bug 42418: (follow-up) Fix more tests using a stale mock_userenv AddReturn now creates a Koha::Checkin record whose user_id is a FK to borrowers. Two more tests ran AddReturn with a userenv pointing to a patron that had already been rolled back: * t/db_dependent/Koha/Checkouts.t: 'automatic_checkin' runs after the file-level transaction rollback, so the librarian mocked in 'filter_by_todays_checkins' no longer exists * t/db_dependent/Patron/Borrower_PrevCheckout.t: 'Check previous checkouts with delay' inherits the patron mocked (and rolled back) by the 'serial' subtest Both raised a 'Broken FK constraint' exception. This patch mocks the userenv with a valid patron in those subtests. Test plan: 1. Without this patch, run: $ ktd --shell k$ prove t/db_dependent/Koha/Checkouts.t \ t/db_dependent/Patron/Borrower_PrevCheckout.t => FAIL: Broken FK constraint storing Koha::Checkin 2. Apply this patch 3. Repeat step 1 => SUCCESS: Tests pass! 4. Sign off :-D Assisted-by: Opus 5.5 (Anthropic) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Failed QA --- Comment #119 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Hi all, taken a look, found a few issues worth fixing before push: Finding 1: the backfill fails on a deleted library 1) Check out '39999000003154' to any patron at http://localhost:8081/cgi-bin/koha/circ/circulation.pl, then check it in at http://localhost:8081/cgi-bin/koha/circ/returns.pl 2) In the KTD shell: koha-mysql kohadev <<'EOF' DROP TABLE checkins; UPDATE old_issues SET branchcode='GONE', checkin_library='GONE'; EOF updatedatabase 3) The output shows 'ERROR: Broken FK constraint', and checkins is empty 4) Clean up: koha-mysql kohadev -e "UPDATE old_issues SET branchcode='CPL', checkin_library='CPL'" updatedatabase Finding 2: the backfill stores checkout data as checkin data. Normal checkins record both correctly, it's only the backfill. 1) Set 'RecordStaffUserOnCheckout' to 'Record' at http://localhost:8081/cgi-bin/koha/admin/preferences.pl?op=search&searchfield=RecordStaffUserOnCheckout 2) Create a staff patron at http://localhost:8081/cgi-bin/koha/members/memberentry.pl?op=add_form&categorycode=S - 'Card number' and 'Username' = 'staffb', 'Password' = 'Staffb1234' - Under 'More' > 'Set permissions', give them 'catalogue', 'circulate' and 'loggedinlibrary' 3) As 'koha', check out '39999000003154' to any patron 4) Log in as 'staffb', use 'Set library' to switch to 'Fairview', and check '39999000003154' in 5) In the KTD shell, check the record created by the checkin. It's correct: staffb's borrowernumber and 'FFL': koha-mysql kohadev -e "SELECT user_id, library_id FROM checkins WHERE checkout_id=(SELECT MAX(issue_id) FROM old_issues)" 6) Then rebuild the table using only the backfill (clearing checkin_library simulates a pre bug 33641 / 24.11 return): koha-mysql kohadev <<'EOF' DROP TABLE checkins; UPDATE old_issues SET checkin_library=NULL; EOF updatedatabase koha-mysql kohadev -e "SELECT user_id, library_id FROM checkins WHERE checkout_id=(SELECT MAX(issue_id) FROM old_issues)" 7) It shows user_id 51 ('koha', who checked it out) and 'CPL' (where it was checked out). user_id should be NULL, and the library shouldn't be guessed from the checkout. Finding 3: CI failure: In kohastructure.sql, checkins sits between cash_registers and categories. It needs to go between categories_branches and checkout_renewals, otherwise the DB compare job (misc4dev run_tests.pl --compare-with) fails: it diffs kohastructure.sql against an alphabetical dump of the upgraded DB. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #121 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209386 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209386&action=edit Bug 42418: (QA follow-up) Only backfill checkins with a known, existing checkin library The checkins backfill from old_issues had two problems, reported by Pedro on comment #119: 1. It failed with 'Broken FK constraint' when an old_issues row referenced a library that had been deleted, and no checkin was backfilled at all. 2. It stored checkout data as checkin data: - user_id was taken from old_issues.issuer_id, which is the staff member who did the checkout, not the checkin. old_issues doesn't record who processed the return. - library_id fell back to old_issues.branchcode (the checkout library) when checkin_library was empty, which is the case for returns made before bug 33641 (24.11). This patch: - Leaves user_id empty on backfilled checkins. - Only backfills returns with a checkin_library that still exists in branches. The rest are skipped and counted in the update output. library_id is NOT NULL, and ON DELETE CASCADE already removes the checkins of deleted libraries, so skipping them is consistent. - Takes the number of backfilled rows from $dbh->do, as $dbh->rows is not reliable on a database handle and the 'Backfilled N checkin records' message was never printed. Test plan: 1. Check out '39999000003154' to any patron and check it in 2. Reproduce the deleted library problem: $ ktd --shell k$ koha-mysql kohadev <<'SQL' DROP TABLE checkins; UPDATE old_issues SET branchcode='GONE', checkin_library='GONE'; SQL k$ updatedatabase => FAIL: 'ERROR: Broken FK constraint', and checkins is empty 3. Clean up: k$ koha-mysql kohadev -e "UPDATE old_issues SET branchcode='CPL', checkin_library='CPL'" k$ updatedatabase 4. Set RecordStaffUserOnCheckout to 'Record' 5. Create a staff patron 'staffb' (password 'Staffb1234') with the catalogue, circulate and loggedinlibrary permissions 6. As 'koha', check out '39999000003154' to any patron 7. Log in as 'staffb', use 'Set library' to switch to 'Fairview', and check '39999000003154' in 8. Rebuild the table using only the backfill, simulating a return made before bug 33641 (24.11): k$ koha-mysql kohadev <<'SQL' DROP TABLE checkins; UPDATE old_issues SET checkin_library=NULL; SQL k$ updatedatabase k$ koha-mysql kohadev -e "SELECT user_id, library_id FROM checkins WHERE checkout_id=(SELECT MAX(issue_id) FROM old_issues)" => FAIL: user_id is 51 ('koha', who checked it out) and library_id is 'CPL' (where it was checked out) 9. Apply this patch 10. Repeat 2 => SUCCESS: No error. The update reports the skipped old_issues 11. Repeat 3 and 8 => SUCCESS: No checkin is backfilled for that return, and the update reports the skipped old_issues 12. Set checkin_library back, then repeat the DROP TABLE, updatedatabase and SELECT from 8 (without the UPDATE): k$ koha-mysql kohadev -e "UPDATE old_issues SET checkin_library='FFL' WHERE checkin_library IS NULL" => SUCCESS: The backfilled checkin has user_id NULL and library_id 'FFL' 13. Sign off :-D Assisted-by: Opus 5.5 (Anthropic) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #123 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Uploading the full patch set again, on top of current main (4e554789838), so it applies cleanly with the QA follow-ups. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #124 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209396 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209396&action=edit Bug 42418: DB changes Adds the checkins table to track individual checkin events with references to the item, staff member, library, desk, checkout, transfer, hold, recall, restriction, and return claim involved. Includes backfill of historical checkin records from old_issues. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #126 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209398 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209398&action=edit Bug 42418: Add Koha::Checkin and Koha::Checkins classes Koha: :Object-based classes for the checkins table, with tests. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #128 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209400 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209400&action=edit Bug 42418: Wire Koha::Checkin into AddReturn Creates a Koha::Checkin record during AddReturn, capturing the checkin context (item, staff, library, desk, checkout, transfer, hold, recall, restriction, claim). The checkin_id is passed to the after_circ_action hook payload for plugin consumption. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #130 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209402 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209402&action=edit Bug 42418: (follow-up) Skip old_issues with no library on backfill Both checkin_library and branchcode are nullable in old_issues. The COALESCE can return NULL, which violates the NOT NULL constraint on checkins.library_id. Filter out those rows. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #132 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209404 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209404&action=edit Bug 42418: (QA follow-up) Keep AddReturn message-content assertions in sync with the new return arity Wiring Koha::Checkin into AddReturn changed its return list from 4 to 5 elements. Three is_deeply assertions on the full return list broke as a result and were weakened down to bare is($doreturn, 0, ...)/ single-key checks instead of being updated for the new arity - losing their ability to catch corruption of $messages content. Update them to slice to the first four elements instead (matching the pattern already used elsewhere in this file for the same reason), and add a Koha::Checkin isa_ok check on the 5th element where a checkin record is expected. Test plan: 1. prove t/db_dependent/Circulation.t => SUCCESS: 90 tests pass. Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #134 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209406 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209406&action=edit Bug 42418: (QA follow-up) Add full coverage for _attach_messages_to_checkin _attach_messages_to_checkin (moved back to C4::Circulation in the previous follow-up) had no coverage at all, direct or indirect - existing tests only ever assert on AddReturn's legacy $messages hashref, never on the Koha::Checkin object_messages this function actually produces. It's a large function (~185 lines, 27 recognised message keys), so cover it directly rather than trying to drive all 27 through real AddReturn scenarios, following the existing precedent in this file of calling C4::Circulation private subs directly in tests (_GetCircControlBranch, _FixOverduesOnReturn). Test plan: 1. prove t/db_dependent/Circulation.t => SUCCESS: 92 tests pass, all 27 message keys exercised. Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #136 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209408 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209408&action=edit Bug 42418: (QA follow-up) Only backfill checkins with a known, existing checkin library The checkins backfill from old_issues had two problems, reported by Pedro on comment #119: 1. It failed with 'Broken FK constraint' when an old_issues row referenced a library that had been deleted, and no checkin was backfilled at all. 2. It stored checkout data as checkin data: - user_id was taken from old_issues.issuer_id, which is the staff member who did the checkout, not the checkin. old_issues doesn't record who processed the return. - library_id fell back to old_issues.branchcode (the checkout library) when checkin_library was empty, which is the case for returns made before bug 33641 (24.11). This patch: - Leaves user_id empty on backfilled checkins. - Only backfills returns with a checkin_library that still exists in branches. The rest are skipped and counted in the update output. library_id is NOT NULL, and ON DELETE CASCADE already removes the checkins of deleted libraries, so skipping them is consistent. - Takes the number of backfilled rows from $dbh->do, as $dbh->rows is not reliable on a database handle and the 'Backfilled N checkin records' message was never printed. Test plan: 1. Check out '39999000003154' to any patron and check it in 2. Reproduce the deleted library problem: $ ktd --shell k$ koha-mysql kohadev <<'SQL' DROP TABLE checkins; UPDATE old_issues SET branchcode='GONE', checkin_library='GONE'; SQL k$ updatedatabase => FAIL: 'ERROR: Broken FK constraint', and checkins is empty 3. Clean up: k$ koha-mysql kohadev -e "UPDATE old_issues SET branchcode='CPL', checkin_library='CPL'" k$ updatedatabase 4. Set RecordStaffUserOnCheckout to 'Record' 5. Create a staff patron 'staffb' (password 'Staffb1234') with the catalogue, circulate and loggedinlibrary permissions 6. As 'koha', check out '39999000003154' to any patron 7. Log in as 'staffb', use 'Set library' to switch to 'Fairview', and check '39999000003154' in 8. Rebuild the table using only the backfill, simulating a return made before bug 33641 (24.11): k$ koha-mysql kohadev <<'SQL' DROP TABLE checkins; UPDATE old_issues SET checkin_library=NULL; SQL k$ updatedatabase k$ koha-mysql kohadev -e "SELECT user_id, library_id FROM checkins WHERE checkout_id=(SELECT MAX(issue_id) FROM old_issues)" => FAIL: user_id is 51 ('koha', who checked it out) and library_id is 'CPL' (where it was checked out) 9. Apply this patch 10. Repeat 2 => SUCCESS: No error. The update reports the skipped old_issues 11. Repeat 3 and 8 => SUCCESS: No checkin is backfilled for that return, and the update reports the skipped old_issues 12. Set checkin_library back, then repeat the DROP TABLE, updatedatabase and SELECT from 8 (without the UPDATE): k$ koha-mysql kohadev -e "UPDATE old_issues SET checkin_library='FFL' WHERE checkin_library IS NULL" => SUCCESS: The backfilled checkin has user_id NULL and library_id 'FFL' 13. Sign off :-D Assisted-by: Opus 5.5 (Anthropic) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #138 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Rebased on top of main again (2712fc37080), it had moved. Only the DBIC [DO NOT PUSH] patch needed regenerating. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #139 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209416 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209416&action=edit Bug 42418: DB changes Adds the checkins table to track individual checkin events with references to the item, staff member, library, desk, checkout, transfer, hold, recall, restriction, and return claim involved. Includes backfill of historical checkin records from old_issues. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #141 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209418 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209418&action=edit Bug 42418: Add Koha::Checkin and Koha::Checkins classes Koha: :Object-based classes for the checkins table, with tests. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #143 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209420 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209420&action=edit Bug 42418: Wire Koha::Checkin into AddReturn Creates a Koha::Checkin record during AddReturn, capturing the checkin context (item, staff, library, desk, checkout, transfer, hold, recall, restriction, claim). The checkin_id is passed to the after_circ_action hook payload for plugin consumption. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #145 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209422 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209422&action=edit Bug 42418: (follow-up) Skip old_issues with no library on backfill Both checkin_library and branchcode are nullable in old_issues. The COALESCE can return NULL, which violates the NOT NULL constraint on checkins.library_id. Filter out those rows. Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #147 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209424 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209424&action=edit Bug 42418: (QA follow-up) Keep AddReturn message-content assertions in sync with the new return arity Wiring Koha::Checkin into AddReturn changed its return list from 4 to 5 elements. Three is_deeply assertions on the full return list broke as a result and were weakened down to bare is($doreturn, 0, ...)/ single-key checks instead of being updated for the new arity - losing their ability to catch corruption of $messages content. Update them to slice to the first four elements instead (matching the pattern already used elsewhere in this file for the same reason), and add a Koha::Checkin isa_ok check on the 5th element where a checkin record is expected. Test plan: 1. prove t/db_dependent/Circulation.t => SUCCESS: 90 tests pass. Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #149 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209426 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209426&action=edit Bug 42418: (QA follow-up) Add full coverage for _attach_messages_to_checkin _attach_messages_to_checkin (moved back to C4::Circulation in the previous follow-up) had no coverage at all, direct or indirect - existing tests only ever assert on AddReturn's legacy $messages hashref, never on the Koha::Checkin object_messages this function actually produces. It's a large function (~185 lines, 27 recognised message keys), so cover it directly rather than trying to drive all 27 through real AddReturn scenarios, following the existing precedent in this file of calling C4::Circulation private subs directly in tests (_GetCircControlBranch, _FixOverduesOnReturn). Test plan: 1. prove t/db_dependent/Circulation.t => SUCCESS: 92 tests pass, all 27 message keys exercised. Signed-off-by: Martin Renvoize <martin.renvoize@openfifth.co.uk> Signed-off-by: Tomás Cohen Arazi <tomascohen@theke.io> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 --- Comment #151 from Tomás Cohen Arazi (tcohen) <tomascohen@gmail.com> --- Created attachment 209428 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=209428&action=edit Bug 42418: (QA follow-up) Only backfill checkins with a known, existing checkin library The checkins backfill from old_issues had two problems, reported by Pedro on comment #119: 1. It failed with 'Broken FK constraint' when an old_issues row referenced a library that had been deleted, and no checkin was backfilled at all. 2. It stored checkout data as checkin data: - user_id was taken from old_issues.issuer_id, which is the staff member who did the checkout, not the checkin. old_issues doesn't record who processed the return. - library_id fell back to old_issues.branchcode (the checkout library) when checkin_library was empty, which is the case for returns made before bug 33641 (24.11). This patch: - Leaves user_id empty on backfilled checkins. - Only backfills returns with a checkin_library that still exists in branches. The rest are skipped and counted in the update output. library_id is NOT NULL, and ON DELETE CASCADE already removes the checkins of deleted libraries, so skipping them is consistent. - Takes the number of backfilled rows from $dbh->do, as $dbh->rows is not reliable on a database handle and the 'Backfilled N checkin records' message was never printed. Test plan: 1. Check out '39999000003154' to any patron and check it in 2. Reproduce the deleted library problem: $ ktd --shell k$ koha-mysql kohadev <<'SQL' DROP TABLE checkins; UPDATE old_issues SET branchcode='GONE', checkin_library='GONE'; SQL k$ updatedatabase => FAIL: 'ERROR: Broken FK constraint', and checkins is empty 3. Clean up: k$ koha-mysql kohadev -e "UPDATE old_issues SET branchcode='CPL', checkin_library='CPL'" k$ updatedatabase 4. Set RecordStaffUserOnCheckout to 'Record' 5. Create a staff patron 'staffb' (password 'Staffb1234') with the catalogue, circulate and loggedinlibrary permissions 6. As 'koha', check out '39999000003154' to any patron 7. Log in as 'staffb', use 'Set library' to switch to 'Fairview', and check '39999000003154' in 8. Rebuild the table using only the backfill, simulating a return made before bug 33641 (24.11): k$ koha-mysql kohadev <<'SQL' DROP TABLE checkins; UPDATE old_issues SET checkin_library=NULL; SQL k$ updatedatabase k$ koha-mysql kohadev -e "SELECT user_id, library_id FROM checkins WHERE checkout_id=(SELECT MAX(issue_id) FROM old_issues)" => FAIL: user_id is 51 ('koha', who checked it out) and library_id is 'CPL' (where it was checked out) 9. Apply this patch 10. Repeat 2 => SUCCESS: No error. The update reports the skipped old_issues 11. Repeat 3 and 8 => SUCCESS: No checkin is backfilled for that return, and the update reports the skipped old_issues 12. Set checkin_library back, then repeat the DROP TABLE, updatedatabase and SELECT from 8 (without the UPDATE): k$ koha-mysql kohadev -e "UPDATE old_issues SET checkin_library='FFL' WHERE checkin_library IS NULL" => SUCCESS: The backfilled checkin has user_id NULL and library_id 'FFL' 13. Sign off :-D Assisted-by: Opus 5.5 (Anthropic) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to main Version(s)| |26.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=42418 --- Comment #154 from Pedro Amorim (ammopt) <pedro.amorim@openfifth.co.uk> --- Release notes suggestion: Adds a checkins table recording each checkin with who, where, and any related events (holds, recalls, etc.). Plugins get it in the after_circ_action payload as checkin. The upgrade backfills past returns from 24.11 onwards (bug 33641) whose library and item still exist, with no staff member, and reports how many were skipped. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=42418 Lucas Gass (lukeg) <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to main |Needs documenting CC| |lucas@bywatersolutions.com --- Comment #156 from Lucas Gass (lukeg) <lucas@bywatersolutions.com> --- No 26.05.x backport here. -- You are receiving this mail because: You are watching all bug changes.