HUE-668 [admin] Enable the LDAP commands to be performed through the UI

Review Request #2007 - Created April 11, 2012 and submitted

Romain Rigaux
old-hue-rw
HUE-668
hue
bcwalrus, jnatkins
commit 372035b581826fd7cf610b4c654a4e69c8413241
Author: Romain Rigaux <romain@cloudera.com>
Date:   Wed Apr 11 11:25:38 2012 -0700

    HUE-668 [admin] Enable the LDAP commands to be performed through the UI
    
    Add a 'Add Ldap user' button to the admin homepage
    Add a 'Add Ldap group' button to the group page
    Add a 'Sync all' users and groups button to the admin homepage

:100644 100644 20e3326... a25108f... M  apps/useradmin/src/useradmin/templates/list_groups.mako
:100644 100644 303a4b4... 133d82d... M  apps/useradmin/src/useradmin/templates/list_users.mako
:000000 100644 0000000... c6355ef... A  apps/useradmin/src/useradmin/templates/sync_ldap_users_groups.mako
:100644 100644 63bf6ba... 6e9f19f... M  apps/useradmin/src/useradmin/tests.py
:100644 100644 407e4ea... e866206... M  apps/useradmin/src/useradmin/urls.py
:100644 100644 4640455... ae40f4d... M  apps/useradmin/src/useradmin/views.p
did again:

adding tests
running tests
importing a LDAP user works
syncing all works
  • 5
  • 0
  • 4
  • 0
  • 9
Description From Last Updated
Let's use .form-horizontal And, you can just do: ... action="${action | u}" ... See http://docs.makotemplates.org/en/latest/filtering.html bc Wong
Would avoid manipulating _errors. Let's change the function name to clean_username(). bc Wong
This probably isn't right. Groups have names, and users have usernames. My guess is this should be 'name' Jonathan Natkins
'name', not 'username' Jonathan Natkins
'name' Jonathan Natkins
  1. Sweet! Can you add a screenshot of the UI so I can check it out?
    1. It does not seem to work on reviewboard, I put them on the bug
  2. 
      
  1. There's an open bug on using modal dialog for complex workflows. https://issues.cloudera.org/browse/HUE-643
    The normal "edit user/group" action should definitely have its own page (rather than using a popup). So should the actions here, I think.
    1. Do you mean we should do a new page now?
      
      Or maybe better: on each new 'add/edit user/group' page we could add the LDAP action.
      
      e.g. 
        * on add page, add a LDAP checkbox which when checked will hide the manual fields and show the DN field
        * on edit page, if the user was imported from LDAP previously show the Sync button
        * keep Sync all on main page
      
      Overall is would be much cleaner.
      
    2. I have a slight preference to do the new actions not as popups. But it's not a strong preference.
      
      With the current import behaviour, keeping the import buttons on the main page is better. Otherwise people will go into the "Add user" form page, and see a form which has no relation to what they're about to do (import user).
  2. Use button groups? http://twitter.github.com/bootstrap/components.html#buttonGroups
  3. apps/useradmin/src/useradmin/templates/list_groups.mako (Diff revision 2)
     
     
     
     
     
     
     
     
    There's some alignment issues here. 
    * Let's not use tabs
    * Shiftwidth seems to be 8 in this file. It's a bit too much for my taste, but let's go with that here.
  4. 'syncLdapSyncBtn' -> 'syncLdapSaveBtn'
  5. Probably don't need urllib anymore.
    Would put '%>' on a new line.
    1. will remove urllib depending on next comment
      new line added before '%>'
  6. Let's use .form-horizontal
    
    And, you can just do:
      ... action="${action | u}" ...
    
    See http://docs.makotemplates.org/en/latest/filtering.html
    1. /useradmin/users/sync_ldap_users_groups
      
      {| u } is using urllib.quote_plus which also quotes the / then transforming the url to:
      %2Fuseradmin%2Fusers%2Fsync_ldap_users_groups
      which is then seen as:
      http://127.0.0.1:8000/useradmin/users//useradmin/users/sync_ldap_users_groups
      
      I could not find a way to have {| u } avoid escaping the / ?
      
      
    2. I see. Custom quoting is fine then.
  7. Would put this in an <div class="alert alert-info">
  8. apps/useradmin/src/useradmin/views.py (Diff revision 2)
     
     
     
     
    Would avoid manipulating _errors. Let's change the function name to clean_username().
    1. Actually it can't as it is depending on the DN field.
      
      I can try to just name it clean() and find a way to display the validation errors of the forms (it is not showing up now) but the error won't show up just next to the username field.
  9. apps/useradmin/src/useradmin/views.py (Diff revision 2)
     
     
     
     
    This feels redundant.
  10. apps/useradmin/src/useradmin/views.py (Diff revision 2)
     
     
     
     
    Same. Would remove.
  11. 
      
  1. 
      
  2. apps/useradmin/src/useradmin/views.py (Diff revision 2)
     
     
    This probably isn't right. Groups have names, and users have usernames. My guess is this should be 'name'
    1. So is it better to duplicate AddLdapUserForm in order to create the AddLdapGroupForm?
      
      It is a bit hacky but I can rename 'username' in 'name' in the parent form and still updates the labels in the __init__() of the child.
      Also if we stick the validation errors directly to the related field (https://docs.djangoproject.com/en/dev/ref/forms/validation/#cleaning-and-validating-fields-that-depend-on-each-other) no bad naming will leak to the user.
    2. It's not a huge amount of code to duplicate. If you're concerned with it, I'd support an AddLdapObjectForm, which both of those subclass. Additionally, the rules for group names are different than the rules for usernames, so we have to make the distinction.
  3. apps/useradmin/src/useradmin/views.py (Diff revision 2)
     
     
    to add a group
  4. apps/useradmin/src/useradmin/views.py (Diff revision 2)
     
     
    'name', not 'username'
  5. apps/useradmin/src/useradmin/views.py (Diff revision 2)
     
     
    let's be a bit more explicit and call out that we're syncing LDAP users/groups, rather than just "the" users and groups
  6. 
      
  1. Great, thanks!
  2. 
      
Review request changed

Status: Closed (submitted)

Loading...