[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: |
|
||||||||||||||||||||
| Issue Links: |
|
||||||||||||||||||||
| 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. Much easier would be if the required could be set on the composite itself. Use case definition: Problem: 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:
Proposed solution:
|
| Comments |
| Comment by Aleksandr Pchelintcev [ 19/Nov/14 ] |
|
| 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. |
| 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:
I see three possible interpretations for it could mean to mark a composite field as "required":
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 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. |