Tomas Jelinek has posted comments on this change. Change subject: userportal\webadmin: Editing of template version for pools ......................................................................
Patch Set 5: (8 comments) https://gerrit.ovirt.org/#/c/36510/5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/pools/PoolListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/pools/PoolListModel.java: Line 227: Line 228: new AsyncQuery(this, new INewAsyncCallback() { Line 229: @Override Line 230: public void onSuccess(Object modell, Object result) { Line 231: final VM vm = (VM) ((VdcQueryReturnValue) result).getReturnValue(); can the VM be null? The previous code expected it to be null and there are checks handling the situation when it is. Here you first expect it is not (e.g. call to vm.isNextRunConfigurationExists()) but than in initExistingPoolModel() you expect it to be null. So please make sure how is it - if it can be null please handle it here properly. If not, please remove the null handling from initExistingPoolModel(). Line 232: if (vm.isNextRunConfigurationExists()) { Line 233: Frontend.getInstance().runQuery(VdcQueryType.GetVmNextRunConfiguration, Line 234: new IdQueryParameters(vm.getId()), Line 235: new AsyncQuery(this, new INewAsyncCallback() { Line 228: new AsyncQuery(this, new INewAsyncCallback() { Line 229: @Override Line 230: public void onSuccess(Object modell, Object result) { Line 231: final VM vm = (VM) ((VdcQueryReturnValue) result).getReturnValue(); Line 232: if (vm.isNextRunConfigurationExists()) { please break this logic down to specific methods - e.g. as a body of the if it would be good to have a well named method. Now it is quite hard to read. Line 233: Frontend.getInstance().runQuery(VdcQueryType.GetVmNextRunConfiguration, Line 234: new IdQueryParameters(vm.getId()), Line 235: new AsyncQuery(this, new INewAsyncCallback() { Line 236: @Override Line 236: @Override Line 237: public void onSuccess(Object a, Object result) { Line 238: final VM nextRunVm = Line 239: (VM) ((VdcQueryReturnValue) result).getReturnValue(); Line 240: if (nextRunVm!=null) { // template version was changed -> load data from template formatter - please add spaces around "!=". Line 241: vm.setVmtGuid(nextRunVm.getVmtGuid()); Line 242: vm.setUseLatestVersion(nextRunVm.isUseLatestVersion()); Line 243: } Line 244: if (vm.isUseLatestVersion()) { // next run, latest template Line 246: new IdQueryParameters(vm.getVmtGuid()), Line 247: new AsyncQuery(this, new INewAsyncCallback() { Line 248: @Override Line 249: public void onSuccess(Object a, Object result) { Line 250: if (result != null && ((VdcQueryReturnValue)result).getReturnValue() != null) { this is copy pasted from the "if (vm.isUseLatestVersion()) " - if it will be broken up to methods it will be simple to re-use Line 251: VmTemplate vmt = (((VdcQueryReturnValue)result).getReturnValue()); Line 252: vm.setVmtGuid(vmt.getId()); Line 253: initExistingPoolModel(vm, true, pool); Line 254: } https://gerrit.ovirt.org/#/c/36510/5/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ExistingPoolModelBehavior.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/ExistingPoolModelBehavior.java: Line 32: private final VM pool; Line 33: Line 34: private InstanceTypeManager instanceTypeManager; Line 35: Line 36: boolean templateVersionWasChanged; private Line 37: boolean forceTemplateData; // indicates that the VM object passed is waiting for next-run upgrade and raw template data should be used instead Line 38: Line 39: public ExistingPoolModelBehavior(VM pool, boolean forceTemplateData) { Line 40: this.pool = pool; Line 33: Line 34: private InstanceTypeManager instanceTypeManager; Line 35: Line 36: boolean templateVersionWasChanged; Line 37: boolean forceTemplateData; // indicates that the VM object passed is waiting for next-run upgrade and raw template data should be used instead - please make private - please make the comment as a javadoc instead Line 38: Line 39: public ExistingPoolModelBehavior(VM pool, boolean forceTemplateData) { Line 40: this.pool = pool; Line 41: this.forceTemplateData = forceTemplateData; Line 100: if (!forceTemplateData) { Line 101: instanceTypeManager.updateAll(); Line 102: } else { Line 103: List<InstanceType> instanceTypeOptions=new ArrayList<InstanceType>(); Line 104: instanceTypeOptions.add(CustomInstanceType.INSTANCE); why are you adding the custom instance type like this? Line 105: getModel().getInstanceTypes().setItems(instanceTypeOptions); Line 106: getModel().getInstanceTypes().setSelectedItem(instanceTypeOptions.get(0)); Line 107: getModel().stopProgress(); Line 108: } Line 124: updateRngDevice(template.getId()); Line 125: getModel().getCustomPropertySheet().deserialize(template.getCustomProperties()); Line 126: Line 127: // check if template-version selected requires to manually load the model instead of using the InstanceTypeManager Line 128: if ((!pool.getVmtGuid().equals(template.getId()) && !pool.isUseLatestVersion()) // template ID changed but not latest as it would cause false-positives please extract this things to named boolean variables and if needed write the comments above the variables. This way it is very hard to read... Line 129: || pool.isUseLatestVersion() != (template instanceof LatestVmTemplate) // latest property was changed Line 130: || (templateVersionWasChanged && forceTemplateData)) { // template data is used - always load manually (excluding initialization) Line 131: if (!templateVersionWasChanged && !forceTemplateData) { Line 132: deactivateInstanceTypeManager(new InstanceTypeManager.ActivatedListener() { -- To view, visit https://gerrit.ovirt.org/36510 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I53686dae694f6ff826bdbeaada1e28fc55fd8d30 Gerrit-PatchSet: 5 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Eldan Shachar <eshac...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Eldan Shachar <eshac...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@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
