[MGNLUI-3227] Improve validation capabilities of the multi fields Created: 29/Oct/14  Updated: 06/Aug/15  Resolved: 08/Jan/15

Status: Closed
Project: Magnolia UI
Component/s: forms
Affects Version/s: 5.2.10, 5.3.4
Fix Version/s: 5.3.6

Type: Improvement Priority: Neutral
Reporter: Christian Ringele Assignee: Aleksandr Pchelintcev
Resolution: Incomplete Votes: 0
Labels: None
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Attachments: JPEG File CompositeField-Required.jpg     Text File CompositeField.patch     PNG File no-validation-indicator-on-required-composite-field.png    
Issue Links:
Cloners
is cloned by MGNLUI-3313 Improve validation capabilities of th... Closed
causality
is causing MGNLUI-3312 SwitchableField doesn't work correctl... Closed
relation
Template:
Patch included:
Yes
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Date of First Response:

 Description   

Initial issue title: Field Validation: Don't ignore 'required' property on the CompositeField itself.
Setting on the CompositeFieldDefinition the required property it is ignored.
So if one needs to ensure that all fields in the composite need to be filled out, one must set to all sub fields a required property.

Much easier would be if the required could be set on the composite itself.

Use case definition:
If the Composite field sets required=true, every sub field in the composite needs a value.

Problem:
CompositeFieldDefinition does not implement isValid(), and the super's isValid is only checking the validation of the sub fields.

This code solves the problem, I included a patch.

@Override
    public boolean isValid() {
        boolean isValid = super.isValid();
        
        if (this.isRequired()) {
            if (isEmpty()) {
               isValid = false;
            }
        }
        return isValid;
    }

UPD:

  • This looks like a part of more fundamental problem: the validate() method of com.vaadin.ui.AbstractField is not invoked at all.
    • If it was invoked then the standard isRequired()/isEmpty() check would be handled.
    • It is supposed to be called within com.vaadin.ui.AbstractField#isValid but AbstractCustomMultiField impl doesn't call super method.
    • As another side effect the value validators of the AbstractCustomMultiField wouldn't be triggered since that also happens in the validate() method.

Proposed solution:

  • Simply call super.isValid() in AbstractCustomMultiField#isValid instead of assigning true as a default value.


 Comments   
Comment by Aleksandr Pchelintcev [ 19/Nov/14 ]
  • This looks like a part of more fundamental problem: the validate() method of com.vaadin.ui.AbstractField is not invoked at all.
    • If it was invoked then the standard isRequired()/isEmpty() check would be handled.
    • It is supposed to be called within com.vaadin.ui.AbstractField#isValid but AbstractCustomMultiField impl doesn't call super method.
    • As another side effect the value validators of the AbstractCustomMultiField wouldn't be triggered since that also happens in the validate() method.
Comment by Christopher Zimmermann [ 08/Dec/14 ]

Reopening because although the form validation does not pass when a composite field is required and all its children are not present - the composite field shows no indication that validation has failed. So a user will not know why the validation failed.
Either the field should show the same validation indicators as other fields - or we should not support validation on a composite field.
See attached screenshot to see problem.

Comment by Andreas Weder [ 10/Dec/14 ]

I've been asked to comment on this. I see some need to make it easier to make all fields of a composite field required, though I'm not sure this goes in the right direction.

Below I'm looking at these questions:

  • What does it mean, if we mark a composite field as required?
  • How do we indicate, if validation fails of at least one of its fields?
  • How do we visually mark a composite field as "required"?

I see three possible interpretations for it could mean to mark a composite field as "required":

  1. if a composite field is marked as "required", all its fields must have a value
  2. if a composite field is marked as "required", at least one of its fields requires a value
  3. if a composite field is marked as "required", at exactly one of its fields requires a value

We don't have to necessarily build all these cases, but we should make sure they remain possible, if we implement a shortcut.

As for showing a message on validation failure, I agree with @Topher: yes, we absolutely need that. In all cases, we should be fine by showing an error message on composite field level. I'd prefer if we could also show a message on the field level, if I have to indicate that one or several fields are missing. However, I would expect such a solution to become rather complex, as the good placement of this largely on form layout and field types.

Visually, we don't have a design for that, but I'd show such a messages on composite field level the same way as we currently do it for any other field. Just show it below the composite field with an arrow pointing towards it, but make sure we can show multiple lines:

   First name is required. 
   Last name is required.

As for how we should indicate if all fields of a composite fields are required, we could set the mark on either the composite field itself or on all its fields. We should be fine for both cases mentioned above if we just mark the composite field as "required", though. Again - and strictly speaking -, this actually depends a lot on the form layout and the complexity of the composite field. If a composite field spreads its field across multiple lines, it would be clearer to mark each individual field as "required". But I think we'll be fine if we just mark the composite as "required".

Comment by Christopher Zimmermann [ 10/Dec/14 ]

Moving to 5.4 version as it requires further analysis, and no support requests require feature as spec-ed in this ticket.

Comment by Maik Jablonski [ 20/Dec/14 ]

Somehow the proposed change landed in Magnolia 5.3.6 which breaks existing code:

Just use a Switchable with a Link- and a TextField. Saving a form containing this Switchable is now impossible as long as you don't type anything into the TextField (regardless if the TextField is selected or not). The #isEmpty() contract for a AbstractTextField is "return super.isEmpty() || getValue().length() == 0;" which isn't handled correctly in #isEmpty() in the AbstractCustomMultiField.

Comment by Mikaël Geljić [ 08/Jan/15 ]

Hi Maik,

You're right, the initial change landed in 5.3.6 (ticket should probably have been split rather than reopoened). Anyway, let me link to MGNLUI-3312, which I believe is the issue you described.

At least this change — as is — should be reverted to start with.

Sorry for any inconvenience this may cause,

Comment by Mikaël Geljić [ 08/Jan/15 ]

Cloned ticket for further improvements. Closing this one with fix version back to 5.3.6.

Generated at Mon Feb 12 09:04:29 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.