Add an integration for Asana.

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

david
rbintegrations
master
f20a471...
rbintegrations

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.
Loading file attachments...

  • 0
  • 0
  • 34
  • 5
  • 39
Description From Last Updated
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    Missing a docstring.

  3. rbintegrations/asana/fields.py (Diff revision 3)
     
     

    Should be localized.

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

    Mind elaborating here on when it's rendered?

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

    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)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

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

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

    This isn't true for this function.

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

    Swap these.

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

    Should be localized.

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

    This is in the wrong import group.

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

    Missing a period.

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

    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)
     
     

    This view should use CheckLoginRequiredViewMixin and CheckLocalSiteAccessViewMixin.

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

    Can you do:

    .asana-field {
        &.selectize-control.loading:before {
            ...
        }
    
        &.selectize-dropdown {
            ...
        }
    }
    
  15. 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)
     
     
     
     

    IDs before classes.

    Same with others further down.

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

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

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

  19. 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. This can nest in #field_rbintegrations_asana above.

  21. Same comments here about double colons and using a span.

  22. No need for parens here.

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

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

  24. Better as callback.bind(this).

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

    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. You can simplify to:

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

    Alphabetical order (case-insensitive for dependencies).

  28. 
      
david
Review request changed

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    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)
     
     
     

    The mixins must go first.

  4. 
      
chipx86
  1. 
      
  2. 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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (0ead0fc)
Loading...