From dba443862980058007bbd59e858f9d43d5b7ecc0 Mon Sep 17 00:00:00 2001 From: Keenan Gugeler Date: Sun, 6 Mar 2022 23:35:18 -0500 Subject: [PATCH] Refactor contest join checks As suggested, merge the access checks into one. Some tests had to be changed, because the old checks didn't check the dates. --- judge/models/contest.py | 37 ++++++------- judge/models/tests/test_contest.py | 85 ++++++++++++----------------- judge/views/contests.py | 13 +++-- templates/contest/contest-tabs.html | 4 +- templates/contest/list.html | 5 +- 5 files changed, 64 insertions(+), 80 deletions(-) diff --git a/judge/models/contest.py b/judge/models/contest.py index 769c366ba3..f7797403db 100644 --- a/judge/models/contest.py +++ b/judge/models/contest.py @@ -1,3 +1,5 @@ +from typing import Optional + from django.core.exceptions import ValidationError from django.core.validators import MaxValueValidator, MinValueValidator, RegexValidator from django.db import models, transaction @@ -379,35 +381,30 @@ def access_check(self, user): return raise self.PrivateContest() - # Assumes the user can access, to avoid the cost again - def is_live_joinable_by(self, user): - if not self.started: - return False + def get_join_type(self, user) -> Optional[int]: + if self.ended: + return None # Virtual Join should not be LIVE or SPECTATE if not user.is_authenticated: - return False + return None if user.profile.id in self.editor_ids or user.profile.id in self.tester_ids: - return False + return ContestParticipation.SPECTATE - if self.has_completed_contest(user): - return False + if not self.started: + return None - if self.limit_join_organizations: - return self.join_organizations.filter(id__in=user.profile.organizations.all()).exists() - return True + if user.profile.id in self.spectator_ids: + return ContestParticipation.SPECTATE - # Also skips access check - def is_spectatable_by(self, user): - if not user.is_authenticated: - return False + if (self.limit_join_organizations and + not self.join_organizations.filter(id__in=user.profile.organizations.all()).exists()): + return None - if user.profile.id in self.editor_ids or user.profile.id in self.tester_ids: - return True + if self.has_completed_contest(user): + return ContestParticipation.SPECTATE - if self.limit_join_organizations: - return self.join_organizations.filter(id__in=user.profile.organizations.all()).exists() - return True + return ContestParticipation.LIVE def is_accessible_by(self, user): try: diff --git a/judge/models/tests/test_contest.py b/judge/models/tests/test_contest.py index 5be197e22b..12ecb1bf72 100644 --- a/judge/models/tests/test_contest.py +++ b/judge/models/tests/test_contest.py @@ -1,3 +1,5 @@ +from typing import Optional + from django.core.exceptions import ValidationError from django.test import SimpleTestCase, TestCase from django.utils import timezone @@ -319,8 +321,7 @@ def test_basic_contest_methods(self): 'superuser': { 'can_see_own_scoreboard': self.assertTrue, 'can_see_full_scoreboard': self.assertTrue, - 'is_live_joinable_by': self.assertFalse, # author - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertSpectate, # author 'is_accessible_by': self.assertTrue, 'is_editable_by': self.assertTrue, 'is_in_contest': self.assertFalse, @@ -328,8 +329,7 @@ def test_basic_contest_methods(self): 'staff_contest_edit_own': { 'can_see_own_scoreboard': self.assertTrue, 'can_see_full_scoreboard': self.assertTrue, - 'is_live_joinable_by': self.assertFalse, # author - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertSpectate, # author 'is_accessible_by': self.assertTrue, 'is_editable_by': self.assertTrue, 'is_in_contest': self.assertFalse, @@ -337,8 +337,7 @@ def test_basic_contest_methods(self): 'staff_contest_see_all': { 'can_see_own_scoreboard': self.assertTrue, 'can_see_full_scoreboard': self.assertTrue, - 'is_live_joinable_by': self.assertTrue, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertLive, 'is_accessible_by': self.assertTrue, 'is_editable_by': self.assertFalse, 'is_in_contest': self.assertFalse, @@ -346,8 +345,7 @@ def test_basic_contest_methods(self): 'staff_contest_edit_all': { 'can_see_own_scoreboard': self.assertTrue, 'can_see_full_scoreboard': self.assertTrue, - 'is_live_joinable_by': self.assertTrue, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertLive, 'is_accessible_by': self.assertTrue, 'is_editable_by': self.assertTrue, 'is_in_contest': self.assertFalse, @@ -356,8 +354,7 @@ def test_basic_contest_methods(self): # scoreboard checks don't do accessibility checks 'can_see_own_scoreboard': self.assertTrue, 'can_see_full_scoreboard': self.assertTrue, - 'is_live_joinable_by': self.assertTrue, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertLive, 'is_accessible_by': self.assertFalse, 'is_editable_by': self.assertFalse, 'is_in_contest': self.assertFalse, @@ -365,8 +362,7 @@ def test_basic_contest_methods(self): 'non_staff_tester': { 'can_see_own_scoreboard': self.assertTrue, 'can_see_full_scoreboard': self.assertTrue, - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertSpectate, 'is_accessible_by': self.assertTrue, 'is_editable_by': self.assertFalse, 'is_in_contest': self.assertFalse, @@ -374,8 +370,7 @@ def test_basic_contest_methods(self): 'anonymous': { 'can_see_own_scoreboard': self.assertTrue, 'can_see_full_scoreboard': self.assertTrue, - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertFalse, + 'get_join_type': self.assertCantJoin, 'is_accessible_by': self.assertFalse, 'is_editable_by': self.assertFalse, 'is_in_contest': self.assertFalse, @@ -454,8 +449,7 @@ def test_contest_hidden_scoreboard_contest_methods(self): 'normal_before_window': { 'can_see_own_scoreboard': self.assertFalse, 'can_see_full_scoreboard': self.assertFalse, - 'is_live_joinable_by': self.assertTrue, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertLive, 'has_completed_contest': self.assertFalse, }, 'normal_during_window': { @@ -466,15 +460,13 @@ def test_contest_hidden_scoreboard_contest_methods(self): 'normal_after_window': { 'can_see_own_scoreboard': self.assertTrue, 'can_see_full_scoreboard': self.assertFalse, - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertSpectate, 'has_completed_contest': self.assertTrue, }, 'non_staff_tester': { 'can_see_own_scoreboard': self.assertFalse, 'can_see_full_scoreboard': self.assertFalse, - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertSpectate, 'has_completed_contest': self.assertFalse, }, } @@ -588,32 +580,25 @@ def test_tester_see_scoreboard_contest_methods(self): def test_public_limit_organization_join_contest(self): data = { 'non_staff_tester': { - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertSpectate, }, 'non_staff_author': { - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertSpectate, }, 'staff_contest_edit_own': { - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertTrue, # curator + 'get_join_type': self.assertSpectate, # curator }, 'staff_contest_edit_all': { - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertFalse, + 'get_join_type': self.assertCantJoin, }, 'normal': { - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertFalse, + 'get_join_type': self.assertCantJoin, }, 'normal_open_org': { - 'is_live_joinable_by': self.assertTrue, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertLive, }, 'normal_after_window': { - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertFalse, # not in org + 'get_join_type': self.assertCantJoin, }, } self._test_object_methods_with_users(self.public_limit_organization_join_contest, data) @@ -660,8 +645,6 @@ def test_private_contest_methods(self): 'normal_open_org': { 'can_see_own_scoreboard': self.assertTrue, 'can_see_full_scoreboard': self.assertTrue, - 'is_live_joinable_by': self.assertTrue, - 'is_spectatable_by': self.assertTrue, 'is_accessible_by': self.assertTrue, 'is_editable_by': self.assertFalse, 'is_in_contest': self.assertFalse, @@ -717,8 +700,7 @@ def test_organization_private_contest_methods(self): 'normal': { 'can_see_own_scoreboard': self.assertTrue, 'can_see_full_scoreboard': self.assertTrue, - 'is_live_joinable_by': self.assertTrue, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertLive, 'is_accessible_by': self.assertTrue, 'is_editable_by': self.assertFalse, 'is_in_contest': self.assertFalse, @@ -745,8 +727,7 @@ def test_future_organization_private_contest_methods(self): 'staff_contest_edit_own': { 'can_see_own_scoreboard': self.assertFalse, 'can_see_full_scoreboard': self.assertFalse, - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertCantJoin, 'is_accessible_by': self.assertFalse, 'is_editable_by': self.assertFalse, 'is_in_contest': self.assertFalse, @@ -754,8 +735,7 @@ def test_future_organization_private_contest_methods(self): 'staff_contest_see_all': { 'can_see_own_scoreboard': self.assertTrue, 'can_see_full_scoreboard': self.assertTrue, - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertCantJoin, 'is_accessible_by': self.assertTrue, 'is_editable_by': self.assertFalse, 'is_in_contest': self.assertFalse, @@ -763,8 +743,7 @@ def test_future_organization_private_contest_methods(self): 'staff_contest_edit_all': { 'can_see_own_scoreboard': self.assertTrue, 'can_see_full_scoreboard': self.assertTrue, - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertCantJoin, 'is_accessible_by': self.assertTrue, 'is_editable_by': self.assertTrue, 'is_in_contest': self.assertFalse, @@ -772,8 +751,7 @@ def test_future_organization_private_contest_methods(self): 'normal': { 'can_see_own_scoreboard': self.assertTrue, 'can_see_full_scoreboard': self.assertTrue, - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertCantJoin, 'is_accessible_by': self.assertTrue, 'is_editable_by': self.assertFalse, 'is_in_contest': self.assertFalse, @@ -782,8 +760,7 @@ def test_future_organization_private_contest_methods(self): # False because contest has not begun 'can_see_own_scoreboard': self.assertFalse, 'can_see_full_scoreboard': self.assertFalse, - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertTrue, + 'get_join_type': self.assertSpectate, 'is_accessible_by': self.assertTrue, 'is_editable_by': self.assertFalse, 'is_in_contest': self.assertFalse, @@ -792,8 +769,7 @@ def test_future_organization_private_contest_methods(self): # False because contest has not begun 'can_see_own_scoreboard': self.assertFalse, 'can_see_full_scoreboard': self.assertFalse, - 'is_live_joinable_by': self.assertFalse, - 'is_spectatable_by': self.assertFalse, + 'get_join_type': self.assertCantJoin, 'is_accessible_by': self.assertFalse, 'is_editable_by': self.assertFalse, 'is_in_contest': self.assertFalse, @@ -1002,6 +978,15 @@ def test_virtual_participation(self): self.assertEqual(participation.start, participation.real_start) self.assertIsInstance(participation.end_time, timezone.datetime) + def assertLive(self, arg: Optional[int], msg: Optional[str] = None) -> None: + self.assertEqual(arg, ContestParticipation.LIVE, msg=msg) + + def assertSpectate(self, arg: Optional[int], msg: Optional[str] = None) -> None: + self.assertEqual(arg, ContestParticipation.SPECTATE, msg=msg) + + def assertCantJoin(self, arg: Optional[int], msg: Optional[str] = None) -> None: + self.assertIsNone(arg, msg=msg) + class ContestTagTestCase(TestCase): @classmethod diff --git a/judge/views/contests.py b/judge/views/contests.py index 551afc1b82..7a6e7793ee 100644 --- a/judge/views/contests.py +++ b/judge/views/contests.py @@ -122,6 +122,8 @@ def get_context_data(self, **kwargs): context['current_contests'] = present context['future_contests'] = future context['finished_contests'] = finished + context['LIVE'] = ContestParticipation.LIVE + context['SPECTATE'] = ContestParticipation.SPECTATE context['now'] = self._now context['first_page_href'] = '.' context['page_suffix'] = '#past-contests' @@ -276,6 +278,9 @@ def get_context_data(self, **kwargs): problem_count=Count('id'), ), ) + participation_type = self.object.get_join_type(self.request.user) + context['can_live_join'] = participation_type == ContestParticipation.LIVE + context['can_spectate'] = participation_type == ContestParticipation.SPECTATE return context @@ -380,13 +385,9 @@ def join_contest(self, request, access_code=None): break else: SPECTATE = ContestParticipation.SPECTATE - LIVE = ContestParticipation.LIVE - if contest.is_live_joinable_by(request.user): - participation_type = LIVE - elif contest.is_spectatable_by(request.user): - participation_type = SPECTATE - else: + participation_type = contest.get_join_type(request.user) + if participation_type is None: return generic_message(request, _('Cannot enter'), _('You are not able to join this contest.')) try: diff --git a/templates/contest/contest-tabs.html b/templates/contest/contest-tabs.html index eb8366df90..3cf2a79b34 100644 --- a/templates/contest/contest-tabs.html +++ b/templates/contest/contest-tabs.html @@ -58,7 +58,7 @@ {{- _('Leave contest') -}} {% endif %}"> - {% elif contest.is_live_joinable_by(request.user) %} + {% elif can_live_join %}
{% csrf_token %} @@ -66,7 +66,7 @@ class="contest-join{% if not has_joined %} first-join{% endif %}" value="{{ _('Join contest') }}">
- {% elif contest.is_spectatable_by(request.user) %} + {% elif can_spectate %}
{% csrf_token %} diff --git a/templates/contest/list.html b/templates/contest/list.html index 9dbc8ed950..17f69f6cc6 100644 --- a/templates/contest/list.html +++ b/templates/contest/list.html @@ -131,13 +131,14 @@ {% macro contest_join(contest, request, finished_contests) %} {% if not request.in_contest %} - {% if contest.is_live_joinable_by(request.user) %} + {% set participation_type = contest.get_join_type(request.user) %} + {% if participation_type == LIVE %} {% csrf_token %}
- {% elif contest.is_spectatable_by(request.user) %} + {% elif participation_type == SPECTATE %}
{% csrf_token %}