Bug 14598

Is it a duplicate of bug 9351?

I would say we need to update existing rows.

*** Bug 9351 has been marked as a duplicate of this bug. ***

*** Bug 14531 has been marked as a duplicate of this bug. ***

(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?

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?

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?

(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. ☺

It does not look to depend on bug 14651

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.

(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

What about comments 23&26?

Patches rebased to master.

(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 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 :(

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.)

(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.

(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.

It took me a minute for what you wrote to sink in. Still, we should address the issue on a separate bug report.

Unit tests fixes submitted on bug 15599!

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.

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)

(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?

(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.

(In reply to Jonathan Druart from comment #50)
> No, it does not update localuse.

This followup should take care of that!

Could someone signoff the last 5 patches please?

We need to fix bug 15599 first.

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?

It seems that I get the warning when an item does not exist.
Maybe we should check the deleteditems table.

Can you quickly explain how the warn is caused?

Missed your last comment, sorry! We usually use deleteditems for statistics too - so it makes sense to include it I think.

In discussion, blocked or fail qa?
I let you choose the status...

Patches rebased so they apply on master.

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>

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>

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>

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>

Created attachment 49982 [details] [review]
Bug 14598 - QA Followup touching CanBookBeIssued subroutine

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

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>

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>

Created attachment 49985 [details] [review]
Bug 14598 [QA Followup] - Make unit test pass

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

Created attachment 49986 [details] [review]
Bug 14598 [QA Followup] - Update localuse statistics also

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

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>

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>

Comments 63, 64 and 65 addressed. Will now take a look at the discussion on bug 15599.

(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.

Created attachment 50018 [details] [review]
Bug 14598: Fix warning from effective_itemtype

(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.

It seems that question 1 on comment 102 has not been addressed.

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.

(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.

(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

(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.

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.

(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?

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 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?

(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...

(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

QA: Looking here now.

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

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>

Created attachment 56952 [details] [review]
Bug 14598: Fix warning from effective_itemtype

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

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>

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.

Pushed to master for 16.11, thanks Tomas, Jonathan, Brendan!

(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,

Pushed to 3.22.x, will be in 3.22.13

Pushed to 16.05.x, for 16.05.06 release

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 ?

Created attachment 62539 [details]
Script to set itemtype on stats

Gwendal, Can try this script in a development environment and tell us if it works?

(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.

Created attachment 62555 [details]
Script to set itemtype on stats

(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.

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).

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.

(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.