Bug 26327

Summary: Include checkout library in fines
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Fines and feesAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Katrin Fischer <katrin.fischer>
Severity: enhancement    
Priority: P5 - low CC: 1joynelson, jonathan.druart, jrobb, katrin.fischer, kyle, lucas, nengard, tomascohen
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This patch adds a new column in the accounting tables for a patron. The column will show the checkout library for charges that are tied to circulation.
Version(s) released in:
20.11.00
Bug Depends on:    
Bug Blocks: 14825    
Attachments: Bug 26327: Add ->library method to Koha::*Checkout
Bug 26327: Include transacting library in fines
Bug 26327: Add to columns settings
Bug 26327: Add ->library method to Koha::*Checkout
Bug 26327: Include transacting library in fines
Bug 26327: Add to columns settings
Bug 26327: (follow-up) Add transacting library to the transactions tab
Bug 26327: (QA follow-up) Fix colspan in boraccount.tt
Bug 26327: Add ->library method to Koha::*Checkout
Bug 26327: Include transacting library in fines
Bug 26327: Add to columns settings
Bug 26327: (follow-up) Add transacting library to the transactions tab
Bug 26327: (QA follow-up) Fix colspan in boraccount.tt
Bug 26327: Add to columns settings
Bug 26327: Add ->library method to Koha::*Checkout
Bug 26327: Include transacting library in fines
Bug 26327: Add to columns settings
Bug 26327: (follow-up) Add transacting library to the transactions tab
Bug 26327: (QA follow-up) Fix colspan in boraccount.tt
Bug 26327: Add exec flag on .t file

Description Tomás Cohen Arazi 2020-08-31 18:29:09 UTC
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.
Comment 1 Tomás Cohen Arazi 2020-08-31 19:22:09 UTC
*** Bug 15985 has been marked as a duplicate of this bug. ***
Comment 2 Tomás Cohen Arazi 2020-08-31 20:16:53 UTC
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
Comment 3 Tomás Cohen Arazi 2020-08-31 20:16:58 UTC
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
Comment 4 Tomás Cohen Arazi 2020-08-31 20:17:02 UTC
Created attachment 109378 [details] [review]
Bug 26327: Add to columns settings
Comment 5 Kyle M Hall 2020-09-02 11:17:59 UTC
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>
Comment 6 Kyle M Hall 2020-09-02 11:18:43 UTC
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>
Comment 7 Kyle M Hall 2020-09-02 11:18:46 UTC
Created attachment 109507 [details] [review]
Bug 26327: Add to columns settings

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2020-09-02 11:19:29 UTC
Looks good! Wondering if we should include the column in boraccount.pl as well.
Comment 9 Tomás Cohen Arazi 2020-09-02 13:07:32 UTC
(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?
Comment 10 Tomás Cohen Arazi 2020-09-02 13:08:07 UTC
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>
Comment 11 Tomás Cohen Arazi 2020-09-02 13:19:01 UTC
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.
Comment 12 Katrin Fischer 2020-09-02 14:53:27 UTC
(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.
Comment 13 Tomás Cohen Arazi 2020-09-02 14:54:34 UTC
(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!
Comment 14 Katrin Fischer 2020-09-12 21:11:36 UTC
There is a display issue in that the total due line (last line of the table) is now "too short".
Comment 15 Tomás Cohen Arazi 2020-09-18 19:40:20 UTC
Created attachment 110412 [details] [review]
Bug 26327: (QA follow-up) Fix colspan in boraccount.tt

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Tomás Cohen Arazi 2020-09-18 19:41:24 UTC
(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.
Comment 17 Katrin Fischer 2020-09-18 20:18:09 UTC
(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 :)
Comment 18 Katrin Fischer 2020-09-20 17:12:40 UTC Comment hidden (obsolete)
Comment 19 Katrin Fischer 2020-09-20 17:12:45 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2020-09-20 17:12:51 UTC Comment hidden (obsolete)
Comment 21 Katrin Fischer 2020-09-20 17:13:02 UTC Comment hidden (obsolete)
Comment 22 Katrin Fischer 2020-09-20 17:13:07 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2020-10-09 10:22:52 UTC
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).
Comment 24 Tomás Cohen Arazi 2020-10-09 17:19:37 UTC
(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
              -
Comment 25 Tomás Cohen Arazi 2020-10-09 17:21:21 UTC Comment hidden (obsolete)
Comment 26 Tomás Cohen Arazi 2020-10-09 17:25:57 UTC
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>
Comment 27 Tomás Cohen Arazi 2020-10-09 17:26:03 UTC
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>
Comment 28 Tomás Cohen Arazi 2020-10-09 17:26:08 UTC
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>
Comment 29 Tomás Cohen Arazi 2020-10-09 17:26:14 UTC
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>
Comment 30 Tomás Cohen Arazi 2020-10-09 17:26:19 UTC
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>
Comment 31 Katrin Fischer 2020-10-09 19:45:54 UTC
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?
Comment 32 Tomás Cohen Arazi 2020-10-09 19:48:42 UTC
(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.
Comment 33 Katrin Fischer 2020-10-09 20:05:16 UTC
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.
Comment 34 Katrin Fischer 2020-10-09 20:28:11 UTC
Thx!
Comment 35 Jonathan Druart 2020-10-14 09:50:16 UTC
Created attachment 111586 [details] [review]
Bug 26327: Add exec flag on .t file
Comment 36 Jonathan Druart 2020-10-14 12:54:32 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 37 Lucas Gass 2020-11-13 14:43:11 UTC
enhancement will not be backported to 20.05.x