Webapi Resource for integrations

Review Request #7105 — Created March 23, 2015 and discarded

Information

Review Board
master

Reviewers

This provides the webapi resource and the views for the integration admin page.

The webapi resource for both Integration and ConfiguredIntegration are provided here. The Integration resource allows for getting the list of Integrations that implements the class and hook defined in review 6918.

The ConfiguredIntegration resource allows GET, PUT and DELETE call to manage individual configured integration.

Unit test done.
Tested with WebapiResource test for both integrations and configuredIntegration resource.

Description From Last Updated

'PERMISSION_DENIED' imported but unused

reviewbotreviewbot

'NOT_LOGGED_IN' imported but unused

reviewbotreviewbot

'webapi_response_errors' imported but unused

reviewbotreviewbot

'webapi_login_required' imported but unused

reviewbotreviewbot

'webapi_check_local_site' imported but unused

reviewbotreviewbot

local variable 'manager' is assigned to but never used

reviewbotreviewbot

'IntegrationManager' imported but unused

reviewbotreviewbot

This function needs a docstring.

brenniebrennie

This function needs a docstring.

brenniebrennie

Please alphabetize these.

brenniebrennie

Undo this change.

brenniebrennie

This method isn't called from anywhere.

brenniebrennie

This will raise an exception if integration_class is None. You should define integration as None before the if integration_class block.

brenniebrennie

ConfiguredIntegration.

brenniebrennie

This docstring will end up in public facing documentation. It should not mention class names of RB-specific things (e.g., ConfiguredIntegration). …

brenniebrennie

ConfiguredIntegration

brenniebrennie

integration-id

brenniebrennie

When is_list is false, this will return a list. Shouldn't it return a single instance?

brenniebrennie

Can we change this to use integration-id ?

brenniebrennie

See previous comment about docstrings.

brenniebrennie

You can use enabled as a key-word argument to the function to not have to look into kwargs.

brenniebrennie

"Delete a configured integration."

brenniebrennie

Can we just do the call to get_integration_manager inside the class's __init__ method? Are we ever expecting to instantiate this …

brenniebrennie

Can we pull the query set out into a variable?

brenniebrennie

Same comment here re: integration managers as in the previous resource.

brenniebrennie

No blank line here.

daviddavid

All str in here should be six.text_type for future Python 3 compatibility. Here and below.

daviddavid

I think I'd prefer if these conditionals were switched: if config and config.integration: return config.integration.name else: return None

daviddavid

Same as above.

daviddavid

Same as above.

daviddavid

Same as above.

daviddavid

This should have a docstring.

daviddavid

This should have a docstring.

daviddavid

No blank line here.

daviddavid

All str items in here should be six.text_type

daviddavid

Please switch the conditional to test if integration: and return None in the else clause.

daviddavid

This should have a docstring.

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/integrations/admin.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/integrations/admin.py
    
    
  2. 
      
XU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/tests/test_configured_integration.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/webapi/tests/test_integration.py
        reviewboard/integrations/admin.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/tests/test_configured_integration.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/webapi/tests/test_integration.py
        reviewboard/integrations/admin.py
    
    
  2. Show all issues
     'PERMISSION_DENIED' imported but unused
    
  3. Show all issues
     'NOT_LOGGED_IN' imported but unused
    
  4. Show all issues
     'webapi_response_errors' imported but unused
    
  5. Show all issues
     'webapi_login_required' imported but unused
    
  6. Show all issues
     'webapi_check_local_site' imported but unused
    
  7. 
      
XU
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/tests/test_configured_integration.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/webapi/tests/test_integration.py
        reviewboard/integrations/admin.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/tests/test_configured_integration.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/webapi/tests/test_integration.py
        reviewboard/integrations/admin.py
    
    
  2. 
      
XU
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/tests/test_configured_integration.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/webapi/tests/test_integration.py
        reviewboard/integrations/admin.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/tests/test_configured_integration.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/webapi/tests/test_integration.py
        reviewboard/integrations/admin.py
    
    
  2. Show all issues
     local variable 'manager' is assigned to but never used
    
  3. 
      
XU
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/tests/test_configured_integration.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/webapi/tests/test_integration.py
        reviewboard/integrations/admin.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/tests/test_configured_integration.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/webapi/tests/test_integration.py
        reviewboard/integrations/admin.py
    
    
  2. Show all issues
     'IntegrationManager' imported but unused
    
  3. 
      
XU
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/tests/test_configured_integration.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/webapi/tests/test_integration.py
        reviewboard/integrations/admin.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/tests/test_configured_integration.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/webapi/tests/test_integration.py
        reviewboard/integrations/admin.py
    
    
  2. 
      
brennie
  1. Can you change references to "webapi" in your summary and description to "WebAPI" ?

  2. reviewboard/integrations/views.py (Diff revision 6)
     
     
    Show all issues

    This function needs a docstring.

  3. reviewboard/integrations/views.py (Diff revision 6)
     
     
    Show all issues

    This function needs a docstring.

  4. reviewboard/webapi/resources/__init__.py (Diff revision 6)
     
     
     
    Show all issues

    Please alphabetize these.

  5. Show all issues

    Undo this change.

  6. Show all issues

    This method isn't called from anywhere.

    1. I understand how this works now, dropping.

  7. 
      
XU
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/tests/test_configured_integration.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/webapi/tests/test_integration.py
        reviewboard/integrations/admin.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/tests/test_configured_integration.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/webapi/tests/test_integration.py
        reviewboard/integrations/admin.py
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/integrations/views.py (Diff revision 7)
     
     
     
     
     
     
     
     
    Show all issues

    This will raise an exception if integration_class is None. You should define integration as None before the if integration_class block.

  3. Show all issues

    ConfiguredIntegration.

  4. reviewboard/webapi/resources/configured_integration.py (Diff revision 7)
     
     
     
     
     
     
     
    Show all issues

    This docstring will end up in public facing documentation. It should not mention class names of RB-specific things (e.g., ConfiguredIntegration). Instead it should just refer to, e.g., "configuration integration".

  5. Show all issues

    ConfiguredIntegration

  6. Show all issues

    integration-id

  7. Show all issues

    When is_list is false, this will return a list. Shouldn't it return a single instance?

    1. The superclass's get_object method in WebAPI resource will be calling get_queryset to get the queryset before reducing it down to a single item. From my understanding of the get_queryset method, it seems to be a pre-process optimzation stage to reduce/specialize the query depending on whether it is a list or not.

  8. Show all issues

    Can we change this to use integration-id ?

  9. reviewboard/webapi/resources/configured_integration.py (Diff revision 7)
     
     
     
     
     
     
    Show all issues

    See previous comment about docstrings.

  10. Show all issues

    You can use enabled as a key-word argument to the function to not have to look into kwargs.

  11. Show all issues

    "Delete a configured integration."

  12. Show all issues

    Can we just do the call to get_integration_manager inside the class's __init__ method?

    Are we ever expecting to instantiate this with a different integration manager?

  13. reviewboard/webapi/resources/integration.py (Diff revision 7)
     
     
     
     
    Show all issues

    Can we pull the query set out into a variable?

  14. Show all issues

    Same comment here re: integration managers as in the previous resource.

  15. 
      
XU
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/tests/test_configured_integration.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/webapi/tests/test_integration.py
        reviewboard/integrations/admin.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/root.py
        reviewboard/integrations/urls.py
        reviewboard/webapi/resources/__init__.py
        reviewboard/webapi/tests/test_configured_integration.py
        reviewboard/integrations/views.py
        reviewboard/webapi/resources/configured_integration.py
        reviewboard/webapi/resources/integration.py
        reviewboard/urls.py
        reviewboard/webapi/tests/test_integration.py
        reviewboard/integrations/admin.py
    
    
  2. 
      
david
  1. Some of the parts of this change look like they'd be more appropriate for /r/7106 (like the admin.py, urls.py, and views.py changes)

  2. Show all issues

    No blank line here.

  3. Show all issues

    All str in here should be six.text_type for future Python 3 compatibility. Here and below.

  4. Show all issues

    I think I'd prefer if these conditionals were switched:

    if config and config.integration:
        return config.integration.name
    else:
        return None
    
  5. Show all issues

    Same as above.

  6. reviewboard/webapi/resources/configured_integration.py (Diff revision 8)
     
     
     
     
     
     
    Show all issues

    Same as above.

  7. Show all issues

    Same as above.

  8. Show all issues

    This should have a docstring.

  9. Show all issues

    This should have a docstring.

  10. Show all issues

    No blank line here.

  11. Show all issues

    All str items in here should be six.text_type

  12. reviewboard/webapi/resources/integration.py (Diff revision 8)
     
     
     
     
     
     
    Show all issues

    Please switch the conditional to test if integration: and return None in the else clause.

  13. Show all issues

    This should have a docstring.

  14. 
      
XU
Review request changed
Status:
Discarded
Change Summary:

Closed in favor of a revised version of Integrations that landed for 3.0.