Merge branch 'hotfix/duplicate-bookmarks-and-submissions' into develop

server/assignments/schema/mutations.py

@@ -1,4 +1,5 @@
 import graphene
+from django.core.exceptions import MultipleObjectsReturned
 from graphene import relay
 from graphql_relay import from_global_id
 from rest_framework.exceptions import PermissionDenied
@@ -22,7 +23,13 @@ class UpdateAssignment(relay.ClientIDMutation):
     def mutate_and_get_payload(cls, root, info, **kwargs):
         assignment_data = kwargs.get('assignment')
         assignment = get_object(Assignment, assignment_data.get('id'))
-        (submission, created) = assignment.submissions.get_or_create(student=info.context.user)
+
+        try:
+            (submission, _created) = assignment.submissions.get_or_create(student=info.context.user)
+        except MultipleObjectsReturned:
+            for submission in assignment.submissions.filter(student=info.context.user):
+                submission.delete()
+            submission = assignment.submissions.create(student=info.context.user)
         submission.text = assignment_data.get('answer', '')
         submission.document = assignment_data.get('document', '')
         final = assignment_data.get('final')

server/assignments/tests/test_duplicate_student_submissions.py

@@ -0,0 +1,60 @@
+from django.test import TestCase, RequestFactory
+from graphene.test import Client
+from graphql_relay import to_global_id
+
+from api.schema import schema
+from assignments.factories import AssignmentFactory
+from assignments.models import StudentSubmission
+from users.models import User
+from users.services import create_users
+
+UPDATE_ASSIGNMENT_MUTATION = """
+mutation UpdateAssignment($input: UpdateAssignmentInput!) {
+  updateAssignment(input: $input) {
+    updatedAssignment {      
+      id
+      submission {
+        id
+        text
+        final
+      }
+    }
+  }
+}
+"""
+
+
+class DuplicateStudentSubmissionsTestCase(TestCase):
+    def setUp(self):
+        create_users()
+
+        self.student1 = User.objects.get(username='student1')
+        self.assignment = AssignmentFactory()
+        self.assignment_id = to_global_id('AssignmentNode', self.assignment.id)
+
+        # create 2 submissions, can happen if multiple requests arrive simultaneously
+        StudentSubmission.objects.create(assignment=self.assignment, student=self.student1, text='abc')
+        StudentSubmission.objects.create(assignment=self.assignment, student=self.student1, text='abcd')
+
+        request = RequestFactory().get('/')
+        request.user = self.student1
+        self.client = Client(schema=schema, context_value=request)
+
+    def test_returns_one_submission(self):
+        query = UPDATE_ASSIGNMENT_MUTATION
+        answer = "Abcdefg"
+        variables = {
+            "input": {
+                "assignment": {
+                    "answer": answer,
+                    "document": "",
+                    "final": True,
+                    "id": self.assignment_id
+                }
+            }
+        }
+
+        result = self.client.execute(query, variables=variables)
+        self.assertIsNone(result.get('errors'))
+        self.assertEquals(
+            result.get('data').get('updateAssignment').get('updatedAssignment').get('submission').get('text'), answer)

server/core/settings.py

@@ -290,9 +290,6 @@ LOGGING = {
         'simple_format': {
             'format': '%(levelname)s %(name)s: %(message)s'
         },
-        'debug_format': {
-            'format': '__debug__ %(levelname)s %(name)s: %(message)s'
-        }
     },
     'disable_existing_loggers': True,
     'handlers': {
@@ -303,41 +300,19 @@ LOGGING = {
         'console': {
             'level': 'INFO',
             'class': 'logging.StreamHandler',
+            'stream': sys.stdout,
             'formatter': 'simple_format'
         },
-        'debug_console': {
-            'level': 'DEBUG',
-            'class': 'logging.StreamHandler',
-            'formatter': 'debug_format'
-        }
     },
     'loggers': {
         '': {
-            'handlers': ['console'],
-            'level': 'WARNING'
-        },
-        'skillbox': {
             'handlers': ['console'],
             'level': 'INFO',
-            'propagate': False
-        },
-        'graphql': {
-            'handlers': ['console'],
-            'level': 'WARNING',
-            'propagate': False
-        },
-        'django': {
-            'handlers': ['console'],
-            'level': 'WARNING'
-        },
-        'django.server': {
-            'handlers': ['console'],
-            'level': 'WARNING',
             'propagate': True,
         },
-        'wagtail': {
+        'django.security.DisallowedHost': {
-            'handlers': ['console'],
+            'handlers': ['mail_admins'],
-            'level': 'WARNING',
+            'level': 'CRITICAL',
             'propagate': False,
         },
     }
@@ -353,21 +328,14 @@ if not DEBUG and os.environ.get('SENTRY_DSN'):
         event['user'] = {'id': id}
         return event
 
+    environment = os.environ.get('SENTRY_ENV', 'localhost')
+
     sentry_sdk.init(
         dsn=os.environ.get('SENTRY_DSN'),
         integrations=[DjangoIntegration()],
         send_default_pii=True,
-        before_send=before_send
+        before_send=before_send,
-    )
+        environment=environment
-
-if not DEBUG and os.environ.get('SENTRY_DSN'):
-    import sentry_sdk
-    from sentry_sdk.integrations.django import DjangoIntegration
-
-    sentry_sdk.init(
-        dsn=os.environ.get('SENTRY_DSN'),
-        integrations=[DjangoIntegration()],
-        send_default_pii=True
     )
 
 RAVEN_DSN_JS = os.environ.get('RAVEN_DSN_JS', '')
@@ -423,6 +391,7 @@ TASKBASE_SUPERPASSWORD = os.environ.get("TASKBASE_SUPERPASSWORD")
 TASKBASE_BASEURL = os.environ.get("TASKBASE_BASEURL")
 ENABLE_SPELLCHECK = True if TASKBASE_BASEURL else False
 
+
 TEST_RUNNER = 'xmlrunner.extra.djangotestrunner.XMLTestRunner'
 TEST_OUTPUT_DIR = './test-reports/'
 TEST_OUTPUT_VERBOSE = 1

server/notes/migrations/0004_auto_20210322_1514.py

@@ -0,0 +1,31 @@
+# Generated by Django 2.2.19 on 2021-03-22 15:14
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('notes', '0003_instrumentbookmark'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='contentblockbookmark',
+            name='uuid',
+            field=models.UUIDField(),
+        ),
+        migrations.AlterField(
+            model_name='instrumentbookmark',
+            name='uuid',
+            field=models.UUIDField(),
+        ),
+        migrations.AddConstraint(
+            model_name='contentblockbookmark',
+            constraint=models.UniqueConstraint(fields=('uuid', 'content_block', 'user'), name='unique_content_bookmark_per_user'),
+        ),
+        migrations.AddConstraint(
+            model_name='instrumentbookmark',
+            constraint=models.UniqueConstraint(fields=('uuid', 'instrument', 'user'), name='unique_instrument_bookmark_per_user'),
+        ),
+    ]

server/notes/models.py

@@ -18,9 +18,14 @@ class Bookmark(models.Model):
 
 
 class ContentBlockBookmark(Bookmark):
-    uuid = models.UUIDField(unique=True)
+    uuid = models.UUIDField(unique=False)
     content_block = models.ForeignKey('books.ContentBlock', on_delete=models.CASCADE)
 
+    class Meta:
+        constraints = [
+            models.UniqueConstraint(fields=['uuid', 'content_block', 'user'], name='unique_content_bookmark_per_user')
+        ]
+
 
 class ModuleBookmark(Bookmark):
     module = models.ForeignKey('books.Module', on_delete=models.CASCADE)
@@ -30,5 +35,10 @@ class ChapterBookmark(Bookmark):
     chapter = models.ForeignKey('books.Chapter', on_delete=models.CASCADE)
 
 class InstrumentBookmark(Bookmark):
-    uuid = models.UUIDField(unique=True)
+    uuid = models.UUIDField(unique=False)
     instrument = models.ForeignKey('basicknowledge.BasicKnowledge', on_delete=models.CASCADE)
+
+    class Meta:
+        constraints = [
+            models.UniqueConstraint(fields=['uuid', 'instrument', 'user'], name='unique_instrument_bookmark_per_user')
+        ]

server/notes/mutations.py

@@ -2,6 +2,8 @@ from builtins import PermissionError
 
 import graphene
 import json
+
+from django.db import IntegrityError
 from graphene import relay
 from graphql_relay import from_global_id
 
@@ -32,11 +34,15 @@ class UpdateContentBookmark(relay.ClientIDMutation):
         content_block = get_object(ContentBlock, content_block_id)
 
         if bookmarked:
-            ContentBlockBookmark.objects.create(
+            try:
-                content_block=content_block,
+                ContentBlockBookmark.objects.create(
-                uuid=uuid,
+                    content_block=content_block,
-                user=user
+                    uuid=uuid,
-            )
+                    user=user
+                )
+            except IntegrityError:
+                # already exists, probably fine
+                return cls(success=True)
         else:
             ContentBlockBookmark.objects.get(
                 content_block=content_block,
@@ -200,11 +206,15 @@ class UpdateInstrumentBookmark(relay.ClientIDMutation):
         instrument = BasicKnowledge.objects.get(slug=instrument_slug)
 
         if bookmarked:
-            InstrumentBookmark.objects.create(
+            try:
-                instrument=instrument,
+                InstrumentBookmark.objects.create(
-                uuid=uuid,
+                    instrument=instrument,
-                user=user
+                    uuid=uuid,
-            )
+                    user=user
+                )
+            except IntegrityError:
+                # already exists, that's probably ok
+                return cls(success=True)
         else:
             InstrumentBookmark.objects.get(
                 instrument=instrument,

server/notes/tests.py

@@ -1,3 +0,0 @@
-from django.test import TestCase
-
-# Create your tests here.

server/notes/tests/test_duplicate_bookmarks.py

@@ -0,0 +1,141 @@
+from django.db import transaction
+from django.test import TestCase, RequestFactory
+from graphene.test import Client
+from graphql_relay import to_global_id
+
+from api.schema import schema
+from books.factories import InstrumentFactory, ContentBlockFactory, ModuleFactory
+from notes.models import InstrumentBookmark, ContentBlockBookmark
+from users.models import User
+from users.services import create_users
+
+UPDATE_INSTRUMENT_BOOKMARK_MUTATION = """
+mutation UpdateInstrumentBookmark($input: UpdateInstrumentBookmarkInput!) {
+  updateInstrumentBookmark(input: $input) {
+    success
+    __typename
+  }
+}
+"""
+
+UPDATE_CONTENT_BLOCK_BOOKMARK_MUTATION = """
+mutation UpdateContentBookmark($input: UpdateContentBookmarkInput!) {
+  updateContentBookmark(input: $input) {
+    success
+  }
+}
+"""
+
+
+class DuplicateBookmarksTestCase(TestCase):
+    def setUp(self):
+        create_users()
+
+        self.student1 = User.objects.get(username='student1')
+        self.student2 = User.objects.get(username='student2')
+        self.instrument = InstrumentFactory(slug='instrument-slug')
+        module = ModuleFactory()
+        self.content_block = ContentBlockFactory(contents=[
+            {
+                'type': 'text_block',
+                'value': {
+                    'text': 'Ein Text'
+                }
+            }
+        ], module=module)
+        self.uuid = '56e45c9b-e0b2-4103-82e0-ad04ba05ac5b'
+        self.other_uuid = '56e45c9b-e0b2-4103-82e0-ad04ba05ac5c'
+        self.content_block_id = to_global_id('ContentBlockNode', self.content_block.id)
+
+        request = RequestFactory().get('/')
+        request.user = self.student1
+        self.client = Client(schema=schema, context_value=request)
+
+        request = RequestFactory().get('/')
+        request.user = self.student2
+        self.client2 = Client(schema=schema, context_value=request)
+
+    def _check_success(self, result, key):
+        self.assertIsNone(result.get('errors'))
+        self.assertTrue(
+            result.get('data').get(key).get('success'))
+
+    def test_two_students_bookmark_the_same_instrument_content(self):
+        variables = {
+            "input": {
+                "bookmarked": True,
+                "instrument": self.instrument.slug,
+                "uuid": self.uuid
+            }
+        }
+
+        result1 = self.client.execute(UPDATE_INSTRUMENT_BOOKMARK_MUTATION, variables=variables)
+        self._check_success(result1, 'updateInstrumentBookmark')
+        self.assertEqual(InstrumentBookmark.objects.count(), 1)
+
+        result2 = self.client2.execute(UPDATE_INSTRUMENT_BOOKMARK_MUTATION, variables=variables)
+        self._check_success(result2, 'updateInstrumentBookmark')
+        self.assertEqual(InstrumentBookmark.objects.count(), 2)
+
+    def test_two_students_bookmark_the_same_content_block_content(self):
+        variables = {
+            "input": {
+                "bookmarked": True,
+                "contentBlock": self.content_block_id,
+                "uuid": self.other_uuid
+            }
+        }
+
+        result1 = self.client.execute(UPDATE_CONTENT_BLOCK_BOOKMARK_MUTATION, variables=variables)
+        self._check_success(result1, 'updateContentBookmark')
+        self.assertEqual(ContentBlockBookmark.objects.count(), 1)
+
+        result2 = self.client2.execute(UPDATE_CONTENT_BLOCK_BOOKMARK_MUTATION, variables=variables)
+        self._check_success(result2, 'updateContentBookmark')
+        self.assertEqual(ContentBlockBookmark.objects.count(), 2)
+
+    def test_returns_one_instrument_bookmark(self):
+        # create 2 bookmarks for same user and content
+        InstrumentBookmark.objects.create(
+            instrument=self.instrument,
+            uuid=self.uuid,
+            user=self.student1
+        )
+
+        variables = {
+            "input": {
+                "bookmarked": True,
+                "instrument": self.instrument.slug,
+                "uuid": self.uuid
+            }
+        }
+
+        # need to do this because of a django test quirk, see:
+        # https://stackoverflow.com/questions/21458387/transactionmanagementerror-you-cant-execute-queries-until-the-end-of-the-atom
+        with transaction.atomic():
+            result = self.client.execute(UPDATE_INSTRUMENT_BOOKMARK_MUTATION, variables=variables)
+        self._check_success(result, 'updateInstrumentBookmark')
+        self.assertEqual(InstrumentBookmark.objects.count(), 1)
+
+    def test_returns_one_content_block_bookmark(self):
+        # create 2 bookmarks for same user and content
+        ContentBlockBookmark.objects.create(
+            content_block=self.content_block,
+            uuid=self.other_uuid,
+            user=self.student1
+        )
+
+        variables = {
+            "input": {
+                "bookmarked": True,
+                "contentBlock": self.content_block_id,
+                "uuid": self.other_uuid
+            }
+        }
+
+        # need to do this because of a django test quirk, see:
+        # https://stackoverflow.com/questions/21458387/transactionmanagementerror-you-cant-execute-queries-until-the-end-of-the-atom
+        with transaction.atomic():
+            result = self.client.execute(UPDATE_CONTENT_BLOCK_BOOKMARK_MUTATION, variables=variables)
+        self._check_success(result, 'updateContentBookmark')
+        self.assertEqual(ContentBlockBookmark.objects.count(), 1)