Bug 20798 - Client side validation for for fund selection prevents adding only some records to a basket
Summary: Client side validation for for fund selection prevents adding only some recor...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 19453
Blocks:
  Show dependency treegraph
 
Reported: 2018-05-21 18:56 UTC by Kyle M Hall
Modified: 2018-06-07 08:12 UTC (History)
4 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 20798: Client side validation for for fund selection prevents adding only some records to a basket (2.39 KB, patch)
2018-05-21 19:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20798: Client side validation for for fund selection prevents adding only some records to a basket (2.48 KB, patch)
2018-05-28 21:45 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 20798: Client side validation for for fund selection prevents adding only some records to a basket (2.54 KB, patch)
2018-05-29 08:29 UTC, Chris Cormack
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2018-05-21 18:56:31 UTC
Bug 19453 "fixed" the fund validation that never actually worked. This validation should be removed altogether instead. It is overly simplistic and simply checks to see if each record in the batch has a fund selected regardless of whether that record will be imported or not.

In addition, the code completely ignores items generated by MarcItemFieldsToOrder such that even if items on that page have a fund selected, the javascript forces the librarian to make a selection from the left-hand fund pulldown even though it will not be used.

Considering this bad validation is basically 'new' as it never worked before, the most sensible course of action would be to 'undo' the change and remove the validation.
Comment 1 Kyle M Hall 2018-05-21 18:57:58 UTC
Video demonstration: https://monosnap.com/file/M11N3U3TaJcGKwRnnMMxNTc07Ia6nK
Comment 2 Kyle M Hall 2018-05-21 19:00:28 UTC
Created attachment 75462 [details] [review]
Bug 20798: Client side validation for for fund selection prevents adding only some records to a basket

Bug 19453 "fixed" the fund validation that never actually worked. This validation should be removed altogether instead. It is overly simplistic and simply checks to see if each record in the batch has a fund selected regardless of whether that record will be imported or not.

In addition, the code completely ignores items generated by MarcItemFieldsToOrder such that even if items on that page have a fund selected, the javascript forces the librarian to make a selection from the left-hand fund pulldown even though it will not be used.

Considering this bad validation is basically 'new' as it never worked before, the most sensible course of action would be to 'undo' the change and remove the validation.

Test Plan:
1) Add records to a basket from an import batch
2) Select only some of those items in the batch
3) Note Koha will not let you import them unless you set funds for *all*
   records in the batch
4) Apply this patch
5) Note Koha no longer forces a budget to be selected in the pulldown
Comment 3 Katrin Fischer 2018-05-28 21:40:01 UTC
I agree that this is a bug and the fix doesn't work correctly, but without any checking, it seems that an arbitrary fund is selected if you don't set one.

I think it would probably be possible to improve this, so that only the entries checked are validated, but that leaves us with the problem that a default fund can be set on the third tab.

I am signing off at this one, but I have filed bug 20830 to start discussion about proper validation.
Comment 4 Katrin Fischer 2018-05-28 21:45:50 UTC
Created attachment 75615 [details] [review]
Bug 20798: Client side validation for for fund selection prevents adding only some records to a basket

Bug 19453 "fixed" the fund validation that never actually worked. This validation should be removed altogether instead. It is overly simplistic and simply checks to see if each record in the batch has a fund selected regardless of whether that record will be imported or not.

In addition, the code completely ignores items generated by MarcItemFieldsToOrder such that even if items on that page have a fund selected, the javascript forces the librarian to make a selection from the left-hand fund pulldown even though it will not be used.

Considering this bad validation is basically 'new' as it never worked before, the most sensible course of action would be to 'undo' the change and remove the validation.

Test Plan:
1) Add records to a basket from an import batch
2) Select only some of those items in the batch
3) Note Koha will not let you import them unless you set funds for *all*
   records in the batch
4) Apply this patch
5) Note Koha no longer forces a budget to be selected in the pulldown

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

https://bugs.koha-community.org/show_bug.cgi?id=20789
Comment 5 Katrin Fischer 2018-05-28 21:46:04 UTC
Yeah there is also MarcFieldsToOrder... so something we won't fix quickly.
But we need to fix this broken workflow now.
Comment 6 Chris Cormack 2018-05-29 08:29:45 UTC
Created attachment 75620 [details] [review]
Bug 20798: Client side validation for for fund selection prevents adding only some records to a basket

Bug 19453 "fixed" the fund validation that never actually worked. This validation should be removed altogether instead. It is overly simplistic and simply checks to see if each record in the batch has a fund selected regardless of whether that record will be imported or not.

In addition, the code completely ignores items generated by MarcItemFieldsToOrder such that even if items on that page have a fund selected, the javascript forces the librarian to make a selection from the left-hand fund pulldown even though it will not be used.

Considering this bad validation is basically 'new' as it never worked before, the most sensible course of action would be to 'undo' the change and remove the validation.

Test Plan:
1) Add records to a basket from an import batch
2) Select only some of those items in the batch
3) Note Koha will not let you import them unless you set funds for *all*
   records in the batch
4) Apply this patch
5) Note Koha no longer forces a budget to be selected in the pulldown

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Chris Cormack <chrisc@catalyst.net.nz>
Comment 7 Nick Clemens 2018-05-29 14:46:48 UTC
Pushed to master for 18.11.

I marked the followup 20830 as priority - this patch restores some previous issues, but is needed to prevent bigger issues in not being able to place orders, should be considered for backport

Awesome work all, thank you.
Comment 8 Martin Renvoize 2018-06-05 10:53:40 UTC
This patch has been pushed to 18.05.x and will be in 18.05.01

I also look forward to the followup on bug 20830
Comment 9 Fridolin SOMERS 2018-06-07 07:12:20 UTC
Pushed to 17.11.x for 17.11.07.

JS code moved but change was easy to apply manually.
Comment 10 Fridolin SOMERS 2018-06-07 08:12:26 UTC
Depends on Bug 19453 not in 17.05.x