Vojtech Szocs has posted comments on this change. Change subject: core, webadmin: Dismissible events ......................................................................
Patch Set 3: Code-Review+1 (4 comments) Nice patch overall, some inline comments for your consideration. https://gerrit.ovirt.org/#/c/41382/3/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendEventsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendEventsResource.java: Line 43: Line 44: @Override Line 45: public Response undelete(Action action) { Line 46: return performAction(VdcActionType.ClearDismissedAuditLogsBySeverity, Line 47: new ClearDismissedAuditLogsBySeverityParameters(Arrays.asList( If we want to List-ify all AuditLogSeverity enum members, we can also do: Arrays.asList(AuditLogSeverity.values()) (If the order of enum members must be as below, please ignore my comment.) Line 48: AuditLogSeverity.ERROR, Line 49: AuditLogSeverity.WARNING, Line 50: AuditLogSeverity.NORMAL, Line 51: AuditLogSeverity.ALERT https://gerrit.ovirt.org/#/c/41382/3/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/events/EventListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/events/EventListModel.java: Line 222 Line 223 Line 224 Line 225 Line 226 +1 on removing this complicated method. Line 202: } Line 203: } Line 204: Line 205: public void dismissEvent() { Line 206: AuditLog auditLog = getSelectedItem(); To be on the safe side, I'd add a null check here, something like: if (auditLog == null) { return; } Line 207: RemoveAuditLogByIdParameters params = new RemoveAuditLogByIdParameters(auditLog.getAuditLogId()); Line 208: Frontend.getInstance().runAction(VdcActionType.RemoveAuditLogById, params, new IFrontendActionAsyncCallback() { Line 209: @Override public void executed(FrontendActionAsyncResult result) { Line 210: EventListModel.this.refresh(); Line 213: } Line 214: Line 215: public void clearAllDismissedEvents() { Line 216: ClearDismissedAuditLogsBySeverityParameters params = new ClearDismissedAuditLogsBySeverityParameters( Line 217: Arrays.asList( IIUC, AuditLogSeverity groups two logical entities - events with: * NORMAL * WARNING * ERROR and alerts with: * ALERT Why not introduce some method to differentiate between these two logical entities? For example: public enum AuditLogSeverity implements Identifiable { ... public boolean isAlert() { ... }; ... } If we had that, we could avoid hard-coding enum members (like below). Line 218: AuditLogSeverity.ERROR, Line 219: AuditLogSeverity.WARNING, Line 220: AuditLogSeverity.NORMAL Line 221: )); -- To view, visit https://gerrit.ovirt.org/41382 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ibae6686f24dcf6afc1fb0db48d803176dd318d88 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Jakub Niedermertl <jnied...@redhat.com> Gerrit-Reviewer: Einav Cohen <eco...@redhat.com> Gerrit-Reviewer: Eli Mesika <emes...@redhat.com> Gerrit-Reviewer: Jakub Niedermertl <jnied...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Ravi Nori <rn...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
