Bug 8637 - Lack of control on mandatory in orderreveive.tt
Summary: Lack of control on mandatory in orderreveive.tt
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: 3.8
Hardware: All All
: P1 - high critical (vote)
Assignee: Christophe Croullebois
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-14 08:37 UTC by Christophe Croullebois
Modified: 2019-06-27 09:24 UTC (History)
4 users (show)

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


Attachments
proposed patch (2.02 KB, patch)
2012-08-14 09:03 UTC, Christophe Croullebois
Details | Diff | Splinter Review
rewrited patch (3.84 KB, patch)
2012-08-16 13:07 UTC, Christophe Croullebois
Details | Diff | Splinter Review
Bug 8637: Lack of control on mandatory in orderreveive.tt (3.89 KB, patch)
2012-08-18 02:08 UTC, Chris Cormack
Details | Diff | Splinter Review
this patch is finalised (1.90 KB, patch)
2012-08-20 12:32 UTC, Christophe Croullebois
Details | Diff | Splinter Review
patch finalized (3.98 KB, patch)
2012-08-20 12:38 UTC, Christophe Croullebois
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8637: Lack of control on mandatory in orderreveive.tt (4.10 KB, patch)
2012-08-22 06:02 UTC, Katrin Fischer
Details | Diff | Splinter Review
proposed patch (5.42 KB, patch)
2012-08-22 13:06 UTC, Christophe Croullebois
Details | Diff | Splinter Review
proposed patch (5.47 KB, patch)
2012-08-22 13:53 UTC, Christophe Croullebois
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 8637: Lack of control on mandatory in orderreveive.tt (5.58 KB, patch)
2012-08-22 20:21 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 Christophe Croullebois 2012-08-14 08:37:58 UTC
Just add a check in Jscript when the form is submited.
Comment 1 Christophe Croullebois 2012-08-14 09:03:50 UTC Comment hidden (obsolete)
Comment 2 Owen Leonard 2012-08-14 17:04:00 UTC
This looks like it works correctly, but I think it would be a good idea to pull a couple of things from the additem.pl template:

<label class="required">[% ite.subfield %] - [% ite.marc_lib %]</label>

...and...

[% IF ( ite.mandatory ) %] <span class="required">Required</span>[% END %]

This would make the mandatory nature of the fields a little more obvious and make it consistent with other forms.

Looking at this makes me wonder... Why don't we use services/itemrecorddisplay.pl for additem.pl too?
Comment 3 Christophe Croullebois 2012-08-16 13:07:02 UTC Comment hidden (obsolete)
Comment 4 Chris Cormack 2012-08-18 02:08:49 UTC Comment hidden (obsolete)
Comment 5 Chris Cormack 2012-08-18 02:09:38 UTC
Works as described, signing off
Comment 6 Jonathan Druart 2012-08-20 11:27:05 UTC
QA Comments:

Please use an existing class for style.
There are .required and span.required in the staff-global.css file.

+ rename the alertString2 with alertString

Marked as Failed QA
Comment 7 Christophe Croullebois 2012-08-20 12:32:27 UTC Comment hidden (obsolete)
Comment 8 Christophe Croullebois 2012-08-20 12:38:17 UTC Comment hidden (obsolete)
Comment 9 Katrin Fischer 2012-08-22 06:02:43 UTC Comment hidden (obsolete)
Comment 10 Christophe Croullebois 2012-08-22 13:06:59 UTC Comment hidden (obsolete)
Comment 11 Katrin Fischer 2012-08-22 13:27:54 UTC
Hi Christophe - what is missing used for?
Comment 12 Christophe Croullebois 2012-08-22 13:41:53 UTC
hello Katrin,
it is Jonathan who prefers to have style in css files, I have just created this class to point the background style on it.
And I have done the some in additem.
But I failed with the last patch, I'll put the right one in a minute.
For testing, it changes nothing, just the patch must be ok as before.
Thx Katrin.
Comment 13 Christophe Croullebois 2012-08-22 13:53:07 UTC Comment hidden (obsolete)
Comment 14 Katrin Fischer 2012-08-22 20:17:18 UTC
Hi Cristophe,
I remember when I tested this morning, that the mandatory fields that were missing values got highlighted in yellow. This does not seem to work for me now. Sorry!
Comment 15 Katrin Fischer 2012-08-22 20:21:01 UTC
Forgot to reload the CSS... works fine now - signing off!
Comment 16 Katrin Fischer 2012-08-22 20:21:58 UTC
Created attachment 11755 [details] [review]
[SIGNED-OFF] Bug 8637: Lack of control on mandatory in orderreveive.tt

Just add a check in Jscript when the form is submited, the same as in additem.tt
On Owen's suggestion I have added the red color and 'Required', the same as in additem.
On Jonathan's suggestion I have used the CSS class for red and italic and I have changed a variable's name (alertString2).
Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz>

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Mandatory fields are now correctly checked on saving.

On other Jonathan's suggestion I have created a new class "missing" and I have added the background to staff-global.css.
The same for additem.tt.

Signed-off-by: Katrin Fischer <Katrin.Fischer.83@web.de>
Retested because of minor CSS change. Works nicely.
Comment 17 Jonathan Druart 2012-08-23 07:47:57 UTC
QA Comments:

Templates modifications only. Patch clean.
It refactores some css style into a new class "missing".

Marked as Passed QA.
Comment 18 Paul Poulain 2012-08-31 12:09:38 UTC
This patch fixes the problem (so i'll push), but it could be improved later: if you recieve more than one item, and click on "Add" before clicking on "Save", you'll be warned of a problem only when you click "Save". You should be warned on each "Add"
(if you add more than 1 recieve, and only 1 lack a mandatory information, you won't know/see which item is wrong !)
Comment 19 Chris Cormack 2012-09-01 02:55:06 UTC
Pushed to 3.8.x, will be in 3.8.5. I agree with Paul this could be improved, but it is a good start already. Kaizen! :)