Bug 28895

Summary: Don't fill in items.location with NewItemsDefaultLocation if blank
Product: Koha Reporter: Jonathan Druart <jonathan.druart+koha>
Component: CatalogingAssignee: Bugs List <koha-bugs>
Status: ASSIGNED --- QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: andrewfh, hannah.co, m.de.rooy, nick, oleonard
Version: unspecified   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=3481
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29308
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 27545    
Bug Blocks:    
Attachments: Bug 28895: Don't use NewItemsDefaultLocation for items edition

Description Jonathan Druart 2021-08-25 10:26:00 UTC
On bug 27545 we extended the use of NewItemsDefaultLocation to affect item creation in different place, not only the add item form from the cataloguing module.

There is at least one unexpected behaviour (see bug 27545 comment 11): if an item is edited and has a blank location, the syspref's value will be used.

We could fix the item edition but it will lead to another inconsistency: if we don't set a location when an item is created, we are enforcing the location to be set to the syspref's value anyway.

What's the exact expected behaviour?
Comment 1 Jonathan Druart 2021-08-25 10:28:00 UTC
Created attachment 124083 [details] [review]
Bug 28895: Don't use NewItemsDefaultLocation for items edition
Comment 2 Jonathan Druart 2021-08-25 10:28:42 UTC
This patch is adjusting the edition behaviour, not the creation. Should we allow empty location if the pref is set?
Comment 3 Katrin Fischer 2021-08-25 12:48:30 UTC
When a location is explicitly set to blank, editing an item should leave it blank. I think that's the easy one.

I don't think that's an inconsistency actually, as the pref should only apply to new items in the first place. So we could fix this independently of the item creation use case.

If it's currently possible to set another location than the pref location, I think it would be good to also be able to set an empty one explicitly.
Comment 4 Andrew Fuerste-Henry 2021-09-17 16:43:32 UTC
Agreed, I would like to be able to set a blank shelving location on a new item, have it go to the location set in NewItemsDefaultLocation, then go to a blank shelving location on a later checkin.
Comment 5 Andrew Fuerste-Henry 2021-09-17 16:53:40 UTC
Actually, testing this on master this morning it's very much not working as intended.

I have NewItemsDefaultLocation set to PROC.
On additem.pl, when I create a load the new item page, location is automatically set to PROC. If I create my item without changing it, the item has both location and shelving_location set to PROC.
There is no reason to set the default to PROC here, but I guess it doesn't hurt anything.

On additem.pl, if I change the shelving location to any value other than PROC before I create my item, both the location and permanent location are set to whatever new value I've selected.
Whatever I've selected should go into permanent_location, but location should be PROC.

If I delete PROC without selecting something else on creating my item, then location and permanent_location end up as PROC anyway.
This bug asserts that in this case I should get location=PROC and permanent_location=NULL. I support that change.

This could debatably be a new bug, but there are a lot of old bugs around this syspref already.
Comment 6 Hannah Co 2021-09-17 17:20:51 UTC
(In reply to Andrew Fuerste-Henry from comment #5)
> Actually, testing this on master this morning it's very much not working as
> intended.
> 
> I have NewItemsDefaultLocation set to PROC.
> On additem.pl, when I create a load the new item page, location is
> automatically set to PROC. If I create my item without changing it, the item
> has both location and shelving_location set to PROC.
> There is no reason to set the default to PROC here, but I guess it doesn't
> hurt anything.
> 
> On additem.pl, if I change the shelving location to any value other than
> PROC before I create my item, both the location and permanent location are
> set to whatever new value I've selected.
> Whatever I've selected should go into permanent_location, but location
> should be PROC.
> 
> If I delete PROC without selecting something else on creating my item, then
> location and permanent_location end up as PROC anyway.
> This bug asserts that in this case I should get location=PROC and
> permanent_location=NULL. I support that change.
> 
> This could debatably be a new bug, but there are a lot of old bugs around
> this syspref already.

In order for UpdateItemLocationOnCheckin to work correctly for items with no shelving location, the NewItemsDefaultLocation needs to stay out of the permanent_location field. If NewItemsDefaultLocation is set to PROC, and UpdateItemLocationOnCheckin has the line "PROC: _PERM_" items with no shelving location remain in processing and don't get shelving location changed to NULL on checkin, as they should. 

I agree that at item creation, if a shelving location is not selected then the item should have location=PROC and permanent_location=NULL.

When editing an item, the NewItemsDefaultLocation currently replaces a NULL value. If the shelving location is NULL, NewItemsDefaultLocation should not replace the NULL upon editing.
Comment 7 Andrew Fuerste-Henry 2021-10-22 14:46:30 UTC
I've filed a separate bug (Bug 29308) for the fact that NewItemsDefaultLocation is going into permanent_location. That's absolutely a regression that breaks existing workflows. It's also a bit distinct from the question here of whether or not a selection of NULL location should mean NULL permanent_location when using NewItemsDefaultLocation.

BUT in testing bug 29308, I confirmed that in 20.05 if one is using NewItemsDefaultLocation but leaves location empty at item creation, one's item is created with the NewItemsDefaultLocation value in items.location but NULL in items.permanent_location.

So this is a change in behavior and the consensus here seems to be we should change it back. Moving out of In Discussion.