Tomas Jelinek has posted comments on this change. Change subject: core,webadmin: show list of changed fields when updating running vm ......................................................................
Patch Set 3: (4 comments) http://gerrit.ovirt.org/#/c/33046/3/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/VmNextRunConfigurationWidget.java File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/VmNextRunConfigurationWidget.java: Line 90: @Override Line 91: public void edit(VmNextRunConfigurationModel object) { Line 92: driver.edit(object); Line 93: cpuPanel.setVisible(object.isCpuPluggable()); Line 94: changedFields.setHTML(SafeHtmlUtils.fromSafeConstant(object.getChangedFields())); not sure about this for this reasons: - using HTML with fromSafeConstant is risky unless you control the message directly like in localize() method here. If you take them like here from the DB, if any of the fields in the getChangedFields() would be a html special char (e.g. '<') the whole page would be broken. Not to mention that there is a (small) potential for an XSS if someone would be able to control the name of any of the fields here (not today, but can imagine in future). - would look nicer to have it visualized as list items. If the VmNextRunConfigurationModel.getChangedFields() was a list of strings, you could do something like this: SafeHtmlBuilder changedFields = new SafeHtmlBuilder(); for (String field: object.getChangedFields()) { SafeHtml escapedField = SafeHtmlUtils.htmlEscape(field); changedFields.append(templates.listItem(escapedField)); } changedFields.setHTML(changedFields.toSafeHtml()); Here the setHTML is safe because the fields are already escaped. Line 95: } Line 96: Line 97: @Override Line 98: public VmNextRunConfigurationModel flush() { http://gerrit.ovirt.org/#/c/33046/3/frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/VmNextRunConfigurationWidget.ui.xml File frontend/webadmin/modules/gwt-common/src/main/java/org/ovirt/engine/ui/common/widget/uicommon/popup/vm/VmNextRunConfigurationWidget.ui.xml: Line 4: xmlns:d="urn:import:org.ovirt.engine.ui.common.widget.dialog" xmlns:e="urn:import:org.ovirt.engine.ui.common.widget.editor" Line 5: xmlns:ge="urn:import:org.ovirt.engine.ui.common.widget.editor.generic"> Line 6: Line 7: <ui:style> Line 8: .verticalPanel>* { maybe you will not need this if you will render the fields using the list items as described in VmNextRunConfigurationWidget. Line 9: display: block; Line 10: clear: both; Line 11: } Line 12: .contentWidgets { http://gerrit.ovirt.org/#/c/33046/3/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/userportal/UserPortalListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/userportal/UserPortalListModel.java: Line 1162: VmNextRunConfigurationModel confirmModel = new VmNextRunConfigurationModel(); Line 1163: confirmModel.setTitle(ConstantsManager.getInstance().getConstants().editNextRunConfigurationTitle()); Line 1164: confirmModel.setHelpTag(HelpTag.edit_next_run_configuration); Line 1165: confirmModel.setHashName("edit_next_run_configuration"); //$NON-NLS-1$ Line 1166: confirmModel.setChangedFields(changedFields.toString()); I would not rely on the toString() method - please use StringUtils.join() instead. But even better would be to use the list and just render it differently (see the comment in VmNextRunConfigurationWidget) Line 1167: confirmModel.setCpuPluggable(selectedItem.getCpuPerSocket() == gettempVm().getCpuPerSocket() && Line 1168: selectedItem.getNumOfSockets() != gettempVm().getNumOfSockets()); Line 1169: confirmModel.getCommands().add(new UICommand("updateExistingVm", UserPortalListModel.this) //$NON-NLS-1$ Line 1170: .setTitle(ConstantsManager.getInstance().getConstants().ok()) http://gerrit.ovirt.org/#/c/33046/3/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmListModel.java: Line 2028: VmNextRunConfigurationModel confirmModel = new VmNextRunConfigurationModel(); Line 2029: confirmModel.setTitle(ConstantsManager.getInstance().getConstants().editNextRunConfigurationTitle()); Line 2030: confirmModel.setHelpTag(HelpTag.edit_next_run_configuration); Line 2031: confirmModel.setHashName("edit_next_run_configuration"); //$NON-NLS-1$ Line 2032: confirmModel.setChangedFields(changedFields.toString()); I would not rely on the toString() method - please use StringUtils.join() instead. But even better would be to use the list and just render it differently (see the comment in VmNextRunConfigurationWidget) Line 2033: confirmModel.setCpuPluggable(selectedItem.getCpuPerSocket() == getcurrentVm().getCpuPerSocket() && Line 2034: selectedItem.getNumOfSockets() != getcurrentVm().getNumOfSockets()); Line 2035: Line 2036: confirmModel.getCommands().add(new UICommand("updateExistingVm", VmListModel.this) //$NON-NLS-1$ -- To view, visit http://gerrit.ovirt.org/33046 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idf13a19d1b193037226cb49bc6c1e8cefc0553b8 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@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
