Summary: | Lack of control on mandatory in orderreveive.tt | ||
---|---|---|---|
Product: | Koha | Reporter: | Christophe Croullebois <christophe.croullebois> |
Component: | Acquisitions | Assignee: | Christophe Croullebois <christophe.croullebois> |
Status: | CLOSED FIXED | QA Contact: | |
Severity: | critical | ||
Priority: | P1 - high | CC: | chris, jonathan.druart, katrin.fischer, paul.poulain |
Version: | 3.8 | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | --- |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: | ||
Attachments: |
proposed patch
rewrited patch Bug 8637: Lack of control on mandatory in orderreveive.tt this patch is finalised patch finalized [SIGNED-OFF] Bug 8637: Lack of control on mandatory in orderreveive.tt proposed patch proposed patch [SIGNED-OFF] Bug 8637: Lack of control on mandatory in orderreveive.tt |
Description
Christophe Croullebois
2012-08-14 08:37:58 UTC
Created attachment 11599 [details] [review] proposed patch In orderreceive.tt if your syspref 'AcqCreateItem' is on receive, you have an item block with mandatory fields, when you submit the form no check is done. It is a complete lack in the code, so I have used the same check as in additem.tt. thx for testing 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? Created attachment 11639 [details] [review] rewrited patch ok, right Owen, I have added two styles in itemrecorddisplay.tt, red and italic and I have changed '*' by 'Required'. About additem effectively we should see, maybe a new report. Thx Created attachment 11673 [details] [review] 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. Signed-off-by: Chris Cormack <chris@bigballofwax.co.nz> Works as described, signing off 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 Created attachment 11698 [details] [review] this patch is finalised I have changed CSS class and a variable's name (alertString2 to alertString) on Jonathan's demand. Created attachment 11699 [details] [review] patch finalized Created attachment 11735 [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. Created attachment 11745 [details] [review] proposed patch On Jonathan's suggestion I have created a new class "missing" and I have added the background to staff-global.css. Hi Christophe - what is missing used for? 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. Created attachment 11748 [details] [review] proposed patch this one must be ok 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! Forgot to reload the CSS... works fine now - signing off! 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. QA Comments: Templates modifications only. Patch clean. It refactores some css style into a new class "missing". Marked as Passed QA. 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 !) 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! :) |