Bug 15985

Summary: Include checkout library in fines
Product: Koha Reporter: Nicole C. Engard <nengard>
Component: CirculationAssignee: Kyle M Hall <kyle>
Status: RESOLVED DUPLICATE QA Contact: Tomás Cohen Arazi <tomascohen>
Severity: enhancement    
Priority: P5 - low CC: chris, gmcharlt, jonathan.druart, josef.moravec, katrin.fischer, kyle.m.hall, kyle, lisettepalouse+koha, martin.renvoize, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22683
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19066
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 19489    
Bug Blocks: 14825, 23066    
Attachments: Bug 15985 - Include transacting library in fines
[SIGNED-OFF] Attachment to Bug 15985 - Include transacting library in fines
Bug 15985 - Include transacting library in fines
Bug 15985 - Include transacting library in fines
Bug 15985: Add new method Koha::Account::Line::checkout
Bug 15985: Include transacting library in fines
Bug 15985: Add new method Koha::Account::Line::checkout
Bug 15985: Add foreign key for issues tables
Bug 15985: (QA Follow up) Fix issues found by koha-qa.pl
Bug 15985: Include transacting library in fines
Bug 15985: Add new method Koha::Account::Line::checkout
Bug 15985: (QA Follow up) Fix issues found by koha-qa.pl
Bug 15985: (QA follow-up) Revert to Koha::Object bases
Bug 15985: Include transacting library in fines
Bug 15985: Add new method Koha::Account::Line::checkout
Bug 15985: (QA Follow up) Fix issues found by koha-qa.pl
Bug 15985: (QA follow-up) Revert to Koha::Object bases
Bug 15985: Add more tests
Bug 15985: Include transacting library in fines
Bug 15985: Add new method Koha::Account::Line::checkout
Bug 15985: (QA Follow up) Fix issues found by koha-qa.pl
Bug 15985: (QA follow-up) Revert to Koha::Object bases
Bug 15985: Add more tests
Bug 15985: Move dbic relationship to correct part of file
Bug 15985: Move new column to the right of checkout date
Bug 15985: Add to columns configuration
Bug 15985: Fix QA script issues
Bug 15985: Remove Koha::Old::Checkout::claim_returned

Description Nicole C. Engard 2016-03-04 19:53:45 UTC Comment hidden (obsolete)
Comment 1 Kyle M Hall 2016-09-06 14:59:38 UTC
Steps 1 through 3 are completed, only step 4 remains!
Comment 2 Kyle M Hall 2016-12-13 13:25:18 UTC
Created attachment 58157 [details] [review]
Bug 15985 - Include transacting library in fines

Some librarians would like to see the library an item was checked out
from in the fines table on the fines payment page.

Test Plan:
1) Apply this patch
2) Find a patron with overdue items that have fines
3) Browse to the pay fines page
4) Note the new "checked out from" column
Comment 3 Jason Robb 2016-12-13 18:05:14 UTC
Created attachment 58163 [details] [review]
[SIGNED-OFF] Attachment to Bug 15985 - Include transacting library in fines

Bug 15985 - Include transacting library in fines

Some librarians would like to see the library an item was checked out
from in the fines table on the fines payment page.

Test Plan:
1) Apply this patch
2) Find a patron with overdue items that have fines
3) Browse to the pay fines page
4) Note the new "checked out from" column

Signed-off-by: Jason Robb <jrobb@sekls.org>
Comment 4 Jonathan Druart 2016-12-14 11:06:20 UTC
I don't understand why this is signed off while the dependencies do not apply.
Comment 5 Josef Moravec 2018-01-02 10:28:08 UTC
Please rebase.
Comment 6 Kyle M Hall 2018-02-02 19:31:07 UTC
Created attachment 71165 [details] [review]
Bug 15985 - Include transacting library in fines

Some librarians would like to see the library an item was checked out
from in the fines table on the fines payment page.

Test Plan:
1) Apply this patch
2) Find a patron with overdue items that have fines
3) Browse to the pay fines page
4) Note the new "checked out from" column

Signed-off-by: Jason Robb <jrobb@sekls.org>
Comment 7 Josef Moravec 2018-02-05 08:38:16 UTC
Sorry Kyle, but not ready to pass QA yet:

first, thing reported by qa tools:
 FAIL	Koha/Checkout.pm
  FAIL	  pod
		 in file Koha/Checkout.pm
		Unknown command 'head'
		*** ERROR: 
   FAIL	  pod coverage
		POD is missing for 'library'


 FAIL	members/pay.pl
   FAIL	  valid
		members/pay.pl had compilation errors.
		Global symbol "$accountline" requires explicit package name 

Then:
When the item with fine is checked out, I can see the name of library, but when I check it in, it disappear. Shouldn't it be still visible?

In pay.pl you are doing this:
$account_line->{checkout}     = Koha::Checkouts->find($accountline->{issue_id});
Maybe be better could be to add new sub to Koha::Account::Line object. You can take care of issue/old_issue table relation.
Comment 8 Kyle M Hall 2018-03-09 15:44:02 UTC
Created attachment 72611 [details] [review]
Bug 15985 - Include transacting library in fines

Some librarians would like to see the library an item was checked out
from in the fines table on the fines payment page.

Test Plan:
1) Apply this patch
2) Find a patron with overdue items that have fines
3) Browse to the pay fines page
4) Note the new "checked out from" column

Signed-off-by: Jason Robb <jrobb@sekls.org>
Comment 9 Kyle M Hall 2018-03-09 15:44:10 UTC
Created attachment 72612 [details] [review]
Bug 15985: Add new method Koha::Account::Line::checkout
Comment 10 Josef Moravec 2018-04-04 12:34:46 UTC
Kyle, I still found some issues with this:

1. reported by QA tools, not sure how to fix this..., :
 FAIL	Koha/Old/Checkout.pm
   FAIL	  valid
			merging failed on 'Koha::Checkout' 
			]
				Koha::Old::Checkout,
			current merge results [
		Inconsistent hierarchy during C3 merge of class 'Koha::Old::Checkout':


2. This still remains:
 FAIL	Koha/Checkout.pm
  FAIL	  pod
		 in file Koha/Checkout.pm
		Unknown command 'head'
		*** ERROR: 
   FAIL	  pod coverage
		POD is missing for 'library'

you should use =head3 I think

3. The code added to Koha/Schema/Result/OldIssue.pm should be after schema loader mark

4. Also, when you make Koha::Checkout base for Koha::Old::Checkout, you should also define relations 'borrower' and 'item' in Koha::Schema::OldIssue
Comment 11 Josef Moravec 2019-03-11 20:46:35 UTC
This should be rebased and adapted on top of bug 19489
Comment 12 Martin Renvoize 2019-05-03 15:54:47 UTC
+1
Comment 13 Kyle M Hall 2019-06-04 12:13:10 UTC
Created attachment 90317 [details] [review]
Bug 15985: Include transacting library in fines

Some librarians would like to see the library an item was checked out
from in the fines table on the fines payment page.

Test Plan:
1) Apply this patch
2) Find a patron with overdue items that have fines
3) Browse to the pay fines page
4) Note the new "checked out from" column

Signed-off-by: Jason Robb <jrobb@sekls.org>
Comment 14 Kyle M Hall 2019-06-04 12:13:27 UTC
Created attachment 90318 [details] [review]
Bug 15985: Add new method Koha::Account::Line::checkout
Comment 15 Martin Renvoize 2019-06-05 16:27:02 UTC
Might this be a good opportunity to introduce a proper foreign key constraint at the DB level for issues.branchcode and old_issues.branchcode?
Comment 16 Kyle M Hall 2019-06-06 11:49:58 UTC
(In reply to Martin Renvoize from comment #15)
> Might this be a good opportunity to introduce a proper foreign key
> constraint at the DB level for issues.branchcode and old_issues.branchcode?

Sounds like a capital idea!
Comment 17 Martin Renvoize 2019-06-06 11:51:57 UTC
I'm resurrecting bug 22421 for the FK piece at the minute.. may also be nice to have bug 22683 feed into this too.
Comment 18 Kyle M Hall 2019-06-06 12:42:53 UTC
Created attachment 90364 [details] [review]
Bug 15985: Add foreign key for issues tables
Comment 19 Kyle M Hall 2019-06-06 13:31:38 UTC
Comment on attachment 90364 [details] [review]
Bug 15985: Add foreign key for issues tables

We should move this patch to its' own bug.
Comment 20 Katrin Fischer 2019-06-09 21:30:57 UTC
Hi Kyle, 

the unit tests pass, but the QA script has some complaints:

 FAIL	Koha/Checkout.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   FAIL	  pod
		*** ERROR: 
		Unknown command 'head'
		 
		 in file Koha/Checkout.pm
		
   FAIL	  pod coverage
		POD is missing for 'library'
   OK	  spelling
   OK	  valid

 FAIL	Koha/Old/Checkout.pm
   OK	  critic
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  pod
   OK	  pod coverage
   OK	  spelling
   FAIL	  valid
		Inconsistent hierarchy during C3 merge of class 'Koha::Old::Checkout':
			current merge results [
				Koha::Old::Checkout,
			]
			merging failed on 'Koha::Checkout' 


 FAIL	koha-tmpl/intranet-tmpl/prog/en/modules/members/pay.tt
   FAIL	  filters
		missing_filter at line 88 (        [% line.checkout.library.branchname %])
   OK	  forbidden patterns
   OK	  git manipulation
   OK	  js_in_body
   OK	  spelling
   OK	  tt_valid
   OK	  valid_template

Can you please check? Also a question for testing: are overdues the only fines where the library will show?
Comment 21 Kyle M Hall 2019-06-11 11:05:05 UTC
Created attachment 90478 [details] [review]
Bug 15985: (QA Follow up) Fix issues found by koha-qa.pl
Comment 22 Kyle M Hall 2019-06-11 11:08:41 UTC
I'm unable to resolve the qa failure

   FAIL	  valid
		Inconsistent hierarchy during C3 merge of class 'Koha::Old::Checkout':
			current merge results [
				Koha::Old::Checkout,
			]
			merging failed on 'Koha::Checkout'


without removing the 'use Modern::Perl', which of course causes a different qa failure. I believe we can safely ignore this one. The actual dependency tree is clear and simple.
Comment 23 Jonathan Druart 2019-06-20 02:52:20 UTC
1. Changes are made in Koha/Schema/Result/OldIssue.pm, above the "DO NOT MODIFY THIS OR ANYTHING ABOVE!" line.
Is it expected? Did you forget to attached the DB changes patch?
If you wanted to add it after that line, why not adding the constraint at DB level?

Why not for Issue.pm?

2.
-use base qw(Koha::Object);
+use base qw(Koha::Checkout);

Is this really needed? Why?
If so I think it must be moved to its own bug report, explaining why we need it and what it will bring. Also tests must be provided for such changes.

Note that both t/db_dependent/Koha/Account/Lines.t and t/db_dependent/Koha/Checkouts.t are passing without that change.
Comment 24 Martin Renvoize 2020-01-10 10:37:34 UTC
Created attachment 97177 [details] [review]
Bug 15985: Include transacting library in fines

Some librarians would like to see the library an item was checked out
from in the fines table on the fines payment page.

Test Plan:
1) Apply this patch
2) Find a patron with overdue items that have fines
3) Browse to the pay fines page
4) Note the new "checked out from" column

Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Martin Renvoize 2020-01-10 10:37:38 UTC
Created attachment 97178 [details] [review]
Bug 15985: Add new method Koha::Account::Line::checkout
Comment 26 Martin Renvoize 2020-01-10 10:37:41 UTC
Created attachment 97179 [details] [review]
Bug 15985: (QA Follow up) Fix issues found by koha-qa.pl

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2020-01-10 10:37:44 UTC
Created attachment 97180 [details] [review]
Bug 15985: (QA follow-up) Revert to Koha::Object bases

Being pragmatic to try and move this bug on, I've reverted the changed
base class for the Old:: classes and cloned the required code from the
corresponding Object classes.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 28 Martin Renvoize 2020-01-10 10:38:51 UTC
Rebased, Cleaned up for QA complaints and Signed off..

Trying to be pragmatic to get the feature in and then we'll handle the further cleanup of login in followup bugs.
Comment 29 Tomás Cohen Arazi 2020-02-29 12:59:01 UTC
This mostly looks good. I'm adding some more tests for full coverage and also changing some bits based on later guidelines as this dev was written a while ago.
Comment 30 Tomás Cohen Arazi 2020-02-29 20:14:29 UTC
Created attachment 99821 [details] [review]
Bug 15985: Include transacting library in fines

Some librarians would like to see the library an item was checked out
from in the fines table on the fines payment page.

Test Plan:
1) Apply this patch
2) Find a patron with overdue items that have fines
3) Browse to the pay fines page
4) Note the new "checked out from" column

Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 31 Tomás Cohen Arazi 2020-02-29 20:14:35 UTC
Created attachment 99822 [details] [review]
Bug 15985: Add new method Koha::Account::Line::checkout
Comment 32 Tomás Cohen Arazi 2020-02-29 20:14:48 UTC
Created attachment 99823 [details] [review]
Bug 15985: (QA Follow up) Fix issues found by koha-qa.pl

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 33 Tomás Cohen Arazi 2020-02-29 20:14:56 UTC
Created attachment 99824 [details] [review]
Bug 15985: (QA follow-up) Revert to Koha::Object bases

Being pragmatic to try and move this bug on, I've reverted the changed
base class for the Old:: classes and cloned the required code from the
corresponding Object classes.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 34 Tomás Cohen Arazi 2020-02-29 20:15:04 UTC
Created attachment 99825 [details] [review]
Bug 15985: Add more tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 35 Jonathan Druart 2020-03-02 08:55:06 UTC
There is not FK on old_issues.branchcode, I think that should be taken into account in Koha::Old::Issue->library

Also, POD and tests missing for Koha::Old::Issue->claim_returned
Comment 36 Jonathan Druart 2020-03-02 08:57:50 UTC
(In reply to Jonathan Druart from comment #23)
> 1. Changes are made in Koha/Schema/Result/OldIssue.pm, above the "DO NOT
> MODIFY THIS OR ANYTHING ABOVE!" line.
> Is it expected? Did you forget to attached the DB changes patch?
> If you wanted to add it after that line, why not adding the constraint at DB
> level?
> 
> Why not for Issue.pm?
> 
> 2.
> -use base qw(Koha::Object);
> +use base qw(Koha::Checkout);
> 
> Is this really needed? Why?
> If so I think it must be moved to its own bug report, explaining why we need
> it and what it will bring. Also tests must be provided for such changes.
> 
> Note that both t/db_dependent/Koha/Account/Lines.t and
> t/db_dependent/Koha/Checkouts.t are passing without that change.

And the 2 points here are been ignored. Please comment.
Comment 37 Kyle M Hall 2020-04-17 12:50:12 UTC
(In reply to Jonathan Druart from comment #36)
> (In reply to Jonathan Druart from comment #23)
> > 1. Changes are made in Koha/Schema/Result/OldIssue.pm, above the "DO NOT
> > MODIFY THIS OR ANYTHING ABOVE!" line.
> > Is it expected? Did you forget to attached the DB changes patch?
> > If you wanted to add it after that line, why not adding the constraint at DB
> > level?
> > 
> > Why not for Issue.pm?

We don't want to delete old issues just because a branch was removed. They are often needed for reporting purposes. It is presumed that current checkouts will be resolved before deleting a branch. That being said, it should be below the DO NOT line. That could be a rebasing accident.

> > 
> > 2.
> > -use base qw(Koha::Object);
> > +use base qw(Koha::Checkout);
> > 
> > Is this really needed? Why?
> > If so I think it must be moved to its own bug report, explaining why we need
> > it and what it will bring. Also tests must be provided for such changes.
> > 
> > Note that both t/db_dependent/Koha/Account/Lines.t and
> > t/db_dependent/Koha/Checkouts.t are passing without that change.
> 

The allows old checkouts objects to inherit the same methods the checkouts object has, without the need to duplicate all the code.
Comment 38 Kyle M Hall 2020-04-17 13:17:14 UTC
Created attachment 103152 [details] [review]
Bug 15985: Include transacting library in fines

Some librarians would like to see the library an item was checked out
from in the fines table on the fines payment page.

Test Plan:
1) Apply this patch
2) Find a patron with overdue items that have fines
3) Browse to the pay fines page
4) Note the new "checked out from" column

Signed-off-by: Jason Robb <jrobb@sekls.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 39 Kyle M Hall 2020-04-17 13:17:25 UTC
Created attachment 103153 [details] [review]
Bug 15985: Add new method Koha::Account::Line::checkout
Comment 40 Kyle M Hall 2020-04-17 13:17:28 UTC
Created attachment 103154 [details] [review]
Bug 15985: (QA Follow up) Fix issues found by koha-qa.pl

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 41 Kyle M Hall 2020-04-17 13:17:32 UTC
Created attachment 103155 [details] [review]
Bug 15985: (QA follow-up) Revert to Koha::Object bases

Being pragmatic to try and move this bug on, I've reverted the changed
base class for the Old:: classes and cloned the required code from the
corresponding Object classes.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 42 Kyle M Hall 2020-04-17 13:17:35 UTC
Created attachment 103156 [details] [review]
Bug 15985: Add more tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 43 Kyle M Hall 2020-04-17 13:17:38 UTC
Created attachment 103157 [details] [review]
Bug 15985: Move dbic relationship to correct part of file
Comment 44 Katrin Fischer 2020-05-02 02:21:31 UTC
1) It looks like we are still stuck with this one:

 FAIL	Koha/Old/Checkout.pm
   FAIL	  valid
		Inconsistent hierarchy during C3 merge of class 'Koha::Old::Checkout':
			current merge results [
				Koha::Old::Checkout,
			]
			merging failed on 'Koha::Checkout' 

I have to admit, I am not sure what it means - is there a chance we could resolve  or at least explain it?


2) =head3 claim_returned

This seems unrelated to the patch - could it have slipped in?
Comment 45 Katrin Fischer 2020-05-02 02:26:09 UTC
3) The column seems to be missing from column configuration (no change to .yml and I can't spot it.

4) I'd like to see it moved before the issue date added by bug 24995.
At the moment it's between Account type and description and I feel like we should keep those together at the beginning of the table.
Comment 46 Kyle M Hall 2020-05-07 11:27:27 UTC
Created attachment 104491 [details] [review]
Bug 15985: Move new column to the right of checkout date
Comment 47 Kyle M Hall 2020-05-07 11:27:38 UTC
Created attachment 104492 [details] [review]
Bug 15985: Add to columns configuration
Comment 48 Katrin Fischer 2020-05-07 21:06:12 UTC
(In reply to Katrin Fischer from comment #44)
> 1) It looks like we are still stuck with this one:
> 
>  FAIL	Koha/Old/Checkout.pm
>    FAIL	  valid
> 		Inconsistent hierarchy during C3 merge of class 'Koha::Old::Checkout':
> 			current merge results [
> 				Koha::Old::Checkout,
> 			]
> 			merging failed on 'Koha::Checkout' 
> 
> I have to admit, I am not sure what it means - is there a chance we could
> resolve  or at least explain it?
> 
> 
> 2) =head3 claim_returned
> 
> This seems unrelated to the patch - could it have slipped in?

Hi Kyle, could you check these out too please?
Comment 49 Kyle M Hall 2020-05-20 12:59:21 UTC
Created attachment 105144 [details] [review]
Bug 15985: Fix QA script issues

* Use original base for old checkout objects
* test rs for results before returning a patron object
Comment 50 Jonathan Druart 2020-05-21 09:22:27 UTC
There is no QA stamp here, back to NQA.
Comment 51 Katrin Fischer 2020-05-22 11:22:49 UTC
The +=head3 claim_returned is still included in this patch set. Please have a look and remove it or explain please.
Comment 52 Kyle M Hall 2020-05-27 14:29:30 UTC
Created attachment 105398 [details] [review]
Bug 15985: Remove Koha::Old::Checkout::claim_returned
Comment 53 Katrin Fischer 2020-05-28 21:56:54 UTC
Hi Kyle, 

I am sorry, but this doesn't quite work as expected. Possibly the table structure changed a bit since this was originally written.

1) Total columns at the end of table are off by at least one column:

Outstanding credits could be applied: 	
Total due if credit applied: 	

2) Hiding columns in column configuration doesn't work.
Comment 54 Tomás Cohen Arazi 2020-08-31 19:22:09 UTC
With Kyle's blessing, I started a new bug that will cleanly implement the same this patchset does, but easier to apply and understand.

*** This bug has been marked as a duplicate of bug 26327 ***