Bug 34666

Summary: _Findgroupreserve is not returning title level matches from the queue for holds with no item group
Product: Koha Reporter: Nick Clemens (kidclamp) <nick>
Component: Hold requestsAssignee: Nick Clemens (kidclamp) <nick>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, kyle, m.de.rooy, tomascohen
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=28205
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.11.00,23.05.04,22.11.10
Circulation function:
Bug Depends on: 24860    
Bug Blocks:    
Attachments: Bug 34666: Unit test
Bug 34666: Allow item_group to be null and still match
Bug 34666: Add a second unit test for item level
Bug 34666: Combine queries in _Findgroupreserve
Bug 34666: Unit test
Bug 34666: Allow item_group to be null and still match
Bug 34666: Add a second unit test for item level
Bug 34666: Combine queries in _Findgroupreserve
Bug 34666: Tidy
Bug 34666: Unit test
Bug 34666: Allow item_group to be null and still match
Bug 34666: Add a second unit test for item level
Bug 34666: Combine queries in _Findgroupreserve
Bug 34666: Tidy

Description Nick Clemens (kidclamp) 2023-08-30 19:02:36 UTC
The check in _Findgroupreserve is eliminating matches with NULL group id's
Comment 1 Nick Clemens (kidclamp) 2023-08-30 19:04:44 UTC
Created attachment 154978 [details] [review]
Bug 34666: Unit test
Comment 2 Nick Clemens (kidclamp) 2023-08-30 19:04:46 UTC
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!
Comment 3 Nick Clemens (kidclamp) 2023-08-30 19:04:48 UTC
Created attachment 154980 [details] [review]
Bug 34666: Add a second unit test for item level
Comment 4 Nick Clemens (kidclamp) 2023-08-30 19:04:50 UTC
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
Comment 5 David Nind 2023-08-30 19:34:11 UTC
Created attachment 154983 [details] [review]
Bug 34666: Unit test

Signed-off-by: David Nind <david@davidnind.com>
Comment 6 David Nind 2023-08-30 19:34:14 UTC
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>
Comment 7 David Nind 2023-08-30 19:34:16 UTC
Created attachment 154985 [details] [review]
Bug 34666: Add a second unit test for item level

Signed-off-by: David Nind <david@davidnind.com>
Comment 8 David Nind 2023-08-30 19:34:19 UTC
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>
Comment 9 Marcel de Rooy 2023-09-01 08:00:56 UTC
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)
Comment 10 Nick Clemens (kidclamp) 2023-09-01 11:02:28 UTC
(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
Comment 11 Marcel de Rooy 2023-09-04 09:44:13 UTC
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
Comment 12 Nick Clemens (kidclamp) 2023-09-05 11:37:25 UTC
(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?
Comment 13 Marcel de Rooy 2023-09-05 11:40:41 UTC
(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.
Comment 14 Nick Clemens (kidclamp) 2023-09-05 13:52:23 UTC
(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
Comment 15 Marcel de Rooy 2023-09-05 14:31:51 UTC
(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
Comment 16 Nick Clemens (kidclamp) 2023-09-05 14:40:48 UTC
Created attachment 155221 [details] [review]
Bug 34666: Tidy
Comment 17 Nick Clemens (kidclamp) 2023-09-05 14:41:44 UTC
(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
Comment 18 Marcel de Rooy 2023-09-06 12:08:55 UTC
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>
Comment 19 Marcel de Rooy 2023-09-06 12:08:57 UTC
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>
Comment 20 Marcel de Rooy 2023-09-06 12:09:00 UTC
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>
Comment 21 Marcel de Rooy 2023-09-06 12:09:03 UTC
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>
Comment 22 Marcel de Rooy 2023-09-06 12:09:05 UTC
Created attachment 155270 [details] [review]
Bug 34666: Tidy

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Jonathan Druart 2023-09-06 13:06:29 UTC
You must use the version of perltidy shipped in ktd.
Comment 24 Jonathan Druart 2023-09-06 13:08:59 UTC
(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.
Comment 25 Marcel de Rooy 2023-09-06 13:38:32 UTC
We used the version here that Mason mentioned on that report. So KTD is using the old version and needs attention.
Comment 26 Tomás Cohen Arazi (tcohen) 2023-09-08 14:54:35 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 27 Fridolin Somers 2023-09-08 19:44:33 UTC
Looks like it depends on Bug 24860
Comment 28 Fridolin Somers 2023-09-08 19:45:10 UTC
Pushed to 23.05.x for 23.05.04
Comment 29 Matt Blenkinsop 2023-09-15 10:08:55 UTC
Nice work everyone!

Pushed to oldstable for 22.11.x