Bug 9351 - item type not recorded correctly in statistics for returns and some local use
Summary: item type not recorded correctly in statistics for returns and some local use
Status: RESOLVED DUPLICATE of bug 14598
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Target Milestone: ---
Assignee: Fridolin SOMERS
QA Contact:
URL:
Whiteboard:
Keywords:
: 13524 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-01-05 00:31 UTC by Galen Charlton
Modified: 2016-01-19 04:52 UTC (History)
14 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed patch (1.49 KB, patch)
2013-05-31 14:14 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Proposed patch (8.27 KB, patch)
2013-06-03 12:39 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Proposed patch (revised) (12.83 KB, patch)
2013-06-03 13:03 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Proposed patch (12.83 KB, patch)
2013-06-06 10:02 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 9351 - item type not recorded correctly in statistics for returns and some local use (1.85 KB, patch)
2013-12-19 17:01 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 9351 - item type not recorded correctly in statistics for returns and some local use (1.77 KB, patch)
2014-12-29 16:36 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 9351 - sql update on existing statistics (1.32 KB, patch)
2014-12-29 16:36 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9351 - item type not recorded correctly in statistics for returns and some local use (1.84 KB, patch)
2015-01-07 12:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 9351 - sql update on existing statistics (1.40 KB, patch)
2015-01-07 12:04 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 9351 - item type not recorded correctly in statistics for returns and some local use - followup (2.00 KB, patch)
2015-01-28 13:17 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
[SIGNED OFF] Bug 9351 - item type not recorded correctly in statistics for returns and some local use (1.89 KB, patch)
2015-02-06 22:23 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF] Bug 9351 - sql update on existing statistics (1.43 KB, patch)
2015-02-06 22:23 UTC, Nick Clemens
Details | Diff | Splinter Review
[SIGNED OFF] Bug 9351 - item type not recorded correctly in statistics for returns and some local use - followup (2.06 KB, patch)
2015-02-06 22:23 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 9351 - item type not recorded correctly in statistics for returns and some local use (1.87 KB, patch)
2015-05-24 12:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9351 - sql update on existing statistics (1.54 KB, patch)
2015-05-24 12:43 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 9351 - item type not recorded correctly in statistics for returns and some local use - followup (2.04 KB, patch)
2015-05-24 12:43 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Galen Charlton 2013-01-05 00:31:03 UTC
When returning a loan, or when returning an item for local use when the RecordLocalUseOnReturn syspref is on, the corresponding entry in the statistics table takes the item type from the bib level.

This is incorrect when item-level item types is enabled, and among other consequences can lead to problematic local use reports from the circ statistics wizard.  In particular, if the library is also doing checkouts to a stats-only borrower to record local use, a circ statistics report on local uses that includes the item type as a row or column can have nonsensical totals because of the NULL statistics.itemtype value.

The converse problem affects sites that use biblio-level item types, since local uses recorded from stats-only patrons use the item-level item type regardless of the setting of item-level_itypes.

The desired behavior is that when recording a local use, the recorded item type should respect the item-level_itypes system preference.
Comment 1 Galen Charlton 2013-01-05 00:41:42 UTC
Fortunately, after the code is fixed, the statistics table can be readily corrected.  The following two statements should do it when item-level item types are in effect:

UPDATE statistics s
SET itemtype = (
  SELECT itype
  FROM items i
  WHERE i.itemnumber = s.itemnumber
)
WHERE itemtype IS NULL
AND type IN ('localuse', 'return')
AND itemnumber IN (SELECT itemnumber FROM items);

UPDATE statistics s
SET itemtype = (
  SELECT itype
  FROM deleteditems i
  WHERE i.itemnumber = s.itemnumber
)
WHERE itemtype IS NULL
AND type IN ('localuse', 'return')
AND itemnumber IN (SELECT itemnumber FROM deleteditems);

The following three statements should do it when bib-level item type are in effect:

UPDATE statistics s
SET itemtype = (
  SELECT itemtype
  FROM (
    SELECT itemnumber, itemtype
	FROM items
	JOIN biblioitems USING (biblioitemnumber)
  ) i
  WHERE i.itemnumber = s.itemnumber
)
WHERE itemtype IS NULL
AND type IN ('localuse', 'return')
AND itemnumber IN (
  SELECT itemnumber 
  FROM items
  JOIN biblioitems USING (biblioitemnumber)
);

UPDATE statistics s
SET itemtype = (
  SELECT itemtype
  FROM (
    SELECT itemnumber, itemtype
	FROM deleteditems
	JOIN biblioitems USING (biblioitemnumber)
  ) i
  WHERE i.itemnumber = s.itemnumber
)
WHERE itemtype IS NULL
AND type IN ('localuse', 'return')
AND itemnumber IN (
  SELECT itemnumber 
  FROM deleteditems 
  JOIN biblioitems USING (biblioitemnumber)
);

UPDATE statistics s
SET itemtype = (
  SELECT itemtype
  FROM (
    SELECT itemnumber, itemtype
	FROM deleteditems
	JOIN deletedbiblioitems USING (biblioitemnumber)
  ) i
  WHERE i.itemnumber = s.itemnumber
)
WHERE itemtype IS NULL
AND type IN ('localuse', 'return')
AND itemnumber IN (
  SELECT itemnumber 
  FROM deleteditems 
  JOIN deletedbiblioitems USING (biblioitemnumber)
);
Comment 2 Fridolin SOMERS 2013-05-31 14:14:51 UTC Comment hidden (obsolete)
Comment 3 Fridolin SOMERS 2013-05-31 14:17:36 UTC
I set the importance to major because statistics are very important for libraries.
Comment 4 Galen Charlton 2013-05-31 14:45:09 UTC
(In reply to comment #2)
> Created attachment 18545 [details] [review] [review]
> Proposed patch
> 
> See commit message

Upon reading the patch, I think there's a problem -- it doesn't take the item-level_itypes system preference into account.
Comment 5 Fridolin SOMERS 2013-05-31 16:31:46 UTC
Indeed, but it works for both item and biblio level : 
In C4::Circualtion::AddIssue : 
  my $item = GetItem('', $barcode)
and C4::Items::GetItem sets itype with itemtype from items or biblioitems if needed.
So $items->{'itype'} is used for both item and biblio level, like in many other places in C4::Circualtion.

Maybe C4::Items::GetItem should use item-level_itypes system preference, but this would be a very sensitive modification with a huge test plan, it shall be for another bug.
Comment 6 Galen Charlton 2013-05-31 17:00:29 UTC
(In reply to comment #5)
> Indeed, but it works for both item and biblio level : 
> In C4::Circualtion::AddIssue : 
>   my $item = GetItem('', $barcode)
> and C4::Items::GetItem sets itype with itemtype from items or biblioitems if
> needed.

Please take a look at CanBookBeIssued -- it fetches the item via GetItem, but also fetches the biblioitem and explicitly checks item-level_itypes.  

What GetItem does when fetchingitype isn't quite the same: it doesn't check the syspref, but it also assumes that one should fall back to the biblio item type if the item-level item type is blank.  I'm not quite sure that's the correct behavior, but that discussion certainly belongs on a separate bug.

> So $items->{'itype'} is used for both item and biblio level, like in many
> other places in C4::Circualtion.

But many other places in C4::Circulation explicitly check item-level_type, and I think the fix for this should be doing that explicit check.
Comment 7 Fridolin SOMERS 2013-06-03 12:39:08 UTC Comment hidden (obsolete)
Comment 8 Fridolin SOMERS 2013-06-03 13:03:37 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2013-06-04 06:20:42 UTC
I think the point of having the itemtype on item level is that you often have several different itemtypes on one record - like some items are for reference, overnight loan, or normal loan. So the statistics would just be wrong and missing the interesting bit, if we don't look at item level here.
Comment 10 Fridolin SOMERS 2013-06-04 16:13:41 UTC
(In reply to comment #9)
And do you agree with the patch ?
Comment 11 Pierre Angot 2013-06-06 06:46:51 UTC
Sandbox setup by tredok.pierre@gmail.com with database 2 and bug 9351 on Thu Jun 6 08:43:00 2013

Something went wrong !
Applying: Bug 9351 - item type not recorded correctly in statistics for returns and some local use
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging C4/Circulation.pm
CONFLICT (content): Merge conflict in C4/Circulation.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 9351 - item type not recorded correctly in statistics for returns and some local use
When you have resolved this problem run "git am --resolved".
If you would prefer to skip this patch, instead run "git am --skip".
To restore the original branch and stop patching run "git am --abort".
Bug 9351 - item type not recorded correctly in statistics for returns and some local use

Proposed patch (revised)
Apply? [yn]
Patch left in /tmp/Proposed-patch-revised-jhptam.patch
Comment 12 Fridolin SOMERS 2013-06-06 10:02:33 UTC Comment hidden (obsolete)
Comment 13 Kyle M Hall 2013-06-21 13:02:32 UTC
At step one, when I was able to check out a Not For Loan item to a patron. I switched back to master and tried again, and my checkout was blocked correctly.
Comment 14 Fridolin SOMERS 2013-12-19 17:01:23 UTC Comment hidden (obsolete)
Comment 15 Fridolin SOMERS 2013-12-19 17:04:30 UTC
I propose that this patch focuses on the problem with statitics on return.
Note that Bug 9532 followup manages GetItem problem.
Comment 16 Chris Cormack 2013-12-22 20:33:58 UTC
Comment on attachment 23692 [details] [review]
Bug 9351 - item type not recorded correctly in statistics for returns and some local use

Review of attachment 23692 [details] [review]:
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +1897,4 @@
>      UpdateStats(
>          $branch, $stat_type, '0', '',
>          $item->{'itemnumber'},
> +        $item->{'itype'},

Shouldn't this be checking the itemlevel itemtypes system preference? 
Otherwise this is going to break for any libraries using biblioitem level itemtypes
Comment 17 Chris Cormack 2013-12-22 20:40:03 UTC
Without 9352 (which is failed QA) we cant push this as GetItem does not respect the systempreference
Comment 18 Fridolin SOMERS 2013-12-23 09:12:57 UTC
(In reply to Chris Cormack from comment #16)
> Comment on attachment 23692 [details] [review] [review]
> Bug 9351 - item type not recorded correctly in statistics for returns and
> some local use
> 
> Review of attachment 23692 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Circulation.pm
> @@ +1897,4 @@
> >      UpdateStats(
> >          $branch, $stat_type, '0', '',
> >          $item->{'itemnumber'},
> > +        $item->{'itype'},
> 
> Shouldn't this be checking the itemlevel itemtypes system preference? 
> Otherwise this is going to break for any libraries using biblioitem level
> itemtypes
No, because $item comes from GetItem() and this method sets $item->{'itype'} with biblioitems.itemtype if bibliolevel itemtypes.
Comment 19 Fridolin SOMERS 2013-12-23 09:14:41 UTC
(In reply to Chris Cormack from comment #17)
> Without 9352 (which is failed QA) we cant push this as GetItem does not
> respect the systempreference
I bet you meen Bug 9532 ;-)
Comment 20 Chris Cormack 2013-12-23 10:17:56 UTC
(In reply to Fridolin SOMERS from comment #18)
> (In reply to Chris Cormack from comment #16)
> > Comment on attachment 23692 [details] [review] [review] [review]
> > Bug 9351 - item type not recorded correctly in statistics for returns and
> > some local use
> > 
> > Review of attachment 23692 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: C4/Circulation.pm
> > @@ +1897,4 @@
> > >      UpdateStats(
> > >          $branch, $stat_type, '0', '',
> > >          $item->{'itemnumber'},
> > > +        $item->{'itype'},
> > 
> > Shouldn't this be checking the itemlevel itemtypes system preference? 
> > Otherwise this is going to break for any libraries using biblioitem level
> > itemtypes
> No, because $item comes from GetItem() and this method sets $item->{'itype'}
> with biblioitems.itemtype if bibliolevel itemtypes.

No, it uses the item level one if it exists and falls back to the biblio level one if it doesn't. It doesn't respect the systempreference. And won't unless bug 9532 is fixed to a point that it can be pushed.
Comment 21 Fridolin SOMERS 2013-12-31 11:39:24 UTC
In fact, the problem of GetItem() is not the main concern of this bug and it seems to be a very complex problem.
I've created Bug 11463 to focus on it.
Comment 22 Fridolin SOMERS 2013-12-31 11:44:00 UTC
I have set 11463 as dependency, but actually, other operations like checkout uses GetItem() and then $item->{itype} in UpdateStats().
So this patch simply sets return operation like other operations.
Comment 23 mathieu saby 2013-12-31 12:08:20 UTC
I did not read all the comments, but for some libraries, what is needed in statistical table is BOTH items.itype and biblioitems.itemtype, and maybe a clear indication (a 3rd column "circulation_type" ?) of which type is was used for circulation rights.

Ex:
In Rennes 2 we use item level for defining circulation rules.
items.itype are codes for "Short Loan", "Normal Loan", "Long loan".
biblioitems.itemtype are "Book", "Serial", "DVD" etc.

For our annual report, we need to know the number of Books, Serials, DVD checked out, even if the type used for defining circulation rules are "Short loan", and not "Book".

But other libraries may have different settings, and may have need to make stats on item level, or maybe cross stats on both level.


M. Saby
Comment 24 Katrin Fischer 2014-01-02 07:54:32 UTC
Maybe a configurable statistics field and itemtype according to the pref could work too? This would give more flexibility to more uses cases and could be solved outside of this bug.
Comment 25 Fridolin SOMERS 2014-01-09 09:25:14 UTC
(In reply to mathieu saby from comment #23)
> I did not read all the comments, but for some libraries, what is needed in
> statistical table is BOTH items.itype and biblioitems.itemtype, and maybe a
> clear indication (a 3rd column "circulation_type" ?) of which type is was
> used for circulation rights.
Very good idea. I think more information will be better for many libraries.
But in another bug because it will be an enhancement.
Comment 26 Jonathan Druart 2014-01-15 11:34:54 UTC
The patch should fix the existing rows in the statistics table.
Marked as Failed QA.
Comment 27 mathieu saby 2014-01-22 08:51:15 UTC
(In reply to Fridolin SOMERS from comment #25)
> (In reply to mathieu saby from comment #23)
> > I did not read all the comments, but for some libraries, what is needed in
> > statistical table is BOTH items.itype and biblioitems.itemtype, and maybe a
> > clear indication (a 3rd column "circulation_type" ?) of which type is was
> > used for circulation rights.
> Very good idea. I think more information will be better for many libraries.
> But in another bug because it will be an enhancement.

See bug 11594
Comment 28 Jonathan Druart 2014-12-11 10:57:46 UTC
How could we move things forward here?
Comment 29 Fridolin SOMERS 2014-12-11 12:54:08 UTC
(In reply to Jonathan Druart from comment #28)
> How could we move things forward here?

In my opinion, this bug opened a big discussion but the main purpose was to fix a specific bug on statistics.
We should integrate this patch and continue discussion in a new bug or in kohadevel.
Comment 30 Jonathan Druart 2014-12-11 12:58:15 UTC
And what about updating existing DB values (comment 26)?
Comment 31 David Cook 2014-12-16 23:48:20 UTC
I absolutely agree with Fridolin on this one.

If you look at every other example of "UpdateStats()" in C4::Circulation, you'll see that it uses $item->{itype} (the two bad examples being $biblio->{itemtype} and $item->{itemtype}).

While it might be an idea to correct the behaviour to take into account "item-level_itypes", I think that's really outside the scope of this bug. This bug is just to create consistency in the usage of UpdateStats() within C4::Circulation.

As for updating existing statistics... I think that's a very difficult idea:

1) If we update existing statistics without telling librarians, we're going to cause huge headaches as their old statistics will be invalidated and their new statistics will likely be very different than expected (even though they're more accurate).

2) If we don't update the statistics, we cause huge headaches as the statistics will be inconsistent. While fixing this in December seems to get around this a bit, you never know when someone is going to apply a fix or switch to a fixed version, so that cannot be banked on.

So... I don't know what the right thing to do here is. I think we probably need to update existing statistics... but we need to be very explicit about that. As Fridolin mentioned, libraries rely heavily on statistics, so if we're meddling with their data, we need to make sure that they _know_ that we're doing it.

Of course, I don't know the best way to do that either, as it might be easy to miscommunicate about this. Maybe... maybe we should update the statistics with this patch, and the release manager and release maintainers should put a very noticeable message in the release notes. In that way, the onus of passing information on to librarian is - quite understandably - on the sysadmin.

Fridolin, if you add some SQL to update the statistics, I'd be happy to sign off on this one.
Comment 32 David Cook 2014-12-17 02:28:49 UTC
I just tried out the following:

UPDATE statistics
SET itemtype =
(select u.itype from
  (select itype,itemnumber from items
   UNION ALL
   select itype,itemnumber from deleteditems
   ) u
where statistics.itemnumber = u.itemnumber
)
WHERE type IN ('localuse', 'return')

This will re-write the "itemtype" for all 'localuse' and 'return' existing statistical entries. 

I checked its accuracy with the following:

SELECT statistics.*, u.itype
from statistics
JOIN (
select * from items
UNION ALL
select * from deleteditems
) u ON u.itemnumber = statistics.itemnumber
WHERE type IN ('localuse', 'return')
order by datetime
;

Looks all right to me.
Comment 33 Fridolin SOMERS 2014-12-29 16:36:37 UTC Comment hidden (obsolete)
Comment 34 Fridolin SOMERS 2014-12-29 16:36:53 UTC Comment hidden (obsolete)
Comment 35 Fridolin SOMERS 2014-12-29 16:41:03 UTC
(In reply to David Cook from comment #31)
> Fridolin, if you add some SQL to update the statistics, I'd be happy to sign
> off on this one.

Here it is.
I've added a "LIMIT 1" to the query in the unlikely case the same itemnumber exists in items and deleteditems.

I've also rebased first patch.
Comment 36 Kyle M Hall 2015-01-07 11:42:27 UTC
*** Bug 13524 has been marked as a duplicate of this bug. ***
Comment 37 Kyle M Hall 2015-01-07 12:04:52 UTC Comment hidden (obsolete)
Comment 38 Kyle M Hall 2015-01-07 12:04:59 UTC Comment hidden (obsolete)
Comment 39 Katrin Fischer 2015-01-27 21:00:35 UTC
Hm, bit confused. 

I tested checkouts and returns with itemtype on the item level. 
For returns the itype column in statistics is set to NULL.

Can you please check?
Comment 40 David Cook 2015-01-27 22:18:48 UTC
(In reply to Katrin Fischer from comment #39)
> Hm, bit confused. 
> 
> I tested checkouts and returns with itemtype on the item level. 
> For returns the itype column in statistics is set to NULL.
> 
> Can you please check?

Nice catch, Katrin!

This block:

@@ -1988,7 +1988,7 @@ sub AddReturn {
                 branch => $branch,
                 type => $stat_type,
                 itemnumber => $item->{'itemnumber'},
-                itemtype => $biblio->{'itemtype'},
+                itemtype => $biblio->{'itype'},
                 borrowernumber => $borrowernumber,
                 ccode => $item->{'ccode'}}
     );

Should actually be like this:

@@ -1988,7 +1988,7 @@ sub AddReturn {
                 branch => $branch,
                 type => $stat_type,
                 itemnumber => $item->{'itemnumber'},
-                itemtype => $biblio->{'itemtype'},
+                itemtype => $item->{'itype'},
                 borrowernumber => $borrowernumber,
                 ccode => $item->{'ccode'}}
     );

There's no such thing as $biblio->{'itype'}.
Comment 41 Fridolin SOMERS 2015-01-28 13:17:29 UTC Comment hidden (obsolete)
Comment 42 Fridolin SOMERS 2015-01-28 13:19:24 UTC
(In reply to David Cook from comment #40)
> (In reply to Katrin Fischer from comment #39)
> > Hm, bit confused. 
> > 
> > I tested checkouts and returns with itemtype on the item level. 
> > For returns the itype column in statistics is set to NULL.
> > 
> > Can you please check?
> 
> Nice catch, Katrin!
> 
> This block:
> 
> @@ -1988,7 +1988,7 @@ sub AddReturn {
>                  branch => $branch,
>                  type => $stat_type,
>                  itemnumber => $item->{'itemnumber'},
> -                itemtype => $biblio->{'itemtype'},
> +                itemtype => $biblio->{'itype'},
>                  borrowernumber => $borrowernumber,
>                  ccode => $item->{'ccode'}}
>      );
> 
> Should actually be like this:
> 
> @@ -1988,7 +1988,7 @@ sub AddReturn {
>                  branch => $branch,
>                  type => $stat_type,
>                  itemnumber => $item->{'itemnumber'},
> -                itemtype => $biblio->{'itemtype'},
> +                itemtype => $item->{'itype'},
>                  borrowernumber => $borrowernumber,
>                  ccode => $item->{'ccode'}}
>      );
> 
> There's no such thing as $biblio->{'itype'}.

Indeed.
This patch seems to live a lot.

I've added a followup to correct this.
In fact the $biblio in AddReturn is not used nor set anywhere.
Comment 43 Nick Clemens 2015-02-06 22:23:16 UTC Comment hidden (obsolete)
Comment 44 Nick Clemens 2015-02-06 22:23:27 UTC Comment hidden (obsolete)
Comment 45 Nick Clemens 2015-02-06 22:23:38 UTC Comment hidden (obsolete)
Comment 46 Katrin Fischer 2015-02-08 19:40:48 UTC
Ok, it works now alright for item-level itypes = specific item, but setting the syspref to bibio level it still takes the itype from the item level?
Comment 47 David Cook 2015-02-09 05:20:55 UTC
(In reply to Katrin Fischer from comment #46)
> Ok, it works now alright for item-level itypes = specific item, but setting
> the syspref to bibio level it still takes the itype from the item level?

Yes, but that's already the default behaviour for the other existing instances of UpdateStats() (e.g. issues and renewals).

I think requiring handling of biblio level itypes would be scope creep for this bug, since this bug is just trying to create consistency.

If we want to support biblio level itypes, I think that should be a separate bug which then fixes it across the board for all calls of UpdateStats().

But that's just my 2 cents :).
Comment 48 Katrin Fischer 2015-02-09 07:06:40 UTC
Hm, are you sure?

>This patch corrects this problem by getting itemtype from $item->{'itype'} >which comes from item or biblio level in GetItem().

Maybe the reason is that I had both and it only falls back if you have no item level itypes? Fridolin, could you take a quick look and confirm what is he correct default behaviour?

I would be really interesting to know if record level itypes still used by a lot of libraries.
Comment 49 David Cook 2015-02-11 23:48:51 UTC
(In reply to Katrin Fischer from comment #48)
> Hm, are you sure?
> 
> >This patch corrects this problem by getting itemtype from $item->{'itype'} >which comes from item or biblio level in GetItem().
> 
> Maybe the reason is that I had both and it only falls back if you have no
> item level itypes? Fridolin, could you take a quick look and confirm what is
> he correct default behaviour?
> 
> I would be really interesting to know if record level itypes still used by a
> lot of libraries.

Hehe. I'm never sure after you ask me if I'm sure ;).

Here's my quick review of the source:

--
Instances of UpdateStats():

1) AddReturn()
Currently uses: "$biblio->{'itemtype'}"
$item defined by: C4::GetItem()

This is interesting because $biblio actually appears to be null in AddReturn(). So it looks like all the stats for returns will probably have a null itemtype. (My claim should only be false if there's a function that modifies $biblio without taking it as an argument, which... would also be suboptimal.)


2) AddRenewal()
Currently uses: $item->{itype}
$item defined by: C4::GetItem()


3) CanBookBeIssued() 
Currently uses: $item->{'itemtype'}
$item defined by: C4::GetItem()

It appears that $item->{'itemtype'} is actually defined by $item->{'itype'}, so changing this should just improve readability. The values should be identical.

NOTE: This only uses UpdateStats() for statistical borrowers

4) AddIssue()
Currently uses: $item->{'itype'}
$item defined by: C4::GetItem()

--

In regards to C4::GetItem():

$item->{itype} will equal "biblioitems.itemtype" if "items.itype" is false.

Katrin: I don't know what that means exactly. I always use item-level item types. I assume this means that if one were to use biblio-level itemtypes, that items.itype would be false (ie null or blank) and thus biblioitems.itemtype would be places in $item->{itype}...

However, regardless of the behaviour in C4::GetItem(), using $item->{'itype'} when calling UpdateStats() should guarantee consistency. 

At the moment, it looks to me that AddRenewal(), CanBookBeIssued(), and AddIssue() are probably using item-level itemtypes... while AddReturn() is probably using a blank value (as UpdateStats will use a blank value if itemtype doesn't exist).
Comment 50 Katrin Fischer 2015-05-24 12:43:32 UTC
Created attachment 39456 [details] [review]
Bug 9351 - item type not recorded correctly in statistics for returns and some local use

When returning a loan, or when returning an item for local use, the corresponding entry in the statistics table takes the item type from the bib level. This is incorrect when item-level item types is enabled.

This patch corrects this problem by getting itemtype from $item->{'itype'} which comes from item or biblio level in GetItem().

Test plan :
- On a catalogue with itemtype on item level, perform a return an look at statistics table
- Idem for a catalogue with itemtype on biblio level

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Nick <nick@quecheelibrary.org>
Comment 51 Katrin Fischer 2015-05-24 12:43:36 UTC
Created attachment 39457 [details] [review]
Bug 9351 - sql update on existing statistics

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Nick <nick@quecheelibrary.org>
Comment 52 Katrin Fischer 2015-05-24 12:43:40 UTC
Created attachment 39458 [details] [review]
Bug 9351 - item type not recorded correctly in statistics for returns and some local use - followup

This patch corrects previous for AddReturn() :
-  $item->{'itype'} instead of $biblio->{'itype'}
-  and my $biblio is not set so it is useless
-  removes FIXME comment

Also adds a TODO comment in CanBookBeIssued() :
- $item->{'itemtype'}=$item->{'itype'}, this line may be useless now

Signed-off-by: Nick <nick@quecheelibrary.org>
Comment 53 Katrin Fischer 2015-05-24 12:44:51 UTC
I've rebased the patches - could someone please take another look and try to unravel any confusion? Thx!
Comment 54 M. Tompsett 2015-06-14 01:56:57 UTC
Where are the tests to prevent regression?
Comment 55 Nick Clemens 2015-06-25 20:34:13 UTC
Not sure what needs to be done here, more than a signoff? Failing because of Mark's test request
Comment 56 David Cook 2015-07-06 02:27:38 UTC
(In reply to Nick Clemens from comment #55)
> Not sure what needs to be done here, more than a signoff? Failing because of
> Mark's test request

I think Katrin just rebased the patches to keep them current. I think the signoff just needed another check.

Seeing as Mark isn't a member of the QA or release teams, I don't think it's necessary to fail the patch because of a lack of unit tests.

That said, tests never hurt, but I don't have time to write any right now, so leaving this as Failed QA.
Comment 57 Tomás Cohen Arazi 2015-08-05 13:10:45 UTC
I tried this patchset along with the regression tests I wrote on bug 14598 (because I missed this bug on my first search). The regression tests pass with this patchset.

But I'm not happy with this patchset, as it seems to touch CanBookBeIssued which is out of the scope of this bug, without regression tests for it (too).
Also, I'm not sure about the DB update, as it seems to update all returns item types, and it should only touch those with NULL values.

So, I propose we stick with bug 14598 (which is missing the DB update step) and mark this one as duplicate.
Comment 58 Fridolin SOMERS 2015-08-05 16:07:37 UTC
(In reply to Tomás Cohen Arazi from comment #57)
> But I'm not happy with this patchset, as it seems to touch CanBookBeIssued
This code has the same problem as AddReturn since it calls UpdateStats with $item->{'itemtype'} instead of $item->{'itype'}. Only it works because there is a dirty hack $item->{'itemtype'}=$item->{'itype'};.
One day maybe this hack will be removed.
Once there is a full abstraction layer with datas surely.

> So, I propose we stick with bug 14598 (which is missing the DB update step)
> and mark this one as duplicate.
+1
Since AddReturn part is much more important.
Comment 59 Tomás Cohen Arazi 2015-08-05 18:29:34 UTC

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