Notification-sending fixes (sio2project#311)

* Fix sending initial results notifications on CE

Move notification-sending logic to a separate function.
This also remedies a rare race condition, in which the user would get
a notification before the updated result is committed to the db.

* Fix the score leak in "submission judged" notifs

It was observable in the last hour of a round if TIME_ZONE
was set to "Europe/Warsaw".

oioioi/contests/handlers.py

@@ -146,19 +146,29 @@ def call_submission_judged(env, submission, **kwargs):
     else:
         assert contest.id == env['contest_id']
         contest.controller.submission_judged(submission, rejudged=env['is_rejudge'])
+    return env
+
 
-
+@transaction.atomic
+@_get_submission_or_skip
+def send_notification_judged(env, submission, kind='NORMAL', **kwargs):
     if submission.user is not None and not env['is_rejudge']:
+        message = "Submission %(submission_id)d by user %(username)s for problem %(short_name)s"
+        if kind == 'INITIAL':
+            notification = 'initial_results'
+            message += " got initial result."
+        else:
+            notification =  'submission_judged'
+            message += " was judged"
         logger.info(
-            "Submission %(submission_id)d by user %(username)s"
-            " for problem %(short_name)s was judged",
+            message,
             {
                 'submission_id': submission.pk,
                 'username': submission.user.username,
                 'short_name': submission.problem_instance.short_name,
             },
             extra={
-                'notification': 'submission_judged',
+                'notification': notification,
                 'user': submission.user,
                 'submission': submission,
             },

oioioi/problems/controllers.py

@@ -179,6 +179,10 @@ def judge(self, submission, extra_args=None, is_rejudge=False):
                     'call_submission_judged',
                     'oioioi.contests.handlers.call_submission_judged',
                 ),
+                (
+                    'send_notification_submission_judged',
+                    'oioioi.contests.handlers.send_notification_judged',
+                ),
                 (
                     'dump_final_env',
                     'oioioi.evalmgr.handlers.dump_env',

oioioi/programs/controllers.py

@@ -944,6 +944,15 @@ def fill_evaluation_environ_post_problem(self, environ, submission):
                     'oioioi.contests.handlers.update_submission_score',
                 ),
             )
+            add_before_placeholder(
+                environ,
+                'after_initial_tests',
+                (
+                    'send_notification_initial_tests_judged',
+                    'oioioi.contests.handlers.send_notification_judged',
+                    dict(kind='INITIAL'),
+                ),
+            )
 
     def get_submission_size_limit(self, problem_instance):
         return problem_instance.problem.controller.get_submission_size_limit(

oioioi/programs/handlers.py

@@ -578,23 +578,6 @@ def make_report(env, kind='NORMAL', save_scores=True, **kwargs):
         group_report.save()
         group_result['result_id'] = group_report.id
 
-    if kind == 'INITIAL':
-        if submission.user is not None and not env.get('is_rejudge', False):
-            logger.info(
-                "Submission %(submission_id)d by user %(username)s"
-                " for problem %(short_name)s got initial result.",
-                {
-                    'submission_id': submission.pk,
-                    'username': submission.user.username,
-                    'short_name': submission.problem_instance.short_name,
-                },
-                extra={
-                    'notification': 'initial_results',
-                    'user': submission.user,
-                    'submission': submission,
-                },
-            )
-
     return env
 
 

oioioi/programs/notifications.py

@@ -1,7 +1,6 @@
-from datetime import datetime, timezone  # pylint: disable=E0611
-
 from django.test import RequestFactory
 from django.urls import reverse
+from django.utils import timezone
 from django.utils.translation import gettext_noop
 
 from oioioi.base.notification import NotificationHandler
@@ -13,7 +12,7 @@ def notification_function_initial_results(arguments):
     request = RequestFactory().get('/', data={'name': u'test'})
     request.user = arguments.user
     request.contest = pi.contest
-    request.timestamp = datetime.now().replace(tzinfo=timezone.utc)
+    request.timestamp = timezone.now()
 
     # Check if any initial result is visible for user
     if not pi.controller.can_see_submission_status(request, arguments.submission):
@@ -54,7 +53,7 @@ def notification_function_submission_judged(arguments):
     request = RequestFactory().get('/', data={'name': u'test'})
     request.user = arguments.user
     request.contest = pi.contest
-    request.timestamp = datetime.now().replace(tzinfo=timezone.utc)
+    request.timestamp = timezone.now()
 
     # Check if the final report is visible to the user
     if not pi.controller.can_see_submission_score(

oioioi/programs/tests.py

@@ -1,7 +1,7 @@
 import os
 import re
 from collections import defaultdict
-from datetime import datetime, timezone  # pylint: disable=E0611
+from datetime import datetime, timezone, timedelta  # pylint: disable=E0611
 
 import pytest
 import six
@@ -17,6 +17,7 @@
 from django.urls import reverse
 from django.utils.html import escape, strip_tags
 from django.utils.http import urlencode
+from django.utils import timezone as django_timezone
 from oioioi.base.notification import NotificationHandler
 from oioioi.base.tests import (
     TestCase,
@@ -28,13 +29,13 @@
 from oioioi.base.utils.test_migrations import TestCaseMigrations
 from oioioi.contests.models import Contest, ProblemInstance, Round, Submission
 from oioioi.contests.scores import IntegerScore
-from oioioi.contests.handlers import call_submission_judged
+from oioioi.contests.handlers import send_notification_judged
 from oioioi.contests.tests import PrivateRegistrationController, SubmitMixin
 from oioioi.filetracker.tests import TestStreamingMixin
 from oioioi.problems.models import Problem
 from oioioi.programs import utils
 from oioioi.programs.controllers import ProgrammingContestController
-from oioioi.programs.handlers import make_report, collect_tests
+from oioioi.programs.handlers import collect_tests
 from oioioi.programs.models import (
     ModelSolution,
     ProblemAllowedLanguage,
@@ -518,6 +519,7 @@ class TestSubmission(TestCase, SubmitFileMixin):
     def setUp(self):
         self.assertTrue(self.client.login(username='test_user'))
 
+    @override_settings(TIME_ZONE="Europe/Warsaw")
     def test_submission_completed_notification(self):
         msg_count = defaultdict(int)
         messages = []
@@ -546,20 +548,35 @@ def fake_send_notification(
         environ['is_rejudge'] = False
         environ['submission_id'] = submission.pk
         environ['contest_id'] = submission.problem_instance.contest.id
-        call_submission_judged(environ)
-
-        environ['is_rejudge'] = True
-        call_submission_judged(environ)
+        send_notification_judged(environ)
 
         # Check if a notification for user 1001 was send
         # And user 1002 doesn't received a notification
+        self.assertEqual(len(messages), 1)
         self.assertEqual(msg_count['user_1001_notifications'], 1)
         self.assertEqual(msg_count['user_1002_notifications'], 0)
 
+        environ['is_rejudge'] = True
+        send_notification_judged(environ)
         self.assertEqual(len(messages), 1)
+
+        environ['is_rejudge'] = False
+        # Results barely not available, should catch timezone-related errors
+        Round.objects.update(
+            results_date=django_timezone.now()+timedelta(minutes=30)
+        )
+        send_notification_judged(environ)
+        self.assertEqual(len(messages), 1)
+
+        Round.objects.update(results_date=None)
+        send_notification_judged(environ)
+        self.assertEqual(len(messages), 1)
+
         self.assertIn('%(score)s', messages[0][0])
         self.assertIn('score', messages[0][1])
         self.assertEqual(messages[0][1]['score'], '34')
+        self.assertIn('was judged', messages[0][0])
+        self.assertNotIn('Initial result', messages[0][0])
 
         NotificationHandler.send_notification = send_notification_backup
 
@@ -899,6 +916,7 @@ class TestNotifications(TestCase):
 
     def test_initial_results_notification(self):
         msg_count = defaultdict(int)
+        messages = []
 
         @classmethod
         def fake_send_notification(
@@ -910,25 +928,38 @@ def fake_send_notification(
         ):
             if user.pk == 1001 and notification_type == 'initial_results':
                 msg_count['user_1001_notifications'] += 1
+            messages.append((notification_message, notificaion_message_arguments))
 
         send_notification_backup = NotificationHandler.send_notification
         NotificationHandler.send_notification = fake_send_notification
-        make_report(
-            {
-                'compilation_result': 'OK',
-                'submission_id': 1,
-                'status': 'OK',
-                'score': None,
-                'max_score': None,
-                'compilation_message': '',
-                'tests': {},
-                'rejudge': False,
-            },
-            'INITIAL',
-        )
+        environ = {
+            'compilation_result': 'OK',
+            'submission_id': 1,
+            'status': 'OK',
+            'score': None,
+            'max_score': None,
+            'compilation_message': '',
+            'tests': {},
+            'is_rejudge': False,
+        }
 
+        send_notification_judged(environ, 'INITIAL')
         # Check if a notification for user 1001 was sent
         self.assertEqual(msg_count['user_1001_notifications'], 1)
+
+        environ['status'] = 'CE'
+        send_notification_judged(environ, 'INITIAL')
+        # We should send a notif even for non-compiling submissions
+        self.assertEqual(msg_count['user_1001_notifications'], 2)
+
+        environ['is_rejudge'] = True
+        send_notification_judged(environ, 'INITIAL')
+        # No notification should be sent for a rejudge
+        self.assertEqual(msg_count['user_1001_notifications'], 2)
+
+        self.assertIn('Initial result', messages[0][0])
+        self.assertNotIn('was judged', messages[0][0])
+
         NotificationHandler.send_notification = send_notification_backup