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
+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.
Created attachment 170105 [details] [review] Bug 33641: DB Update Added return_branch to issues table and old_issues table
Created attachment 170106 [details] [review] Bug 33641: DB update
Created attachment 170107 [details] [review] Bug 33641: DO NOT PUSH - Schema Updates
Created attachment 170108 [details] [review] Bug 33641: DB update
Created attachment 170109 [details] [review] Bug 33641: DO NOT PUSH - Schema Updates
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
Created attachment 170111 [details] [review] Bug 33641: Unit Tests
Created attachment 170138 [details] [review] Bug 33641: DB update Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Created attachment 170139 [details] [review] Bug 33641: DO NOT PUSH - Schema Updates Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
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>
Created attachment 170141 [details] [review] Bug 33641: Unit Tests Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
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?
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>
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>
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>
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>
OK.. It makes UNION queries a tad simpler.. No regressions found, nice enhancement. Passing QA
This is a rather small patch, any chance we could turn return_branch into checkin_library?
(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?
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>
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>
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>
Created attachment 174513 [details] [review] Bug 33641: (follow-up) Change return_branch to checkin_library
Created attachment 174514 [details] [review] Bug 33641: DO NOT PUSH - Schema Updates
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.
Pushed for 24.11! Well done everyone, thank you!
I think we broke tons of tests here. Please check on Jenkins, especially api related ones.
Created attachment 174536 [details] [review] Bug 33641: (follow-up) Add field to api spec
(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
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)
Follow-up patch "Add field to api spec" picked for main.
Created attachment 174569 [details] [review] Bug 33641: Fix unit tests
Created attachment 174570 [details] [review] Bug 33641: Fix StoreLastBorrower.t
Created attachment 174571 [details] [review] Bug 33641: Fix unit tests
Created attachment 174572 [details] [review] Bug 33641: (Follow-up) Fix unit tests
Created attachment 174573 [details] [review] Bug 33641: (Follow-up) Fix unit tests
Created attachment 174574 [details] [review] Bug 33641: (Follow-up) Fix unit tests
We still need fixes for indexer.t that should also mock userenv.
Created attachment 174584 [details] [review] Bug 33641: (Follow-up) Fix unit tests
(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?
Created attachment 174585 [details] [review] Bug 33641: (Follow-up) Fix Indexer.t
(In reply to Matt Blenkinsop from comment #42) > Created attachment 174585 [details] [review] [review] > Bug 33641: (Follow-up) Fix Indexer.t Pushed!