diff --git a/.scratch/chart-preview-feedback/issues/02-use-active-chart-availability-for-song-detail-actions.md b/.scratch/chart-preview-feedback/issues/02-use-active-chart-availability-for-song-detail-actions.md index b2eadc5..bec002c 100644 --- a/.scratch/chart-preview-feedback/issues/02-use-active-chart-availability-for-song-detail-actions.md +++ b/.scratch/chart-preview-feedback/issues/02-use-active-chart-availability-for-song-detail-actions.md @@ -1,6 +1,6 @@ # Use active chart availability for song detail actions -Status: needs-triage +Status: done ## What to build @@ -8,12 +8,12 @@ Use lightweight active-chart availability from the chart service when rendering ## Acceptance criteria -- [ ] The chart service exposes an async active-chart availability or metadata lookup that does not download chart bytes. -- [ ] Single-song detail keyboards show the manual chart button only when an active chart exists. -- [ ] Non-admin song detail views hide administrator actions while still reflecting chart button availability. -- [ ] Admin song detail views keep edit, archive, and upload chart actions visible according to existing administrator checks. -- [ ] Upload chart remains available to administrators whether or not an active chart exists. -- [ ] Service and handler tests cover chart availability, chart button visibility, and admin action visibility. +- [x] The chart service exposes an async active-chart availability or metadata lookup that does not download chart bytes. +- [x] Single-song detail keyboards show the manual chart button only when an active chart exists. +- [x] Non-admin song detail views hide administrator actions while still reflecting chart button availability. +- [x] Admin song detail views keep edit, archive, and upload chart actions visible according to existing administrator checks. +- [x] Upload chart remains available to administrators whether or not an active chart exists. +- [x] Service and handler tests cover chart availability, chart button visibility, and admin action visibility. ## Blocked by diff --git a/src/handlers/navigation.py b/src/handlers/navigation.py index cc3d643..5cbaee3 100644 --- a/src/handlers/navigation.py +++ b/src/handlers/navigation.py @@ -5,7 +5,7 @@ from telegram import InlineKeyboardButton, InlineKeyboardMarkup, Update from telegram.ext import CallbackQueryHandler, ContextTypes, MessageHandler, filters -from bot.runtime import get_song_service +from bot.runtime import get_chart_service, get_song_service from handlers.backup import export_backup_command from handlers.charts import send_chart_for_song_id from handlers.common import help_command, send_home_screen @@ -432,10 +432,13 @@ async def _render_song_detail( await query.edit_message_text(str(error)) return + chart_service = get_chart_service(context) + has_active_chart = await chart_service.has_active_chart(song_id) keyboard = _song_detail_keyboard( song_id=song_id, page=page, is_admin=is_admin_user(update, context), + has_active_chart=has_active_chart, ) await query.edit_message_text( "Деталі пісні:\n" + format_song(song), @@ -444,10 +447,18 @@ async def _render_song_detail( ) -def _song_detail_keyboard(*, song_id: int, page: int, is_admin: bool) -> InlineKeyboardMarkup: - rows: list[list[InlineKeyboardButton]] = [ - [InlineKeyboardButton("Переглянути гармонію", callback_data=f"song:view:{song_id}")], - ] +def _song_detail_keyboard( + *, + song_id: int, + page: int, + is_admin: bool, + has_active_chart: bool, +) -> InlineKeyboardMarkup: + rows: list[list[InlineKeyboardButton]] = [] + if has_active_chart: + rows.append( + [InlineKeyboardButton("Переглянути гармонію", callback_data=f"song:view:{song_id}")] + ) if is_admin: rows.append( [ diff --git a/src/services/chart_service.py b/src/services/chart_service.py index 25966cc..1127307 100644 --- a/src/services/chart_service.py +++ b/src/services/chart_service.py @@ -135,6 +135,15 @@ async def get_active_chart_file(self, song_id: int) -> ChartFile: content=stored_binary.content, ) + async def has_active_chart(self, song_id: int) -> bool: + async with self._session_factory() as session: + statement = select(SongChart.id).where( + SongChart.song_id == song_id, + SongChart.status == SongChartStatus.ACTIVE, + ) + chart_id = await session.scalar(statement) + return chart_id is not None + def _clean_optional(value: str | None) -> str | None: if value is None: diff --git a/tests/test_chart_service.py b/tests/test_chart_service.py index 81c404c..3aaadf0 100644 --- a/tests/test_chart_service.py +++ b/tests/test_chart_service.py @@ -17,6 +17,7 @@ def __init__(self, bucket: str = "song-vault-charts") -> None: self.bucket = bucket self.objects: dict[str, tuple[bytes, str]] = {} self.deleted: list[str] = [] + self.get_requests: list[tuple[str, str]] = [] async def ensure_ready(self) -> None: return None @@ -37,6 +38,7 @@ async def put_chart( ) async def get_chart(self, *, bucket: str, object_key: str) -> StoredChartBinary: + self.get_requests.append((bucket, object_key)) if bucket != self.bucket or object_key not in self.objects: raise ChartStorageError("Object not found.") content, content_type = self.objects[object_key] @@ -217,3 +219,25 @@ async def test_chart_service_raises_when_song_has_no_chart( with pytest.raises(SongChartNotFoundError): await chart_service.get_active_chart_file(song.id) + + +@pytest.mark.asyncio +async def test_chart_service_reports_active_chart_availability_without_downloading_bytes( + chart_fixture: tuple[ChartService, FakeSessionFactory, FakeChartStorage], +) -> None: + chart_service, session_factory, storage = chart_fixture + song = session_factory.add_song(4, title="Firm Foundation") + + assert await chart_service.has_active_chart(song.id) is False + + await chart_service.upload_chart( + song.id, + ChartUpload( + original_filename="firm-foundation.png", + content_type="image/png", + content=b"chart-bytes", + ), + ) + + assert await chart_service.has_active_chart(song.id) is True + assert storage.get_requests == [] diff --git a/tests/test_navigation.py b/tests/test_navigation.py index b312087..65d8650 100644 --- a/tests/test_navigation.py +++ b/tests/test_navigation.py @@ -6,7 +6,7 @@ import pytest -from bot.runtime import SETTINGS_KEY, SONG_SERVICE_KEY +from bot.runtime import CHART_SERVICE_KEY, SETTINGS_KEY, SONG_SERVICE_KEY from config.settings import Settings from handlers.navigation import ( SEARCH_PENDING_KEY, @@ -45,8 +45,11 @@ def build_song( def build_context( *, song_service: object, + chart_service: object | None = None, admin_ids: tuple[int, ...] = (1,), ) -> SimpleNamespace: + if chart_service is None: + chart_service = SimpleNamespace(has_active_chart=AsyncMock(return_value=True)) settings = Settings( TELEGRAM_BOT_TOKEN="token", ADMIN_TELEGRAM_USER_IDS=admin_ids, @@ -58,6 +61,7 @@ def build_context( bot_data={ SETTINGS_KEY: settings, SONG_SERVICE_KEY: song_service, + CHART_SERVICE_KEY: chart_service, } ), ) @@ -270,7 +274,8 @@ async def test_backup_menu_rejects_non_admin_with_shared_copy() -> None: async def test_song_detail_for_non_admin_hides_admin_action_buttons() -> None: song = build_song() song_service = SimpleNamespace(get_song=AsyncMock(return_value=song)) - context = build_context(song_service=song_service, admin_ids=(1,)) + chart_service = SimpleNamespace(has_active_chart=AsyncMock(return_value=True)) + context = build_context(song_service=song_service, chart_service=chart_service, admin_ids=(1,)) context.user_data[SONG_BROWSER_STATE_KEY] = { "mode": "browse", "title": "Активні пісні", @@ -290,13 +295,35 @@ async def test_song_detail_for_non_admin_hides_admin_action_buttons() -> None: assert "Завантажити гармонію" not in labels assert "Переглянути гармонію" in labels assert "Назад до результатів" in labels + chart_service.has_active_chart.assert_awaited_once_with(5) + + +@pytest.mark.asyncio +async def test_song_detail_for_non_admin_hides_chart_button_when_no_active_chart() -> None: + song = build_song() + song_service = SimpleNamespace(get_song=AsyncMock(return_value=song)) + chart_service = SimpleNamespace(has_active_chart=AsyncMock(return_value=False)) + context = build_context(song_service=song_service, chart_service=chart_service, admin_ids=(1,)) + update, query = build_callback_update(data="song:detail:5:0", user_id=2) + + await navigation_callback_router(update, context) + + keyboard = query.edit_message_text.await_args.kwargs["reply_markup"] + labels = [button.text for row in keyboard.inline_keyboard for button in row] + assert "Переглянути гармонію" not in labels + assert "Редагувати" not in labels + assert "Архівувати" not in labels + assert "Завантажити гармонію" not in labels + assert "Назад до результатів" in labels + chart_service.has_active_chart.assert_awaited_once_with(5) @pytest.mark.asyncio async def test_song_detail_for_admin_shows_admin_action_buttons() -> None: song = build_song() song_service = SimpleNamespace(get_song=AsyncMock(return_value=song)) - context = build_context(song_service=song_service, admin_ids=(1,)) + chart_service = SimpleNamespace(has_active_chart=AsyncMock(return_value=True)) + context = build_context(song_service=song_service, chart_service=chart_service, admin_ids=(1,)) context.user_data[SONG_BROWSER_STATE_KEY] = { "mode": "browse", "title": "Активні пісні", @@ -314,6 +341,27 @@ async def test_song_detail_for_admin_shows_admin_action_buttons() -> None: assert "Редагувати" in labels assert "Архівувати" in labels assert "Завантажити гармонію" in labels + assert "Переглянути гармонію" in labels + chart_service.has_active_chart.assert_awaited_once_with(5) + + +@pytest.mark.asyncio +async def test_song_detail_for_admin_keeps_admin_actions_when_no_active_chart() -> None: + song = build_song() + song_service = SimpleNamespace(get_song=AsyncMock(return_value=song)) + chart_service = SimpleNamespace(has_active_chart=AsyncMock(return_value=False)) + context = build_context(song_service=song_service, chart_service=chart_service, admin_ids=(1,)) + update, query = build_callback_update(data="song:detail:5:0", user_id=1) + + await navigation_callback_router(update, context) + + keyboard = query.edit_message_text.await_args.kwargs["reply_markup"] + labels = [button.text for row in keyboard.inline_keyboard for button in row] + assert "Переглянути гармонію" not in labels + assert "Редагувати" in labels + assert "Архівувати" in labels + assert "Завантажити гармонію" in labels + chart_service.has_active_chart.assert_awaited_once_with(5) @pytest.mark.asyncio