[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:
duplicate
is duplicated by MGNLUI-3786 Action commit displays error message ... Closed
relation
is related to MGNLUI-2684 Error flash on user/group/role creation Closed
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 MGNLUI-2684. See description there for details.



 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 MGNLUI-2701

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):

  • DefaultEditorActionRenderer
    • #isValid? => validate (1)
    • checks validity for the sake of re-enabling commit button
  • Fire action (e.g. SaveFormAction)
    • #showValidation (only UI state changes)
    • #isValid? => validate (2)
    • proper validation upon save (as in many actions)
  • AbstractTextField#beforeClientResponse
    • All AbstractComponents check their error messages (component errors)
    • All AbstractFields additionally check their error messages from validation—if vaadin validation is visible
    • #getErrorMessages => validate (3)
  • FormSection#beforeClientResponse
    • All AbstractComponents check their error messages (component errors)
    • delegates #getErrorMessage to all child components—if our custom validation is visible
    • => validate (4)

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:

  • #3 could be prevented by using AbstractField#setValidationVisible in our Form (in addition to our custom state)
  • both #3 and #4 could be done by rewriting the validation sequence in many actions => if (isValid?) first, then showValidation(!isValid)
  • #1 can be done before the triggering the action, as a minimal first step; otherwise unclear how we would remove the isValid call from here atm, maybe checking validation visibility instead?

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
There are currently two PR's open for the ticket, which one do we suppose to take into account or just judge both of them ?

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.

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