https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17250 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |17249 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17249 [Bug 17249] Koha::AuthorisedValues - Remove GetKohaAuthorisedValuesFromField -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17250 --- Comment #1 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 55186 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55186&action=edit Bug 17250 - Koha::AuthorisedValues - Remove GetAuthValCode The subroutine C4::Koha::GetAuthValCode returned the authorised value category for a given kohafield. This can be acchieve easily using a new Koha::AuthorisedValues->search_by_koha_field method which will mimic search_by_marc_field. Test plan: Confirm that the description is correctly displayed on the following pages: - detail and moredetail of a bibliographic page (itemlost, damaged, materials) - Set AcqCreateItem=ordering and receiving items. The description for notforloan, restricted, location, ccode, etc. field should be displayed. - Items search form - On the checkout list from the circulation.pl and returns.pl pages, the description for "materials" should be displayed Note that GetKohaAuthorisedValuesMapping is going to be removed on bug 17251. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17250 Owen Leonard <oleonard@myacpl.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Patch doesn't apply --- Comment #2 from Owen Leonard <oleonard@myacpl.org> --- Sorry, this doesn't apply: Applying: Bug 17250 - Koha::AuthorisedValues - Remove GetAuthValCode fatal: sha1 information is lacking or useless (C4/Items.pm). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Patch failed at 0001 Bug 17250 - Koha::AuthorisedValues - Remove GetAuthValCode I really thought I had all the correct dependencies applied. I applied this patch right on top of the branch I used to test Bug 17249. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17250 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #55186|0 |1 is obsolete| | --- Comment #3 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 55494 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55494&action=edit Bug 17250 - Koha::AuthorisedValues - Remove GetAuthValCode The subroutine C4::Koha::GetAuthValCode returned the authorised value category for a given kohafield. This can be acchieve easily using a new Koha::AuthorisedValues->search_by_koha_field method which will mimic search_by_marc_field. Test plan: Confirm that the description is correctly displayed on the following pages: - detail and moredetail of a bibliographic page (itemlost, damaged, materials) - Set AcqCreateItem=ordering and receiving items. The description for notforloan, restricted, location, ccode, etc. field should be displayed. - Items search form - On the checkout list from the circulation.pl and returns.pl pages, the description for "materials" should be displayed Note that GetKohaAuthorisedValuesMapping is going to be removed on bug 17251. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17250 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17250 --- Comment #6 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Owen Leonard from comment #4)

When I go to detail.pl or moredetail.pl I get this error:

Can't use string ("Koha::MarcSubfieldStructure") as a HASH ref while "strict refs" in use at Koha/Object.pm line 248.

Did I miss a dependency or something?

No, my bad. Sorry! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17250 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Patch doesn't apply |Needs Signoff --- Comment #8 from Marc Véron <veron@veron.ch> --- Sorry, missed dependency... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17250 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17250 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #56269|0 |1 is obsolete| | --- Comment #11 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 56291 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56291&action=edit Bug 17250: Do not retrieve marc subfield structure when the authorised value is not defined GetAuthValCode did not return anything if the authorised_value column was not defined. Our new calls to Koha::MarcSubfieldStructures->search should behave the same -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17250 --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 56391 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56391&action=edit [PASSED QA] Bug 17250: Do not retrieve marc subfield structure when the authorised value is not defined GetAuthValCode did not return anything if the authorised_value column was not defined. Our new calls to Koha::MarcSubfieldStructures->search should behave the same Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17250 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA --- Comment #14 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Happy with code, no regressions found. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17250 Jacek Ablewicz <abl@biblos.pk.edu.pl> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |abl@biblos.pk.edu.pl --- Comment #16 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- This code (C4/Items.pm, line 1378): # get notforloan complete status if applicable $av = Koha::AuthorisedValues->search_by_koha_field({ frameworkcode => $data->{frameworkcode}, kohafield => 'items.notforloan', authorised_value => $data->{itemnotforloan} }); $av = $av->count ? $av->next : undef; (and the next two similar statements below, for items.restricted and items.stack) doesn't work for me as expected. I'm testing on current master; same problem[s] on jessie and wheezy. If authorised value code in a given item record is undef|NULL or 0 (items.itemnotfoloan is most often 0, items.restricted is usually NULL - depends on the database contents), ->search_by_koha_field->count returns more then 1, and ->next returns some random (usually the 1st one in DB, but it's not guaranteed) AV record, eg. with result description being 'Ordered' for item.itemnoforloan = 0. If itemnoforloan in item record is not null and not zero, e.g. 1 or -1 (= item not for loan, item ordered), $av->count fails (it's not a fatal error) with the following warnings in the logs: DBD::mysql::st execute failed: Column 'authorised_value' in where clause is ambiguous [for Statement "SELECT COUNT( * ) FROM (SELECT `me`.`id`, `me`.`category`, `me`.`authorised_value`, `me`.`lib`, `me`.`lib_opac`, `me`.`imageurl` FROM `authorised_values` `me` JOIN `authorised_value_categories` `category` ON `category`.`category_name` = `me`.`category` LEFT JOIN `marc_subfield_structure` `marc_subfield_structures` ON `marc_subfield_structures`.`authorised_value` = `category`.`category_name` WHERE ( ( `authorised_value` = ? AND `marc_subfield_structures`.`frameworkcode` = ? AND `marc_subfield_structures`.`kohafield` = ? ) ) GROUP BY `me`.`id`, `me`.`category`, `me`.`authorised_value`, `me`.`lib`, `me`.`lib_opac`, `me`.`imageurl`) `me`" with ParamValues: 0=-1, 1='', 2='items.notforloan'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832. No method count found for Koha::AuthorisedValues DBIx::Class::Storage::DBI::_dbh_execute(): Column 'authorised_value' in where clause is ambiguous at /home/koha/devkohaclone/Koha/Objects.pm line 307 and the result AV descriptions are empty strings. Another side effect of those code changes is that search speed is now considerably slower, after converting it to DBIx, ->search_by_koha_field followed by ->count and|or ->next is taking ca 8-14 miliseconds; for a search with 50 results (and 2 items per biblio record on average), search performance penalty is somewhere around: 0.012 * 50 * 2 * 3 = 3.6 seconds (= ~15-25% of the CPU cycles involved in the search are spent in those 3 code parts, while in pre-DBIx version, it was 3-5% of the cycles). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17250 --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #17)

...

Another side effect of those code changes is that search speed is now considerably slower, after converting it to DBIx, ->search_by_koha_field followed by ->count and|or ->next is taking ca 8-14 miliseconds; for a search with 50 results (and 2 items per biblio record on average), search performance penalty is somewhere around:

0.012 * 50 * 2 * 3 = 3.6 seconds

(= ~15-25% of the CPU cycles involved in the search are spent in those 3 code parts, while in pre-DBIx version, it was 3-5% of the cycles).

I expected a speed difference, but not so big. The idea behind the AV refactoring is to cache everything to speed up this area. I will need to focus on that.

A quick benchmark using firebug: I notice 1.7s to 2.1s between 1e17c9290 and master. Why do you think ->search_by_koha_field is the guilty? It seems only called one from searchResults (for a search) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17250 --- Comment #20 from Jacek Ablewicz <abl@biblos.pk.edu.pl> --- (In reply to Jonathan Druart from comment #18)

Why do you think ->search_by_koha_field is the guilty? It seems only called one from searchResults (for a search)

In my test it (and the following ->count / ->next) got called 3 times per each item in the search results (100 results = w/ 198 items total): <http://koha-ms.biblos.pk.edu.pl/local/temp/nytprof.41_100_results/C4-Items-pm-1355-line.html#1304> <http://koha-ms.biblos.pk.edu.pl/local/temp/nytprof.41_100_results/Koha-AuthorisedValues-pm-1165-line.html#85> (this particular profile is from master @ wheezy, but I measured it in master @ jessie manually, and it does not seem to be much different speed-wise, despite the newer and more optimised version of DBIx::Class in jessie). But it is probably config dependent - I profiled this search with all XSLT* sysprefs enabled (i.e. set to 'default'). I'm wondering if the AV descriptions fetched in GetItemsInfo() are even used for anything (for the search results page).. I think they are used in biblio details display (but I'm not entirely sure about that either), maybe this code can be removed, or called only when really necessary. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17250 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |20067 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20067 [Bug 20067] Authorised values for ORDER_CANCELLATION_REASON are displayed on notice (catalogue/detail.pl) -- You are receiving this mail because: You are watching all bug changes.