Bug 15985 got lengthy and the patchset got over-complicated on the QA process and the guidelines changes in between. I propose a fresh restart here.
*** Bug 15985 has been marked as a duplicate of this bug. ***
Created attachment 109376 [details] [review] Bug 26327: Add ->library method to Koha::*Checkout This patch adds a 'library' relationship to the Issue and OldIssue resultsets, to be used in the Koha::*Checkout classes corresponding accessors. This way we will be able to eventually embed and filter on library fields on the API. The ->library method is then added to the classes- To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Old.t t/db_dependent/Koha/Checkout.t => SUCCESS: Tests pass! 3. Sign off :-D
Created attachment 109377 [details] [review] Bug 26327: Include transacting library in fines This patch introduces a new column on the fines table to display the checkout library when the fine is tied to a checkout. To test: 1. Have an overdue item that generated a fee 2. Look at the 'Accounting' tab, 'Make a payment' => FAIL: There's no information about the library 3. Apply this patch 4. Restart all 5. Reload and repeat 2 => SUCCESS: There's a column with the library name! 6. Identify the issue_id for the mentioned checkout and: $ koha-mysql kohadev > UPDATE old_issues SET branchcode='TOM' WHERE issue_id=11; (i.e. set the branchcode to an invalid value, to emulate a deleted branch). 7. Reload => SUCCESS: It doesn't explode, it doesn't display a library name. 8. Sign off :-D
Created attachment 109378 [details] [review] Bug 26327: Add to columns settings
Created attachment 109505 [details] [review] Bug 26327: Add ->library method to Koha::*Checkout This patch adds a 'library' relationship to the Issue and OldIssue resultsets, to be used in the Koha::*Checkout classes corresponding accessors. This way we will be able to eventually embed and filter on library fields on the API. The ->library method is then added to the classes- To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Old.t t/db_dependent/Koha/Checkout.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 109506 [details] [review] Bug 26327: Include transacting library in fines This patch introduces a new column on the fines table to display the checkout library when the fine is tied to a checkout. To test: 1. Have an overdue item that generated a fee 2. Look at the 'Accounting' tab, 'Make a payment' => FAIL: There's no information about the library 3. Apply this patch 4. Restart all 5. Reload and repeat 2 => SUCCESS: There's a column with the library name! 6. Identify the issue_id for the mentioned checkout and: $ koha-mysql kohadev > UPDATE old_issues SET branchcode='TOM' WHERE issue_id=11; (i.e. set the branchcode to an invalid value, to emulate a deleted branch). 7. Reload => SUCCESS: It doesn't explode, it doesn't display a library name. 8. Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 109507 [details] [review] Bug 26327: Add to columns settings Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Looks good! Wondering if we should include the column in boraccount.pl as well.
(In reply to Kyle M Hall from comment #8) > Looks good! Wondering if we should include the column in boraccount.pl as > well. That's a great idea, Kyle. I'm submitting a follow-up. Can you review it?
Created attachment 109531 [details] [review] Bug 26327: (follow-up) Add transacting library to the transactions tab Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
I have one doubt about this patches that add new columns to existing tables: should we make the new columns hidden by default? It is a trivial follow-up patch but I was wondering if we had a policy for this.
(In reply to Tomás Cohen Arazi from comment #11) > I have one doubt about this patches that add new columns to existing tables: > should we make the new columns hidden by default? It is a trivial follow-up > patch but I was wondering if we had a policy for this. I feel like it might be beneficial to show it - it's easy to hide and it hightlights a change. In the past, we didn't have the table configuration and new columns always showed up... so maybe it's not that much of a change. I can see why we would want to hide it too - maybe need more opinions, but don't see this as a blocker.
(In reply to Katrin Fischer from comment #12) > (In reply to Tomás Cohen Arazi from comment #11) > > I have one doubt about this patches that add new columns to existing tables: > > should we make the new columns hidden by default? It is a trivial follow-up > > patch but I was wondering if we had a policy for this. > > I feel like it might be beneficial to show it - it's easy to hide and it > hightlights a change. In the past, we didn't have the table configuration > and new columns always showed up... so maybe it's not that much of a change. > > I can see why we would want to hide it too - maybe need more opinions, but > don't see this as a blocker. Thanks!
There is a display issue in that the total due line (last line of the table) is now "too short".
Created attachment 110412 [details] [review] Bug 26327: (QA follow-up) Fix colspan in boraccount.tt Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Katrin Fischer from comment #14) > There is a display issue in that the total due line (last line of the table) > is now "too short". Thanks for pointing it out, and to Owen for clarifying on IRC. My brain is malfunctioning it seems.
(In reply to Tomás Cohen Arazi from comment #16) > (In reply to Katrin Fischer from comment #14) > > There is a display issue in that the total due line (last line of the table) > > is now "too short". > > Thanks for pointing it out, and to Owen for clarifying on IRC. My brain is > malfunctioning it seems. I was struggling a bit to describe it - but I think you can tell that I tried :)
Created attachment 110421 [details] [review] Bug 26327: Add ->library method to Koha::*Checkout This patch adds a 'library' relationship to the Issue and OldIssue resultsets, to be used in the Koha::*Checkout classes corresponding accessors. This way we will be able to eventually embed and filter on library fields on the API. The ->library method is then added to the classes- To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Old.t t/db_dependent/Koha/Checkout.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 110422 [details] [review] Bug 26327: Include transacting library in fines This patch introduces a new column on the fines table to display the checkout library when the fine is tied to a checkout. To test: 1. Have an overdue item that generated a fee 2. Look at the 'Accounting' tab, 'Make a payment' => FAIL: There's no information about the library 3. Apply this patch 4. Restart all 5. Reload and repeat 2 => SUCCESS: There's a column with the library name! 6. Identify the issue_id for the mentioned checkout and: $ koha-mysql kohadev > UPDATE old_issues SET branchcode='TOM' WHERE issue_id=11; (i.e. set the branchcode to an invalid value, to emulate a deleted branch). 7. Reload => SUCCESS: It doesn't explode, it doesn't display a library name. 8. Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 110423 [details] [review] Bug 26327: Add to columns settings Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 110424 [details] [review] Bug 26327: (follow-up) Add transacting library to the transactions tab Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 110425 [details] [review] Bug 26327: (QA follow-up) Fix colspan in boraccount.tt Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Can you rebase please? Applying: Bug 26327: (follow-up) Add transacting library to the transactions tab error: sha1 information is lacking or useless (admin/columns_settings.yml).
(In reply to Jonathan Druart from comment #23) > Can you rebase please? > > Applying: Bug 26327: (follow-up) Add transacting library to the transactions > tab > error: sha1 information is lacking or useless (admin/columns_settings.yml). What git version are you using? Mine gave me a really trivial merge conflict: $ git --version git version 2.28.0 $ git diff diff --cc admin/columns_settings.yml index 9400dcf3691,3f1d19ee79d..00000000000 --- a/admin/columns_settings.yml +++ b/admin/columns_settings.yml @@@ -666,7 -666,7 +666,11 @@@ modules - columnname: issuedate - ++<<<<<<< HEAD + columnname: home_library ++======= + columnname: checked_out_from ++>>>>>>> Bug 26327: (follow-up) Add transacting library to the transactions tab - columnname: note -
Created attachment 111417 [details] [review] Bug 26327: Add to columns settings Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 111420 [details] [review] Bug 26327: Add ->library method to Koha::*Checkout This patch adds a 'library' relationship to the Issue and OldIssue resultsets, to be used in the Koha::*Checkout classes corresponding accessors. This way we will be able to eventually embed and filter on library fields on the API. The ->library method is then added to the classes- To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Old.t t/db_dependent/Koha/Checkout.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 111421 [details] [review] Bug 26327: Include transacting library in fines This patch introduces a new column on the fines table to display the checkout library when the fine is tied to a checkout. To test: 1. Have an overdue item that generated a fee 2. Look at the 'Accounting' tab, 'Make a payment' => FAIL: There's no information about the library 3. Apply this patch 4. Restart all 5. Reload and repeat 2 => SUCCESS: There's a column with the library name! 6. Identify the issue_id for the mentioned checkout and: $ koha-mysql kohadev > UPDATE old_issues SET branchcode='TOM' WHERE issue_id=11; (i.e. set the branchcode to an invalid value, to emulate a deleted branch). 7. Reload => SUCCESS: It doesn't explode, it doesn't display a library name. 8. Sign off :-D Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 111422 [details] [review] Bug 26327: Add to columns settings Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 111423 [details] [review] Bug 26327: (follow-up) Add transacting library to the transactions tab Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 111424 [details] [review] Bug 26327: (QA follow-up) Fix colspan in boraccount.tt Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Hm, either the bug title or the bug description are wrong - it's either transacting library or checkout library, but both are not the same necessarily?
(In reply to Katrin Fischer from comment #31) > Hm, either the bug title or the bug description are wrong - it's either > transacting library or checkout library, but both are not the same > necessarily? The description links transacting+checkouts, that's why I thought it was correct. Feel free to adjust it as I'm not attached to that description.
I am not the patch author :) My thought: the checkout library is the library an item was checked out from. A transacting library for a fine would be the one where it was created manually or if item related it might depend on which library was used for creating the fine (thinking about CircControl) or for payments where it was paid at.
Thx!
Created attachment 111586 [details] [review] Bug 26327: Add exec flag on .t file
Pushed to master for 20.11, thanks to everybody involved!
enhancement will not be backported to 20.05.x