Bug 10543 - false mandatory warning in additem
Summary: false mandatory warning in additem
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Cataloging (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Fridolin SOMERS
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-07-05 13:19 UTC by Fridolin SOMERS
Modified: 2014-12-07 20:02 UTC (History)
7 users (show)

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


Attachments
Proposed patch (3.73 KB, patch)
2013-07-05 14:00 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 10543 - false mandatory warning in additem (4.00 KB, patch)
2013-07-11 14:41 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 10543 - Unify item mandatory subfields check (5.04 KB, patch)
2013-07-11 14:53 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
Bug 10543 - add mandatory subfield check to addorderiso2709.pl (1.91 KB, patch)
2013-07-11 14:55 UTC, Fridolin SOMERS
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10543 - false mandatory warning in additem (4.06 KB, patch)
2013-07-17 02:36 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10543 - Unify item mandatory subfields check (5.09 KB, patch)
2013-07-17 02:38 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 10543 - add mandatory subfield check to addorderiso2709.pl (1.96 KB, patch)
2013-07-17 02:38 UTC, Srdjan Jankovic
Details | Diff | Splinter Review
[PASSED QA] Bug 10543 - false mandatory warning in additem (4.13 KB, patch)
2013-07-29 21:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10543 - Unify item mandatory subfields check (5.13 KB, patch)
2013-07-29 21:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
[PASSED QA] Bug 10543 - add mandatory subfield check to addorderiso2709.pl (3.03 KB, patch)
2013-07-29 21:10 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Fridolin SOMERS 2013-07-05 13:19:24 UTC
When editing a item, we get a popup warning :
"1 mandatory fields empty (highlighted)".
But all mandatory fields are filled.

This is because there is a field with a huge content (more than 100 characters), it it displayed in a textarea instead of input.
This braked javascript  form check.
Comment 1 Fridolin SOMERS 2013-07-05 14:00:50 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2013-07-05 15:10:54 UTC
Could you please provide a test plan for this patch?
Comment 3 Chris Cormack 2013-07-07 03:32:49 UTC
Kyle the test plan is on the patch (in the commit message as it should be)
Comment 4 Kyle M Hall 2013-07-08 12:20:05 UTC
Test plan from patch:

Test plan :
- In a framework, make sure you have in item field :
  An optionnal subfield (without authorised value) then a mandatory subfield then an optional subfield next to mandatory subfield
  For example : $i (optional), $r (mandatory) and $s (optional)
- Open a biblio record and create a new item
- Enter more than 100 characters in $i, fill $r and $s
- Click "Add item"
- Edit this item
=> You get a textarea for $i
- Empty $s and Save
=> Without patch, you get a warning and form is not saved and you see that $s is yellow while it should be $r
=> With patch, form is saved
- Re-edit this item
- Empty $r and save
=> You get a warning and form is not saved
Comment 5 Srdjan Jankovic 2013-07-10 04:31:24 UTC
Maybe we should consider putting that function in a separate js while we are at it? Having it tripled does not feel right.
Comment 6 Fridolin SOMERS 2013-07-10 07:19:55 UTC
> Maybe we should consider putting that function in a separate js while we are at it? Having it tripled does not feel right.
It is only double and it is quite short. In acquisition there is a template condition : [% IF (AcqCreateItemReceiving) %]
Comment 7 Galen Charlton 2013-07-10 15:07:56 UTC
(In reply to Fridolyn SOMERS from comment #6)
> > Maybe we should consider putting that function in a separate js while we are at it? Having it tripled does not feel right.
> It is only double and it is quite short. In acquisition there is a template
> condition : [% IF (AcqCreateItemReceiving) %]

No, there are three: serials-edit.tt includes a version of that same check; the serials receiving form should be checked to see whether the same bug applies there as well.
Comment 8 Fridolin SOMERS 2013-07-11 14:41:30 UTC Comment hidden (obsolete)
Comment 9 Fridolin SOMERS 2013-07-11 14:53:12 UTC Comment hidden (obsolete)
Comment 10 Fridolin SOMERS 2013-07-11 14:55:41 UTC Comment hidden (obsolete)
Comment 11 Fridolin SOMERS 2013-07-11 14:57:40 UTC
(In reply to Galen Charlton from comment #7)
> No, there are three: serials-edit.tt includes a version of that same check;
> the serials receiving form should be checked to see whether the same bug
> applies there as well.

Thanks for your help, you where totally right.
Comment 12 Srdjan Jankovic 2013-07-16 00:14:11 UTC
Fridolyn,
The last one, shouldn't it be just CheckMandatorySubfields()?
Comment 13 Fridolin SOMERS 2013-07-16 07:17:24 UTC
(In reply to Srdjan Jankovic from comment #12)
> Fridolyn,
> The last one, shouldn't it be just CheckMandatorySubfields()?

The last patch ?
No, the CheckMandatorySubfields() has an argument which is the parent object to use for searching subfield lines. This is necessary for the serial edition where there are hidden subfield lines, one must not check all line of the form.
Comment 14 Srdjan Jankovic 2013-07-17 02:36:25 UTC Comment hidden (obsolete)
Comment 15 Srdjan Jankovic 2013-07-17 02:38:08 UTC Comment hidden (obsolete)
Comment 16 Srdjan Jankovic 2013-07-17 02:38:52 UTC Comment hidden (obsolete)
Comment 17 Katrin Fischer 2013-07-29 21:09:51 UTC
Created attachment 19980 [details] [review]
[PASSED QA] Bug 10543 - false mandatory warning in additem

When editing a item, you get a popup warning :
  "1 mandatory fields empty (highlighted)".
But all mandatory fields are filled.
This is because there is a field with a huge content (more than 100 characters), it it displayed in a textarea instead of input.
This patch corrects javascript form check by using siblings().
Also adds a more precise filter to find mandatory inputs because looking in full page may bring unwanted results.

Test plan :
- In a framework, make sure you have in item field :
  An optionnal subfield (without authorised value) then a mandatory subfield then an optional subfield next to mandatory subfield
  For example : $i (optional), $r (mandatory) and $s (optional)
- Open a biblio record and create a new item
- Enter more than 100 characters in $i, fill $r and $s
- Click "Add item"
- Edit this item
=> You get a textarea for $i
- Empty $s and Save
=> Without patch, you get a warning and form is not saved and you see that $s is yellow while it should be $r
=> With patch, form is saved
- Re-edit this item
- Empty $r and save
=> You get a warning and form is not saved

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Notes on last patch.
Comment 18 Katrin Fischer 2013-07-29 21:10:06 UTC
Created attachment 19981 [details] [review]
[PASSED QA] Bug 10543 - Unify item mandatory subfields check

Use of CheckMandatorySubfields from cataloging.js everywhere an item cataloging form is checked for mandatories

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 19 Katrin Fischer 2013-07-29 21:10:30 UTC
Created attachment 19982 [details] [review]
[PASSED QA] Bug 10543 - add mandatory subfield check to addorderiso2709.pl

When adding orders from staged file, you can create one order per record using item form in the bottom of addorderiso2709.pl.
This form was missing check.

Signed-off-by: Srdjan <srdjan@catalyst.net.nz>
Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>

All 3 patches pass tests and QA script.

Some additional notes on the test plan from the first patch:
It took me a while to figure out a combination of subfields
that would trigger the bug. I am not sure about the condition
that transforms the input field into a textbox as it does
not seem to happen for every field.

What worked in the end was (MARC21):
- $g cost, normal purchase price = mandatory
- $h serial enumeration/chronolgy = optional, filled with
  text over 100 characters

Checked for:
- Adding items in cataloguing
  Was able to reproduce the problem, mandatory check works
  as expected.
- Adding items from staged file in acq
  Was not able to trigger the problem, because we don't edit
  the item here, but mandatory check works as expected.
- Addings items for serial subscriptions
  Was not able to trigger the problem, because we don't edit
  the items here, but mandatory check works better as before
  as missing mandatory fields are now highlighted and weren't
  before.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Comment 20 Galen Charlton 2013-07-30 14:54:06 UTC
Pushed to master.  Thanks, Fridolyn!
Comment 21 Tomás Cohen Arazi 2013-08-28 17:06:22 UTC
This patch has been pushed to 3.12.x, will be in 3.12.5.

Thanks Fridolyn!
Comment 22 Bernardo Gonzalez Kriegel 2013-09-14 19:12:11 UTC
Pushed to 3.10.x, will be in 3.10.11