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