Bug 27545

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.

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

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>

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>

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.

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>

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>

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

Pushed to master for 21.05, thanks to everybody involved!

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

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?

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