Work is currently in progress on this bug for Horowhenua District Council.

Is this bug still being worked on by Horowhenua? We are very interested in this one.

It's very much on the backburner, but is fairly close to being done. If memory serves, it needs some minor schema changes, and that should be about it.

Has there been any further movement on this bug?

Checking back in to see if the patch for this has moved forward, any further. Thanks.

Is this a bit overengineered? Could you not just extend the query?

(In reply to Robin Sheat from comment #11)
> Is this a bit overengineered? Could you not just extend the query?

I can see why you'd think that. I'd consider this as an opportunity to add some more useful Koha objects. There's little point to having an OO model if we don't take advantage of it, and there always has to be that first step!

DBD::mysql::db do failed: Lock wait timeout exceeded; try restarting transaction at ../t/db_dependent/Members/GetAllIssues.t line 20.
# Looks like your test exited with 255 before it could output anything.
../t/db_dependent/Members/GetAllIssues.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 16/16 subtests 

Test Summary Report
-------------------
../t/db_dependent/Members/GetAllIssues.t (Wstat: 65280 Tests: 0 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 16 tests but ran 0.
Files=1, Tests=0, 56 wallclock secs ( 0.01 usr  0.03 sys +  1.19 cusr  0.22 csys =  1.45 CPU)
Result: FAIL

Setting to Failed QA

I'm unable to reproduce your test failure:

prove t/db_dependent/Members/GetAllIssues.t
t/db_dependent/Members/GetAllIssues.t .. ok
All tests successful.
Files=1, Tests=16,  5 wallclock secs ( 0.02 usr  0.01 sys +  1.06 cusr  0.08 csys =  1.17 CPU)
Result: PASS

(In reply to Aleisha Amohia from comment #16)
> DBD::mysql::db do failed: Lock wait timeout exceeded; try restarting
> transaction at ../t/db_dependent/Members/GetAllIssues.t line 20.
> # Looks like your test exited with 255 before it could output anything.
> ../t/db_dependent/Members/GetAllIssues.t .. Dubious, test returned 255
> (wstat 65280, 0xff00)
> Failed 16/16 subtests 
> 
> Test Summary Report
> -------------------
> ../t/db_dependent/Members/GetAllIssues.t (Wstat: 65280 Tests: 0 Failed: 0)
>   Non-zero exit status: 255
>   Parse errors: Bad plan.  You planned 16 tests but ran 0.
> Files=1, Tests=0, 56 wallclock secs ( 0.01 usr  0.03 sys +  1.19 cusr  0.22
> csys =  1.45 CPU)
> Result: FAIL
> 
> Setting to Failed QA

I was afraid of the complexity in the call stack, so did some tests:

$ more bug_8483.pl
use Modern::Perl;
use C4::Members;
my $borrowernumber=42;
my $issues = GetAllIssues( $borrowernumber, 'date_due desc' );

> select count(*) from old_issues where borrowernumber=42;  
43
> select count(*) from issues where borrowernumber=42;
7

$ git checkout master; git reset --hard origin/master
$ git checkout -b bug_8483; git bz apply 8483

$ git checkout master; time perl bug_8483.pl
perl bug_8483.pl  1.23s user 0.04s system 99% cpu 1.270 total
$ git checkout bug_8483; time perl bug_8483.pl
perl bug_8483.pl  1.50s user 0.05s system 99% cpu 1.560 total

# => Ok we get the old_issues in addition of 


# Adding some issues to the patron
> select count(*) from issues where borrowernumber=42;
49
# master
perl bug_8483.pl  1.21s user 0.08s system 99% cpu 1.280 total
# bug_8483
perl bug_8483.pl  1.82s user 0.07s system 97% cpu 1.933 total

210
# master
perl bug_8483.pl  1.21s user 0.07s system 98% cpu 1.293 total
# bug_8483
perl bug_8483.pl  2.73s user 0.09s system 95% cpu 2.959 total

600
# master
perl bug_8483.pl  1.28s user 0.03s system 97% cpu 1.340 total
# bug_8483
perl bug_8483.pl  4.78s user 0.10s system 94% cpu 5.154 total

With 540 checkouts I get the following with your script:

on master:
real	0m1.023s
user	0m0.980s
sys	0m0.044s

with bug 8483:
real	0m1.032s
user	0m0.980s
sys	0m0.052s

(In reply to Kyle M Hall from comment #27)
> With 540 checkouts I get the following with your script:
> 
> on master:
> real	0m1.023s
> user	0m0.980s
> sys	0m0.044s
> 
> with bug 8483:
> real	0m1.032s
> user	0m0.980s
> sys	0m0.052s

Is it 500 checkouts for 1 patron?

(In reply to Jonathan Druart from comment #28)
> (In reply to Kyle M Hall from comment #27)
> > With 540 checkouts I get the following with your script:
> > 
> > on master:
> > real	0m1.023s
> > user	0m0.980s
> > sys	0m0.044s
> > 
> > with bug 8483:
> > real	0m1.032s
> > user	0m0.980s
> > sys	0m0.052s
> 
> Is it 500 checkouts for 1 patron?

Yes, all the checkouts are for one patron.

tested again, without plack, and got exactly the same difference.

The patch does not apply anymore.
Additionally, reading the discussion, how can I generate some performance figures to compare with the different figures given already?

Created attachment 49139 [details] [review]
Bug 8483 - Move Koha::Biblioitem(s) to Koha::BiblioItem(s)

Created attachment 49140 [details] [review]
Bug 8483 - Add checkout classes

Signed-off-by: Heather Braum <hbraum@nekls.org>

Created attachment 49141 [details] [review]
Bug 8483 - Add record and item related classes

Signed-off-by: Heather Braum <hbraum@nekls.org>

Created attachment 49142 [details] [review]
Bug 8483 - Borrower reading history should include deleted items

When showing a borrower's reading history, it'd be good to be able to
include the deleted items also, as we have that information available.

Test Plan:
1) Apply this patch
2) prove t/db_dependent/Members/GetAllIssues.t
3) Check out and return an item for a patron
4) Delete that item
5) Verify the record still shows in the patron's reading history
6) Delete the record entirely
7) Verify the record still shows in the patron's reading history,
   but that the title is now just text instead of a hyperlink.

Signed-off-by: Heather Braum <hbraum@nekls.org>

Created attachment 49145 [details] [review]
Bug 8483 - Speed improvements

Test Case, Patron with 502 checkouts:
Master: 2.7 seconds
Original Patch: 4.8 seconds
With followup: 3.6 seconds

Given the improvements this patch makes, the uncommon usage of the feature, and the OO improvements, I think the minor decrease in the script speed this patch causes is worth the cost.

The trick done in Koha::Checkout->item may be dangerous.
Same for Koha::Item->biblio and Koha::Item->biblioitem
You are not fetching the item on each call. That mean you could retrieve an outdated info in some scenarios.

The mix between the Koha::Object and DBIx::Class calls is also weird.

I'd like to get some other QA point of views on this patch set.

Note that tests are not provided and will be a blocker.

(In reply to Jonathan Druart from comment #47)
> The trick done in Koha::Checkout->item may be dangerous.
> Same for Koha::Item->biblio and Koha::Item->biblioitem
> You are not fetching the item on each call. That mean you could retrieve an
> outdated info in some scenarios.
> 
> The mix between the Koha::Object and DBIx::Class calls is also weird.
> 
> I'd like to get some other QA point of views on this patch set.

Does not look good to me too.
We should probably not use _new_from_dbic outside Object[s] too.

Should we move this out of the list for now? "In Discussion" or "Failed QA"?

I think we can agree that the feature itself would be good to have, hope we can agree about implementation too :) 

Moving this to 'failed qa' (unit tests) for now - please take a look at the comments and feel free to bring it up at the next dev meeting if you think this will help moving it forward.

I need someone to tell me what needs changed in order to for this to pqa. All of these changes are internal to Koha::Object and it's children. Taking advantage of the underlying DBIC object should not be an issue. Can someone please tell me specifically what needs to be changed?

Checking in here. Can anyone on the QA team help Kyle with his question?

(In reply to Kyle M Hall from comment #51)
> I need someone to tell me what needs changed in order to for this to pqa.
> All of these changes are internal to Koha::Object and it's children. Taking
> advantage of the underlying DBIC object should not be an issue. Can someone
> please tell me specifically what needs to be changed?

I am sorry but I do not have any good answers.
It seems that we are stuck with our Koha::Object[s] implementation at different places. Here you are trying to cache something when we should rely on DBIx::Class.

(In reply to Jonathan Druart from comment #53)
> (In reply to Kyle M Hall from comment #51)
> > I need someone to tell me what needs changed in order to for this to pqa.
> > All of these changes are internal to Koha::Object and it's children. Taking
> > advantage of the underlying DBIC object should not be an issue. Can someone
> > please tell me specifically what needs to be changed?
> 
> I am sorry but I do not have any good answers.
> It seems that we are stuck with our Koha::Object[s] implementation at
> different places. Here you are trying to cache something when we should rely
> on DBIx::Class.

Can you describe how we should be relying on DBIx::Class here? Are you saying I should be getting the data via the underlying DBIC connection which I can then wrap and return? If so I can do that!

still valid in 20.05

This will be fixed on bug 33948.

*** This bug has been marked as a duplicate of bug 33948 ***

If you were interested in this, can you help testing bug 33948 please?