From 28a93e16d8eb263adcaba0ac0ee40c6bf0452cd6 Mon Sep 17 00:00:00 2001 From: Benjamin Cutler Date: Tue, 12 Dec 2023 19:19:27 -0700 Subject: [PATCH] improve UI/UX for removing runs from the schedule without deleting them [#186669498] --- bundles/admin/scheduleEditor/index.js | 8 +- bundles/admin/scheduleEditor/speedrun.js | 3 +- bundles/public/orderTarget.tsx | 3 +- tests/test_speedrun.py | 178 ++++++++++++----------- tracker/admin/event.py | 6 +- tracker/models/event.py | 15 +- tracker/util.py | 19 +++ tracker/views/api.py | 4 +- tracker/views/commands.py | 61 ++++++-- 9 files changed, 186 insertions(+), 111 deletions(-) diff --git a/bundles/admin/scheduleEditor/index.js b/bundles/admin/scheduleEditor/index.js index cd4302d5f..ad3b3a07c 100644 --- a/bundles/admin/scheduleEditor/index.js +++ b/bundles/admin/scheduleEditor/index.js @@ -107,7 +107,9 @@ function dispatch(dispatch) { }, moveSpeedrun: (source, destination, before) => { dispatch(actions.models.setInternalModelField('speedrun', source, 'moving', true)); - dispatch(actions.models.setInternalModelField('speedrun', destination, 'moving', true)); + if (destination != null) { + dispatch(actions.models.setInternalModelField('speedrun', destination, 'moving', true)); + } dispatch(actions.models.setInternalModelField('speedrun', source, 'errors', null)); dispatch( actions.models.command({ @@ -122,7 +124,9 @@ function dispatch(dispatch) { }, always: () => { dispatch(actions.models.setInternalModelField('speedrun', source, 'moving', false)); - dispatch(actions.models.setInternalModelField('speedrun', destination, 'moving', false)); + if (destination != null) { + dispatch(actions.models.setInternalModelField('speedrun', destination, 'moving', false)); + } }, }), ); diff --git a/bundles/admin/scheduleEditor/speedrun.js b/bundles/admin/scheduleEditor/speedrun.js index 448b8a2a5..fcd9ba5aa 100644 --- a/bundles/admin/scheduleEditor/speedrun.js +++ b/bundles/admin/scheduleEditor/speedrun.js @@ -162,7 +162,7 @@ class Speedrun extends React.Component { }; nullOrder_ = () => { - this.props.saveField(this.props.speedrun, 'order', null); + this.props.moveSpeedrun(this.props.speedrun.pk, null, true); }; cancelEdit_ = () => { @@ -197,6 +197,7 @@ Speedrun.propTypes = { speedrun: SpeedrunShape.isRequired, draft: SpeedrunShape, moveSpeedrun: PropTypes.func, + updateField: PropTypes.func, saveField: PropTypes.func, saveModel: PropTypes.func, cancelEdit: PropTypes.func, diff --git a/bundles/public/orderTarget.tsx b/bundles/public/orderTarget.tsx index c0bd709fb..009095470 100644 --- a/bundles/public/orderTarget.tsx +++ b/bundles/public/orderTarget.tsx @@ -32,7 +32,8 @@ function OrderTarget({ {Target(false)} {nullOrder ? ( - ❌ + unordered + ) : null} diff --git a/tests/test_speedrun.py b/tests/test_speedrun.py index 61202c7bd..47030256c 100755 --- a/tests/test_speedrun.py +++ b/tests/test_speedrun.py @@ -1,5 +1,7 @@ import datetime +import itertools import random +from typing import Iterable, Optional, Union from unittest import skipIf import django @@ -10,6 +12,7 @@ import tracker.models as models from tracker import settings +from tracker.util import pairwise from . import randgen from .util import today_noon @@ -165,108 +168,121 @@ def setUp(self): name='Test Run 4', run_time='0:20:00', setup_time='0:05:00', order=None ) - def test_after_to_before(self): + def assertResults( + self, + moving: models.SpeedRun, + other: Optional[models.SpeedRun], + before: bool, + *, + expected_change_count: int = 0, + expected_error_keys: Optional[Union[list[str], type]] = None, + expected_status_code: int = 200, + ): from tracker.views.commands import MoveSpeedRun - output, status = MoveSpeedRun( - {'moving': self.run2.id, 'other': self.run1.id, 'before': True} + output, status_code = MoveSpeedRun( + { + 'moving': moving.id, + 'other': other.id if other else None, + 'before': before, + } ) - self.assertEqual(status, 200) - self.assertEqual(len(output), 2) - self.run1.refresh_from_db() - self.run2.refresh_from_db() - self.run3.refresh_from_db() - self.assertEqual(self.run1.order, 2) - self.assertEqual(self.run2.order, 1) - self.assertEqual(self.run3.order, 3) - - def test_after_to_after(self): - from tracker.views.commands import MoveSpeedRun + self.assertEqual(status_code, expected_status_code) + if expected_status_code == 200: + self.assertEqual(len(output), expected_change_count) + if expected_error_keys is not None: + self.assertIsInstance(output, dict, msg='Expected a dict') + self.assertTrue('error' in output, msg='Expected an `error` key in dict') + # a bit goofy perhaps but hopefully commands go away soon + if isinstance(expected_error_keys, type): + self.assertIsInstance(output['error'], expected_error_keys) + else: + self.assertEqual(set(output['error'].keys()), set(expected_error_keys)) + + def assertRunsInOrder( + self, ordered: Iterable[models.SpeedRun], unordered: Iterable[models.SpeedRun] + ): + all_runs = list(itertools.chain(ordered, unordered)) + + self.assertNotEqual(len(all_runs), 0, msg='Run list was empty') + + for r in all_runs: + r.refresh_from_db() + + for a, b in pairwise(all_runs): + self.assertEqual( + a.event_id, b.event_id, msg='Runs are from different events' + ) - output, status = MoveSpeedRun( - {'moving': self.run3.id, 'other': self.run1.id, 'before': False} + # be exhaustive + self.assertEqual( + len(all_runs), + models.SpeedRun.objects.filter(event=all_runs[0].event_id).count(), + msg='Not all runs for this event were provided', ) - self.assertEqual(status, 200) - self.assertEqual(len(output), 2) - self.run1.refresh_from_db() - self.run2.refresh_from_db() - self.run3.refresh_from_db() - self.assertEqual(self.run1.order, 1) - self.assertEqual(self.run2.order, 3) - self.assertEqual(self.run3.order, 2) - def test_before_to_before(self): - from tracker.views.commands import MoveSpeedRun + for n, (a, b) in enumerate(pairwise(ordered), start=1): + with self.subTest(f'ordered {n}: {a}, {b}'): + self.assertEqual(a.order, n, msg='Order was wrong') + self.assertEqual(b.order, n + 1, msg='Order was wrong') + self.assertEqual(a.endtime, b.starttime, msg='Run times do not match') - output, status = MoveSpeedRun( - {'moving': self.run1.id, 'other': self.run3.id, 'before': True} - ) - self.assertEqual(status, 200) - self.assertEqual(len(output), 2) - self.run1.refresh_from_db() - self.run2.refresh_from_db() - self.run3.refresh_from_db() - self.assertEqual(self.run1.order, 2) - self.assertEqual(self.run2.order, 1) - self.assertEqual(self.run3.order, 3) + for r in unordered: + with self.subTest(f'unordered {r}'): + self.assertIsNone(r.order, msg='Run should have been unordered') - def test_before_to_after(self): - from tracker.views.commands import MoveSpeedRun + def test_after_to_before(self): + self.assertResults(self.run2, self.run1, True, expected_change_count=2) + self.assertRunsInOrder([self.run2, self.run1, self.run3], [self.run4]) - output, status = MoveSpeedRun( - {'moving': self.run1.id, 'other': self.run2.id, 'before': False} - ) - self.assertEqual(status, 200) - self.assertEqual(len(output), 2) - self.run1.refresh_from_db() - self.run2.refresh_from_db() - self.run3.refresh_from_db() - self.assertEqual(self.run1.order, 2) - self.assertEqual(self.run2.order, 1) - self.assertEqual(self.run3.order, 3) + def test_after_to_after(self): + self.assertResults(self.run3, self.run1, False, expected_change_count=2) + self.assertRunsInOrder([self.run1, self.run3, self.run2], [self.run4]) - def test_unordered_to_before(self): - from tracker.views.commands import MoveSpeedRun + def test_before_to_before(self): + self.assertResults(self.run1, self.run3, True, expected_change_count=2) + self.assertRunsInOrder([self.run2, self.run1, self.run3], [self.run4]) - output, status = MoveSpeedRun( - {'moving': self.run4.id, 'other': self.run2.id, 'before': True} - ) - self.assertEqual(status, 200) - self.assertEqual(len(output), 3) - self.run2.refresh_from_db() - self.run3.refresh_from_db() - self.run4.refresh_from_db() - self.assertEqual(self.run2.order, 3) - self.assertEqual(self.run3.order, 4) - self.assertEqual(self.run4.order, 2) + def test_before_to_after(self): + self.assertResults(self.run1, self.run2, False, expected_change_count=2) + self.assertRunsInOrder([self.run2, self.run1, self.run3], [self.run4]) - def test_unordered_to_after(self): - from tracker.views.commands import MoveSpeedRun + def test_unordered_to_before(self): + self.assertResults(self.run4, self.run2, True, expected_change_count=3) + self.assertRunsInOrder([self.run1, self.run4, self.run2, self.run3], []) - output, status = MoveSpeedRun( - {'moving': self.run4.id, 'other': self.run2.id, 'before': False} + def test_unordered_to_after(self): + self.assertResults(self.run4, self.run2, False, expected_change_count=2) + self.assertRunsInOrder([self.run1, self.run2, self.run4, self.run3], []) + + def test_remove_from_order(self): + self.assertResults(self.run2, None, True, expected_change_count=2) + self.assertRunsInOrder([self.run1, self.run3], [self.run2, self.run4]) + + def test_already_removed(self): + self.assertResults(self.run4, None, True, expected_status_code=400) + + def test_error_cases(self): + self.assertResults( + self.run2, + self.run2, + True, + expected_error_keys=str, + expected_status_code=400, ) - self.assertEqual(status, 200) - self.assertEqual(len(output), 2) - self.run2.refresh_from_db() - self.run3.refresh_from_db() - self.run4.refresh_from_db() - self.assertEqual(self.run2.order, 2) - self.assertEqual(self.run3.order, 4) - self.assertEqual(self.run4.order, 3) def test_too_long_for_anchor(self): - from tracker.views.commands import MoveSpeedRun - self.run2.anchor_time = self.run2.starttime self.run2.save() - output, status = MoveSpeedRun( - {'moving': self.run3.id, 'other': self.run2.id, 'before': True} + self.assertResults( + self.run3, + self.run2, + True, + expected_error_keys=['setup_time'], + expected_status_code=400, ) - self.assertEqual(status, 400) - class TestSpeedRunAdmin(TransactionTestCase): def setUp(self): diff --git a/tracker/admin/event.py b/tracker/admin/event.py index d717484db..a78ad3838 100644 --- a/tracker/admin/event.py +++ b/tracker/admin/event.py @@ -718,12 +718,12 @@ class SpeedRunAdmin(EventLockedMixin, CustomModelAdmin): list_display = ( 'name', 'category', - 'description', 'runners_', 'hosts_', 'commentators_', 'onsite', 'starttime', + 'anchored', 'run_time', 'setup_time', ) @@ -772,6 +772,10 @@ def hosts_(self, instance): def commentators_(self, instance): return ', '.join(str(h) for h in instance.commentators.all()) + @admin.display(description='Anchored', boolean=True) + def anchored(self, instance): + return instance.anchor_time is not None + def bids(self, instance): if instance.id is not None: return format_html( diff --git a/tracker/models/event.py b/tracker/models/event.py index f17a19444..56a577f7e 100644 --- a/tracker/models/event.py +++ b/tracker/models/event.py @@ -20,19 +20,6 @@ from .fields import TimestampField from .util import LatestEvent -# TODO: remove when 3.10 is oldest supported version - -try: - from itertools import pairwise -except ImportError: - - def pairwise(iterable): - # pairwise('ABCDEFG') --> AB BC CD DE EF FG - a, b = itertools.tee(iterable) - next(b, None) - return zip(a, b) - - __all__ = [ 'Event', 'PostbackURL', @@ -568,7 +555,7 @@ def clean(self): 'order': 'Next anchor in the order would occur before this one' } ) - for c, n in pairwise( + for c, n in util.pairwise( itertools.chain( [self], SpeedRun.objects.filter( diff --git a/tracker/util.py b/tracker/util.py index c7c52d1b9..428d7f316 100644 --- a/tracker/util.py +++ b/tracker/util.py @@ -12,6 +12,7 @@ try: import zoneinfo except ImportError: + # noinspection PyUnresolvedReferences from backports import zoneinfo @@ -128,3 +129,21 @@ def flatten(iterable): def utcnow() -> datetime.datetime: return datetime.datetime.now(datetime.timezone.utc) + + +def set_mismatch(expected, actual): + return set(expected) - set(actual), set(actual) - set(expected) + + +try: + from itertools import pairwise +except ImportError: + # TODO: remove when 3.10 is oldest supported version + + def pairwise(iterable): + import itertools + + # pairwise('ABCDEFG') --> AB BC CD DE EF FG + a, b = itertools.tee(iterable) + next(b, None) + return zip(a, b) diff --git a/tracker/views/api.py b/tracker/views/api.py index 208f30410..f3b3c1e85 100644 --- a/tracker/views/api.py +++ b/tracker/views/api.py @@ -745,9 +745,11 @@ def command(request): func = getattr(commands, data['command'], None) if func: if request.user.has_perm(func.permission): - output, status = func(data) + output, status = func({k: v for k, v in data.items() if k != 'command'}) if status == 200: output = serializers.serialize('json', output, ensure_ascii=False) + else: + output = json.dumps(output) else: output = json.dumps({'error': 'permission denied'}) status = 403 diff --git a/tracker/views/commands.py b/tracker/views/commands.py index f7e0ebc5b..0d26ca096 100644 --- a/tracker/views/commands.py +++ b/tracker/views/commands.py @@ -1,9 +1,8 @@ -import json - from django.core.exceptions import ValidationError from django.db import transaction from tracker.models import SpeedRun +from tracker.util import set_mismatch __all__ = [ 'MoveSpeedRun', @@ -11,14 +10,47 @@ def MoveSpeedRun(data): - moving = SpeedRun.objects.get(pk=data['moving']) - other = SpeedRun.objects.get(pk=data['other']) - if moving.event_id != other.event_id: - return json.dumps({'error': 'Runs are not in the same event'}), 400 - before = bool(data['before']) try: + expected_keys = {'moving', 'other', 'before'} + missing_keys, extra_keys = set_mismatch(expected_keys, data.keys()) + if missing_keys or extra_keys: + error = ValidationError( + 'required keys were missing and/or extra keys were provided' + ) + if missing_keys: + error.error_dict = error.update_error_dict( + {'missing_keys': missing_keys} + ) + if extra_keys: + error.error_dict = error.update_error_dict({'extra_keys': extra_keys}) + raise error + moving = SpeedRun.objects.filter(pk=data['moving'], event__locked=False).first() + other = ( + SpeedRun.objects.filter(pk=data['other'], event__locked=False).first() + if data.get('other', None) + else None + ) + if moving is None: + raise ValidationError('moving run does not exist or is locked') + if data['other'] is not None and other is None: + raise ValidationError('other run does not exist or is locked') + if moving == other: + raise ValidationError('runs are the same run') + if other and moving.event_id != other.event_id: + raise ValidationError('runs are not from the same event') + before = bool(data['before']) with transaction.atomic(): - if moving.order is None: + if other is None: + if moving.order is None: + return [], 400 # nothing to do + else: + runs = SpeedRun.objects.filter( + event=moving.event, order__gt=moving.order + ).select_for_update() + final = None + first = moving.order + diff = -1 + elif moving.order is None: if before: runs = SpeedRun.objects.filter( event=moving.event, order__gte=other.order @@ -81,12 +113,21 @@ def MoveSpeedRun(data): first_run = SpeedRun.objects.get(event=moving.event, order=first) first_run.clean() models = first_run.save() + if other is None: + models = models + [moving] return models, 200 except ValidationError as e: + result = {} if hasattr(e, 'error_dict'): - return json.dumps({'error': e.message_dict}), 400 + result.update({'error': e.message_dict}) + elif len(e.messages) > 1: + result.update({'error': e.messages}) else: - return json.dumps({'error': e.messages}), 400 + result.update({'error': e.message}) + if getattr(e, 'code', None) is not None: + result['code'] = e.code + + return result, 400 MoveSpeedRun.permission = 'tracker.change_speedrun'