Alona Kaplan has posted comments on this change. Change subject: webadmin: Aggregate error messages in Frontend.runMultipleActions() ......................................................................
Patch Set 2: (4 comments) http://gerrit.ovirt.org/#/c/25837/2/frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/Frontend.java File frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/Frontend.java: Line 744: final boolean aggregateErrors, Line 745: final List<String> errorMessages) { Line 746: if (actionTypes.isEmpty() || parameters.isEmpty() || callbacks.isEmpty()) { Line 747: if (aggregateErrors && errorMessages != null && !errorMessages.isEmpty()) { Line 748: failureEventHandler(null, errorMessages); Each returnValue has a description. IMO you should show it. The format of the messages should be similar to- FrontendEventsHandlerImpl.runMultipleActionFailed Line 749: } Line 750: return; Line 751: } Line 752: Line 764: actionTypes.remove(0); Line 765: parameters.remove(0); Line 766: callbacks.remove(0); Line 767: if (aggregateErrors && returnValue != null && !returnValue.getSucceeded()) { Line 768: errorMessages.addAll(returnValue.getCanDoActionMessages()); 1. What about returnValue.getFault()? There are some cases handleActionResult(...) gets the error from returnValue.getFault() 2. The message shouldn't be shown if getEventsHandler().isRaiseErrorModalPanel(...) is false. 3. In my opinion, the aggregation code should be somehow part of handleActionResult(...) and not here. This will also solve points 1, 2. Line 769: } Line 770: runMultipleActions(actionTypes, Line 771: parameters, Line 772: callbacks, Line 971: && (getEventsHandler().isRaiseErrorModalPanel(actionType, result.getFault()))) { Line 972: if (result.getCanDoActionMessages().size() <= 1) { Line 973: String errorMessage = !result.getCanDoAction() || !result.getCanDoActionMessages().isEmpty() Line 974: ? getRunActionErrorMessage(result.getCanDoActionMessages()) : result.getFault().getMessage(); Line 975: Not in the scope of your patch. But- error message should be localized Line 976: failureEventHandler(result.getDescription(), errorMessage); Line 977: } else { Line 978: failureEventHandler(result.getDescription(), result.getCanDoActionMessages()); Line 979: } Line 973: String errorMessage = !result.getCanDoAction() || !result.getCanDoActionMessages().isEmpty() Line 974: ? getRunActionErrorMessage(result.getCanDoActionMessages()) : result.getFault().getMessage(); Line 975: Line 976: failureEventHandler(result.getDescription(), errorMessage); Line 977: } else { It is not in the scope of your patch. But if you"ll move the aggregation code to here as I recommended, it can help you- failureEventHandler(..) and runActionExecutionFailed(...) use different event handlers. IMO it should be somehow merged. Line 978: failureEventHandler(result.getDescription(), result.getCanDoActionMessages()); Line 979: } Line 980: } Line 981: } -- To view, visit http://gerrit.ovirt.org/25837 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0236b7d9316dff3d9bd22f2127318eefc37bbda1 Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Alexander Wels <aw...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
