Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add created_by to record list #708

Merged
merged 7 commits into from
Sep 12, 2018
Merged

Add created_by to record list #708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
5c36316
Add created_by username to record list
pcaisse Sep 4, 2018
e963d0e
Add test for created_by
pcaisse Sep 7, 2018
76e6230
Add created by field to frontend
pcaisse Sep 7, 2018
93acba4
Only show "Create by" field to admins and analysts
pcaisse Sep 10, 2018
61399ce
Add to test to ensure public clients do not see "Created by" field
pcaisse Sep 10, 2018
b3ed4d6
Use email address but fall back to username if not available
pcaisse Sep 10, 2018
2cb3206
Separate out created_by tests
pcaisse Sep 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion app/data/serializers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@
import datetime
import pytz

from rest_framework.serializers import (ModelSerializer, SerializerMethodField, ValidationError)
from rest_framework.serializers import (
CharField,
ModelSerializer,
SerializerMethodField,
ValidationError,
)

from ashlar import serializers
from ashlar import serializer_fields
Expand Down Expand Up @@ -48,6 +53,14 @@ def filter_details_only(self, key, value):
raise serializer_fields.DropJsonKeyException


class DetailsReadOnlyRecordNonPublicSerializer(DetailsReadOnlyRecordSerializer):
"""
Serialize records with only read-only fields included plus non-public fields
(only available to admins and analysts)
"""
created_by = CharField()
Copy link
Contributor

@rmartz rmartz Sep 11, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit worried that having multiple serializer classes for minor differences in response is a bit of an antipatern. Some day I'd like to see what it'd take to consolidate our ReadOnly/Write sererializers, but for this from reading online I think we can add the field dynamically in __init__():

def __init__(self, *args, **kwargs):
    super(OrderSerializer, self).__init__(*args, **kwargs)
 
    if is_admin_or_writer(self.context['request'].user):
        self.fields['created_by']  = CharField()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm having trouble getting this working (self.context is empty). I'm also not entirely sure I like the idea of having fewer serializers whose fields we change dynamically. Seems like it might be hard to understand and maintain.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't realize that VSCode publishes PR comments immediately and edited my comment after I posted. Tested afterwards and self.context['request'] should work.

I may be overly cautious from getting burned by creating subclasses and discovering months later that their pivot factors weren't as fundamental as I originally though. My concern is that class inheritance doesn't scale the way we'd likely need for controlling the fields in a given response. Inheritance forces a linear decision tree, where the process of selecting a serializer class must follow down one pre-determined branch first, then the next, etc. It works well if each decision is a categorical distinction (Like a JSON parser class vs CSV parser class), but as the decisions become less categorical and more business logic it raises the risk that you have conflicting decision points.

Like if we want to add another level of controlling a field based on a factor orthogonal to user role (Such as a feature flag, perhaps), it isn't clear how the levels would be structured - worst case, if they are truly independent we'd need a cartesian product of serializer classes. In that case, although it's decidedly less elegant, I think having fields dynamically determined in code would be the only maintainable solution.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All that said, it's not like doing class inheritance or dynamic fields for this PR specifically is an inflection point that we can't revisit later on. It's pretty squarely a Type 2 decision, so if we like class inheritance more for now then 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Late to this party but one thing I've noticed is that DRF sometimes gets in the way of making serializers too intelligent, so the "different classes plus switching logic in get_serializer_class" is often a good way to make DRF happy even if it sometimes feels a bit strange to have a profusion of serializer subclasses.



class DetailsReadOnlyRecordSchemaSerializer(serializers.RecordSchemaSerializer):
"""Serialize Schema with only read-only fields included"""
schema = serializer_fields.MethodTransformJsonField('make_read_only_schema')
Expand Down
51 changes: 46 additions & 5 deletions app/data/tests/test_views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,9 +35,15 @@ def set_up_admin_client(self):
self.admin_client.force_authenticate(user=self.admin)

def set_up_public_client(self):
self.public = User.objects.create_user('public', 'public@ashlar', 'public')
self.public.groups.add(Group.objects.get(name='public'))
self.public.save()
try:
self.public = User.objects.get(username='public')
except User.DoesNotExist:
self.public = User.objects.create_user('public', 'public@ashlar', 'public')
self.public.save()

if not self.public.groups.exists():
self.public.groups.add(Group.objects.get(name='public'))

self.public_client = APIClient()
self.public_client.force_authenticate(user=self.public)

Expand Down Expand Up @@ -105,13 +111,31 @@ def set_up_records(self):
location_text='Equator',
schema=self.schema)

def set_up_audit_log(self):
self.audit_log_entry1 = RecordAuditLogEntry.objects.create(
user=self.admin,
username=self.admin.username,
action=RecordAuditLogEntry.ActionTypes.CREATE,
record=self.record2,
record_uuid=self.record2.uuid,
)
# CREATE audit log entry where user has been deleted
self.audit_log_entry2 = RecordAuditLogEntry.objects.create(
user=None,
username='banana',
action=RecordAuditLogEntry.ActionTypes.CREATE,
record=self.record3,
record_uuid=self.record3.uuid,
)

class DriverRecordViewTestCase(APITestCase, ViewTestSetUpMixin):
def setUp(self):
super(DriverRecordViewTestCase, self).setUp()

self.set_up_admin_client()
self.set_up_public_client()
self.set_up_records()
self.set_up_audit_log()
self.factory = APIRequestFactory()

def test_toddow(self):
Expand Down Expand Up @@ -156,6 +180,21 @@ def test_arbitrary_filters(self):
response_data2 = json.loads(self.admin_client.get(url2).content)
self.assertEqual(len(response_data2), 2)

def test_created_by_admin_client_email(self):
url = '/api/records/{uuid}/?details_only=True'.format(uuid=self.record2.uuid)
response_data = json.loads(self.admin_client.get(url).content)
self.assertEqual(response_data['created_by'], self.audit_log_entry1.user.email)

def test_created_by_admin_client_username(self):
url = '/api/records/{uuid}/?details_only=True'.format(uuid=self.record3.uuid)
response_data = json.loads(self.admin_client.get(url).content)
self.assertEqual(response_data['created_by'], self.audit_log_entry2.username)

def test_created_by_public_client(self):
url = '/api/records/?details_only=True'
public_response_data = json.loads(self.public_client.get(url).content)
self.assertTrue(all('created_by' not in result for result in public_response_data['results']))

def test_tilekey_param(self):
"""Ensure that the tilekey param stores a SQL query in Redis and returns an access token"""
# Since the call to store in redis won't have access to a real Redis instance under test,
Expand All @@ -178,16 +217,18 @@ def test_tilekey_param(self):

def test_get_serializer_class(self):
"""Test that get_serializer_class returns read-only serializer correctly"""
read_only = User.objects.create_user('public', '[email protected]', 'public')
view = DriverRecordViewSet()
mock_req = mock.Mock(spec=Request)
mock_req.user = read_only
mock_req.user = self.public
view.request = mock_req
serializer_class = view.get_serializer_class()
self.assertEqual(serializer_class, DetailsReadOnlyRecordSerializer)

def test_audit_log_creation(self):
"""Test that audit logs are generated on create operations"""
# Start from clean slate
RecordAuditLogEntry.objects.all().delete()

url = '/api/records/'
post_data = {
'data': {
Expand Down
52 changes: 38 additions & 14 deletions app/data/views.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,16 +13,21 @@

from django.conf import settings
from django.db import transaction
from django.db.models import (Case,
When,
IntegerField,
DateTimeField,
CharField,
UUIDField,
Value,
Count,
Sum,
Q)
from django.db.models import (
Case,
CharField,
Count,
DateTimeField,
IntegerField,
OuterRef,
Q,
Subquery,
Sum,
UUIDField,
Value,
When,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 Glad I'm not the only one who prefers this over stalactite style.

)
from django.db.models.functions import Coalesce
from django_redis import get_redis_connection

from rest_framework import viewsets
Expand Down Expand Up @@ -60,7 +65,8 @@
from models import RecordAuditLogEntry, RecordDuplicate, RecordCostConfig
from serializers import (DriverRecordSerializer, DetailsReadOnlyRecordSerializer,
DetailsReadOnlyRecordSchemaSerializer, RecordAuditLogEntrySerializer,
RecordDuplicateSerializer, RecordCostConfigSerializer)
RecordDuplicateSerializer, RecordCostConfigSerializer,
DetailsReadOnlyRecordNonPublicSerializer)
import transformers
from driver import mixins

Expand Down Expand Up @@ -103,13 +109,31 @@ def get_serializer_class(self):
if details_only_param == 'True' or details_only_param == 'true':
requested_details_only = True

if is_admin_or_writer(self.request.user) and not requested_details_only:
return DriverRecordSerializer
if is_admin_or_writer(self.request.user):
if requested_details_only:
return DetailsReadOnlyRecordNonPublicSerializer
else:
return DriverRecordSerializer
return DetailsReadOnlyRecordSerializer

def get_queryset(self):
"""Override default model ordering"""
qs = super(DriverRecordViewSet, self).get_queryset()
if self.get_serializer_class() is DetailsReadOnlyRecordNonPublicSerializer:
# Add in `created_by` field for user who created the record
created_by_query = (
RecordAuditLogEntry.objects.filter(
record=OuterRef('pk'),
action=RecordAuditLogEntry.ActionTypes.CREATE
)
.annotate(
# Fall back to username if the user has been deleted
email_or_username=Coalesce('user__email', 'username')
)
.values('email_or_username')
[:1]
)
qs = qs.annotate(created_by=Subquery(created_by_query, output_field=CharField()))
# Override default model ordering
return qs.order_by('-occurred_from')

def get_filtered_queryset(self, request):
Expand Down
1 change: 1 addition & 0 deletions web/app/i18n/en-us.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,6 +185,7 @@
"MODIFIED": "Modified",
"MODIFIED_BY": "Modified by",
"DATE_AND_TIME": "Date & Time",
"CREATED_BY": "Created by",
"DATE_FILTER": "Filter events by date of occurrence",
"CREATED_FILTER": "Filter events by date of record creation",
"DETAILS": "Details",
Expand Down
6 changes: 6 additions & 0 deletions web/app/scripts/views/record/list-partial.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@
{{ ::headerKey }}
</th>

<th ng-if="ctl.userCanWrite">{{ 'RECORD.CREATED_BY' | translate }}</th>

<!-- View/Edit links -->
<th></th>
</tr>
Expand All @@ -31,6 +33,10 @@
</driver-details-field>
</td>

<td ng-if="ctl.userCanWrite" class="created_by">
{{ ::record.created_by }}
</td>

<td class="links">
<a ng-click="ctl.showDetailsModal(record)">
<span class="glyphicon glyphicon-log-in"></span> {{ 'COMMON.VIEW' | translate }}
Expand Down