[MGNLUI-3749] Regression: Error flash on user/group/role creation Created: 15/Jan/16 Updated: 15/Apr/16 Resolved: 14/Apr/16 |
|
| Status: | Closed |
| Project: | Magnolia UI |
| Component/s: | None |
| Affects Version/s: | None |
| Fix Version/s: | 5.4.6 |
| Type: | Bug | Priority: | Major |
| Reporter: | Milan Divilek | Assignee: | Hieu Nguyen Duc |
| Resolution: | Fixed | Votes: | 0 |
| Labels: | support | ||
| Remaining Estimate: | 0d | ||
| Time Spent: | 9d | ||
| Original Estimate: | 5d | ||
| 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)
|
||||||||||||||||
| Bug DoR: |
[ ]*
Steps to reproduce, expected, and actual results filled
[ ]*
Affected version filled
|
||||||||||||||||
| Date of First Response: | |||||||||||||||||
| Sprint: | Saigon 39 | ||||||||||||||||
| Story Points: | 5 | ||||||||||||||||
| Description |
|
This issue was already once fixed in |
| Comments |
| Comment by Hieu Nguyen Duc [ 29/Mar/16 ] |
|
1) Problem: When "commit" button is clicked, it fires an action to validate fields, save user and close dialog on success. Unfortunately it calls isValid again for validation before setting button to enabled. It causes an error message to show up at the moment the dialog is closed. listener.onActionFired(name, new HashMap<String, Object>()); if (!BaseDialog.CANCEL_ACTION_NAME.equals(name) && listener instanceof EditorValidator && !((EditorValidator) listener).isValid()) { button.setEnabled(true); } 2) Solution: Remove the validation from that condition clause. This means no matter whether "commit" action passes validation, the button is always set to enabled. if (!BaseDialog.CANCEL_ACTION_NAME.equals(name)) { button.setEnabled(true); } |
| Comment by Sang Ngo Huu [ 29/Mar/16 ] |
|
hieu.nguyen It is not simple that we just remove the code, please check |
| Comment by Hieu Nguyen Duc [ 31/Mar/16 ] |
|
1) The old solution is not good because it calls button.setEnabled() in parallel with the operation onActionFired(). It means the button could be enabled before saving operation finishes. This becomes wrong if the action takes much more time. 2) Another potential workaround needs to add an extra method onComplete() to the interface EditorCallback. The main purpose is to pass isValid out to the caller (OpenCreateDialogAction). public interface EditorCallback { ... void onComplete(String actionName, boolean isValid); } After the action finishes, EventBus will fire an event for DefaultEditorActionRenderer to catch up and handle it. This way we can check isValid and set button to enabled. In OpenCreateDialogAction @Override public void onComplete(String actionName, boolean isValid) { eventBus.fireEvent(new OperationEvent(isValid)); } In DefaultEditorActionRenderer eventBus.addHandler(OperationEvent.class, new OperationEventHandler() { @Override public void operationCompleted(boolean isValid) { if (!isValid) { button.setEnabled(true); } } }); 3) Downside: It breaks the API. We have to implement onComplete() everywhere we implement EditorCallback. A quickly solution for this downside is to create AbstractEditorCallback that implements EditorCallback for users not have to implement onComplete(). |
| Comment by Hieu Nguyen Duc [ 04/Apr/16 ] |
|
New potential solution: In info.magnolia.ui.vaadin.form.Form, remove the logic of counting errors from isValid(). Move it into showValidation(). This way isValid() won't change shared state errorAmount but instead returns only true/false, and consequently won't display error flash.
@Override
public void showValidation(boolean isVisible) {
isValidationVisible = isVisible;
if(isVisible) {
getState().errorAmount = countErrors();
}
final Iterator<Component> it = tabSheet.iterator();
while (it.hasNext()) {
final Component c = it.next();
if (c instanceof MagnoliaFormTab) {
((MagnoliaFormTab) c).setValidationVisible(isVisible);
}
}
}
countErrors counts the number of invalid fields. |
| Comment by Mikaël Geljić [ 06/Apr/16 ] |
|
Thanks hieu.nguyen, we've finally been looking into this with ilgun: Your first assumption was correct: DefaultEditorActionRenderer validates a second time after action is performed and session is saved.
That said, it still doesn't solve the issue entirely: there are two more occurrences of validation happening after the request, action and renderer have been processed (i.e. after session save):
Let's check the potential options we have there too (cc apchelintcev) Re: changing the errorAmount, I haven't gone through this proposal yet, maybe you have ilgun? |
| Comment by Mikaël Geljić [ 06/Apr/16 ] |
|
Summarizing our thoughts here for now:
Meanwhile, we may bring other ideas which would not affect the actions; while on mid-term we would like to abstract the validation handling away from the actions |
| Comment by Ilgun Ilgun [ 12/Apr/16 ] |
|
hieu.nguyen |
| Comment by Hieu Nguyen Duc [ 13/Apr/16 ] |
|
ilgun I prefer the latest one because it covers more cases. You can decline the old one. |