diff --git a/reviewboard/integrations/forms.py b/reviewboard/integrations/forms.py index ea13f43666e23350a08d56f9718a73bb6a554502..470a2324a2c5bf4d2f3263300a039c3e59149e7c 100644 --- a/reviewboard/integrations/forms.py +++ b/reviewboard/integrations/forms.py @@ -3,7 +3,9 @@ from __future__ import unicode_literals from django import forms +from django.utils import six from django.utils.translation import ugettext_lazy as _ +from djblets.forms.fields import ConditionsField from djblets.integrations.forms import (IntegrationConfigForm as DjbletsIntegrationConfigForm) @@ -20,14 +22,46 @@ class IntegrationConfigForm(DjbletsIntegrationConfigForm): Integrations should subclass this and provide additional fields that they want to display to the user. They must provide a :py:class:`Meta` class containing the fieldsets they want to display. + + Attributes: + limit_to_local_site (reviewboard.site.models.LocalSite): + The optional LocalSite to limit this configuration to. Any + configuration-related fields or logic that might need to be bound + to a LocalSite must make use of this. """ - model_fields = ( - DjbletsIntegrationConfigForm.model_fields + - ('local_site',) - ) + model_fields = DjbletsIntegrationConfigForm.model_fields + ('local_site',) local_site = forms.ModelChoiceField( label=_('Local Site'), queryset=LocalSite.objects.all(), required=False) + + def __init__(self, *args, **kwargs): + """Initialize the form. + + Args: + limit_to_local_site (reviewboard.site.models.LocalSite, optional): + The Local Site to limit configurations to. If ``None`` (or not + provided), the configuration's Local Site (or lack thereof) can + be specified by the user. + + *args (tuple): + Positional arguments to pass to the parent form. + + **kwargs (dict): + Keyword arguments to pass to the parent form. + """ + local_site = kwargs.pop('limit_to_local_site', None) + self.limit_to_local_site = local_site + + super(IntegrationConfigForm, self).__init__(*args, **kwargs) + + if local_site: + self.fields['local_site'].queryset = \ + LocalSite.objects.filter(pk=local_site.pk) + + # Limit LocalSites for all condition fields. + for field in six.itervalues(self.fields): + if isinstance(field, ConditionsField): + field.choice_kwargs['local_site'] = local_site diff --git a/reviewboard/integrations/models.py b/reviewboard/integrations/models.py index 018712571952a2e27aaf7b01be7505130c504378..43452995ec24ded3a8ffcb6a4ce9fb9d26f4019e 100644 --- a/reviewboard/integrations/models.py +++ b/reviewboard/integrations/models.py @@ -2,7 +2,10 @@ from __future__ import unicode_literals +import logging + from django.db import models +from djblets.conditions import ConditionSet from djblets.integrations.models import BaseIntegrationConfig from reviewboard.integrations.base import GetIntegrationManagerMixin @@ -22,3 +25,92 @@ class IntegrationConfig(GetIntegrationManagerMixin, BaseIntegrationConfig): related_name='integration_configs', blank=True, null=True) + + def load_conditions(self, form_cls, conditions_key='conditions'): + """Load a set of conditions from the configuration. + + This loads and deserializes a set of conditions from the configuration + stored in the provided key. Those conditions can then be matched by the + caller. + + If the conditions are not found, this will return ``None``. + + If the conditions cannot be deserialized, this will log some diagnostic + output and error messages and return ``None``. + + Args: + form_cls (type): + The configuration form class that owns the condition field. + + This will generally be ``my_integration.form_cls``, + but can be another form in more complex integrations. + + conditions_key (unicode, optional): + The key for the conditions data in the configuration. + Defaults to "conditions". + + Returns: + djblets.conditions.conditions.ConditionSet: + The condition set based on the data, if found and if it could be + loaded. Otherwise, ``None`` will be returned. + """ + conditions_data = self.get(conditions_key) + + if not conditions_data: + return None + + try: + return ConditionSet.deserialize( + form_cls.base_fields[conditions_key].choices, + conditions_data, + choice_kwargs={ + 'local_site': self.local_site, + }) + except: + logging.exception('Unable to load bad condition set data for ' + 'integration configuration ID=%s for key="%s"', + self.pk, conditions_key) + logging.debug('Bad conditions data = %r', conditions_data) + + return None + + def match_conditions(self, form_cls, conditions_key='conditions', + **match_kwargs): + """Filter configurations based on a review request. + + If the configuration contains a ``conditions`` key, and the + configuration form contains a matching field, this will check + the conditions for matches against the review request. + + Args: + form_cls (type): + The configuration form class that owns the condition field. + + This will generally be ``my_integration.form_cls``, + but can be another form in more complex integrations. + + conditions_key (unicode, optional): + The key for the conditions data in the configuration. + Defaults to "conditions". + + **match_kwargs (dict): + Keyword arguments to match for the conditions. + + Returns: + bool: + ``True`` if the specified conditions in the configuration matches + the provided keyword arguments. ``False`` if not. + """ + condition_set = self.load_conditions(form_cls, conditions_key) + + if condition_set: + try: + return condition_set.matches(**match_kwargs) + except Exception as e: + logging.exception( + 'Unexpected failure when matching conditions for ' + 'integration configuration ID=%s, config_key=%s, ' + 'match_kwargs=%r: %s', + self.pk, conditions_key, match_kwargs, e) + + return False diff --git a/reviewboard/integrations/tests/__init__.py b/reviewboard/integrations/tests/__init__.py new file mode 100644 index 0000000000000000000000000000000000000000..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391 diff --git a/reviewboard/integrations/tests/test_configs.py b/reviewboard/integrations/tests/test_configs.py new file mode 100644 index 0000000000000000000000000000000000000000..29008ec52f56c465b2b27c813306f2c40fc28b9c --- /dev/null +++ b/reviewboard/integrations/tests/test_configs.py @@ -0,0 +1,153 @@ +from __future__ import unicode_literals + +import logging + +from djblets.conditions import ConditionSet +from djblets.forms.fields import ConditionsField +from djblets.testing.decorators import add_fixtures +from kgb import SpyAgency + +from reviewboard.integrations.forms import IntegrationConfigForm +from reviewboard.integrations.models import IntegrationConfig +from reviewboard.reviews.conditions import ReviewRequestConditionChoices +from reviewboard.testing.testcase import TestCase + + +class MyConfigForm(IntegrationConfigForm): + my_conditions = ConditionsField( + choices=ReviewRequestConditionChoices) + + +class IntegrationConfigTests(SpyAgency, TestCase): + """Unit tests for reviewboard.integrations.models.IntegrationConfig.""" + + def test_load_conditions(self): + """Testing IntegrationConfig.load_conditions""" + config = IntegrationConfig() + config.settings['my_conditions'] = { + 'mode': 'all', + 'conditions': [ + { + 'choice': 'branch', + 'op': 'is', + 'value': 'master', + }, + { + 'choice': 'summary', + 'op': 'contains', + 'value': '[WIP]', + }, + ], + } + + condition_set = config.load_conditions(MyConfigForm, + conditions_key='my_conditions') + + self.assertEqual(condition_set.mode, ConditionSet.MODE_ALL) + + conditions = condition_set.conditions + self.assertEqual(len(conditions), 2) + + condition = conditions[0] + self.assertEqual(condition.choice.choice_id, 'branch') + self.assertEqual(condition.operator.operator_id, 'is') + self.assertEqual(condition.value, 'master') + + condition = conditions[1] + self.assertEqual(condition.choice.choice_id, 'summary') + self.assertEqual(condition.operator.operator_id, 'contains') + self.assertEqual(condition.value, '[WIP]') + + def test_load_conditions_with_empty(self): + """Testing IntegrationConfig.load_conditions with empty or missing + data + """ + config = IntegrationConfig() + config.settings['conditions'] = None + + self.assertIsNone(config.load_conditions(MyConfigForm)) + + def test_load_conditions_with_bad_data(self): + """Testing IntegrationConfig.load_conditions with bad data""" + config = IntegrationConfig() + config.settings['conditions'] = 'dfsafas' + + self.spy_on(logging.debug) + self.spy_on(logging.exception) + + self.assertIsNone(config.load_conditions(MyConfigForm)) + self.assertTrue(logging.debug.spy.called) + self.assertTrue(logging.exception.spy.called) + + @add_fixtures(['test_users']) + def test_match_conditions(self): + """Testing IntegrationConfig.match_conditions""" + config = IntegrationConfig() + config.settings['my_conditions'] = { + 'mode': 'all', + 'conditions': [ + { + 'choice': 'branch', + 'op': 'is', + 'value': 'master', + }, + { + 'choice': 'summary', + 'op': 'contains', + 'value': '[WIP]', + }, + ], + } + + review_request = self.create_review_request( + branch='master', + summary='[WIP] This is a test.') + + self.assertTrue(config.match_conditions( + MyConfigForm, + conditions_key='my_conditions', + review_request=review_request)) + + review_request = self.create_review_request( + branch='master', + summary='This is a test.') + + self.assertFalse(config.match_conditions( + MyConfigForm, + conditions_key='my_conditions', + review_request=review_request)) + + @add_fixtures(['test_users']) + def test_match_conditions_sandbox(self): + """Testing IntegrationConfig.match_conditions with exceptions + sandboxed + """ + config = IntegrationConfig() + config.settings['my_conditions'] = { + 'mode': 'all', + 'conditions': [ + { + 'choice': 'branch', + 'op': 'is', + 'value': 'master', + }, + { + 'choice': 'summary', + 'op': 'contains', + 'value': '[WIP]', + }, + ], + } + + self.create_review_request( + branch='master', + summary='[WIP] This is a test.') + + self.spy_on(logging.exception) + + self.assertFalse(config.match_conditions( + MyConfigForm, + conditions_key='my_conditions', + review_request='test')) + + self.assertTrue(logging.exception.spy.called) diff --git a/reviewboard/integrations/tests/test_forms.py b/reviewboard/integrations/tests/test_forms.py new file mode 100644 index 0000000000000000000000000000000000000000..92164f2a8c92dd1e3d3428c3006dd93ab19722aa --- /dev/null +++ b/reviewboard/integrations/tests/test_forms.py @@ -0,0 +1,69 @@ +from __future__ import unicode_literals + +from django.test import RequestFactory +from djblets.forms.fields import ConditionsField + +from reviewboard.integrations.base import Integration, get_integration_manager +from reviewboard.integrations.forms import IntegrationConfigForm +from reviewboard.reviews.conditions import ReviewRequestConditionChoices +from reviewboard.site.models import LocalSite +from reviewboard.testing.testcase import TestCase + + +class MyIntegration(Integration): + pass + + +class MyConfigForm(IntegrationConfigForm): + my_conditions = ConditionsField(ReviewRequestConditionChoices) + + +class IntegrationConfigFormTests(TestCase): + """Unit tests for reviewboard.integrations.forms.IntegrationConfigForm.""" + + def setUp(self): + super(IntegrationConfigFormTests, self).setUp() + + self.integration = MyIntegration(get_integration_manager()) + self.request = RequestFactory().request() + + def test_init(self): + """Testing IntegrationConfigForm initialization""" + form = MyConfigForm(self.integration, self.request) + + local_site_1 = LocalSite.objects.create(name='local-site-1') + local_site_2 = LocalSite.objects.create(name='local-site-2') + + local_site_ids = list( + form.fields['local_site'].queryset + .order_by('pk') + .values_list('pk', flat=True) + ) + + self.assertIsNone(form.limit_to_local_site) + self.assertEqual(local_site_ids, [local_site_1.pk, local_site_2.pk]) + self.assertEqual(form.fields['my_conditions'].choice_kwargs, {}) + + def test_init_with_limit_to_local_site(self): + """Testing IntegrationConfigForm initialization with + limit_to_local_site + """ + LocalSite.objects.create(name='local-site-1') + local_site = LocalSite.objects.create(name='local-site-2') + + form = MyConfigForm(self.integration, self.request, + limit_to_local_site=local_site) + + local_site_ids = list( + form.fields['local_site'].queryset + .order_by('pk') + .values_list('pk', flat=True) + ) + + self.assertEqual(form.limit_to_local_site, local_site) + self.assertEqual(local_site_ids, [local_site.pk]) + self.assertEqual( + form.fields['my_conditions'].choice_kwargs, + { + 'local_site': local_site, + }) diff --git a/reviewboard/integrations/tests/test_views.py b/reviewboard/integrations/tests/test_views.py new file mode 100644 index 0000000000000000000000000000000000000000..bc26fa3b8749e456e436d44e38d7d51bef6b8ba5 --- /dev/null +++ b/reviewboard/integrations/tests/test_views.py @@ -0,0 +1,50 @@ +from __future__ import unicode_literals + +from django.test import RequestFactory + +from reviewboard.integrations.base import Integration, get_integration_manager +from reviewboard.integrations.models import IntegrationConfig +from reviewboard.integrations.views import AdminIntegrationConfigFormView +from reviewboard.site.models import LocalSite +from reviewboard.testing.testcase import TestCase + + +class MyIntegration(Integration): + pass + + +class AdminIntegrationConfigFormViewTests(TestCase): + """Unit tests for AdminIntegrationConfigFormView.""" + + def setUp(self): + super(AdminIntegrationConfigFormViewTests, self).setUp() + + self.integration = MyIntegration(get_integration_manager()) + self.config = IntegrationConfig() + self.request = RequestFactory().request() + + # NOTE: integration and config are normally set in dispatch(), but + # we're not calling into all that, so we're taking advantage of + # the fact that Django's class-based generic views will set any + # attribute passed in during construction. + self.view = AdminIntegrationConfigFormView( + request=self.request, + integration=self.integration, + config=self.config) + + def test_get_form_kwargs(self): + """Testing AdminIntegrationConfigFormView.get_form_kwargs""" + form_kwargs = self.view.get_form_kwargs() + + self.assertIsNone(form_kwargs['limit_to_local_site']) + + def test_get_form_kwargs_with_local_site(self): + """Testing AdminIntegrationConfigFormView.get_form_kwargs with + LocalSite + """ + # This is normally set by LocalSiteMiddleware. + local_site = LocalSite.objects.create(name='local-site-1') + self.request.local_site = local_site + + form_kwargs = self.view.get_form_kwargs() + self.assertEqual(form_kwargs['limit_to_local_site'], local_site) diff --git a/reviewboard/integrations/views.py b/reviewboard/integrations/views.py index 2e600aee0a717a3eaa7f358a3b2d0b9fa5c417e3..f7e9d7e2ef77f015214a6d456d8f87f8cda496a8 100644 --- a/reviewboard/integrations/views.py +++ b/reviewboard/integrations/views.py @@ -24,6 +24,26 @@ class AdminIntegrationConfigFormView(GetIntegrationManagerMixin, """ return local_site_reverse('integration-list', request=self.request) + def get_form_kwargs(self): + """Return keyword arguments to pass to the form. + + This will, by default, provide ``integration`` and configuration + ``instance`` keyword arguments to the form during initialization, + along with the ``request`` and ``local_site`` (if any). + + Subclases can override it with additional arguments if needed. + + Returns: + dict: + A dictionary of keyword arguments to pass to the form. + """ + form_kwargs = \ + super(AdminIntegrationConfigFormView, self).get_form_kwargs() + form_kwargs['limit_to_local_site'] = \ + getattr(form_kwargs['request'], 'local_site', None) + + return form_kwargs + class AdminIntegrationListView(GetIntegrationManagerMixin, BaseAdminIntegrationListView): diff --git a/reviewboard/testing/testcase.py b/reviewboard/testing/testcase.py index 64bf3b64803c79ee1642e3aef7dee1e98f4c067a..e4afb10c68dda450683acbfa00e66d2a32f34ce4 100644 --- a/reviewboard/testing/testcase.py +++ b/reviewboard/testing/testcase.py @@ -412,7 +412,9 @@ class TestCase(FixturesCompilerMixin, DjbletsTestCase): summary='Test Summary', description='Test Description', testing_done='Testing', - submitter='doc', local_id=1001, + submitter='doc', + branch='my-branch', + local_id=1001, bugs_closed='', status='P', public=False, publish=False, commit_id=None, changenum=None, repository=None, id=None, @@ -450,6 +452,7 @@ class TestCase(FixturesCompilerMixin, DjbletsTestCase): review_request = ReviewRequest( summary=summary, description=description, + branch=branch, testing_done=testing_done, local_site=local_site, local_id=local_id,