From 2c2251afbc5bd3efa23c81a403fe7fb5bb129339 Mon Sep 17 00:00:00 2001 From: Troy Sankey Date: Wed, 18 Oct 2023 12:58:13 -0400 Subject: [PATCH] refactor: split assignment serializer into admin vs. non-admin This gives the benefit of allowing us to remove duplicate code (in-memory calculations of recent_action + learner_state). ENT-7780 --- .../apps/api/serializers/__init__.py | 5 +- .../content_assignments/assignment.py | 40 ++++++++---- .../apps/api/v1/tests/test_allocation_view.py | 20 ------ .../api/v1/tests/test_assignment_views.py | 5 -- .../views/content_assignments/assignments.py | 4 ++ .../content_assignments/assignments_admin.py | 8 +-- .../apps/content_assignments/models.py | 63 +------------------ 7 files changed, 41 insertions(+), 104 deletions(-) diff --git a/enterprise_access/apps/api/serializers/__init__.py b/enterprise_access/apps/api/serializers/__init__.py index ec7958a31..52b54ece1 100644 --- a/enterprise_access/apps/api/serializers/__init__.py +++ b/enterprise_access/apps/api/serializers/__init__.py @@ -1,7 +1,10 @@ """ API serializers module. """ -from .content_assignments.assignment import LearnerContentAssignmentResponseSerializer +from .content_assignments.assignment import ( + LearnerContentAssignmentAdminResponseSerializer, + LearnerContentAssignmentResponseSerializer +) from .content_assignments.assignment_configuration import ( AssignmentConfigurationCreateRequestSerializer, AssignmentConfigurationDeleteRequestSerializer, diff --git a/enterprise_access/apps/api/serializers/content_assignments/assignment.py b/enterprise_access/apps/api/serializers/content_assignments/assignment.py index 387e9b9fe..35b463dbb 100644 --- a/enterprise_access/apps/api/serializers/content_assignments/assignment.py +++ b/enterprise_access/apps/api/serializers/content_assignments/assignment.py @@ -43,9 +43,11 @@ class LearnerContentAssignmentRecentActionSerializer(serializers.Serializer): action_type = serializers.ChoiceField( help_text='Type of the recent action.', choices=AssignmentRecentActionTypes.CHOICES, + source='recent_action', ) timestamp = serializers.DateTimeField( help_text='Date and time when the action was taken.', + source='recent_action_time', ) @@ -60,18 +62,6 @@ class LearnerContentAssignmentResponseSerializer(serializers.ModelSerializer): help_text='All actions associated with this assignment.', many=True, ) - recent_action = LearnerContentAssignmentRecentActionSerializer( - help_text='Structured data about the most recent action. Meant to power a frontend table column.', - source='get_recent_action_data', - ) - learner_state = serializers.ChoiceField( - help_text=( - 'learner_state is an admin-facing dynamic state, not to be confused with `state`. Meant to power a ' - 'frontend table column.' - ), - choices=AssignmentLearnerStates.CHOICES, - source='get_learner_state', - ) class Meta: model = LearnerContentAssignment @@ -86,6 +76,32 @@ class Meta: 'transaction_uuid', 'last_notification_at', 'actions', + ] + read_only_fields = fields + + +class LearnerContentAssignmentAdminResponseSerializer(LearnerContentAssignmentResponseSerializer): + """ + A read-only Serializer for responding to requests for ``LearnerContentAssignment`` records FOR ADMINS. + + Important: This serializer depends on extra dynamic fields annotated by calling + ``LearnerContentAssignment.annotate_dynamic_fields_onto_queryset()`` on the assignment queryset. + """ + + recent_action = LearnerContentAssignmentRecentActionSerializer( + help_text='Structured data about the most recent action. Meant to power a frontend table column.', + source='*', + ) + learner_state = serializers.ChoiceField( + help_text=( + 'learner_state is an admin-facing dynamic state, not to be confused with `state`. Meant to power a ' + 'frontend table column.' + ), + choices=AssignmentLearnerStates.CHOICES, + ) + + class Meta(LearnerContentAssignmentResponseSerializer.Meta): + fields = LearnerContentAssignmentResponseSerializer.Meta.fields + [ 'recent_action', 'learner_state', ] diff --git a/enterprise_access/apps/api/v1/tests/test_allocation_view.py b/enterprise_access/apps/api/v1/tests/test_allocation_view.py index dc9fb86cd..30cc734aa 100644 --- a/enterprise_access/apps/api/v1/tests/test_allocation_view.py +++ b/enterprise_access/apps/api/v1/tests/test_allocation_view.py @@ -147,11 +147,6 @@ def test_allocate_happy_path(self, mock_allocate, mock_can_allocate): 'transaction_uuid': None, 'uuid': str(self.alice_assignment.uuid), 'actions': [], - 'recent_action': { - 'action_type': AssignmentRecentActionTypes.ASSIGNED, - 'timestamp': self.alice_assignment.created.strftime('%Y-%m-%dT%H:%M:%S.%fZ'), - }, - 'learner_state': None, }, ], 'created': [ @@ -166,11 +161,6 @@ def test_allocate_happy_path(self, mock_allocate, mock_can_allocate): 'transaction_uuid': None, 'uuid': str(self.bob_assignment.uuid), 'actions': [], - 'recent_action': { - 'action_type': AssignmentRecentActionTypes.ASSIGNED, - 'timestamp': self.bob_assignment.created.strftime('%Y-%m-%dT%H:%M:%S.%fZ'), - }, - 'learner_state': AssignmentLearnerStates.NOTIFYING, }, ], 'no_change': [ @@ -185,11 +175,6 @@ def test_allocate_happy_path(self, mock_allocate, mock_can_allocate): 'transaction_uuid': None, 'uuid': str(self.carol_assignment.uuid), 'actions': [], - 'recent_action': { - 'action_type': AssignmentRecentActionTypes.ASSIGNED, - 'timestamp': self.carol_assignment.created.strftime('%Y-%m-%dT%H:%M:%S.%fZ'), - }, - 'learner_state': AssignmentLearnerStates.NOTIFYING, }, ], } @@ -420,11 +405,6 @@ def test_allocate_happy_path_e2e( 'transaction_uuid': None, 'uuid': str(new_allocation.uuid), 'actions': [], - 'recent_action': { - 'action_type': AssignmentRecentActionTypes.ASSIGNED, - 'timestamp': new_allocation.created.strftime('%Y-%m-%dT%H:%M:%S.%fZ'), - }, - 'learner_state': AssignmentLearnerStates.NOTIFYING, }, ], 'no_change': [], diff --git a/enterprise_access/apps/api/v1/tests/test_assignment_views.py b/enterprise_access/apps/api/v1/tests/test_assignment_views.py index 081d867d8..1ac992db3 100644 --- a/enterprise_access/apps/api/v1/tests/test_assignment_views.py +++ b/enterprise_access/apps/api/v1/tests/test_assignment_views.py @@ -593,11 +593,6 @@ def test_retrieve(self, role_context_dict): } for action in self.requester_assignment_accepted.actions.order_by('completed_at') ], - 'recent_action': { - 'action_type': AssignmentRecentActionTypes.ASSIGNED, - 'timestamp': self.requester_assignment_accepted.created.strftime('%Y-%m-%dT%H:%M:%S.%fZ'), - }, - 'learner_state': None, } def test_retrieve_other_assignment_not_found(self): diff --git a/enterprise_access/apps/api/v1/views/content_assignments/assignments.py b/enterprise_access/apps/api/v1/views/content_assignments/assignments.py index df260e11f..6300fefa6 100644 --- a/enterprise_access/apps/api/v1/views/content_assignments/assignments.py +++ b/enterprise_access/apps/api/v1/views/content_assignments/assignments.py @@ -61,6 +61,10 @@ def get_queryset(self): """ A base queryset to list or retrieve ``LearnerContentAssignment`` records. In this viewset, only the assignments assigned to the requester are returned. + + Unlike in LearnerContentAssignmentAdminViewSet, here we are not going to annotate the extra dynamic fields using + `annotate_dynamic_fields_onto_queryset()`, so we will NOT serialize `learner_state` and `recent_action` for each + assignment. """ return LearnerContentAssignment.objects.filter( learner_email=self.requesting_user_email, diff --git a/enterprise_access/apps/api/v1/views/content_assignments/assignments_admin.py b/enterprise_access/apps/api/v1/views/content_assignments/assignments_admin.py index b351cf961..e747f2121 100644 --- a/enterprise_access/apps/api/v1/views/content_assignments/assignments_admin.py +++ b/enterprise_access/apps/api/v1/views/content_assignments/assignments_admin.py @@ -46,7 +46,7 @@ class LearnerContentAssignmentAdminViewSet( Viewset supporting all Admin CRUD operations on ``LearnerContentAssignment`` records. """ permission_classes = (permissions.IsAuthenticated,) - serializer_class = serializers.LearnerContentAssignmentResponseSerializer + serializer_class = serializers.LearnerContentAssignmentAdminResponseSerializer authentication_classes = (JwtAuthentication, authentication.SessionAuthentication) filter_backends = (filters.NoFilterOnDetailBackend, OrderingFilter) filterset_class = filters.LearnerContentAssignmentAdminFilter @@ -94,7 +94,7 @@ def get_queryset(self): tags=[CONTENT_ASSIGNMENT_ADMIN_CRUD_API_TAG], summary='Retrieve a content assignment by UUID.', responses={ - status.HTTP_200_OK: serializers.LearnerContentAssignmentResponseSerializer, + status.HTTP_200_OK: serializers.LearnerContentAssignmentAdminResponseSerializer, status.HTTP_404_NOT_FOUND: None, # TODO: test that this actually returns 404 instead of 403 on RBAC error. }, ) @@ -120,7 +120,7 @@ def list(self, request, *args, **kwargs): tags=[CONTENT_ASSIGNMENT_ADMIN_CRUD_API_TAG], summary='Cancel an assignment by UUID.', responses={ - status.HTTP_200_OK: serializers.LearnerContentAssignmentResponseSerializer, + status.HTTP_200_OK: serializers.LearnerContentAssignmentAdminResponseSerializer, status.HTTP_404_NOT_FOUND: None, status.HTTP_422_UNPROCESSABLE_ENTITY: None, }, @@ -150,7 +150,7 @@ def cancel(self, request, *args, uuid=None, **kwargs): # Serialize the assignment object obtained via get_queryset() instead of the one from the assignments_api. # Only the former has the additional dynamic fields annotated, and those are required for serialization. assignment_to_cancel.refresh_from_db() - response_serializer = serializers.LearnerContentAssignmentResponseSerializer(assignment_to_cancel) + response_serializer = serializers.LearnerContentAssignmentAdminResponseSerializer(assignment_to_cancel) return Response(response_serializer.data, status=status.HTTP_200_OK) else: return Response(status=status.HTTP_422_UNPROCESSABLE_ENTITY) diff --git a/enterprise_access/apps/content_assignments/models.py b/enterprise_access/apps/content_assignments/models.py index 710b5ad01..3aaf49902 100644 --- a/enterprise_access/apps/content_assignments/models.py +++ b/enterprise_access/apps/content_assignments/models.py @@ -294,66 +294,6 @@ def add_successful_reminded_action(self): completed_at=timezone.now(), ) - def get_recent_action_data(self): - """ - Return structured data about the most recent action, meant to feed the serializer. - - Recent action can ONLY be one of: - * AssignmentRecentActionTypes.REMINDED: Most recent ``reminded`` action. - * AssignmentRecentActionTypes.ASSIGNED: Assignment record creation event, if no reminded actions exist. - - Notes: - * These are not 1:1 with EITHER the AssignmentAction types, OR internal lifecycle states of assignments. - Instead it is a hybrid of both. - * This logic duplicates what is performed in annotate_dynamic_fields_onto_queryset() to annotate the - `recent_action` and `recent_action_time` fields on an assignment queryset. We rely on the serializer to - function even when the assignment object is not derived from a viewset's get_queryset(). - - Returns: - dict: { - 'action_type': , - 'timestamp':