Bug 13074 - C4::Items::_build_default_values_for_mod_marc should use Koha::Cache
Summary: C4::Items::_build_default_values_for_mod_marc should use Koha::Cache
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 12874 16428
Blocks: 7172 8089 16431
  Show dependency treegraph
 
Reported: 2014-10-11 19:04 UTC by Tomás Cohen Arazi
Modified: 2017-12-07 22:17 UTC (History)
9 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 13074: Use Koha::Cache to cache the defaults values of a MARC record (8.57 KB, patch)
2016-05-03 10:15 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13074: Use Koha::Cache to cache the defaults values of a MARC record (10.66 KB, patch)
2016-05-03 13:10 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13074: Use Koha::Cache to cache the defaults values of a MARC record (10.71 KB, patch)
2016-05-09 14:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13074: (followup) remove useless diag (962 bytes, patch)
2016-05-09 14:14 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13074: Use Koha::Cache to cache the defaults values of a MARC record (10.68 KB, patch)
2016-06-10 16:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13074: (followup) remove useless diag (962 bytes, patch)
2016-06-10 16:08 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 13074: Use Koha::Cache to cache the defaults values of a MARC record (10.74 KB, patch)
2016-06-11 13:31 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 13074: (followup) remove useless diag (1019 bytes, patch)
2016-06-11 13:31 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2014-10-11 19:04:07 UTC
It currently uses a global variable:

our %default_values_for_mod_from_marc;
Comment 1 Jonathan Druart 2016-05-03 10:15:47 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2016-05-03 13:10:37 UTC Comment hidden (obsolete)
Comment 3 Bernardo Gonzalez Kriegel 2016-05-04 18:44:20 UTC
Hi,
found a fail in t/db_dependent/Items.t, subtest 4 of test 10

    not ok 4 - items.barcode is not mapped anymore, so the DB column has not been updated

    #   Failed test 'items.barcode is not mapped anymore, so the DB column has not been updated'
    #   at t/db_dependent/Items.t line 661.
    #          got: undef
    #     expected: 'a barcode'
    # Looks like you failed 1 test of 4.
not ok 10 - C4::Items::_build_default_values_for_mod_marc

Something in my data?
Does not fail in master.
Comment 4 Bernardo Gonzalez Kriegel 2016-05-04 18:48:18 UTC
(In reply to Bernardo Gonzalez Kriegel from comment #3)
> Hi,
> found a fail in t/db_dependent/Items.t, subtest 4 of test 10

What I found is that I need more sleep, forget dependency again. Sorry
Comment 5 Tomás Cohen Arazi 2016-05-09 14:14:48 UTC Comment hidden (obsolete)
Comment 6 Tomás Cohen Arazi 2016-05-09 14:14:57 UTC Comment hidden (obsolete)
Comment 7 Nick Clemens 2016-06-10 15:47:22 UTC
Small conflict in Items.pm

Also, did you mean to use  $another_barcode here:
660 	    $item = GetItem($item_itemnumber);
661 	    is ( $item->{barcode}, $a_barcode, 'items.barcode is not mapped anymore, so the DB column has not been updated' );
Comment 8 Jonathan Druart 2016-06-10 16:08:03 UTC Comment hidden (obsolete)
Comment 9 Jonathan Druart 2016-06-10 16:08:14 UTC Comment hidden (obsolete)
Comment 10 Jonathan Druart 2016-06-10 16:09:13 UTC
(In reply to Nick Clemens from comment #7)
> Small conflict in Items.pm
> 
> Also, did you mean to use  $another_barcode here:
> 660 	    $item = GetItem($item_itemnumber);
> 661 	    is ( $item->{barcode}, $a_barcode, 'items.barcode is not mapped
> anymore, so the DB column has not been updated' );

No I don't think so.
I don't remember exactly what I did, just remember that they were crazy tests to write...
Comment 11 Jonathan Druart 2016-06-10 16:11:35 UTC
(In reply to Jonathan Druart from comment #10)
> (In reply to Nick Clemens from comment #7)
> > Small conflict in Items.pm
> > 
> > Also, did you mean to use  $another_barcode here:
> > 660 	    $item = GetItem($item_itemnumber);
> > 661 	    is ( $item->{barcode}, $a_barcode, 'items.barcode is not mapped
> > anymore, so the DB column has not been updated' );
> 
> No I don't think so.
> I don't remember exactly what I did, just remember that they were crazy
> tests to write...

Ok so actually we do not want to update the DB column because the marc field is not mapped. Just what the test description says :)
Comment 12 Nick Clemens 2016-06-10 18:15:58 UTC
Yes, entirely, just read that backwards in my head.

Tests pass, but I have to reset plack to see the change happen when testing in client.  Will test again next week to see if it is me

(In reply to Jonathan Druart from comment #11)
> Ok so actually we do not want to update the DB column because the marc field
> is not mapped. Just what the test description says :)
Comment 13 Jonathan Druart 2016-06-11 13:17:11 UTC
(In reply to Nick Clemens from comment #12)
> Tests pass, but I have to reset plack to see the change happen when testing
> in client.  Will test again next week to see if it is me

Yes, confirmed.
This should be fixed by bug 16431.
Comment 14 Nick Clemens 2016-06-11 13:31:08 UTC
Created attachment 52254 [details] [review]
Bug 13074: Use Koha::Cache to cache the defaults values of a MARC record

With the global %default_values_for_mod_from_marc variable, the changes
made to the marc_subfield_structure table and especially the links
between MARC and DB fields are not safe and might be outdated (if a
field is linked/unlinked)

Test plan:
Under Plack:
- Link the barcode field, edit a record and set a barcode.
- Remove the mapping for the barcode field and then update again the
  barcode of the record.
The items.barcode DB field must not have been updated.

Without this patch, the field should have been updated.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Nick Clemens 2016-06-11 13:31:14 UTC
Created attachment 52255 [details] [review]
Bug 13074: (followup) remove useless diag

On running all Items-related tests some useless diag showed up. Removing it.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Kyle M Hall 2016-06-17 14:30:31 UTC
Pushed for 16.11, thanks Jonathan, Tomas!
Comment 17 Frédéric Demians 2016-06-24 05:44:03 UTC
Pushed in 16.05. Will be in 16.05.01.
Comment 18 Julian Maurice 2016-07-01 06:34:59 UTC
Patches pushed to 3.22.x, will be in 3.22.9