Summary

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

Review Request #7947 — Created Feb. 6, 2016 and submitted April 1, 2016, 7:05 p.m.

Information

Owner

chipx86

Repository

Djblets

Branch

release-0.10.x

Bugs

Depends On

Reviewers

Groups

djblets

People

Description

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.

Testing Done

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.

Files

Issues

Description	From	Last Updated
local variable 'config3' is assigned to but never used	reviewbot	Feb. 6, 2016, 8:54 p.m.
local variable 'config4' is assigned to but never used	reviewbot	Feb. 6, 2016, 8:54 p.m.
local variable 'config2' is assigned to but never used	reviewbot	Feb. 6, 2016, 8:55 p.m.
local variable 'config3' is assigned to but never used	reviewbot	Feb. 6, 2016, 8:55 p.m.
local variable 'config4' is assigned to but never used	reviewbot	Feb. 6, 2016, 8:55 p.m.
Col: 5 E303 too many blank lines (2)	reviewbot	Feb. 6, 2016, 8:55 p.m.
local variable 'config3' is assigned to but never used	reviewbot	Feb. 6, 2016, 8:55 p.m.
local variable 'config1' is assigned to but never used	reviewbot	Feb. 6, 2016, 8:55 p.m.
local variable 'config3' is assigned to but never used	reviewbot	Feb. 6, 2016, 8:56 p.m.
local variable 'manager1' is assigned to but never used	reviewbot	Feb. 6, 2016, 8:56 p.m.
local variable 'manager2' is assigned to but never used	reviewbot	Feb. 6, 2016, 8:56 p.m.
For @property, don't we write the docstring as more of an attribute form than a method form?	david	Feb. 10, 2016, 4:10 p.m.
Maybe use logging.exception instead of formatting in e?	david	Feb. 10, 2016, 4:11 p.m.
Same here.	david	Feb. 10, 2016, 4:11 p.m.
I think it would be better to say "An example would be when..." rather than "This is useful, for instance, …	david	Feb. 10, 2016, 4:11 p.m.
", though," doesn't add anything.	david	Feb. 10, 2016, 4:12 p.m.
Is this the best order to tear things down? It seems like it should be in the reverse order from …	david	March 5, 2016, 7:14 p.m.
You don't need to format in e when using logging.exception.	david	Feb. 10, 2016, 4:47 p.m.
Shouldn't this link to #?	david	Feb. 10, 2016, 4:47 p.m.
Can we translate the string with {% trans %} instead of gettext()?	david	Feb. 10, 2016, 4:52 p.m.
Doesn't dispatch return a response?	david	Feb. 10, 2016, 4:55 p.m.
Same here.	david	Feb. 10, 2016, 4:55 p.m.
And here.	david	Feb. 10, 2016, 4:55 p.m.
Should this use self.get_config?	david	Feb. 10, 2016, 5:02 p.m.
Missing "Returns:"	david	Feb. 10, 2016, 5:01 p.m.
And here.	david	Feb. 10, 2016, 5:01 p.m.
This file needs unicode_literals	david	March 7, 2016, 4:37 p.m.
This returns a response, not a request.	david	March 7, 2016, 4:37 p.m.
Still missing unicode_literals. Did you forget to git add?	david	March 30, 2016, 4:46 p.m.

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

djblets/integrations/tests/test_integration.py (Diff revision 1)
Show all issues
```
 local variable 'config3' is assigned to but never used
```
djblets/integrations/tests/test_integration.py (Diff revision 1)
Show all issues
```
 local variable 'config4' is assigned to but never used
```
djblets/integrations/tests/test_integration.py (Diff revision 1)
Show all issues
```
 local variable 'config2' is assigned to but never used
```
djblets/integrations/tests/test_integration.py (Diff revision 1)
Show all issues
```
 local variable 'config3' is assigned to but never used
```
djblets/integrations/tests/test_integration.py (Diff revision 1)
Show all issues
```
 local variable 'config4' is assigned to but never used
```
djblets/integrations/tests/test_integration.py (Diff revision 1)
Show all issues
```
Col: 5
 E303 too many blank lines (2)
```
djblets/integrations/tests/test_manager.py (Diff revision 1)
Show all issues
```
 local variable 'config3' is assigned to but never used
```
djblets/integrations/tests/test_manager.py (Diff revision 1)
Show all issues
```
 local variable 'config1' is assigned to but never used
```
djblets/integrations/tests/test_manager.py (Diff revision 1)
Show all issues
```
 local variable 'config3' is assigned to but never used
```
djblets/integrations/tests/test_manager.py (Diff revision 1)
Show all issues
```
 local variable 'manager1' is assigned to but never used
```
djblets/integrations/tests/test_manager.py (Diff revision 1)
Show all issues
```
 local variable 'manager2' is assigned to but never used
```

Change Summary:


Split the GenerationSynchronizer changes and docs into other review requests.
Fixed unused variables in the unit tests.

Description:

		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.
-
-		Documentation has been written on both consuming the integrations
-		framework and writing integrations.

Diff:

Revision 2 (+2387)

Show changes

	djblets/settings.py
	djblets/integrations/__init__.py
	djblets/integrations/app.py
	djblets/integrations/errors.py
	djblets/integrations/forms.py
	djblets/integrations/hooks.py
	djblets/integrations/integration.py
	djblets/integrations/manager.py
	15 more

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

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?

chipx86 Feb. 10, 2016, 5:02 p.m.

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

djblets/integrations/hooks.py (Diff revision 2)

Show all issues

Maybe use logging.exception instead of formatting in e?

djblets/integrations/hooks.py (Diff revision 2)
Show all issues
```
Same here.
```

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

djblets/integrations/integration.py (Diff revision 2)
Show all issues
```
", though," doesn't add anything.
```

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.

chipx86 Feb. 10, 2016, 5:02 p.m.

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?

djblets/integrations/manager.py (Diff revision 2)

Show all issues

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

chipx86 Feb. 10, 2016, 5:02 p.m.

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.

djblets/integrations/templates/integrations/configure_integration.html (Diff revision 2)
Show all issues
```
Shouldn't this link to #?
```
1. chipx86 Feb. 10, 2016, 5:02 p.m.
  Yep.

djblets/integrations/templates/integrations/configure_integration.html (Diff revision 2)

Show all issues

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

chipx86 Feb. 10, 2016, 5:02 p.m.

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

djblets/integrations/views.py (Diff revision 2)

Show all issues

Doesn't dispatch return a response?

chipx86 Feb. 10, 2016, 5:02 p.m.

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

djblets/integrations/views.py (Diff revision 2)
Show all issues
```
Same here.
```
djblets/integrations/views.py (Diff revision 2)
Show all issues
```
And here.
```

djblets/integrations/views.py (Diff revision 2)

Show all issues

Should this use self.get_config?

chipx86 Feb. 10, 2016, 5:02 p.m.

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.

djblets/integrations/views.py (Diff revision 2)
Show all issues
```
Missing "Returns:"
```
djblets/integrations/views.py (Diff revision 2)
Show all issues
```
And here.
```

Change Summary:


Fixed some docstrings.
Moved to logging.exception when sandboxing errors in BaseIntegrationsHook.
Removed BaseIntegrationConfigFormView.get_config().
Added keyword arguments to BaseIntegrationConfigFormView.get_config_query_kwargs.
Fixed the Delete link for integrations to use <a href="#">.

Diff:

Revision 3 (+2380)

Show changes

	djblets/settings.py
	djblets/integrations/__init__.py
	djblets/integrations/app.py
	djblets/integrations/errors.py
	djblets/integrations/forms.py
	djblets/integrations/hooks.py
	djblets/integrations/integration.py
	djblets/integrations/manager.py
	15 more

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

djblets/integrations/errors.py (Diff revision 3)
Show all issues
```
This file needs unicode_literals
```
djblets/integrations/views.py (Diff revision 3)
Show all issues
```
This returns a response, not a request.
```

Change Summary:


Added a missing unicode_literals
Fixed a docstring.

Diff:

Revision 4 (+2380)

Show changes

	djblets/staticbundles.py
	djblets/integrations/__init__.py
	djblets/integrations/app.py
	djblets/integrations/errors.py
	djblets/integrations/forms.py
	djblets/integrations/hooks.py
	djblets/integrations/integration.py
	djblets/integrations/manager.py
	15 more

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

djblets/integrations/errors.py (Diff revision 4)

Show all issues

Still missing unicode_literals. Did you forget to git add?

chipx86 March 30, 2016, 4:46 p.m.

Apparently I never published the fix to this :(

Change Summary:

Add a missing unicode_literals import.

Diff:

Revision 5 (+2382)

Show changes

	djblets/staticbundles.py
	djblets/integrations/__init__.py
	djblets/integrations/app.py
	djblets/integrations/errors.py
	djblets/integrations/forms.py
	djblets/integrations/hooks.py
	djblets/integrations/integration.py
	djblets/integrations/manager.py
	15 more

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

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-0.10.x (4ba3beb)