• 
      

    Add a module for creating and managing integrations with other services.

    Review Request #7947 — Created Feb. 6, 2016 and submitted

    Information

    Djblets
    release-0.10.x

    Reviewers

    Modern web applications often need to integrate with other services,
    such as chat services. Each generally needs to be able to support any
    number of user-provided configurations, efficiently.
    
    This change adds a framework for more easily producing these
    integrations. Consuming applications just need to set up some classes
    and URLs, and will then be able to define integrations or, if they
    support extensions, will allow others to define integrations.
    
    A single integration can have any number of configurations, which
    applications can choose to bind to users, organizations, or anything
    else they choose. Configurations can be enabled or disabled, can have a
    name, and can have arbitrary settings configured.
    
    Integrations are instantiated when registered, but their logic and state
    are only initialized once there's one or more enabled configurations in
    the database for that integration. This allows a large number of
    integrations to be registered, without them having to take up any
    processing time unless used.
    
    An integration will generally listen to signals or other events, and may
    want to register into parts of the application. This overlaps a great
    deal with the needs of extensions, and so they've been written to be
    hook owners, allowing the user of ExtensionHooks. Like an extension,
    these hooks are bound to the lifecycle of an integration instance.
    
    There are built-in views available for listing all integrations (and
    their configurations), and creating/editing/deleting configurations.
    These can be used in the administration UI, or in other pages, depending
    on the needs of the application. To facilitate this, there are base
    classes for the views and there are administration-specific subclasses.
    
    Djblets just provides the foundation for integrations. Consuming
    applications are expected to subclass some of the classes to provide
    additional information, such as an instance of an IntegrationManager
    or additional configuration data.

    Unit tests pass.

    Modified rbslack to use integrations, and tested that they work.

    Verified manually that integrations are only enabled when there are
    enabled configurations, and that this state is recomputed properly when
    enabling/disabling/creating/deleting integrations.

    Read through the docs and checked links.


    Description From Last Updated

    local variable 'config3' is assigned to but never used

    reviewbot reviewbot

    local variable 'config4' is assigned to but never used

    reviewbot reviewbot

    local variable 'config2' is assigned to but never used

    reviewbot reviewbot

    local variable 'config3' is assigned to but never used

    reviewbot reviewbot

    local variable 'config4' is assigned to but never used

    reviewbot reviewbot

    Col: 5 E303 too many blank lines (2)

    reviewbot reviewbot

    local variable 'config3' is assigned to but never used

    reviewbot reviewbot

    local variable 'config1' is assigned to but never used

    reviewbot reviewbot

    local variable 'config3' is assigned to but never used

    reviewbot reviewbot

    local variable 'manager1' is assigned to but never used

    reviewbot reviewbot

    local variable 'manager2' is assigned to but never used

    reviewbot reviewbot

    For @property, don't we write the docstring as more of an attribute form than a method form?

    david david

    Maybe use logging.exception instead of formatting in e?

    david david

    Same here.

    david david

    I think it would be better to say "An example would be when..." rather than "This is useful, for instance, …

    david david

    ", though," doesn't add anything.

    david david

    Is this the best order to tear things down? It seems like it should be in the reverse order from …

    david david

    You don't need to format in e when using logging.exception.

    david david

    Shouldn't this link to #?

    david david

    Can we translate the string with {% trans %} instead of gettext()?

    david david

    Doesn't dispatch return a response?

    david david

    Same here.

    david david

    And here.

    david david

    Should this use self.get_config?

    david david

    Missing "Returns:"

    david david

    And here.

    david david

    This file needs unicode_literals

    david david

    This returns a response, not a request.

    david david

    Still missing unicode_literals. Did you forget to git add?

    david david
    reviewbot
    1. Tool: PEP8 Style Checker
      Processed Files:
          djblets/integrations/__init__.py
          djblets/integrations/tests/testcases.py
          djblets/integrations/errors.py
          djblets/integrations/tests/test_models.py
          djblets/integrations/hooks.py
          djblets/integrations/mixins.py
          djblets/integrations/views.py
          djblets/integrations/urls.py
          djblets/integrations/models.py
          djblets/cache/synchronizer.py
          djblets/integrations/middleware.py
          djblets/integrations/integration.py
          djblets/integrations/manager.py
          djblets/integrations/tests/test_integration.py
          djblets/integrations/app.py
          djblets/integrations/tests/models.py
          djblets/integrations/tests/test_manager.py
          djblets/integrations/tests/test_hooks.py
          djblets/integrations/forms.py
          djblets/settings.py
      
      Ignored Files:
          docs/djblets/guides/integrations/supporting-integrations.rst
          djblets/static/djblets/css/integrations.less
          docs/djblets/guides/integrations/writing-integrations.rst
          docs/djblets/guides/integrations/index.rst
          docs/djblets/guides/index.rst
          docs/djblets/guides/extensions/index.rst
          djblets/integrations/tests/__init__.py
          djblets/integrations/templates/integrations/configure_integration.html
          docs/djblets/coderef/index.rst
          djblets/integrations/templates/integrations/integration_list.html
      
      
      
      Tool: Pyflakes
      Processed Files:
          djblets/integrations/__init__.py
          djblets/integrations/tests/testcases.py
          djblets/integrations/errors.py
          djblets/integrations/tests/test_models.py
          djblets/integrations/hooks.py
          djblets/integrations/mixins.py
          djblets/integrations/views.py
          djblets/integrations/urls.py
          djblets/integrations/models.py
          djblets/cache/synchronizer.py
          djblets/integrations/middleware.py
          djblets/integrations/integration.py
          djblets/integrations/manager.py
          djblets/integrations/tests/test_integration.py
          djblets/integrations/app.py
          djblets/integrations/tests/models.py
          djblets/integrations/tests/test_manager.py
          djblets/integrations/tests/test_hooks.py
          djblets/integrations/forms.py
          djblets/settings.py
      
      Ignored Files:
          docs/djblets/guides/integrations/supporting-integrations.rst
          djblets/static/djblets/css/integrations.less
          docs/djblets/guides/integrations/writing-integrations.rst
          docs/djblets/guides/integrations/index.rst
          docs/djblets/guides/index.rst
          docs/djblets/guides/extensions/index.rst
          djblets/integrations/tests/__init__.py
          djblets/integrations/templates/integrations/configure_integration.html
          docs/djblets/coderef/index.rst
          djblets/integrations/templates/integrations/integration_list.html
      
      
    2. Show all issues
       local variable 'config3' is assigned to but never used
      
    3. Show all issues
       local variable 'config4' is assigned to but never used
      
    4. Show all issues
       local variable 'config2' is assigned to but never used
      
    5. Show all issues
       local variable 'config3' is assigned to but never used
      
    6. Show all issues
       local variable 'config4' is assigned to but never used
      
    7. Show all issues
      Col: 5
       E303 too many blank lines (2)
      
    8. Show all issues
       local variable 'config3' is assigned to but never used
      
    9. Show all issues
       local variable 'config1' is assigned to but never used
      
    10. Show all issues
       local variable 'config3' is assigned to but never used
      
    11. Show all issues
       local variable 'manager1' is assigned to but never used
      
    12. Show all issues
       local variable 'manager2' is assigned to but never used
      
    13. 
        
    chipx86
    reviewbot
    1. Tool: PEP8 Style Checker
      Processed Files:
          djblets/integrations/__init__.py
          djblets/integrations/tests/testcases.py
          djblets/integrations/errors.py
          djblets/integrations/tests/test_models.py
          djblets/integrations/hooks.py
          djblets/integrations/mixins.py
          djblets/integrations/views.py
          djblets/integrations/urls.py
          djblets/integrations/models.py
          djblets/integrations/forms.py
          djblets/integrations/middleware.py
          djblets/integrations/integration.py
          djblets/integrations/manager.py
          djblets/integrations/tests/test_integration.py
          djblets/integrations/app.py
          djblets/integrations/tests/test_manager.py
          djblets/integrations/tests/test_hooks.py
          djblets/integrations/tests/models.py
          djblets/settings.py
      
      Ignored Files:
          djblets/integrations/templates/integrations/integration_list.html
          djblets/static/djblets/css/integrations.less
          djblets/integrations/templates/integrations/configure_integration.html
          djblets/integrations/tests/__init__.py
      
      
      
      Tool: Pyflakes
      Processed Files:
          djblets/integrations/__init__.py
          djblets/integrations/tests/testcases.py
          djblets/integrations/errors.py
          djblets/integrations/tests/test_models.py
          djblets/integrations/hooks.py
          djblets/integrations/mixins.py
          djblets/integrations/views.py
          djblets/integrations/urls.py
          djblets/integrations/models.py
          djblets/integrations/forms.py
          djblets/integrations/middleware.py
          djblets/integrations/integration.py
          djblets/integrations/manager.py
          djblets/integrations/tests/test_integration.py
          djblets/integrations/app.py
          djblets/integrations/tests/test_manager.py
          djblets/integrations/tests/test_hooks.py
          djblets/integrations/tests/models.py
          djblets/settings.py
      
      Ignored Files:
          djblets/integrations/templates/integrations/integration_list.html
          djblets/static/djblets/css/integrations.less
          djblets/integrations/templates/integrations/configure_integration.html
          djblets/integrations/tests/__init__.py
      
      
    2. 
        
    david
    1. 
        
    2. djblets/integrations/forms.py (Diff revision 2)
       
       
      Show all issues

      For @property, don't we write the docstring as more of an attribute form than a method form?

      1. Oops, yeah. Mentally skipped over the @property when going back and writing docs.

    3. djblets/integrations/hooks.py (Diff revision 2)
       
       
      Show all issues

      Maybe use logging.exception instead of formatting in e?

    4. djblets/integrations/hooks.py (Diff revision 2)
       
       
      Show all issues

      Same here.

    5. djblets/integrations/integration.py (Diff revision 2)
       
       
      Show all issues

      I think it would be better to say "An example would be when..." rather than "This is useful, for instance, when..."

    6. djblets/integrations/integration.py (Diff revision 2)
       
       
      Show all issues

      ", though," doesn't add anything.

    7. djblets/integrations/integration.py (Diff revision 2)
       
       
       
       
      Show all issues

      Is this the best order to tear things down? It seems like it should be in the reverse order from enable.

      1. I kind of went back and forth on this.. I'm not sold on the current order, but will explain my thoughts, and you can let me know if I'm neglecting something important.

        So first I'm setting enabled to False early so that if anything being run needs to check if the integration is being shut down, it can determine that. I set enabled to True early in enable_integration for the same reasons.

        Then I'm invoking integration-specific shutdown behavior. I don't know that this needs to be done before or after shutdown_hooks. Either order is probably okay. I put it before just to keep as much state around as possible while the integration was doing whatever it does.

        Then clearing hooks.

        I could move enabled to last, effectively inversing enable. Maybe that's better in some way?

    8. djblets/integrations/manager.py (Diff revision 2)
       
       
       
       
      Show all issues

      You don't need to format in e when using logging.exception.

      1. I include it so that the "Unexpected error" and the error message from the exception are right together. That makes it much easier when reading logs. Otherwise, you have to dig through to the "ExceptionType: message" bit of the exception at the end. When viewing in things like Papertrail, or working with alerts from Logentries, that's much more of a pain than having a comprehensive error message.

    9. Show all issues

      Shouldn't this link to #?

    10. Show all issues

      Can we translate the string with {% trans %} instead of gettext()?

      1. If I do that, I then have to deal with JS-escaping the content, which would mean doing:

        {% trans "blah blah" as somevar %}
        if (confirm("{{somevar|escapejs}}")) {
        

        It's safer and easier going through gettext().

    11. djblets/integrations/views.py (Diff revision 2)
       
       
       
      Show all issues

      Doesn't dispatch return a response?

      1. Thanks for catching that. Stupid mistake, copy/pasted..

    12. djblets/integrations/views.py (Diff revision 2)
       
       
       
      Show all issues

      Same here.

    13. djblets/integrations/views.py (Diff revision 2)
       
       
       
      Show all issues

      And here.

    14. djblets/integrations/views.py (Diff revision 2)
       
       
       
       
       
      Show all issues

      Should this use self.get_config?

      1. Oops, actually, get_config() was supposed to be removed. It was an earlier attempt at adding the extensibility needed to let Review Board and RBCommons factor in Local Sites, but I never finished that..

        Integration.get_config() only works with enabled configurations (since it's there to help integrations loop through the configs they should be operating on), so I wasn't able to use it without changing assumptions there. So I added get_config_query_kwargs to replace it, along with this code for performing this query, but left get_config().

        Removing and fixing up the rest.

    15. djblets/integrations/views.py (Diff revision 2)
       
       
      Show all issues

      Missing "Returns:"

    16. djblets/integrations/views.py (Diff revision 2)
       
       
       
      Show all issues

      And here.

    17. 
        
    chipx86
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          djblets/integrations/__init__.py
          djblets/integrations/tests/testcases.py
          djblets/integrations/errors.py
          djblets/integrations/tests/test_models.py
          djblets/integrations/hooks.py
          djblets/integrations/mixins.py
          djblets/integrations/views.py
          djblets/integrations/urls.py
          djblets/integrations/models.py
          djblets/integrations/forms.py
          djblets/integrations/middleware.py
          djblets/integrations/integration.py
          djblets/integrations/manager.py
          djblets/integrations/tests/test_integration.py
          djblets/integrations/app.py
          djblets/integrations/tests/test_manager.py
          djblets/integrations/tests/test_hooks.py
          djblets/integrations/tests/models.py
          djblets/settings.py
      
      Ignored Files:
          djblets/integrations/templates/integrations/integration_list.html
          djblets/static/djblets/css/integrations.less
          djblets/integrations/templates/integrations/configure_integration.html
          djblets/integrations/tests/__init__.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          djblets/integrations/__init__.py
          djblets/integrations/tests/testcases.py
          djblets/integrations/errors.py
          djblets/integrations/tests/test_models.py
          djblets/integrations/hooks.py
          djblets/integrations/mixins.py
          djblets/integrations/views.py
          djblets/integrations/urls.py
          djblets/integrations/models.py
          djblets/integrations/forms.py
          djblets/integrations/middleware.py
          djblets/integrations/integration.py
          djblets/integrations/manager.py
          djblets/integrations/tests/test_integration.py
          djblets/integrations/app.py
          djblets/integrations/tests/test_manager.py
          djblets/integrations/tests/test_hooks.py
          djblets/integrations/tests/models.py
          djblets/settings.py
      
      Ignored Files:
          djblets/integrations/templates/integrations/integration_list.html
          djblets/static/djblets/css/integrations.less
          djblets/integrations/templates/integrations/configure_integration.html
          djblets/integrations/tests/__init__.py
      
      
    2. 
        
    david
    1. 
        
    2. djblets/integrations/errors.py (Diff revision 3)
       
       
      Show all issues

      This file needs unicode_literals

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

      This returns a response, not a request.

    4. 
        
    chipx86
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          djblets/integrations/__init__.py
          djblets/integrations/tests/testcases.py
          djblets/integrations/errors.py
          djblets/integrations/tests/test_models.py
          djblets/integrations/hooks.py
          djblets/integrations/mixins.py
          djblets/integrations/views.py
          djblets/integrations/urls.py
          djblets/integrations/models.py
          djblets/integrations/forms.py
          djblets/integrations/middleware.py
          djblets/integrations/integration.py
          djblets/integrations/manager.py
          djblets/integrations/tests/test_integration.py
          djblets/integrations/app.py
          djblets/integrations/tests/test_manager.py
          djblets/integrations/tests/test_hooks.py
          djblets/integrations/tests/models.py
          djblets/staticbundles.py
      
      Ignored Files:
          djblets/integrations/templates/integrations/integration_list.html
          djblets/static/djblets/css/integrations.less
          djblets/integrations/templates/integrations/configure_integration.html
          djblets/integrations/tests/__init__.py
      
      
      
      Tool: PEP8 Style Checker
      Processed Files:
          djblets/integrations/__init__.py
          djblets/integrations/tests/testcases.py
          djblets/integrations/errors.py
          djblets/integrations/tests/test_models.py
          djblets/integrations/hooks.py
          djblets/integrations/mixins.py
          djblets/integrations/views.py
          djblets/integrations/urls.py
          djblets/integrations/models.py
          djblets/integrations/forms.py
          djblets/integrations/middleware.py
          djblets/integrations/integration.py
          djblets/integrations/manager.py
          djblets/integrations/tests/test_integration.py
          djblets/integrations/app.py
          djblets/integrations/tests/test_manager.py
          djblets/integrations/tests/test_hooks.py
          djblets/integrations/tests/models.py
          djblets/staticbundles.py
      
      Ignored Files:
          djblets/integrations/templates/integrations/integration_list.html
          djblets/static/djblets/css/integrations.less
          djblets/integrations/templates/integrations/configure_integration.html
          djblets/integrations/tests/__init__.py
      
      
    2. 
        
    david
    1. 
        
    2. djblets/integrations/errors.py (Diff revision 4)
       
       
      Show all issues

      Still missing unicode_literals. Did you forget to git add?

      1. Apparently I never published the fix to this :(

    3. 
        
    chipx86
    reviewbot
    1. Tool: Pyflakes
      Processed Files:
          djblets/integrations/__init__.py
          djblets/integrations/tests/testcases.py
          djblets/integrations/errors.py
          djblets/integrations/tests/test_models.py
          djblets/integrations/hooks.py
          djblets/integrations/mixins.py
          djblets/integrations/views.py
          djblets/integrations/urls.py
          djblets/integrations/models.py
          djblets/integrations/forms.py
          djblets/integrations/middleware.py
          djblets/integrations/integration.py
          djblets/integrations/manager.py
          djblets/integrations/tests/test_integration.py
          djblets/integrations/app.py
          djblets/integrations/tests/test_manager.py
          djblets/integrations/tests/test_hooks.py
          djblets/integrations/tests/models.py
          djblets/staticbundles.py
      
      Ignored Files:
          djblets/integrations/templates/integrations/integration_list.html
          djblets/static/djblets/css/integrations.less
          djblets/integrations/templates/integrations/configure_integration.html
          djblets/integrations/tests/__init__.py
      
      
    2. 
        
    reviewbot
    1. Tool: PEP8 Style Checker
      Processed Files:
          djblets/integrations/__init__.py
          djblets/integrations/tests/testcases.py
          djblets/integrations/errors.py
          djblets/integrations/tests/test_models.py
          djblets/integrations/hooks.py
          djblets/integrations/mixins.py
          djblets/integrations/views.py
          djblets/integrations/urls.py
          djblets/integrations/models.py
          djblets/integrations/forms.py
          djblets/integrations/middleware.py
          djblets/integrations/integration.py
          djblets/integrations/manager.py
          djblets/integrations/tests/test_integration.py
          djblets/integrations/app.py
          djblets/integrations/tests/test_manager.py
          djblets/integrations/tests/test_hooks.py
          djblets/integrations/tests/models.py
          djblets/staticbundles.py
      
      Ignored Files:
          djblets/integrations/templates/integrations/integration_list.html
          djblets/static/djblets/css/integrations.less
          djblets/integrations/templates/integrations/configure_integration.html
          djblets/integrations/tests/__init__.py
      
      
    2. 
        
    david
    1. Ship It!
    2. 
        
    chipx86
    Review request changed
    Status:
    Completed
    Change Summary:
    Pushed to release-0.10.x (4ba3beb)