Lior Vernia has posted comments on this change. Change subject: webadmin: Add managemenet network field to new/edit cluster dialog ......................................................................
Patch Set 21: (5 comments) http://gerrit.ovirt.org/#/c/37141/21/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/clusters/ClusterModel.java: Line 1364: clusterModel.getDataCenter().setIsChangable(selectedDataCenter == null); Line 1365: Line 1366: clusterModel.setMigrateOnErrorOption(clusterModel.getEntity().getMigrateOnError()); Line 1367: Line 1368: if (getIsEdit() && dataCenterId != null) { There's some code duplication in this class. This actually duplicates the logic setting whether the management network is disabled - so I would call !getManagementNetwork().getIsChangable() instead. Line 1369: loadCurrentClusteManagementNetwork(); Line 1370: } Line 1371: } Line 1372: }; Line 1372: }; Line 1373: AsyncDataProvider.getInstance().getDataCenterList(_asyncQuery); Line 1374: } Line 1375: Line 1376: private void loadCurrentClusteManagementNetwork() { Typo. Line 1377: final AsyncQuery getManagementNetworkQuery = new AsyncQuery(this, new INewAsyncCallback() { Line 1378: @Override Line 1379: public void onSuccess(Object model, Object returnValue) { Line 1380: final ClusterModel clusterModel = (ClusterModel) model; Line 1403: new AsyncQuery(clusterModel, new INewAsyncCallback() { Line 1404: @Override Line 1405: public void onSuccess(Object model, Object returnValue) { Line 1406: Network defaultManagementNetwork = (Network) returnValue; Line 1407: defaultManagementNetworkCache.put(dataCenterId, defaultManagementNetwork); If you want to make sure that what you set as the selected item is part of the fetched networks (and not what getDefaultManageNetworkQuery returns), then I would perform the iteration here, when putting the entry in the first place. Line 1408: selectDefaultManagemmentNetwork(dcNetworks, defaultManagementNetwork); Line 1409: } Line 1410: }); Line 1411: AsyncDataProvider.getInstance() Line 1415: }); Line 1416: AsyncDataProvider.getInstance().getAllDataCenterNetworks(getAllDataCenterNetworksQuery, dataCenterId); Line 1417: } Line 1418: Line 1419: private void selectDefaultManagemmentNetwork(List<Network> dcNetworks, Network defaultManagementNetwork) { 1. Typo. 2. As mentioned above - I don't think the iteration is needed here. 3. With the above changes, the method could be without parameters... It could fetch the management network from the map by the dcId member, then if it's not null setSelectedItem() accordingly. Line 1420: if (defaultManagementNetwork != null) { Line 1421: for (Network network : dcNetworks) { Line 1422: if (network.getId().equals(defaultManagementNetwork.getId())) { Line 1423: getManagementNetwork().setSelectedItem(network); Line 1834: } Line 1835: }; Line 1836: AsyncDataProvider.getInstance().getDataCenterVersions(_asyncQuery, selectedDataCenter.getId()); Line 1837: Line 1838: if (!getIsEdit() || isClusterDetached()) { Similarly, this is called iff the management network field is enable - so I would call getManagementNetwork().getIsChangable() instead. This will make it much easier to understand that loadCurrentClusterManagementNetwork() and loadDcNetworks() are mutually exclusive. Line 1839: loadDcNetworks(selectedDataCenter.getId()); Line 1840: } Line 1841: } Line 1842: -- To view, visit http://gerrit.ovirt.org/37141 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I055babd6037f127235c349499f1545396e38333f Gerrit-PatchSet: 21 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Yevgeny Zaspitsky <yzasp...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Oved Ourfali <oourf...@redhat.com> Gerrit-Reviewer: Yevgeny Zaspitsky <yzasp...@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
