Bug 6557 - biblioitems.totalissues unused
Summary: biblioitems.totalissues unused
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Database (show other bugs)
Version: 3.10
Hardware: All All
: P3 normal (vote)
Target Milestone: ---
Assignee: Jared Camins-Esakov
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 8115 19310
  Show dependency treegraph
 
Reported: 2011-07-06 11:49 UTC by Nicole C. Engard
Modified: 2017-09-13 18:56 UTC (History)
5 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 6557: Syspref to control total issue counting (3.83 KB, patch)
2012-05-24 19:38 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6557: automatically increment totalissues (3.31 KB, patch)
2012-05-24 19:38 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6557: add script to update totalissues from stats (12.21 KB, patch)
2012-05-24 19:38 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6557: Syspref to control total issue counting (4.08 KB, patch)
2012-06-06 15:39 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6557: automatically increment totalissues (3.49 KB, patch)
2012-06-06 15:39 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6557: add script to update totalissues from stats (12.39 KB, patch)
2012-06-06 15:40 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6557: add script to update totalissues from stats (12.43 KB, patch)
2012-06-06 15:49 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6557: add script to update totalissues from stats (12.47 KB, patch)
2012-06-06 16:05 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6557: add script to update totalissues from stats (13.30 KB, patch)
2012-06-06 16:22 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6557: add script to update totalissues from stats (13.30 KB, patch)
2012-06-06 16:24 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6557: add script to update totalissues from stats (13.63 KB, patch)
2012-06-06 16:37 UTC, Jared Camins-Esakov
Details | Diff | Splinter Review
Bug 6557: Syspref to control total issue counting (20.50 KB, patch)
2012-06-06 16:50 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 6557: Record bib popularity in totalissues (21.07 KB, patch)
2012-06-08 14:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 6557 follow-up UNIMARC 924$0 in default french frameworks (24.77 KB, patch)
2012-06-29 13:24 UTC, Paul Poulain
Details | Diff | Splinter Review
Bug 6557 follow-up UNIMARC 942$0 in default french frameworks (24.77 KB, patch)
2012-06-29 13:25 UTC, Paul Poulain
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2011-07-06 11:49:28 UTC
The totalissues column in the biblioitems table appears to not be populated. I have tested on my system and had another Koha user test on theirs.  This column should be populated or it should be removed if it has been replaced with the items.issues column.

Nicole
Comment 1 D Ruth Bavousett 2012-01-05 13:36:28 UTC
Nicole,

The funny thing is, I'm discovering, that the *index* of this is what's used to derive popularity sorting the in OPAC.  So it's used--but nothing in Koha ever sets it.  I can set it in migration scripts easily (filling 942$0 with the sum of the 952$l values), but nothing will ever update that value in Koha.
Comment 2 Nicole C. Engard 2012-01-05 13:52:26 UTC
Yikes! That's not good at all. Upping the priority a bit here.
Comment 3 Jared Camins-Esakov 2012-05-17 23:43:25 UTC
Part 1. Basic description.
Although Koha has had the option to sort search results by “popularity” for years, the feature was never fully implemented. A working popularity sort would allow patrons (and staff) to view their search results sorted according to which items are most popular. In order to implement this feature 942$0 (total issues) will need to be automatically updated when items are checked out, and the indexing configuration adjusted slightly to take advantage of popularity data.

Part 2. Additional features.
Several additional changes are required to take advantage of the improved popularity support:
* A system preference is required to allow users to disable the automatic popularity index update
* The DOM indexing configuration should be set up so that popularity can automatically be computed by summing the number of times each item was issued (this will be done on a separate bug report, since the DOM indexing configuration is dependent on bug 7818, and the rest of the work on this bug is not)
* A script to populate field 942$0 with historical data is needed, since Koha has not been keeping 942$0 up to date
Comment 4 Jared Camins-Esakov 2012-05-24 19:38:28 UTC Comment hidden (obsolete)
Comment 5 Jared Camins-Esakov 2012-05-24 19:38:40 UTC Comment hidden (obsolete)
Comment 6 Jared Camins-Esakov 2012-05-24 19:38:48 UTC Comment hidden (obsolete)
Comment 7 Jared Camins-Esakov 2012-05-24 19:39:55 UTC
Changing to normal because after some thought I've decided this could conceivably be safe for backporting.
Comment 8 Liz Rea 2012-06-05 17:15:04 UTC
I found a problem in step 6 of the testing protocol - I have issues in my DB but every item shows 0 issues when the script is run.

Trivial problem - the documentation refers to update_total_issues.pl, the name of the script in the filesystem is update_totalissues.pl.
Comment 9 Jared Camins-Esakov 2012-06-06 15:39:39 UTC Comment hidden (obsolete)
Comment 10 Jared Camins-Esakov 2012-06-06 15:39:50 UTC Comment hidden (obsolete)
Comment 11 Jared Camins-Esakov 2012-06-06 15:40:01 UTC Comment hidden (obsolete)
Comment 12 Jared Camins-Esakov 2012-06-06 15:49:13 UTC Comment hidden (obsolete)
Comment 13 Jared Camins-Esakov 2012-06-06 16:05:35 UTC Comment hidden (obsolete)
Comment 14 Jared Camins-Esakov 2012-06-06 16:22:20 UTC Comment hidden (obsolete)
Comment 15 Jared Camins-Esakov 2012-06-06 16:24:04 UTC Comment hidden (obsolete)
Comment 16 Jared Camins-Esakov 2012-06-06 16:37:06 UTC Comment hidden (obsolete)
Comment 17 Liz Rea 2012-06-06 16:50:28 UTC Comment hidden (obsolete)
Comment 18 Ian Walls 2012-06-06 19:26:50 UTC
UpdateTotalIssues() has an avoidable inefficiency:  it calls both GetBiblioData, and GetMarcBiblio.  GetBiblioData returns all columns from biblio, biblioitems and itemtypes.  We only need biblio.frameworkcode, biblioitems.totalissues and the $record (which can be obtained from biblioitems.marcxml).

Additionally, the call from C4/Circulation to UpdateTotalIssues() occurs in AddIssue, which already has a copy of the biblio pulled in from a call to GetBiblioFromItemnumber (biblioitems.marc and biblioitems.marcxml).

So we already have all the data we need in AddIssues to do the update; it would just be a matter of creating the MARC::Record object and doing a ModBiblio to update the right field.

Modifying the cronjob to also return the MARC record and frameworkcode would let both means interface with the subroutine, so the library can still decide whether to take the slightened performance hit (or build from stats the first time).
Comment 19 Ian Walls 2012-06-08 13:36:09 UTC
Talking this out in IRC, while it may be more efficient to modify the subroutine to take in parts that we already have in AddIssue, it would not make for clean, readable code.  And since computing popularity really doesn't need to be an up-to-the-minute thing, and a script is provided to do it on cron, I just can't justify holding up QA for this.

Code is very clean, well documented and non-invasive.  Provides a mechanism for both real-time and scheduled updates, and the initial population can be handled using the cron script as well.

Assigning to Marcel for QA
Comment 20 Marcel de Rooy 2012-06-08 14:03:46 UTC
Created attachment 9986 [details] [review]
Bug 6557: Record bib popularity in totalissues
Comment 21 Marcel de Rooy 2012-06-08 15:02:41 UTC
QA Comment
Nice feature. Well documented incl. test plan. Code looks good.

Just a thought on UpdateTotalIssues:
BTW Note the comment from Ian earlier.
Also: something simple: checking if biblionumber makes sense? Record exists? (Strange things happen..)
Could be handled in the same followup suggested for Ians remarks.

Will your use of commit trigger commit-ineffective-warnings in most cases (autocommit will be on by default) ?
You do not have begin_work / rollback stuff.
Since you offer a commit parameter, I wonder if you promise too much? Normally, the update is written directly to the database.
If you actually would use a transaction with begin_work, you should check if it is possible etc. too.
No blocker for me.

process_stats could have used a few comments on the various queries used.
See now what it does, but should not need that deduction ;)

Passed QA
Comment 22 Paul Poulain 2012-06-09 10:04:24 UTC
Some questions about this patch:
 * why do you use a cronjob instead of UPDATing when there is any circulation. The items.issues field is updated this way, for consistency biblioitems.totalissue should be too
 * I can't see a case where a library would want NOT to record totalissue. Thus, I can't see a case where the syspref would be set to 0. So, is the syspref needed ? The fact that you declared this bug as "normal" and not as "enhancement" make me feel it's more a bugguy behaviour thant something the library would be able to change.
Comment 23 Jared Camins-Esakov 2012-06-09 11:27:52 UTC
Paul,

(In reply to comment #22)
> Some questions about this patch:
>  * why do you use a cronjob instead of UPDATing when there is any
> circulation. The items.issues field is updated this way, for consistency
> biblioitems.totalissue should be too

Updating the biblio at circ is very expensive. It is much better not to edit the bib record at all at circulation. By making the update-at-circ optional, we won't be giving any libraries with underpowered servers an unpleasant shock.

>  * I can't see a case where a library would want NOT to record totalissue.
> Thus, I can't see a case where the syspref would be set to 0. So, is the
> syspref needed ? The fact that you declared this bug as "normal" and not as
> "enhancement" make me feel it's more a bugguy behaviour thant something the
> library would be able to change.
Comment 24 Paul Poulain 2012-06-25 15:53:11 UTC
I can't decide myself to push this patch. Let me share my comments:
 * it add a syspref to decide wether we should update this field through a cron or in real time. The FAUSP (Fighters Against Useless SysPrefs) commitee has some concern: if it's long to do, who will set this syspref to 1 ? Small libraries have a low-grade hardware, large libraries circulate a lot. My conclusion is that noone will set the syspref to 1. So the syspref could/should be removed.
 * OTOH, the UpdateTotalIssues sub uses ModBiblio. It appears that it's because the popularity sorting is based on 942$0 field. that's why it's so long & I can't see another method to populate this field. 
 * In UNIMARC default setup, the totalissue field is attached to 942$0 like in MARC21. Quite surprising according to me, but not wrong afaik
 * The install documentation should be updated to reflect the misc/cronjobs/update_totalissues.pl (can be done later. Having crontab.example updated is a good 1st step)

Please comment my point 1 (2, 3 and 4 are just here as a reminder, even if 4 will require a follow-up or another bugzilla entry)
Comment 25 Jared Camins-Esakov 2012-06-25 16:01:33 UTC
Paul,

(In reply to comment #24)
> I can't decide myself to push this patch. Let me share my comments:
>  * it add a syspref to decide wether we should update this field through a
> cron or in real time. The FAUSP (Fighters Against Useless SysPrefs) commitee
> has some concern: if it's long to do, who will set this syspref to 1 ? Small
> libraries have a low-grade hardware, large libraries circulate a lot. My
> conclusion is that noone will set the syspref to 1. So the syspref
> could/should be removed.

I think it is worth having the syspref and the option for realtime update for two reasons:
1. in the future performance may improve, and this is definitely a feature that people might want. By including the syspref now, we forestall the requests that it be added.
2. For very small libraries, setting up a cron job can be a significant hardship, and if you're circulating one item per minute on average, the performance hit from realtime updating doesn't really matter.
3. (I'm like the Spanish inquisition, with fear, surprise, and ruthless efficiency... can I start over?) Although I consider the performance hit to be significant, the fact is we're not looking at an operation any more inefficient than checking out any item was in 3.2.

>  * OTOH, the UpdateTotalIssues sub uses ModBiblio. It appears that it's
> because the popularity sorting is based on 942$0 field. that's why it's so
> long & I can't see another method to populate this field. 
>  * In UNIMARC default setup, the totalissue field is attached to 942$0 like
> in MARC21. Quite surprising according to me, but not wrong afaik

I noticed that. And was baffled by it. But I figure, if it ain't broke...

>  * The install documentation should be updated to reflect the
> misc/cronjobs/update_totalissues.pl (can be done later. Having
> crontab.example updated is a good 1st step)
Comment 26 Marcel de Rooy 2012-06-27 14:11:10 UTC
(In reply to comment #24)
> The FAUSP (Fighters Against Useless SysPrefs) commitee
I recently saw this patch removing a pref :-)
http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8263

(In reply to comment #25)
> Although I consider the performance hit to be significant
Could you make this more concrete? Did you test the difference in performance? Instead of just mentioning some milliseconds on a specific server, could we come to a reasonably accurate percentage or so? 
If the performance increase would be below say 10%, we may not need two ways. But we probably do if we reach 50%?
Comment 27 Jared Camins-Esakov 2012-06-27 14:19:05 UTC
(In reply to comment #26)
> (In reply to comment #24)
> > The FAUSP (Fighters Against Useless SysPrefs) commitee
> I recently saw this patch removing a pref :-)
> http://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=8263
> 
> (In reply to comment #25)
> > Although I consider the performance hit to be significant
> Could you make this more concrete? Did you test the difference in
> performance? Instead of just mentioning some milliseconds on a specific
> server, could we come to a reasonably accurate percentage or so? 
> If the performance increase would be below say 10%, we may not need two
> ways. But we probably do if we reach 50%?

As I recall, the performance hit was around 30%. Reducing the performance hit at the cost of clean code separation is -- as I discussed with Ian -- not a good idea, because it would make maintenance a nightmare, and move us one step closer to C4::Search levels of mess. I would say that the syspref would be a candidate for future removal, once DOM has entirely replaced GRS-1 (can we hope this will be true for 3.14? Well, maybe 3.16), but I am not comfortable with adding a feature that legacy libraries will be unable to take advantage of (instant popularity updates).
Comment 28 Marcel de Rooy 2012-06-27 14:25:21 UTC
30% would justify the pref IMO.
Speaking about committees, is there already a committee to fight against Yet Another Cron Job ? ;)
Some day we need a layer on top of prefs and cronjobs. One intelligent cronjob that does everything based on settings and yes a few more prefs ..
Comment 29 Jared Camins-Esakov 2012-06-27 14:28:14 UTC
(In reply to comment #28)
> 30% would justify the pref IMO.
> Speaking about committees, is there already a committee to fight against Yet
> Another Cron Job ? ;)
> Some day we need a layer on top of prefs and cronjobs. One intelligent
> cronjob that does everything based on settings and yes a few more prefs ..

That would be very nice. I'd like to kill both the instant updating and the cron job (making it into a script that only needs to be run on special occasions or if you want something weird for popularity) for 3.16, but as I said, I'm just not comfortable with doing it now because of the ramifications this would have for libraries with legacy data.
Comment 30 Paul Poulain 2012-06-29 13:24:57 UTC Comment hidden (obsolete)
Comment 31 Paul Poulain 2012-06-29 13:25:28 UTC
Created attachment 10562 [details] [review]
Bug 6557 follow-up UNIMARC 942$0 in default french frameworks
Comment 32 Paul Poulain 2012-06-29 13:58:25 UTC
Patch pushed but NOTE :
* i've provided a follow-up for french UNIMARC frameworks, that had no 942$0 at all, making the script badly fail:
$ misc/cronjobs/update_totalissues.pl --use-items
Tag "0" is not a valid tag. at /home/paul/koha.dev/koha-community//C4/Biblio.pm line 3870

A follow-up could be written to add a more clear error message: if biblioitems.totalissues isn't linked to a MARC field, issue a message before dying.

* I'll also send a mail to koha-devel & koha-translate to warn ppl in case there is another language framework that has this problem

* I'll advertise this patch in my RM newsletter

* chris_c = in my opinion, it's a bugfix, but a very long standing one, and, if you want to backport it to 3.8, you'll have to write very clearly how to deal with it in the release note ! (I think it [c|sh]ould wait for 3.10)
Comment 33 Jared Camins-Esakov 2012-12-31 01:17:09 UTC
There have been no further reports of problems so I am marking this bug resolved.