|
Description
Jonathan Druart
2016-11-16 10:48:17 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. Created attachment 57533 [details] [review] Bug 17642: Try to fix AV code 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. It was not Josef, but Jacek, sorry guys :) Created attachment 57569 [details] [review] Bug 17642: Try to fix AV code 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 Created attachment 57571 [details] [review] Bug 17642: use find_by_koha_field when needed 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 Created attachment 57573 [details] [review] Bug 17642: use get_description_by_koha_field when needed 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 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. (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! 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. 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 ? (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. (In reply to Jonathan Druart from comment #15) > The 2 methods are still used from the get_description* methods. OK I see QA: Working on this one now 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. 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 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]. Created attachment 57628 [details] [review] Bug 17642: Try to fix AV code Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> 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> 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> 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> 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> 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> 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> 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> 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. 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. 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.) 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> 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> 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> 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> 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> 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> 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> 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> Just adding in the signoff line to verify that indeed I did test and was happy in the signoff capacity.. Thanks for QAing Marcel :) Pushed to master for 16.11, thanks Jonathan, Marcel! (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 (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. |