We have a foreign key constraint in return_claims for issue_id.. this means that if you have 'MarkLostItemsAsReturned' enabled, we first add the issue_id linking to the original checkout, then immediately move the checkout to old_issues and nuke the issue_id from the claims_return table :(
Created attachment 127711 [details] [review] Bug 29495: Drop issue_id constraint from return_claims This patch removes the issue_id constraint from return_claims. Due to the nature of our dual table approach to checkouts/old_checkouts we can't safely hae this constraint and not lose data. Prior to this commit, when an item is checked in we move the checkout from checkouts to old_checkouts.. this therefore triggers the delete of the issue_id from the return_claims table as described by the foreign key constraint.
Created attachment 127712 [details] [review] Bug 29495: DBIC Schema Rebuild
Created attachment 127713 [details] [review] Bug 29495: Add DBIC relationships back into Schema This patch adds the relationship accessors back into the affected Schema classes, now below the fold so they are retained during dbic rebuilds.
Created attachment 127714 [details] [review] Bug 29495: Add code level constraint replacement This patch adds a check in Koha::Checkouts::ReturnClaim::store to replace the database level foreign key check.
DB update should use primary_key_exists so it is idempotent. The last patch could add claim object methods that use something like return Koha::Checkouts->new_from_dbic( $self->_result->issue ) to allow prefetching, but I don't think that's absolutely necessary.
Created attachment 127765 [details] [review] Bug 29495: Drop issue_id constraint from return_claims This patch removes the issue_id constraint from return_claims. Due to the nature of our dual table approach to checkouts/old_checkouts we can't safely hae this constraint and not lose data. Prior to this commit, when an item is checked in we move the checkout from checkouts to old_checkouts.. this therefore triggers the delete of the issue_id from the return_claims table as described by the foreign key constraint.
Created attachment 127766 [details] [review] Bug 29495: DBIC Schema Rebuild
Created attachment 127767 [details] [review] Bug 29495: Add DBIC relationships back into Schema This patch adds the relationship accessors back into the affected Schema classes, now below the fold so they are retained during dbic rebuilds.
Created attachment 127768 [details] [review] Bug 29495: Add code level constraint replacement This patch adds a check in Koha::Checkouts::ReturnClaim::store to replace the database level foreign key check.
Created attachment 127769 [details] [review] Bug 29495: Update relationship methods This patch updates the relationship methods found in Koha::Checkouts::ReturnClaim so that they are prefetchable and embeddable on the API.
Thanks for the review Kyle.. I've implemented the requested improvements now :)
Created attachment 127809 [details] [review] Bug 29495: Drop issue_id constraint from return_claims This patch removes the issue_id constraint from return_claims. Due to the nature of our dual table approach to checkouts/old_checkouts we can't safely hae this constraint and not lose data. Prior to this commit, when an item is checked in we move the checkout from checkouts to old_checkouts.. this therefore triggers the delete of the issue_id from the return_claims table as described by the foreign key constraint. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 127810 [details] [review] Bug 29495: DBIC Schema Rebuild Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 127811 [details] [review] Bug 29495: Add DBIC relationships back into Schema This patch adds the relationship accessors back into the affected Schema classes, now below the fold so they are retained during dbic rebuilds. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 127812 [details] [review] Bug 29495: Add code level constraint replacement This patch adds a check in Koha::Checkouts::ReturnClaim::store to replace the database level foreign key check. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 127813 [details] [review] Bug 29495: Update relationship methods This patch updates the relationship methods found in Koha::Checkouts::ReturnClaim so that they are prefetchable and embeddable on the API. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 127937 [details] [review] Bug 29495: Unit Tests Test plan 1. Run updated tests prior to applying new patches.. pass 2. Run updated tests after applying new patches.. pass
Created attachment 127938 [details] [review] Bug 29495: Drop issue_id constraint from return_claims This patch removes the issue_id constraint from return_claims. Due to the nature of our dual table approach to checkouts/old_checkouts we can't safely hae this constraint and not lose data. Prior to this commit, when an item is checked in we move the checkout from checkouts to old_checkouts.. this therefore triggers the delete of the issue_id from the return_claims table as described by the foreign key constraint. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 127939 [details] [review] Bug 29495: DBIC Schema Rebuild Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 127940 [details] [review] Bug 29495: Add DBIC relationships back into Schema This patch adds the relationship accessors back into the affected Schema classes, now below the fold so they are retained during dbic rebuilds. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 127941 [details] [review] Bug 29495: Add code level constraint replacement This patch adds a check in Koha::Checkouts::ReturnClaim::store to replace the database level foreign key check. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 127942 [details] [review] Bug 29495: Update relationship methods This patch updates the relationship methods found in Koha::Checkouts::ReturnClaim so that they are prefetchable and embeddable on the API. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 127943 [details] [review] Bug 29495: (follow-up) Use 'item' relationship This patch adds the missing 'item' relationship in Checkouts::ReturnClaim and then uses it from the resolve method. This improve the reliability of the resolution code so it works when the item has already been checked in (without having to check Old::Checkouts).
Created attachment 127944 [details] [review] Bug 29495: Add relation tests This patch adds missing tests for relationship accessors in the ReturnClaim class.
Added missing unit tests.. we're well covered now.. over to QA
(Wouldn't it be better to put effort in merging the tables?)
(In reply to Jonathan Druart from comment #26) > (Wouldn't it be better to put effort in merging the tables?) We have blocked a lot of important bug fixes because of the merge table question... at the moment I think fixing bugs is more reachable than the table merge which also could never be backported.
Created attachment 129105 [details] [review] Bug 29495: Unit Tests Test plan 1. Run updated tests prior to applying new patches.. pass 2. Run updated tests after applying new patches.. pass Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 129106 [details] [review] Bug 29495: Drop issue_id constraint from return_claims This patch removes the issue_id constraint from return_claims. Due to the nature of our dual table approach to checkouts/old_checkouts we can't safely hae this constraint and not lose data. Prior to this commit, when an item is checked in we move the checkout from checkouts to old_checkouts.. this therefore triggers the delete of the issue_id from the return_claims table as described by the foreign key constraint. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 129107 [details] [review] Bug 29495: DBIC Schema Rebuild Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 129109 [details] [review] Bug 29495: Add DBIC relationships back into Schema This patch adds the relationship accessors back into the affected Schema classes, now below the fold so they are retained during dbic rebuilds. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 129110 [details] [review] Bug 29495: Add code level constraint replacement This patch adds a check in Koha::Checkouts::ReturnClaim::store to replace the database level foreign key check. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 129111 [details] [review] Bug 29495: Update relationship methods This patch updates the relationship methods found in Koha::Checkouts::ReturnClaim so that they are prefetchable and embeddable on the API. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 129112 [details] [review] Bug 29495: (follow-up) Use 'item' relationship This patch adds the missing 'item' relationship in Checkouts::ReturnClaim and then uses it from the resolve method. This improve the reliability of the resolution code so it works when the item has already been checked in (without having to check Old::Checkouts). Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 129113 [details] [review] Bug 29495: Add relation tests This patch adds missing tests for relationship accessors in the ReturnClaim class. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 22.05, thanks to everybody involved [U+1F984]
Arf this breaks the unit test t/db_dependent/TestBuilder.t : Value not allowed for auto_incr issue_id in Issue at /kohadevbox/koha/t/lib/TestBuilder.pm line 387. not ok 507 - Testing Koha::Checkouts::ReturnClaims # Failed test 'Testing Koha::Checkouts::ReturnClaims' # at t/db_dependent/TestBuilder.t line 401. # got: '' # expected: 'Koha::Checkouts::ReturnClaim' not ok 508 - Module Koha::Checkouts::ReturnClaims should have koha_object[s]_class method if needed # Failed test 'Module Koha::Checkouts::ReturnClaims should have koha_object[s]_class method if needed' # at t/db_dependent/TestBuilder.t line 404. # got: 'Can't call method "get_from_storage" on an undefined value at t/db_dependent/TestBuilder.t line 403. # ' # expected: '' 1..508 # Looks like you failed 2 tests of 508. Can you have a look please ?
Created attachment 129547 [details] [review] Bug 29495: (follow-up) Fix Tests Update DBIC relation types to prevent double issue creation during object build.
(In reply to Martin Renvoize from comment #38) > Created attachment 129547 [details] [review] [review] > Bug 29495: (follow-up) Fix Tests > > Update DBIC relation types to prevent double issue creation during > object build. Don't push that right now.
Created attachment 129575 [details] [review] Bug 29495: Fix TestBuilder.t Value not allowed for auto_incr issue_id in Issue at /kohadevbox/koha/t/lib/TestBuilder.pm line 387.
Martin, this patch fixes the failure and don't modify the relation. It seems better to me. What do you think?
Seems like a better approach to me :).. I wasn't close enough to TestBuilder to spot where I'd need to do that. Thanks Jonathan
Created attachment 129582 [details] [review] Bug 29495: Fix TestBuilder.t Value not allowed for auto_incr issue_id in Issue at /kohadevbox/koha/t/lib/TestBuilder.pm line 387. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Thanks Martin and Jonathan I understand the keyword "additional_work_needed". UT is now OK : kohadev-koha@kohadevbox:/kohadevbox/koha$ prove t/db_dependent/TestBuilder.t t/db_dependent/TestBuilder.t .. ok All tests successful. Files=1, Tests=15, 42 wallclock secs ( 0.07 usr 0.01 sys + 32.63 cusr 8.45 csys = 41.16 CPU) Result: PASS Tell me if can be pushed.
(In reply to Fridolin Somers from comment #44) > Tell me if can be pushed. Yes, it's ready for push.
Pushed master : Bug 29495: Fix TestBuilder.t
Pushed to 21.11.x for 21.11.03
I'm getting a qa failure after applying these to 21.05. Should this maybe depend on bug 28588? Please rebase for backport to 21.05 if possible. Thanks! Here's the error I got: FAIL t/db_dependent/Koha/Checkouts/ReturnClaim.t FAIL valid Version control conflict marker Version control conflict marker Type of arg 1 to Test::Exception::throws_ok must be block or sub {} (not reference constructor) Type of arg 1 to Test::Exception::throws_ok must be block or sub {} (not reference constructor) Type of arg 1 to Test::Exception::throws_ok must be block or sub {} (not reference constructor) Version control conflict marker t/db_dependent/Koha/Checkouts/ReturnClaim.t had compilation errors.
Created attachment 130460 [details] [review] [21.05.x] Bug 29495: Unit Tests Test plan 1. Run updated tests prior to applying new patches.. pass 2. Run updated tests after applying new patches.. pass Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130461 [details] [review] [21.05.x] Bug 29495: Drop issue_id constraint from return_claims This patch removes the issue_id constraint from return_claims. Due to the nature of our dual table approach to checkouts/old_checkouts we can't safely hae this constraint and not lose data. Prior to this commit, when an item is checked in we move the checkout from checkouts to old_checkouts.. this therefore triggers the delete of the issue_id from the return_claims table as described by the foreign key constraint. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130462 [details] [review] [21.05.x] Bug 29495: DBIC Schema Rebuild Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130463 [details] [review] [21.05.x] Bug 29495: Add DBIC relationships back into Schema This patch adds the relationship accessors back into the affected Schema classes, now below the fold so they are retained during dbic rebuilds. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130464 [details] [review] [21.05.x] Bug 29495: Add code level constraint replacement This patch adds a check in Koha::Checkouts::ReturnClaim::store to replace the database level foreign key check. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130465 [details] [review] [21.05.x] Bug 29495: Update relationship methods This patch updates the relationship methods found in Koha::Checkouts::ReturnClaim so that they are prefetchable and embeddable on the API. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130466 [details] [review] [21.05.x] Bug 29495: (follow-up) Use 'item' relationship This patch adds the missing 'item' relationship in Checkouts::ReturnClaim and then uses it from the resolve method. This improve the reliability of the resolution code so it works when the item has already been checked in (without having to check Old::Checkouts). Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130467 [details] [review] [21.05.x] Bug 29495: Fix TestBuilder.t Value not allowed for auto_incr issue_id in Issue at /kohadevbox/koha/t/lib/TestBuilder.pm line 387. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Thanks for the 21.05 patches, Tomas! You did not make a 21.05 version of the "Add relationship tests" patch. The merge errors on that one seemed pretty simple to clear, so I'm not sure what's the source of the qa error I'm getting now (with all of your 21.05 patches and the regular version of Add Relationship Tests): root@kohadevbox:koha(rmain2105)$ qa -c 9 -v 2 testing 9 commit(s) (applied to 774e847 '94 Bug 29804: Fix Koha::Hold->is_pick') Processing files before patches |========================>| 7 / 7 (100.00%) Processing files after patches |========================>| 7 / 7 (100.00%) OK Koha/Checkouts/ReturnClaim.pm OK Koha/Schema/Result/Issue.pm OK Koha/Schema/Result/OldIssue.pm OK Koha/Schema/Result/ReturnClaim.pm OK installer/data/mysql/kohastructure.sql FAIL t/db_dependent/Koha/Checkouts/ReturnClaim.t FAIL valid Bareword "dt_from_string" not allowed while "strict subs" in use t/db_dependent/Koha/Checkouts/ReturnClaim.t had compilation errors. OK t/lib/TestBuilder.pm
Created attachment 130752 [details] [review] [21.05.x] Bug 29495: Unit Tests Test plan 1. Run updated tests prior to applying new patches.. pass 2. Run updated tests after applying new patches.. pass Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130753 [details] [review] [21.05.x] Bug 29495: Drop issue_id constraint from return_claims This patch removes the issue_id constraint from return_claims. Due to the nature of our dual table approach to checkouts/old_checkouts we can't safely hae this constraint and not lose data. Prior to this commit, when an item is checked in we move the checkout from checkouts to old_checkouts.. this therefore triggers the delete of the issue_id from the return_claims table as described by the foreign key constraint. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130754 [details] [review] [21.05.x] Bug 29495: DBIC Schema Rebuild Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130755 [details] [review] [21.05.x] Bug 29495: Add DBIC relationships back into Schema This patch adds the relationship accessors back into the affected Schema classes, now below the fold so they are retained during dbic rebuilds. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130756 [details] [review] [21.05.x] Bug 29495: Add code level constraint replacement This patch adds a check in Koha::Checkouts::ReturnClaim::store to replace the database level foreign key check. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130757 [details] [review] [21.05.x] Bug 29495: Update relationship methods This patch updates the relationship methods found in Koha::Checkouts::ReturnClaim so that they are prefetchable and embeddable on the API. Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 130758 [details] [review] [21.05.x] Bug 29495: Fix TestBuilder.t Value not allowed for auto_incr issue_id in Issue at /kohadevbox/koha/t/lib/TestBuilder.pm line 387. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
(In reply to Andrew Fuerste-Henry from comment #57) > Thanks for the 21.05 patches, Tomas! You did not make a 21.05 version of the > "Add relationship tests" patch. The merge errors on that one seemed pretty > simple to clear, so I'm not sure what's the source of the qa error I'm > getting now (with all of your 21.05 patches and the regular version of Add > Relationship Tests): Hi, the item relationship is not particularly used by this bug. It is introduced as an enhancement follow-up that could've been on its own bug. So I don't include it. This new patchset has the relationship removed from the schema as well. Please only include the [21.05.x] patches, and retry. I got no failures locally.
Pushed to 21.05.x for 21.05.11
> 1. Run updated tests prior to applying new patches.. pass > 2. Run updated tests after applying new patches.. pass I suppose that it shouldn't pass prior to applying new patches. (1.) At least that's what I have on 20.11.x
Backported: Pushed to 20.11.x branch for 20.11.15
Does this need to be backported to 19.11.x?
Whilst 19.11 probably falls fowl of this bug, I don't think it warrants a backport.
Not backported to 19.11.x