Bug 17250 - Koha::AuthorisedValues - Remove GetAuthValCode
Summary: Koha::AuthorisedValues - Remove GetAuthValCode
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 17249
Blocks: 15799 17251 20067
  Show dependency treegraph
 
Reported: 2016-09-05 14:16 UTC by Jonathan Druart
Modified: 2018-02-22 22:58 UTC (History)
4 users (show)

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


Attachments
Bug 17250 - Koha::AuthorisedValues - Remove GetAuthValCode (23.80 KB, patch)
2016-09-05 15:40 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17250 - Koha::AuthorisedValues - Remove GetAuthValCode (23.77 KB, patch)
2016-09-12 14:54 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17250 - Koha::AuthorisedValues - Remove GetAuthValCode (23.78 KB, patch)
2016-09-15 08:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17250 - Koha::AuthorisedValues - Remove GetAuthValCode (23.95 KB, patch)
2016-10-04 10:23 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 17250: Do not retrieve marc subfield structure when the authorised value is not defined (8.00 KB, patch)
2016-10-12 12:53 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17250: Do not retrieve marc subfield structure when the authorised value is not defined (8.00 KB, patch)
2016-10-12 14:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
[PASSED QA] Bug 17250 - Koha::AuthorisedValues - Remove GetAuthValCode (24.03 KB, patch)
2016-10-13 13:00 UTC, Martin Renvoize
Details | Diff | Splinter Review
[PASSED QA] Bug 17250: Do not retrieve marc subfield structure when the authorised value is not defined (8.07 KB, patch)
2016-10-13 13:00 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2016-09-05 14:16:02 UTC

    
Comment 1 Jonathan Druart 2016-09-05 15:40:55 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2016-09-09 13:00:30 UTC
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.
Comment 3 Jonathan Druart 2016-09-12 14:54:38 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2016-09-14 16:28:33 UTC
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?
Comment 5 Jonathan Druart 2016-09-15 08:06:49 UTC
Created attachment 55580 [details] [review]
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.
Comment 6 Jonathan Druart 2016-09-15 08:07:22 UTC
(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!
Comment 7 Marc Véron 2016-10-04 09:45:13 UTC
Wanted to test, but patch does not apply, sorry.
fatal: sha1 information is lacking or useless (C4/Circulation.pm).
Comment 8 Marc Véron 2016-10-04 09:46:34 UTC
Sorry, missed dependency...
Comment 9 Marc Véron 2016-10-04 10:23:40 UTC
Created attachment 56017 [details] [review]
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.

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>
Comment 10 Jonathan Druart 2016-10-12 12:53:49 UTC
Created attachment 56269 [details] [review]
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
Comment 11 Jonathan Druart 2016-10-12 14:30:22 UTC
Created attachment 56291 [details] [review]
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
Comment 12 Martin Renvoize 2016-10-13 13:00:45 UTC
Created attachment 56390 [details] [review]
[PASSED QA] 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.

Followed test plan, works as expected.
Signed-off-by: Marc Véron <veron@veron.ch>

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2016-10-13 13:00:59 UTC
Created attachment 56391 [details] [review]
[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>
Comment 14 Martin Renvoize 2016-10-13 13:01:54 UTC
Happy with code, no regressions found.
Comment 15 Kyle M Hall 2016-10-21 15:33:08 UTC
Pushed to master for 16.11, thanks Jonathan!
Comment 16 Jacek Ablewicz 2016-11-16 09:17:37 UTC
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).
Comment 17 Jonathan Druart 2016-11-16 10:53:17 UTC
(In reply to Jacek Ablewicz from comment #16)
> 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.

See bug 17642 for a fix.

> 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.
Comment 18 Jonathan Druart 2016-11-16 14:32:48 UTC
(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)
Comment 19 Jonathan Druart 2016-11-16 15:13:01 UTC
Ha, C4::XSLT::buildKohaItemsNamespace !
Comment 20 Jacek Ablewicz 2016-11-16 15:18:49 UTC
(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.
Comment 21 Jacek Ablewicz 2016-11-16 15:19:19 UTC
(In reply to Jonathan Druart from comment #19)
> Ha, C4::XSLT::buildKohaItemsNamespace !

Yep ;)