Martin Mucha has posted comments on this change. Change subject: webadmin: gui for adding macPool permissions. ......................................................................
Patch Set 14: (5 comments) 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 st I don't know role of this class. But getter of this field is accessed (from ...Provider). So is one for "systemPermissionListModel". If we're about to remove "macPoolPermissionsListModel" we (probably) have to replace it with permissionListModel (not present here) and not with systemPermissionListModel (which is present). There's some fun going on in #configure() (see mine todo), but even if we remove this field from this class, we have to keep singleton of this class in ...Provider class, since .getModel() is accessed repeatedly, so there has to be singleton defined somewhere. Update: when I do just described change, app won't even start up. 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/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. yes. See MacPoolPermissionsListModel for comment. Why to allow adding roles which aren't meaningfull in given context. 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. yes. See MacPoolPermissionsListModel for comment. Why to allow adding roles which aren't meaningfull in given context. 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/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 Done 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 exist ah,great. So when I started I've found this in SystemPermissionView, used it and made a note to create component or some util out of it, but such component already exist. Sadly did not found that already existing component, so SystemPermissionView and others are just duplicate uncleaned mess. will update (this class only) 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
