You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cloudstack.apache.org by Ian Duffy <ia...@ianduffy.ie> on 2013/07/24 19:50:54 UTC
Review Request 12907: WIP: Add LDAP Account addition UI. Feedback and Help
wanted.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12907/
-----------------------------------------------------------
Review request for cloudstack, Abhinandan Prateek, Brian Federle, Jessica Wang, Pranav Saxena, and Sebastien Goasguen.
Repository: cloudstack-git
Description
-------
Note: JS files were formatted with same formatting that was applied to origin/master.
This is purely a work in progress. I'm submitting it as I'd like somebody to give *detailed* feedback/reviewal before I go much further.
The code works/does-what-I-want but I'm not sure if its "correct" and follows cloudstack UI practices.
I had to do some ugly css in order to get the display I wanted.
I'm currently having issues with fields marked as "required" for some reason the requirement doesn't seem to be enforced.
I'm not sure if a loader appears should the request to listAllLdapUsers be slow to respond.
I'm not sure how to add a if ldapEnabled display this view else display old view condition.
I'm not sure how to detail with cases where the user might not have firstname, lastname or email set in ldap.
I'm not sure what happens if listAllLdapUsers returns a massive list of users... will it load on scroll? Does paging need to be implemented API side? How is this done?
For testing purposes there is a ldap server included in this branch. You can launch it with:
mvn -pl :cloud-plugin-user-authenticator-ldap ldap:run
and then configure it at Global Settings -> ldap.basedn = dc=cloudstack,dc=org
Also global settings -> LDAP Configuration, hostname: localhost, port: 10389.
Diffs
-----
client/tomcatconf/commands.properties.in 9e14d0f
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapAddConfigurationCmd.java 62736b16087561a7e25893cd46115795100c609e
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccount.java PRE-CREATION
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapDeleteConfigurationCmd.java 329b91b
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListAllUsersCmd.java 087d156
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListConfigurationCmd.java 6707878
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapUserSearchCmd.java e6a40d0
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/response/LdapConfigurationResponse.java d583346
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/response/LdapUserResponse.java 40ba0ce
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapAuthenticator.java 2916202
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapConfiguration.java 8f31ce5
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapConfigurationVO.java d3ff820
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapContextFactory.java 30bdc5b
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManager.java c961d2c
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java be9b3d5
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUser.java 7c65e60
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManager.java 54802cf
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUtils.java 453dc0a
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapAddConfigurationCmdSpec.groovy ebade1e
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapConfigurationSpec.groovy 91c9baf
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapConfigurationVO.groovy 8135901
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapContextFactorySpec.groovy bb20fb3
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapDeleteConfigurationCmdSpec.groovy 664fd64
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapListAllUsersCmdSpec.groovy 30cd7cc
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapListConfigurationCmdSpec.groovy a7c1979
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapManagerImplSpec.groovy 5dfecb9
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapSearchUserCmdSpec.groovy d72878b
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapUserManagerSpec.groovy 489c250
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapUserResponseSpec.groovy 105203b
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapUserSpec.groovy 6131267
ui/css/cloudstack3.css 4545e96
ui/index.jsp 34f0c54
ui/scripts/accounts.js bad8435
ui/scripts/accountsWizard.js PRE-CREATION
ui/scripts/affinity.js a9c6695
ui/scripts/autoscaler.js 15a9dac
ui/scripts/cloud.core.callbacks.js 6eb7644
ui/scripts/cloudStack.js c0ff7f2
ui/scripts/configuration.js 8bc40d6
ui/scripts/dashboard.js e8ab6c5
ui/scripts/docs.js c8ef0d9
ui/scripts/domains.js 01f4236
ui/scripts/events.js bd50887
ui/scripts/globalSettings.js ac63015
ui/scripts/installWizard.js 46769fa
ui/scripts/instanceWizard.js ff130d3
ui/scripts/instances.js 9b27d93
ui/scripts/lbStickyPolicy.js c0e2bfa
ui/scripts/network.js 95a93bc
ui/scripts/plugins.js 3c5bc0f
ui/scripts/projects.js ea1e6db
ui/scripts/regions.js 4be600f
ui/scripts/sharedFunctions.js a9f833c
ui/scripts/storage.js ad0965a
ui/scripts/system.js 3038a8a
ui/scripts/templates.js dbb0083
ui/scripts/ui-custom/accountsWizard.js PRE-CREATION
ui/scripts/ui-custom/affinity.js 1a23ff7
ui/scripts/ui-custom/autoscaler.js 2f6ce38
ui/scripts/ui-custom/dashboard.js 6d92318
ui/scripts/ui-custom/enableStaticNAT.js 1b2bf7b
ui/scripts/ui-custom/granularSettings.js 02d5c1f
ui/scripts/ui-custom/healthCheck.js 4b42fa7
ui/scripts/ui-custom/installWizard.js c53a642
ui/scripts/ui-custom/instanceWizard.js 31b4baa
ui/scripts/ui-custom/ipRules.js 34b2398
ui/scripts/ui-custom/login.js 0dbbf82
ui/scripts/ui-custom/physicalResources.js 5173172
ui/scripts/ui-custom/pluginListing.js 3dcce98
ui/scripts/ui-custom/projectSelect.js aef49ed
ui/scripts/ui-custom/projects.js f1f9eba
ui/scripts/ui-custom/recurringSnapshots.js 985f369
ui/scripts/ui-custom/regions.js 9fc36f3
ui/scripts/ui-custom/securityRules.js 2e2c9ac
ui/scripts/ui-custom/uploadVolume.js 996d8ac
ui/scripts/ui-custom/vpc.js 4edccf1
ui/scripts/ui-custom/zoneChart.js 5d4e0c0
ui/scripts/ui-custom/zoneFilter.js 9e6a493
ui/scripts/ui-custom/zoneWizard.js 877dbc0
ui/scripts/ui/core.js 18c3363
ui/scripts/ui/dialog.js 7f82eea
ui/scripts/ui/events.js bd609d2
ui/scripts/ui/utils.js 39ef3e3
ui/scripts/ui/widgets/cloudBrowser.js 9a56bb3
ui/scripts/ui/widgets/dataTable.js 1b3ea82
ui/scripts/ui/widgets/detailView.js 0bccef5
ui/scripts/ui/widgets/listView.js bc68a72
ui/scripts/ui/widgets/multiEdit.js 08bd0bf
ui/scripts/ui/widgets/notifications.js 0299603
ui/scripts/ui/widgets/overlay.js ecf12e6
ui/scripts/ui/widgets/tagger.js 9af6fb7
ui/scripts/ui/widgets/toolTip.js 6967acc
ui/scripts/ui/widgets/treeView.js fa1ceb6
ui/scripts/vm_snapshots.js c50c7e1
ui/scripts/vpc.js e90d8a7
ui/scripts/zoneWizard.js 04687fe
Diff: https://reviews.apache.org/r/12907/diff/
Testing
-------
Compiled... unit tests passed, integration tests passed.
Viewed in browser, got expected results.
Thanks,
Ian Duffy
Re: Review Request 12907: WIP: Add LDAP Account addition UI. Feedback and
Help wanted.
Posted by Ian Duffy <ia...@ianduffy.ie>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/12907/
-----------------------------------------------------------
(Updated July 24, 2013, 5:52 p.m.)
Review request for cloudstack, Abhinandan Prateek, Brian Federle, Jessica Wang, Pranav Saxena, and Sebastien Goasguen.
Repository: cloudstack-git
Description (updated)
-------
Note: JS files were formatted with same formatting that was applied to origin/master.
This is purely a work in progress. I'm submitting it as I'd like somebody to give *detailed* feedback/reviewal before I go much further.
The code works/does-what-I-want but I'm not sure if its "correct" and follows cloudstack UI practices.
I had to do some ugly css in order to get the display I wanted.
I'm currently having issues with fields marked as "required" for some reason the requirement doesn't seem to be enforced.
I'm not sure if a loader appears should the request to listAllLdapUsers be slow to respond.
I'm not sure how to add a if ldapEnabled display this view else display old view condition.
I'm not sure how to detail with cases where the user might not have firstname, lastname or email set in ldap.
I'm not sure what happens if listAllLdapUsers returns a massive list of users... will it load on scroll? Does paging need to be implemented API side? How is this done?
I'm not sure how to get the newly created user to pop up in the table after the add account screen is closed.
I'm not sure how to notify the administrator that their user has successfully created or failed to create via the little pop up box.
For testing purposes there is a ldap server included in this branch. You can launch it with:
mvn -pl :cloud-plugin-user-authenticator-ldap ldap:run
and then configure it at Global Settings -> ldap.basedn = dc=cloudstack,dc=org
Also global settings -> LDAP Configuration, hostname: localhost, port: 10389.
Diffs
-----
client/tomcatconf/commands.properties.in 9e14d0f
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapAddConfigurationCmd.java 62736b16087561a7e25893cd46115795100c609e
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapCreateAccount.java PRE-CREATION
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapDeleteConfigurationCmd.java 329b91b
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListAllUsersCmd.java 087d156
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapListConfigurationCmd.java 6707878
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/command/LdapUserSearchCmd.java e6a40d0
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/response/LdapConfigurationResponse.java d583346
plugins/user-authenticators/ldap/src/org/apache/cloudstack/api/response/LdapUserResponse.java 40ba0ce
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapAuthenticator.java 2916202
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapConfiguration.java 8f31ce5
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapConfigurationVO.java d3ff820
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapContextFactory.java 30bdc5b
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManager.java c961d2c
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapManagerImpl.java be9b3d5
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUser.java 7c65e60
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUserManager.java 54802cf
plugins/user-authenticators/ldap/src/org/apache/cloudstack/ldap/LdapUtils.java 453dc0a
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapAddConfigurationCmdSpec.groovy ebade1e
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapConfigurationSpec.groovy 91c9baf
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapConfigurationVO.groovy 8135901
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapContextFactorySpec.groovy bb20fb3
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapDeleteConfigurationCmdSpec.groovy 664fd64
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapListAllUsersCmdSpec.groovy 30cd7cc
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapListConfigurationCmdSpec.groovy a7c1979
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapManagerImplSpec.groovy 5dfecb9
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapSearchUserCmdSpec.groovy d72878b
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapUserManagerSpec.groovy 489c250
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapUserResponseSpec.groovy 105203b
plugins/user-authenticators/ldap/test/groovy/org/apache/cloudstack/ldap/LdapUserSpec.groovy 6131267
ui/css/cloudstack3.css 4545e96
ui/index.jsp 34f0c54
ui/scripts/accounts.js bad8435
ui/scripts/accountsWizard.js PRE-CREATION
ui/scripts/affinity.js a9c6695
ui/scripts/autoscaler.js 15a9dac
ui/scripts/cloud.core.callbacks.js 6eb7644
ui/scripts/cloudStack.js c0ff7f2
ui/scripts/configuration.js 8bc40d6
ui/scripts/dashboard.js e8ab6c5
ui/scripts/docs.js c8ef0d9
ui/scripts/domains.js 01f4236
ui/scripts/events.js bd50887
ui/scripts/globalSettings.js ac63015
ui/scripts/installWizard.js 46769fa
ui/scripts/instanceWizard.js ff130d3
ui/scripts/instances.js 9b27d93
ui/scripts/lbStickyPolicy.js c0e2bfa
ui/scripts/network.js 95a93bc
ui/scripts/plugins.js 3c5bc0f
ui/scripts/projects.js ea1e6db
ui/scripts/regions.js 4be600f
ui/scripts/sharedFunctions.js a9f833c
ui/scripts/storage.js ad0965a
ui/scripts/system.js 3038a8a
ui/scripts/templates.js dbb0083
ui/scripts/ui-custom/accountsWizard.js PRE-CREATION
ui/scripts/ui-custom/affinity.js 1a23ff7
ui/scripts/ui-custom/autoscaler.js 2f6ce38
ui/scripts/ui-custom/dashboard.js 6d92318
ui/scripts/ui-custom/enableStaticNAT.js 1b2bf7b
ui/scripts/ui-custom/granularSettings.js 02d5c1f
ui/scripts/ui-custom/healthCheck.js 4b42fa7
ui/scripts/ui-custom/installWizard.js c53a642
ui/scripts/ui-custom/instanceWizard.js 31b4baa
ui/scripts/ui-custom/ipRules.js 34b2398
ui/scripts/ui-custom/login.js 0dbbf82
ui/scripts/ui-custom/physicalResources.js 5173172
ui/scripts/ui-custom/pluginListing.js 3dcce98
ui/scripts/ui-custom/projectSelect.js aef49ed
ui/scripts/ui-custom/projects.js f1f9eba
ui/scripts/ui-custom/recurringSnapshots.js 985f369
ui/scripts/ui-custom/regions.js 9fc36f3
ui/scripts/ui-custom/securityRules.js 2e2c9ac
ui/scripts/ui-custom/uploadVolume.js 996d8ac
ui/scripts/ui-custom/vpc.js 4edccf1
ui/scripts/ui-custom/zoneChart.js 5d4e0c0
ui/scripts/ui-custom/zoneFilter.js 9e6a493
ui/scripts/ui-custom/zoneWizard.js 877dbc0
ui/scripts/ui/core.js 18c3363
ui/scripts/ui/dialog.js 7f82eea
ui/scripts/ui/events.js bd609d2
ui/scripts/ui/utils.js 39ef3e3
ui/scripts/ui/widgets/cloudBrowser.js 9a56bb3
ui/scripts/ui/widgets/dataTable.js 1b3ea82
ui/scripts/ui/widgets/detailView.js 0bccef5
ui/scripts/ui/widgets/listView.js bc68a72
ui/scripts/ui/widgets/multiEdit.js 08bd0bf
ui/scripts/ui/widgets/notifications.js 0299603
ui/scripts/ui/widgets/overlay.js ecf12e6
ui/scripts/ui/widgets/tagger.js 9af6fb7
ui/scripts/ui/widgets/toolTip.js 6967acc
ui/scripts/ui/widgets/treeView.js fa1ceb6
ui/scripts/vm_snapshots.js c50c7e1
ui/scripts/vpc.js e90d8a7
ui/scripts/zoneWizard.js 04687fe
Diff: https://reviews.apache.org/r/12907/diff/
Testing
-------
Compiled... unit tests passed, integration tests passed.
Viewed in browser, got expected results.
Thanks,
Ian Duffy