Bug 22421 - accountlines.issue_id is missing a foreign key constraint
Summary: accountlines.issue_id is missing a foreign key constraint
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 5334 14825 17976
  Show dependency treegraph
 
Reported: 2019-02-27 11:42 UTC by Martin Renvoize
Modified: 2024-03-26 14:00 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22421: Add tests for new constraints (2.38 KB, patch)
2019-02-27 13:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22421: Add missing constraints (3.98 KB, patch)
2019-02-27 13:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22421: Ensure old_issue_id is populated in accountlines (1.05 KB, patch)
2019-02-27 13:55 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22421: Add tests for new constraints (2.71 KB, patch)
2019-06-06 11:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22421: Add missing constraints (4.45 KB, patch)
2019-06-06 11:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22421: Ensure old_issue_id is populated in accountlines (1.21 KB, patch)
2019-06-06 11:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22421: Add tests for new constraints (2.70 KB, patch)
2019-06-06 12:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22421: Add missing constraints (4.45 KB, patch)
2019-06-06 12:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22421: Ensure old_issue_id is populated in accountlines (1.21 KB, patch)
2019-06-06 12:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22421: (follow-up) Correct checkout relation (2.22 KB, patch)
2019-06-06 12:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22421: Add tests for new constraints (2.77 KB, patch)
2019-06-06 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22421: Add missing constraints (4.51 KB, patch)
2019-06-06 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22421: Ensure old_issue_id is populated in accountlines (1.28 KB, patch)
2019-06-06 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22421: (follow-up) Correct checkout relation (2.29 KB, patch)
2019-06-06 13:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 22421: Add tests for new constraints (4.38 KB, patch)
2024-02-08 14:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22421: Add missing constraints (6.49 KB, patch)
2024-02-08 14:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22421: Ensure old_issue_id is populated in accountlines (1.25 KB, patch)
2024-02-08 14:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22421: (follow-up) Correct checkout relation (4.52 KB, patch)
2024-02-08 14:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22421: Schema Update (4.56 KB, patch)
2024-02-08 14:42 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22421: Add tests for new constraints (3.60 KB, patch)
2024-02-16 20:48 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22421: Add missing constraints (5.69 KB, patch)
2024-02-16 20:48 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22421: Ensure old_issue_id is populated in accountlines (1.30 KB, patch)
2024-02-16 20:49 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22421: (follow-up) Correct checkout relation (4.57 KB, patch)
2024-02-16 20:49 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 22421: Schema Update (4.62 KB, patch)
2024-02-16 20:49 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2019-02-27 11:42:14 UTC

    
Comment 1 Martin Renvoize 2019-02-27 12:30:28 UTC
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.
Comment 2 Martin Renvoize 2019-02-27 13:55:32 UTC
Created attachment 85773 [details] [review]
Bug 22421: Add tests for new constraints
Comment 3 Martin Renvoize 2019-02-27 13:55:34 UTC
Created attachment 85774 [details] [review]
Bug 22421: Add missing constraints
Comment 4 Martin Renvoize 2019-02-27 13:55:37 UTC
Created attachment 85775 [details] [review]
Bug 22421: Ensure old_issue_id is populated in accountlines
Comment 5 Josef Moravec 2019-03-15 10:15:22 UTC
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...
Comment 6 Martin Renvoize 2019-06-06 11:45:00 UTC
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>
Comment 7 Martin Renvoize 2019-06-06 11:45:04 UTC
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>
Comment 8 Martin Renvoize 2019-06-06 11:45:06 UTC
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>
Comment 9 Martin Renvoize 2019-06-06 12:26:24 UTC
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>
Comment 10 Martin Renvoize 2019-06-06 12:26:27 UTC
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>
Comment 11 Martin Renvoize 2019-06-06 12:26:30 UTC
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>
Comment 12 Martin Renvoize 2019-06-06 12:26:33 UTC
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>
Comment 13 Kyle M Hall 2019-06-06 13:09:13 UTC
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>
Comment 14 Kyle M Hall 2019-06-06 13:09:23 UTC
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>
Comment 15 Kyle M Hall 2019-06-06 13:09:26 UTC
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>
Comment 16 Kyle M Hall 2019-06-06 13:09:28 UTC
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>
Comment 17 Katrin Fischer 2019-06-12 05:54:18 UTC
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.
Comment 18 Katrin Fischer 2019-06-12 06:01:32 UTC
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.
Comment 19 Katrin Fischer 2019-07-11 06:30:17 UTC
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
Comment 20 Martin Renvoize 2024-02-08 14:42:42 UTC
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>
Comment 21 Martin Renvoize 2024-02-08 14:42:45 UTC
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>
Comment 22 Martin Renvoize 2024-02-08 14:42:48 UTC
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>
Comment 23 Martin Renvoize 2024-02-08 14:42:51 UTC
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>
Comment 24 Martin Renvoize 2024-02-08 14:42:53 UTC
Created attachment 161923 [details] [review]
Bug 22421: Schema Update
Comment 25 Martin Renvoize 2024-02-08 14:43:36 UTC
Rebased and brought up to modern standards..

Back to read for QA
Comment 26 Victor Grousset/tuxayo 2024-02-16 20:48:55 UTC
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>
Comment 27 Victor Grousset/tuxayo 2024-02-16 20:48:58 UTC
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>
Comment 28 Victor Grousset/tuxayo 2024-02-16 20:49:01 UTC
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>
Comment 29 Victor Grousset/tuxayo 2024-02-16 20:49:03 UTC
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>
Comment 30 Victor Grousset/tuxayo 2024-02-16 20:49:06 UTC
Created attachment 162248 [details] [review]
Bug 22421: Schema Update

Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 31 Victor Grousset/tuxayo 2024-02-16 20:49:40 UTC
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)
Comment 32 Victor Grousset/tuxayo 2024-02-16 22:03:34 UTC
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.