EDUCATOR-4638 | Add operating_user kwargs to DeferrableMixin.save()

Author: iloveagent57

CHANGELOG.rst

@@ -14,6 +14,11 @@ Change Log
 Unreleased
 ~~~~~~~~~~
 
+[0.9.4] - 2019-09-24
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+
+* Let the ``DeferrableMixin.save()`` method take an optional ``operating_user`` parameter.
+
 [0.9.3] - 2019-09-20
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 

super_csv/__init__.py

@@ -4,6 +4,6 @@
 
 from __future__ import absolute_import, unicode_literals
 
-__version__ = '0.9.3'
+__version__ = '0.9.4'
 
 default_app_config = 'super_csv.apps.SuperCSVConfig'  # pylint: disable=invalid-name

super_csv/mixins.py

@@ -17,7 +17,6 @@
 from celery_utils.logged_task import LoggedTask
 from crum import get_current_user
 from django.conf import settings
-from django.contrib.auth import get_user_model
 from django.db import DatabaseError, transaction
 from django.utils.translation import ugettext as _
 from six import text_type
@@ -81,13 +80,10 @@ class DeferrableMixin(object):
     Mixin that automatically commits data using celery.
 
     Subclasses should specify `size_to_defer` to tune when to
-    run the commit synchronously or asynchronously
+    run the commit synchronously or asynchronously.
 
     Subclasses must override get_unique_path to uniquely identify
-    this task
-
-    Subclasses can define a field `user_id` which will be loaded and saved in the CSVOperation.
-    Otherwise, the current request (if any) user will be used.
+    this task.
     """
     # if the number of rows is greater than size_to_defer,
     # run the task asynchonously. Otherwise, commit immediately.
@@ -97,9 +93,13 @@ class DeferrableMixin(object):
     def get_unique_path(self):
         raise NotImplementedError()
 
-    def save(self, op_name=None):
+    def save(self, operation_name=None, operating_user=None):
         """
         Save the state of this object to django storage.
+
+        Clients may pass an optional ``operating_user`` kwarg to
+        indicate the ``auth.User`` who is saving this operation state.
+        Otherwise, the current request's (if any) user will be recorded.
         """
         state = self.__dict__.copy()
         for k in list(state):
@@ -108,24 +108,20 @@ def save(self, op_name=None):
                 del state[k]
             elif isinstance(v, set):
                 state[k] = list(v)
+
         state['__class__'] = (self.__class__.__module__, self.__class__.__name__)
-        if not op_name:
-            op_name = 'stage' if self.can_commit else 'commit'
 
-        user_id = state.get('user_id')
-        if user_id:
-            user = get_user_model().objects.get(id=user_id)
-        else:
-            user = get_current_user()
+        if not operation_name:
+            operation_name = 'stage' if self.can_commit else 'commit'
 
         operation = CSVOperation.record_operation(
-                        self,
-                        self.get_unique_path(),
-                        op_name,
-                        json.dumps(state),
-                        original_filename=state.get('filename', ''),
-                        user=user,
-                    )
+            self,
+            self.get_unique_path(),
+            operation_name,
+            json.dumps(state),
+            original_filename=state.get('filename', ''),
+            user=operating_user or get_current_user(),
+        )
         return operation
 
     @classmethod

tests/test_csv.py

@@ -16,6 +16,9 @@
 
 
 class DummyProcessor(csv_processor.CSVProcessor):
+    """
+    Fixture class the inherits from CSVProcessor.
+    """
     max_file_size = 20
     columns = ['foo', 'bar']
     required_columns = ['foo', 'bar']
@@ -48,16 +51,44 @@ class DummyChecksumProcessor(csv_processor.ChecksumMixin, DummyProcessor):
 
 class DummyDeferrableProcessor(csv_processor.DeferrableMixin, DummyProcessor):
     size_to_defer = 1
+    test_set = set()
+
+    def get_unique_path(self):
+        return 'test'
+
+
+USERNAME_FROM_SUBCLASS = 'user_specified_by_client'
+
+
+class DummyDeferrableProcessorSavingUser(csv_processor.DeferrableMixin, DummyProcessor):
+    """
+    Fixture that inherits from DeferrableMixin and overrides a save() method,
+    which calls the parent save() method with a non-null ``operating_user`` kwarg.
+    """
+    size_to_defer = 1
+
+    def save(self, operation_name=None, operating_user=None):
+        user = get_user_model().objects.get(username=USERNAME_FROM_SUBCLASS)
+        return super(DummyDeferrableProcessorSavingUser, self).save(
+            operation_name=operation_name,
+            operating_user=user,
+        )
 
     def get_unique_path(self):
         return 'test'
 
 
 @ddt.ddt
 class CSVTestCase(TestCase):
-    def setUp(self):
-        super(CSVTestCase, self).setUp()
-        self.dummy_csv = 'foo,bar\r\n1,1\r\n2,2\r\n'
+    """
+    Test class for CSVProcessor and DeferrableMixin classes.
+    """
+    @classmethod
+    def setUpTestData(cls):
+        super(CSVTestCase, cls).setUpTestData()
+        cls.dummy_csv = 'foo,bar\r\n1,1\r\n2,2\r\n'
+        cls.user = get_user_model().objects.create_user(username='testuser', password='12345')
+        cls.user_from_subclass = get_user_model().objects.create(username=USERNAME_FROM_SUBCLASS, password='12345')
 
     def tearDown(self):
         super(CSVTestCase, self).tearDown()
@@ -66,7 +97,7 @@ def tearDown(self):
     def test_write(self):
         buf = io.StringIO()
         processor = DummyProcessor(dummy_arg=True)
-        assert processor.dummy_arg is True
+        assert processor.dummy_arg is True  # pylint: disable=no-member
         processor.write_file(buf)
         data = buf.getvalue()
         assert data == self.dummy_csv
@@ -158,14 +189,20 @@ def test_defer_too_small(self):
         assert operation is not None
 
     @mock.patch('super_csv.mixins.get_current_user')
-    def test_user(self, patch_get_user):
-        user = get_user_model().objects.create_user(username='testuser', password='12345')
-        buf = ContentFile(self.dummy_csv)
+    def test_operating_user_is_recorded_from_request(self, patch_get_user):
         processor = DummyDeferrableProcessor()
-        patch_get_user.side_effect = (user, None, None)
-        processor.user_id = user.id
-        processor.process_file(buf)
+        patch_get_user.return_value = self.user
+        processor.process_file(ContentFile(self.dummy_csv))
+        csv_operations = models.CSVOperation.objects.all()
+        assert len(csv_operations) == 3
+        for csv_operation in csv_operations:
+            assert csv_operation.user == self.user
+
+    @mock.patch('super_csv.mixins.get_current_user', return_value=None)
+    def test_operating_user_is_recorded_by_subclass(self, patch_get_user):  # pylint: disable=unused-argument
+        processor = DummyDeferrableProcessorSavingUser()
+        processor.process_file(ContentFile(self.dummy_csv))
         csv_operations = models.CSVOperation.objects.all()
         assert len(csv_operations) == 3
         for csv_operation in csv_operations:
-            assert csv_operation.user == user
+            assert csv_operation.user == self.user_from_subclass

tests/test_models.py

@@ -6,8 +6,6 @@
 
 from __future__ import absolute_import, unicode_literals
 
-from datetime import datetime, timedelta
-
 from django.conf import settings
 from django.test import TestCase
 from mock import patch