As exposed with the introduction of Koha::Checkouts::Renewal(s), which has two relationships (checkout and old_checkout) defined at the DBIC level (and not at DB level) that point the same column name, to the same fk name, TestBuilder fails in some situations. Looking at the code I found there's some code for detecting duplicate fk introduced by bug 16155, but it doesn't seem to be effective: it fails randomly depending on the key that is picked. I will submit my attempt to make things work, but more eyes are needed.
Created attachment 137599 [details] [review] Bug 31133: Regression tests (1) Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 137600 [details] [review] Bug 31133: Add a way to handle multiple relationships on same column This patch introduces a syntax for TestBuilder so we can specify which is the preferred relationship in situations like Koha::Checkouts::Renewals that have more than one virtual FK [1] for the same column. In this case, there are the following relationships: * checkouts * old_checkouts both of which link the *checkout_id* column to the *issue_id* column of the **issues** and **old_issues** tables respectively. It does so by defining a mapping between the (concatenated, maybe multiple) related source column and foreign counterpart. The mapping structure is explained in the POD. To test: 1. Apply the regression tests 2. Run the tests a couple times: $ kshell k$ prove t/db_dependent/TestBuilder_multi_vfk.t => FAIL: Tests fail often, randomly! 3. Apply this patch 4. Repeat 2 => SUCCESS: Tests pass! No random failures! 5. Bonus, run: $ kshell k$ prove t/db_dependent/TestBuilder.t => FAIL: It fails consistently about Koha::Biblio::ItemGroup [1] i.e. defined at DBIC level but not present on the DB structure. DISCLAIMER: This is not a complete solution as it still requires a way to override the hardcoded default. This would require a special syntax for build() and build_object() which I prefer to delay until we really need it. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Ok, this should cover one part of the problem. There's still Koha::Biblio::ItemGroup which is failing, ran out of fuel today/yesterday, late already :-D
Did you notice that "Bug 30275: (QA follow-up) Trivial fix to schema file" fixes this problem?
(In reply to Jonathan Druart from comment #4) > Did you notice that "Bug 30275: (QA follow-up) Trivial fix to schema file" > fixes this problem? Hum no, it did not!
Created attachment 137603 [details] [review] [ALTERNATIVE-PATCH] Bug 31133: Don't generate values for more than one FK
What about this alternative, Tomas? At least we won't have more code to add for other specific cases.
That's pretty elegant and simple.. I like
Created attachment 137604 [details] [review] [ALTERNATIVE-PATCH] Bug 31133: Don't generate values for more than one FK
It was not working, randomly failing! The second patch should be correct.
Note that I don't get any other failures, t/db_dependent/TestBuilder.t is passing.
Note that I don't get any other failures, t/db_dependent/TestBuilder.t is passing.(In reply to Jonathan Druart from comment #11) > Note that I don't get any other failures, t/db_dependent/TestBuilder.t is > passing. Wrong! Hard morning... Can't use string ("Koha::Biblio::ItemGroup") as a HASH ref while "strict refs" in use at /kohadevbox/koha/Koha/Object.pm line 830.
(In reply to Tomás Cohen Arazi from comment #3) > Ok, this should cover one part of the problem. There's still > Koha::Biblio::ItemGroup which is failing, ran out of fuel today/yesterday, > late already :-D I've attached a patch on bug 24857 for that: "Bug 24857: Remove object_class from singular classes".
My first implementation was similar to Jonathan's. The 'problem' was that it randomly generated old_issues and it felt like a very rare use case. It felt like it was better to just have a default resolution path. I'm fine with either.
Created attachment 137634 [details] [review] Bug 31133: Don't generate values for more than one FK Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 22.11. Nice work everyone, thanks!