Work to be done: 1) Add column issue_id to accountlines table 2) Modify fines generation to set issue_id for fine upon generation 3) Modify updatedatabase.pl to insert issue_id in accountlines for currently accruing fines 4) On the pay fines page, use this data to display the library where the item was checked out from
Steps 1 through 3 are completed, only step 4 remains!
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
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>
I don't understand why this is signed off while the dependencies do not apply.
Please rebase.
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>
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.
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>
Created attachment 72612 [details] [review] Bug 15985: Add new method Koha::Account::Line::checkout
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
This should be rebased and adapted on top of bug 19489
+1
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>
Created attachment 90318 [details] [review] Bug 15985: Add new method Koha::Account::Line::checkout
Might this be a good opportunity to introduce a proper foreign key constraint at the DB level for issues.branchcode and old_issues.branchcode?
(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!
I'm resurrecting bug 22421 for the FK piece at the minute.. may also be nice to have bug 22683 feed into this too.
Created attachment 90364 [details] [review] Bug 15985: Add foreign key for issues tables
Comment on attachment 90364 [details] [review] Bug 15985: Add foreign key for issues tables We should move this patch to its' own bug.
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?
Created attachment 90478 [details] [review] Bug 15985: (QA Follow up) Fix issues found by koha-qa.pl
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.
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.
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>
Created attachment 97178 [details] [review] Bug 15985: Add new method Koha::Account::Line::checkout
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>
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>
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.
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.
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>
Created attachment 99822 [details] [review] Bug 15985: Add new method Koha::Account::Line::checkout
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>
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>
Created attachment 99825 [details] [review] Bug 15985: Add more tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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
(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.
(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.
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>
Created attachment 103153 [details] [review] Bug 15985: Add new method Koha::Account::Line::checkout
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>
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>
Created attachment 103156 [details] [review] Bug 15985: Add more tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 103157 [details] [review] Bug 15985: Move dbic relationship to correct part of file
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?
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.
Created attachment 104491 [details] [review] Bug 15985: Move new column to the right of checkout date
Created attachment 104492 [details] [review] Bug 15985: Add to columns configuration
(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?
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
There is no QA stamp here, back to NQA.
The +=head3 claim_returned is still included in this patch set. Please have a look and remove it or explain please.
Created attachment 105398 [details] [review] Bug 15985: Remove Koha::Old::Checkout::claim_returned
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.
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 ***