Bug 7817 - Items Permanent location (shelving location) is set to NULL when item is edited
Summary: Items Permanent location (shelving location) is set to NULL when item is edited
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: Main
Hardware: All All
: P1 - high critical (vote)
Assignee: Olli-Antti Kivilahti
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-03-23 16:00 UTC by Joy Nelson
Modified: 2019-06-27 09:24 UTC (History)
10 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 7817 - Items Permanent location (shelving location) is set to NULL when item is edited (985 bytes, patch)
2014-06-18 15:32 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 7817 - Items Permanent location (shelving location) is set to NULL when item is edited (1.27 KB, patch)
2014-06-19 03:32 UTC, David Cook
Details | Diff | Splinter Review
Bug 7817 [SIGNED OFF] - Items Permanent location (shelving location) is set to NULL when item is edited (1.27 KB, patch)
2014-06-19 03:32 UTC, David Cook
Details | Diff | Splinter Review
Bug 7817: A DB field without a default mapping is set to a default value on editing (6.84 KB, patch)
2014-06-20 11:44 UTC, Jonathan Druart
Details | Diff | Splinter Review
[SIGNED-OFF COUNTERPATCH] Bug 7817: A DB field without a default mapping is set to a default value on editing (7.08 KB, patch)
2014-07-30 16:09 UTC, Owen Leonard
Details | Diff | Splinter Review
[SIGNED_OFF ORIGINAL] Bug 7817 - Items Permanent location (shelving location) is set to NULL when item is edited (1.42 KB, patch)
2014-08-18 11:12 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7817: [QA FOLLOW-UP FOR ORIGINAL] QA Follow-up for original patch (2.13 KB, patch)
2014-08-18 11:29 UTC, Marcel de Rooy
Details | Diff | Splinter Review
[QA FOLLOW-UP FOR ORIGINAL] Bug 7817: Follow-up for original patch (2.09 KB, patch)
2014-08-25 10:47 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7817: QA Follow-up: Print a warning when permanent location is mapped (1.77 KB, patch)
2014-08-25 11:26 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 7817: (followup) add test on MARC bibliographic framework test (3.26 KB, patch)
2014-09-08 15:30 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Joy Nelson 2012-03-23 16:00:51 UTC
When an item is edited, the field items.permanent_location is set to NULL.  This causes significant problems when a library is utilizing the CART and PROC system preferences to assign CART and PROC shelving locations.  If items.permanent_location is NULL then there is no way to restore the original shelving location for these items. 

This results in significant data loss for libraries.
Comment 1 Chris Cormack 2012-04-20 00:14:39 UTC
I suspect this is because permanent_location is not mapped to a marc field.
Comment 2 Kyle M Hall 2012-05-16 16:36:57 UTC
Is the solution then to add a default mapping to Koha? The problem is that all the possible 952 subfields are being used. I tried mapping it to 960$l just to see what would happen, and items.permanent_location is still set to NULL when the item is edited.
Comment 3 Katrin Fischer 2012-05-16 16:46:24 UTC
Hi Kyle,

according to the MARC it's also possible to use upper case letters. We have used some of them with more_subfields_xml in migrations.

But I am not sure we should really treat this field lika a normal item field. Perhaps it should be tied to the functions it was added for?
So before Koha changes the location to a temporary location (CART or PROC) it could save the current location to the permanent_location field. Would that make sense?
Comment 4 Kyle M Hall 2012-05-16 17:49:13 UTC
I have found the problem:

Bug 6034, Shelving cart feature can wipe permanent shelving location in certain
circumstances, added "permanent_location => undef" at line 430 of Items.pm.
When permanent_locations is not mapped, it always uses null as the value from
this hash.

Solution: comment out line 430. I don't know if this will have any other
affects.

Potential problem: If a library decides to map permanent_location to a marc
field, they will be unable to set it to a null value.

Possibly better solution: build %default_values_for_mod_from_marc via a
subroutine that checks to see if each column is mapped to a marc field, and
deletes that key if it is not. That way, any unmapped fields will not be nulled
when an item is edited.
Comment 5 Fred P 2012-08-17 16:15:22 UTC
We implemented with the capital C for permanent location, editing the default MARC framework to add 952 C, and defining that like 952 c, referencing authorized value for LOC, and displaying on tab 10 (items(10)) so that it appears on the "Edit item" screen. 

Not sure if this breaks any of the CART functions.
Comment 6 Jorge de Cardenas 2012-08-20 13:46:09 UTC
(In reply to comment #3)
> Hi Kyle,
> 
> according to the MARC it's also possible to use upper case letters. We have
> used some of them with more_subfields_xml in migrations.
> 
> But I am not sure we should really treat this field lika a normal item
> field. Perhaps it should be tied to the functions it was added for?
> So before Koha changes the location to a temporary location (CART or PROC)
> it could save the current location to the permanent_location field. Would
> that make sense?

I would be interested to know the consequences of mapping to an upper case (952C) for Permanent Location and having it available on the items edit page. It appears that LibLime might be doing this. We want to implement a temporary location. Items are pulled for a display and the location is changed to reflect that temporary location. When that item is checked out and checked back in the Permanent Location is copied to the Location, automatically and the item goes back to the regular shelf. 

We have been doing this using the Lost status but that requires custom coding to allow holds on certain items with a Lost Status and displaying their Temporary Location. When these items are circulated they revert back to a normal Available status.
Comment 7 Olli-Antti Kivilahti 2014-06-18 15:32:49 UTC Comment hidden (obsolete)
Comment 8 Olli-Antti Kivilahti 2014-06-18 16:39:41 UTC
I would like to know why such a serious issue hasn't been fixed to master?
Clearly this problem has been identified years ago and somebody must have fixed it (because atleast the creator of  ReturnToShelvingCart uses this feature), but the fix never made it here???
Why?

I was able to fix the ~4500 missing permanent_locations from our installation specific identifier in the items.itemcallnumber. But others are not so lucky.
Comment 9 David Cook 2014-06-19 03:01:24 UTC
(In reply to Olli-Antti Kivilahti from comment #8)
> I would like to know why such a serious issue hasn't been fixed to master?
> Clearly this problem has been identified years ago and somebody must have
> fixed it (because atleast the creator of  ReturnToShelvingCart uses this
> feature), but the fix never made it here???
> Why?
> 
> I was able to fix the ~4500 missing permanent_locations from our
> installation specific identifier in the items.itemcallnumber. But others are
> not so lucky.

I imagine this issue hasn't been fixed, because it requires a specific configuration and someone to notice a problem.

I believe that for this issue to occur you would need the following config:

0) Set the Shelving Location for an item during 'Add item' (this will populate 'item.location' and 'item.permanent_location' with the LOC authorized value for the shelving location

AND 
-

1a) 'NewItemsDefaultLocation' set to 'PROC' and 'InProcessingToShelvingCart' set to 'ON'

OR 

1b) 'ReturnToShelvingCart' set to 'ON'

-
AND

2) Edit the item before checking it (since editing the item will clear 'item.permanent_location' due to a missing Koha => MARC mapping 

THEN

Run 'cart_to_shelf.pl'

--

At Prosentient, none of our libraries use any of these system preferences, so it hasn't been an issue. 

As for other libraries/vendors, I suspect that there could be any number of reasons for a fix not making it here. Either people don't use these preferences, they haven't noticed the problem, or they've created a custom solution that they don't think is appropriate for the community.

--

I haven't looked at the code (and I probably won't since it doesn't affect any of my libraries at present), but there's an explanation.
Comment 10 David Cook 2014-06-19 03:32:28 UTC Comment hidden (obsolete)
Comment 11 David Cook 2014-06-19 03:32:52 UTC Comment hidden (obsolete)
Comment 12 David Cook 2014-06-19 03:37:38 UTC
So much for not looking into this one, eh? ;)

By the way, I've tested this fix with the "cart_to_shelf.pl" cronjob, and it works perfectly. 

The "AddItem" and "ModItem" scripts now both properly populate "permanent_location" from "location" (whereas only "AddItem" did it properly before).

CartToShelf() doesn't run into the same problem as the Item Editor because it's using the row from the items table rather than HTML form input, so permanent_location will already be set correctly when running "ModItem" and thus not overwritten by "Cart". Same goes for ShelfToCart().

Voila.
Comment 13 Jonathan Druart 2014-06-19 12:51:08 UTC
I never seen this bug, but I think the Kyle's suggestion is better:

> Possibly better solution: build %default_values_for_mod_from_marc via a
> subroutine that checks to see if each column is mapped to a marc field, and
> deletes that key if it is not. That way, any unmapped fields will not be
> nulled
> when an item is edited.

Marked as Failed QA.

I will try to provide a patch soon.
Comment 14 David Cook 2014-06-19 23:37:38 UTC
(In reply to Jonathan Druart from comment #13)
> I never seen this bug, but I think the Kyle's suggestion is better:
> 
> > Possibly better solution: build %default_values_for_mod_from_marc via a
> > subroutine that checks to see if each column is mapped to a marc field, and
> > deletes that key if it is not. That way, any unmapped fields will not be
> > nulled
> > when an item is edited.
> 
> Marked as Failed QA.
> 
> I will try to provide a patch soon.

Jonathan, why not pass this one as QA first so that the broken behaviour can be stopped sooner rather than later as per Olli's request?

As for building %default_values_for_mod_from_marc for only mapped columns, I agree that is a better solution. I would gladly sign off that patch as well :).
Comment 15 Jonathan Druart 2014-06-20 10:36:33 UTC
(In reply to David Cook from comment #14)
> Jonathan, why not pass this one as QA first so that the broken behaviour can
> be stopped sooner rather than later as per Olli's request?

It seems the patch is not correct.
Kyle explained the problem in his comment (comment 4):
"Potential problem: If a library decides to map permanent_location to a marc
field, they will be unable to set it to a null value."
Comment 16 Jonathan Druart 2014-06-20 11:44:27 UTC Comment hidden (obsolete)
Comment 17 Jonathan Druart 2014-06-20 11:46:10 UTC
I am not really confident in this patch, so please test it deeply!

It would be great to add some unit tests for the C4::Items::ModItemFromMarc routine...
Comment 18 Jonathan Druart 2014-07-25 13:39:05 UTC
You can obsolete my patch if nobody wants it.
Comment 19 Owen Leonard 2014-07-30 16:09:45 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2014-08-07 13:18:25 UTC
I will be looking at both patches soon (the version of Olli and Jonathan) in connection to QA if not passed by someone else :)
Comment 21 Marcel de Rooy 2014-08-18 11:12:41 UTC
Created attachment 30923 [details] [review]
[SIGNED_OFF ORIGINAL] Bug 7817 - Items Permanent location (shelving location) is set to NULL when item is edited

A quick fix to prevent more damages.
No perceived side-effects so far.

Signed-off-by: David Cook <dcook@prosentient.com.au>

This is actually a perfectly good fix for this issue. I've changed
the explanatory comment to explain why.

Another option would be to remove the 'exists' check in the sub
_do_column_fixes_for_mod(), but this is just as functional.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Detailed comment on Bugzilla.
Adding a small follow-up.
Comment 22 Marcel de Rooy 2014-08-18 11:29:26 UTC Comment hidden (obsolete)
Comment 23 Marcel de Rooy 2014-08-18 11:36:03 UTC
QA Comment:

I looked at the first signed patch and also the signed counterpatch.
I have a strong preference for the original patch. I think that the reason for failing QA on that patch was not really valid. If failed because "If a library decides to map permanent_location to a marc field, they will be unable to set it to a null value." A library should never decide that.

Someone who maps permanent_location to a marc field, is doing something very weird! This item field is really a Koha internal for e.g. the CART functions. Nobody should use it for another reason (same for a few other item fields). We could hide such fields from using them in admin/koha2marclinks.pl and admin/marc_subfields_structure.pl (new report?)

I think that one of the reasons for this problem is the historically grown 'hybrid' approach of items in Koha. We offer a MARC structure to edit them, but we do not save them in MARC but just in item fields. When saving an edited item, we call ModItemFromMarc with its default_values_for_mod_from_marc hash. Jonathan rebuilds this hash, but imo we should more focus on this subroutine. Let the subroutine only map the 952 fields to item fields, instead of doing what it does now (new marc record, transformMarcToKoha, get_unlinked_fields, etc.) This is too complicated. The second patch does not  simplify the situation (not saying that it is theoretically correct btw).

The first patch removes a non-marc item field from the default_values_for_mod_from_marc hash. This is a correct action; it should not be there. Other non-marc item fields should not be there either. Like paidfor (see follow-up).

The counterpatch is a more drastic approach. It is not simple as mentioned. It does a lot more so it will add at least some to performance. And since it touches a lot more, it should be tested a lot more. I would favor pushing the first patch and refactor ModItemFromMarc with the counterpatch as a base.

I will add a followup for the first patch and unhide the first patch, leaving the counterpatch as-is and set this report to In Discussion for one or two weeks, allowing all involved to give their opinion too.
Comment 24 Marcel de Rooy 2014-08-18 11:57:22 UTC
First point for the discussion (with thanks to cait):

Should we add a dbrev (here) for installations that mapped internal fields to their MARC structure in Koha? Or at least check permanent_location and paidfor under this report?
Comment 25 Katrin Fischer 2014-08-24 19:28:07 UTC
I think we need to make sure to cause no data loss for libraries that have changed the mappings. Maybe we should concentrate on the permanent location first with this patch and then concentrate on the paidfor in another patch series.

Because... this paidfor is a bit of a mess:

I researched a bit and until Fri, 18 Jul 2008:
http://git.koha-community.org/gitweb/?p=koha.git;a=commit;h=bc408475f8afb0df633908b2bc537d6f4ecb90ba
we delivered the standard frameworks with the nonpublic note mapped to paidfor. As there was never an update to change this, this would still be the case for older installations. I know the mapping exists in some of our installations that date back to 3.2.

Also, it looks like the mapping still exists in the _current_ MARC21 FA frameworks (de-DE, en, es-ES, it-IT, nb-NO) and some UNIMARC frameworks. So the mappings are inconsistent right now.

Bug 4222 tried to introduce a new database column for the nonpublic note, but sadly it's currently stuck in Failed QA because of some remaining problems with the last patch.
Comment 26 Marcel de Rooy 2014-08-25 07:16:27 UTC
(In reply to Katrin Fischer from comment #25)
> I think we need to make sure to cause no data loss for libraries that have
> changed the mappings. Maybe we should concentrate on the permanent location
> first with this patch and then concentrate on the paidfor in another patch
> series.
> 

Thanks for looking at paidfor.
I will open up a new report for that one and remove it from the follow-up.
Comment 27 Marcel de Rooy 2014-08-25 10:47:43 UTC
Created attachment 31133 [details] [review]
[QA FOLLOW-UP FOR ORIGINAL] Bug 7817: Follow-up for original patch

This patch removes the commented line for permanent_location.
It adds a more general comment.
Adjusts the exists check on permanent_location.
Adds a reference to bug 12817 that will deal with paidfor similarly.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 28 Marcel de Rooy 2014-08-25 11:06:49 UTC
I will open another new report for a dbrev that moves a mapped permanent_location (which should NOT be mapped!!) to the more_subfields_xml item field. That should be tested on its own.
What I propose here, is add a dbrev warning if a library did map it.
Comment 29 Marcel de Rooy 2014-08-25 11:26:50 UTC
Created attachment 31134 [details] [review]
Bug 7817: QA Follow-up: Print a warning when permanent location is mapped

Item field permanent location should not be mapped to the MARC structure.
If a library did so, this dbrev will print a warning.
If refers to report 12818 for help in moving the data to more_subfields_xml.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Tested by putting permanent_location in and out of the MARC structure.
Comment 30 Marcel de Rooy 2014-08-25 12:20:08 UTC
Will leave this problem in discussion for another week to allow for further feedback.
Comment 31 Marcel de Rooy 2014-09-01 07:24:36 UTC
Moving the original patch now to Passed QA. (See comment23.)
Comment 32 Jonathan Druart 2014-09-01 08:02:58 UTC
(In reply to M. de Rooy from comment #31)
> Moving the original patch now to Passed QA. (See comment23.)

What about the more generic way? Should we open a new bug report not to loose the patch (I understood that the only problem you found is the paidfor field)?
Comment 33 Marcel de Rooy 2014-09-01 08:33:20 UTC
(In reply to Jonathan Druart from comment #32)
> (In reply to M. de Rooy from comment #31)
> > Moving the original patch now to Passed QA. (See comment23.)
> 
> What about the more generic way? Should we open a new bug report not to
> loose the patch (I understood that the only problem you found is the paidfor
> field)?

Yes, please move it to a new report.
Your patch could be the framework for refactoring ModItemFromMarc.
Comment 34 Jonathan Druart 2014-09-04 14:48:28 UTC
(In reply to M. de Rooy from comment #33)
> Yes, please move it to a new report.

Done, see bug 12874.
Comment 35 Tomás Cohen Arazi 2014-09-08 15:30:00 UTC
Created attachment 31464 [details] [review]
Bug 7817: (followup) add test on MARC bibliographic framework test

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>
Comment 36 Tomás Cohen Arazi 2014-09-08 15:41:34 UTC
Patches pushed to master.

Thanks Olli and Marcel.