Generally the permanent_location and location should match, however, prior to bug 23463 one could: 1 - Map permanent_location to a marc field 2 - Expose that field in the item editor 3 - Change the location of the item and retain the permanent location on saving 4 - This created a 'temporary' location that was cleared upon next checkin Many libraries used this for adding items to displays. Since bug 23463, however, the item updates only fields that have changed - if one changes the location but not the permanent location then koha sees that no permanent location was set and reverts it to the location We need to force passing through the permanent_location if it is visible+mapped in the item edit form, regardless of whether it changes or not
Can you have a look at bug 27545? I feel like the second patch will fix this.
Created attachment 117570 [details] [review] Bug 27837: Update permanent location if passed into ModItemFromMarc
Created attachment 117572 [details] [review] Bug 27837: Add unit tests
(In reply to Jonathan Druart from comment #1) > Can you have a look at bug 27545? I feel like the second patch will fix this. It does not. I ran some manual tests that failed, and also ran the unit tests from this patch set, which also failed.
Test plan: 1 - Map permanent_location to a marc field 2 - Expose that field in the item editor 3 - Edit an item, changing location to X and permanent location to Y, save 4 - confirm record shows location=x, permanent_location=y 5 - Edit item again, change location to Z, do not change permanent location, save 6 - confirm location and permanent_location both equal Z 7 - apply patch, restart 8 - repeat steps 3-5 9 - confirm location=Z, permanent location=Y 10 - prove t/db_dependent/Items.t
Tests fail if permanent_location is mapped to something in Koha to MARC Mapping. root@kohadevbox:koha(master)$ prove t/db_dependent/Items.t t/db_dependent/Items.t .. 11/14 # Failed test 'new permanent location set as expected' # at t/db_dependent/Items.t line 1057. # got: 'A' # expected: 'B' # Failed test 'permanent location remains unchanged as expected' # at t/db_dependent/Items.t line 1078. # got: 'A' # expected: 'B' # Looks like you failed 2 tests of 6. # Failed test 'permanent_location' # at t/db_dependent/Items.t line 1079. # Looks like you failed 1 test of 6. # Failed test 'ModItemFromMarc' # at t/db_dependent/Items.t line 1082. # Looks like you failed 1 test of 14. t/db_dependent/Items.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/14 subtests Test Summary Report ------------------- t/db_dependent/Items.t (Wstat: 256 Tests: 14 Failed: 1) Failed test: 14 Non-zero exit status: 1 Files=1, Tests=14, 5 wallclock secs ( 0.02 usr 0.00 sys + 3.11 cusr 0.47 csys = 3.60 CPU) Result: FAIL
Talked to Kyle, clarified expected behavior. Tests pass with patch applied but permanent_location not mapped in Koha to MARC Mapping. Once it's added to Koha to MARC Mapping, tests fail. This is expected behavior.
Created attachment 117625 [details] [review] Bug 27837: Update permanent location if passed into ModItemFromMarc Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 117626 [details] [review] Bug 27837: Add unit tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Created attachment 117627 [details] [review] Bug 27837: Update permanent location if passed into ModItemFromMarc Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov>
Created attachment 117628 [details] [review] Bug 27837: Add unit tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rhonda Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov>
Indeed, bug 27545 is only about item creation! The fix looks a bit weird but I didn't find some better, I've tried to fix at Koha::Item->store level but we definitely need to tell the column is dirty from ModItemFromMarc. However I think we should mock the situation where the tests are failing if the field is mapped.
Created attachment 117634 [details] [review] Bug 27837: Prevent the tests to fail is the field is mapped
Works as expected, tests and the QA script are happy.
Created attachment 117728 [details] [review] Bug 27837: Update permanent location if passed into ModItemFromMarc Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 117729 [details] [review] Bug 27837: Add unit tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 117730 [details] [review] Bug 27837: Prevent the tests to fail is the field is mapped Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 117735 [details] [review] Bug 27837: Permanent location to be empty string, check if defined
Created attachment 117738 [details] [review] Bug 27837: Permanent location to be empty string, check if key exists
Can you explain this follow-up, Kyle? What's the use case, how did you find it? We certainly need a couple of tests to cover it.
I've tested, QA script and unit tests pass. Also the following test plan checks out: Test plan: 1 - Map permanent_location to a marc field 2 - Expose that field in the item editor 3 - Edit an item, changing location to X and permanent location to Y, save 4 - confirm record shows location=x, permanent_location=y 5 - Edit item again, change location to Z, do not change permanent location, save 6 - confirm location and permanent_location both equal Z 7 - apply patch, restart 8 - repeat steps 3-5 9 - confirm location=Z, permanent location=Y 10 - prove t/db_dependent/Items.t
Created attachment 120361 [details] [review] Bug 27837: Update permanent location if passed into ModItemFromMarc Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 120362 [details] [review] Bug 27837: Add unit tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 120363 [details] [review] Bug 27837: Prevent the tests to fail is the field is mapped Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 120364 [details] [review] Bug 27837: Permanent location to be empty string, check if key exists Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
(In reply to Jonathan Druart from comment #20) > Can you explain this follow-up, Kyle? > What's the use case, how did you find it? > We certainly need a couple of tests to cover it. I read this as that we need 2 more unit tests that cover some more cases? Setting to failed QA to alert Kyle.
Created attachment 120562 [details] [review] Bug 27837: Update permanent location if passed into ModItemFromMarc Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 120563 [details] [review] Bug 27837: Add unit tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 120564 [details] [review] Bug 27837: Prevent the tests to fail is the field is mapped Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 120565 [details] [review] Bug 27837: Unit tests for blank permanent location
Created attachment 120566 [details] [review] Bug 27837: Permanent location to be empty string, check if key exists Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
(In reply to Katrin Fischer from comment #26) > (In reply to Jonathan Druart from comment #20) > > Can you explain this follow-up, Kyle? > > What's the use case, how did you find it? > > We certainly need a couple of tests to cover it. > > I read this as that we need 2 more unit tests that cover some more cases? > Setting to failed QA to alert Kyle. I added Unit tests, but neither the tests nor testing in the staff client pass - it seems the followup doesn't fix the issue. Can you look Kyle?
Created attachment 120616 [details] [review] Bug 27837: Check the MARC directly, not the transformed item If permanent location is mapped, it will exist in the MARC. However, when we are transforming our item hash to MARC, we are losing our permanent location mapping, at least in unit tests. By checking the original MARC for the permenent location, we remove the risk of losing it the "empty" permenent location through the transformation process. We only need to know that it existed in the original item marc to know we need to update it in the database.
Created attachment 120628 [details] [review] Bug 27837: Always pass permanent location from items editor if it was defined By always passing permenent location from the items editor, we can know if permanent location was defined in the framework. This means we can know if we should update permanent location independently of location even if permenent location has an empty value.
Created attachment 120629 [details] [review] Bug 27837: Always pass permanent location from items editor if it was defined By always passing permenent location from the items editor, we can know if permanent location was defined in the framework. This means we can know if we should update permanent location independently of location even if permenent location has an empty value.
I am a bit worried about the added warnings from running the tests with and without this patch set: With patch: kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Items.t t/db_dependent/Items.t .. 2/14 Use of uninitialized value in numeric eq (==) at t/db_dependent/Items.t line 247. Use of uninitialized value $tag in hash element at /usr/share/perl5/MARC/Record.pm line 202. Use of uninitialized value $tag in regexp compilation at /usr/share/perl5/MARC/Record.pm line 206. Use of uninitialized value $tag in hash element at /usr/share/perl5/MARC/Record.pm line 207. t/db_dependent/Items.t .. 12/14 Use of uninitialized value $tag in hash element at /usr/share/perl5/MARC/Record.pm line 202. Use of uninitialized value $tag in hash element at /usr/share/perl5/MARC/Record.pm line 202. Use of uninitialized value $tag in hash element at /usr/share/perl5/MARC/Record.pm line 202. Use of uninitialized value $tag in hash element at /usr/share/perl5/MARC/Record.pm line 202. t/db_dependent/Items.t .. ok All tests successful. Files=1, Tests=14, 11 wallclock secs ( 0.05 usr 0.01 sys + 7.52 cusr 1.11 csys = 8.69 CPU) Result: PASS Without patch: kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ git checkout master Switched to branch 'master' Your branch is up-to-date with 'origin/master'. kohadev-koha@kohadevbox:/home/vagrant/kohaclone$ prove t/db_dependent/Items.t t/db_dependent/Items.t .. 2/14 Use of uninitialized value in numeric eq (==) at t/db_dependent/Items.t line 247. t/db_dependent/Items.t .. ok All tests successful.
Created attachment 120832 [details] [review] Bug 27837: (follow-up) Do not check subfield unless tag defined This was causing extra warnings when the permanent_location field was unmapped. We only need to check if there is a field defined as you can define a mapping without both a field and subfield, so the existence of one implies the other
Created attachment 121044 [details] [review] Bug 27837: Update permanent location if passed into ModItemFromMarc Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 121045 [details] [review] Bug 27837: Add unit tests Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com> Signed-off-by: Rhonda Kuiper <rkuiper@roundrocktexas.gov> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 121046 [details] [review] Bug 27837: Prevent the tests to fail is the field is mapped Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 121047 [details] [review] Bug 27837: Unit tests for blank permanent location
Created attachment 121048 [details] [review] Bug 27837: Permanent location to be empty string, check if key exists Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 121049 [details] [review] Bug 27837: Check the MARC directly, not the transformed item If permanent location is mapped, it will exist in the MARC. However, when we are transforming our item hash to MARC, we are losing our permanent location mapping, at least in unit tests. By checking the original MARC for the permenent location, we remove the risk of losing it the "empty" permenent location through the transformation process. We only need to know that it existed in the original item marc to know we need to update it in the database. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 121050 [details] [review] Bug 27837: Always pass permanent location from items editor if it was defined By always passing permenent location from the items editor, we can know if permanent location was defined in the framework. This means we can know if we should update permanent location independently of location even if permenent location has an empty value. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 121051 [details] [review] Bug 27837: (follow-up) Do not check subfield unless tag defined This was causing extra warnings when the permanent_location field was unmapped. We only need to check if there is a field defined as you can define a mapping without both a field and subfield, so the existence of one implies the other Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 121052 [details] [review] Bug 27837: (QA follow-up) Minor Spelling/Typo correction Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Works as expected, nice use of TDD, tests all passing, QA script happy and no new warnings introduced now. Passing QA
Last patch will be squashed when pushed.
Pushed to master for 21.05, thanks to everybody involved!
Pushed to 20.11.x for 20.11.06
Pushed to 20.05.x for 20.05.12
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.