This is a more challenging one, as issue_id refers to not one but two distinct tables .. issues and old_issues.. as such we either need to merge issues and old_issues or split this constraint into two distinct fields and ensure they're properly populated. I've opted for the distinct fields in this patch for now as it's the simpler approach and can be easily switched at a later date for the issue/old_issue merge.
Created attachment 85773 [details] [review] Bug 22421: Add tests for new constraints
Created attachment 85774 [details] [review] Bug 22421: Add missing constraints
Created attachment 85775 [details] [review] Bug 22421: Ensure old_issue_id is populated in accountlines
Sorry, can't apply: Applying: Bug 22421: Add tests for new constraints error: sha1 information is lacking or useless (t/db_dependent/Koha/Account/Lines.t). There are also some templates referencing issue_id, it should be fixed, and Koha::Accout::Line->checkout should be adjusted too...
Created attachment 90352 [details] [review] Bug 22421: Add tests for new constraints Test plan: * Read the updated tests and verify that they make sense and do not remove anything previously tested for. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 90353 [details] [review] Bug 22421: Add missing constraints DB update to add a new old_issue_id field to accountlines and set foreign key constraints for both the new field and the existing issue_id field. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 90354 [details] [review] Bug 22421: Ensure old_issue_id is populated in accountlines Update C4::Circulation::MarkIssueReturned to handle moving issue_id to old_issue_id in accountlines. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 90359 [details] [review] Bug 22421: Add tests for new constraints Test plan: * Read the updated tests and verify that they make sense and do not remove anything previously tested for. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 90360 [details] [review] Bug 22421: Add missing constraints DB update to add a new old_issue_id field to accountlines and set foreign key constraints for both the new field and the existing issue_id field. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 90361 [details] [review] Bug 22421: Ensure old_issue_id is populated in accountlines Update C4::Circulation::MarkIssueReturned to handle moving issue_id to old_issue_id in accountlines. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 90362 [details] [review] Bug 22421: (follow-up) Correct checkout relation The Koha::Account::Line->checkout relationship accessor needed an update to respect the new fields and foreign keys introduced here. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 90365 [details] [review] Bug 22421: Add tests for new constraints Test plan: * Read the updated tests and verify that they make sense and do not remove anything previously tested for. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 90366 [details] [review] Bug 22421: Add missing constraints DB update to add a new old_issue_id field to accountlines and set foreign key constraints for both the new field and the existing issue_id field. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 90367 [details] [review] Bug 22421: Ensure old_issue_id is populated in accountlines Update C4::Circulation::MarkIssueReturned to handle moving issue_id to old_issue_id in accountlines. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 90368 [details] [review] Bug 22421: (follow-up) Correct checkout relation The Koha::Account::Line->checkout relationship accessor needed an update to respect the new fields and foreign keys introduced here. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Trying to test this, but I am having issues getting a lost replacement fee to charge. I have set a replacement default cost on item level and set WhenLostChargeReplacementFee to Charge. Items are returned on setting the lost status. Not sure if this is related yet, but maybe you can help verify.
I also tried charging overdue fines and returning the items. After returning them, the due date and return date information are no longer visible in accountlines (not implemented yet?). old_issue_id is set, so that works. I'll have to reset my db to check the lost behaviour, running a bit low on time right now, help appreciated.
I got myself into trouble here: 1) Check out item backdated, so it's overdue 2) Run fines.pl to accrue fines 3) Return it 4) Verify title etc. still show - Apply patchset 1) Run updatedatabase and dbic, restart_all 2) Check out same item backdated again 3) Return from patron account DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`accountlines`, CONSTRAINT `accountlines_ibfk_issues` FOREIGN KEY (`issue_id`) REFERENCES `issues` (`issue_id`) ON DELETE SET NULL ON UPDATE CASCADE) [for Statement "INSERT INTO `accountlines` ( `accounttype`, `amount`, `amountoutstanding`, `borrowernumber`, `branchcode`, `date`, `description`, `interface`, `issue_id`, `itemnumber`, `manager_id`, `note`, `payment_type`, `status`) VALUES ( ?, ?, ?, ?, ?, NOW(), ?, ?, ?, ?, ?, ?, ?, ? )" with ParamValues: 0='OVERDUE', 1=0.9, 2=0.9, 3=51, 4=undef, 5='Conspirator : 07/01/2019 23:59', 6='intranet', 7=4, 8=22, 9=undef, 10='', 11=undef, 12='UNRETURNED'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1836. Broken FK constraint at /usr/share/perl5/Exception/Class/Base.pm line 88 Exception::Class::Base::throw('Koha::Exceptions::Object::FKConstraint', 'error', 'Broken FK constraint', 'broken_fk', 'issue_id') called at /home/vagrant/kohaclone/Koha/Object.pm line 163
Created attachment 161919 [details] [review] Bug 22421: Add tests for new constraints Test plan: * Read the updated tests and verify that they make sense and do not remove anything previously tested for. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 161920 [details] [review] Bug 22421: Add missing constraints DB update to add a new old_issue_id field to accountlines and set foreign key constraints for both the new field and the existing issue_id field. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 161921 [details] [review] Bug 22421: Ensure old_issue_id is populated in accountlines Update C4::Circulation::MarkIssueReturned to handle moving issue_id to old_issue_id in accountlines. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 161922 [details] [review] Bug 22421: (follow-up) Correct checkout relation The Koha::Account::Line->checkout relationship accessor needed an update to respect the new fields and foreign keys introduced here. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 161923 [details] [review] Bug 22421: Schema Update
Rebased and brought up to modern standards.. Back to read for QA
Created attachment 162244 [details] [review] Bug 22421: Add tests for new constraints Test plan: * Read the updated tests and verify that they make sense and do not remove anything previously tested for. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 162245 [details] [review] Bug 22421: Add missing constraints DB update to add a new old_issue_id field to accountlines and set foreign key constraints for both the new field and the existing issue_id field. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 162246 [details] [review] Bug 22421: Ensure old_issue_id is populated in accountlines Update C4::Circulation::MarkIssueReturned to handle moving issue_id to old_issue_id in accountlines. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 162247 [details] [review] Bug 22421: (follow-up) Correct checkout relation The Koha::Account::Line->checkout relationship accessor needed an update to respect the new fields and foreign keys introduced here. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 162248 [details] [review] Bug 22421: Schema Update Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
I'm not sure about all of the implications and places to change but at least here is another signoff. Changes in tests make sense. Additional testing: Confirmed that purged old_issues (cleanup_database.pl) are handled well :) (accountlines.issue_id is nulled)
QA script reports this: WARN C4/Circulation.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 1095, now: 1098) WARN Koha/Schema/Result/Accountline.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 142, now: 162) WARN Koha/Schema/Result/Issue.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 108, now: 112) WARN Koha/Schema/Result/OldIssue.pm WARN tidiness The file is less tidy than before (bad/messy lines before: 106, now: 110) At the time of the 1st submission these patches, perltidy was already in Coding Guidelines (PERL1), and was about default perltidy config. Likely a lot of current errors are from default config and not or few customization. So at least a lot at the messy lines would be needed.
Taking a look at this one... It looks like there are some test failures: t/db_dependent/Koha/Account/Line.t .. 1/15 # Looks like you planned 4 tests but ran 2. # Failed test 'library() tests' # at t/db_dependent/Koha/Account/Line.t line 173. DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'me.old_issue_id' in 'field list' at /kohadevbox/koha/Koha/Objects.pm line 95 # Looks like your test exited with 255 just after 4. t/db_dependent/Koha/Account/Line.t .. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 12/15 subtests t/db_dependent/Circulation.t .. 17/74 # Failed test 'CanBookBeRenewed tests' # at t/db_dependent/Circulation.t line 1753. DBIx::Class::Storage::DBI::_dbh_execute(): DBI Exception: DBD::mysql::st execute failed: Unknown column 'me.old_issue_id' in 'field list' at /kohadevbox/koha/Koha/Objects.pm line 319 # Looks like your test exited with 11 just after 17. t/db_dependent/Circulation.t .. Dubious, test returned 11 (wstat 2816, 0xb00) Failed 58/74 subtests I'm also not sure it makes sense to put all steps of the dbrev inside the "unless" that checks for the existence of the old_issue_id column. If for some reason the column exists but the foreign keys don't exist (e.g. a previous update failed in the middle), the whole dbrev would be skipped and the foreign keys wouldn't be created. I think it should be pretty doable to have a separate check for most individual steps of the dbrev.
Created attachment 166631 [details] [review] Bug 22421: (follow-up) Check issue during add_debit We need additional handling for the case where a debt may be added with an issue_id after the issue is returned. This patch tries to fix that at a low level, but introduces new test failures in t/db_dependent/Circulation.t to be investigated.
More work needed here so leaving in FQA
This is needed to move the ODUE notices to TT which would be a real nice thing. Can we get this moving again?
Created attachment 173287 [details] [review] Bug 22421 (follow-up): Check issue during add_credit Additional handling for the case where a credit is added with an issue_id after the issue is returned. add_credit will keep the same method signature, where the issue_id parameter may now refer to a current issue or an old issue. It will then internally determine whether it is a current or old checkout and handle it appropriately. If the caller is updating an account line, or adding a credit to refund an existing debit, they may not need to know whether it is for a current checkout or an old checkout. In this case, they can use the account line's ->checkout accessor to pass in the appropriate issue_id regardless of whether it is a current or old checkout.
Created attachment 173288 [details] [review] Bug 22421: (follow-up) Use checkout accessor to get correct issue_id In some cases, we may want to update an AccountLine that is linked to a checkout without knowing whether it is a current or old checkout. In these cases, use the AccountLines ->checkout accessor to ensure we get the issue_id correctly, regardless of whether it is a current issue or and old_issue. This also applies when generating a new AccountLine based on an existing AccountLine (for example, adding a credit to refund a previously charged fine)
Created attachment 173289 [details] [review] Bug 22421: (follow-up) Fix tests Need to search by old_issue_id instead of issue_id when checking the value of an accountline after calling AddReturn on the linked checkout
I was able to get tests passing again with a few small adjustments to pm files to handle cases where the attached checkout may be a current checkout or an old checkout. I do still think an improvement could be made to the reliability of the dbrev (see https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22421#c33), so leaving at FQA for now. (I can most likely come back and submit a patch for that later today.) Finally, removing myself as the QA contact, as I feel I've made enough changes in my follow-up patches to warrant another pair of eyes on them before this moves into PQA status.
Created attachment 173305 [details] [review] Bug 22421: (follow-up) Improve recoverability if dbrev fails in the middle
Created attachment 173790 [details] [review] Bug 22421: Add tests for new constraints Test plan: * Read the updated tests and verify that they make sense and do not remove anything previously tested for. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173791 [details] [review] Bug 22421: Add missing constraints DB update to add a new old_issue_id field to accountlines and set foreign key constraints for both the new field and the existing issue_id field. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173792 [details] [review] Bug 22421: Ensure old_issue_id is populated in accountlines Update C4::Circulation::MarkIssueReturned to handle moving issue_id to old_issue_id in accountlines. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173793 [details] [review] Bug 22421: (follow-up) Correct checkout relation The Koha::Account::Line->checkout relationship accessor needed an update to respect the new fields and foreign keys introduced here. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173794 [details] [review] Bug 22421: Schema Update Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173795 [details] [review] Bug 22421: (follow-up) Check issue during add_debit We need additional handling for the case where a debt may be added with an issue_id after the issue is returned. This patch tries to fix that at a low level, but introduces new test failures in t/db_dependent/Circulation.t to be investigated. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173796 [details] [review] Bug 22421: (follow-up): Check issue during add_credit Additional handling for the case where a credit is added with an issue_id after the issue is returned. add_credit will keep the same method signature, where the issue_id parameter may now refer to a current issue or an old issue. It will then internally determine whether it is a current or old checkout and handle it appropriately. If the caller is updating an account line, or adding a credit to refund an existing debit, they may not need to know whether it is for a current checkout or an old checkout. In this case, they can use the account line's ->checkout accessor to pass in the appropriate issue_id regardless of whether it is a current or old checkout. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173797 [details] [review] Bug 22421: (follow-up) Use checkout accessor to get correct issue_id In some cases, we may want to update an AccountLine that is linked to a checkout without knowing whether it is a current or old checkout. In these cases, use the AccountLines ->checkout accessor to ensure we get the issue_id correctly, regardless of whether it is a current issue or and old_issue. This also applies when generating a new AccountLine based on an existing AccountLine (for example, adding a credit to refund a previously charged fine) Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173798 [details] [review] Bug 22421: (follow-up) Fix tests Need to search by old_issue_id instead of issue_id when checking the value of an accountline after calling AddReturn on the linked checkout Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 173799 [details] [review] Bug 22421: (follow-up) Improve recoverability if dbrev fails in the middle Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I'm happy with all the follow-ups etc.. it all makes sense to me.. Honestly.. with the first revision of this written by me 5+ years ago I have no recollection of that code. I think we should set this PQA to prevent further holdup of downstream bugs.
Pushed for 24.11! Well done everyone, thank you!
I am afraid we have some broken tests that probably relate to the changes here: t_db_dependent_api_v1_patrons_accounts_t
Created attachment 174281 [details] [review] Bug 22421: (QA follow-up) Update API mapping and specs We missed some cases where the API specs and mappings were missing for the new old_issue_id field. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Please remove the additional_work_needed flag if you're done Katrin.. it's getting impossible to keep on top of this stuff.
(In reply to Martin Renvoize (ashimema) from comment #56) > Please remove the additional_work_needed flag if you're done Katrin.. it's > getting impossible to keep on top of this stuff. The keyword is on my review list for today. I usually remove them, but missed the "push note" here too.