diff --git a/make_post_sell/models/shop.py b/make_post_sell/models/shop.py index 40734ff..9cbf5d8 100644 --- a/make_post_sell/models/shop.py +++ b/make_post_sell/models/shop.py @@ -3,6 +3,7 @@ import logging import threading import time import uuid +from collections import Counter from sqlalchemy import Column, BigInteger, Boolean, Unicode, UnicodeText, func from sqlalchemy import and_ @@ -868,6 +869,45 @@ def reforge_discovery_ring(shop): return ring +def validate_discovery_ring(shop): + """Check ring topology health against the shop's public products. + + A healthy ring: one entry per visibility==1 product, no duplicates, + no stale IDs, no orphaned products. Any violation surfaces as a + populated list in the returned dict. + + Returns: + { + "valid": bool, + "ring_length": int, + "public_count": int, + "duplicates": list[str], # IDs appearing more than once + "orphans": list[str], # public product IDs missing from ring + "stale": list[str], # ring IDs no longer public/present + "length_mismatch": bool, # ring_length != public_count + } + """ + ring = shop.discovery_ring or [] + public_ids = {str(p.id) for p in shop.products if p.visibility == 1} + ring_ids = set(ring) + + counts = Counter(ring) + duplicates = sorted(pid for pid, c in counts.items() if c > 1) + stale = sorted(ring_ids - public_ids) + orphans = sorted(public_ids - ring_ids) + length_mismatch = len(ring) != len(public_ids) + + return { + "valid": not (duplicates or stale or orphans or length_mismatch), + "ring_length": len(ring), + "public_count": len(public_ids), + "duplicates": duplicates, + "orphans": orphans, + "stale": stale, + "length_mismatch": length_mismatch, + } + + # Tracks which shops have a background reforge in progress and whether # another reforge was requested while one was already running (dirty bit). _reforge_running = {} # shop_id_str -> True while a thread is active @@ -909,6 +949,18 @@ def reforge_discovery_ring_async(shop_id, session_factory): session.commit() log.info("Ring reforged for shop %s (%d products)", shop.name, len(ring)) + health = validate_discovery_ring(shop) + if not health["valid"]: + log.warning( + "Ring topology anomaly for shop %s: " + "duplicates=%d orphans=%d stale=%d length_mismatch=%s", + shop.name, + len(health["duplicates"]), + len(health["orphans"]), + len(health["stale"]), + health["length_mismatch"], + ) + with _reforge_guard: if _reforge_dirty.pop(shop_id_str, False): session.expire_all() diff --git a/make_post_sell/routes.py b/make_post_sell/routes.py index 1b8da08..afc9241 100644 --- a/make_post_sell/routes.py +++ b/make_post_sell/routes.py @@ -158,6 +158,7 @@ def includeme(config): config.add_route("shop_about_slug", "/s/{shop_id}/{slug:.*}/about") config.add_route("discovery_ring_json", "/s/{shop_id}/ring.json") + config.add_route("discovery_ring_health", "/s/{shop_id}/ring/health.json") config.add_route("shop_slug", "/s/{shop_id}/{slug:.*}") # content routes. diff --git a/make_post_sell/tests/test_functional.py b/make_post_sell/tests/test_functional.py index 7615e5f..44668c8 100644 --- a/make_post_sell/tests/test_functional.py +++ b/make_post_sell/tests/test_functional.py @@ -3056,6 +3056,116 @@ class AuthenticatedFunctionalTests(_AuthenticatedBase): # Should NOT have triggered reforge mock_async.assert_not_called() + def test_ring_health_requires_login(self): + """Anonymous request to ring health endpoint is forbidden.""" + shop = self._create_shop_helper( + shop_params={**self.shop1_params, "name": "ring-health-anon-shop"} + ) + shop_id = str(shop.id) + self.testapp.get("/log-out") + + res = self.testapp.get(f"/s/{shop_id}/ring/health.json", status=403) + self.assertIn("Shop editor access required", res.json["error"]) + + def test_ring_health_requires_mod(self): + """A logged-in non-owner cannot access another shop's ring health.""" + shop = self._create_shop_helper( + shop_params={**self.shop1_params, "name": "ring-health-mod-shop"}, + log_out_user=True, + ) + shop_id = str(shop.id) + + # Log in as a different user who doesn't own this shop + self.log_in_user(self.user2_creds) + res = self.testapp.get(f"/s/{shop_id}/ring/health.json", status=403) + self.assertIn("Shop editor access required", res.json["error"]) + + def test_ring_health_returns_valid_json_for_mod(self): + """Shop owner gets a populated health report with healthy ring.""" + shop = self._create_shop_helper( + shop_params={**self.shop1_params, "name": "ring-health-ok-shop"} + ) + shop_id = str(shop.id) + + # Enable watch mode and create 2 products so the ring has content + self.testapp.post( + f"/s/{shop_id}/settings", + {"form_section": "ribbon-settings", "watch_mode": "1", "submit": "Save Settings"}, + status=302, + ) + self.testapp.post( + f"/p/new?shop_id={shop_id}", self.product1_params + ).follow() + self.testapp.post( + f"/p/new?shop_id={shop_id}", + {**self.product1_params, "title": "second product"}, + ).follow() + + # Force a synchronous reforge so the ring is populated deterministically + from ..models.shop import reforge_discovery_ring + self.dbsession.expire(shop) + reforge_discovery_ring(shop) + self.dbsession.flush() + transaction.commit() + + res = self.testapp.get(f"/s/{shop_id}/ring/health.json", status=200) + data = res.json + self.assertTrue(data["valid"]) + self.assertEqual(data["ring_length"], 2) + self.assertEqual(data["public_count"], 2) + self.assertEqual(data["duplicates"], []) + self.assertEqual(data["orphans"], []) + self.assertEqual(data["stale"], []) + self.assertFalse(data["length_mismatch"]) + self.assertEqual(data["shop_id"], shop_id) + + def test_ring_health_detects_stale_after_visibility_change(self): + """Flipping a product to unlisted surfaces in the health report.""" + shop = self._create_shop_helper( + shop_params={**self.shop1_params, "name": "ring-health-stale-shop"} + ) + shop_id = str(shop.id) + + self.testapp.post( + f"/s/{shop_id}/settings", + {"form_section": "ribbon-settings", "watch_mode": "1", "submit": "Save Settings"}, + status=302, + ) + self.testapp.post( + f"/p/new?shop_id={shop_id}", self.product1_params + ).follow() + self.testapp.post( + f"/p/new?shop_id={shop_id}", + {**self.product1_params, "title": "second product"}, + ).follow() + + from ..models.shop import reforge_discovery_ring + self.dbsession.expire(shop) + reforge_discovery_ring(shop) + products = get_all_products(self.dbsession).all() + + # Mark one product unlisted — ring still contains it → stale + stale_id = str(products[0].id) + products[0].visibility = 2 + self.dbsession.flush() + transaction.commit() + + res = self.testapp.get(f"/s/{shop_id}/ring/health.json", status=200) + data = res.json + self.assertFalse(data["valid"]) + self.assertIn(stale_id, data["stale"]) + + def test_ring_health_shop_not_found(self): + """Unknown shop_id returns 404.""" + self._create_shop_helper( + shop_params={**self.shop1_params, "name": "ring-health-404-shop"} + ) + res = self.testapp.get( + "/s/00000000-0000-0000-0000-000000000000/ring/health.json", + status=404, + ) + self.assertIn("Shop not found", res.json["error"]) + def _create_content_with_metadata(self, shop_name, title, description, file_metadata): """Helper: create shop + watch mode + content product with given file_metadata. Returns (product_id_str, product_slug) — safe to use after transaction.commit(). diff --git a/make_post_sell/tests/test_integration.py b/make_post_sell/tests/test_integration.py index d635f40..2a2f67e 100644 --- a/make_post_sell/tests/test_integration.py +++ b/make_post_sell/tests/test_integration.py @@ -3351,6 +3351,112 @@ class TestAsyncRingReforgeIntegration(DatabaseIntegrationTests): self.assertEqual(ring[0], public_product_id) +class TestValidateDiscoveryRingIntegration(DatabaseIntegrationTests): + """End-to-end validator tests against real shop + product rows.""" + + def _make_shop(self, name="validate-ring-shop"): + user = get_or_create_user_by_email(self.dbsession, "validate@example.com") + self.dbsession.add(user) + self.dbsession.flush() + + shop = Shop(name, "555-555-5555", "123 Validate St", "desc") + shop.watch_mode_enabled = True + self.dbsession.add(shop) + self.dbsession.flush() + return shop + + def _make_products(self, shop, count, visibility=1): + products = [] + for i in range(count): + p = Product(f"Song {i}", f"Desc {i}") + p.shop_id = shop.id + p.visibility = visibility + self.dbsession.add(p) + products.append(p) + self.dbsession.flush() + return products + + def test_valid_after_fresh_reforge(self): + """Immediately after reforge, validator reports healthy ring.""" + from ..models.shop import reforge_discovery_ring, validate_discovery_ring + + shop = self._make_shop() + self._make_products(shop, 5) + reforge_discovery_ring(shop) + self.dbsession.flush() + + health = validate_discovery_ring(shop) + self.assertTrue(health["valid"]) + self.assertEqual(health["ring_length"], 5) + self.assertEqual(health["public_count"], 5) + self.assertEqual(health["orphans"], []) + self.assertEqual(health["stale"], []) + self.assertEqual(health["duplicates"], []) + + def test_detects_orphan_after_new_product(self): + """Adding a public product after reforge leaves it orphaned.""" + from ..models.shop import reforge_discovery_ring, validate_discovery_ring + + shop = self._make_shop(name="orphan-shop") + self._make_products(shop, 3) + reforge_discovery_ring(shop) + self.dbsession.flush() + + new_product = Product("Latecomer", "Arrived post-reforge") + new_product.shop_id = shop.id + new_product.visibility = 1 + self.dbsession.add(new_product) + self.dbsession.flush() + + health = validate_discovery_ring(shop) + self.assertFalse(health["valid"]) + self.assertEqual(health["orphans"], [str(new_product.id)]) + self.assertTrue(health["length_mismatch"]) + + def test_detects_stale_after_visibility_change(self): + """Flipping a product to unlisted leaves a stale entry in the ring.""" + from ..models.shop import reforge_discovery_ring, validate_discovery_ring + + shop = self._make_shop(name="stale-shop") + products = self._make_products(shop, 4) + reforge_discovery_ring(shop) + self.dbsession.flush() + + # Mark one product unlisted — it stays in the ring but isn't public anymore + products[1].visibility = 2 + self.dbsession.flush() + + health = validate_discovery_ring(shop) + self.assertFalse(health["valid"]) + self.assertEqual(health["stale"], [str(products[1].id)]) + + def test_reforge_clears_stale_and_orphan(self): + """A fresh reforge after changes restores a healthy ring.""" + from ..models.shop import reforge_discovery_ring, validate_discovery_ring + + shop = self._make_shop(name="heal-shop") + products = self._make_products(shop, 3) + reforge_discovery_ring(shop) + self.dbsession.flush() + + # Add orphan and create stale + new_product = Product("Orphan", "Added later") + new_product.shop_id = shop.id + new_product.visibility = 1 + self.dbsession.add(new_product) + products[0].visibility = 0 # now private → stale in ring + self.dbsession.flush() + + before = validate_discovery_ring(shop) + self.assertFalse(before["valid"]) + + reforge_discovery_ring(shop) + self.dbsession.flush() + + after = validate_discovery_ring(shop) + self.assertTrue(after["valid"]) + + class TestKaraokeTrackAclIntegration(DatabaseIntegrationTests): """Integration tests for karaoke track ACL behavior across visibility changes.""" diff --git a/make_post_sell/tests/test_models.py b/make_post_sell/tests/test_models.py index 4a8f42f..64c7246 100644 --- a/make_post_sell/tests/test_models.py +++ b/make_post_sell/tests/test_models.py @@ -2566,6 +2566,128 @@ class TestDiscoveryRing(unittest.TestCase): self.assertEqual(json.loads(shop.json_discovery_ring), ring) +class TestValidateDiscoveryRing(unittest.TestCase): + """Test the ring topology validator.""" + + def _make_shop(self): + return Shop( + "validate-shop", + "555-555-5555", + "123 Ring St", + "Shop for validator tests", + ) + + def _make_product(self, title, desc, visibility=1): + from ..models.product import Product + p = Product(title, desc) + p.visibility = visibility + p.created_timestamp = 100 + return p + + def test_valid_ring(self): + """Ring exactly matches public products → valid.""" + from ..models.shop import validate_discovery_ring + shop = self._make_shop() + p1 = self._make_product("A", "alpha") + p2 = self._make_product("B", "beta") + p3 = self._make_product("C", "gamma") + shop.discovery_ring = [str(p1.id), str(p2.id), str(p3.id)] + with mock.patch.object(type(shop), 'products', new_callable=mock.PropertyMock) as mp: + mp.return_value = [p1, p2, p3] + health = validate_discovery_ring(shop) + self.assertTrue(health["valid"]) + self.assertEqual(health["ring_length"], 3) + self.assertEqual(health["public_count"], 3) + self.assertEqual(health["duplicates"], []) + self.assertEqual(health["orphans"], []) + self.assertEqual(health["stale"], []) + self.assertFalse(health["length_mismatch"]) + + def test_empty_shop_empty_ring_is_valid(self): + """No products + empty ring is still a valid state.""" + from ..models.shop import validate_discovery_ring + shop = self._make_shop() + shop.discovery_ring = [] + with mock.patch.object(type(shop), 'products', new_callable=mock.PropertyMock) as mp: + mp.return_value = [] + health = validate_discovery_ring(shop) + self.assertTrue(health["valid"]) + self.assertEqual(health["ring_length"], 0) + self.assertEqual(health["public_count"], 0) + + def test_detects_duplicates(self): + """Same ID appearing twice in the ring is flagged.""" + from ..models.shop import validate_discovery_ring + shop = self._make_shop() + p1 = self._make_product("A", "alpha") + p2 = self._make_product("B", "beta") + shop.discovery_ring = [str(p1.id), str(p2.id), str(p1.id)] + with mock.patch.object(type(shop), 'products', new_callable=mock.PropertyMock) as mp: + mp.return_value = [p1, p2] + health = validate_discovery_ring(shop) + self.assertFalse(health["valid"]) + self.assertEqual(health["duplicates"], [str(p1.id)]) + + def test_detects_orphans(self): + """Public products missing from ring are flagged as orphans.""" + from ..models.shop import validate_discovery_ring + shop = self._make_shop() + p1 = self._make_product("A", "alpha") + p2 = self._make_product("B", "beta") + p3 = self._make_product("C", "gamma") + # p3 is public but not in ring (added after reforge) + shop.discovery_ring = [str(p1.id), str(p2.id)] + with mock.patch.object(type(shop), 'products', new_callable=mock.PropertyMock) as mp: + mp.return_value = [p1, p2, p3] + health = validate_discovery_ring(shop) + self.assertFalse(health["valid"]) + self.assertEqual(health["orphans"], [str(p3.id)]) + self.assertTrue(health["length_mismatch"]) + + def test_detects_stale(self): + """Ring IDs whose products became non-public are stale.""" + from ..models.shop import validate_discovery_ring + shop = self._make_shop() + p1 = self._make_product("A", "alpha", visibility=1) + p2 = self._make_product("B", "beta", visibility=2) # unlisted now + p3 = self._make_product("C", "gamma", visibility=0) # private now + shop.discovery_ring = [str(p1.id), str(p2.id), str(p3.id)] + with mock.patch.object(type(shop), 'products', new_callable=mock.PropertyMock) as mp: + mp.return_value = [p1, p2, p3] + health = validate_discovery_ring(shop) + self.assertFalse(health["valid"]) + self.assertEqual(sorted(health["stale"]), sorted([str(p2.id), str(p3.id)])) + + def test_detects_stale_deleted_product(self): + """Ring IDs that don't exist in shop.products at all are stale.""" + from ..models.shop import validate_discovery_ring + shop = self._make_shop() + p1 = self._make_product("A", "alpha") + ghost_id = "deadbeef-dead-beef-dead-beefdeadbeef" + shop.discovery_ring = [str(p1.id), ghost_id] + with mock.patch.object(type(shop), 'products', new_callable=mock.PropertyMock) as mp: + mp.return_value = [p1] + health = validate_discovery_ring(shop) + self.assertFalse(health["valid"]) + self.assertIn(ghost_id, health["stale"]) + + def test_length_mismatch_without_orphans_or_stale(self): + """Ring with duplicates but all IDs present has length mismatch.""" + from ..models.shop import validate_discovery_ring + shop = self._make_shop() + p1 = self._make_product("A", "alpha") + p2 = self._make_product("B", "beta") + # Duplicate of p1 — ring length 3, public_count 2 + shop.discovery_ring = [str(p1.id), str(p1.id), str(p2.id)] + with mock.patch.object(type(shop), 'products', new_callable=mock.PropertyMock) as mp: + mp.return_value = [p1, p2] + health = validate_discovery_ring(shop) + self.assertFalse(health["valid"]) + self.assertTrue(health["length_mismatch"]) + self.assertEqual(health["ring_length"], 3) + self.assertEqual(health["public_count"], 2) + + class TestAsyncDiscoveryRing(unittest.TestCase): """Test the async ring reforge with dirty bit debounce.""" diff --git a/make_post_sell/views/watch.py b/make_post_sell/views/watch.py index 1dbf765..dab34ba 100644 --- a/make_post_sell/views/watch.py +++ b/make_post_sell/views/watch.py @@ -3,7 +3,7 @@ import logging from pyramid.view import view_config from ..models.product import get_media_type, get_related_products, get_ring_related_products -from ..models.shop import get_shop_by_id +from ..models.shop import get_shop_by_id, validate_discovery_ring from ..lib.currency import cents_to_dollars from ..lib.time_funcs import timestamp_to_ago_string, timestamp_to_datetime @@ -309,3 +309,27 @@ def discovery_ring_json(request): "ring_length": len(ring), "ring": ring, } + + +@view_config(route_name="discovery_ring_health", renderer="json") +def discovery_ring_health(request): + """Mod-only JSON diagnostic for discovery ring topology. + + Returns duplicates, orphans (public products missing from ring), + stale (ring IDs no longer public), and length mismatch flags. + """ + shop_id = request.matchdict.get("shop_id") + shop = get_shop_by_id(request.dbsession, shop_id) + if not shop: + request.response.status_int = 404 + return {"error": "Shop not found"} + + if not (request.user and request.user.authenticated + and request.user.can_edit_shop(shop)): + request.response.status_int = 403 + return {"error": "Shop editor access required"} + + health = validate_discovery_ring(shop) + health["shop_id"] = str(shop.id) + health["shop_name"] = shop.name + return health