Bug 21927 - Acq: Allow blank values in pull downs in the item form when subfield is mandatory
Summary: Acq: Allow blank values in pull downs in the item form when subfield is manda...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Katrin Fischer
QA Contact: Testopia
URL:
Keywords:
Depends on: 14662
Blocks:
  Show dependency treegraph
 
Reported: 2018-11-30 11:54 UTC by Katrin Fischer
Modified: 2020-05-08 20:57 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06


Attachments
Bug 21927: Acq - Add blank values in pull downs of mandatory item subfields (4.14 KB, patch)
2020-04-14 06:47 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 21927: Acq - Add blank values in pull downs of mandatory item subfields (4.18 KB, patch)
2020-04-24 08:18 UTC, David Nind
Details | Diff | Splinter Review
Bug 21927: Acq - Add blank values in pull downs of mandatory item subfields (4.26 KB, patch)
2020-04-24 13:13 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 2018-11-30 11:54:07 UTC
This has already been fixed for the item form in cataloguing (bug 14662), but sadly we missed that there is an item form in acq too!

To test:
- Set AcQCreateItem to "on order"
- Create a new order line
- Check that item type shows and has an empty entry that is preselected
- Change 952y in the ACQ framework to be mandatory
- Verify the empty entry is gone and the first value is preselected (bug)
Comment 1 Katrin Fischer 2019-04-26 10:28:00 UTC
After some digging in the code I believe we need the same or similar changes as bug 14662 for C4::Items::PrepareItemrecordDisplay.
Comment 2 Katrin Fischer 2020-04-14 06:47:05 UTC
Created attachment 102892 [details] [review]
Bug 21927: Acq - Add blank values in pull downs of mandatory item subfields

This is the same fix as on bug 14662, which fixed the behaviour in
cataloguing, but for the item form in acquisitions.

The code assumes 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. As the field
can never be 'unset', there will never be a 'required' warning.
Furthermore, it might be counterproductive to use mandatory fields,
as it might be easily forgotten to change the preselected value and
those mistakes will be hard to find.

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:
- This is best tested with an ACQ framework, but default can be used
  when no ACQ framework was created.
- In your MARC bibliographic framework:
  - In 952 make itemtype, classification source and some other pull downs
    like location or collection mandatory and set them to visibel if needed
- Create a new basket with 'items created while ordering'
- Add a new order, an existing record with 942$c set will work best
- Add items for your order line
  - 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 order line
    - Verify that classification source is preselected according to the
      DefaultClassificationSource system preference (try unsetting it later)
    - Verify all mandatory fields can be set to empty
    - Verify that you can't save before correctly setting them
  - Change your frameworks and set a default for itemtype (Ex: BK) and
    another mandatory and non-mandatory field of your choice
  - Add a new order line and item and verify the defaults are selected
Comment 3 David Nind 2020-04-24 08:18:52 UTC
Created attachment 103632 [details] [review]
Bug 21927: Acq - Add blank values in pull downs of mandatory item subfields

This is the same fix as on bug 14662, which fixed the behaviour in
cataloguing, but for the item form in acquisitions.

The code assumes 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. As the field
can never be 'unset', there will never be a 'required' warning.
Furthermore, it might be counterproductive to use mandatory fields,
as it might be easily forgotten to change the preselected value and
those mistakes will be hard to find.

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:
- This is best tested with an ACQ framework, but default can be used
  when no ACQ framework was created.
- In your MARC bibliographic framework:
  - In 952 make itemtype, classification source and some other pull downs
    like location or collection mandatory and set them to visibel if needed
- Create a new basket with 'items created while ordering'
- Add a new order, an existing record with 942$c set will work best
- Add items for your order line
  - 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 order line
    - Verify that classification source is preselected according to the
      DefaultClassificationSource system preference (try unsetting it later)
    - Verify all mandatory fields can be set to empty
    - Verify that you can't save before correctly setting them
  - Change your frameworks and set a default for itemtype (Ex: BK) and
    another mandatory and non-mandatory field of your choice
  - Add a new order line and item and verify the defaults are selected

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 Jonathan Druart 2020-04-24 11:09:19 UTC
What about the 2 left?

1658                             push @authorised_values, ""
1659                               unless ( $tagslib->{$tag}->{$subfield}->{mandatory} );


1667                             push @authorised_values, ""
1668                               unless ( $tagslib->{$tag}->{$subfield}->{mandatory} );
Comment 5 Katrin Fischer 2020-04-24 11:23:21 UTC
If these are the lines for hombranch, holdingbranch - check the linked bug. We decided branch should not have empty entries, so I kept the same behaviour here.
Comment 6 Jonathan Druart 2020-04-24 13:13:20 UTC
Created attachment 103682 [details] [review]
Bug 21927: Acq - Add blank values in pull downs of mandatory item subfields

This is the same fix as on bug 14662, which fixed the behaviour in
cataloguing, but for the item form in acquisitions.

The code assumes 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. As the field
can never be 'unset', there will never be a 'required' warning.
Furthermore, it might be counterproductive to use mandatory fields,
as it might be easily forgotten to change the preselected value and
those mistakes will be hard to find.

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:
- This is best tested with an ACQ framework, but default can be used
  when no ACQ framework was created.
- In your MARC bibliographic framework:
  - In 952 make itemtype, classification source and some other pull downs
    like location or collection mandatory and set them to visibel if needed
- Create a new basket with 'items created while ordering'
- Add a new order, an existing record with 942$c set will work best
- Add items for your order line
  - 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 order line
    - Verify that classification source is preselected according to the
      DefaultClassificationSource system preference (try unsetting it later)
    - Verify all mandatory fields can be set to empty
    - Verify that you can't save before correctly setting them
  - Change your frameworks and set a default for itemtype (Ex: BK) and
    another mandatory and non-mandatory field of your choice
  - Add a new order line and item and verify the defaults are selected

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 7 Martin Renvoize 2020-04-27 10:18:17 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 8 Joy Nelson 2020-05-08 20:57:06 UTC
Backported to 19.11.x for 19.11.06