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.  'PERMISSION_DENIED' imported but unused
    
  3.  'NOT_LOGGED_IN' imported but unused
    
  4.  'webapi_response_errors' imported but unused
    
  5.  'webapi_login_required' imported but unused
    
  6.  '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.  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.  '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)
     
     

    This function needs a docstring.

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

    This function needs a docstring.

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

    Please alphabetize these.

  5. Undo this change.

  6. 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)
     
     
     
     
     
     
     
     

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

  3. ConfiguredIntegration.

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

    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. ConfiguredIntegration

  6. 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.

  7. Can we change this to use integration-id ?

  8. reviewboard/webapi/resources/configured_integration.py (Diff revision 7)
     
     
     
     
     
     

    See previous comment about docstrings.

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

  10. "Delete a configured integration."

  11. 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?

  12. reviewboard/webapi/resources/integration.py (Diff revision 7)
     
     
     
     

    Can we pull the query set out into a variable?

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

  14. 
      
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. No blank line here.

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

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

    if config and config.integration:
        return config.integration.name
    else:
        return None
    
  5. Same as above.

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

    Same as above.

  7. Same as above.

  8. This should have a docstring.

  9. This should have a docstring.

  10. No blank line here.

  11. All str items in here should be six.text_type

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

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

  13. 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.

Loading...