Martin Mucha has posted comments on this change. Change subject: webadmin: gui for adding macPool permissions. ......................................................................
Patch Set 14: (2 comments) 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 I don't see how we can parameterized GetAllRolesQuery to obtain this. Query is: "I need all roles for macPool manipulation" this is not something you can generalize for whole system, since context (here "for macpool" is too varying). It probably has to be special query. There isn't RFE probably because it's not (easily? sanely?) doable. Limiting this on gui part isn't answer either, since this should be checked on backend part as well. The question is like in another comment. This works and already exist. Why to remove it, when it's better (it is) just to be equally wrong as other code? This patch will be smaller, alright, but it will be worse as well, since adding unrelated permissions to macpools is nonsense. Also consider usability: user would have to know, that he needs to pick from LONG list specific role, which name he probably has to look up in documentation all read whole LONG list and give it a shot. This is really bad UX. I agree it'd make things easier, but this approach kinda defeats purpose of UI ~ it should serve user. 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/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. Done Line 72: Line 73: @DefaultStringValue("Profiles") Line 74: String networkProfilesTitle(); Line 75: -- 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
