Merge pull request #271 from 18F/master

Hair of Angel release

ereqs_admin/apps.py

@@ -1,7 +1,7 @@
 import logging
 
 from django.apps import AppConfig
-from django.db.models.signals import post_migrate
+from django.db.models.signals import m2m_changed, post_migrate, post_save
 
 logger = logging.getLogger(__name__)
 
@@ -17,8 +17,50 @@ def create_editors(**kwargs):
             content_type__app_label='reqs')
 
 
+def adminlog_post_save(sender, instance, **kwargs):
+    """Django's admin already logs when edits are made. Pass that along to our
+    logging system."""
+    # Can't load these at the top as this module is loaded before the apps are
+    # completely loaded
+    from django.contrib.admin.models import ADDITION, CHANGE, DELETION  # noqa
+    if instance.action_flag == ADDITION:
+        logger.info("%s created %s '%s'", instance.user.username,
+                    instance.content_type, instance.object_repr)
+    elif instance.action_flag == DELETION:
+        logger.info("%s deleted %s '%s'", instance.user.username,
+                    instance.content_type, instance.object_repr)
+    elif instance.action_flag == CHANGE:
+        logger.info("%s changed %s %s: '%s'", instance.user.username,
+                    instance.content_type, instance.object_repr,
+                    instance.get_change_message())
+
+
+def log_m2m_change(sender, instance, action, reverse, model, pk_set, **kwargs):
+    """Log changes for many-to-many fields, notably around permissions and
+    groups"""
+    model_name = model._meta.verbose_name_plural
+    instance_model = instance._meta.verbose_name
+    if action == 'post_add':
+        objects_added = list(model.objects.filter(pk__in=pk_set))
+        logger.info("%s given to %s '%s': %s", model_name, instance_model,
+                    instance, objects_added)
+    elif action == 'post_remove':
+        objects_added = list(model.objects.filter(pk__in=pk_set))
+        logger.info("%s removed from %s '%s': %s", model_name, instance_model,
+                    instance, objects_added)
+    elif action == 'post_clear':
+        logger.info("All %s removed from %s '%s'", model_name, instance_model,
+                    instance)
+
+
 class EreqsAdminConfig(AppConfig):
     name = 'ereqs_admin'
 
     def ready(self):
+        from django.contrib.auth.models import Group, User     # noqa
         post_migrate.connect(create_editors, sender=self)
+        post_save.connect(adminlog_post_save, sender='admin.LogEntry')
+        m2m_changed.connect(log_m2m_change, sender=User.groups.through)
+        m2m_changed.connect(log_m2m_change,
+                            sender=User.user_permissions.through)
+        m2m_changed.connect(log_m2m_change, sender=Group.permissions.through)

ereqs_admin/tests/apps_tests.py

@@ -28,3 +28,29 @@ def test_create_editors_once():
     assert Group.objects.count() == 1
     # three permissions per model
     assert Group.objects.first().permissions.count() == 3*4
+
+
+class MockLogger():
+    def __init__(self):
+        self.logs = []
+
+    def info(self, fmt, *args):
+        self.logs.append(fmt % args)    # noqa log format uses %s
+
+
+def test_admin_logging(monkeypatch, admin_client):
+    """Spot check that we receive logs around Group edits"""
+    monkeypatch.setattr(apps, 'logger', MockLogger())
+    admin_client.post('/admin/auth/group/add/', {
+        'name': 'Looking For', 'permissions': '1'})
+    group = Group.objects.get(name='Looking For')
+    admin_client.post('/admin/auth/group/{0}/change/'.format(group.pk), {
+        'name': 'Looking For Group', 'permissions': ['2']
+    })
+
+    logs = '\n'.join(apps.logger.logs)
+    assert "permissions given to group 'Looking For'" in logs
+    assert "admin created group 'Looking For'" in logs
+    assert "<Permission:" in logs
+    assert "Changed name" in logs
+    assert "permissions removed from group 'Looking For Group'" in logs

omb_eregs/settings.py

@@ -172,10 +172,16 @@
 LOGGING = {
     'version': 1,
     'disable_existing_loggers': False,
+    'formatters': {
+        'default': {
+            'format': '%(levelname)s %(asctime)s %(name)-20s %(message)s',
+        }
+    },
     'handlers': {
         'console': {
             'level': 'INFO',
             'class': 'logging.StreamHandler',
+            'formatter': 'default',
         }
     },
     'loggers': {

reqs/migrations/0024_auto_20170425_2317.py

@@ -0,0 +1,21 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11 on 2017-04-25 23:17
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+import django.db.models.deletion
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('reqs', '0023_auto_20170418_1751'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='requirement',
+            name='policy',
+            field=models.ForeignKey(on_delete=django.db.models.deletion.CASCADE, related_name='requirements', to='reqs.Policy'),
+        ),
+    ]

reqs/models.py

@@ -70,8 +70,8 @@ class Meta:
     policy_status = models.CharField(max_length=256, blank=True)
 
     def __str__(self):
-        text = self.title[:40]
-        if len(self.title) > 40:
+        text = self.title[:100]
+        if len(self.title) > 100:
             text += '...'
         if self.omb_policy_id:
             return '{0}: ({1}) {2}'.format(
@@ -83,7 +83,8 @@ class Requirement(models.Model):
     class Meta:
         ordering = ['req_id']
 
-    policy = models.ForeignKey(Policy, on_delete=models.CASCADE)
+    policy = models.ForeignKey(Policy, on_delete=models.CASCADE,
+                               related_name='requirements')
     req_id = models.CharField(max_length=16, unique=True)
     issuing_body = models.CharField(max_length=512)
     policy_section = models.CharField(max_length=1024, blank=True)

reqs/serializers.py

@@ -4,11 +4,14 @@
 
 
 class PolicySerializer(serializers.ModelSerializer):
+    total_reqs = serializers.IntegerField(read_only=True)
+    relevant_reqs = serializers.IntegerField(read_only=True)
+
     class Meta:
         model = Policy
         fields = (
             'policy_number', 'title', 'uri', 'omb_policy_id', 'policy_type',
-            'issuance', 'sunset', 'id'
+            'issuance', 'sunset', 'id', 'total_reqs', 'relevant_reqs',
         )
 
 

reqs/templates/admin/reqs/policy/change_form.html

@@ -18,7 +18,7 @@ <h2>Requirements</h2>
           </tr>
         </thead>
         <tbody>
-          {% for req in original.requirement_set.all %}
+          {% for req in original.requirements.all %}
           <tr class="form-row {% cycle "row1" "row2" %}">
             {% comment %}
               We'll let the req text be a bit longer in this view.

reqs/tests/views_tests.py

@@ -1,3 +1,5 @@
+from collections import namedtuple
+
 import pytest
 from model_mommy import mommy
 from rest_framework.test import APIClient
@@ -44,14 +46,14 @@ def test_requirement_filtering_topic(path, num_results):
 def test_requirements_queryset_order():
     """We should receive results in # of matches order"""
     client = APIClient()
-    for i in range(6):
-        mommy.make(Topic, name=str(i + 1)*4)
+    topics = [mommy.make(Topic, name=str(i + 1)*4) for i in range(6)]
     req1, req2, req3 = [mommy.make(Requirement, req_id=str(i + 1))
                         for i in range(3)]
     req1.topics.add('1111', '2222')
     req2.topics.add('2222', '3333', '4444')
     req3.topics.add('1111', '5555', '6666')
-    response = client.get('/requirements/?topics__name__in=1111,3333,4444')
+    param = ','.join(str(topics[i].pk) for i in (0, 2, 3))
+    response = client.get('/requirements/?topics__id__in=' + param)
     req_ids = [req['req_id'] for req in response.json()['results']]
     assert req_ids == ['2', '1', '3']
 
@@ -127,3 +129,84 @@ def test_requirements_ordered_by_bad_key(params):
     response = client.get(path)
     req_ids = [req['req_id'] for req in response.json()['results']]
     assert req_ids == ['0', '1', '2']
+
+
+PolicySetup = namedtuple('PolicySetup', ('topics', 'policies', 'reqs'))
+
+
+@pytest.fixture
+def policy_setup():
+    topics = mommy.make(Topic, _quantity=3)
+    policies = [mommy.make(Policy, policy_number='0'),
+                mommy.make(Policy, policy_number='1')]
+    reqs = [mommy.make(Requirement, policy=policies[0], _quantity=3),
+            mommy.make(Requirement, policy=policies[1], _quantity=4)]
+    reqs[0][0].topics.add(topics[0].name)
+    reqs[0][1].topics.add(topics[1].name)
+    reqs[0][2].topics.add(topics[0].name, topics[1].name)
+    reqs[1][0].topics.add(topics[1].name)
+    reqs[1][1].topics.add(topics[1].name, topics[2].name)
+    yield PolicySetup(topics, policies, reqs)
+
+
+@pytest.mark.django_db
+def test_policies_counts_no_params(policy_setup):
+    """The API endpoint should include all requirements when no params are
+    given"""
+    _, _, reqs = policy_setup
+    client = APIClient()
+
+    response = client.get("/policies/").json()
+    assert response['count'] == 2
+    assert response['results'][0]['total_reqs'] == len(reqs[0])
+    assert response['results'][0]['relevant_reqs'] == len(reqs[0])
+    assert response['results'][1]['total_reqs'] == len(reqs[1])
+    assert response['results'][1]['relevant_reqs'] == len(reqs[1])
+
+
+@pytest.mark.django_db
+def test_policies_counts_filter_req(policy_setup):
+    """The API endpoint should include only relevant policies when we filter
+    by an attribute of a requirement"""
+    _, _, reqs = policy_setup
+    client = APIClient()
+
+    path = "/policies/?requirements__req_id=" + reqs[1][1].req_id
+    response = client.get(path).json()
+    assert response['count'] == 1
+    assert response['results'][0]['total_reqs'] == len(reqs[1])
+    assert response['results'][0]['relevant_reqs'] == 1
+
+
+@pytest.mark.django_db
+def test_policies_counts_filter_by_one_topic(policy_setup):
+    """The API endpoint should include only relevant policies when we filter
+    by a single topic"""
+    topics, _, reqs = policy_setup
+    client = APIClient()
+
+    path = "/policies/?requirements__topics__id__in={0}".format(topics[0].pk)
+    response = client.get(path).json()
+    assert response['count'] == 1
+    assert response['results'][0]['total_reqs'] == len(reqs[0])
+    # reqs[0][0] and reqs[0][2]
+    assert response['results'][0]['relevant_reqs'] == 2
+
+
+@pytest.mark.django_db
+def test_policies_counts_filter_by_multiple_topics(policy_setup):
+    """The API endpoint should include only relevant policies when we filter
+    by multiple topics"""
+    topics, _, reqs = policy_setup
+    client = APIClient()
+
+    path = "/policies/?requirements__topics__id__in={0},{1}".format(
+        topics[0].pk, topics[2].pk)
+    response = client.get(path).json()
+    assert response['count'] == 2
+    assert response['results'][0]['total_reqs'] == len(reqs[0])
+    # reqs[0][0] and reqs[0][2]
+    assert response['results'][0]['relevant_reqs'] == 2
+    assert response['results'][1]['total_reqs'] == len(reqs[1])
+    # reqs[1][1]
+    assert response['results'][1]['relevant_reqs'] == 1