Bug 14662 - Allow blank values in pull downs in cataloguing forms when subfield is mandatory
Summary: Allow blank values in pull downs in cataloguing forms when subfield is mandatory
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low major with 1 vote (vote)
Assignee: Katrin Fischer
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks: 21148
  Show dependency treegraph
 
Reported: 2015-08-07 13:32 UTC by Katrin Fischer
Modified: 2018-09-21 10:07 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 14662: Add empty entries to pull downs on item form for mandatory subfields (3.35 KB, patch)
2018-08-02 00:01 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14662: Add empty entries to pull downs on item form for mandatory subfields (3.40 KB, patch)
2018-08-02 13:53 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 14662: Add empty entries to pull downs in cataloguing form for mandatory subfields (2.76 KB, patch)
2018-08-02 14:00 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 14662: Add empty entries to pull downs in cataloguing form for mandatory subfields (2.80 KB, patch)
2018-08-02 14:08 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 14662: Add empty entries to pull downs on item form for mandatory subfields (3.58 KB, patch)
2018-08-06 14:35 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14662: Add empty entries to pull downs in cataloguing form for mandatory subfields (2.88 KB, patch)
2018-08-06 14:36 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2015-08-07 13:32:47 UTC
If you mark a subfield as mandatory and it's linked to an authorised value the first value will be preselected and there is no empty entry in the list. This means the mandatory warning can never be triggered and a library might not notice that a wrong value has been selected.

I think the behaviour should be that for a new item the field is empty at the beginning, so it will warn if you try to save without selecting a value.
Comment 1 Katrin Fischer 2016-12-27 12:09:00 UTC
We keep running into this. As it is now, 'mandatory' makes no sense for fields with a pull down list as it will never trigger the message and it's easier to spot items where you forgot to set the item type than items where the first entry of the list was saved accidentally.
Comment 2 Katrin Fischer 2017-06-05 10:48:27 UTC
This causes quite a bit of trouble - upping severity a bit.

Example: Item type
Koha is unhappy if no item type is set for an item, so it makes a lot of sense to set this field to mandatory.
Setting it to mandatory, will result in the first item of the list to be preselected and often left unchanged.
There is no way to find those, that have been incorrectly set.
We have a "default" option in the frameworks, this should be used when you want to preselect, mandatory should just enforce that the value is set before saving.
Comment 3 Hugh Rundle 2018-02-20 23:18:41 UTC
This is a gigantic pain and as Katrin mentions, difficult to identify after the fact.

Just to clarify how I would expect this to work, Katrin's combined descriptions are correct:

* If there is a default value: dropdown should be preselected to the default
* If no default value: dropdown should be preselected to blank
* If field is compulsory and no default value: Koha should show an error complaining that there is no value assigned to the field in question.

Currently making something mandatory preselects the first value in the dropdown and doesn't give an error or warning. This is very rarely what users would want and never what administrators would expect.
Comment 4 Katrin Fischer 2018-02-21 06:35:25 UTC
Hi Hugh, thx for your comment - I was starting to think it was only me (note the dates on the comments!) :)
Comment 5 Katrin Fischer 2018-07-25 11:44:38 UTC
Summarizing with a test plan and description of expected behaviour:

To test:
- Mark collection, itemtype and damaged as mandatory in your MARC framework 
- Add a new item
- Verify that instead of the fields being empty, the first value is selected
  If there is an itemtype on bilbio level, this itemtype should be selected.

Expected behaviour:
- All pull downs should offer an empty entry
- Empty should be preselected, if there is no
  - existing value
  - default in the framework
  - Only for itemtypes: value in 942$c
- If one of those is set, this value should be preselected.
Comment 6 Katrin Fischer 2018-08-02 00:01:18 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2018-08-02 00:02:03 UTC
Having a go at this :)
Comment 8 Owen Leonard 2018-08-02 13:53:03 UTC
Created attachment 77443 [details] [review]
Bug 14662: Add empty entries to pull downs on item form for mandatory subfields

The code assumed that if a subfield is marked as mandatory, there should be no
empty entry in the pull downs.

This assumption is not correct, as it leads to the first entry of the
pull down being preselected if there is no default set. Which means you
will never be alerted of any cataloguing errors and errors will be very
hard to find later on.

Correct behaviour would be to preselect the empty value when there is
no default. This means on saving the item an error message is triggered
and the cataloger is forced to set the value.

To test:
- Adapt your frameworks:
  - Make 942$c non-mandatory
  - In 952 make itemtype, classification source and some other pull downs
    like location or collection mandatory
- Add a new item
  - Verify that the first value of each pull down is preselected,
    there is no way to trigger the 'required' error
- Apply patch
  - Add a new item
    - Verify that classification source is preselected according to the
      DefaultClassificationSource system preference
    - Verify that the itemtype is preselected according to 942$c in
      the bibliographic record
    - Verify all mandatory fields can be set to empty
    - Verify that you can't save before correctly setting them
  - Change the 942$c in the record to empty
  - Add another item
    - Verify the itemtype is now empty
  - Change your frameworks and set a default for itemtype (Ex: BK)
  - Repeat default check with another pull down like collection or location

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 9 Katrin Fischer 2018-08-02 14:00:55 UTC Comment hidden (obsolete)
Comment 10 Owen Leonard 2018-08-02 14:08:17 UTC
Created attachment 77445 [details] [review]
Bug 14662: Add empty entries to pull downs in cataloguing form for mandatory subfields

First patch deals with the problem on the item form, this
patch fixes the same problem on catalouging form for the
bibliographic record.

To test:
- Adapt your frameworks:
  - Make sure 942$c is mandatory
  - Link another field to an authorised value, for example: 942$n to YESNO
- Add a new record
  - Verify that the first value of each pull down is preselected,
    there is no way to trigger the 'required' error
- Apply patch
  - Add a record
    - Verify that classification source is preselected according to the
      DefaultClassificationSource system preference
    - Verify that 942$c now defaults to empty
    - Verify all your other mandatory subfields can be set to empty
    - Verify that you can't save before correctly setting them
  - Chane your frameworks
    - Add a default value for 942$c, for example: CF
  - Add another record
    - Verify the itemtype is now preset to your default itemtype
  - Edit an existing record, verify all values show correctly

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 11 Jonathan Druart 2018-08-06 13:49:27 UTC
It does not work for me when adding a new item.
Comment 12 Jonathan Druart 2018-08-06 14:10:54 UTC
(In reply to Jonathan Druart from comment #11)
> It does not work for me when adding a new item.

Hum wait, let me follow the test plan without skipping lines :)
Comment 13 Jonathan Druart 2018-08-06 14:35:57 UTC
Created attachment 77490 [details] [review]
Bug 14662: Add empty entries to pull downs on item form for mandatory subfields

The code assumed that if a subfield is marked as mandatory, there should be no
empty entry in the pull downs.

This assumption is not correct, as it leads to the first entry of the
pull down being preselected if there is no default set. Which means you
will never be alerted of any cataloguing errors and errors will be very
hard to find later on.

Correct behaviour would be to preselect the empty value when there is
no default. This means on saving the item an error message is triggered
and the cataloger is forced to set the value.

To test:
- Adapt your frameworks:
  - Make 942$c non-mandatory
  - In 952 make itemtype, classification source and some other pull downs
    like location or collection mandatory
- Add a new item
  - Verify that the first value of each pull down is preselected,
    there is no way to trigger the 'required' error
- Apply patch
  - Add a new item
    - Verify that classification source is preselected according to the
      DefaultClassificationSource system preference
    - Verify that the itemtype is preselected according to 942$c in
      the bibliographic record
    - Verify all mandatory fields can be set to empty
    - Verify that you can't save before correctly setting them
  - Change the 942$c in the record to empty
  - Add another item
    - Verify the itemtype is now empty
  - Change your frameworks and set a default for itemtype (Ex: BK)
  - Repeat default check with another pull down like collection or location

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
We do not want empty values for branches (holdingbranch and homebranch
must be mandatory, see bug 21011)
Comment 14 Jonathan Druart 2018-08-06 14:36:01 UTC
Created attachment 77491 [details] [review]
Bug 14662: Add empty entries to pull downs in cataloguing form for mandatory subfields

First patch deals with the problem on the item form, this
patch fixes the same problem on catalouging form for the
bibliographic record.

To test:
- Adapt your frameworks:
  - Make sure 942$c is mandatory
  - Link another field to an authorised value, for example: 942$n to YESNO
- Add a new record
  - Verify that the first value of each pull down is preselected,
    there is no way to trigger the 'required' error
- Apply patch
  - Add a record
    - Verify that classification source is preselected according to the
      DefaultClassificationSource system preference
    - Verify that 942$c now defaults to empty
    - Verify all your other mandatory subfields can be set to empty
    - Verify that you can't save before correctly setting them
  - Chane your frameworks
    - Add a default value for 942$c, for example: CF
  - Add another record
    - Verify the itemtype is now preset to your default itemtype
  - Edit an existing record, verify all values show correctly

Signed-off-by: Owen Leonard <oleonard@myacpl.org>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 15 Nick Clemens 2018-08-08 21:32:18 UTC
Awesome work all!

Pushed to master for 18.11
Comment 16 Martin Renvoize 2018-08-09 10:06:40 UTC
Pushed to 18.05.x for 18.05.03
Comment 17 Fridolin SOMERS 2018-08-28 13:08:03 UTC
Pushed to 17.11.x for 17.11.09