The check in _Findgroupreserve is eliminating matches with NULL group id's
Created attachment 154978 [details] [review] Bug 34666: Unit test
Created attachment 154979 [details] [review] Bug 34666: Allow item_group to be null and still match The current logic requires that the grop ids match, but this eliminates null matches from the group. The fallout essentially is that the queue won't be checked to fill holds in cases of title level matches where holds don't have an item group id. The queue checks the transport cost matrix while the check reserves check does not, so this may have an impact on holds costs and delivery times To test: 0 - Apply unit test patch 1 - prove -v t/db_dependent/Reserves.t 2 - It fails 3 - Apply this patch 4 - prove -v t/db_dependent/Reserves.t 5 - It passes!
Created attachment 154980 [details] [review] Bug 34666: Add a second unit test for item level
Created attachment 154981 [details] [review] Bug 34666: Combine queries in _Findgroupreserve The queries here are the same except for 2 differences: 1 - They check if the hold was on a particular item 2 - The latter confirms that the reserve item group matches the item's item group For 1, it doesn't matter - only 1 item can be mapped ot a reserve, itemnumber is the primary key for hold_fill_targets - so we are either matching it in the first query or the second, either way we get the same reserve - the returns are the same so we don't care which query it came from For 2, this has already been checked when the queue was built. We don't need to verify the match because it wouldn't be in the targets if they didn't match To test: 1 - Apply second unit test patch 2 - prove t/db_dependent/Reserves.t 3 - It should pass 4 - Apply this patch 5 - prove t/db_dependent/Reserves.t 6 - It continues to pass
Created attachment 154983 [details] [review] Bug 34666: Unit test Signed-off-by: David Nind <david@davidnind.com>
Created attachment 154984 [details] [review] Bug 34666: Allow item_group to be null and still match The current logic requires that the grop ids match, but this eliminates null matches from the group. The fallout essentially is that the queue won't be checked to fill holds in cases of title level matches where holds don't have an item group id. The queue checks the transport cost matrix while the check reserves check does not, so this may have an impact on holds costs and delivery times To test: 0 - Apply unit test patch 1 - prove -v t/db_dependent/Reserves.t 2 - It fails 3 - Apply this patch 4 - prove -v t/db_dependent/Reserves.t 5 - It passes! Signed-off-by: David Nind <david@davidnind.com>
Created attachment 154985 [details] [review] Bug 34666: Add a second unit test for item level Signed-off-by: David Nind <david@davidnind.com>
Created attachment 154986 [details] [review] Bug 34666: Combine queries in _Findgroupreserve The queries here are the same except for 2 differences: 1 - They check if the hold was on a particular item 2 - The latter confirms that the reserve item group matches the item's item group For 1, it doesn't matter - only 1 item can be mapped ot a reserve, itemnumber is the primary key for hold_fill_targets - so we are either matching it in the first query or the second, either way we get the same reserve - the returns are the same so we don't care which query it came from For 2, this has already been checked when the queue was built. We don't need to verify the match because it wouldn't be in the targets if they didn't match To test: 1 - Apply second unit test patch 2 - prove t/db_dependent/Reserves.t 3 - It should pass 4 - Apply this patch 5 - prove t/db_dependent/Reserves.t 6 - It continues to pass Signed-off-by: David Nind <david@davidnind.com>
Sorry, but I didnt invent it but spent hours already on tidying myself: WARN t/db_dependent/Reserves.t WARN tidiness The file is less tidy than before (bad/messy lines before: 510, now: 515)
(In reply to Marcel de Rooy from comment #9) > Sorry, but I didnt invent it but spent hours already on tidying myself: > > WARN t/db_dependent/Reserves.t > WARN tidiness > The file is less tidy than before (bad/messy lines before: > 510, now: 515) I don't recreate - applied to latest master, pulled latest KTD
Please fetch latest qa tools: WARN t/db_dependent/Reserves.t OK critic OK forbidden patterns OK git manipulation OK pod SKIP spelling WARN tidiness The file is less tidy than before (bad/messy lines before: 510, now: 515) OK valid
(In reply to Marcel de Rooy from comment #11) > Please fetch latest qa tools: > > WARN t/db_dependent/Reserves.t > OK critic > OK forbidden patterns > OK git manipulation > OK pod > SKIP spelling > WARN tidiness > The file is less tidy than before (bad/messy lines before: > 510, now: 515) > OK valid Fresh KTD, pulled - qa-tools pulled - I get tidiness warnings (often) but not for these patches. Do you have other patches applied?
(In reply to Nick Clemens from comment #12) > (In reply to Marcel de Rooy from comment #11) > > Please fetch latest qa tools: > > > > WARN t/db_dependent/Reserves.t > > OK critic > > OK forbidden patterns > > OK git manipulation > > OK pod > > SKIP spelling > > WARN tidiness > > The file is less tidy than before (bad/messy lines before: > > 510, now: 515) > > OK valid > > Fresh KTD, pulled - qa-tools pulled - I get tidiness warnings (often) but > not for these patches. Do you have other patches applied? Interesting. I remember a discussion earlier if there is a difference between the Koha git repo perltidyrc and the one used in KTD. Could you compare them? I am just using the git repo perltidyrc.
(In reply to Marcel de Rooy from comment #13) > (In reply to Nick Clemens from comment #12) > > (In reply to Marcel de Rooy from comment #11) > > > Please fetch latest qa tools: > > > > > > WARN t/db_dependent/Reserves.t > > > OK critic > > > OK forbidden patterns > > > OK git manipulation > > > OK pod > > > SKIP spelling > > > WARN tidiness > > > The file is less tidy than before (bad/messy lines before: > > > 510, now: 515) > > > OK valid > > > > Fresh KTD, pulled - qa-tools pulled - I get tidiness warnings (often) but > > not for these patches. Do you have other patches applied? > > Interesting. I remember a discussion earlier if there is a difference > between the Koha git repo perltidyrc and the one used in KTD. Could you > compare them? I am just using the git repo perltidyrc. I only find the one file
(In reply to Nick Clemens from comment #14) > (In reply to Marcel de Rooy from comment #13) > > (In reply to Nick Clemens from comment #12) > > > (In reply to Marcel de Rooy from comment #11) > > > > Please fetch latest qa tools: > > > > > > > > WARN t/db_dependent/Reserves.t > > > > OK critic > > > > OK forbidden patterns > > > > OK git manipulation > > > > OK pod > > > > SKIP spelling > > > > WARN tidiness > > > > The file is less tidy than before (bad/messy lines before: > > > > 510, now: 515) > > > > OK valid > > > > > > Fresh KTD, pulled - qa-tools pulled - I get tidiness warnings (often) but > > > not for these patches. Do you have other patches applied? > > > > Interesting. I remember a discussion earlier if there is a difference > > between the Koha git repo perltidyrc and the one used in KTD. Could you > > compare them? I am just using the git repo perltidyrc. > > I only find the one file Tomas: In view of above, could you confirm that KTD uses the perltidyrc from the root directory of the Koha git repo ? cat .perltidyrc # delete backup if no errors --backup-file-extension='/' --character-encoding=utf8 --output-line-ending=unix --iterations=2 # } else { --cuddled-else # Use the full indent size for continuations --continuation-indentation=4 --no-outdent-long-lines --maximum-line-length=120 --break-at-old-comma-breakpoints And which version, Nick? root@master:/usr/share/koha# perltidy -v This is perltidy, v20230309
Created attachment 155221 [details] [review] Bug 34666: Tidy
(In reply to Marcel de Rooy from comment #15) > And which version, Nick? > root@master:/usr/share/koha# perltidy -v > This is perltidy, v20230309 kohadev-koha@kohadevbox:koha(master)$ perltidy -v This is perltidy, v20200110 Updated perltidy, got the error, patch attached
Created attachment 155266 [details] [review] Bug 34666: Unit test Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 155267 [details] [review] Bug 34666: Allow item_group to be null and still match The current logic requires that the grop ids match, but this eliminates null matches from the group. The fallout essentially is that the queue won't be checked to fill holds in cases of title level matches where holds don't have an item group id. The queue checks the transport cost matrix while the check reserves check does not, so this may have an impact on holds costs and delivery times To test: 0 - Apply unit test patch 1 - prove -v t/db_dependent/Reserves.t 2 - It fails 3 - Apply this patch 4 - prove -v t/db_dependent/Reserves.t 5 - It passes! Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 155268 [details] [review] Bug 34666: Add a second unit test for item level Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 155269 [details] [review] Bug 34666: Combine queries in _Findgroupreserve The queries here are the same except for 2 differences: 1 - They check if the hold was on a particular item 2 - The latter confirms that the reserve item group matches the item's item group For 1, it doesn't matter - only 1 item can be mapped ot a reserve, itemnumber is the primary key for hold_fill_targets - so we are either matching it in the first query or the second, either way we get the same reserve - the returns are the same so we don't care which query it came from For 2, this has already been checked when the queue was built. We don't need to verify the match because it wouldn't be in the targets if they didn't match To test: 1 - Apply second unit test patch 2 - prove t/db_dependent/Reserves.t 3 - It should pass 4 - Apply this patch 5 - prove t/db_dependent/Reserves.t 6 - It continues to pass Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 155270 [details] [review] Bug 34666: Tidy Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
You must use the version of perltidy shipped in ktd.
(In reply to Jonathan Druart from comment #23) > You must use the version of perltidy shipped in ktd. And, if it's too old and need to be upgraded, see bug 30002. Looks like we have 20230309-1~koha1 but it's not the version installed in ktd.
We used the version here that Mason mentioned on that report. So KTD is using the old version and needs attention.
Pushed to master for 23.11. Nice work everyone, thanks!
Looks like it depends on Bug 24860
Pushed to 23.05.x for 23.05.04
Nice work everyone! Pushed to oldstable for 22.11.x