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

reviewbotreviewbot

local variable 'config4' is assigned to but never used

reviewbotreviewbot

local variable 'config2' is assigned to but never used

reviewbotreviewbot

local variable 'config3' is assigned to but never used

reviewbotreviewbot

local variable 'config4' is assigned to but never used

reviewbotreviewbot

Col: 5 E303 too many blank lines (2)

reviewbotreviewbot

local variable 'config3' is assigned to but never used

reviewbotreviewbot

local variable 'config1' is assigned to but never used

reviewbotreviewbot

local variable 'config3' is assigned to but never used

reviewbotreviewbot

local variable 'manager1' is assigned to but never used

reviewbotreviewbot

local variable 'manager2' is assigned to but never used

reviewbotreviewbot

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

daviddavid

Maybe use logging.exception instead of formatting in e?

daviddavid

Same here.

daviddavid

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

daviddavid

", though," doesn't add anything.

daviddavid

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

daviddavid

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

daviddavid

Shouldn't this link to #?

daviddavid

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

daviddavid

Doesn't dispatch return a response?

daviddavid

Same here.

daviddavid

And here.

daviddavid

Should this use self.get_config?

daviddavid

Missing "Returns:"

daviddavid

And here.

daviddavid

This file needs unicode_literals

daviddavid

This returns a response, not a request.

daviddavid

Still missing unicode_literals. Did you forget to git add?

daviddavid
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)