Bug 14598

Summary: itemtype is not set on statistics by C4::Circulation::AddReturn
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: CirculationAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: andreas.hedstrom.mace, fridolin.somers, gmcharlt, gwendal.joncour, jonathan.druart, julian.maurice, jweaver, katrin.fischer, kyle.m.hall, kyle, m.de.rooy, marjorie.barry-vila, mtj, tomascohen, veron
Version: master   
Hardware: All   
OS: All   
See Also: http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14531
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351
Change sponsored?: Sponsored Patch complexity: Medium patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 16216    
Bug Blocks: 15599, 17572, 17573, 17574, 17575, 17587, 17603, 17636, 17646, 17647, 17653, 18460    
Attachments: Bug 14598: (regression tests) AddReturn should store itemtype on 'statistics' table
Bug 14598: Make C4::Circulation::AddReturn store the right itemtype
Bug 14598 [QA Followup] - Don't die on bad barcode
Bug 14598 [QA Followup] - Don't die on bad barcode
Bug 14598: (QA followup) Don't die on bad barcode
[SIGNED-OFF] Bug 14598: (regression tests) AddReturn should store itemtype on 'statistics' table
[SIGNED-OFF] Bug 14598: Make C4::Circulation::AddReturn store the right itemtype
[SIGNED-OFF] Bug 14598: (QA followup) Don't die on bad barcode
Bug 14598: (regression tests) AddReturn should store itemtype on 'statistics' table
Bug 14598: Make C4::Circulation::AddReturn store the right itemtype
Bug 14598: (QA followup) Don't die on bad barcode
[SIGNED-OFF] Bug 14598: (regression tests) AddReturn should store itemtype on 'statistics' table
[SIGNED-OFF] Bug 14598: Make C4::Circulation::AddReturn store the right itemtype
[SIGNED-OFF] Bug 14598: (QA followup) Don't die on bad barcode
Bug 14598: (DB update) fix NULL itemtypes in statistics on 'return' rows
Bug 14598: (DB update) fix NULL itemtypes in statistics on 'return' rows
[SIGNED-OFF] Bug 14598: (DB update) fix NULL itemtypes in statistics on 'return' rows
Bug 14598: (regression tests) AddReturn should store itemtype on 'statistics' table
Bug 14598: Make C4::Circulation::AddReturn store the right itemtype
Bug 14598: (QA followup) Don't die on bad barcode
Bug 14598: (DB update) fix NULL itemtypes in statistics on 'return' rows
Bug 14598 - QA Followup touching CanBookBeIssued subroutine
Bug 14598 [QA Followup] - Only fetch bib data if needed
Bug 14598 [QA Followup] - Correct the behavior of GetItem
Bug 14598 [QA Followup] - Correct the behavior of GetItem
Bug 14598 [QA Followup] - Correct the behavior of GetItem
Bug 14598 [QA Followup] - Make unit test pass
Bug 14598 [QA Followup] - Update localuse statistics also
Bug 14598: (regression tests) AddReturn should store itemtype on 'statistics' table
Bug 14598: Make C4::Circulation::AddReturn store the right itemtype
Bug 14598: (QA followup) Don't die on bad barcode
Bug 14598: (DB update) fix NULL itemtypes in statistics on 'return' rows
Bug 14598 - QA Followup touching CanBookBeIssued subroutine
Bug 14598 [QA Followup] - Only fetch bib data if needed
Bug 14598 [QA Followup] - Correct the behavior of GetItem
Bug 14598 [QA Followup] - Make unit test pass
Bug 14598 [QA Followup] - Update localuse statistics also
Bug 14598: (regression tests) AddReturn should store itemtype on 'statistics' table
Bug 14598: Make C4::Circulation::AddReturn store the right itemtype
Bug 14598: (QA followup) Don't die on bad barcode
Bug 14598: (DB update) fix NULL itemtypes in statistics on 'return' rows
Bug 14598 - QA Followup touching CanBookBeIssued subroutine
Bug 14598 [QA Followup] - Only fetch bib data if needed
Bug 14598 [QA Followup] - Correct the behavior of GetItem
Bug 14598 [QA Followup] - Make unit test pass
Bug 14598 [QA Followup] - Update localuse statistics also
Bug 14598: (followup) Remove unused and non-existent C4::ItemType include
Rebased
Rebased
Rebased
Rebased
Rebased
Rebased
Rebased
Rebased
Rebased
Rebased
Bug 14598: (regression tests) AddReturn should store itemtype on 'statistics' table
Bug 14598: Make C4::Circulation::AddReturn store the right itemtype
Bug 14598: (QA followup) Don't die on bad barcode
Bug 14598: (DB update) fix NULL itemtypes in statistics on 'return' rows
Bug 14598 - QA Followup touching CanBookBeIssued subroutine
Bug 14598 [QA Followup] - Only fetch bib data if needed
Bug 14598 [QA Followup] - Correct the behavior of GetItem
Bug 14598 [QA Followup] - Make unit test pass
Bug 14598 [QA Followup] - Update localuse statistics also
Bug 14598: (followup) Remove unused and non-existent C4::ItemType include
Bug 14598: (QA followup) use deleted{items|biblioitems} during upgrade
Bug 14598: Fix warning from effective_itemtype
Bug 14598: (regression tests) AddReturn should store itemtype on 'statistics' table
Bug 14598: Make C4::Circulation::AddReturn store the right itemtype
Bug 14598: (QA followup) Don't die on bad barcode
Bug 14598: (DB update) fix NULL itemtypes in statistics on 'return' rows
Bug 14598 - QA Followup touching CanBookBeIssued subroutine
Bug 14598 [QA Followup] - Only fetch bib data if needed
Bug 14598 [QA Followup] - Correct the behavior of GetItem
Bug 14598 [QA Followup] - Make unit test pass
Bug 14598 [QA Followup] - Update localuse statistics also
Bug 14598: (followup) Remove unused and non-existent C4::ItemType include
Bug 14598: (QA followup) use deleted{items|biblioitems} during upgrade
Bug 14598: Fix warning from effective_itemtype
Bug 14598: [QA Follow-up] Small changes
Script to set itemtype on stats
Script to set itemtype on stats
Set itemtype on stats - Rennes 2 version

Description Tomás Cohen Arazi 2015-07-24 03:48:59 UTC

    
Comment 1 Tomás Cohen Arazi 2015-07-27 15:04:03 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2015-07-27 15:04:08 UTC Comment hidden (obsolete)
Comment 3 Kyle M Hall 2015-07-27 15:17:33 UTC Comment hidden (obsolete)
Comment 4 Kyle M Hall 2015-07-27 15:18:55 UTC Comment hidden (obsolete)
Comment 5 Tomás Cohen Arazi 2015-07-27 15:23:00 UTC Comment hidden (obsolete)
Comment 6 Kyle M Hall 2015-07-27 15:25:34 UTC Comment hidden (obsolete)
Comment 7 Kyle M Hall 2015-07-27 15:25:41 UTC Comment hidden (obsolete)
Comment 8 Kyle M Hall 2015-07-27 15:25:43 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2015-08-05 09:28:15 UTC
Is it a duplicate of bug 9351?

I would say we need to update existing rows.
Comment 10 Tomás Cohen Arazi 2015-08-05 18:28:48 UTC Comment hidden (obsolete)
Comment 11 Tomás Cohen Arazi 2015-08-05 18:28:56 UTC Comment hidden (obsolete)
Comment 12 Tomás Cohen Arazi 2015-08-05 18:29:03 UTC Comment hidden (obsolete)
Comment 13 Tomás Cohen Arazi 2015-08-05 18:29:34 UTC
*** Bug 9351 has been marked as a duplicate of this bug. ***
Comment 14 Tomás Cohen Arazi 2015-08-05 18:30:44 UTC
*** Bug 14531 has been marked as a duplicate of this bug. ***
Comment 15 Kyle M Hall 2015-08-18 16:55:45 UTC Comment hidden (obsolete)
Comment 16 Kyle M Hall 2015-08-18 16:55:51 UTC Comment hidden (obsolete)
Comment 17 Kyle M Hall 2015-08-18 16:55:54 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2015-08-18 17:19:10 UTC
(In reply to Jonathan Druart from comment #9)
> Is it a duplicate of bug 9351?
> 
> I would say we need to update existing rows.

What about existing rows?
Comment 19 Tomás Cohen Arazi 2015-08-18 19:49:01 UTC Comment hidden (obsolete)
Comment 20 Jonathan Druart 2015-08-19 11:23:48 UTC
I am not sure it's a good idea to use existing subroutines/methods in the updatedb process.
It cans work now but how to know it won't be broken in several versions?
Comment 21 Tomás Cohen Arazi 2015-09-01 14:56:25 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2015-09-01 16:30:15 UTC Comment hidden (obsolete)
Comment 23 Jonathan Druart 2015-09-03 10:02:23 UTC
Tomás, you have marked this bug as a duplicate of bug 9351 but you don't take care of the CanBookBeIssued subroutine.
Do you plan to update it as well?
Comment 24 Tomás Cohen Arazi 2015-09-03 10:41:42 UTC
(In reply to Jonathan Druart from comment #23)
> Tomás, you have marked this bug as a duplicate of bug 9351 but you don't
> take care of the CanBookBeIssued subroutine.
> Do you plan to update it as well?

It is GBSD, I will probably fill a new bug and take care of that too, yeah. ☺
Comment 25 Jonathan Druart 2015-09-03 10:49:24 UTC
It does not look to depend on bug 14651
Comment 26 Jonathan Druart 2015-09-03 10:55:21 UTC
The thing is that the update queries can take ages on a big DB, and we will have to loop again if everything is not fixed at the same time.
Comment 27 Tomás Cohen Arazi 2015-09-03 12:39:38 UTC
(In reply to Jonathan Druart from comment #25)
> It does not look to depend on bug 14651

That's your fault; it originally depended on it :-D
Comment 28 Jonathan Druart 2015-09-15 07:54:11 UTC
What about comments 23&26?
Comment 29 Tomás Cohen Arazi 2015-11-09 19:57:15 UTC Comment hidden (obsolete)
Comment 30 Tomás Cohen Arazi 2015-11-09 19:57:26 UTC Comment hidden (obsolete)
Comment 31 Tomás Cohen Arazi 2015-11-09 19:57:36 UTC Comment hidden (obsolete)
Comment 32 Tomás Cohen Arazi 2015-11-09 19:57:45 UTC Comment hidden (obsolete)
Comment 33 Tomás Cohen Arazi 2015-11-09 20:22:08 UTC
Patches rebased to master.
Comment 34 Tomás Cohen Arazi 2015-11-18 22:10:48 UTC
(In reply to Jonathan Druart from comment #23)
> Tomás, you have marked this bug as a duplicate of bug 9351 but you don't
> take care of the CanBookBeIssued subroutine.
> Do you plan to update it as well?

When I wrote this patches I wasn't aware of bug 9351. That bug lacked tests, and I considered my approach a better one.
This patches don't cover the "localuse" use case, but they fix a bug, and have regression tests. Someone could write a followup, but I didn't find the time to do it myself. Maybe the mistake was to mark the original as duplicate.
Comment 35 Brendan Gallagher 2015-11-19 21:15:33 UTC Comment hidden (obsolete)
Comment 36 Katrin Fischer 2015-11-20 01:32:12 UTC
Comment on attachment 45030 [details] [review]
Bug 14598 - QA Followup touching CanBookBeIssued subroutine

I think this is not quite right yet considering item-level_itypes. 
I have another solution that I think is correct... but while the tests in Koha look great I totally fail on writing patches that pass :(
Comment 37 Kyle M Hall 2015-11-20 14:24:30 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2015-12-18 15:00:38 UTC Comment hidden (obsolete)
Comment 39 Kyle M Hall 2015-12-18 15:39:07 UTC Comment hidden (obsolete)
Comment 40 Kyle M Hall 2015-12-18 15:40:21 UTC Comment hidden (obsolete)
Comment 41 Jesse Weaver 2016-01-08 00:04:08 UTC
One of the new test fails after applying all patches, and I'm not quite sure why:

[elf]:~/p/koha> prove t/db_dependent/Circulation/Returns.t -- [2016-01-07 17:01] ------------------------------------------------------------------------------------------------------------
t/db_dependent/Circulation/Returns.t .. 1/3 item-level_itypes set but no itemtype set for item (Koha::Schema::Result::Item=HASH(0x7666618)->itemnumber) at /home/feoh3/p/koha/Koha/Schema/Result/Item.pm line 653.
item-level_itypes set but no itemtype set for item (Koha::Schema::Result::Item=HASH(0x7677b68)->itemnumber) at /home/feoh3/p/koha/Koha/Schema/Result/Item.pm line 653.
    
    #   Failed test 'biblio-level itype recorded on statistics for return as a fallback for null item-level itype'
    #   at t/db_dependent/Circulation/Returns.t line 167.
    #          got: 'zmH5aYw18'
    #     expected: 'JTI'
    # Looks like you failed 1 test of 2.

#   Failed test 'AddReturn logging on statistics table (item-level_itypes=1)'
#   at t/db_dependent/Circulation/Returns.t line 170.
# Looks like you failed 1 test of 3.
t/db_dependent/Circulation/Returns.t .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 1/3 subtests 

Test Summary Report
-------------------
t/db_dependent/Circulation/Returns.t (Wstat: 256 Tests: 3 Failed: 1)
  Failed test:  2
  Non-zero exit status: 1
Files=1, Tests=3,  1 wallclock secs ( 0.02 usr  0.00 sys +  1.19 cusr  0.04 csys =  1.25 CPU)
Result: FAIL

(FYI: the same warning that appears at the top of the test output here also appears when I prove t/db_dependent/Items.t, though that test passes.)
Comment 42 Kyle M Hall 2016-01-12 16:05:32 UTC Comment hidden (obsolete)
Comment 43 Jonathan Druart 2016-01-13 11:51:02 UTC
(In reply to Jesse Weaver from comment #41)
> (FYI: the same warning that appears at the top of the test output here also
> appears when I prove t/db_dependent/Items.t, though that test passes.)

We should not expect warnings on proving, they must be caught be warning_is/warning_like.
Comment 44 Kyle M Hall 2016-01-19 12:51:48 UTC
(In reply to Jonathan Druart from comment #43)
> (In reply to Jesse Weaver from comment #41)
> > (FYI: the same warning that appears at the top of the test output here also
> > appears when I prove t/db_dependent/Items.t, though that test passes.)
> 
> We should not expect warnings on proving, they must be caught be
> warning_is/warning_like.

The warnings are unavoidable, unless we change the behavior that falls back to the record itemtype if there is no item itype ( even though item-level_itypes is enabled ). 

I think are actually testing the cases that specifically triggers this error condition.

Either way, I think the issue is outside the scope of this bug. We should decide whether or not this is a bug that needs fixed on a separate report.
Comment 45 Kyle M Hall 2016-01-19 12:53:28 UTC
It took me a minute for what you wrote to sink in. Still, we should address the issue on a separate bug report.
Comment 46 Kyle M Hall 2016-01-19 13:50:20 UTC
Unit tests fixes submitted on bug 15599!
Comment 47 Jonathan Druart 2016-01-19 14:20:25 UTC
Kyle,
With the last patches, you consider that all the concerns have been taken into account, that's it?
It's what I understand, but in this case, we need to update the existing rows in the update DB entry.
Comment 48 Katrin Fischer 2016-01-19 14:23:27 UTC
I think other routines (CanBookBeIssued, ...?) still suffer the same problem - I tried fixing CanBookBeIssued, but failed at writing the unit tests. We should make sure there are updated separate bug reports for those - best would be to fix them all before the next release (consistency in reporting)
Comment 49 Kyle M Hall 2016-01-19 14:25:40 UTC
(In reply to Jonathan Druart from comment #47)
> Kyle,
> With the last patches, you consider that all the concerns have been taken
> into account, that's it?
> It's what I understand, but in this case, we need to update the existing
> rows in the update DB entry.

There is already a db update in updatedatabase.pl, is this what you are looking for?
Comment 50 Jonathan Druart 2016-01-19 14:27:21 UTC
(In reply to Kyle M Hall from comment #49)
> (In reply to Jonathan Druart from comment #47)
> > Kyle,
> > With the last patches, you consider that all the concerns have been taken
> > into account, that's it?
> > It's what I understand, but in this case, we need to update the existing
> > rows in the update DB entry.
> 
> There is already a db update in updatedatabase.pl, is this what you are
> looking for?

No, it does not update localuse.
Comment 51 Kyle M Hall 2016-01-19 14:34:17 UTC Comment hidden (obsolete)
Comment 52 Kyle M Hall 2016-01-19 14:34:54 UTC
(In reply to Jonathan Druart from comment #50)
> No, it does not update localuse.

This followup should take care of that!
Comment 53 Jonathan Druart 2016-01-20 10:57:09 UTC
Could someone signoff the last 5 patches please?
Comment 54 Tomás Cohen Arazi 2016-01-21 13:40:12 UTC Comment hidden (obsolete)
Comment 55 Tomás Cohen Arazi 2016-01-21 13:40:19 UTC Comment hidden (obsolete)
Comment 56 Tomás Cohen Arazi 2016-01-21 13:40:29 UTC Comment hidden (obsolete)
Comment 57 Tomás Cohen Arazi 2016-01-21 13:40:35 UTC Comment hidden (obsolete)
Comment 58 Tomás Cohen Arazi 2016-01-21 13:40:41 UTC Comment hidden (obsolete)
Comment 59 Tomás Cohen Arazi 2016-01-21 13:40:51 UTC Comment hidden (obsolete)
Comment 60 Tomás Cohen Arazi 2016-01-21 13:41:01 UTC Comment hidden (obsolete)
Comment 61 Tomás Cohen Arazi 2016-01-21 13:41:08 UTC Comment hidden (obsolete)
Comment 62 Tomás Cohen Arazi 2016-01-21 13:41:48 UTC Comment hidden (obsolete)
Comment 63 Jonathan Druart 2016-01-25 11:51:56 UTC
We need to fix bug 15599 first.
Comment 64 Jonathan Druart 2016-01-29 12:12:52 UTC
When I have updated the DB, I got a lot of warnings:

item-level_itypes set but no itype defined for item (3751) at installer/data/mysql/updatedatabase.pl line 11728.

Should we keep them or hide them?
Comment 65 Jonathan Druart 2016-01-29 12:16:02 UTC
It seems that I get the warning when an item does not exist.
Maybe we should check the deleteditems table.
Comment 66 Katrin Fischer 2016-01-29 12:16:58 UTC
Can you quickly explain how the warn is caused?
Comment 67 Katrin Fischer 2016-01-29 12:17:52 UTC
Missed your last comment, sorry! We usually use deleteditems for statistics too - so it makes sense to include it I think.
Comment 68 Jonathan Druart 2016-03-23 22:38:57 UTC
In discussion, blocked or fail qa?
I let you choose the status...
Comment 69 Tomás Cohen Arazi 2016-04-01 15:20:51 UTC Comment hidden (obsolete)
Comment 70 Tomás Cohen Arazi 2016-04-01 15:21:00 UTC Comment hidden (obsolete)
Comment 71 Tomás Cohen Arazi 2016-04-01 15:21:05 UTC Comment hidden (obsolete)
Comment 72 Tomás Cohen Arazi 2016-04-01 15:21:11 UTC Comment hidden (obsolete)
Comment 73 Tomás Cohen Arazi 2016-04-01 15:21:16 UTC Comment hidden (obsolete)
Comment 74 Tomás Cohen Arazi 2016-04-01 15:21:25 UTC Comment hidden (obsolete)
Comment 75 Tomás Cohen Arazi 2016-04-01 15:21:34 UTC Comment hidden (obsolete)
Comment 76 Tomás Cohen Arazi 2016-04-01 15:21:50 UTC Comment hidden (obsolete)
Comment 77 Tomás Cohen Arazi 2016-04-01 15:21:59 UTC Comment hidden (obsolete)
Comment 78 Tomás Cohen Arazi 2016-04-01 15:22:29 UTC
Patches rebased so they apply on master.
Comment 79 Tomás Cohen Arazi 2016-04-01 15:26:24 UTC Comment hidden (obsolete)
Comment 80 Tomás Cohen Arazi 2016-04-06 13:43:21 UTC Comment hidden (obsolete)
Comment 81 Tomás Cohen Arazi 2016-04-06 13:43:37 UTC Comment hidden (obsolete)
Comment 82 Tomás Cohen Arazi 2016-04-06 13:43:50 UTC Comment hidden (obsolete)
Comment 83 Tomás Cohen Arazi 2016-04-06 13:44:01 UTC Comment hidden (obsolete)
Comment 84 Tomás Cohen Arazi 2016-04-06 13:44:14 UTC Comment hidden (obsolete)
Comment 85 Tomás Cohen Arazi 2016-04-06 13:44:25 UTC Comment hidden (obsolete)
Comment 86 Tomás Cohen Arazi 2016-04-06 13:44:38 UTC Comment hidden (obsolete)
Comment 87 Tomás Cohen Arazi 2016-04-06 13:44:47 UTC Comment hidden (obsolete)
Comment 88 Tomás Cohen Arazi 2016-04-06 13:44:59 UTC Comment hidden (obsolete)
Comment 89 Tomás Cohen Arazi 2016-04-06 13:45:10 UTC Comment hidden (obsolete)
Comment 90 Tomás Cohen Arazi 2016-04-06 14:46:23 UTC
Created attachment 49978 [details] [review]
Bug 14598: (regression tests) AddReturn should store itemtype on 'statistics' table

This patch adds tests for C4::Circulation::AddReturn(). Both tests are wrapped
inside a subtest, and look for AddReturn storing the right itemtype on the
'statistics' table.

Note: It also refactors the file a bit, to avoid side effects. And uses TestBuilder
to make it independent on already-present db data. It also removes warnings by mocking
C4::Context::userenv

To test:
- Apply the patch
- Run the tests:
  $ prove t/db_dependent/Circulation/Returns.t
=> FAIL: Tests fail bacause AddReturn is not storing the itemtype
- Sign off :-D

Sponsored-by: Universidad Empresarial Siglo 21

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 91 Tomás Cohen Arazi 2016-04-06 14:46:31 UTC
Created attachment 49979 [details] [review]
Bug 14598: Make C4::Circulation::AddReturn store the right itemtype

This patch makes C4::Circulation::AddReturn correctly store the itemtype
on the 'statistics' table.

To reproduce:
- Checkout master.
- Make a checkout.
- Check the 'statistics' table and notice the itemtype is correctly set
  > SELECT * FROM statistics;
- Check the item in.
- Check the 'statistics' table and notice the itemtype is not set
  > SELECT * FROM statistics WHERE type="return";
=> FAIL: itemtype is set to NULL

To test:
- Apply the regression tests patch
- Run the tests:
  $ prove t/db_dependent/Circulation/Returns.t
=> FAIL: Tests fail
- Apply this patch
- Run the regression tests:
  $ prove t/db_dependent/Circulation/Returns.t
=> SUCCESS: Tests now pass.
- Repeat the 'To reproduce' steps
=> SUCCESS: itemtype is now correctly set (in real life)
- Happily sign off :-D

Sponsored-by: Universidad Empresarial Siglo 21

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 92 Tomás Cohen Arazi 2016-04-06 14:46:39 UTC
Created attachment 49980 [details] [review]
Bug 14598: (QA followup) Don't die on bad barcode

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 93 Tomás Cohen Arazi 2016-04-06 14:46:48 UTC
Created attachment 49981 [details] [review]
Bug 14598: (DB update) fix NULL itemtypes in statistics on 'return' rows

This patch introduces an updatedatabase.pl entry that takes care of
updating existing statistics rows.

It does so by looping on the statistics rows, collecting itemnumber occurences
that are have NULL itemtypes.
It then chooses the right itemtype following what is proposed on bug 14651, and
then updates the rows in statistics using the calculated itemtype.

Regards

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 94 Tomás Cohen Arazi 2016-04-06 14:46:57 UTC
Created attachment 49982 [details] [review]
Bug 14598 - QA Followup touching CanBookBeIssued subroutine

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 95 Tomás Cohen Arazi 2016-04-06 14:47:13 UTC
Created attachment 49983 [details] [review]
Bug 14598 [QA Followup] - Only fetch bib data if needed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 96 Tomás Cohen Arazi 2016-04-06 14:47:19 UTC
Created attachment 49984 [details] [review]
Bug 14598 [QA Followup] - Correct the behavior of GetItem

Currently GetItem sets itemtype to the biblio itemtype if no item level
itemtype exists. Instead, it should only do this if item_level-itypes
is not set.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 97 Tomás Cohen Arazi 2016-04-06 14:47:25 UTC
Created attachment 49985 [details] [review]
Bug 14598 [QA Followup] - Make unit test pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 98 Tomás Cohen Arazi 2016-04-06 14:47:31 UTC
Created attachment 49986 [details] [review]
Bug 14598 [QA Followup] - Update localuse statistics also

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 99 Tomás Cohen Arazi 2016-04-06 14:47:36 UTC
Created attachment 49987 [details] [review]
Bug 14598: (followup) Remove unused and non-existent C4::ItemType include

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Comment 100 Tomás Cohen Arazi 2016-04-06 14:47:43 UTC
Created attachment 49988 [details] [review]
Bug 14598: (QA followup) use deleted{items|biblioitems} during upgrade

If the 'statistics' table contains circulation information about items
that are no longer on the DB it will raise a warning. This patch
makes the updatedatabase.pl script use the deleteditems and deletedbiblioitems
tables to get information for those items on the statistics table.

To reproduce:
- Have your sample DB contain some circulation data on the statistics table
- Make sure some of them have NULL itemtype:
> UPDATE statistics SET itemtype = NULL WHERE type='return';
- Check the upgrade query catches them:
> SELECT s.itemnumber, i.itype, b.itemtype FROM  ( SELECT DISTINCT itemnumber    FROM statistics    WHERE ( type = "return" OR type = "localuse" ) AND itemtype IS NULL ) s  LEFT JOIN  ( SELECT itemnumber,biblionumber, itype      FROM items    UNION    SELECT itemnumber,biblionumber, itype      FROM deleteditems ) i  ON (s.itemnumber=i.itemnumber)  LEFT JOIN  ( SELECT biblionumber, itemtype      FROM biblioitems    UNION    SELECT biblionumber, itemtype      FROM deletedbiblioitems ) b  ON (i.biblionumber=b.biblionumber);
+------------+-------+----------+
| itemnumber | itype | itemtype |
+------------+-------+----------+
|        732 | BK    | BK       |
|        731 | BK    | BK       |
+------------+-------+----------+
2 rows in set (0.00 sec)

- Delete the items, and some biblio too.
- Re-run the query
=> SUCCESS: Same results
- Go reset to NULL the itemtypes
> UPDATE statistics SET itemtype = NULL WHERE type='return';
- Run the updatedatabase.pl script:
 $ sudo koha-shell koahdev ; cd kohaclone
 $ perl installer/data/mysql/updatedatabase.pl
=> SUCCESS: No warnings

Note: It is possible that on production sites, if the sysadmin is cleaning the
deleted{items|biblioitems|biblio} tables, there will be warnings. This is expected
as they need to know some data lacks information.

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>
Comment 101 Tomás Cohen Arazi 2016-04-06 14:49:50 UTC
Comments 63, 64 and 65 addressed. Will now take a look at the discussion on bug 15599.
Comment 102 Jonathan Druart 2016-04-07 11:01:15 UTC
(I have squashed the patches for the review)

1/
C4/Circulation.pm
1868     my $biblio   = $item_level_itypes ? undef : GetBiblioData( $item->{ biblionumber } ); # don't get bib data unless we need it

Should not we use GetBiblioItemByBiblioNumber instead to avoid 2 left joins per return?

2/
  prove t/db_dependent/Items.t t/db_dependent/Circulation/Returns.t
displays warnings like
  item-level_itypes set but no itemtype set for item (Koha::Schema::Result::Item=HASH(0xce35808)->itemnumber) at Koha/Schema/Result/Item.pm line 662.
Comment 103 Jonathan Druart 2016-04-07 11:04:04 UTC
Created attachment 50018 [details] [review]
Bug 14598: Fix warning from effective_itemtype
Comment 104 Jonathan Druart 2016-04-07 11:04:29 UTC
(In reply to Jonathan Druart from comment #102)
> 2/
>   prove t/db_dependent/Items.t t/db_dependent/Circulation/Returns.t
> displays warnings like
>   item-level_itypes set but no itemtype set for item
> (Koha::Schema::Result::Item=HASH(0xce35808)->itemnumber) at
> Koha/Schema/Result/Item.pm line 662.

Fixed in last patch.
Comment 105 Marcel de Rooy 2016-04-25 11:12:02 UTC
It seems that question 1 on comment 102 has not been addressed.
Comment 106 Tomás Cohen Arazi 2016-04-25 15:58:41 UTC
I'm abandoning this bug. It went too far.

All warnings raised by this patchset (on another tests) are just proof that the tests scenarios the other tests used, were missing things (define an itemtype for a created item, for instance).

Those are all new bugs that need to be filled. I filled one and provided a patch. But this is really out of the scope of this bug. If someone fills those bugs, I will try to help fixing them. But I'm leaving this.
Comment 107 Jonathan Druart 2016-04-25 16:03:25 UTC
(In reply to Tomás Cohen Arazi from comment #106)
> I'm abandoning this bug. It went too far.

C'mon!!! :)

Question 1 on comment 102 was

> C4/Circulation.pm
> 1868     my $biblio   = $item_level_itypes ? undef : GetBiblioData( $item->{
> biblionumber } ); # don't get bib data unless we need it
> 
> Should not we use GetBiblioItemByBiblioNumber instead to avoid 2 left joins
> per return?

If you think I am right, I can provide a patch for that.
Comment 108 Tomás Cohen Arazi 2016-05-20 04:22:12 UTC
(In reply to Jonathan Druart from comment #107)
> (In reply to Tomás Cohen Arazi from comment #106)
> > I'm abandoning this bug. It went too far.
> 
> C'mon!!! :)
> 
> Question 1 on comment 102 was
> 
> > C4/Circulation.pm
> > 1868     my $biblio   = $item_level_itypes ? undef : GetBiblioData( $item->{
> > biblionumber } ); # don't get bib data unless we need it
> > 
> > Should not we use GetBiblioItemByBiblioNumber instead to avoid 2 left joins
> > per return?
> 
> If you think I am right, I can provide a patch for that.

You are right :-D
Comment 109 Jonathan Druart 2016-05-20 17:02:23 UTC
(In reply to Tomás Cohen Arazi from comment #108)
> (In reply to Jonathan Druart from comment #107)
> > (In reply to Tomás Cohen Arazi from comment #106)
> > > I'm abandoning this bug. It went too far.
> > 
> > C'mon!!! :)
> > 
> > Question 1 on comment 102 was
> > 
> > > C4/Circulation.pm
> > > 1868     my $biblio   = $item_level_itypes ? undef : GetBiblioData( $item->{
> > > biblionumber } ); # don't get bib data unless we need it
> > > 
> > > Should not we use GetBiblioItemByBiblioNumber instead to avoid 2 left joins
> > > per return?
> > 
> > If you think I am right, I can provide a patch for that.
> 
> You are right :-D

Forget that, GetBiblioItemByBiblioNumber has a weird return.
Comment 110 Jonathan Druart 2016-05-20 17:03:29 UTC
What about (really) deleted items?
The ones which is not in items or deleteditems table?
The update DB entry will raise the warning, but not sure it's relevant.
Comment 111 Jonathan Druart 2016-05-20 17:07:44 UTC
(In reply to Jonathan Druart from comment #110)
> What about (really) deleted items?
> The ones which is not in items or deleteditems table?
> The update DB entry will raise the warning, but not sure it's relevant.

Forget that, again.

But what about deleted items, the ones in the deleteditems table?
Comment 112 Jonathan Druart 2016-05-20 17:10:28 UTC
Before pushing this, it could be great to execute the update DB entry on a large DB, which has been updated from several Koha versions.
Does anyone have that on hand?
Comment 113 Tomás Cohen Arazi 2016-06-29 23:04:27 UTC
I have a proposal to un-sink this bug. Can we provide a fix_action_logs.pl maintenance script with a --itemtypes option switch (that could be extended in the future) to do the action_logs update, and have the updatadatabase.pl script JUST ALERT the user about the situation?

Jonathan: would that mitigate your concerns?
Comment 114 Jonathan Druart 2016-07-03 14:50:04 UTC
(In reply to Tomás Cohen Arazi from comment #113)
> I have a proposal to un-sink this bug. Can we provide a fix_action_logs.pl
> maintenance script with a --itemtypes option switch (that could be extended
> in the future) to do the action_logs update, and have the updatadatabase.pl
> script JUST ALERT the user about the situation?
> 
> Jonathan: would that mitigate your concerns?

Which concerns? I just asked someone to test the update DB entry on a "real" DB.
I was ready to pass QA on this one actually...
Comment 115 Fridolin Somers 2016-07-11 07:01:38 UTC
(In reply to Jonathan Druart from comment #112)
> Before pushing this, it could be great to execute the update DB entry on a
> large DB, which has been updated from several Koha versions.
> Does anyone have that on hand?

I'm gone try asap
Comment 116 Marcel de Rooy 2016-10-28 07:08:33 UTC
QA: Looking here now.
Comment 117 Marcel de Rooy 2016-10-28 08:07:57 UTC
Created attachment 56941 [details] [review]
Bug 14598: (regression tests) AddReturn should store itemtype on 'statistics' table

This patch adds tests for C4::Circulation::AddReturn(). Both tests are wrapped
inside a subtest, and look for AddReturn storing the right itemtype on the
'statistics' table.

Note: It also refactors the file a bit, to avoid side effects. And uses TestBuilder
to make it independent on already-present db data. It also removes warnings by mocking
C4::Context::userenv

To test:
- Apply the patch
- Run the tests:
  $ prove t/db_dependent/Circulation/Returns.t
=> FAIL: Tests fail bacause AddReturn is not storing the itemtype
- Sign off :-D

Sponsored-by: Universidad Empresarial Siglo 21

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 118 Marcel de Rooy 2016-10-28 08:08:03 UTC
Created attachment 56942 [details] [review]
Bug 14598: Make C4::Circulation::AddReturn store the right itemtype

This patch makes C4::Circulation::AddReturn correctly store the itemtype
on the 'statistics' table.

To reproduce:
- Checkout master.
- Make a checkout.
- Check the 'statistics' table and notice the itemtype is correctly set
  > SELECT * FROM statistics;
- Check the item in.
- Check the 'statistics' table and notice the itemtype is not set
  > SELECT * FROM statistics WHERE type="return";
=> FAIL: itemtype is set to NULL

To test:
- Apply the regression tests patch
- Run the tests:
  $ prove t/db_dependent/Circulation/Returns.t
=> FAIL: Tests fail
- Apply this patch
- Run the regression tests:
  $ prove t/db_dependent/Circulation/Returns.t
=> SUCCESS: Tests now pass.
- Repeat the 'To reproduce' steps
=> SUCCESS: itemtype is now correctly set (in real life)
- Happily sign off :-D

Sponsored-by: Universidad Empresarial Siglo 21

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 119 Marcel de Rooy 2016-10-28 08:08:09 UTC
Created attachment 56943 [details] [review]
Bug 14598: (QA followup) Don't die on bad barcode

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 120 Marcel de Rooy 2016-10-28 08:08:14 UTC
Created attachment 56944 [details] [review]
Bug 14598: (DB update) fix NULL itemtypes in statistics on 'return' rows

This patch introduces an updatedatabase.pl entry that takes care of
updating existing statistics rows.

It does so by looping on the statistics rows, collecting itemnumber occurences
that are have NULL itemtypes.
It then chooses the right itemtype following what is proposed on bug 14651, and
then updates the rows in statistics using the calculated itemtype.

Regards

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 121 Marcel de Rooy 2016-10-28 08:08:20 UTC
Created attachment 56945 [details] [review]
Bug 14598 - QA Followup touching CanBookBeIssued subroutine

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 122 Marcel de Rooy 2016-10-28 08:08:26 UTC
Created attachment 56946 [details] [review]
Bug 14598 [QA Followup] - Only fetch bib data if needed

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 123 Marcel de Rooy 2016-10-28 08:08:31 UTC
Created attachment 56947 [details] [review]
Bug 14598 [QA Followup] - Correct the behavior of GetItem

Currently GetItem sets itemtype to the biblio itemtype if no item level
itemtype exists. Instead, it should only do this if item_level-itypes
is not set.

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 124 Marcel de Rooy 2016-10-28 08:08:36 UTC
Created attachment 56948 [details] [review]
Bug 14598 [QA Followup] - Make unit test pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 125 Marcel de Rooy 2016-10-28 08:08:42 UTC
Created attachment 56949 [details] [review]
Bug 14598 [QA Followup] - Update localuse statistics also

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 126 Marcel de Rooy 2016-10-28 08:08:47 UTC
Created attachment 56950 [details] [review]
Bug 14598: (followup) Remove unused and non-existent C4::ItemType include

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 127 Marcel de Rooy 2016-10-28 08:08:52 UTC
Created attachment 56951 [details] [review]
Bug 14598: (QA followup) use deleted{items|biblioitems} during upgrade

If the 'statistics' table contains circulation information about items
that are no longer on the DB it will raise a warning. This patch
makes the updatedatabase.pl script use the deleteditems and deletedbiblioitems
tables to get information for those items on the statistics table.

To reproduce:
- Have your sample DB contain some circulation data on the statistics table
- Make sure some of them have NULL itemtype:
> UPDATE statistics SET itemtype = NULL WHERE type='return';
- Check the upgrade query catches them:
> SELECT s.itemnumber, i.itype, b.itemtype FROM  ( SELECT DISTINCT itemnumber    FROM statistics    WHERE ( type = "return" OR type = "localuse" ) AND itemtype IS NULL ) s  LEFT JOIN  ( SELECT itemnumber,biblionumber, itype      FROM items    UNION    SELECT itemnumber,biblionumber, itype      FROM deleteditems ) i  ON (s.itemnumber=i.itemnumber)  LEFT JOIN  ( SELECT biblionumber, itemtype      FROM biblioitems    UNION    SELECT biblionumber, itemtype      FROM deletedbiblioitems ) b  ON (i.biblionumber=b.biblionumber);
+------------+-------+----------+
| itemnumber | itype | itemtype |
+------------+-------+----------+
|        732 | BK    | BK       |
|        731 | BK    | BK       |
+------------+-------+----------+
2 rows in set (0.00 sec)

- Delete the items, and some biblio too.
- Re-run the query
=> SUCCESS: Same results
- Go reset to NULL the itemtypes
> UPDATE statistics SET itemtype = NULL WHERE type='return';
- Run the updatedatabase.pl script:
 $ sudo koha-shell koahdev ; cd kohaclone
 $ perl installer/data/mysql/updatedatabase.pl
=> SUCCESS: No warnings

Note: It is possible that on production sites, if the sysadmin is cleaning the
deleted{items|biblioitems|biblio} tables, there will be warnings. This is expected
as they need to know some data lacks information.

Signed-off-by: Tomas Cohen Arazi <tomascohen@unc.edu.ar>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 128 Marcel de Rooy 2016-10-28 08:08:58 UTC
Created attachment 56952 [details] [review]
Bug 14598: Fix warning from effective_itemtype

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 129 Marcel de Rooy 2016-10-28 08:09:03 UTC
Created attachment 56953 [details] [review]
Bug 14598: [QA Follow-up] Small changes

[1] Renames the dbrev version to XXX; adding a my for $sth, although
    not strictly necessary it feels better.
[2] Circulation.t stumbles over:
    Undefined subroutine &C4::Circulation::GetItem called at ... line 1283.
    Somehow this is related to use_ok instead of a regular use.
    Worked around this by require_ok and adding a regular use.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 130 Marcel de Rooy 2016-10-28 08:16:17 UTC
Code looks good to me.

> it could be great to execute the update DB entry on a large DB
Months passed by in the meantime and nothing happened. I think that some more test coverage on the smaller changes in essential circulation routines is crucial too now.

As expected, the dbrev warns on items too that are not found in both items as well as deleteditems. (We could probably do without them..)

Several tests are flooded now with:
item-level_itypes set but no itemtype set for item (2144749608) at /usr/share/koha/masterclone/Koha/Schema/Result/Item.pm line 698.
Especially Circulation.t
Note that this comes from creating items without itemtype in these tests. We should adjust those tests.
In the actual logs I do not hope/expect to see this warning ;)

Passed QA

RM: Push now or wait. The dbrev is already in updatedatabase.
Comment 131 Kyle M Hall 2016-10-28 11:31:46 UTC
Pushed to master for 16.11, thanks Tomas, Jonathan, Brendan!
Comment 132 Fridolin Somers 2016-11-02 12:29:09 UTC
(In reply to Marcel de Rooy from comment #130)
> Passed QA

Thanks a lot for your job, I'have not find the time to work en this but I will have a close look at it when upgrading our machines.

Best regards,
Comment 133 Julian Maurice 2016-11-30 13:01:42 UTC
Pushed to 3.22.x, will be in 3.22.13
Comment 134 Mason James 2016-12-14 10:32:45 UTC
Pushed to 16.05.x, for 16.05.06 release
Comment 135 Gwendal JONCOUR 2017-04-18 13:59:51 UTC
Here in Université Rennes 2, France, we tried on a dump of our production, with statistics from 2012 to now. For the numbers, we have approximately 2.4M lines in statistics table (300K distinct itemnumbers from return stats with no itemtype set), 900K lines in items, 46K in deleteditems, 500K lines in biblioitems and 70K in deletedbiblioitems.

Despite days running the updatedatabase.pl script, the first query of the 16.06.00.039 update keep stuck in "sending data" state. We tried to upgrade our test server to equivalent to our production server (12 cores and 48GB of RAM) with no more result. We tried also to reduce our statistics to keep only those from 2015 to now but the query keep stuck.

Is there anybody who succeed to apply the 16.06.00.039 update script to a production equivalent database ?
Comment 136 Jonathan Druart 2017-04-21 16:31:40 UTC
Created attachment 62539 [details]
Script to set itemtype on stats
Comment 137 Jonathan Druart 2017-04-21 16:32:23 UTC
Gwendal, Can try this script in a development environment and tell us if it works?
Comment 138 Jonathan Druart 2017-04-21 16:33:36 UTC
(In reply to Jonathan Druart from comment #137)
> Gwendal, Can try this script in a development environment and tell us if it
> works?

Forget that, it will not change anything.
Comment 139 Jonathan Druart 2017-04-21 17:01:28 UTC
Created attachment 62555 [details]
Script to set itemtype on stats
Comment 140 Jonathan Druart 2017-04-21 17:02:25 UTC
(In reply to Jonathan Druart from comment #139)
> Created attachment 62555 [details]
> Script to set itemtype on stats

Gwendal, let's try this one. It should be take hours too but should not stuck in "sending data" state.
Comment 141 Gwendal JONCOUR 2017-04-25 14:09:09 UTC
Created attachment 62682 [details]
Set itemtype on stats - Rennes 2 version

Well... I start to run your script on our database and when I checked the mysql queries called in the loop, I saw they took 70s each to be executed. Extrapolating from that, the 300k loops will take a little more than eight months to run. It will be a pretty long downtime for a production update :-\

I make my own version of this update function looping on itemtype and call serately items tables and biblioitems tables. On our database, it took 1h20m to update (for info, we have "item-level_itypes" parameter on so the script rarely call the biblioitem query).
Comment 142 Andreas Hedström Mace 2017-08-31 09:10:07 UTC
Has anything more been done about this? The last comment by Gwendal doesn't sound promising, and will make updates difficult for any library with a large database.
Comment 143 Fridolin Somers 2017-09-12 14:29:35 UTC
(In reply to Gwendal JONCOUR from comment #135)
> Is there anybody who succeed to apply the 16.06.00.039 update script to a
> production equivalent database ?

We had nearly all our clients successfully upgraded from 3.22 to 16.11.
It took sometimes an hour or two on large/old databases.