• 
      

    Add an integration for Asana.

    Review Request #9363 — Created Nov. 11, 2017 and submitted

    Information

    rbintegrations
    master
    f20a471...

    Reviewers

    This change adds a new integration for linking to Asana tasks from
    review requests. This consists of a few pieces: the integration config
    which connects to a specific Asana workspace, and a review request field
    which allows people to search for and link to multiple tasks.

    The UI is currently fully functional, but at a later date I'd like to
    make it look a bit more like the related object selector used in the
    admin UI, with the search box shown above the list of selected items
    (instead of swapping out the entire UI for what looks more like "tags"
    inside the selectize widget).

    • Tested success and error conditions within the integration config UI.
    • Added a bunch of tasks and verified that everything loaded correctly.
    • Saved the field in a variety of states.

    Description From Last Updated

    I just mentioned this in the Trello change as well, but the one thing I want a better assurance of …

    chipx86chipx86

    F401 'django.utils.six' imported but unused

    reviewbotreviewbot

    E501 line too long (130 > 79 characters)

    reviewbotreviewbot

    E501 line too long (82 > 79 characters)

    reviewbotreviewbot

    E501 line too long (130 > 79 characters)

    reviewbotreviewbot

    Missing a docstring.

    chipx86chipx86

    Should be localized.

    chipx86chipx86

    Mind elaborating here on when it's rendered?

    chipx86chipx86

    You can simplify this a bit by starting off with: if not value: return '' Lines will be less crowded …

    chipx86chipx86

    Can we maybe use templates for rendering instead of hard-coding HTML here?

    chipx86chipx86

    E501 line too long (130 > 79 characters)

    reviewbotreviewbot

    This isn't true for this function.

    chipx86chipx86

    Swap these.

    chipx86chipx86

    Should be localized.

    chipx86chipx86

    This is in the wrong import group.

    chipx86chipx86

    Missing a period.

    chipx86chipx86

    We should catch exceptions and log them so this doesn't crash if something goes wrong (network issues, data issues, whatever).

    chipx86chipx86

    This view should use CheckLoginRequiredViewMixin and CheckLocalSiteAccessViewMixin.

    chipx86chipx86

    Can you do: .asana-field { &.selectize-control.loading:before { ... } &.selectize-dropdown { ... } }

    chipx86chipx86

    Too many colons. Can we use a class instead? I have a change pending for post-3.0 that will get rid …

    chipx86chipx86

    IDs before classes. Same with others further down.

    chipx86chipx86

    Instead of hard-coding 13px, can we use an existing variable definition?

    chipx86chipx86

    Can we defein variables for the colors being used? Here and below.

    chipx86chipx86

    What do we need flex here for? It still doesn't have wide-enough working support.

    chipx86chipx86

    This can nest in #field_rbintegrations_asana above.

    chipx86chipx86

    Same comments here about double colons and using a span.

    chipx86chipx86

    No need for parens here.

    chipx86chipx86

    Looks like we're duplicating the one from the Python field. Can we standardize these somehow?

    chipx86chipx86

    Better as callback.bind(this).

    chipx86chipx86

    If you do: if (apiKey.length === 0) { return; } You can save an indentation level and reduce the line …

    chipx86chipx86

    You can simplify to: $accessToken.on('change keyup', changeWorkspaceEnabled);

    chipx86chipx86

    Alphabetical order (case-insensitive for dependencies).

    chipx86chipx86

    F401 'json' imported but unused

    reviewbotreviewbot

    F401 'django.utils.html.format_html' imported but unused

    reviewbotreviewbot

    F401 'django.utils.html.format_html_join' imported but unused

    reviewbotreviewbot

    E501 line too long (82 > 79 characters)

    reviewbotreviewbot

    Can we set a logger = ... above, and use that here and anywhere else we might be logging?

    chipx86chipx86

    The mixins must go first.

    chipx86chipx86

    F401 'kgb' imported but unused

    reviewbotreviewbot
    Checks run (1 failed, 1 succeeded)
    flake8 failed.
    JSHint passed.

    flake8

    david
    Review request changed
    Commit:
    c396825ecf165824ec8864fa7f96208f1bb48165
    435719ff5795b0260ae582e8c4d0f1692dd0b027

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    david
    Review request changed
    Change Summary:

    Typo.

    Commit:
    435719ff5795b0260ae582e8c4d0f1692dd0b027
    af2745347ce1e92f02b7a41b77583e8c3aaf24d1

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    chipx86
    1. 
        
    2. rbintegrations/asana/fields.py (Diff revision 3)
       
       
      Show all issues

      Missing a docstring.

    3. rbintegrations/asana/fields.py (Diff revision 3)
       
       
      Show all issues

      Should be localized.

    4. rbintegrations/asana/fields.py (Diff revision 3)
       
       
      Show all issues

      Mind elaborating here on when it's rendered?

    5. rbintegrations/asana/fields.py (Diff revision 3)
       
       
       
       
       
      Show all issues

      You can simplify this a bit by starting off with:

      if not value:
          return ''
      

      Lines will be less crowded that way.

    6. rbintegrations/asana/fields.py (Diff revision 3)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      Can we maybe use templates for rendering instead of hard-coding HTML here?

    7. rbintegrations/asana/fields.py (Diff revision 3)
       
       
       
      Show all issues

      This isn't true for this function.

    8. rbintegrations/asana/integration.py (Diff revision 3)
       
       
       
      Show all issues

      Swap these.

    9. rbintegrations/asana/integration.py (Diff revision 3)
       
       
      Show all issues

      Should be localized.

    10. rbintegrations/asana/views.py (Diff revision 3)
       
       
      Show all issues

      This is in the wrong import group.

    11. rbintegrations/asana/views.py (Diff revision 3)
       
       
      Show all issues

      Missing a period.

    12. rbintegrations/asana/views.py (Diff revision 3)
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      We should catch exceptions and log them so this doesn't crash if something goes wrong (network issues, data issues, whatever).

    13. rbintegrations/asana/views.py (Diff revision 3)
       
       
      Show all issues

      This view should use CheckLoginRequiredViewMixin and CheckLocalSiteAccessViewMixin.

    14. rbintegrations/static/css/asana/asana.less (Diff revision 3)
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      Can you do:

      .asana-field {
          &.selectize-control.loading:before {
              ...
          }
      
          &.selectize-dropdown {
              ...
          }
      }
      
    15. Show all issues

      Too many colons.

      Can we use a class instead? I have a change pending for post-3.0 that will get rid of FontAwesome spinners (which are broken and officially deprecated in terms of being recommended) and replace them with a non-wobbly spinner. It will require a <span> of some sort, and it'll be easier for me to update this if we're not duplicating FontAwesome CSS here.

      1. Can you post a version of that change somewhere so I can see? We're relatively limited with what we can do in selectize but I might be able to make it work.

      2. I'm just going to proceed for now.

        We can't add a span. Maybe with your change you can add a macro to defs.less (.spinner()) and then I can invoke that on the pseudo-element?

      3. Yeah don't worry about it for now. If you can add some comment containing the word "spinner," though, it'll help me grep for this when I get back to this change.

    16. rbintegrations/static/css/asana/asana.less (Diff revision 3)
       
       
       
       
      Show all issues

      IDs before classes.

      Same with others further down.

    17. Show all issues

      Instead of hard-coding 13px, can we use an existing variable definition?

    18. rbintegrations/static/css/asana/asana.less (Diff revision 3)
       
       
       
       
      Show all issues

      Can we defein variables for the colors being used? Here and below.

    19. Show all issues

      What do we need flex here for? It still doesn't have wide-enough working support.

      1. It's what asana uses, so I suspect anyone who's using this has a browser that works, but I'll work around it.

    20. Show all issues

      This can nest in #field_rbintegrations_asana above.

    21. Show all issues

      Same comments here about double colons and using a span.

    22. Show all issues

      No need for parens here.

    23. rbintegrations/static/js/asana/asanaFieldView.es6.js (Diff revision 3)
       
       
       
       
       
       
       
       
       
       
       
       
       
      Show all issues

      Looks like we're duplicating the one from the Python field. Can we standardize these somehow?

    24. Show all issues

      Better as callback.bind(this).

    25. rbintegrations/static/js/asana/integrationConfig.es6.js (Diff revision 3)
       
       
       
       
       
       
      Show all issues

      If you do:

      if (apiKey.length === 0) {
          return;
      }
      

      You can save an indentation level and reduce the line widths for the rest of these.

    26. Show all issues

      You can simplify to:

      $accessToken.on('change keyup', changeWorkspaceEnabled);
      
    27. setup.py (Diff revision 3)
       
       
       
      Show all issues

      Alphabetical order (case-insensitive for dependencies).

    28. 
        
    david
    Review request changed
    Commit:
    af2745347ce1e92f02b7a41b77583e8c3aaf24d1
    1fbcaf4aa55fe76edecd735c3117a0c0e5baab69

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    david
    david
    chipx86
    1. 
        
    2. rbintegrations/asana/views.py (Diff revision 6)
       
       
      Show all issues

      Can we set a logger = ... above, and use that here and anywhere else we might be logging?

    3. rbintegrations/asana/views.py (Diff revision 6)
       
       
       
      Show all issues

      The mixins must go first.

    4. 
        
    chipx86
    1. 
        
    2. Show all issues

      I just mentioned this in the Trello change as well, but the one thing I want a better assurance of is that data from Local Sites can't leak, and that this will be safe on RBCommons.

      Can you add unit tests for anything involving Local Site/configuration/review request accesses?

      1. The only things that could possibly "leak" are the two views, both of which are guarded by the appropriate mixins. Is there a specific concern you had?

    3. 
        
    david
    Review request changed
    Commit:
    4f114272c5f8e62e5a2ca72950422c29246737e7
    5619891155076b2087ce7b45459dff134d9050c4

    Checks run (1 failed, 1 succeeded)

    flake8 failed.
    JSHint passed.

    flake8

    david
    chipx86
    1. Ship It!
    2. 
        
    david
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to master (0ead0fc)