Bug 13146 - Improve GetRecordValue function by caching field mapping
Summary: Improve GetRecordValue function by caching field mapping
Status: Patch doesn't apply
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Frédéric Demians
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 15342
  Show dependency treegraph
 
Reported: 2014-10-25 17:30 UTC by Frédéric Demians
Modified: 2017-10-15 11:12 UTC (History)
4 users (show)

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


Attachments
Bug 13146 Improve GetRecordValue function by caching field mapping (5.20 KB, patch)
2014-10-25 18:20 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13146 Improve GetRecordValue function by caching field mapping (5.10 KB, patch)
2014-10-25 21:25 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13146 UT (4.02 KB, patch)
2015-10-28 16:31 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 13146 Improve GetRecordValue function by caching field mapping (5.10 KB, patch)
2015-10-28 16:32 UTC, Frédéric Demians
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Demians 2014-10-25 17:30:57 UTC
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. The
Koha cache system should be used here, as for framework structure for example.
Comment 1 Frédéric Demians 2014-10-25 18:20:45 UTC Comment hidden (obsolete)
Comment 2 Frédéric Demians 2014-10-25 21:25:55 UTC Comment hidden (obsolete)
Comment 3 Jonathan Druart 2015-02-16 15:54:27 UTC
Frédéric, It would be great to provide a couple of tests to confirm the patch does not introduce any regressions.
Comment 4 j.kylmala 2015-08-31 12:25:10 UTC
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?
Comment 5 Frédéric Demians 2015-10-28 16:31:28 UTC
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.
Comment 6 Frédéric Demians 2015-10-28 16:32:11 UTC
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
Comment 7 Frédéric Demians 2015-10-28 16:32:47 UTC
(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 8 M. Tompsett 2017-01-12 17:03:38 UTC
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.
Comment 9 Katrin Fischer 2017-10-15 11:12:31 UTC
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