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.

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?

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.

(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!

Wanted to test, but patch does not apply, sorry.
fatal: sha1 information is lacking or useless (C4/Circulation.pm).

Sorry, missed dependency...

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>

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

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

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>

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>

Happy with code, no regressions found.

Pushed to master for 16.11, thanks Jonathan!

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).

(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.

(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)

Ha, C4::XSLT::buildKohaItemsNamespace !

(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.

(In reply to Jonathan Druart from comment #19)
> Ha, C4::XSLT::buildKohaItemsNamespace !

Yep ;)