Bug 33641

Summary: We should record return library in old checkouts (oldissues)
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: Architecture, internals, and plumbingAssignee: Nick Clemens (kidclamp) <nick>
Status: Needs documenting --- QA Contact: Martin Renvoize (ashimema) <martin.renvoize>
Severity: enhancement    
Priority: P5 - low CC: andrew, bugzilla, kebliss, lucas, martin.renvoize, rcoert
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18994
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00
Circulation function:
Bug Depends on: 33010    
Bug Blocks:    
Attachments: Bug 33641: DB Update
Bug 33641: DB update
Bug 33641: DO NOT PUSH - Schema Updates
Bug 33641: DB update
Bug 33641: DO NOT PUSH - Schema Updates
Bug 33641: Code changes
Bug 33641: Unit Tests
Bug 33641: DB update
Bug 33641: DO NOT PUSH - Schema Updates
Bug 33641: Code changes
Bug 33641: Unit Tests
Bug 33641: DB update
Bug 33641: DO NOT PUSH - Schema Updates
Bug 33641: Code changes
Bug 33641: Unit Tests
Bug 33641: DB update
Bug 33641: Code changes
Bug 33641: Unit Tests
Bug 33641: (follow-up) Change return_branch to checkin_library
Bug 33641: DO NOT PUSH - Schema Updates
Bug 33641: (follow-up) Add field to api spec
Bug 33641: (follow-up) Add field to api spec
Bug 33641: Fix unit tests
Bug 33641: Fix StoreLastBorrower.t
Bug 33641: Fix unit tests
Bug 33641: (Follow-up) Fix unit tests
Bug 33641: (Follow-up) Fix unit tests
Bug 33641: (Follow-up) Fix unit tests
Bug 33641: (Follow-up) Fix unit tests
Bug 33641: (Follow-up) Fix Indexer.t

Description Nick Clemens (kidclamp) 2023-04-29 21:05:18 UTC
See bug 33010 - the intention is to list the checkouts returned at current branch, but we only store this info in statistics. It would be handy to keep this info in the issue/oldissues tables
Comment 1 Andrew Fuerste-Henry 2023-10-02 19:14:47 UTC
+1, This would be helpful data and is not particularly accessible with the current structure. I'm currently trying to write a query to determine how often our items are returned somewhere other than the branch at which they were checked out and the manner in which we record things is making is very difficult.
Comment 2 Eric Garcia 2024-08-06 17:11:34 UTC
Created attachment 170105 [details] [review]
Bug 33641: DB Update

Added return_branch to issues table and old_issues table
Comment 3 Nick Clemens (kidclamp) 2024-08-06 17:38:47 UTC
Created attachment 170106 [details] [review]
Bug 33641: DB update
Comment 4 Nick Clemens (kidclamp) 2024-08-06 17:38:49 UTC
Created attachment 170107 [details] [review]
Bug 33641: DO NOT PUSH - Schema Updates
Comment 5 Eric Garcia 2024-08-06 18:00:30 UTC
Created attachment 170108 [details] [review]
Bug 33641: DB update
Comment 6 Eric Garcia 2024-08-06 18:00:32 UTC
Created attachment 170109 [details] [review]
Bug 33641: DO NOT PUSH - Schema Updates
Comment 7 Eric Garcia 2024-08-06 19:15:09 UTC
Created attachment 170110 [details] [review]
Bug 33641: Code changes

To test:

1. Apply patch, restart_all, updatedatabase
2. Check out an item to a patron
3. Query the database
    - select return_branch from issues;     should return NULL
    - select branchcode from issues;        should return the branchcode of the library you checkout the item out from
    - select return_branch from old_issues; should match the branchcode from the previous query
Comment 8 Eric Garcia 2024-08-06 19:15:11 UTC
Created attachment 170111 [details] [review]
Bug 33641: Unit Tests
Comment 9 ByWater Sandboxes 2024-08-07 14:12:58 UTC
Created attachment 170138 [details] [review]
Bug 33641: DB update

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 10 ByWater Sandboxes 2024-08-07 14:12:59 UTC
Created attachment 170139 [details] [review]
Bug 33641: DO NOT PUSH - Schema Updates

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 11 ByWater Sandboxes 2024-08-07 14:13:01 UTC
Created attachment 170140 [details] [review]
Bug 33641: Code changes

To test:

1. Apply patch, restart_all, updatedatabase
2. Check out an item to a patron
3. Query the database
    - select return_branch from issues;     should return NULL
    - select branchcode from issues;        should return the branchcode of the library you checkout the item out from
    - select return_branch from old_issues; should match the branchcode from the previous query

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 12 ByWater Sandboxes 2024-08-07 14:13:03 UTC
Created attachment 170141 [details] [review]
Bug 33641: Unit Tests

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 13 Martin Renvoize (ashimema) 2024-10-28 15:23:16 UTC
I'm not sure we really need the 'return_branch' in the issues table at all.. it will always be empty.. could we not just add it to the old_issues table only?
Comment 14 Martin Renvoize (ashimema) 2024-10-28 15:35:42 UTC
Created attachment 173562 [details] [review]
Bug 33641: DB update

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 15 Martin Renvoize (ashimema) 2024-10-28 15:35:45 UTC
Created attachment 173563 [details] [review]
Bug 33641: DO NOT PUSH - Schema Updates

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize (ashimema) 2024-10-28 15:35:47 UTC
Created attachment 173564 [details] [review]
Bug 33641: Code changes

To test:

1. Apply patch, restart_all, updatedatabase
2. Check out an item to a patron
3. Query the database
    - select return_branch from issues;     should return NULL
    - select branchcode from issues;        should return the branchcode of the library you checkout the item out from
    - select return_branch from old_issues; should match the branchcode from the previous query

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize (ashimema) 2024-10-28 15:35:49 UTC
Created attachment 173565 [details] [review]
Bug 33641: Unit Tests

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 18 Martin Renvoize (ashimema) 2024-10-28 15:36:27 UTC
OK.. It makes UNION queries a tad simpler..

No regressions found, nice enhancement.

Passing QA
Comment 19 Katrin Fischer 2024-10-30 13:08:47 UTC
This is a rather small patch, any chance we could turn return_branch into checkin_library?
Comment 20 Katrin Fischer 2024-11-11 16:59:36 UTC
(In reply to Katrin Fischer from comment #19)
> This is a rather small patch, any chance we could turn return_branch into
> checkin_library?

Hm, I didn't mean to block this with my comment. Would anyone be willing to adopt?
Comment 21 Nick Clemens (kidclamp) 2024-11-14 12:44:27 UTC
Created attachment 174510 [details] [review]
Bug 33641: DB update

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Nick Clemens (kidclamp) 2024-11-14 12:44:29 UTC
Created attachment 174511 [details] [review]
Bug 33641: Code changes

To test:

1. Apply patch, restart_all, updatedatabase
2. Check out an item to a patron
3. Query the database
    - select return_branch from issues;     should return NULL
    - select branchcode from issues;        should return the branchcode of the library you checkout the item out from
    - select return_branch from old_issues; should match the branchcode from the previous query

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 23 Nick Clemens (kidclamp) 2024-11-14 12:44:32 UTC
Created attachment 174512 [details] [review]
Bug 33641: Unit Tests

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 24 Nick Clemens (kidclamp) 2024-11-14 12:44:34 UTC
Created attachment 174513 [details] [review]
Bug 33641: (follow-up) Change return_branch to checkin_library
Comment 25 Nick Clemens (kidclamp) 2024-11-14 12:44:36 UTC
Created attachment 174514 [details] [review]
Bug 33641: DO NOT PUSH - Schema Updates
Comment 26 Katrin Fischer 2024-11-14 12:59:48 UTC
Hm, this needs some more work:

            ALTER TABLE old_issues
            ADD COLUMN checkin_library varchar(10) DEFAULT NULL
            COMMENT 'foreign key, linking to the branches table for the location the item was returned'
            AFTER returndate

There is no FK or link, it's only adding the column. I have adapted the comments accordingly.
Comment 27 Katrin Fischer 2024-11-14 14:32:56 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 28 Katrin Fischer 2024-11-14 15:53:57 UTC
I think we broke tons of tests here. Please check on Jenkins, especially api related ones.
Comment 29 Nick Clemens (kidclamp) 2024-11-14 20:44:16 UTC
Created attachment 174536 [details] [review]
Bug 33641: (follow-up) Add field to api spec
Comment 30 Nick Clemens (kidclamp) 2024-11-14 20:44:50 UTC
(In reply to Nick Clemens (kidclamp) from comment #29)
> Created attachment 174536 [details] [review] [review]
> Bug 33641: (follow-up) Add field to api spec

Asking for Tomas' input, this fixes tests, but I am sure is incomplete
Comment 31 Tomás Cohen Arazi (tcohen) 2024-11-14 21:28:12 UTC
Created attachment 174538 [details] [review]
Bug 33641: (follow-up) Add field to api spec

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Edit: I changed the attribute name to match our guidelines (tcohen)
Comment 32 Katrin Fischer 2024-11-15 08:11:14 UTC
Follow-up patch "Add field to api spec" picked for main.
Comment 33 Matt Blenkinsop 2024-11-15 11:19:45 UTC
Created attachment 174569 [details] [review]
Bug 33641: Fix unit tests
Comment 34 Matt Blenkinsop 2024-11-15 11:20:29 UTC
Created attachment 174570 [details] [review]
Bug 33641: Fix StoreLastBorrower.t
Comment 35 Matt Blenkinsop 2024-11-15 11:27:09 UTC
Created attachment 174571 [details] [review]
Bug 33641: Fix unit tests
Comment 36 Matt Blenkinsop 2024-11-15 11:27:50 UTC
Created attachment 174572 [details] [review]
Bug 33641: (Follow-up) Fix unit tests
Comment 37 Matt Blenkinsop 2024-11-15 11:31:15 UTC
Created attachment 174573 [details] [review]
Bug 33641: (Follow-up) Fix unit tests
Comment 38 Matt Blenkinsop 2024-11-15 11:34:16 UTC
Created attachment 174574 [details] [review]
Bug 33641: (Follow-up) Fix unit tests
Comment 39 Katrin Fischer 2024-11-15 13:15:22 UTC
We still need fixes for indexer.t that should also mock userenv.
Comment 40 Matt Blenkinsop 2024-11-15 14:44:47 UTC
Created attachment 174584 [details] [review]
Bug 33641: (Follow-up) Fix unit tests
Comment 41 Katrin Fischer 2024-11-15 14:47:38 UTC
(In reply to Matt Blenkinsop from comment #40)
> Created attachment 174584 [details] [review] [review]
> Bug 33641: (Follow-up) Fix unit tests

Sorry, I forgot to note here that I had already pushed "Fix unit tests". Can you please provide the additional change separately?
Comment 42 Matt Blenkinsop 2024-11-15 14:48:03 UTC
Created attachment 174585 [details] [review]
Bug 33641: (Follow-up) Fix Indexer.t
Comment 43 Katrin Fischer 2024-11-15 14:50:22 UTC
(In reply to Matt Blenkinsop from comment #42)
> Created attachment 174585 [details] [review] [review]
> Bug 33641: (Follow-up) Fix Indexer.t

Pushed!
Comment 44 Lucas Gass (lukeg) 2024-12-05 21:22:29 UTC
Enhancement, no 24.05.x backport