Bug 7241 - circulation action logs record biblionumber instead of item number
Summary: circulation action logs record biblionumber instead of item number
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Transaction logs (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Adrien SAURAT
QA Contact: Marcel de Rooy
URL:
Keywords:
: 7256 7869 (view as bug list)
Depends on:
Blocks:
 
Reported: 2011-11-17 22:08 UTC by Albert Oller
Modified: 2016-06-21 12:53 UTC (History)
16 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
proposed patch (7.14 KB, patch)
2012-01-09 10:00 UTC, Adrien SAURAT
Details | Diff | Splinter Review
Bug 7241: corrects log action for CIRC and CATALOGUING (7.47 KB, patch)
2012-02-07 14:01 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
proposed patch #2 (1.14 KB, patch)
2012-06-01 09:18 UTC, Adrien SAURAT
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7241: fix for the CIRC logs -> itemnumber stored correctly (1.24 KB, patch)
2012-08-07 13:01 UTC, Owen Leonard
Details | Diff | Splinter Review
proposed patch #3 (3.59 KB, patch)
2012-09-07 13:26 UTC, Adrien SAURAT
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7241: circulation action logs record biblionumber instead of item number (3.68 KB, patch)
2012-09-21 14:48 UTC, Kyle M Hall
Details | Diff | Splinter Review
new proposed patch (3.55 KB, patch)
2013-01-22 13:52 UTC, Adrien SAURAT
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 7241: circulation action logs record biblionumber instead of item number (3.63 KB, patch)
2013-01-30 13:14 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 7241: circulation action logs record biblionumber instead of item number (3.58 KB, patch)
2013-03-17 16:31 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Follow-up for DB changes (2.56 KB, patch)
2013-03-21 13:35 UTC, Adrien SAURAT
Details | Diff | Splinter Review
Bug 7241: circulation action logs record biblionumber instead of item number (3.70 KB, patch)
2013-03-21 14:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7241: circulation action logs record biblionumber instead of item number (3.70 KB, patch)
2013-03-21 14:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7241 follow-up: DB changes (2.65 KB, patch)
2013-03-21 14:01 UTC, Marcel de Rooy
Details | Diff | Splinter Review
follow-up fixing the template (1.78 KB, patch)
2013-03-21 15:12 UTC, Adrien SAURAT
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Albert Oller 2011-11-17 22:08:14 UTC
Issue log and and circulation logs are incorrectly recording biblionumber instead of item number, creating incorrect logs.
Comment 1 Sophie MEYNIEUX 2011-11-18 08:39:48 UTC
This problem was also reported by one of our client
http://suivi.biblibre.com/view.php?id=8205
Comment 2 Ian Walls 2011-11-18 14:02:07 UTC
There are two possible resolutions:  either change the link-creating code to expect biblionumber instead of itemnumber, or start recording item number instead.   The former is easier, but doesn't record as good of data.  The latter is more thorough, but requires a database update to find an itemnumber for the recorded biblionumber... this may not be the item circulated (impossible to find out), but it's better than nothing.
Comment 3 Katrin Fischer 2011-11-23 14:50:59 UTC
*** Bug 7256 has been marked as a duplicate of this bug. ***
Comment 4 Adrien SAURAT 2011-12-19 13:08:29 UTC
In fact, the correct ID is stored in the action_logs table :
- when modifying a biblio, the biblionumber is stored in the "object" field;
- when modifying an item, the itemnumber is stored in the "object" field.

The problem is that, when the page "viewlogs" tries to display these log entries, there is no way to know if the value stored was an itemnumber or a biblionumber !


Where could we store this information ?
1) A new field ? (would be unuseful for many possible actions)

2) New values for the "action" field ?
We now use ADD, MODIFY and DELETE.
We could have ADD_ITEM, MODIFY_ITEM, DELETE_ITEM, ADD_BIBLIO, MODIFY_BIBLIO and DELETE_BIBLIO.

3) More info in the "info" field ?
For example, when a systempreference is modified, the info field contains a string like this : "OPACUserCSS | new/path/test.css"
We could then begin the info with "ITEM | " or "BIBLIO | ".
Comment 5 Adrien SAURAT 2011-12-19 13:36:34 UTC
There may be another (better?) way:

Example of the "info" field content for an item:
LDR 995 _f00658000058359 _qj _p25.00 _m2011-12-05 _10 _911139 _cMON _20 _k599.72 _n2012-01-04 _o0 _eDoc jeunesse _bVID 

Example of the "info" field content for a biblio:
BEFORE=>LDR 02769cam 2200553 4500 010 _a2070534812 _bvol. 1 _bbr. _d69 F 010 _a2-07-053482-0 _bvol. 2 _bbr ... bla bla bla ... eDoc adultes _bFLA _qa _rPapiers collés _t1 995 _f08307000011057 _p10.70 _m2009-07-10 _10 _974042 _cFLA _20 _k523 DUP H1 _o0 _eDoc adultes _bFLA _qa _rPapiers collés _t1 
"LDR 995"


Ok, for the biblio it begins with "BEFORE=>" but I did not propose this as I'm not sure this will stay so (and maybe should the item also begins with BEFORE=> ?).

But ! The item info begins with "LDR 995", and this string may be enough to be sure we have an item here.
Comment 6 Katrin Fischer 2011-12-19 13:48:27 UTC
Be careful, this will be different for MARC21 as item information is stored in 952 there.
Comment 7 Adrien SAURAT 2011-12-19 14:20:47 UTC
Ok, important indeed.

Can we consider that the field "info" for an item will ever begin either with "LDR 955" or with "LDR 952" ?
Comment 8 Adrien SAURAT 2011-12-19 14:34:33 UTC
I mean of course "LDR 995" or "LDR 952".

I don't have any Marc21 database to test the "LDR 952" output.
Comment 9 Adrien SAURAT 2011-12-27 09:54:06 UTC
As seen with hdl today, it would be better to add a new field in the table.

I've seen that the code sometimes searches for a value "item" in the info "field". It was maybe the case earlier, but now info contains the resulting string of a MARC RecordWe already have a field called "object" containing the code,  "as_formatted" method call. Some parts of the code may be obsolete then, or need to be updated.

We need indeed more information about the object but as the general info field is already used for the MARC Record "pretty string", I'll add a field called "object_info".
When dealing with cataloguing, it will contain "item" or "biblio" to help displaying the correct link.
Comment 10 Adrien SAURAT 2012-01-06 16:20:48 UTC
For information (as my previous message were probably confusing) :

As Sophie told in Comment 1 we had a user reporting this kind of problem, but in fact it was about the CATALOGUING logs (log problem there too).

I'm preparing a patch which will add a new column "objectinfo" and solve this. But I still have to cover the main topic of this BZ 7241, ISSUE and CIRCULATION logs (or is Albert working on this ? can I assign the bug to me? )
Comment 11 Adrien SAURAT 2012-01-09 10:00:56 UTC Comment hidden (obsolete)
Comment 12 Jared Camins-Esakov 2012-02-07 14:01:22 UTC Comment hidden (obsolete)
Comment 13 Ian Walls 2012-02-16 17:31:40 UTC
Any change to the way the log information is recorded needs to work retroactively, or else the logs will be unusable before the date of the upgrade.  Going from itemnumber to biblionumber is easy, since it's one-way mapping.

Marking as Failed QA until the problem of fixing existing log entries is resolved.
Comment 14 Adrien SAURAT 2012-02-23 13:17:29 UTC
As it is now, the biblionumber is stored instead of the itemnumber.
We have to go from biblio to item, I thus can't see a way to completely correct the old logs. It seems dangerous to me to take a random itemnumber.

For the "1 biblio = 1 item" cases we would be pretty accurate (when the item wasn't replaced), but when a biblio is related to several items it could be very misleading to display a randomly chosen itemnumber.

Maybe we can make a script correcting the logs when only one item exists for the biblio, and replacing the log info with a message explaining what happened for the other cases. Message like "itemnumber not available : biblionumber = 323232"
Comment 15 Koha Team University Lyon 3 2012-04-04 16:50:37 UTC
related to Bug 7869
Comment 16 Adrien SAURAT 2012-04-27 07:43:55 UTC
Any comments on this ?
How should we try to correct the old logs ?
Comment 17 Adrien SAURAT 2012-06-01 09:18:51 UTC Comment hidden (obsolete)
Comment 18 Frédéric Demians 2012-07-09 16:16:17 UTC
This patch must be applied. The more we wait, the more logs are
populated with bad informations. Ian suggestion to take the first item
is not that bad. An update warning could explain the situation. The logs
would be inaccurate but usable.
Comment 19 Owen Leonard 2012-07-20 18:33:30 UTC
Frédéric could you vote with your signoff?
Comment 20 Owen Leonard 2012-08-07 13:01:11 UTC Comment hidden (obsolete)
Comment 21 Owen Leonard 2012-08-07 13:02:18 UTC
After applying the patch entries in the action_logs tables for ISSUE and RETURN show the itemnumber instead of the biblionumber.
Comment 22 Mason James 2012-08-10 08:55:24 UTC
(In reply to comment #20)
> Created attachment 11415 [details] [review]
> [SIGNED-OFF] Bug 7241: fix for the CIRC logs -> itemnumber stored correctly
> 
> Signed-off-by: Owen Leonard <oleonard@myacpl.org>

patch looks good, passing QA


mason@xen1:~/git/head$ koha-qa.pl 
        * a82aaaf Bug 7241: fix for the CIRC logs -> itemnumber stored correctly
                C4/Circulation.pm

- perlcritic-progressive tests... OK
- perl -c syntax tests... OK
- xt/tt_valid.t tests... OK
- xt/author/valid-template.t tests... OK
Comment 23 Owen Leonard 2012-08-21 18:12:49 UTC
*** Bug 7869 has been marked as a duplicate of this bug. ***
Comment 24 Paul Poulain 2012-09-03 15:40:01 UTC
Well, Thinking of it a little bit more, I've reverted the patch.

Having history badly broken for existing instances is really bad, because we will never be able to fix the problem properly once action_logs contains mixed informations (biblio & items)

And I've an easy proposal to fix what can be fixed:
The following SQL:
UPDATE action_logs SET object=(SELECT itemnumber FROM items WHERE biblionumber= action_logs.object LIMIT 1) WHERE module='CIRCULATION' AND action in ('ISSUE','RETURN');

Set the 1st item number (as we can't know which item was circulated once things have been anonymised). It's better than nothing.

I propose that you submit a new patch including an updatedatabase with this SQL

Thanks
Comment 25 Adrien SAURAT 2012-09-07 08:59:40 UTC
Ok, I made a test with this request and it modified all the timestamps of my updated rows.
That's because this field is designed to store the date of each sql update:
`timestamp` timestamp NOT NULL DEFAULT CURRENT_TIMESTAMP ON UPDATE CURRENT_TIMESTAMP,

It seems to me that, for these logs, the important date is the "library action" date and not the "last SQL modification" date.

---

I suggest adding also the following line before the UPDATE:
ALTER TABLE action_logs CHANGE timestamp timestamp TIMESTAMP NOT NULL DEFAULT CURRENT_TIMESTAMP;

The timestamp will still be automatically set when creating a row, but updating won't change the original date/time.
Comment 26 Adrien SAURAT 2012-09-07 13:26:05 UTC Comment hidden (obsolete)
Comment 27 Galen Charlton 2012-09-11 16:24:52 UTC
(In reply to comment #24)
> Well, Thinking of it a little bit more, I've reverted the patch.
> 
> Having history badly broken for existing instances is really bad, because we
> will never be able to fix the problem properly once action_logs contains
> mixed informations (biblio & items)
> 
> And I've an easy proposal to fix what can be fixed:
> The following SQL:
> UPDATE action_logs SET object=(SELECT itemnumber FROM items WHERE
> biblionumber= action_logs.object LIMIT 1) WHERE module='CIRCULATION' AND
> action in ('ISSUE','RETURN');
> 
> Set the 1st item number (as we can't know which item was circulated once
> things have been anonymised). It's better than nothing.
> 
> I propose that you submit a new patch including an updatedatabase with this
> SQL

As a possible supplement, how about tagging the object column with an indication of whether it's an old-style log of a biblionumber.  E.g.,

UPDATE action_logs SET object = 'biblio ' || object WHERE module = 'CIRCULATION' AND action IN ('ISSUE', 'RETURN');

Then viewlog.pl could assume that if the value is tagged with a 'biblio ' prefix that it contains an bib number; if it isn't, it contains an item number.

Doing it this way would mean that the logs are as accurate as possible.

Another, farther-reaching option would be to add biblionumber and itemnumber columns to action_logs.
Comment 28 Chris Cormack 2012-09-12 07:47:16 UTC
(In reply to comment #27)
> (In reply to comment #24)
> > Well, Thinking of it a little bit more, I've reverted the patch.
> > 
> > Having history badly broken for existing instances is really bad, because we
> > will never be able to fix the problem properly once action_logs contains
> > mixed informations (biblio & items)
> > 
> > And I've an easy proposal to fix what can be fixed:
> > The following SQL:
> > UPDATE action_logs SET object=(SELECT itemnumber FROM items WHERE
> > biblionumber= action_logs.object LIMIT 1) WHERE module='CIRCULATION' AND
> > action in ('ISSUE','RETURN');
> > 
> > Set the 1st item number (as we can't know which item was circulated once
> > things have been anonymised). It's better than nothing.
> > 
> > I propose that you submit a new patch including an updatedatabase with this
> > SQL
> 
> As a possible supplement, how about tagging the object column with an
> indication of whether it's an old-style log of a biblionumber.  E.g.,
> 
> UPDATE action_logs SET object = 'biblio ' || object WHERE module =
> 'CIRCULATION' AND action IN ('ISSUE', 'RETURN');
> 
> Then viewlog.pl could assume that if the value is tagged with a 'biblio '
> prefix that it contains an bib number; if it isn't, it contains an item
> number.
> 
> Doing it this way would mean that the logs are as accurate as possible.
> 
> Another, farther-reaching option would be to add biblionumber and itemnumber
> columns to action_logs.

+1
Comment 29 Paul Poulain 2012-09-17 10:07:48 UTC
(In reply to comment #27)
> (In reply to comment #24)
> > Well, Thinking of it a little bit more, I've reverted the patch.
> > 
> > Having history badly broken for existing instances is really bad, because we
> > will never be able to fix the problem properly once action_logs contains
> > mixed informations (biblio & items)
> > 
> > And I've an easy proposal to fix what can be fixed:
> > The following SQL:
> > UPDATE action_logs SET object=(SELECT itemnumber FROM items WHERE
> > biblionumber= action_logs.object LIMIT 1) WHERE module='CIRCULATION' AND
> > action in ('ISSUE','RETURN');
> > 
> > Set the 1st item number (as we can't know which item was circulated once
> > things have been anonymised). It's better than nothing.
> > 
> > I propose that you submit a new patch including an updatedatabase with this
> > SQL
> 
> As a possible supplement, how about tagging the object column with an
> indication of whether it's an old-style log of a biblionumber.  E.g.,
> 
> UPDATE action_logs SET object = 'biblio ' || object WHERE module =
> 'CIRCULATION' AND action IN ('ISSUE', 'RETURN');
> 
> Then viewlog.pl could assume that if the value is tagged with a 'biblio '
> prefix that it contains an bib number; if it isn't, it contains an item
> number.
> 
> Doing it this way would mean that the logs are as accurate as possible.
It means Adrien would have to update viewlog.pl to take care of this ? I'm not sure I'm for it. It introduce some specific code to circumvent an error in what is saved.

> Another, farther-reaching option would be to add biblionumber and itemnumber
> columns to action_logs.
mmm... the 'object' term is generic, it can also contain a borrowernumber,...
Maybe that was a mistake, and it would be better to have a "biblionumber", "itemnumber", "borrowernumber",... field (one field for each needed kind of information). But that would be for another patch ;-)
Comment 30 Kyle M Hall 2012-09-21 14:48:23 UTC Comment hidden (obsolete)
Comment 31 Paul Poulain 2012-10-10 14:53:34 UTC
QA comment:

logaction API is:
sub logaction {
    my ($modulename, $actionname, $objectnumber, $infos)=@_;

the checkin/checkout is registered with:
logaction("CIRCULATION", "ISSUE", $borrower->{'borrowernumber'}, $biblio->{'biblionumber'})
that you change to:
logaction("CIRCULATION", "ISSUE", $borrower->{'borrowernumber'}, $biblio->{'itemnumber'})

So the (previous and wrong) biblionumber and the (new and correct) itemnumber is the 4th parameter. That is $info
And $info goes to (surprise ;-) ) action_logs.info field
the "object" field is reserved for the borrowernumber

It means that the UPDATE is wrong:
UPDATE action_logs SET object=(SELECT itemnumber FROM items WHERE biblionumber= action_logs.object LIMIT 1) WHERE module='CIRCULATION' AND action in ('ISSUE','RETURN');

should be
UPDATE action_logs SET info=(SELECT itemnumber FROM items WHERE biblionumber= action_logs.info LIMIT 1) WHERE module='CIRCULATION' AND action in ('ISSUE','RETURN');

I had proposed the SQL in comment 24 without testing it, just for the general idea (I should have told it !)

It seems neither Adrien nor Kyle have tested too ...

Please resubmit a working patch ! (once again, my SQL is untested !!!)

(OTOH, +1 for the timestamp update)
Comment 32 Adrien SAURAT 2013-01-22 13:52:01 UTC Comment hidden (obsolete)
Comment 33 Kyle M Hall 2013-01-30 13:14:02 UTC Comment hidden (obsolete)
Comment 34 Marcel de Rooy 2013-03-17 16:31:25 UTC
QA Comment:
This patch deserves to be pushed soon, but I am still having doubts on the following:

1 You remove the timestamp on update. Why? If you do not want to change the timestamp when correcting the old log lines, remove the timestamp temporarily on update and put it back when you are ready. Or try adding timestamp=timestamp in your update statement (did not test that).
I think we should keep the on update clause (in terms of consistency).
This would mean that you leave kohastructure unmodified.

2 Many people already commented on the old log lines. You choose the first item as a workaround. (Another approach could have been: Delete information that is incorrect, or somehow route the old lines to biblio detail and the new ones to item detail, perhaps adding biblio to info field for old ones.)
It is somewhat arbitrary. But could you at least add a separate print line in the updatedatabase output warning the sysadmin about this?

Please send a followup and switch back to Signed off.
Comment 35 Marcel de Rooy 2013-03-17 16:31:48 UTC Comment hidden (obsolete)
Comment 36 Adrien SAURAT 2013-03-19 08:35:27 UTC
I planned to remove the automatic update, not because it seemed useless to me, but because I see this as a bug.
From the librarian point of view: there's no update, they only generate log lines and can view them (read only) in the tools section.
From an admin point of view: it is seldom needed to make any update on this, but if this has to happen, when the admin changes something in a line, he loses the date related to the library action, which is, I think, the important information!

I don't use this often and I'm not a librarian so I could be mistaken, and anyway for clarity this should maybe discussed in another bug, so I'll make a new patch restoring the former behaviour.
Comment 37 Adrien SAURAT 2013-03-21 13:35:50 UTC Comment hidden (obsolete)
Comment 38 Marcel de Rooy 2013-03-21 13:45:07 UTC
QA: Having a quick look now..
Comment 39 Marcel de Rooy 2013-03-21 14:00:08 UTC Comment hidden (obsolete)
Comment 40 Marcel de Rooy 2013-03-21 14:01:34 UTC
Created attachment 16637 [details] [review]
Bug 7241: circulation action logs record biblionumber instead of item number

Issue log and and circulation logs are incorrectly recording biblionumber instead of item number, creating incorrect logs.
This patch corrects this, and modifies the action_logs table in two ways:
- the timestamp is not updated anymore with UPDATES (but still automatically filled when INSERTing)
- to partially correct the broken logs, the log history is filled with the first found item for each biblio

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 41 Marcel de Rooy 2013-03-21 14:01:39 UTC
Created attachment 16638 [details] [review]
Bug 7241 follow-up: DB changes

Reset timestamp modification on update after patch application.
Give more information about the log history fix.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 42 Marcel de Rooy 2013-03-21 14:17:19 UTC
QA Comment:
Looks good to me now. Thanks for adjustments.
Note that I had lots of nulls in info field now for a test database where lots of biblios and items were removed. Normally, this should be an exception. Repeated a few records for valid biblios and items. That was okay.

Passed QA
Comment 43 Marcel de Rooy 2013-03-21 14:24:19 UTC
Sorry, Adrien:
One final request:
Please make the item parameter in the url for item details itemnumber.
Comment 44 Marcel de Rooy 2013-03-21 14:25:12 UTC
Just send a followup, test and sign. Set back to Passed QA. Thx.
Comment 45 Adrien SAURAT 2013-03-21 15:12:36 UTC
Created attachment 16661 [details] [review]
follow-up fixing the template

Indeed, the template had not been modified yet and needed a few tweaks (itemnumber, tooltip, anchor).
Comment 46 Jared Camins-Esakov 2013-03-21 15:44:37 UTC
This patch has been pushed to master.
Comment 47 Adrien SAURAT 2013-03-29 13:36:31 UTC
Could it be added to the 3.8 updates too?
Several 3.8 librairies could make use of this.
Comment 48 Marcel de Rooy 2013-03-29 13:41:49 UTC
(In reply to comment #47)
> Could it be added to the 3.8 updates too?
> Several 3.8 librairies could make use of this.

Better contact Rmaint directly?
Comment 49 Chris Cormack 2013-04-24 09:55:43 UTC
Pushed to 3.10.x will be in 3.10.6

Patches don't apply for 3.8.x
Comment 50 Isabela Terra 2015-09-11 12:20:05 UTC
I am having the same problem in 3.18.05.
How fix it?
Comment 51 Katrin Fischer 2015-09-11 12:29:35 UTC
Hi Isabela,

can you please create a new bug report with a step by step description on how to reproduce the problem? And link to this one? This has already been pushed a long time ago, so it might be a slight variation of the original problem.