http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Galen Charlton <gmcharlt@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |gmcharlt@gmail.com |ity.org | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Fridolyn SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|gmcharlt@gmail.com |fridolyn.somers@biblibre.co | |m --- Comment #2 from Fridolyn SOMERS <fridolyn.somers@biblibre.com> --- Created attachment 18545 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18545&action=edit Proposed patch See commit message -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Fridolyn SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |major -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 --- Comment #4 from Galen Charlton <gmcharlt@gmail.com> --- (In reply to comment #2)

Created attachment 18545 [details] [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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Fridolyn SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #5 from Fridolyn SOMERS <fridolyn.somers@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Fridolyn SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18545|0 |1 is obsolete| | --- Comment #7 from Fridolyn SOMERS <fridolyn.somers@biblibre.com> --- Created attachment 18626 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18626&action=edit Proposed patch Revised patch. I revised AddReturn and AddRenewal to be like AddIssue. See commit message. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Fridolyn SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18626|0 |1 is obsolete| | --- Comment #8 from Fridolyn SOMERS <fridolyn.somers@biblibre.com> --- Created attachment 18627 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=18627&action=edit Proposed patch (revised) Unifies code also in CanBookBeIssued and CanBookBeRenewed. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de --- Comment #9 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Pierre Angot <tredok.pierre@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply CC| |tredok.pierre@gmail.com --- Comment #11 from Pierre Angot <tredok.pierre@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Fridolyn SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |kyle@bywatersolutions.com --- Comment #13 from Kyle M Hall <kyle@bywatersolutions.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #18699|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 --- Comment #19 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- (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 ;-)

-- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=11463 Depends on|9532 |11463 --- Comment #21 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 --- Comment #23 from mathieu saby <mathsabypro@gmail.com> --- 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 --- Comment #25 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- (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.

-- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 --- Comment #27 from mathieu saby <mathsabypro@gmail.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 --- Comment #29 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 David Cook <dcook@prosentient.com.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dcook@prosentient.com.au --- Comment #31 from David Cook <dcook@prosentient.com.au> --- 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. -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #23692|0 |1 is obsolete| | --- Comment #33 from Fridolin SOMERS <fridolyn.somers@biblibre.com> --- Created attachment 34818 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=34818&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|11463 | -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nengard@gmail.com --- Comment #36 from Kyle M Hall <kyle@bywatersolutions.com> --- *** Bug 13524 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #34818|0 |1 is obsolete| | Attachment #34819|0 |1 is obsolete| | --- Comment #37 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 34994 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=34994&action=edit [SIGNED-OFF] 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 R. Crago <ridingthegravytrain@yahoo.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |ridingthegravytrain@yahoo.c | |om -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #39 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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? -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Fridolin SOMERS <fridolyn.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Nick Clemens <nick@quecheelibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #34994|0 |1 is obsolete| | --- Comment #43 from Nick Clemens <nick@quecheelibrary.org> --- Created attachment 35712 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35712&action=edit [SIGNED OFF] 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Nick Clemens <nick@quecheelibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #35578|0 |1 is obsolete| | --- Comment #45 from Nick Clemens <nick@quecheelibrary.org> --- Created attachment 35714 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=35714&action=edit [SIGNED OFF] 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> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 --- Comment #47 from David Cook <dcook@prosentient.com.au> --- (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 :). -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 --- Comment #49 from David Cook <dcook@prosentient.com.au> --- (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). -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 --- Comment #51 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Created attachment 39457 --> http://bugs.koha-community.org/bugzilla3/attachment.cgi?id=39457&action=edit Bug 9351 - sql update on existing statistics Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Nick <nick@quecheelibrary.org> -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #35713|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 --- Comment #53 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- I've rebased the patches - could someone please take another look and try to unravel any confusion? Thx! -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Nick Clemens <nick@quecheelibrary.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |nick@quecheelibrary.org --- Comment #55 from Nick Clemens <nick@quecheelibrary.org> --- Not sure what needs to be done here, more than a signoff? Failing because of Mark's test request -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |http://bugs.koha-community. | |org/bugzilla3/show_bug.cgi? | |id=14598 -- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 --- Comment #58 from Fridolin SOMERS <fridolin.somers@biblibre.com> --- (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.

-- You are receiving this mail because: You are watching all bug changes.

http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=9351 Chris Cormack <chris@bigballofwax.co.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords|Academy | -- You are receiving this mail because: You are watching all bug changes.