Bug 17642 - Authorised values code is broken because of the refactoring
Summary: Authorised values code is broken because of the refactoring
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Jonathan Druart
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 15799
Blocks: 18435
  Show dependency treegraph
 
Reported: 2016-11-16 10:48 UTC by Jonathan Druart
Modified: 2019-06-27 09:24 UTC (History)
7 users (show)

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


Attachments
Bug 17642: Try to fix AV code (6.22 KB, patch)
2016-11-16 10:49 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17642: Try to fix AV code (6.22 KB, patch)
2016-11-16 17:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17642: Add find_by_koha_field (4.87 KB, patch)
2016-11-16 17:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17642: use find_by_koha_field when needed (8.94 KB, patch)
2016-11-16 17:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17642: Add get_description_by_koha_field (4.17 KB, patch)
2016-11-16 17:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17642: use get_description_by_koha_field when needed (11.05 KB, patch)
2016-11-16 17:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17642: Add and use get_descriptions_by_koha_field (17.85 KB, patch)
2016-11-16 17:31 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17642: Do not explode if no authorised value exist (2.21 KB, patch)
2016-11-17 11:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 17642: [QA Follow-up] Issues_stats.pl is not plack safe (1.99 KB, patch)
2016-11-18 07:39 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17642: Try to fix AV code (6.32 KB, patch)
2016-11-18 10:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17642: Add find_by_koha_field (4.97 KB, patch)
2016-11-18 10:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17642: use find_by_koha_field when needed (9.03 KB, patch)
2016-11-18 10:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17642: Add get_description_by_koha_field (4.27 KB, patch)
2016-11-18 10:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17642: use get_description_by_koha_field when needed (11.14 KB, patch)
2016-11-18 10:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17642: Add and use get_descriptions_by_koha_field (17.95 KB, patch)
2016-11-18 10:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17642: Do not explode if no authorised value exist (2.31 KB, patch)
2016-11-18 10:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17642: [QA Follow-up] Issues_stats.pl is not plack safe (2.05 KB, patch)
2016-11-18 10:21 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17642: Try to fix AV code (6.34 KB, patch)
2016-11-18 15:50 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17642: Add find_by_koha_field (4.99 KB, patch)
2016-11-18 15:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17642: use find_by_koha_field when needed (9.05 KB, patch)
2016-11-18 15:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17642: Add get_description_by_koha_field (4.29 KB, patch)
2016-11-18 15:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17642: use get_description_by_koha_field when needed (11.17 KB, patch)
2016-11-18 15:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17642: Add and use get_descriptions_by_koha_field (17.97 KB, patch)
2016-11-18 15:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17642: Do not explode if no authorised value exist (2.33 KB, patch)
2016-11-18 15:51 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 17642: [QA Follow-up] Issues_stats.pl is not plack safe (2.07 KB, patch)
2016-11-18 15:51 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-11-16 10:48:17 UTC

    
Comment 1 Jonathan Druart 2016-11-16 10:48:45 UTC
From bug 17250 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.
Comment 2 Jonathan Druart 2016-11-16 10:49:38 UTC
Created attachment 57533 [details] [review]
Bug 17642: Try to fix AV code
Comment 3 Jonathan Druart 2016-11-16 10:51:29 UTC
Josef, I have tried to fix the issue you raised on bug 17250. I have to run and I don't have time to polish this patch but, if you can, please try it and tell me if it fixes everything.
We need to be very careful about these changes, that can easily introduce side effects.
Comment 4 Jonathan Druart 2016-11-16 14:35:02 UTC
It was not Josef, but Jacek, sorry guys :)
Comment 5 Jonathan Druart 2016-11-16 17:31:01 UTC
Created attachment 57569 [details] [review]
Bug 17642: Try to fix AV code
Comment 6 Jonathan Druart 2016-11-16 17:31:05 UTC
Created attachment 57570 [details] [review]
Bug 17642: Add find_by_koha_field

When we call search_by_koha_field with an authorised_value, we actually
expect only 1 value
Comment 7 Jonathan Druart 2016-11-16 17:31:09 UTC
Created attachment 57571 [details] [review]
Bug 17642: use find_by_koha_field when needed
Comment 8 Jonathan Druart 2016-11-16 17:31:12 UTC
Created attachment 57572 [details] [review]
Bug 17642: Add get_description_by_koha_field

Most of the time we just need the descriptions (lib or
opac_description), so let's add a new method for that and cache the
descriptions in L1.
Ideally we should cache it in L2 as well, but the AV code is not robust
enough to allow that
Comment 9 Jonathan Druart 2016-11-16 17:31:16 UTC
Created attachment 57573 [details] [review]
Bug 17642: use get_description_by_koha_field when needed
Comment 10 Jonathan Druart 2016-11-16 17:31:20 UTC
Created attachment 57574 [details] [review]
Bug 17642: Add and use get_descriptions_by_koha_field

Ok I am silly, we needed to replace to use the cache mechanism for
search_by_koha_field, not find_by_koha_field...
Let's create another subroutine
Comment 11 Jonathan Druart 2016-11-16 17:32:18 UTC
As describe by Jacek on bug 17250 comment 16, we have 2 major problems here:
1/ A critical one is that the feature is wrong and either explode or return bad result => Has to be fixed before the release. This *could* be fixed with only the first patch
2/ The search will take now more time than before the AV refactoring due to DBIx::Class calls (slower than DBI ofc).
That *could* be fixed with the whole patch set. But as you can see, it's not trivial changes and if affects the code in a lot of places.

I really need feedbacks quickly on this patch set to polish it to make it acceptable. I won't work on it before the release if we decide to delay its integration.
Comment 12 Kyle M Hall 2016-11-16 20:02:41 UTC
(In reply to Jonathan Druart from comment #11)
> As describe by Jacek on bug 17250 comment 16, we have 2 major problems here:
> 1/ A critical one is that the feature is wrong and either explode or return
> bad result => Has to be fixed before the release. This *could* be fixed with
> only the first patch
> 2/ The search will take now more time than before the AV refactoring due to
> DBIx::Class calls (slower than DBI ofc).
> That *could* be fixed with the whole patch set. But as you can see, it's not
> trivial changes and if affects the code in a lot of places.
> 
> I really need feedbacks quickly on this patch set to polish it to make it
> acceptable. I won't work on it before the release if we decide to delay its
> integration.

I like the concept you have here. I haven't had a chance to actually test it yet, but I think it's work proceeding!
Comment 13 Jonathan Druart 2016-11-17 11:24:50 UTC
Created attachment 57595 [details] [review]
Bug 17642: Do not explode if no authorised value exist

Can't call method "lib" on an undefined value at
/home/vagrant/kohaclone/Koha/AuthorisedValues.pm line 137.
Comment 14 Marcel de Rooy 2016-11-17 13:46:11 UTC
t/db_dependent/AuthorisedValues.t: Would be nice if tests got a description
Koha/AuthorizedValues.pm: No POD for methods; is there still any use for find_by_koha_field and search_by_koha_field?

Squash these patches ?
Comment 15 Jonathan Druart 2016-11-17 14:15:47 UTC
(In reply to Marcel de Rooy from comment #14)
> t/db_dependent/AuthorisedValues.t: Would be nice if tests got a description
> Koha/AuthorizedValues.pm: No POD for methods; is there still any use for
> find_by_koha_field and search_by_koha_field?
> 
> Squash these patches ?

No please don't squash.
The 2 methods are still used from the get_description* methods.
Comment 16 Marcel de Rooy 2016-11-17 14:19:31 UTC
(In reply to Jonathan Druart from comment #15)
> The 2 methods are still used from the get_description* methods.
OK I see
Comment 17 Marcel de Rooy 2016-11-18 07:30:32 UTC
QA: Working on this one now
Comment 18 Marcel de Rooy 2016-11-18 07:39:57 UTC
Created attachment 57616 [details] [review]
Bug 17642: [QA Follow-up] Issues_stats.pl is not plack safe

Resolve:
Variable "$ccodes" is not available at reports/issues_stats.pl line 358.
Variable "$locations" is not available at reports/issues_stats.pl line 359.
Variable "$ccodes" is not available at reports/issues_stats.pl line 358.
Variable "$locations" is not available at reports/issues_stats.pl line 359.

Test plan:
Run the script. Choose Collection or Shelving Location.
Comment 19 Jonathan Druart 2016-11-18 09:34:44 UTC
Quick other benchmarks using time wget opac url (plack + memcached + 100 results displayed with default XSLT):
prior to refactoring (430a641): 4.7-5.2s
master: 8-10s
master + 17642: 5-5.5s
Comment 20 Marcel de Rooy 2016-11-18 10:17:02 UTC
Timing XSLTParse4Display( 1, $record, 'OPACXSLTResultsDisplay', 1, 0 );
Commit 0faf643661f23eddd066488b26004428e57bfe18
master-koha@WRKM006:/usr/share/koha/masterclone$ perl zz 2>/dev/null
  0.190
  0.093
  0.079
  0.076
  0.075
Previous commit plus 17642 patches
master-koha@WRKM006:/usr/share/koha/masterclone$ perl zz
  0.173
  0.042
  0.025
  0.023
  0.024
When you have sufficient hits and increase the number of results per page, you will gain more.

In Firefox I am seeing 4,6-5,3s versus around 4,1 - 4,6s [new].
Comment 21 Marcel de Rooy 2016-11-18 10:21:11 UTC
Created attachment 57628 [details] [review]
Bug 17642: Try to fix AV code

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 22 Marcel de Rooy 2016-11-18 10:21:15 UTC
Created attachment 57629 [details] [review]
Bug 17642: Add find_by_koha_field

When we call search_by_koha_field with an authorised_value, we actually
expect only 1 value

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 23 Marcel de Rooy 2016-11-18 10:21:20 UTC
Created attachment 57630 [details] [review]
Bug 17642: use find_by_koha_field when needed

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 24 Marcel de Rooy 2016-11-18 10:21:24 UTC
Created attachment 57631 [details] [review]
Bug 17642: Add get_description_by_koha_field

Most of the time we just need the descriptions (lib or
opac_description), so let's add a new method for that and cache the
descriptions in L1.
Ideally we should cache it in L2 as well, but the AV code is not robust
enough to allow that

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Marcel de Rooy 2016-11-18 10:21:29 UTC
Created attachment 57632 [details] [review]
Bug 17642: use get_description_by_koha_field when needed

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 26 Marcel de Rooy 2016-11-18 10:21:33 UTC
Created attachment 57633 [details] [review]
Bug 17642: Add and use get_descriptions_by_koha_field

Ok I am silly, we needed to replace to use the cache mechanism for
search_by_koha_field, not find_by_koha_field...
Let's create another subroutine

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 27 Marcel de Rooy 2016-11-18 10:21:38 UTC
Created attachment 57634 [details] [review]
Bug 17642: Do not explode if no authorised value exist

Can't call method "lib" on an undefined value at
/home/vagrant/kohaclone/Koha/AuthorisedValues.pm line 137.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2016-11-18 10:21:43 UTC
Created attachment 57635 [details] [review]
Bug 17642: [QA Follow-up] Issues_stats.pl is not plack safe

Resolve:
Variable "$ccodes" is not available at reports/issues_stats.pl line 358.
Variable "$locations" is not available at reports/issues_stats.pl line 359.
Variable "$ccodes" is not available at reports/issues_stats.pl line 358.
Variable "$locations" is not available at reports/issues_stats.pl line 359.

Test plan:
Run the script. Choose Collection or Shelving Location.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 29 Marcel de Rooy 2016-11-18 10:24:30 UTC
QA Comment:
Looks good to me: bug fixed, performance seems okay.
Another signoff or additional QA signoff is welcome.
But in case these are not available, I would be happy to promote this to PQA.
Comment 30 Martin Renvoize 2016-11-18 12:34:26 UTC
I dug through this code a little over the past few days too and my gut feeling was that it seemed a step in the right direction and safe.

I hadn't signed off/qa'd though as I felt I wasn't in a position in koha recently to fully anticipate any possible fallout in this particular area of the codebase. 

It looks like you've done a thorough job of testing and performance checking from here Marcel so you'd have my +1 for PQA.
Comment 31 Marcel de Rooy 2016-11-18 13:25:18 UTC
Asked Martin to add a signoff. He also did limited testing and looked at the code. That makes my signoff be the QA pass. (Note earlier comments about 'polishing', POD/test descriptions etc.)
Comment 32 Martin Renvoize 2016-11-18 15:50:55 UTC
Created attachment 57650 [details] [review]
Bug 17642: Try to fix AV code

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Martin Renvoize 2016-11-18 15:51:00 UTC
Created attachment 57651 [details] [review]
Bug 17642: Add find_by_koha_field

When we call search_by_koha_field with an authorised_value, we actually
expect only 1 value

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Martin Renvoize 2016-11-18 15:51:06 UTC
Created attachment 57652 [details] [review]
Bug 17642: use find_by_koha_field when needed

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Martin Renvoize 2016-11-18 15:51:12 UTC
Created attachment 57653 [details] [review]
Bug 17642: Add get_description_by_koha_field

Most of the time we just need the descriptions (lib or
opac_description), so let's add a new method for that and cache the
descriptions in L1.
Ideally we should cache it in L2 as well, but the AV code is not robust
enough to allow that

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Martin Renvoize 2016-11-18 15:51:17 UTC
Created attachment 57654 [details] [review]
Bug 17642: use get_description_by_koha_field when needed

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Martin Renvoize 2016-11-18 15:51:22 UTC
Created attachment 57655 [details] [review]
Bug 17642: Add and use get_descriptions_by_koha_field

Ok I am silly, we needed to replace to use the cache mechanism for
search_by_koha_field, not find_by_koha_field...
Let's create another subroutine

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Martin Renvoize 2016-11-18 15:51:31 UTC
Created attachment 57656 [details] [review]
Bug 17642: Do not explode if no authorised value exist

Can't call method "lib" on an undefined value at
/home/vagrant/kohaclone/Koha/AuthorisedValues.pm line 137.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 39 Martin Renvoize 2016-11-18 15:51:36 UTC
Created attachment 57657 [details] [review]
Bug 17642: [QA Follow-up] Issues_stats.pl is not plack safe

Resolve:
Variable "$ccodes" is not available at reports/issues_stats.pl line 358.
Variable "$locations" is not available at reports/issues_stats.pl line 359.
Variable "$ccodes" is not available at reports/issues_stats.pl line 358.
Variable "$locations" is not available at reports/issues_stats.pl line 359.

Test plan:
Run the script. Choose Collection or Shelving Location.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 40 Martin Renvoize 2016-11-18 15:52:31 UTC
Just adding in the signoff line to verify  that indeed I did test and was happy in the signoff capacity.. Thanks for QAing Marcel :)
Comment 41 Kyle M Hall 2016-11-18 15:54:05 UTC
Pushed to master for 16.11, thanks Jonathan, Marcel!
Comment 42 Mason James 2016-12-14 11:34:18 UTC
(In reply to Kyle M Hall from comment #41)
> Pushed to master for 16.11, thanks Jonathan, Marcel!

hmm, this patch seems to depend on a not-yet-pushed patch (15799)
is that ok?

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15799
Comment 43 Jonathan Druart 2016-12-14 12:02:25 UTC
(In reply to Mason James from comment #42)
> (In reply to Kyle M Hall from comment #41)
> > Pushed to master for 16.11, thanks Jonathan, Marcel!
> 
> hmm, this patch seems to depend on a not-yet-pushed patch (15799)
> is that ok?
> 
> https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15799

Yes, bug 15799 is an omnibus. But you do not need it for 16.05.