Shubhendu Tripathi has posted comments on this change. Change subject: webadmin: UI to set the volume snapshot configurations ......................................................................
Patch Set 10: (11 comments) http://gerrit.ovirt.org/#/c/36295/10/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/GlusterClusterSnapshotConfigModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/GlusterClusterSnapshotConfigModel.java: Line 74: } Line 75: Line 76: private void setAvailabilities() { Line 77: getClusters().setIsAvailable(true); Line 78: getClusterConfigOptions().setIsAvailable(true); > Setting these explicitly is not required since they default to true ok. will remove this. Line 79: } Line 80: Line 81: public boolean validate() { Line 82: boolean isValid = true; Line 127: // set the entity items Line 128: listModel.setItems(coll); Line 129: } Line 130: }), Line 131: getClusters().getSelectedItem().getId(), > possible NPE if default cluster is not set Its actually selected from a dropdown and chances are less. Anyway would handle the NPE in the beginning. Line 132: null); Line 133: } http://gerrit.ovirt.org/#/c/36295/10/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/GlusterVolumeSnapshotConfigModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/gluster/GlusterVolumeSnapshotConfigModel.java: Line 98: Line 99: private void setAvailabilities() { Line 100: getClusterName().setIsAvailable(true); Line 101: getVolumeName().setIsAvailable(true); Line 102: getConfigOptions().setIsAvailable(true); > same here done Line 103: } Line 104: Line 105: public boolean validate() { Line 106: boolean isValid = true; Line 118: Line 119: return isValid; Line 120: } Line 121: Line 122: private void populateConfigOptions() { > Use startProgress and stopProgress to avoid displaying an empty dialog done Line 123: AsyncDataProvider.getInstance().getGlusterSnapshotConfig(new AsyncQuery(this, new INewAsyncCallback() { Line 124: Line 125: @Override Line 126: public void onSuccess(Object model, Object returnValue) { Line 136: EntityModel<VolumeSnapshotOptionModel> cfgModel = new EntityModel<VolumeSnapshotOptionModel>(); Line 137: VolumeSnapshotOptionModel option = new VolumeSnapshotOptionModel(); Line 138: option.setOptionName(cfg.getParamName()); Line 139: option.setOptionValue(cfg.getParamValue()); Line 140: for (GlusterVolumeSnapshotConfig clusterCfg : clusterConfigOptions) { > you could form a map of Option->Value, so that looping every time can be av done Line 141: if (clusterCfg.getParamName().equals(cfg.getParamName())) { Line 142: option.setCorrespodingClusterValue(clusterCfg.getParamValue()); Line 143: break; Line 144: } http://gerrit.ovirt.org/#/c/36295/10/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/volumes/VolumeListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/volumes/VolumeListModel.java: Line 502: : asyncTask.getStatus() != JobExecutionStatus.STARTED; Line 503: } Line 504: } Line 505: Line 506: if (list.size() == 1) { > you could do done Line 507: GlusterVolumeEntity volumeEntity = list.get(0); Line 508: GlusterAsyncTask asyncTask = volumeEntity.getAsyncTask(); Line 509: allowStopRebalance = Line 510: volumeEntity.getStatus() == GlusterStatus.UP && asyncTask != null Line 1263: GlusterClusterSnapshotConfigModel clusterSnapshotConfigModel = (GlusterClusterSnapshotConfigModel) getWindow(); Line 1264: Line 1265: if (!clusterSnapshotConfigModel.validate()) { Line 1266: return; Line 1267: } > validation should be done in the above function itself done Line 1268: Line 1269: Guid clusterId = clusterSnapshotConfigModel.getClusters().getSelectedItem().getId(); Line 1270: List<GlusterVolumeSnapshotConfig> vdsParams = new ArrayList<GlusterVolumeSnapshotConfig>(); Line 1271: for (EntityModel<GlusterVolumeSnapshotConfig> clusterCfg : clusterSnapshotConfigModel.getClusterConfigOptions() Line 1368: Line 1369: public void onConfigureVolumeSnapshotOptions() { Line 1370: GlusterVolumeSnapshotConfigModel volumeSnapshotConfigModel = (GlusterVolumeSnapshotConfigModel) getWindow(); Line 1371: Line 1372: if (!volumeSnapshotConfigModel.validate()) { > same here done Line 1373: return; Line 1374: } Line 1375: Line 1376: GlusterVolumeEntity volumeEntity = volumeSnapshotConfigModel.getSelectedVolumeEntity(); http://gerrit.ovirt.org/#/c/36295/10/frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java File frontend/webadmin/modules/uicompat/src/main/java/org/ovirt/engine/ui/uicompat/UIConstants.java: Line 2516: Line 2517: @DefaultStringValue("Configuring volume snapshot options\n\n" Line 2518: + "Changing configuration parameters will lead to deletion of snapshots if they exceed the new limit.\n\n" Line 2519: + "Are you sure you want to continue?") Line 2520: String youAreAboutChangeSnapshotConfigurationMsg(); > This message might be misleading if the user has modified some configuratio I remember this message comes as is in gluster CLI as well. Anyway will confirm and see if some changes required. Also this check happens only if the value is changed. http://gerrit.ovirt.org/#/c/36295/10/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterClusterSnapshotConfigureOptionsPopupView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterClusterSnapshotConfigureOptionsPopupView.ui.xml: Line 22: padding-bottom: 5px; Line 23: padding-top: 30px; Line 24: } Line 25: Line 26: .messageLabel { > is this being used? I dont think so. Will remove this. Line 27: color: #FF0000; Line 28: left: 10px; Line 29: padding-left: 5px; Line 30: } http://gerrit.ovirt.org/#/c/36295/10/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotConfigureOptionsPopupView.ui.xml File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/gluster/GlusterVolumeSnapshotConfigureOptionsPopupView.ui.xml: Line 22: padding-bottom: 5px; Line 23: padding-top: 30px; Line 24: } Line 25: Line 26: .messageLabel { > same here done. will check and remove if not referred. Line 27: color: #FF0000; Line 28: left: 10px; Line 29: padding-left: 5px; Line 30: } -- To view, visit http://gerrit.ovirt.org/36295 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I08b5e592b818266106bc1891c3aa2f3ba3541d36 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: Kanagaraj M <kmayi...@redhat.com> Gerrit-Reviewer: Shubhendu Tripathi <shtri...@redhat.com> Gerrit-Reviewer: Vojtech Szocs <vsz...@redhat.com> Gerrit-Reviewer: anmolbabu <anb...@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
