HUE-7774 [core] Increase the limit of User and Group list in Hue Autocomplete box

Review Request #12251 - Created Dec. 19, 2017 and submitted

Roohi Syeda
hue
HUE-7774-Autocomplete
HUE-7774
hue
enricoberti, jgauthier, johan, ranade, romain, weixia, yingc
commit 14989c2c062fe7e51b36c87d4b018f45e38c6e02
Author: Roohi <roohisyeda@cloudera.com>
Date:   Tue Dec 19 12:51:33 2017 -0800

    HUE-7774 [core] Increase the limit of User and Group list in Hue Autocomplete box

:100644 100644 16b5613d39... ab98df674e... M	apps/useradmin/src/useradmin/views.py
:100644 100644 a42acb926e... 90d31523c7... M	desktop/core/src/desktop/static/desktop/js/ko.hue-bindings.js
:100644 100644 71bb860c24... 3a3165a175... M	desktop/core/src/desktop/static/desktop/js/share2.vm.js
:100644 100644 c4124a9acd... a660d97260... M	desktop/core/src/desktop/templates/common_share.mako
:100644 100644 d9b62b1388... bbad0ef26e... M	desktop/core/src/desktop/urls.py

Manual on Chrome

  • 2
  • 0
  • 55
  • 0
  • 57
Description From Last Updated
nit indentation cf. below Romain Rigaux
nit: indentation c5_autocomplete_filter_by_groupname = make_logged_in_client( 'user_doesnt_match_autocomplete_filter', is_superuser=False, groupname='group_test_list_for_autocomplete' ) Romain Rigaux
  1. Nice! :-) Only blocker on the frontend code would be the arrow functions and it would be good to move the css class defs to .less.

    The share2.vm.js is quite old and messy so I'm glad you got it to work, the idea is to improve it soon and get rid of global functions etc.

  2. apps/useradmin/src/useradmin/views.py (Diff revision 1)
     
     

    Is it case sensitive?

    1. Good catch. Will fix it.

  3. apps/useradmin/src/useradmin/views.py (Diff revision 1)
     
     
     

    Perhaps still keep an upper limit, for when the autocomplete_filter is empty?

  4. Nit: userMap, idToUserMap, groupMap

  5. IE11 doesn't support arrow functions. For now we have to stick with function (user) { .... }, later we might consider babel or similar.

  6. desktop/core/src/desktop/static/desktop/js/share2.vm.js (Diff revision 1)
     
     
     
     
     
     
     

    Nit: We're moving towards having all API calls in apiHelper.js, so instead:

    var apiHelper = ApiHelper.getInstance();

    apiHelper.fetchUsers({ userids: JSON.stringify(allusers) }).done(...).fail(...);

    There's probably one for the '/desktop/api2/doc' there as well.

  7. Nit: self.idToUserMap

  8. Nit: ApiHelper as above.

  9. desktop/core/src/desktop/static/desktop/js/share2.vm.js (Diff revision 1)
     
     
     
     
     
     
     
     

    var item = {};

    Nit: Actually no need for 'item':

    shareViewModel.items.push({
    data: {...},
    value: ...
    });

  10. desktop/core/src/desktop/static/desktop/js/share2.vm.js (Diff revision 1)
     
     
     
     
     
     
     
     

    Same as above

  11. Nit: Space around '='

  12. Move this to the .less files, either:

    hue-cross-version.less or hue-cui-override.less (for cui overrides).

    Generate the css with 'make css' or 'grunt watch less' (while developing) and we currently commit the generated css files as well.

    First 'npm install' if not done already

  13. This diverts from the common look and feel, was there a specific reason for it?

    1. I saw that the previous typeahead plugin had rounded menu, so I thought I shouldn't change that. Should I remove it?

    2. Ah, ok, we haven't really touched this in a while so it's lagging behind a bit style-wise. I'd say tone the radius down to be more consistent with the rest of Hue, you can do border-radius: @hue-panel-border-radius; in less (from hue-mixins.less, 2px).

  14. Nit: For colors use the @cui- colors or in this case @hue-primary-color-dark (after moving to .less)

  15. Nit: Move styles into classes

  16. Nit: Same, and instead of strong use font-weight: bold; as we try to keep the look and feel in the css.

  17. Nit: valueUpdate not needed with textInput.

    You can also drop the 'clearable x' classes as they're added by the clearable binding.

  18. 
      
  1. Nice!

  2. apps/useradmin/src/useradmin/views.py (Diff revision 1)
     
     

    If autocomplete_filter is empty, shouldn't we just not filter?

    if autocomplete_filter:
    users = users.filter(...=autocomplete_filter)

    That way we still have an autocomplete without having to type a first char?

    1. Please see my reply at line 115 of old file.

  3. apps/useradmin/src/useradmin/views.py (Diff revision 1)
     
     

    nit: space after ,

    usergroups,

  4. apps/useradmin/src/useradmin/views.py (Diff revision 1)
     
     
     

    +1

    Something like 100 maybe?

    1. The widget itself takes a parameter 'minLength' as to when to start getting new source (call source function). I forgot to ask this in our 1:1, should we use this parameter (I haven't tested it this yet) and set it to 2 or should we do as Johan and you suggested.

  5. apps/useradmin/src/useradmin/views.py (Diff revision 1)
     
     

    all() --> can be removed

  6. I see it was that dirty before :)
  7. 
      
  1. 
      
  2. apps/useradmin/src/useradmin/views.py (Diff revision 1)
     
     

    Got it. Didn't realize that this api is actually being used in other places too.

  3. 
      
  1. Nice!

    Almost! Just nits and how about also removing the old code and updating the tests?

  2. apps/useradmin/src/useradmin/views.py (Diff revision 2)
     
     

    AFIK this is called only in one API and the tests (useradmin tests.py), so better migrate it all to the new logic and update the tests?

    e.g.

    ./build/env/bin/hue test specific useradmin.tests:TestUserAdmin.test_list_for_autocomplete &> logs

  3. apps/useradmin/src/useradmin/views.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    As you can see we are doubling again the code duplication here and we should not need the old part anymore? (116-126)

    1. I think I am missing some point, but don't we need the old code when autocomplete_filter is empty?

    2. contains='' should match everything https://docs.djangoproject.com/en/dev/ref/models/querysets/#contains
      ?

  4. apps/useradmin/src/useradmin/views.py (Diff revision 2)
     
     

    keep new line?

  5. apps/useradmin/src/useradmin/views.py (Diff revision 2)
     
     

    nit:

    'users' : --> 'users':

  6. Why the 'var errorCallback ='

    ? (no needed)

  7. cui means "Cloudera UI", a set of common styles abd libs shared across Cloudera webapps

  8. nit: indent +2

  9. 
      
  1. 
      
  2. desktop/core/src/desktop/static/desktop/less/hue-cui-overrides.less (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    Nice! :-) Better in hue-cross-version.less as hue-cui-overrides.less is dedicated to styles that override the ones that come from cui, and the autocomplete isn't part of cui.

    Also, general indentation a bit off

    Just a note, for when you rebase, if you get a conflict on the generated .css files, just run "make css" and resolve with your new generated version.

    1. Thanks for the suggestion.

  3. border-radius: @hue-panel-border-radius;

  4. 
      
  1. Last nits

  2. apps/useradmin/src/useradmin/views.py (Diff revisions 2 - 3)
     
     

    No way to keep if 'if request.GET.get('only_mygroups')' ?

    1. This had to be changed after I wrote the python tests were only_mygroups value is a String value and not boolean value. The ajax works fine w/o the change that is keep it just 'if request.GET.get('only_mygroups')'

  3. apps/useradmin/src/useradmin/views.py (Diff revisions 2 - 3)
     
     
  4. apps/useradmin/src/useradmin/views.py (Diff revisions 2 - 3)
     
     

    Always limit?

  5. apps/useradmin/src/useradmin/tests.py (Diff revision 3)
     
     

    nit: space after :True,

  6. apps/useradmin/src/useradmin/tests.py (Diff revision 3)
     
     
     

    nit: merge both lines?

  7. 
      
  1. 
      
  2. apps/useradmin/src/useradmin/tests.py (Diff revision 4)
     
     

    nit: space after comma ,u'user_test_list_for_autocomplete2'

  3. apps/useradmin/src/useradmin/tests.py (Diff revision 4)
     
     
     

    nit indentation cf. below

  4. apps/useradmin/src/useradmin/tests.py (Diff revision 4)
     
     

    nit: indentation

    c5_autocomplete_filter_by_groupname = make_logged_in_client(
    'user_doesnt_match_autocomplete_filter',
    is_superuser=False,
    groupname='group_test_list_for_autocomplete'
    )

  5. apps/useradmin/src/useradmin/tests.py (Diff revision 4)
     
     

    If removing

    'only_mygroups': False

    no need to have the

    .lower() == 'true':

    or

    .lower() == 'false':

    (basically, we only check if the flag is there or not)

  6. nit: no space after map(

  7. nit: no space before :

  8. nit, not space before :

  9. 
      
  1. Nice!

  2. apps/useradmin/src/useradmin/tests.py (Diff revision 5)
     
     

    space after :

  3. apps/useradmin/src/useradmin/tests.py (Diff revision 5)
     
     

    space after comma

  4. apps/useradmin/src/useradmin/tests.py (Diff revision 5)
     
     

    nit: same

  5. apps/useradmin/src/useradmin/tests.py (Diff revision 5)
     
     

    space after :

  6. apps/useradmin/src/useradmin/views.py (Diff revision 5)
     
     

    PS:

    if request.ajax:

    , HTTP_X_REQUESTED_WITH='XMLHttpRequest'

    not needed

  7. apps/useradmin/src/useradmin/views.py (Diff revision 5)
     
     

    Add a limit on size of userids?

    e.g. [:100]

  8. apps/useradmin/src/useradmin/views.py (Diff revision 5)
     
     

    "" --> "[]"

    or we will have an exception if there is no userids

  9. 
      
  1. 
      
  2. apps/useradmin/src/useradmin/views.py (Diff revision 6)
     
     

    all() not needed

  3. desktop/core/src/desktop/static/desktop/js/apiHelper.js (Diff revision 6)
     
     
     
     
     
     

    nit: indent -2

  4. 
      
Review request changed

Status: Closed (submitted)

Loading...