Bug 29495 - Issue link is lost in return claims when using 'MarkLostItemsAsReturned'
Summary: Issue link is lost in return claims when using 'MarkLostItemsAsReturned'
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on:
Blocks: 29519 31110
  Show dependency treegraph
 
Reported: 2021-11-16 14:08 UTC by Martin Renvoize
Modified: 2022-07-06 14:56 UTC (History)
10 users (show)

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


Attachments
Bug 29495: Drop issue_id constraint from return_claims (2.37 KB, patch)
2021-11-17 10:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: DBIC Schema Rebuild (2.80 KB, patch)
2021-11-17 10:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: Add DBIC relationships back into Schema (2.79 KB, patch)
2021-11-17 10:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: Add code level constraint replacement (1.06 KB, patch)
2021-11-17 10:29 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: Drop issue_id constraint from return_claims (2.46 KB, patch)
2021-11-18 07:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: DBIC Schema Rebuild (2.80 KB, patch)
2021-11-18 07:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: Add DBIC relationships back into Schema (2.78 KB, patch)
2021-11-18 07:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: Add code level constraint replacement (1.06 KB, patch)
2021-11-18 07:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: Update relationship methods (2.36 KB, patch)
2021-11-18 07:45 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: Drop issue_id constraint from return_claims (2.53 KB, patch)
2021-11-18 16:00 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29495: DBIC Schema Rebuild (2.87 KB, patch)
2021-11-18 16:00 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29495: Add DBIC relationships back into Schema (2.85 KB, patch)
2021-11-18 16:00 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29495: Add code level constraint replacement (1.13 KB, patch)
2021-11-18 16:00 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29495: Update relationship methods (2.42 KB, patch)
2021-11-18 16:00 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 29495: Unit Tests (2.84 KB, patch)
2021-11-22 18:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: Drop issue_id constraint from return_claims (2.53 KB, patch)
2021-11-22 18:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: DBIC Schema Rebuild (2.87 KB, patch)
2021-11-22 18:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: Add DBIC relationships back into Schema (2.85 KB, patch)
2021-11-22 18:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: Add code level constraint replacement (1.13 KB, patch)
2021-11-22 18:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: Update relationship methods (2.43 KB, patch)
2021-11-22 18:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: (follow-up) Use 'item' relationship (2.09 KB, patch)
2021-11-22 18:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: Add relation tests (7.66 KB, patch)
2021-11-22 18:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: Unit Tests (2.90 KB, patch)
2022-01-06 18:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29495: Drop issue_id constraint from return_claims (2.58 KB, patch)
2022-01-06 18:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29495: DBIC Schema Rebuild (2.92 KB, patch)
2022-01-06 18:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29495: Add DBIC relationships back into Schema (2.90 KB, patch)
2022-01-06 18:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29495: Add code level constraint replacement (1.18 KB, patch)
2022-01-06 18:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29495: Update relationship methods (2.39 KB, patch)
2022-01-06 18:19 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29495: (follow-up) Use 'item' relationship (2.14 KB, patch)
2022-01-06 18:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29495: Add relation tests (7.71 KB, patch)
2022-01-06 18:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29495: (follow-up) Fix Tests (2.23 KB, patch)
2022-01-18 11:41 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29495: Fix TestBuilder.t (895 bytes, patch)
2022-01-18 15:01 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29495: Fix TestBuilder.t (959 bytes, patch)
2022-01-18 16:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
[21.05.x] Bug 29495: Unit Tests (2.91 KB, patch)
2022-02-10 18:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[21.05.x] Bug 29495: Drop issue_id constraint from return_claims (2.55 KB, patch)
2022-02-10 18:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[21.05.x] Bug 29495: DBIC Schema Rebuild (2.93 KB, patch)
2022-02-10 18:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[21.05.x] Bug 29495: Add DBIC relationships back into Schema (2.91 KB, patch)
2022-02-10 18:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[21.05.x] Bug 29495: Add code level constraint replacement (1.19 KB, patch)
2022-02-10 18:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[21.05.x] Bug 29495: Update relationship methods (2.40 KB, patch)
2022-02-10 18:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[21.05.x] Bug 29495: (follow-up) Use 'item' relationship (1.24 KB, patch)
2022-02-10 18:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[21.05.x] Bug 29495: Fix TestBuilder.t (969 bytes, patch)
2022-02-10 18:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[21.05.x] Bug 29495: Unit Tests (2.91 KB, patch)
2022-02-17 15:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[21.05.x] Bug 29495: Drop issue_id constraint from return_claims (2.55 KB, patch)
2022-02-17 15:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[21.05.x] Bug 29495: DBIC Schema Rebuild (2.93 KB, patch)
2022-02-17 15:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[21.05.x] Bug 29495: Add DBIC relationships back into Schema (2.91 KB, patch)
2022-02-17 15:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[21.05.x] Bug 29495: Add code level constraint replacement (1.19 KB, patch)
2022-02-17 15:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[21.05.x] Bug 29495: Update relationship methods (2.40 KB, patch)
2022-02-17 15:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[21.05.x] Bug 29495: Fix TestBuilder.t (969 bytes, patch)
2022-02-17 15:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2021-11-16 14:08:04 UTC
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 :(
Comment 1 Martin Renvoize 2021-11-17 10:29:25 UTC
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.
Comment 2 Martin Renvoize 2021-11-17 10:29:28 UTC
Created attachment 127712 [details] [review]
Bug 29495: DBIC Schema Rebuild
Comment 3 Martin Renvoize 2021-11-17 10:29:32 UTC
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.
Comment 4 Martin Renvoize 2021-11-17 10:29:35 UTC
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.
Comment 5 Kyle M Hall 2021-11-17 14:12:53 UTC
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.
Comment 6 Martin Renvoize 2021-11-18 07:45:06 UTC
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.
Comment 7 Martin Renvoize 2021-11-18 07:45:10 UTC
Created attachment 127766 [details] [review]
Bug 29495: DBIC Schema Rebuild
Comment 8 Martin Renvoize 2021-11-18 07:45:13 UTC
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.
Comment 9 Martin Renvoize 2021-11-18 07:45:17 UTC
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.
Comment 10 Martin Renvoize 2021-11-18 07:45:21 UTC
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.
Comment 11 Martin Renvoize 2021-11-18 07:46:15 UTC
Thanks for the review Kyle.. I've implemented the requested improvements now :)
Comment 12 Andrew Fuerste-Henry 2021-11-18 16:00:35 UTC
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>
Comment 13 Andrew Fuerste-Henry 2021-11-18 16:00:39 UTC
Created attachment 127810 [details] [review]
Bug 29495: DBIC Schema Rebuild

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 14 Andrew Fuerste-Henry 2021-11-18 16:00:43 UTC
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>
Comment 15 Andrew Fuerste-Henry 2021-11-18 16:00:47 UTC
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>
Comment 16 Andrew Fuerste-Henry 2021-11-18 16:00:51 UTC
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>
Comment 17 Martin Renvoize 2021-11-22 18:00:31 UTC
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
Comment 18 Martin Renvoize 2021-11-22 18:00:35 UTC
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>
Comment 19 Martin Renvoize 2021-11-22 18:00:38 UTC
Created attachment 127939 [details] [review]
Bug 29495: DBIC Schema Rebuild

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 20 Martin Renvoize 2021-11-22 18:00:42 UTC
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>
Comment 21 Martin Renvoize 2021-11-22 18:00:46 UTC
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>
Comment 22 Martin Renvoize 2021-11-22 18:00:50 UTC
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>
Comment 23 Martin Renvoize 2021-11-22 18:00:53 UTC
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).
Comment 24 Martin Renvoize 2021-11-22 18:00:57 UTC
Created attachment 127944 [details] [review]
Bug 29495: Add relation tests

This patch adds missing tests for relationship accessors in the
ReturnClaim class.
Comment 25 Martin Renvoize 2021-11-22 18:01:25 UTC
Added missing unit tests.. we're well covered now.. over to QA
Comment 26 Jonathan Druart 2021-12-01 15:04:16 UTC
(Wouldn't it be better to put effort in merging the tables?)
Comment 27 Katrin Fischer 2021-12-19 14:21:02 UTC
(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.
Comment 28 Tomás Cohen Arazi 2022-01-06 18:19:07 UTC
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>
Comment 29 Tomás Cohen Arazi 2022-01-06 18:19:15 UTC
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>
Comment 30 Tomás Cohen Arazi 2022-01-06 18:19:26 UTC
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>
Comment 31 Tomás Cohen Arazi 2022-01-06 18:19:35 UTC
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>
Comment 32 Tomás Cohen Arazi 2022-01-06 18:19:47 UTC
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>
Comment 33 Tomás Cohen Arazi 2022-01-06 18:19:59 UTC
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>
Comment 34 Tomás Cohen Arazi 2022-01-06 18:20:12 UTC
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>
Comment 35 Tomás Cohen Arazi 2022-01-06 18:20:35 UTC
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>
Comment 36 Fridolin Somers 2022-01-18 00:56:24 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 37 Fridolin Somers 2022-01-18 06:07:04 UTC
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 ?
Comment 38 Martin Renvoize 2022-01-18 11:41:36 UTC
Created attachment 129547 [details] [review]
Bug 29495: (follow-up) Fix Tests

Update DBIC relation types to prevent double issue creation during
object build.
Comment 39 Jonathan Druart 2022-01-18 12:04:44 UTC
(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.
Comment 40 Jonathan Druart 2022-01-18 15:01:49 UTC
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.
Comment 41 Jonathan Druart 2022-01-18 15:02:50 UTC
Martin, this patch fixes the failure and don't modify the relation. It seems better to me. What do you think?
Comment 42 Martin Renvoize 2022-01-18 15:43:54 UTC
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
Comment 43 Martin Renvoize 2022-01-18 16:25:25 UTC
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>
Comment 44 Fridolin Somers 2022-01-18 19:15:46 UTC
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.
Comment 45 Jonathan Druart 2022-01-19 08:33:40 UTC
(In reply to Fridolin Somers from comment #44)
> Tell me if can be pushed.

Yes, it's ready for push.
Comment 46 Fridolin Somers 2022-01-19 09:04:35 UTC
Pushed master :
  Bug 29495: Fix TestBuilder.t
Comment 47 Kyle M Hall 2022-02-04 18:32:18 UTC
Pushed to 21.11.x for 21.11.03
Comment 48 Andrew Fuerste-Henry 2022-02-10 16:44:33 UTC
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.
Comment 49 Tomás Cohen Arazi 2022-02-10 18:57:13 UTC
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>
Comment 50 Tomás Cohen Arazi 2022-02-10 18:57:17 UTC
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>
Comment 51 Tomás Cohen Arazi 2022-02-10 18:57:22 UTC
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>
Comment 52 Tomás Cohen Arazi 2022-02-10 18:57:28 UTC
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>
Comment 53 Tomás Cohen Arazi 2022-02-10 18:57:33 UTC
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>
Comment 54 Tomás Cohen Arazi 2022-02-10 18:57:37 UTC
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>
Comment 55 Tomás Cohen Arazi 2022-02-10 18:57:43 UTC
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>
Comment 56 Tomás Cohen Arazi 2022-02-10 18:57:48 UTC
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>
Comment 57 Andrew Fuerste-Henry 2022-02-11 12:24:50 UTC
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
Comment 58 Tomás Cohen Arazi 2022-02-17 15:23:01 UTC
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>
Comment 59 Tomás Cohen Arazi 2022-02-17 15:23:08 UTC
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>
Comment 60 Tomás Cohen Arazi 2022-02-17 15:23:13 UTC
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>
Comment 61 Tomás Cohen Arazi 2022-02-17 15:23:19 UTC
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>
Comment 62 Tomás Cohen Arazi 2022-02-17 15:23:25 UTC
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>
Comment 63 Tomás Cohen Arazi 2022-02-17 15:23:31 UTC
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>
Comment 64 Tomás Cohen Arazi 2022-02-17 15:23:37 UTC
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>
Comment 65 Tomás Cohen Arazi 2022-02-17 16:53:21 UTC
(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.
Comment 66 Andrew Fuerste-Henry 2022-02-17 20:01:39 UTC
Pushed to 21.05.x for 21.05.11
Comment 67 Victor Grousset/tuxayo 2022-02-21 09:27:12 UTC
> 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
Comment 68 Victor Grousset/tuxayo 2022-02-23 21:59:03 UTC
Backported: Pushed to 20.11.x branch for 20.11.15
Comment 69 wainuiwitikapark 2022-02-24 03:56:01 UTC
Does this need to be backported to 19.11.x?
Comment 70 Martin Renvoize 2022-02-24 10:19:35 UTC
Whilst 19.11 probably falls fowl of this bug, I don't think it warrants a backport.
Comment 71 wainuiwitikapark 2022-02-24 21:35:48 UTC
Not backported to 19.11.x