[MGNLUI-3715] Only values for selected options should be saved when using SwitchableFieldDefinition Created: 11/Dec/15 Updated: 27/Jan/17 Resolved: 17/Mar/16 |
|
| Status: | Closed |
| Project: | Magnolia UI |
| Component/s: | forms |
| Affects Version/s: | 5.4.3 |
| Fix Version/s: | 5.4.6 |
| Type: | Improvement | Priority: | Major |
| Reporter: | Richard Gange | Assignee: | Oanh Thai Hoang |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | forms, support, ux | ||
| Remaining Estimate: | 0d | ||
| Time Spent: | 7d 2.75h | ||
| Original Estimate: | 5d | ||
| Attachments: |
|
||||||||
| Issue Links: |
|
||||||||
| Template: |
|
||||||||
| 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)
|
||||||||
| Release notes required: |
Yes
|
||||||||
| Documentation update required: |
Yes
|
||||||||
| Date of First Response: | |||||||||
| Sprint: | Saigon 35 | ||||||||
| Story Points: | 8 | ||||||||
| Description |
|
When we change this, care should be taken that values in all options remain available while the user mainly switches between them. The values for all other options but the selected one should only be cleared once you save the form. Not all systems behave this way, but most do - it's commonly seen as good and expected behavior. So, I fill in a value for option 1, switch to option 2, fill in another value there, then switch back to option 1. My initial value I've set in option 1 should still be there. Once I save the form, only the value for option 1 is stored. The value for option 2 is discarded. Update: This ticket just support for basic Switchable field using SwitchableTransformer, not providing any delegating transformer for the switchable field for now. |
| Comments |
| Comment by Oanh Thai Hoang [ 22/Jan/16 ] |
|
Hi mgeljic], Why do all value of all options still show after clicking save ?
Approach:
@Override
public void writeToItem(PropertysetItem newValues) {
// Alter newsValues to clear all data that not belong to selections
String currentSelection = (String) newValues.getItemProperty(propertyPrefix).getValue();
// Get iterator.
Iterator<?> propertyNames = newValues.getItemPropertyIds().iterator();
while (propertyNames.hasNext()) {
String propertyName = (String) propertyNames.next();
String compositePropertyName = getCompositePropertyName(propertyName);
if (!propertyName.equalsIgnoreCase(currentSelection) && !propertyName.equalsIgnoreCase(propertyPrefix)){
relatedFormItem.removeItemProperty(compositePropertyName);
} else{
if (newValues.getItemProperty(propertyName) != null) {
relatedFormItem.addItemProperty(compositePropertyName, newValues.getItemProperty(propertyName));
// Need to remove it from the removedProperties map
((JcrNodeAdapter)relatedFormItem).unRemoveItemProperty(compositePropertyName);
}
}
}
}
/**
* Remove a property from a removedProperties map.
* If the property was already remove, remove it for the removedProperties Map.
*/
public boolean unRemoveItemProperty(Object id) {
boolean res = false;
if (getRemovedProperties().containsKey(id)) {
getRemovedProperties().remove(id);
res = true;
}
return res;
}
Disadvantage: just support for default transformer SwitchableTransformer Git patch: https://gist.github.com/oanhthai/ba7614a110b865b073ff Opinion: |
| Comment by Oanh Thai Hoang [ 25/Jan/16 ] |
|
I have been done this ticket and created PR. This PR is the fix for SwitchableTransformer but doesn't include delegating transformers classes. |
| Comment by Mikaël Geljić [ 26/Jan/16 ] |
|
We've been keeping those switchable values historically, also because there was no reason to wipe them out; besides, if user changed his mind at some point, he could easily switch back with pre-filled value (Although this is more a duty for versioning). It's unclear to me if users trust this at the moment, but we can easily document the change. Now that we ignore validation for invisible fields, it means one can save invalid values into those. To some extent, it's a similar question as Thanks oanh.thai, I'll check it out—ideally delegating transformers should be covered as well, but I've observed inconsistent cases with validation too. |
| Comment by Mikaël Geljić [ 21/Mar/16 ] |
|
Definitely worth release notes, and maybe a note in Switchable field docu as well:
|
| Comment by Vincent Gombert [ 27/May/16 ] |
|
Hello, |
| Comment by Richard Gange [ 27/May/16 ] |
|
Hi vgombert- To answer your question, no. Can you expand on the use case? Are we talking about a manual switch? It's possible to store some predefined settings in configuration or a content app or something and then reference the settings using a name/id. Basically you link to the settings which are in the form of a set or record. HTH |
| Comment by Vincent Gombert [ 31/May/16 ] |
|
Hi Richard, A simple use case is a link with different origins, let's say internal or external to keep it simple (actually there are more). The switchable field allows us to save properties for several origins and later change the origin without having to type again properties for other origin. If I understand this JIRA well, that won't be possible anymore in 5.4.6. Regards, |
| Comment by Mikaël Geljić [ 02/Jun/16 ] |
|
Hi vgombert, With the plain SwitchableTransformer, indeed this is no longer possible (I was never too fond of this change either, but here's the tip for now: if you use the DelegatingCompositeFieldTransformer it will keep working, as the parent field has no control over property changes within its sub-fields). Cheers, |