Bug 35840 - Local use is double-counted when using both RecordLocalUseOnReturn and statistical patrons
Summary: Local use is double-counted when using both RecordLocalUseOnReturn and statis...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Lucas Gass
QA Contact: Martin Renvoize
URL:
Keywords:
: 35839 (view as bug list)
Depends on: 27992
Blocks: 16122 35950
  Show dependency treegraph
 
Reported: 2024-01-18 22:38 UTC by Andrew Fuerste-Henry
Modified: 2024-03-19 21:14 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.04


Attachments
Bug 35840: Only AddReturn if item is onloan (1.43 KB, patch)
2024-01-19 15:31 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 35840: Only AddReturn if item is onloan (1.49 KB, patch)
2024-01-19 16:08 UTC, Biblibre Sandboxes
Details | Diff | Splinter Review
Bug 35840: Fix Local use being double-counted in CanBookBeIssued (2.14 KB, patch)
2024-01-19 20:39 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 35840: (QA follow-up) Make sure return and localuse is recorded and unit tests (3.49 KB, patch)
2024-01-25 00:45 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 35840: Fix Local use being double-counted in CanBookBeIssued (2.20 KB, patch)
2024-01-25 16:07 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 35840: (QA follow-up) Make sure return and localuse is recorded and unit tests (3.54 KB, patch)
2024-01-25 16:07 UTC, Brendan Lawlor
Details | Diff | Splinter Review
Bug 35840: Add option to AddReturn to skip recording of localuse (2.77 KB, patch)
2024-01-29 18:52 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 35840: Add option to AddReturn to skip recording of localuse (2.79 KB, patch)
2024-01-29 19:20 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 35840: Add unit tests (1.89 KB, patch)
2024-01-29 19:42 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 35840: Add option to AddReturn to skip recording of localuse (2.85 KB, patch)
2024-02-02 21:27 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 35840: Add unit tests (1.95 KB, patch)
2024-02-02 21:27 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 35840: Add option to AddReturn to skip recording of localuse (2.92 KB, patch)
2024-03-08 14:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35840: Add unit tests (2.01 KB, patch)
2024-03-08 14:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35840: (QA follow-up) Tidy and critic fix (4.65 KB, patch)
2024-03-08 14:42 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2024-01-18 22:38:35 UTC
To recreate:
- enable RecordLocalUseOnReturn
- create a statistical patron category and a patron in it
- check an item out to your statistical patron
- count local use entries with a report: SELECT * FROM statistics WHERE type='localuse' AND itemnumber= (your item)
- see that your one checkout has created 2 entries in statistics

It seems plausible that a single library might want to use these two features concurrently to count different things, but doing so creates an inflated count.
Comment 1 Magnus Enger 2024-01-19 06:52:55 UTC
*** Bug 35839 has been marked as a duplicate of this bug. ***
Comment 2 Lucas Gass 2024-01-19 15:13:46 UTC
This is due to Bug 27992, which adds a call to AddReturn
Comment 3 Lucas Gass 2024-01-19 15:31:46 UTC
Created attachment 161212 [details] [review]
Bug 35840: Only AddReturn if item is onloan

To test:
1. APPLY PATCH, restart_all
2. Turn on RecordLocalUseOnReturn
3. Create a Statistical patron.
4. Check an item out to a regular patron.
5. Check the item out to a Statistical patron.
6. This should trigger a return and you will see 2 entries in the statistics table, one for localuse and one for a return.
7. Try checking out an item to the Stats patron that is NOT checked out.
8. You should only see 1 entry, localuse, in the statistics table.
Comment 4 Biblibre Sandboxes 2024-01-19 16:08:18 UTC
Created attachment 161217 [details] [review]
Bug 35840: Only AddReturn if item is onloan

To test:
1. APPLY PATCH, restart_all
2. Turn on RecordLocalUseOnReturn
3. Create a Statistical patron.
4. Check an item out to a regular patron.
5. Check the item out to a Statistical patron.
6. This should trigger a return and you will see 2 entries in the statistics table, one for localuse and one for a return.
7. Try checking out an item to the Stats patron that is NOT checked out.
8. You should only see 1 entry, localuse, in the statistics table.

Signed-off-by: Andrew Fuerste-Henry <andrewfh@dubcolib.org>
Comment 5 Emily Lamancusa 2024-01-19 16:45:24 UTC
Hmmm...looking at the patch from 27992, all of the checking for holds, recalls, return claims, and lost or withdrawn statuses is based on the return values from AddReturn. So avoiding the call when the item isn't checked out would undo a lot of the functionality for 27992. (I did a quick test on master and confirmed this.) Wouldn't it be better to skip logging the local use in CanBookBeIssued if RecordLocalUseOnReturn is enabled (probably with a comment in the code that it is being skipped because AddReturn will log the local use in that case)?

Either way, this is also going to need a unit test. :)
Comment 6 Lucas Gass 2024-01-19 20:39:40 UTC
Created attachment 161223 [details] [review]
Bug 35840: Fix Local use being double-counted in CanBookBeIssued

To test:
1. APPLY PATCH, restart_all
2. Turn on RecordLocalUseOnReturn
3. Create a Statistical patron.
4. Check an item out to a regular patron.
5. Check the item out to a Statistical patron.
6. This should trigger a return and you will see 2 entries in the statistics table, one for localuse and one for a return.
7. Try checking out an item to the Stats patron that is NOT checked out.
8. You should only see 1 entry, localuse, in the statistics table.
Comment 7 Lucas Gass 2024-01-19 20:40:40 UTC
(In reply to Emily Lamancusa from comment #5)
> Hmmm...looking at the patch from 27992, all of the checking for holds,
> recalls, return claims, and lost or withdrawn statuses is based on the
> return values from AddReturn. So avoiding the call when the item isn't
> checked out would undo a lot of the functionality for 27992. (I did a quick
> test on master and confirmed this.) Wouldn't it be better to skip logging
> the local use in CanBookBeIssued if RecordLocalUseOnReturn is enabled
> (probably with a comment in the code that it is being skipped because
> AddReturn will log the local use in that case)?
> 
> Either way, this is also going to need a unit test. :)

Maybe this? If you like this better I'll write a unit test. Let me know.
Comment 8 Emily Lamancusa 2024-01-19 21:24:18 UTC
Works for me! Just one tweak to line 2476:

- }) unless $localuse;
+ }) unless ( $localuse && $stat_type eq 'localuse' );

Otherwise, when there is an actual return, it skips logging that, too.
Comment 9 Lucas Gass 2024-01-19 23:06:40 UTC
(In reply to Emily Lamancusa from comment #8)
> Works for me! Just one tweak to line 2476:
> 
> - }) unless $localuse;
> + }) unless ( $localuse && $stat_type eq 'localuse' );
> 
> Otherwise, when there is an actual return, it skips logging that, too.

Are you saying it should add 2 entries to the stats table if a checked out item is checked out to the stats patron? 1 for localuse and 1 for a return? Seems like it should be one or the other, not both.
Comment 10 Lucas Gass 2024-01-19 23:13:11 UTC
> Are you saying it should add 2 entries to the stats table if a checked out
> item is checked out to the stats patron? 1 for localuse and 1 for a return?
> Seems like it should be one or the other, not both.

Im 2nd guessing now. I'm thinking that if it actually does a return it should be logged as such but NOT logged as a localuse.
Comment 11 Lucas Gass 2024-01-19 23:14:50 UTC
This one needs to go 'In discussion'
Comment 12 Lucas Gass 2024-01-22 16:08:23 UTC
(In reply to Lucas Gass from comment #10)
> > Are you saying it should add 2 entries to the stats table if a checked out
> > item is checked out to the stats patron? 1 for localuse and 1 for a return?
> > Seems like it should be one or the other, not both.
> 
> Im 2nd guessing now. I'm thinking that if it actually does a return it
> should be logged as such but NOT logged as a localuse.

In this scenario a book had been checked out to a patron but for whatever reason is left behind at the library. Staff finds it and checks it in. That should count as a return and not local use.
Comment 13 Emily Lamancusa 2024-01-22 16:09:30 UTC
(In reply to Lucas Gass from comment #9)

> Are you saying it should add 2 entries to the stats table if a checked out
> item is checked out to the stats patron? 1 for localuse and 1 for a return?
> Seems like it should be one or the other, not both.

That's a good question... At the very least, we shouldn't change the existing behavior that a statistical patron records a localuse no matter what, imo, because that's the primary purpose of a statistical patron.


For MCPL, I think we would want both - we use both pieces of data to track library usage in different ways, and in this scenario, we would generally assume that the item really was returned by one patron and later used locally by another. (It's pretty common for items to get accidentally skipped when a staff member is scanning a pile of returns, and get put back on the shelf without actually getting checked in.)

But I'm sure there are other libraries that use those stats in very different ways, and may have different opinions - I agree we need to get more input on this.
Comment 14 Andrew Fuerste-Henry 2024-01-22 16:38:05 UTC
I tend to agree with Emily's assumption: an item found to be already checked out when scanned for a local use via statistical patron was likely returned by one patron, not properly checked in, and then later used by another patron. In that case, it would be correct to record both a return *and* a local use.
Comment 15 Lucas Gass 2024-01-22 16:51:02 UTC
So maybe, in an effort to move this forward and stop the double counting of localuse, we keep current existing behavior and stop the double increment of localuse? That seems like the kind of thing that could be a problem and inflate stats. 

We could move the discussion about  localuse + return / just return to another bug?
Comment 16 Andrew Fuerste-Henry 2024-01-22 17:25:43 UTC
(In reply to Lucas Gass from comment #15)
> So maybe, in an effort to move this forward and stop the double counting of
> localuse, we keep current existing behavior and stop the double increment of
> localuse? That seems like the kind of thing that could be a problem and
> inflate stats. 
> 
> We could move the discussion about  localuse + return / just return to
> another bug?

This sounds like a good path to me.
Comment 17 Lucas Gass 2024-01-22 18:47:52 UTC
Filed Bug 35860 to continue the discussion. Setting this one back to FQA and will have a patch soon!
Comment 18 Lucas Gass 2024-01-25 00:45:12 UTC
Created attachment 161374 [details] [review]
Bug 35840: (QA follow-up) Make sure return and localuse is recorded and unit tests

To test:
prove t/db_dependent/Circulation.t
Comment 19 Brendan Lawlor 2024-01-25 16:07:57 UTC
Created attachment 161442 [details] [review]
Bug 35840: Fix Local use being double-counted in CanBookBeIssued

To test:
1. APPLY PATCH, restart_all
2. Turn on RecordLocalUseOnReturn
3. Create a Statistical patron.
4. Check an item out to a regular patron.
5. Check the item out to a Statistical patron.
6. This should trigger a return and you will see 2 entries in the statistics table, one for localuse and one for a return.
7. Try checking out an item to the Stats patron that is NOT checked out.
8. You should only see 1 entry, localuse, in the statistics table.

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 20 Brendan Lawlor 2024-01-25 16:07:59 UTC
Created attachment 161443 [details] [review]
Bug 35840: (QA follow-up) Make sure return and localuse is recorded and unit tests

To test:
prove t/db_dependent/Circulation.t

Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org>
Comment 21 Brendan Lawlor 2024-01-25 16:11:32 UTC
Tested, works as described and it makes sense too.

Now I want to learn how these tests work...  
prove t/db_dependent/Circulation.t
t/db_dependent/Circulation.t .. ok     
All tests successful.
Files=1, Tests=68, 27 wallclock secs ( 0.08 usr  0.00 sys + 20.93 cusr  2.38 csys = 23.39 CPU)
Result: PASS
Comment 22 Marcel de Rooy 2024-01-26 09:43:03 UTC
Confusing material.
Why did we add AddReturn to CanBookBeIssued in the first place? It feels not good.
Consider this out of scope :)

Would be helpful to have the module changes in one patch and the test in another. Now they are spread. Could be squashed?

Please add POD changes for the new parameter of AddReturn.

The name $localuse is not clear enough imo. You want to skip localuse, right?

The unit test is not completely clear too. The results of various searches on item3 could overlap or so?
Could you make more explicit what really happens for both patrons?

Needs a bit of attention still. Thanks.
Comment 23 Lucas Gass 2024-01-29 18:05:38 UTC
> Why did we add AddReturn to CanBookBeIssued in the first place? It feels not
> good.

I added it here: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27992 

Can you propose a better way to do this?
Comment 24 Lucas Gass 2024-01-29 18:52:38 UTC
Created attachment 161612 [details] [review]
Bug 35840: Add option to AddReturn to skip recording of localuse

To test:
1. APPLY PATCH, restart_all
2. Turn on RecordLocalUseOnReturn
3. Create a Statistical patron.
4. Check an item out to a regular patron.
5. Check the item out to a Statistical patron.
6. This should trigger a return and you will see 2 entries in the statistics table, one for localuse and one for a return.
7. Try checking out an item to the Stats patron that is NOT checked out.
8. You should only see 1 entry, localuse, in the statistics table.
Comment 25 Lucas Gass 2024-01-29 19:20:16 UTC
Created attachment 161613 [details] [review]
Bug 35840: Add option to AddReturn to skip recording of localuse

To test:
1. APPLY PATCH, restart_all
2. Turn on RecordLocalUseOnReturn
3. Create a Statistical patron.
4. Check an item out to a regular patron.
5. Check the item out to a Statistical patron.
6. This should trigger a return and you will see 2 entries in the statistics table, one for localuse and one for a return.
7. Try checking out an item to the Stats patron that is NOT checked out.
8. You should only see 1 entry, localuse, in the statistics table.
Comment 26 Lucas Gass 2024-01-29 19:42:07 UTC
Created attachment 161614 [details] [review]
Bug 35840: Add unit tests
Comment 27 Marcel de Rooy 2024-01-30 13:05:36 UTC
(In reply to Lucas Gass from comment #23)
> > Why did we add AddReturn to CanBookBeIssued in the first place? It feels not
> > good.
> 
> I added it here:
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27992 
> 
> Can you propose a better way to do this?

That is no simple question. Depends on the whole context of why (and when) we are doing it, and the use cases for it.
But it feels that it should not be there.
Comment 28 Lucas Gass 2024-01-30 15:34:20 UTC
> That is no simple question. Depends on the whole context of why (and when)
> we are doing it, and the use cases for it.
> But it feels that it should not be there.

In CanBookBeIssued there is:

if ( $patron->category->category_type eq 'X' ) 

where we do some things in the borrower is a statistical patron. This is mostly for updating the stats table 'C4::Stats::UpdateStats'. As of Bug 27992, this also includes a call to AddReturn to make sure anything checked out to a stats patron is returned. 

Maybe we should open a new bug report to discuss the overall architecture?
Comment 29 Lucas Gass 2024-01-30 15:43:51 UTC
> Maybe we should open a new bug report to discuss the overall architecture?

I have opened Bug 35950.
Comment 30 Andrew Fuerste-Henry 2024-02-02 21:27:32 UTC
Created attachment 161732 [details] [review]
Bug 35840: Add option to AddReturn to skip recording of localuse

To test:
1. APPLY PATCH, restart_all
2. Turn on RecordLocalUseOnReturn
3. Create a Statistical patron.
4. Check an item out to a regular patron.
5. Check the item out to a Statistical patron.
6. This should trigger a return and you will see 2 entries in the statistics table, one for localuse and one for a return.
7. Try checking out an item to the Stats patron that is NOT checked out.
8. You should only see 1 entry, localuse, in the statistics table.

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Comment 31 Andrew Fuerste-Henry 2024-02-02 21:27:35 UTC
Created attachment 161733 [details] [review]
Bug 35840: Add unit tests

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Comment 32 Martin Renvoize 2024-02-12 17:07:16 UTC
Whilst I agree that having AddReturn embedding inside CanBookBeIssued is wrong, it is at least contained to 'statistical patrons'.

I think we should allow this through QA, but I really want to see the logic moved out of CanBookBeIssued and into the controllers calling it really.. are you intending to work on bug 25950 at all Lucas?
Comment 33 Lucas Gass 2024-02-12 17:23:16 UTC
(In reply to Martin Renvoize from comment #32)
> Whilst I agree that having AddReturn embedding inside CanBookBeIssued is
> wrong, it is at least contained to 'statistical patrons'.
> 
> I think we should allow this through QA, but I really want to see the logic
> moved out of CanBookBeIssued and into the controllers calling it really..
> are you intending to work on bug 25950 at all Lucas?

Martin, assuming you mean Bug 35950 I am happy to take that up.
Comment 34 Martin Renvoize 2024-03-08 14:34:38 UTC
Created attachment 162963 [details] [review]
Bug 35840: Add option to AddReturn to skip recording of localuse

To test:
1. APPLY PATCH, restart_all
2. Turn on RecordLocalUseOnReturn
3. Create a Statistical patron.
4. Check an item out to a regular patron.
5. Check the item out to a Statistical patron.
6. This should trigger a return and you will see 2 entries in the statistics table, one for localuse and one for a return.
7. Try checking out an item to the Stats patron that is NOT checked out.
8. You should only see 1 entry, localuse, in the statistics table.

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 35 Martin Renvoize 2024-03-08 14:34:41 UTC
Created attachment 162964 [details] [review]
Bug 35840: Add unit tests

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Martin Renvoize 2024-03-08 14:35:30 UTC
All working well and tested.

Passing QA
Comment 37 Martin Renvoize 2024-03-08 14:42:51 UTC
Created attachment 162965 [details] [review]
Bug 35840: (QA follow-up) Tidy and critic fix
Comment 38 Katrin Fischer 2024-03-08 17:20:29 UTC
At some point, we should make this nicer and easier to understand what's going on:

&AddReturn( $barcode, $branch [,$exemptfine] [,$returndate] [,$skip_localuse ] );

AddReturn( $item_object->barcode, C4::Context->userenv->{'branch'}, undef, undef, 1 )
Comment 39 Katrin Fischer 2024-03-08 17:59:25 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 40 Fridolin Somers 2024-03-11 14:18:58 UTC
(In reply to Katrin Fischer from comment #38)
> At some point, we should make this nicer and easier to understand what's
> going on:
> 
> &AddReturn( $barcode, $branch [,$exemptfine] [,$returndate] [,$skip_localuse
> ] );
> 
> AddReturn( $item_object->barcode, C4::Context->userenv->{'branch'}, undef,
> undef, 1 )

+1
We should use a %params hash
Comment 41 Fridolin Somers 2024-03-11 14:19:17 UTC
Pushed to 23.11.x for 23.11.04
Comment 42 Lucas Gass 2024-03-19 21:14:15 UTC
Failing unit tests on 23.05.x:

not ok 29 - CanBookBeIssued + Statistic patrons "X"

#   Failed test 'CanBookBeIssued + Statistic patrons "X"'
#   at /kohadevbox/koha/t/db_dependent/Circulation.t line 2358.
DBIx::Class::ResultSource::_minimal_valueset_satisfying_constraint(): Unable to satisfy requested constraint 'primary', missing values for column(s): 'borrowernumber' at /kohadevbox/koha/Koha/Objects.pm line 96
# Looks like your test exited with 11 just after 29.
Dubious, test returned 11 (wstat 2816, 0xb00)
Failed 40/68 subtests