diff --git a/app/routes/matches.py b/app/routes/matches.py index 6944843..9d7c8fa 100644 --- a/app/routes/matches.py +++ b/app/routes/matches.py @@ -25,7 +25,7 @@ from app.models import ( PersonalNote, ) from datetime import datetime, timedelta -from app.discord_bot import send_schedule_notification +from app.services.scheduling import notify_participants, zip_participants matches_bp = Blueprint('matches', __name__, url_prefix='/matches') @@ -375,25 +375,14 @@ def create_match(tryout_id): db.session.commit() - # Discord notifications - event_date_str = date_obj.strftime('%Y-%m-%d') - event_time_str = ( - f"{start_time.strftime('%I:%M %p')} - {end_time.strftime('%I:%M %p')}" - if start_time and end_time - else 'TBD' + notify_participants( + title=match.title, + date=date_obj, + start_time=start_time, + end_time=end_time, + participants=zip_participants(notified_player_ids, notified_participant_ids), + fallback_id=match.id, ) - for i, player_id in enumerate(notified_player_ids): - reference_id = ( - notified_participant_ids[i] if i < len(notified_participant_ids) else match.id - ) - send_schedule_notification( - user_id=player_id, - event_type='match', - event_title=match.title, - event_date=event_date_str, - event_time=event_time_str, - reference_id=reference_id, - ) flash(_('Match scheduled successfully!'), 'success') return redirect(url_for('tryouts.view_tryout', tryout_id=tryout_id)) @@ -553,27 +542,14 @@ def edit_match(match_id): db.session.commit() - # Discord notifications - end_time_val = match.end_time or (match.start_time if match.start_time else None) - if match.start_time and end_time_val: - event_time_str = ( - f"{match.start_time.strftime('%I:%M %p')} - {end_time_val.strftime('%I:%M %p')}" - ) - else: - event_time_str = 'TBD' - event_date_str = match.date.strftime('%Y-%m-%d') - for i, player_id in enumerate(notified_player_ids): - reference_id = ( - notified_participant_ids[i] if i < len(notified_participant_ids) else match.id - ) - send_schedule_notification( - user_id=player_id, - event_type='match', - event_title=match.title, - event_date=event_date_str, - event_time=event_time_str, - reference_id=reference_id, - ) + notify_participants( + title=match.title, + date=match.date, + start_time=match.start_time, + end_time=match.end_time, + participants=zip_participants(notified_player_ids, notified_participant_ids), + fallback_id=match.id, + ) flash(_('Match updated successfully!'), 'success') return redirect(url_for('tryouts.view_tryout', tryout_id=tryout.id)) diff --git a/app/routes/team_matches.py b/app/routes/team_matches.py index fd65b27..584a473 100644 --- a/app/routes/team_matches.py +++ b/app/routes/team_matches.py @@ -19,7 +19,7 @@ from app.models import ( ) from app.permissions import can_manage_org_team, coach_org_teams, visible_org_teams from datetime import datetime, timedelta -from app.discord_bot import send_schedule_notification +from app.services.scheduling import notify_participants, zip_participants team_matches_bp = Blueprint('team_matches', __name__, url_prefix='/team-matches') @@ -204,27 +204,17 @@ def create_match(team_id): db.session.commit() - # Discord notifications - event_date_str = date_obj.strftime('%Y-%m-%d') - event_time_str = ( - f"{start_time.strftime('%I:%M %p')} - {end_time.strftime('%I:%M %p')}" - if start_time and end_time - else 'TBD' + notify_participants( + title=team_match.title, + date=date_obj, + start_time=start_time, + end_time=end_time, + participants=zip_participants( + [tp.player_id for tp in team_players], notified_participant_ids + ), + fallback_id=team_match.id, ) - for i, tp in enumerate(team_players): - reference_id = ( - notified_participant_ids[i] if i < len(notified_participant_ids) else team_match.id - ) - send_schedule_notification( - user_id=tp.player_id, - event_type='match', - event_title=team_match.title, - event_date=event_date_str, - event_time=event_time_str, - reference_id=reference_id, - ) - flash(_('Team match "%(title)s" scheduled successfully!', title=title), 'success') return redirect(url_for('team_matches.list_matches')) diff --git a/app/services/scheduling.py b/app/services/scheduling.py new file mode 100644 index 0000000..96fac5a --- /dev/null +++ b/app/services/scheduling.py @@ -0,0 +1,96 @@ +"""Announcing a scheduled match to the people who have to be there. + +The same twenty lines appeared three times — matches.create_match, +matches.edit_match and team_matches.create_match — each formatting the date +and time itself, then walking two parallel lists in lockstep to pair a +player with the participant row that a Discord reaction has to find again +(ARCH-003). + +Three copies meant three chances to drift, and they had: +create_match read the times from local variables it had just parsed, while +edit_match re-derived them from the saved row and substituted the start +time for a missing end time. The rule kept here is the more careful of the +two. +""" + +import logging + +from app.discord_bot import send_schedule_notification + +logger = logging.getLogger(__name__) + +#: What the bot shows when a match has no usable time. +TIME_UNKNOWN = 'TBD' + + +def format_event_time(start_time, end_time): + """Render a match's time range the way the Discord message expects. + + Args: + start_time: A time, or None. + end_time: A time, or None. Falls back to start_time, so a match with + only a start still announces something useful. + + Returns: + str: 'HH:MM AM - HH:MM PM', or TIME_UNKNOWN. + """ + if not start_time: + return TIME_UNKNOWN + finish = end_time or start_time + return f'{start_time.strftime("%I:%M %p")} - {finish.strftime("%I:%M %p")}' + + +def zip_participants(player_ids, participant_ids): + """Pair each player with the participant row that was created for them. + + The routes build these as two parallel lists, appended in step. Pairing + them by index is what the original code did, and it is only correct as + long as they stay in step — hence one place to look at rather than + three. A short participant list yields None, which + :func:`notify_participants` turns into the fallback reference. + + Args: + player_ids: Player primary keys, in creation order. + participant_ids: Participant row ids, in the same order. + + Yields: + tuple[int, int | None]: (player_id, participant_id). + """ + for index, player_id in enumerate(player_ids): + yield player_id, participant_ids[index] if index < len(participant_ids) else None + + +def notify_participants(*, title, date, start_time, end_time, participants, fallback_id): + """Tell each participant that a match has been scheduled or changed. + + Args: + title: Match title, shown in the message. + date: The match date. + start_time: Start time, or None. + end_time: End time, or None. + participants: Iterable of (player_id, participant_id) pairs. + participant_id is what a Discord reaction resolves back to, so + attendance lands on the right row. + fallback_id: Reference to use when a participant row has no id — + the match's own, which the bot can still act on. + + Returns: + int: How many notifications were handed to the bot. + """ + event_date = date.strftime('%Y-%m-%d') + event_time = format_event_time(start_time, end_time) + + sent = 0 + for player_id, participant_id in participants: + if not player_id: + continue + send_schedule_notification( + user_id=player_id, + event_type='match', + event_title=title, + event_date=event_date, + event_time=event_time, + reference_id=participant_id or fallback_id, + ) + sent += 1 + return sent diff --git a/tests/test_scheduling_service.py b/tests/test_scheduling_service.py new file mode 100644 index 0000000..38687e9 --- /dev/null +++ b/tests/test_scheduling_service.py @@ -0,0 +1,100 @@ +"""Announcing a scheduled match — ARCH-003. + +The same twenty lines lived in three routes. They had already drifted: +create_match formatted the time from the values it had just parsed and +printed 'TBD' whenever either end was missing, while edit_match re-derived +them from the saved row and substituted the start time for a missing end. +A match with a start and no end therefore announced a time in one route and +'TBD' in the other, for the same data. + +app/services/scheduling.py keeps the more careful of the two rules. +""" + +from datetime import date, time + +import pytest + +from app.services import scheduling +from app.services.scheduling import TIME_UNKNOWN, format_event_time, zip_participants + + +class TestTimeFormatting: + def test_a_full_range_is_rendered(self): + assert format_event_time(time(18, 0), time(20, 30)) == '06:00 PM - 08:30 PM' + + def test_a_missing_end_falls_back_to_the_start(self): + """This is the behaviour create_match did not have: it printed TBD + and lost the start time it knew perfectly well.""" + assert format_event_time(time(18, 0), None) == '06:00 PM - 06:00 PM' + + def test_no_start_means_nothing_useful_to_say(self): + assert format_event_time(None, time(20, 0)) == TIME_UNKNOWN + assert format_event_time(None, None) == TIME_UNKNOWN + + +class TestParticipantPairing: + def test_players_pair_with_their_participant_rows(self): + assert list(zip_participants([7, 8, 9], [70, 80, 90])) == [(7, 70), (8, 80), (9, 90)] + + def test_a_short_participant_list_yields_none(self): + """The routes append to two lists in step; if they ever fall out of + step the notification still goes out, against the match itself.""" + assert list(zip_participants([7, 8], [70])) == [(7, 70), (8, None)] + + def test_no_players_means_no_pairs(self): + assert list(zip_participants([], [])) == [] + + +class TestNotifying: + @pytest.fixture + def sent(self, monkeypatch): + calls = [] + monkeypatch.setattr( + scheduling, 'send_schedule_notification', lambda **kwargs: calls.append(kwargs) + ) + return calls + + def test_each_participant_is_told(self, sent): + count = scheduling.notify_participants( + title='Scrim', + date=date(2030, 4, 1), + start_time=time(19, 0), + end_time=time(21, 0), + participants=[(1, 11), (2, 22)], + fallback_id=99, + ) + + assert count == 2 + assert [call['user_id'] for call in sent] == [1, 2] + assert [call['reference_id'] for call in sent] == [11, 22] + assert {call['event_date'] for call in sent} == {'2030-04-01'} + assert {call['event_time'] for call in sent} == {'07:00 PM - 09:00 PM'} + + def test_a_participant_without_a_row_falls_back_to_the_match(self, sent): + """A Discord reaction resolves the reference back to a record; the + match id is the one thing that always exists.""" + scheduling.notify_participants( + title='Scrim', + date=date(2030, 4, 1), + start_time=None, + end_time=None, + participants=[(1, None)], + fallback_id=99, + ) + + assert sent[0]['reference_id'] == 99 + assert sent[0]['event_time'] == TIME_UNKNOWN + + def test_a_missing_player_is_skipped(self, sent): + assert ( + scheduling.notify_participants( + title='Scrim', + date=date(2030, 4, 1), + start_time=None, + end_time=None, + participants=[(None, 11), (2, 22)], + fallback_id=99, + ) + == 1 + ) + assert [call['user_id'] for call in sent] == [2]