Bug 5860 - Adding duplicate stocknumber will fail silently
Summary: Adding duplicate stocknumber will fail silently
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: PATCH-Sent (DO NOT USE) critical (vote)
Assignee: Katrin Fischer
QA Contact: Bugs List
URL:
Keywords:
Depends on: 5861
Blocks: 5839
  Show dependency treegraph
 
Reported: 2011-03-13 21:31 UTC by Katrin Fischer
Modified: 2019-06-27 09:24 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Patch (1.98 KB, patch)
2011-03-29 20:54 UTC, Katrin Fischer
Details | Diff | Splinter Review
Signed patch (2.30 KB, patch)
2011-04-04 11:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Follow up patch (1.42 KB, patch)
2011-05-13 02:03 UTC, Chris Cormack
Details | Diff | Splinter Review
Signed followup patch (1.69 KB, patch)
2011-05-13 12:51 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Followup patch for 3.2.x (3.36 KB, patch)
2011-05-16 09:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Followup patch for 3.4.x (2.71 KB, patch)
2011-05-16 09:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Followup patch for master (2.74 KB, patch)
2011-05-16 09:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Patch for MASTER, tested and signed off (2.79 KB, patch)
2011-05-17 11:07 UTC, Katrin Fischer
Details | Diff | Splinter Review
Patch for 3.4.x, tested and signed off (2.79 KB, patch)
2011-05-17 11:57 UTC, Katrin Fischer
Details | Diff | Splinter Review
Patch for 3.2.x, tested and signed off (3.44 KB, patch)
2011-05-17 12:42 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 Katrin Fischer 2011-03-13 21:31:02 UTC
To test:

Add new field 952 $i to framework and map to items.stocknumber.
Add a new item, make stocknumber 1.
Add another item, make stocknumber 1.

The second stocknumber will not be saved, because the items.stocknumber is unique.

The logs show an error message:
[Sun Mar 13 22:12:37 2011] [error] [client ::1] [Sun Mar 13 22:12:37 2011] additem.pl: DBD::mysql::st execute failed: Duplicate entry '1' for key 'itemstocknumberidx' at /home/katrin/kohaclone/C4/Items.pm line 1896., referer: http://localhost:8080/cgi-bin/koha/cataloguing/additem.pl

We need an error message for the user like we have barcode, which is also unique.
Comment 1 Paul Poulain 2011-03-13 21:47:47 UTC
I'm not sure that stocknumber must be unique. I think it would be better to have it not unique accepted (functionnaly, at least in France, it's common to have libraries having multiple stocknumber, even if it's intellectually strange, I agree. For example, when a library buys 4 of the same book at the same time, the 4 have the same stocknumber)
Comment 2 Katrin Fischer 2011-03-13 22:18:04 UTC
I think we agree that it is a bug at the moment that can be solved in 2 different ways:

1) items.stocknumber stays unique and we give a proper error message.
2) items.stucknumber is changed to not be unique, no error message required.

I think the situation in Germany might be similar to France here. It can happen that a stocknumber is assigned for more than 1 item. I think I saw it happen for serials items. And if you have old data it can happen that a stocknumber was duplicated by accident.

See also discussion about stocknumber's uniqueness on bug 5839
Comment 3 Katrin Fischer 2011-03-25 11:39:50 UTC
Is someone strictly opposed to changing items.stocknumber to be not unique? 
I would like to work on that this weekend.
Comment 4 Katrin Fischer 2011-03-29 20:54:26 UTC Comment hidden (obsolete)
Comment 5 Marcel de Rooy 2011-04-04 11:21:33 UTC Comment hidden (obsolete)
Comment 6 Chris Cormack 2011-04-04 21:33:10 UTC
Patch pushed, please test
Comment 7 Katrin Fischer 2011-04-18 10:03:10 UTC
This patch needs a follow-up for 3.2.x and master.

Problem: The applied patch and update will work only for databases created after the stocknumber field was added to the items table.

For databases created before stocknumber was introduced to the database the update will fail with an error message and the unique attribute will not be removed.

The problem is, that the indexname in kohastructure.pl is different to the name in updatadatabase.pl:

kohastructur.sql:
1033   KEY `itemstocknumberidx` (`stocknumber`),

updatadatabase.pl:
3251  ALTER TABLE items ADD UNIQUE INDEX itemsstocknumberidx (stocknumber);

I am not sure how to solve this so that no error message will be shown while updating the database. And not sure I can work on this in time for the 3.4 release. If someone else has time and is able to fix this - please do.
Comment 8 Chris Cormack 2011-05-13 02:03:27 UTC
Follow up patch sent
Comment 9 Chris Cormack 2011-05-13 02:03:54 UTC Comment hidden (obsolete)
Comment 10 Marcel de Rooy 2011-05-13 12:41:18 UTC
(In reply to comment #9)
> Created attachment 4092 [details] [review]
> Follow up patch

Patch uses select on information_schema. If a server runs multiple Koha instances [and versions], this could not be reliable.
Statement show indexes from items where key_name=... runs on current db.
Sending signed patch including this change.
Comment 11 Marcel de Rooy 2011-05-13 12:51:35 UTC Comment hidden (obsolete)
Comment 12 Katrin Fischer 2011-05-13 12:52:11 UTC
Hi Marcel, thx for your work on that! 

I talked to Chris about it in the morning and we debated if checking for the uniqueness of the index might be away to make it easier not relying on version numbers.
We also need a patch for 3.2.x and 3.4, because the bug is currently in all maintained versions. People are at different stages when updating, some might be in between and have run into a mysql error message and are already past the update with index still being unique.
I think what we need to do is add another database update at the end. Making sure that in every case the index will be fixed. As I said a check for uniqueness using the information_schema tablae and the NOT_UNIQUE field might be easier here.
Comment 13 Marcel de Rooy 2011-05-13 12:56:13 UTC
(In reply to comment #12)
> I talked to Chris about it in the morning and we debated if checking for the
> uniqueness of the index might be away to make it easier not relying on version
> numbers.
> We also need a patch for 3.2.x and 3.4, because the bug is currently in all
> maintained versions. People are at different stages when updating, some might
> be in between and have run into a mysql error message and are already past the
> update with index still being unique.
> I think what we need to do is add another database update at the end. Making
> sure that in every case the index will be fixed. As I said a check for
> uniqueness using the information_schema tablae and the NOT_UNIQUE field might
> be easier here.
I would not use information_schema if you do not include name of database (Table schema).
Do you send another revision?
Comment 14 Katrin Fischer 2011-05-13 13:18:23 UTC
Hi Marcel,

I will try to work on it at the weekend, but will be happy if someone else steps in. I tried "show indexes from items" and see the field non_unique - so I think we can combine both ideas to improve this.
Comment 15 Marcel de Rooy 2011-05-16 07:52:46 UTC
Sending two additinal patches for 32x and 34x.
Idea is: just delete both indexes if needed and recreate the non-unique one.
Comment 16 Marcel de Rooy 2011-05-16 09:51:30 UTC
After some discussion on IRC with Chris and Katrin, now submitting three new revised patches: one for master, one for 32x and one for 34x. Incorporating all changes in one. 
The patch tests if it is needed to replace the index.
Setting status back to needs signoff.
Katrin or Chris: could you sign it please?
Comment 17 Marcel de Rooy 2011-05-16 09:53:00 UTC Comment hidden (obsolete)
Comment 18 Marcel de Rooy 2011-05-16 09:53:45 UTC Comment hidden (obsolete)
Comment 19 Marcel de Rooy 2011-05-16 09:54:29 UTC Comment hidden (obsolete)
Comment 20 Katrin Fischer 2011-05-17 11:07:45 UTC
Created attachment 4141 [details] [review]
Patch for MASTER, tested and signed off
Comment 21 Katrin Fischer 2011-05-17 11:57:03 UTC
Created attachment 4143 [details] [review]
Patch for 3.4.x, tested and signed off
Comment 22 Katrin Fischer 2011-05-17 12:42:07 UTC
Created attachment 4144 [details] [review]
Patch for 3.2.x, tested and signed off
Comment 23 Chris Cormack 2011-06-12 23:49:35 UTC
Patch for master pushed, please test
Comment 24 Katrin Fischer 2011-07-17 13:46:39 UTC
Patches have been pushed to master, 3.4.x. and 3.2.x. Closing this resolved.