This problem was also reported by one of our client
http://suivi.biblibre.com/view.php?id=8205

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.

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

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

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.

Be careful, this will be different for MARC21 as item information is stored in 952 there.

Ok, important indeed.

Can we consider that the field "info" for an item will ever begin either with "LDR 955" or with "LDR 952" ?

I mean of course "LDR 995" or "LDR 952".

I don't have any Marc21 database to test the "LDR 952" output.

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.

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

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.

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"

related to Bug 7869

Any comments on this ?
How should we try to correct the old logs ?

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.

Frédéric could you vote with your signoff?

After applying the patch entries in the action_logs tables for ISSUE and RETURN show the itemnumber instead of the biblionumber.

(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

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

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

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.

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

(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

(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 ;-)

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)

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.

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.

QA: Having a quick look now..

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>

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>

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

Sorry, Adrien:
One final request:
Please make the item parameter in the url for item details itemnumber.

Just send a followup, test and sign. Set back to Passed QA. Thx.

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

This patch has been pushed to master.

Could it be added to the 3.8 updates too?
Several 3.8 librairies could make use of this.

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

Pushed to 3.10.x will be in 3.10.6

Patches don't apply for 3.8.x

I am having the same problem in 3.18.05.
How fix it?

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.