Lior Vernia has posted comments on this change. Change subject: webadmin: gui for adding macPool permissions. ......................................................................
Patch Set 14: (11 comments) http://gerrit.ovirt.org/#/c/32211/14/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/dataprovider/AsyncDataProvider.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/dataprovider/AsyncDataProvider.java: Line 3284: } Line 3285: else if (entity instanceof CpuProfile) Line 3286: { Line 3287: return ((CpuProfile) entity).getId(); Line 3288: } else if (entity instanceof MacPool) { This change isn't required if you rebase this patch on top of the succeeding one refactoring this method. Line 3289: return ((MacPool) entity).getId(); Line 3290: } Line 3291: return Guid.Empty; Line 3292: } http://gerrit.ovirt.org/#/c/32211/14/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/help/HelpTag.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/help/HelpTag.java: Line 276: Line 277: remove_shared_mac_pools("remove_shared_mac_pools", HelpTagType.WEBADMIN, "Configure > MAC Address Pools > Remove"), //$NON-NLS-1$ $NON-NLS-2$ Line 278: Line 279: //TODO MM: is this correct comment? Line 280: remove_shared_mac_pool_permissions("remove_shared_mac_pool_permissions", HelpTagType.WEBADMIN, "Configure > MAC Address Pools > Remove"), //$NON-NLS-1$ $NON-NLS-2$ Now that you're using PermissionListModel, this can be removed (that's okay, because the usual remove_permission tag is good enough). Line 281: Line 282: new_domain("new_domain", HelpTagType.WEBADMIN, "Storage Tab > New Domain"), //$NON-NLS-1$ //$NON-NLS-2$ Line 283: Line 284: new_external_subnet("new_external_subnet", HelpTagType.WEBADMIN, "Networks main tab -> External Subnet sub tab -> New"), //$NON-NLS-1$ //$NON-NLS-2$ http://gerrit.ovirt.org/#/c/32211/14/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/CommonModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/CommonModel.java: Line 84: private SystemPermissionListModel systemPermissionListModel; Line 85: private ClusterPolicyListModel clusterPolicyListModel; Line 86: private InstanceTypeListModel instanceTypeListModel; Line 87: private SharedMacPoolListModel sharedMacPoolListModel; Line 88: private MacPoolPermissionsListModel macPoolPermissionsListModel; I don't think this (nor any other addition in this file) is required for stuff to work. Subtab model changes are handled by the corresponding main tab in the configure dialog. Line 89: Line 90: // NOTE: when adding a new ListModel here, be sure to add it to the list in initItems() Line 91: private ListWithDetailsAndReportsModel dataCenterList; Line 92: private ClusterListModel clusterList; http://gerrit.ovirt.org/#/c/32211/14/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/macpool/MacPoolPermissionsListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/macpool/MacPoolPermissionsListModel.java: Line 3: import org.ovirt.engine.ui.uicommonweb.models.configure.PermissionListModel; Line 4: import org.ovirt.engine.ui.uicommonweb.models.users.AdElementListModel; Line 5: import org.ovirt.engine.ui.uicommonweb.models.users.AdElementListModelForMacPoolRoles; Line 6: Line 7: public class MacPoolPermissionsListModel extends PermissionListModel { What's the incentive for this class and related classes? To only show roles relevant to MAC pools when adding/editing a permission? That's cool, but since all other entities don't filter permissions - I don't see the point in doing this here. If filtering is wanted, it should be done in an infrastructural fashion - maybe by adding a parameter to the GetAllRolesQuery. However, I'm not aware of such an RFE. I suggest for now to remove this code and use the default PermissionListModel - it would save a ton of code in this patch. Line 8: Line 9: @Override Line 10: protected AdElementListModel createAdElementListModel() { Line 11: return new AdElementListModelForMacPoolRoles(); http://gerrit.ovirt.org/#/c/32211/14/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/macpool/SharedMacPoolListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/macpool/SharedMacPoolListModel.java: Line 122: setConfirmWindow(null); Line 123: } Line 124: Line 125: private void onRemove() { Line 126: setConfirmWindow(null); Please revert this change - it's customary in many Model classes to have cancel() perform common commands to closing the dialog. Line 127: ArrayList<VdcActionParametersBase> params = new ArrayList<VdcActionParametersBase>(); Line 128: for (MacPool macPool : (Iterable<MacPool>) getSelectedItems()) { Line 129: params.add(new RemoveMacPoolByIdParameters(macPool.getId())); Line 130: } http://gerrit.ovirt.org/#/c/32211/14/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/users/AdElementListModel.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/users/AdElementListModel.java: Line 209: Line 210: queryRoleList(); Line 211: } Line 212: Line 213: protected void queryRoleList() { If you remove the subclass, this change isn't needed. Line 214: AsyncDataProvider.getInstance().getRoleList(new AsyncQuery(this, new INewAsyncCallback() { Line 215: Line 216: @Override Line 217: public void onSuccess(Object model, Object result) { http://gerrit.ovirt.org/#/c/32211/14/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/users/AdElementListModelForMacPoolRoles.java File frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/users/AdElementListModelForMacPoolRoles.java: Line 10: import org.ovirt.engine.ui.frontend.Frontend; Line 11: import org.ovirt.engine.ui.frontend.IAsyncConverter; Line 12: import org.ovirt.engine.ui.frontend.INewAsyncCallback; Line 13: Line 14: public class AdElementListModelForMacPoolRoles extends AdElementListModel { As I stated in another file, this class isn't needed. Line 15: @Override Line 16: protected void queryRoleList() { Line 17: Frontend.getInstance().runQuery(VdcQueryType.GetAllMacPoolRoles, Line 18: new MultilevelAdministrationsQueriesParameters(), http://gerrit.ovirt.org/#/c/32211/14/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 67: @DefaultStringValue("Clusters") Line 68: String clustersTitle(); Line 69: Line 70: @DefaultStringValue("MAC Pool Permissions") Line 71: String macPoolPermissionsTitle(); Is this used anywhere? Likely leftover from MacPoolPermissionListModel. Line 72: Line 73: @DefaultStringValue("Profiles") Line 74: String networkProfilesTitle(); Line 75: Line 203: @DefaultStringValue("Remove MAC Address Pool(s)") Line 204: String removeSharedMacPoolsTitle(); Line 205: Line 206: @DefaultStringValue("Remove MAC Address Pool Permission(s)") Line 207: String removeSharedMacPoolPermissions(); Same. Line 208: Line 209: @DefaultStringValue("Logical Networks") Line 210: String logicalNetworksTitle(); Line 211: http://gerrit.ovirt.org/#/c/32211/14/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/popup/configure/ConfigurePopupPresenterWidget.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/presenter/popup/configure/ConfigurePopupPresenterWidget.java: Line 30: private ClusterPolicyModelProvider clusterPolicyModelProvider; Line 31: private ClusterPolicyClusterModelProvider clusterPolicyClusterModelProvider; Line 32: private InstanceTypeModelProvider instanceTypeModelProvider; Line 33: private SharedMacPoolModelProvider sharedMacPoolModelProvider; Line 34: private MacPoolPermissionModelProvider macPoolPermissionModelProvider; This isn't required in this file - the fetching of relevant permissions for a specific MAC pool should be performed from SharedMacPoolListModel.syncSearch() or something similar. Line 35: Line 36: @Inject Line 37: public ConfigurePopupPresenterWidget(EventBus eventBus, ViewDef view, Line 38: RoleModelProvider roleModelProvider, http://gerrit.ovirt.org/#/c/32211/14/frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/macpool/SharedMacPoolView.java File frontend/webadmin/modules/webadmin/src/main/java/org/ovirt/engine/ui/webadmin/section/main/view/popup/macpool/SharedMacPoolView.java: Line 156: } Line 157: Line 158: private SimpleActionTable<Permissions> createAuthorizationTable() { Line 159: Line 160: SimpleActionTable<Permissions> permissionsTable = new SimpleActionTable<Permissions>(macPoolPermissionModelProvider, Haven't reviewed this file thoroughly, but maybe here you can use the existing PermissionListModelTable? Line 161: headerlessResources, Line 162: tableResources, Line 163: eventBus, Line 164: clientStorage); -- To view, visit http://gerrit.ovirt.org/32211 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Idb6b442e7eebce367b9008f4b9c1df9ade84745f Gerrit-PatchSet: 14 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: Alona Kaplan <alkap...@redhat.com> Gerrit-Reviewer: Lior Vernia <lver...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@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
