Bug 27545 - NewItemsDefaultLocation is only used from additem.pl
Summary: NewItemsDefaultLocation is only used from additem.pl
Status: Pushed to master
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Keywords:
Depends on: 3481
Blocks: 27526 28895
  Show dependency treegraph
 
Reported: 2021-01-25 14:14 UTC by Jonathan Druart
Modified: 2021-08-25 10:26 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
The NewItemsDefaultLocation syste prefernce setting is now also used when adding items in the serials or acquisition module or when importing items through the staged MARC import tools.
Version(s) released in:
21.05.00


Attachments
Bug 27545: Use NewItemsDefaultLocation from places where an item is created (8.19 KB, patch)
2021-01-25 14:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27545: Keep the location if passed (9.08 KB, patch)
2021-01-25 14:21 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 27545: Use NewItemsDefaultLocation from places where an item is created (8.25 KB, patch)
2021-03-03 15:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27545: Keep the location if passed (9.14 KB, patch)
2021-03-03 15:23 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 27545: Use NewItemsDefaultLocation from places where an item is created (8.30 KB, patch)
2021-03-05 10:58 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27545: Keep the location if passed (9.20 KB, patch)
2021-03-05 10:58 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2021-01-25 14:14:54 UTC
The syspref NewItemsDefaultLocation is used to set a default value for item's location.

But it seems that there are some weirdness in the behaviour:
1. It's only used from additem. It seems that it should be used from acq and serial modules as well. And maybe for the items import too.
2. It set the location even if another one has been picked from the UI
=> We UI must preselect the syspref's value, but the controller must pick what has been selected on the UI
Comment 1 Jonathan Druart 2021-01-25 14:21:40 UTC
Created attachment 115775 [details] [review]
Bug 27545: Use NewItemsDefaultLocation from places where an item is created

The syspref NewItemsDefaultLocation is used to set a default value for item's location.

But it seems that there are some weirdness in the behaviour:
1. It's only used from additem. It seems that it should be used from acq and serial modules as well. And maybe for the items import too.
2. It set the location even if another one has been picked from the UI
=> We UI must preselect the syspref's value, but the controller must pick what has been selected on the UI

This patch is adding the default to the UI and extend the use of the
pref to other areas.

Test plan:
Set a value to NewItemsDefaultLocation
Catalogue a new item and confirm that the syspref's value is picked to
selected the default value on the add item form
Same behaviour should apply to the acquisition and serial modules
When importing items, the default location must be used if the imported
items did not have a location defined.
Comment 2 Jonathan Druart 2021-01-25 14:21:43 UTC
Created attachment 115776 [details] [review]
Bug 27545: Keep the location if passed

Change in behaviour here, but expect. We must keep the location value if
passed to store.
Same for the permanent_location
Comment 3 Tomás Cohen Arazi 2021-03-03 15:23:11 UTC
Created attachment 117615 [details] [review]
Bug 27545: Use NewItemsDefaultLocation from places where an item is created

The syspref NewItemsDefaultLocation is used to set a default value for item's location.

But it seems that there are some weirdness in the behaviour:
1. It's only used from additem. It seems that it should be used from acq and serial modules as well. And maybe for the items import too.
2. It set the location even if another one has been picked from the UI
=> We UI must preselect the syspref's value, but the controller must pick what has been selected on the UI

This patch is adding the default to the UI and extend the use of the
pref to other areas.

Test plan:
Set a value to NewItemsDefaultLocation
Catalogue a new item and confirm that the syspref's value is picked to
selected the default value on the add item form
Same behaviour should apply to the acquisition and serial modules
When importing items, the default location must be used if the imported
items did not have a location defined.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 Tomás Cohen Arazi 2021-03-03 15:23:15 UTC
Created attachment 117616 [details] [review]
Bug 27545: Keep the location if passed

Change in behaviour here, but expect. We must keep the location value if
passed to store.
Same for the permanent_location

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 5 Tomás Cohen Arazi 2021-03-03 15:28:06 UTC
Tested this patches and they work as expected:
- Tests pass
- Adding/editing an item has the expected behavior
- Creating items from Acquisitions (on placing the order) also follows the default
- QA script is happy

Note about serials: when it comes to serials, the new subscription form doesn't default to the syspref. I believe this is the correct behavior so we don't change people's workflow. The location dropdown works as a local-to-the-subscription default for items, which is respected in the following steps, the edge case would be setting it to 'None' and checking the item is not defaulting to the NewItemsDefaultLocation value.

TL;DR

Works as expected, if someone really needs the serial subscription form default the Location option to some value, it should be discussed on an enhancement bug.
Comment 6 Martin Renvoize 2021-03-05 10:58:41 UTC
Created attachment 117804 [details] [review]
Bug 27545: Use NewItemsDefaultLocation from places where an item is created

The syspref NewItemsDefaultLocation is used to set a default value for item's location.

But it seems that there are some weirdness in the behaviour:
1. It's only used from additem. It seems that it should be used from acq and serial modules as well. And maybe for the items import too.
2. It set the location even if another one has been picked from the UI
=> We UI must preselect the syspref's value, but the controller must pick what has been selected on the UI

This patch is adding the default to the UI and extend the use of the
pref to other areas.

Test plan:
Set a value to NewItemsDefaultLocation
Catalogue a new item and confirm that the syspref's value is picked to
selected the default value on the add item form
Same behaviour should apply to the acquisition and serial modules
When importing items, the default location must be used if the imported
items did not have a location defined.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize 2021-03-05 10:58:45 UTC
Created attachment 117805 [details] [review]
Bug 27545: Keep the location if passed

Change in behaviour here, but expect. We must keep the location value if
passed to store.
Same for the permanent_location

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize 2021-03-05 10:59:54 UTC
Weird that no-one appears to have noticed this.. I would say it's a bug as much as an enhancement.

Anywho, it works well now and takes a 'least surprise' approach.

Works well, Includes tests, Passes tests and QA script.

Passing QA
Comment 9 Jonathan Druart 2021-03-09 16:19:57 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 10 Fridolin Somers 2021-03-19 13:36:31 UTC
Even if it is sort of a bug, I prefer not backport this change of behavior to stable 20.11.x.

Enhancement not pushed to 20.11.x
Comment 11 Hannah Co 2021-08-19 21:10:55 UTC
Edit Item adds the NewItemsDefaultLocation, if the existing item has a blank shelving location. Our library expects that the Edit Item function would preserve the blank shelving location, while Add Item would fill in NewItemsDefaultLocation. 

Is filling in NewItemsDefaultLocation expected on item edit, or does the editing behavior need to be reviewed?
Comment 12 Jonathan Druart 2021-08-25 10:26:26 UTC
(In reply to Hannah Co from comment #11)
> Edit Item adds the NewItemsDefaultLocation, if the existing item has a blank
> shelving location. Our library expects that the Edit Item function would
> preserve the blank shelving location, while Add Item would fill in
> NewItemsDefaultLocation. 
> 
> Is filling in NewItemsDefaultLocation expected on item edit, or does the
> editing behavior need to be reviewed?

I've opened bug 28895 for discussion.