From af2953995cae01dabcac831b831b9bc10a7c88a2 Mon Sep 17 00:00:00 2001 From: Allison Date: Mon, 27 Apr 2026 16:31:31 -0400 Subject: [PATCH] fix(marketing): call marketing.landing view directly (avoid redirect loop) recordings.index previously redirected anonymous users to url_for('marketing.landing'), but both endpoints are mounted at '/'. Since recordings_bp registers first, Flask's URL map routed back to recordings.index -> infinite redirect loop. Now we invoke the marketing landing view function directly for anonymous requests, preserving the URL map and avoiding the loop. --- src/api/recordings.py | 15 +++-- tests/test_marketing_root_redirect.py | 82 +++++++++++++++++---------- 2 files changed, 62 insertions(+), 35 deletions(-) diff --git a/src/api/recordings.py b/src/api/recordings.py index ef7e993..2ae4834 100644 --- a/src/api/recordings.py +++ b/src/api/recordings.py @@ -39,6 +39,7 @@ from src.file_exporter import export_recording, mark_export_as_deleted from src.utils.ffprobe import get_codec_info, get_creation_date, FFProbeError from src.utils.audio_conversion import convert_if_needed from src.utils.file_hash import compute_file_sha256 +from src.marketing.routes import landing as _marketing_landing # Create blueprint recordings_bp = Blueprint('recordings', __name__) @@ -1360,16 +1361,22 @@ def reset_status(recording_id): def index(): """Root route handler. - Anonymous users are redirected to the marketing landing page so the - public site is reachable at "/". Authenticated users continue to see - the recordings dashboard (legacy Speakr UI). + Anonymous users see the marketing landing page so the public site is + reachable at "/". Authenticated users continue to see the recordings + dashboard (legacy Speakr UI). Phase 1 of marketing redesign 2026 (B-1.3) replaced the previous @login_required decorator with this inline check to resolve the route collision between recordings_bp.index and marketing_bp.landing. + + NOTE: We invoke the marketing.landing view function directly (rather + than redirecting via url_for('marketing.landing')) because both + endpoints are mounted at "/". Since recordings_bp is registered first, + Flask's URL map resolves "/" to recordings.index, so a redirect would + loop back into this same handler indefinitely. """ if not current_user.is_authenticated: - return redirect(url_for('marketing.landing')) + return _marketing_landing() # Check if user is a group admin is_team_admin = GroupMembership.query.filter_by( diff --git a/tests/test_marketing_root_redirect.py b/tests/test_marketing_root_redirect.py index f6c4e86..87880ed 100644 --- a/tests/test_marketing_root_redirect.py +++ b/tests/test_marketing_root_redirect.py @@ -6,6 +6,13 @@ redirected anonymous users to /login. After B-1.3, anonymous users land on the public marketing site; only authenticated users see the legacy Speakr dashboard. +Implementation note: recordings.index calls marketing.routes.landing() +directly (not via redirect) because both endpoints are mounted at "/". +A redirect to url_for('marketing.landing') would resolve back to +recordings.index (registered first in the URL map) and cause an infinite +redirect loop. The expected behavior is therefore a direct 200 render, +NOT a redirect. + Pattern: no conftest.py, env vars set at module load time, then import src.app.app directly. Mirrors tests/test_blueprint_registration.py. """ @@ -21,44 +28,56 @@ os.environ.setdefault('SECRET_KEY', 'test-secret-key-for-marketing-root-redirect from src.app import app # noqa: E402 -def test_anonymous_user_at_root_does_not_go_to_login(): - """Anonymous user GET / must NOT be redirected to /login.""" +def test_anonymous_user_no_login_redirect(): + """Anonymous user GET / must NOT redirect to /login.""" client = app.test_client() response = client.get('/', follow_redirects=False) if response.status_code in (301, 302, 303, 307, 308): location = response.headers.get('Location', '') assert '/login' not in location, ( - f"Expected anonymous user to see marketing, but redirected to: {location}" + f"Expected no /login redirect, got: {location}" ) -def test_anonymous_user_at_root_sees_marketing_or_redirects_to_marketing(): - """Anonymous user GET / must see marketing landing (200) OR redirect to marketing.landing.""" +def test_anonymous_user_at_root_sees_marketing(): + """Anonymous user GET / receives marketing landing directly (no redirect). + + The handler must invoke the marketing.landing view function directly + rather than redirecting, because url_for('marketing.landing') resolves + back to '/' and would loop into recordings.index forever. + """ client = app.test_client() response = client.get('/', follow_redirects=False) + # Direct render -- must be 200, NOT a redirect + assert response.status_code == 200, ( + f"Expected 200 at / for anonymous (direct render, no redirect " + f"to avoid loop), got {response.status_code}" + ) + # Verify it's the marketing placeholder content + body_lower = response.data.lower() + assert (b'dictia' in body_lower) or (b'marketing' in body_lower), ( + f"Expected marketing content at /, got: {response.data[:200]!r}" + ) - if response.status_code in (301, 302, 303, 307, 308): - # Acceptable: marketing landing is served via redirect from - # recordings_bp.index when the URL map prefers the recordings rule. - location = response.headers.get('Location', '') - assert '/login' not in location, ( - f"Anonymous user redirected to login instead of marketing: {location}" - ) - # The redirect target should be the marketing landing or root itself. - # Because marketing.landing is mounted at '/', url_for('marketing.landing') - # produces '/' — the redirect Location should not point back at any - # private route. - assert ('/admin' not in location and '/account' not in location), ( - f"Unexpected redirect from / : {location}" - ) - else: - assert response.status_code == 200, ( - f"Expected 200 at / for anonymous, got {response.status_code}" - ) - body_lower = response.data.lower() - assert (b'marketing' in body_lower) or (b'dictia' in body_lower), ( - "Expected marketing landing content at / for anonymous user" - ) + +def test_root_route_no_redirect_loop(): + """GET / must not bounce through redirects. + + Following redirects should reach a final 200 in <=1 hops; if Flask + returns >1 redirect chain at /, it indicates the recordings.index + handler is redirecting back to itself via url_for('marketing.landing'). + """ + client = app.test_client() + response = client.get('/', follow_redirects=False) + # Either an immediate 200 (preferred) or at most one redirect. + assert response.status_code not in (301, 302, 303, 307, 308) or ( + '/login' not in response.headers.get('Location', '') + and response.headers.get('Location', '') != '/' + ), ( + f"Detected potential redirect loop at /: status=" + f"{response.status_code}, Location=" + f"{response.headers.get('Location', '')}" + ) def test_root_route_resolves_to_recordings_index(): @@ -66,15 +85,16 @@ def test_root_route_resolves_to_recordings_index(): The collision between recordings.index and marketing.landing is deliberate: recordings.index is registered first and intercepts the - request, then redirects anonymous users to marketing.landing - (which itself maps to '/'). This test pins that contract so a - future refactor can't silently swap the registration order. + request, then dispatches anonymous users to the marketing.landing + view function directly (no URL-level redirect). This test pins that + contract so a future refactor can't silently swap the registration + order. """ rules_for_root = [ r for r in app.url_map.iter_rules() if str(r) == '/' ] endpoints = {r.endpoint for r in rules_for_root} - # Both endpoints must exist for the redirect chain to work. + # Both endpoints must exist; recordings.index is the active one. assert 'recordings.index' in endpoints, ( f"Expected recordings.index to own '/', got endpoints: {endpoints}" )