Uploaded image for project: 'OFBiz'
  1. OFBiz
  2. OFBIZ-7511

Add proper form validation across the application

    Details

    • Type: Improvement
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: Trunk
    • Fix Version/s: Upcoming Release
    • Component/s: ALL APPLICATIONS
    • Labels:
      None
    • Sprint:
      2016 - OFBiz Community Day 4

      Description

      This is the place holder task for adding proper form validation across the application.

      1.
      Proper Form Validation should be there while adding new Example Feature Sub-task Closed Michael Brohl
       
      2.
      Proper form validation should be there while creating Agreement Roles Sub-task Closed Michael Brohl
       
      3.
      Proper form validation should be there while creating Agreement Item Facility Sub-task Closed Michael Brohl
       
      4.
      Proper form validation should be there while creating Agreement Item Party Sub-task Closed Michael Brohl
       
      5.
      Proper Form validation should be added while creating new agreement product Sub-task Closed Michael Brohl
       
      6.
      Client Side validation missing on Create Payment screen Sub-task Closed Michael Brohl
       
      7.
      Client side validation required in Tax Infos (Add Party Tax Authority Info) Sub-task Closed Michael Brohl
       
      8.
      Client Side validation missing on Create New Tax Authority Form Sub-task Closed Michael Brohl
       
      9.
      Client side validation on AddProductCostComponentCalc form on catalog Sub-task Closed Michael Brohl
       
      10.
      Client side validation required in Add Billing Accounts Role Sub-task Closed Michael Brohl
       
      11.
      Client side validation on Add product categories form in catalog Sub-task Closed Michael Brohl
       
      12.
      Require client side validation while creating Product Association Sub-task Closed Michael Brohl
       
      13.
      Proper client side validation while creating price rule from catalog manager Sub-task Closed Michael Brohl
       
      14.
      Require check on Add Facility Party Role Screen Sub-task Closed Jacopo Cappellato
       
      15.
      Client side validation is missing at Create User Login form Sub-task Closed Michael Brohl
       
      16.
      Client side validation is missing at Create Ebay Account form Sub-task Closed Michael Brohl
       
      17.
      Client side validation is missing at Add Ebay Configuration form Sub-task Closed Michael Brohl
       

        Activity

        Hide
        deepak.dixit Deepak Dixit added a comment -

        I'd prefer component level ticket for this kind of effort

        Show
        deepak.dixit Deepak Dixit added a comment - I'd prefer component level ticket for this kind of effort
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        You miss something to do it?

        Show
        jacques.le.roux Jacques Le Roux added a comment - You miss something to do it?
        Hide
        amardeepsj Amardeep Singh Jhajj added a comment -

        +1.

        We have lot of screens where client side validation is missing, so component level effort seems to be correct approach instead of making tickets for each page. Thanks.

        Show
        amardeepsj Amardeep Singh Jhajj added a comment - +1. We have lot of screens where client side validation is missing, so component level effort seems to be correct approach instead of making tickets for each page. Thanks.
        Hide
        deepak.dixit Deepak Dixit added a comment -

        I'll create component level ticket as well

        Show
        deepak.dixit Deepak Dixit added a comment - I'll create component level ticket as well
        Hide
        deepak.dixit Deepak Dixit added a comment -

        I think we can fix this at framework level as well,instead of adding required-label field for all form, we can fix this required-field at the time of form rendering.

        I think this has been handled in auto-filed-service tag in form.

        Show
        deepak.dixit Deepak Dixit added a comment - I think we can fix this at framework level as well,instead of adding required-label field for all form, we can fix this required-field at the time of form rendering. I think this has been handled in auto-filed-service tag in form.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        Not sure about auto-filed-service tag in form, but +1 for the idea

        Show
        jacques.le.roux Jacques Le Roux added a comment - Not sure about auto-filed-service tag in form, but +1 for the idea
        Hide
        diveshdut Divesh Dutta added a comment - - edited

        auto-fields-service handles required option i.e if IN attribute is not optional then required is set to "true" in form widget. so where ever auto-fields-service is used fields will be automatically work as required field, but in forms where you want to make field mandatory but that field in not mandatory in service definition, there is no option available.

        Also forms which are directly not based on service, for those forms, there is no option available. So such cases, this is the only option available i.e you will have to define required explicitly in form.

        I don't see any thing to to be fixed at framework level here as auto-fields-service handles this already . Approach taken in ticket can be done when, you cannot use auto-fields-service or auto-fields-service is used but fields are overridden in form. So my +1 for the approach taken in this ticket. I agree that we can manage this effort at component level instead of each form.

        Deepak Dixit what do you have in mind when you say we can fix this required-field at the time of form rendering ? There is no bug in required-field.

        Show
        diveshdut Divesh Dutta added a comment - - edited auto-fields-service handles required option i.e if IN attribute is not optional then required is set to "true" in form widget. so where ever auto-fields-service is used fields will be automatically work as required field, but in forms where you want to make field mandatory but that field in not mandatory in service definition, there is no option available. Also forms which are directly not based on service, for those forms, there is no option available. So such cases, this is the only option available i.e you will have to define required explicitly in form. I don't see any thing to to be fixed at framework level here as auto-fields-service handles this already . Approach taken in ticket can be done when, you cannot use auto-fields-service or auto-fields-service is used but fields are overridden in form. So my +1 for the approach taken in this ticket. I agree that we can manage this effort at component level instead of each form. Deepak Dixit what do you have in mind when you say we can fix this required-field at the time of form rendering ? There is no bug in required-field.
        Hide
        deepak.dixit Deepak Dixit added a comment -

        Thanks Divesh Dutta for your detailed input,

        My proposal to identify form request and if an event (type of service) is associated with request than we can use that to identify required fields and in OFBiz most of the forms are using request that call an service type of event.

        Currently we are using this pattern to render hyperlink whit link-type=auto. It check if request has an service type event then it create hidden form else it render the anchor tag.

        Show
        deepak.dixit Deepak Dixit added a comment - Thanks Divesh Dutta for your detailed input, My proposal to identify form request and if an event (type of service) is associated with request than we can use that to identify required fields and in OFBiz most of the forms are using request that call an service type of event. Currently we are using this pattern to render hyperlink whit link-type=auto. It check if request has an service type event then it create hidden form else it render the anchor tag.
        Hide
        deepak.dixit Deepak Dixit added a comment - - edited

        There is no bug in required-field attribute, we can fix or can say improve form rendering mechanism to handle the required-field based on target.

        Show
        deepak.dixit Deepak Dixit added a comment - - edited There is no bug in required-field attribute, we can fix or can say improve form rendering mechanism to handle the required-field based on target.
        Hide
        mridul.pathak Mridul Pathak added a comment -

        Hi Deepak Dixit

        auto-field-service tag does the job already and provides the required flexibility to override validations as well and when auto-field-service already provides this facility I do not see it as something that needs to be fixed at framework level. The problem I see here is that in many forms auto-field-service might have been used but when the fields are overridden validations are not applied properly and this is some thing that needs to be fixed.
        In fact, the approach you are suggesting seems to be hardcoding validations at the renderer level and would remove the flexibility we have at this moment, I might be mistaken though.

        Show
        mridul.pathak Mridul Pathak added a comment - Hi Deepak Dixit auto-field-service tag does the job already and provides the required flexibility to override validations as well and when auto-field-service already provides this facility I do not see it as something that needs to be fixed at framework level. The problem I see here is that in many forms auto-field-service might have been used but when the fields are overridden validations are not applied properly and this is some thing that needs to be fixed. In fact, the approach you are suggesting seems to be hardcoding validations at the renderer level and would remove the flexibility we have at this moment, I might be mistaken though.
        Hide
        deepak.dixit Deepak Dixit added a comment -

        Hi Mridul Pathak,

        How can we say its hardcoding? I am proposing same that used in auto-field-service, if we did not use auto-field-service in form, than we can use the target to analyze the required field. So this is not hard coding at rendering level.

        Show
        deepak.dixit Deepak Dixit added a comment - Hi Mridul Pathak , How can we say its hardcoding? I am proposing same that used in auto-field-service, if we did not use auto-field-service in form, than we can use the target to analyze the required field. So this is not hard coding at rendering level.
        Hide
        mridul.pathak Mridul Pathak added a comment -

        If we are rendering validations by default through form renderer using the auto-field-service validation, would there still be the flexibility to override validation like auto-field-service provide?

        Show
        mridul.pathak Mridul Pathak added a comment - If we are rendering validations by default through form renderer using the auto-field-service validation, would there still be the flexibility to override validation like auto-field-service provide?
        Hide
        diveshdut Divesh Dutta added a comment -

        Ok, Deepak Dixit I am getting your point. Your solution will work when we call service. But in case of event this solution will not work and in that case we will have to explicitly define required field in form as done in this task.

        So I think the solution you are proposing is not generic.

        Show
        diveshdut Divesh Dutta added a comment - Ok, Deepak Dixit I am getting your point. Your solution will work when we call service. But in case of event this solution will not work and in that case we will have to explicitly define required field in form as done in this task. So I think the solution you are proposing is not generic.
        Hide
        deepak.dixit Deepak Dixit added a comment -

        I am not getting the point "override validation like auto-field-service provide?'

        I think currently we don't have any option in auto-field-service to override required field.

        I am not proposing to remove required-field attribute if user want to override it so he can use required-field to override behavior.

        Show
        deepak.dixit Deepak Dixit added a comment - I am not getting the point "override validation like auto-field-service provide?' I think currently we don't have any option in auto-field-service to override required field. I am not proposing to remove required-field attribute if user want to override it so he can use required-field to override behavior.
        Hide
        diveshdut Divesh Dutta added a comment - - edited

        Also I agree with Mridul that when we already have auto-fields-service feature which allows you to render all the fields of service in form and put required validation on fields in forms which are not optional in service, then why we are you proposing to identify request with an event of type service and then putting validations. auto-fields service already does the job.

        Show
        diveshdut Divesh Dutta added a comment - - edited Also I agree with Mridul that when we already have auto-fields-service feature which allows you to render all the fields of service in form and put required validation on fields in forms which are not optional in service, then why we are you proposing to identify request with an event of type service and then putting validations. auto-fields service already does the job.
        Hide
        diveshdut Divesh Dutta added a comment -

        why will we not use auto-fields-service ? Develper should use auto-field service when ever possible, and when developer will use auto-field-service then we don't have to work on solution you are proposing.

        Show
        diveshdut Divesh Dutta added a comment - why will we not use auto-fields-service ? Develper should use auto-field service when ever possible, and when developer will use auto-field-service then we don't have to work on solution you are proposing.
        Hide
        deepak.dixit Deepak Dixit added a comment -

        If we can enhance form rendering then why go and add required filed on each form?

        If form uses auto-fields service then its done by default, and if form not using auto-fields service then we can enhance form widget.

        I am done with my part
        If still it make sense to go and add required-field on each form than its fine I don't have any concern.

        Show
        deepak.dixit Deepak Dixit added a comment - If we can enhance form rendering then why go and add required filed on each form? If form uses auto-fields service then its done by default, and if form not using auto-fields service then we can enhance form widget. I am done with my part If still it make sense to go and add required-field on each form than its fine I don't have any concern.
        Hide
        deepak.dixit Deepak Dixit added a comment -

        Make sense Divesh,

        developer should use auto-service-field when ever possible.
        I had a discussion with Divesh Dutta regarding this, and we found that if we use auto-field-service and than override field in form than its override the required-filed behavior and set it to false.

        I think here we can fix/improve the auto-field-service behavior, If an field is set required true by auto-field-service than form field should not override this behavior until unless explicitly set required-fiedd in overridden attribute

        Show
        deepak.dixit Deepak Dixit added a comment - Make sense Divesh, developer should use auto-service-field when ever possible. I had a discussion with Divesh Dutta regarding this, and we found that if we use auto-field-service and than override field in form than its override the required-filed behavior and set it to false. I think here we can fix/improve the auto-field-service behavior, If an field is set required true by auto-field-service than form field should not override this behavior until unless explicitly set required-fiedd in overridden attribute
        Hide
        diveshdut Divesh Dutta added a comment -

        Yea this solution makes sense Deepak.

        Show
        diveshdut Divesh Dutta added a comment - Yea this solution makes sense Deepak.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        When I have to choice I always prefer the solution which maximally automates (and I guess most of us do ).

        It seems Deepak proposition fits here. Let me resume to see if we are all on the same page. If a form uses a service, instead of using the manual auto-field-service element, we would have a widget coded mechanism able to automaticaly infer the field to be rendered as required, based on the service definition. So auto-field-service would be deprecated, we could remove it. But of course in case you would need to change the default behaviour (required true from service) and have a field not required a required-field attribute forced to false would be used.

        Show
        jacques.le.roux Jacques Le Roux added a comment - When I have to choice I always prefer the solution which maximally automates (and I guess most of us do ). It seems Deepak proposition fits here. Let me resume to see if we are all on the same page. If a form uses a service, instead of using the manual auto-field-service element, we would have a widget coded mechanism able to automaticaly infer the field to be rendered as required, based on the service definition. So auto-field-service would be deprecated, we could remove it. But of course in case you would need to change the default behaviour (required true from service) and have a field not required a required-field attribute forced to false would be used.
        Hide
        diveshdut Divesh Dutta added a comment - - edited

        Jacques Le Roux this discussion has been concluded. There was confusion in actual problem area. Actually the problem area is:

        When you use auto-field-service in form say for eg:

        <form name="EditExampleFeature" type="single" target="updateExampleFeature" title="" default-map-name="exampleFeature">
                <auto-fields-service service-name="updateExampleFeature"/>
                <field name="submitButton" title="${uiLabelMap.CommonCreate}"><submit button-type="button"/></field>
         </form>
        

        Then exampleFeatureId is mandatory field in this form because exampleFeatureId is mandatory attribute of service. Now if you create same form then where you want to use tool tip in exampleFeatureId ,

        <form name="EditExampleFeature" type="single" target="updateExampleFeature" title="" default-map-name="exampleFeature">
                <auto-fields-service service-name="updateExampleFeature"/>
                 <override field name="exampleFeatureId" title="${uiLabelMap.ExampleExampleFeatureId}" tooltip="${uiLabelMap.CommonNotModifRecreat}"><display/></field>
                <field name="submitButton" title="${uiLabelMap.CommonCreate}"><submit button-type="button"/></field>
         </form>
        

        In this case, you have overridden the exampleFeatureId field, but during overriding required property is lost. And to solve this problem, Deepak is recommending that when you override the field , then by default required property should be inherited from auto-fields service, so that developer does not have to explicitly define required-field=true . And we agreed on this proposal of Deepak .

        So we agreed on solution which maximally automates. Hope this helps to understand the problem area and solution concluded.

        Show
        diveshdut Divesh Dutta added a comment - - edited Jacques Le Roux this discussion has been concluded. There was confusion in actual problem area. Actually the problem area is: When you use auto-field-service in form say for eg: <form name= "EditExampleFeature" type= "single" target= "updateExampleFeature" title= "" default -map-name=" exampleFeature"> <auto-fields-service service-name= "updateExampleFeature" /> <field name= "submitButton" title= "${uiLabelMap.CommonCreate}" ><submit button-type= "button" /></field> </form> Then exampleFeatureId is mandatory field in this form because exampleFeatureId is mandatory attribute of service. Now if you create same form then where you want to use tool tip in exampleFeatureId , <form name= "EditExampleFeature" type= "single" target= "updateExampleFeature" title= "" default -map-name=" exampleFeature"> <auto-fields-service service-name= "updateExampleFeature" /> <override field name= "exampleFeatureId" title= "${uiLabelMap.ExampleExampleFeatureId}" tooltip= "${uiLabelMap.CommonNotModifRecreat}" ><display/></field> <field name= "submitButton" title= "${uiLabelMap.CommonCreate}" ><submit button-type= "button" /></field> </form> In this case, you have overridden the exampleFeatureId field, but during overriding required property is lost. And to solve this problem, Deepak is recommending that when you override the field , then by default required property should be inherited from auto-fields service, so that developer does not have to explicitly define required-field=true . And we agreed on this proposal of Deepak . So we agreed on solution which maximally automates. Hope this helps to understand the problem area and solution concluded.
        Hide
        jacques.le.roux Jacques Le Roux added a comment -

        OK, this was not quite clear to me. I was expecting we could replace auto-field-service by automating it, like having

        <form name="EditExampleFeature" type="single" target="updateExampleFeature" title="" default-map-name="exampleFeature">
                <field name="submitButton" title="${uiLabelMap.CommonCreate}"><submit button-type="button"/></field>
         </form>
        

        See no need for auto-field-service. If it's not there, we assume we should do the same automatically when parsing

        But now that I think about it, it maybe adds more confusion than anything else Verbosity has its advantages!

        So +1 for the consensus.

        Show
        jacques.le.roux Jacques Le Roux added a comment - OK, this was not quite clear to me. I was expecting we could replace auto-field-service by automating it, like having <form name= "EditExampleFeature" type= "single" target= "updateExampleFeature" title= "" default -map-name=" exampleFeature"> <field name= "submitButton" title= "${uiLabelMap.CommonCreate}" ><submit button-type= "button" /></field> </form> See no need for auto-field-service. If it's not there, we assume we should do the same automatically when parsing But now that I think about it, it maybe adds more confusion than anything else Verbosity has its advantages! So +1 for the consensus.
        Hide
        soledad Nicolas Malin added a comment - - edited

        Thanks Diversh for the explanation, I had some difficulty than Jacques to understand exactly the problem.

        When we extend a form we can surcharge a field with only the wanted attribute to complete or change. Maybe override a field came from auto-field-* element would be treated like that.

        <form name="EditExampleFeature" type="single" target="updateExampleFeature" title="" default-map-name="exampleFeature">
                <auto-fields-service service-name="updateExampleFeature"/>
                <field name="exampleFeatureId" tooltip="${uiLabelMap.CommonNotModifRecreat}"/>
                <field name="submitButton" title="${uiLabelMap.CommonCreate}"><submit button-type="button"/></field>
         </form>
        

        Or for not disturbing developer and xsd, we can change the field analyse like service definition

        <form name="EditExampleFeature" type="single" target="updateExampleFeature" title="" default-map-name="exampleFeature">
                <auto-fields-service service-name="updateExampleFeature"/>
                <override field name="exampleFeatureId" tooltip="${uiLabelMap.CommonNotModifRecreat}"/>
                <field name="submitButton" title="${uiLabelMap.CommonCreate}"><submit button-type="button"/></field>
         </form>
        

        I quite aware of going off on a tangent, but sometime a hight level idea can resolve a specific case and some others !

        Show
        soledad Nicolas Malin added a comment - - edited Thanks Diversh for the explanation, I had some difficulty than Jacques to understand exactly the problem. When we extend a form we can surcharge a field with only the wanted attribute to complete or change. Maybe override a field came from auto-field-* element would be treated like that. <form name= "EditExampleFeature" type= "single" target= "updateExampleFeature" title= "" default -map-name=" exampleFeature"> <auto-fields-service service-name= "updateExampleFeature" /> <field name= "exampleFeatureId" tooltip= "${uiLabelMap.CommonNotModifRecreat}" /> <field name= "submitButton" title= "${uiLabelMap.CommonCreate}" ><submit button-type= "button" /></field> </form> Or for not disturbing developer and xsd, we can change the field analyse like service definition <form name= "EditExampleFeature" type= "single" target= "updateExampleFeature" title= "" default -map-name=" exampleFeature"> <auto-fields-service service-name= "updateExampleFeature" /> <override field name= "exampleFeatureId" tooltip= "${uiLabelMap.CommonNotModifRecreat}" /> <field name= "submitButton" title= "${uiLabelMap.CommonCreate}" ><submit button-type= "button" /></field> </form> I quite aware of going off on a tangent, but sometime a hight level idea can resolve a specific case and some others !
        Hide
        mbrohl Michael Brohl added a comment -

        Hey guys,

        all sub-tasks are closed now, can I close this issue too?

        Show
        mbrohl Michael Brohl added a comment - Hey guys, all sub-tasks are closed now, can I close this issue too?
        Hide
        mbrohl Michael Brohl added a comment -

        Closing this issue because all subtasks are closed and no objections against closing the umbrella issue.

        Show
        mbrohl Michael Brohl added a comment - Closing this issue because all subtasks are closed and no objections against closing the umbrella issue.

          People

          • Assignee:
            mbrohl Michael Brohl
            Reporter:
            deepak.dixit Deepak Dixit
          • Votes:
            0 Vote for this issue
            Watchers:
            8 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development

                Agile