Bug 8483 - Borrower reading history should include deleted items
Summary: Borrower reading history should include deleted items
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Kyle M Hall
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-20 14:18 UTC by Robin Sheat
Modified: 2017-09-08 18:02 UTC (History)
17 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 8483 - Borrower reading history should include deleted items (33.63 KB, patch)
2015-07-16 11:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Add checkout classes (6.82 KB, patch)
2015-07-16 12:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Add checkout classes (6.82 KB, patch)
2015-07-16 12:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Add record and item related classes (19.47 KB, patch)
2015-07-16 12:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Borrower reading history should include deleted items (7.16 KB, patch)
2015-07-16 12:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Add checkout classes (6.82 KB, patch)
2015-08-26 16:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Add record and item related classes (14.54 KB, patch)
2015-08-26 16:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Borrower reading history should include deleted items (7.16 KB, patch)
2015-08-26 16:09 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Add checkout classes (6.82 KB, patch)
2015-09-22 14:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Add record and item related classes (14.54 KB, patch)
2015-09-22 14:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Borrower reading history should include deleted items (8.17 KB, patch)
2015-09-22 14:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Borrower reading history should include deleted items (9.25 KB, patch)
2015-09-28 16:05 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Borrower reading history should include deleted items (9.47 KB, patch)
2015-09-28 16:34 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Add checkout classes (6.85 KB, patch)
2015-09-28 18:09 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 8483 - Add record and item related classes (14.57 KB, patch)
2015-09-28 18:09 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 8483 - Borrower reading history should include deleted items (9.52 KB, patch)
2015-09-28 18:09 UTC, Nicole C. Engard
Details | Diff | Splinter Review
Bug 8483 - Add checkout classes (6.85 KB, patch)
2016-03-14 17:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Add record and item related classes (14.55 KB, patch)
2016-03-14 17:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Borrower reading history should include deleted items (9.11 KB, patch)
2016-03-14 17:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Move Koha::Biblioitem(s) to Koha::BiblioItem(s) (3.32 KB, patch)
2016-03-14 17:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Add checkout classes (6.85 KB, patch)
2016-03-14 17:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Add record and item related classes (12.89 KB, patch)
2016-03-14 17:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Borrower reading history should include deleted items (9.16 KB, patch)
2016-03-14 17:51 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Move Koha::Biblioitem(s) to Koha::BiblioItem(s) (3.32 KB, patch)
2016-03-14 18:17 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Add checkout classes (6.85 KB, patch)
2016-03-14 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Add record and item related classes (12.89 KB, patch)
2016-03-14 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Borrower reading history should include deleted items (9.16 KB, patch)
2016-03-14 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Speed improvements (3.79 KB, patch)
2016-03-14 18:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Speed improvements (3.28 KB, patch)
2016-03-14 18:19 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 8483 - Speed improvements (2.83 KB, patch)
2016-03-14 18:20 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Sheat 2012-07-20 14:18:13 UTC
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.
Comment 1 Liz Rea 2012-11-19 20:42:07 UTC
Work is currently in progress on this bug for Horowhenua District Council.
Comment 2 HB-NEKLS 2013-11-12 14:46:59 UTC
Is this bug still being worked on by Horowhenua? We are very interested in this one.
Comment 3 Robin Sheat 2013-11-12 21:39:42 UTC
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.
Comment 4 HB-NEKLS 2014-12-10 00:08:14 UTC
Has there been any further movement on this bug?
Comment 5 HB-NEKLS 2015-07-01 00:28:40 UTC
Checking back in to see if the patch for this has moved forward, any further. Thanks.
Comment 6 Kyle M Hall 2015-07-16 11:49:00 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2015-07-16 12:00:17 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2015-07-16 12:03:01 UTC Comment hidden (obsolete)
Comment 9 Kyle M Hall 2015-07-16 12:03:08 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2015-07-16 12:03:11 UTC Comment hidden (obsolete)
Comment 11 Robin Sheat 2015-07-17 01:11:24 UTC
Is this a bit overengineered? Could you not just extend the query?
Comment 12 Kyle M Hall 2015-08-26 16:05:42 UTC
(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!
Comment 13 Kyle M Hall 2015-08-26 16:09:23 UTC Comment hidden (obsolete)
Comment 14 Kyle M Hall 2015-08-26 16:09:30 UTC Comment hidden (obsolete)
Comment 15 Kyle M Hall 2015-08-26 16:09:33 UTC Comment hidden (obsolete)
Comment 16 Aleisha Amohia 2015-08-27 03:12:44 UTC
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
Comment 17 Kyle M Hall 2015-09-14 14:33:59 UTC
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
Comment 18 Kyle M Hall 2015-09-22 14:40:48 UTC Comment hidden (obsolete)
Comment 19 Kyle M Hall 2015-09-22 14:40:57 UTC Comment hidden (obsolete)
Comment 20 Kyle M Hall 2015-09-22 14:41:00 UTC Comment hidden (obsolete)
Comment 21 Kyle M Hall 2015-09-28 16:05:50 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2015-09-28 16:34:14 UTC Comment hidden (obsolete)
Comment 23 Nicole C. Engard 2015-09-28 18:09:24 UTC Comment hidden (obsolete)
Comment 24 Nicole C. Engard 2015-09-28 18:09:33 UTC Comment hidden (obsolete)
Comment 25 Nicole C. Engard 2015-09-28 18:09:36 UTC Comment hidden (obsolete)
Comment 26 Jonathan Druart 2015-10-01 11:56:53 UTC
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
Comment 27 Kyle M Hall 2015-10-09 12:04:42 UTC
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
Comment 28 Jonathan Druart 2015-10-12 09:58:59 UTC
(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?
Comment 29 Kyle M Hall 2015-10-20 18:36:19 UTC
(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.
Comment 30 Jonathan Druart 2015-10-21 14:58:20 UTC
tested again, without plack, and got exactly the same difference.
Comment 31 Marcel de Rooy 2016-02-19 09:45:28 UTC
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?
Comment 32 Kyle M Hall 2016-03-14 17:13:36 UTC Comment hidden (obsolete)
Comment 33 Kyle M Hall 2016-03-14 17:13:45 UTC Comment hidden (obsolete)
Comment 34 Kyle M Hall 2016-03-14 17:13:49 UTC Comment hidden (obsolete)
Comment 35 Kyle M Hall 2016-03-14 17:51:35 UTC Comment hidden (obsolete)
Comment 36 Kyle M Hall 2016-03-14 17:51:45 UTC Comment hidden (obsolete)
Comment 37 Kyle M Hall 2016-03-14 17:51:48 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2016-03-14 17:51:52 UTC Comment hidden (obsolete)
Comment 39 Kyle M Hall 2016-03-14 18:17:55 UTC
Created attachment 49139 [details] [review]
Bug 8483 - Move Koha::Biblioitem(s) to Koha::BiblioItem(s)
Comment 40 Kyle M Hall 2016-03-14 18:18:03 UTC
Created attachment 49140 [details] [review]
Bug 8483 - Add checkout classes

Signed-off-by: Heather Braum <hbraum@nekls.org>
Comment 41 Kyle M Hall 2016-03-14 18:18:07 UTC
Created attachment 49141 [details] [review]
Bug 8483 - Add record and item related classes

Signed-off-by: Heather Braum <hbraum@nekls.org>
Comment 42 Kyle M Hall 2016-03-14 18:18:11 UTC
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>
Comment 43 Kyle M Hall 2016-03-14 18:18:15 UTC Comment hidden (obsolete)
Comment 44 Kyle M Hall 2016-03-14 18:19:05 UTC Comment hidden (obsolete)
Comment 45 Kyle M Hall 2016-03-14 18:20:24 UTC
Created attachment 49145 [details] [review]
Bug 8483 - Speed improvements
Comment 46 Kyle M Hall 2016-03-14 18:31:21 UTC
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.
Comment 47 Jonathan Druart 2016-04-13 15:58:57 UTC
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.
Comment 48 Marcel de Rooy 2016-04-14 06:41:07 UTC
(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.
Comment 49 Katrin Fischer 2016-04-14 09:37:03 UTC
Should we move this out of the list for now? "In Discussion" or "Failed QA"?
Comment 50 Katrin Fischer 2016-04-18 05:56:37 UTC
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.
Comment 51 Kyle M Hall 2016-04-18 12:21:29 UTC
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?
Comment 52 Nicole C. Engard 2016-05-12 16:07:59 UTC
Checking in here. Can anyone on the QA team help Kyle with his question?
Comment 53 Jonathan Druart 2016-05-13 20:24:20 UTC
(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.
Comment 54 Kyle M Hall 2016-06-07 18:26:17 UTC
(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!