Bug 13146

Frédéric, It would be great to provide a couple of tests to confirm the patch does not introduce any regressions.

Looks like the results are the same with or without the patch. Also the SQL calls disappeared (good!). But I'm not sure what is subtitle field mapping. Is it about MARC records subfield something?

Created attachment 44132 [details] [review]
Bug 13146 UT

Run the test before applying the second patch:

  prove -v t/db_dependent/FieldMapping.t

Apply the 2nd patch, and redo the test.

Created attachment 44133 [details] [review]
Bug 13146 Improve GetRecordValue function by caching field mapping

GetRecordValue function is called each time a specific field mapping is
required. For example, bug 12692 display biblio record subtitle for each hold
awaiting pickup or overdue. As it is implemented now, GetRecordValue queries
directly the fieldmapping table to determine in which tag/letter a field has to
be searched. It means, that the same mapping are requested over and over.

This patch uses Koha cache system to store a complete data structure
representing the field mapping.

TO TEST:

(1) Look at the code! and check that Koha caching system is properly used.

(2) Find out a Koha function where subtitle field mapping is used. Make you
    choice: sending a basket, tag list, overdues with fines, renew item, etc.

(3) Note that the display of subtitle field hasn't changed.

(4) If you really want to see how less queries are sent to MySQL with this
    patch, you have to monitor your MySQL log files!

    Suggestion to do it on a Debian box:

    - In MySQL client: SHOW VARIABLES LIKE "general_log%";
      It gives you MySQL log file path. Usually, /var/run/mysqld/mysqld.log
    - Enable log, with: SET GLOBAL general_log = 'ON';
    - Display a page displaying the subtitle field
    - Compare request send with/withouth this patch. Without the patch, you
      will see plenty of query like that:

      SELECT fieldcode, subfieldcode FROM fieldmapping

(In reply to Jonathan Druart from comment #3)
> Frédéric, It would be great to provide a couple of tests to confirm the
> patch does not introduce any regressions.

Done.

Comment on attachment 44132 [details] [review]
Bug 13146 UT

Review of attachment 44132 [details] [review]:
-----------------------------------------------------------------

Test::DBIx::Class isn't required to run Koha, just the tests, correct? Probably should skip tests we know we can't run. This will run fine on kohadevbox, but not necessarily on another users koha git box.

::: t/db_dependent/FieldMapping.t
@@ +21,5 @@
> +use File::Basename;
> +use File::Path;
> +use DateTime;
> +use Test::MockModule;
> +use Test::More tests => 18;

Just use Test::More; because of suggested change below.

@@ +25,5 @@
> +use Test::More tests => 18;
> +use C4::Biblio;
> +use Koha::Schema;
> +
> +

use Module::Load::Conditional qw/check_install/;

BEGIN {
    if ( check_install( module => 'Test::DBIx::Class' ) ) {
        plan tests => 18; # or is it 19... whatever works.
    } else {
        plan skip_all => "Need Test::DBIx::Class"
    }
}

@@ +43,5 @@
> +    _new_schema => sub { return Schema(); }
> +);
> +
> +
> +if (0) {

What is the point of keeping the code, if it isn't run?

@@ +55,5 @@
> +}
> +
> +
> +SetFieldMapping('', 'maintitle', '245', 'a');
> +ok my $fm = Fieldmapping->find( {field => 'maintitle'} )

Multiple side-effects on the same line of code is more difficult to read.

@@ +56,5 @@
> +
> +
> +SetFieldMapping('', 'maintitle', '245', 'a');
> +ok my $fm = Fieldmapping->find( {field => 'maintitle'} )
> +   => 'maintitle field mapping properly set for default framework';

While this is valid, given that you did the simpler version of is() below, could this not be:
my $fm = Fieldmapping->find( {field => 'maintitle'} );
ok( defined $fm, 'maintitle field mapping properly set for default framework');
instead? This is more readable.

@@ +60,5 @@
> +   => 'maintitle field mapping properly set for default framework';
> +
> +SetFieldMapping('', 'subtitle', '245', 'b');
> +ok $fm = Fieldmapping->find( {field => 'subtitle'} )
> +   => 'subtitle field mapping properly set for default framework';

same here.

@@ +70,5 @@
> +}, 'expected fields are there';
> +
> +SetFieldMapping('BOOK', 'subtitle', '245', 'b');
> +ok $fm = Fieldmapping->find( {frameworkcode => 'BOOK', field => 'subtitle'} )
> +   => 'subtitle field mapping properly set for BOOK framework';

same here.

Patch doesn't apply. It looked to me that SetFieldMapping has been removed since this was written?

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 13146 UT
Applying: Bug 13146 Improve GetRecordValue function by caching field mapping
Using index info to reconstruct a base tree...
M	C4/Biblio.pm
Falling back to patching base and 3-way merge...
Auto-merging C4/Biblio.pm
CONFLICT (content): Merge conflict in C4/Biblio.pm
Failed to merge in the changes.
Patch failed at 0001 Bug 13146 Improve GetRecordValue function by caching field mapping
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-13146-Improve-GetRecordValue-function-by-cachi-G9pMqQ.patch

I set invalid since Bug 11529 removed method C4::Biblio::GetRecordValue :

https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=91834