Bug 7458 - New call number allocation plugin
Summary: New call number allocation plugin
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: 3.8
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Srdjan Jankovic
QA Contact: Ian Walls
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-01-23 07:24 UTC by Srdjan Jankovic
Modified: 2013-12-05 20:04 UTC (History)
4 users (show)

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


Attachments
patch (4.85 KB, patch)
2012-01-23 07:49 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Signed off patch (5.23 KB, patch)
2012-01-26 01:30 UTC, Melia Meggs
Details | Diff | Splinter Review
patch (4.95 KB, patch)
2012-01-30 02:34 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (5.00 KB, patch)
2012-01-30 09:11 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (5.05 KB, patch)
2012-01-30 09:55 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (5.05 KB, patch)
2012-01-31 00:17 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (5.56 KB, patch)
2012-02-01 02:20 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (7.07 KB, patch)
2012-02-06 00:38 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
patch (5.40 KB, patch)
2012-02-08 02:15 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Signed off patch (5.77 KB, patch)
2012-02-15 22:25 UTC, Melia Meggs
Details | Diff | Splinter Review
follow-up patch (1.21 KB, patch)
2012-03-07 04:46 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
Signed off follow up patch (1.32 KB, patch)
2012-03-09 18:17 UTC, Melia Meggs
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Srdjan Jankovic 2012-01-23 07:24:44 UTC
A library needs a call number allocation plugin that is different to the existing one.
Comment 1 Srdjan Jankovic 2012-01-23 07:49:10 UTC Comment hidden (obsolete)
Comment 2 Melia Meggs 2012-01-26 01:30:59 UTC Comment hidden (obsolete)
Comment 3 Melia Meggs 2012-01-26 01:40:54 UTC
Here's what I did for testing:

* On the Home › Administration › MARC Frameworks › Framework Structure › Tag
952 Subfield Structure › Edit subfields constraints for subfield o (Full call
number in my case) click on more constraints, pick plugin callnumber-KU.pl
* Home > Cataloging > search for a record
* Edit > Edit items

Scroll down to o - Full Call Number.  I typed in the following:

FREN 0 (then click on ...) - FREN 0000 is returned
FREN 0 (then click on ...) - FREN 0001 is returned (the first unused value
beginning with this prefix) 
FREN 01 (then click on ...) - FREN 0100 is returned

And the one prefix that is different:

FEAT AM (then click on ...) - FEAT AM01 is returned
FEAT AM (then click on ...) - FEAT AM02 is returned

I think it works as expected, so I am signing off.
Comment 4 Srdjan Jankovic 2012-01-30 02:34:15 UTC Comment hidden (obsolete)
Comment 5 Srdjan Jankovic 2012-01-30 09:11:11 UTC Comment hidden (obsolete)
Comment 6 Srdjan Jankovic 2012-01-30 09:55:43 UTC Comment hidden (obsolete)
Comment 7 Srdjan Jankovic 2012-01-31 00:17:24 UTC Comment hidden (obsolete)
Comment 8 Katrin Fischer 2012-01-31 06:47:26 UTC
Hi Srdjan, I am starting to wonder what's happening here - I see you have replaced the patch a lot of times, but not sure what's the difference or what got fixed with each. Could you add some comments to help us bug wranglers? :)
Comment 9 Srdjan Jankovic 2012-01-31 08:05:54 UTC
I'll blame it on Melia, although most of them are of the "but wait, I did not test this situation" kind. And a copyright fix.

Melia;

Here is a problem.

1. I added the call number FREN 0001 to a book named "Dog"
2. I then found a book named "Cat" and entered "FREN 0" and was given FREN 0002. So far, so good.
3. I changed the call number for "Cat" to FREN 0200
4. On the next record, I entered "FREN 0" and was given FREN 201 … when I should have been given FREN 0002 as that was free

In short, the script is producing the first available number after the highest number in the range. I could get around this problem by entering "FREN 00" but the problem continues as you work through the numbers.

1. So let's say that I have "Elephant" at FREN 250, "Eloise" at FREN 500, and "Eloise at the Plaza" at FREN 750.
2. "FREN 0" returns FREN 751, and I have to enter "FREN 00" to get FREN 0002.
3. In our system, French feature films have to fall between FREN 0200 and FREN 0999, but if I want something in the FREN range above 200 and I enter "FREN 02" I get FREN 0251 because FREN 0250 is in the system already … and the only way to get the available FREN 0201 would be to enter "FREN 020".
4. Once the range FREN 0200 – FREN 0209 is in use, I am unsure whether "FREN 020" would produce FREN 0210 (the lowest available number in the range) or FREN 0251 (the first available number after the highest number in the range). If the latter, we will need to enter up to the third digit to fill in the gaps between FREN 0211 and FREN 0249 inclusive … needing to use "FREN 021", "FREN 022", "FREN 023" and "FREN 024" to get them all.

What we need is for the script to find the first available number in the range.

1. "FREN 0" should not return FREN 201 unless all of the numbers between FREN 0001 and FREN 0200 are already in use.
2. "FREN 02" should return the first number available after FREN 0200. If FREN 0200 - FREN 0299 are in use, it should return FREN 0300.
3. "FREN 1" should produce the first available number after FREN 1000.
Comment 10 Melia Meggs 2012-01-31 23:17:09 UTC
Hey Srdjan,

I applied the latest patch, but I have a problem.  It now returns the same barcode over and over.  Example:

I entered an item with call number FREN 0 - it returned FREN 0000. (correct)
I entered an item with call number FREN 0 - it returned FREN 0001. (correct)
I entered an item with call number FREN 0 - it returned FREN 0001. (problem)
I entered an item with call number FREN 0 - it returned FREN 0001 again. (problem)

Thanks,

Melia
Comment 11 Srdjan Jankovic 2012-02-01 02:20:02 UTC Comment hidden (obsolete)
Comment 12 Srdjan Jankovic 2012-02-01 02:20:56 UTC
Let's see it now.
Comment 13 Melia Meggs 2012-02-02 22:56:14 UTC
This is very nearly perfect now. Only one thing - is it possible to prevent it from ever returning FREN 0000? Can we make it start with FREN 0 - returns FREN 0001 as the first possible value?
Comment 14 Srdjan Jankovic 2012-02-06 00:38:10 UTC Comment hidden (obsolete)
Comment 15 Ian Walls 2012-02-07 15:12:32 UTC
Since this plugin is for a callnumber schema used by a single library, I do not believe it in itself warrants any kind of changes to core Koha code.  In many ways, it's like a third-party service; this call number schema is locally developed by KU, and it's reusability is... unlikely.  Having it as a plugin is just fine; when that plugin starts to dictate changes to the way the rest of Koha behaves, I believe that's a problem.

An index on items.itemcallnumber isn't a bad idea, but it should be filed as it's own separate bug report and patch.  If that index is required for this plugin to work, please list the newly filed bug report as a dependency.
Comment 16 Brendan Gallagher 2012-02-07 17:48:00 UTC
(In reply to comment #15)
> Since this plugin is for a callnumber schema used by a single library, I do not
> believe it in itself warrants any kind of changes to core Koha code.  In many
> ways, it's like a third-party service; this call number schema is locally
> developed by KU, and it's reusability is... unlikely.  Having it as a plugin is
> just fine; when that plugin starts to dictate changes to the way the rest of
> Koha behaves, I believe that's a problem.
> 
> An index on items.itemcallnumber isn't a bad idea, but it should be filed as
> it's own separate bug report and patch.  If that index is required for this
> plugin to work, please list the newly filed bug report as a dependency.

Wait this is not passing because the QAM thinks that others will not use this?  That's kind of a crazy idea.  So since something is programmed in a good manner and works and does not effect any other aspects of Koha - unless you change your framework to use this plugin - AND the QAM even said that the indexes on itemcallnumber are a good idea (well shouldn't the QAM pass this?). 

This comment "Having it as a plugin is
> just fine; when that plugin starts to dictate changes to the way the rest of
> Koha behaves, I believe that's a problem."  Should be examined again - reviewing the code I do not see how this changes core Koha code???  It adds an index to itemcallnumber - which even the QAM agrees to..

Sorry just a bit perplexed as to why this was failed QA?
Comment 17 Ian Walls 2012-02-07 18:10:05 UTC
The current patch implements both the plugin (which is in the scope of this bug report) AND a database change to add an index on items.itemcallnumber (out of scope).  I do not object to either change; even if the new plugin isn't used by anyone, it can safely sit in the plugins directory without harming anything.  The index on items.itemcallnumber may well provide useful, as I'm sure there are other places in Koha that perform SQL WHERE clauses on itemcallnumber (C4::Search::SearchAcquisitions() for example).

What I object to is the two enhancements being bundled together.  This new plugin should exist entirely on it's own, in it's own patch.  The new itemcallnumber index should be it's own change, on it's own bug report, tested independently.  If this plugin does indeed dependent on this index, a dependency between the two bug reports should be added.

Moving to "In Discussion" to solicit further opinions on this.
Comment 18 Brendan Gallagher 2012-02-07 18:14:37 UTC
(In reply to comment #17)
> The current patch implements both the plugin (which is in the scope of this bug
> report) AND a database change to add an index on items.itemcallnumber (out of
> scope).  I do not object to either change; even if the new plugin isn't used by
> anyone, it can safely sit in the plugins directory without harming anything. 
> The index on items.itemcallnumber may well provide useful, as I'm sure there
> are other places in Koha that perform SQL WHERE clauses on itemcallnumber
> (C4::Search::SearchAcquisitions() for example).
> 
> What I object to is the two enhancements being bundled together.  This new
> plugin should exist entirely on it's own, in it's own patch.  The new
> itemcallnumber index should be it's own change, on it's own bug report, tested
> independently.  If this plugin does indeed dependent on this index, a
> dependency between the two bug reports should be added.
> 
> Moving to "In Discussion" to solicit further opinions on this.

Great Makes Sense...
Comment 19 Srdjan Jankovic 2012-02-07 23:53:35 UTC
I have no problems splitting it, it is just that I was not aware that database changes need to be separated. To me it makes sense to put them together, but I will create a new bug report if it makes it easier from the QA perspective.
Comment 20 Srdjan Jankovic 2012-02-08 02:15:50 UTC Comment hidden (obsolete)
Comment 21 Melia Meggs 2012-02-15 22:25:32 UTC
Created attachment 7668 [details] [review]
Signed off patch

Works as expected.  I have signed off.
Comment 22 Ian Walls 2012-02-16 16:36:29 UTC
Comment on attachment 7668 [details] [review]
Signed off patch

Thank you for splitting this patch off from it's database-changing dependency.  As that patch has now passed QA, and this one is just the addition of new (albeit highly specialized) plugin script, marking this one as Passed QA.
Comment 23 Paul Poulain 2012-02-17 10:25:53 UTC
I haven't tested it myself, but it just add a plugin, so, even if it does not work, it can't break another part of Koha

Setting version to rel_3_8 and will advertise the feature when 3.8 is released
Comment 24 Srdjan Jankovic 2012-03-07 04:46:50 UTC Comment hidden (obsolete)
Comment 25 Melia Meggs 2012-03-09 18:17:36 UTC
Created attachment 8129 [details] [review]
Signed off follow up patch
Comment 26 Melia Meggs 2012-03-09 18:18:41 UTC
Follow up patch corrects for duplicate call numbers in the database.  Works better now, so I am signing off.
Comment 27 Ian Walls 2012-03-09 20:05:24 UTC
Simple change to SQL to get distinct callnumbers.  Only affects this plugin, so marking this as Passed QA.
Comment 28 Paul Poulain 2012-03-12 17:20:54 UTC
follow-up pushed, still untested by me as it's very specific
Comment 29 Jared Camins-Esakov 2012-12-31 00:42:46 UTC
There have been no further reports of problems so I am marking this bug resolved.