Bug 7817

I suspect this is because permanent_location is not mapped to a marc field.

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.

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 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.

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.

(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.

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.

(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.

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.

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.

(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 :).

(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."

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...

You can obsolete my patch if nobody wants it.

I will be looking at both patches soon (the version of Olli and Jonathan) in connection to QA if not passed by someone else :)

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.

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.

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?

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.

(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.

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>

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.

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.

Will leave this problem in discussion for another week to allow for further feedback.

Moving the original patch now to Passed QA. (See comment23.)

(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)?

(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.

(In reply to M. de Rooy from comment #33)
> Yes, please move it to a new report.

Done, see bug 12874.

Created attachment 31464 [details] [review]
Bug 7817: (followup) add test on MARC bibliographic framework test

Signed-off-by: Tomas Cohen Arazi <tomascohen@gmail.com>

Patches pushed to master.

Thanks Olli and Marcel.