Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,19 +1,19 @@
# Use active chart availability for song detail actions

Status: needs-triage
Status: done

## What to build

Use lightweight active-chart availability from the chart service when rendering single-song detail actions, so the manual chart button is shown only when an active chart exists and administrator actions keep their existing rules.

## 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

Expand Down
21 changes: 16 additions & 5 deletions src/handlers/navigation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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),
Expand All @@ -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(
[
Expand Down
9 changes: 9 additions & 0 deletions src/services/chart_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
24 changes: 24 additions & 0 deletions tests/test_chart_service.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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]
Expand Down Expand Up @@ -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 == []
54 changes: 51 additions & 3 deletions tests/test_navigation.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -58,6 +61,7 @@ def build_context(
bot_data={
SETTINGS_KEY: settings,
SONG_SERVICE_KEY: song_service,
CHART_SERVICE_KEY: chart_service,
}
),
)
Expand Down Expand Up @@ -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": "Активні пісні",
Expand All @@ -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": "Активні пісні",
Expand All @@ -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
Expand Down
Loading