From c9f2dffabf6369df360c5f9ad7a12c0415026310 Mon Sep 17 00:00:00 2001 From: Fringg Date: Fri, 13 Mar 2026 05:45:46 +0300 Subject: [PATCH] fix: address 6-agent review findings for PricingEngine MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit H1: log error when tariff_id set but tariff relationship not loaded H2: warn on CLASSIC_PERIOD_PRICES→PERIOD_PRICES fallback M1: fix float division in miniapp tariff purchase (use PricingEngine.apply_discount) M2: fix format_period Russian pluralization for teen-hundreds (111-119, etc.) M3: deduplicate _resolve_discount_percent — import from pricing_utils M4: fix N+1 queries in compute_simple_subscription_price (batch fetch) M5: add period_days validation tests (negative, zero, float) M6: add user=None tests for tariff and classic modes M7: fix float division in calculate_prorated_price (use // instead of /) L1: add context to _calculate_servers_price error log L2: add comment clarifying ClassicBreakdown.group_discount_pct type L3: add test for original_total property L4: inline _apply_percentage_discount wrapper in subscription_purchase_service L5: replace global _server_id_counter with itertools.count() in tests --- app/services/pricing_engine.py | 22 +++- app/services/subscription_purchase_service.py | 19 ++- app/services/subscription_service.py | 27 +---- app/utils/formatting.py | 20 ++-- app/utils/pricing_utils.py | 12 +- app/webapi/routes/miniapp.py | 4 +- tests/test_pricing_engine.py | 112 +++++++++++++++++- 7 files changed, 161 insertions(+), 55 deletions(-) diff --git a/app/services/pricing_engine.py b/app/services/pricing_engine.py index 0a2a9e6f..579b9db7 100644 --- a/app/services/pricing_engine.py +++ b/app/services/pricing_engine.py @@ -42,6 +42,7 @@ class ClassicBreakdown: base_traffic_gb: int purchased_traffic_gb: int extra_devices: int + # NB: dict[str, int] per-category (period/servers/traffic/devices), unlike TariffBreakdown's single int group_discount_pct: dict[str, int] offer_discount_pct: int @@ -110,8 +111,8 @@ class PricingEngine: try: servers = await get_server_squads_by_uuids(db, country_uuids) - except Exception as e: - logger.error('Ошибка пакетной загрузки серверов', error=str(e)) + except Exception as e: # intentional broad catch: pricing must not crash on DB errors, servers_price=0 is safe (user pays less) + logger.error('Ошибка пакетной загрузки серверов', error=str(e), squad_uuids=country_uuids) return 0, [{'uuid': uuid, 'id': None, 'price': 0, 'status': 'error'} for uuid in country_uuids] server_map = {s.squad_uuid: s for s in servers} @@ -196,8 +197,15 @@ class PricingEngine: if not isinstance(period_days, int) or period_days <= 0: raise ValueError(f'Invalid period_days: {period_days}') - if subscription.tariff_id is not None and subscription.tariff is not None: - return await self._calculate_tariff_mode(db, subscription, period_days, user=user) + if subscription.tariff_id is not None: + if subscription.tariff is None: + logger.error( + 'tariff_id set but tariff relationship not loaded, falling back to classic mode', + subscription_id=getattr(subscription, 'id', None), + tariff_id=subscription.tariff_id, + ) + else: + return await self._calculate_tariff_mode(db, subscription, period_days, user=user) return await self._calculate_classic_mode(db, subscription, period_days, user=user) # ------------------------------------------------------------------ @@ -297,6 +305,12 @@ class PricingEngine: base_price_original = CLASSIC_PERIOD_PRICES.get(period_days) if base_price_original is None: base_price_original = PERIOD_PRICES.get(period_days, 0) + if base_price_original > 0: + logger.warning( + 'CLASSIC_PERIOD_PRICES miss, falling back to PERIOD_PRICES — verify price is not from tariff regime', + period_days=period_days, + fallback_price_kopeks=base_price_original, + ) # --- Per-category discount percents --- period_pct = 0 diff --git a/app/services/subscription_purchase_service.py b/app/services/subscription_purchase_service.py index 1ca9aba3..4a2c163e 100644 --- a/app/services/subscription_purchase_service.py +++ b/app/services/subscription_purchase_service.py @@ -27,6 +27,7 @@ from app.database.models import ServerSquad, Subscription, SubscriptionStatus, T from app.localization.texts import get_texts from app.services.subscription_service import SubscriptionService from app.utils.pricing_utils import ( + apply_percentage_discount, calculate_months_from_days, format_period_description, validate_pricing_calculation, @@ -266,15 +267,9 @@ class PurchaseBalanceError(Exception): super().__init__(message) -def _apply_percentage_discount(amount: int, percent: int) -> tuple[int, int]: - """Delegate to shared apply_percentage_discount (uses PricingEngine internally).""" - from app.utils.pricing_utils import apply_percentage_discount - - return apply_percentage_discount(amount, percent) - def _apply_discount_to_monthly_component(amount_per_month: int, percent: int, months: int) -> dict[str, int]: - discounted_per_month, discount_per_month = _apply_percentage_discount(amount_per_month, percent) + discounted_per_month, discount_per_month = apply_percentage_discount(amount_per_month, percent) return { 'original_per_month': amount_per_month, 'discounted_per_month': discounted_per_month, @@ -293,7 +288,7 @@ def _apply_promo_offer_discount(user: User | None, amount: int) -> tuple[int, in percent = _get_promo_offer_discount_percent(user) if amount <= 0 or percent <= 0: return amount, 0, 0 - discounted, discount_value = _apply_percentage_discount(amount, percent) + discounted, discount_value = apply_percentage_discount(amount, percent) return discounted, discount_value, percent @@ -303,7 +298,7 @@ def _build_server_option( texts, ) -> PurchaseServerOption: base_per_month = int(getattr(server, 'price_kopeks', 0) or 0) - discounted_per_month, _ = _apply_percentage_discount(base_per_month, discount_percent) + discounted_per_month, _ = apply_percentage_discount(base_per_month, discount_percent) return PurchaseServerOption( uuid=server.squad_uuid, name=getattr(server, 'display_name', server.squad_uuid) or server.squad_uuid, @@ -387,7 +382,7 @@ class MiniAppSubscriptionPurchaseService: base_price_original = PERIOD_PRICES.get(period_days, 0) period_discount_percent = user.get_promo_discount('period', period_days) - base_price, base_discount_total = _apply_percentage_discount(base_price_original, period_discount_percent) + base_price, base_discount_total = apply_percentage_discount(base_price_original, period_discount_percent) base_price_label = texts.format_price(base_price) base_price_original_label = ( texts.format_price(base_price_original) @@ -520,7 +515,7 @@ class MiniAppSubscriptionPurchaseService: for package in packages: value = int(package.get('gb') or 0) price_per_month = int(package.get('price') or 0) - discounted_per_month, discount_value = _apply_percentage_discount(price_per_month, discount_percent) + discounted_per_month, discount_value = apply_percentage_discount(price_per_month, discount_percent) label = texts.format_traffic(value or 0) options.append( PurchaseTrafficOption( @@ -594,7 +589,7 @@ class MiniAppSubscriptionPurchaseService: ) -> PurchaseDevicesConfig: discount_percent = user.get_promo_discount('devices', period_days) unit_price = settings.PRICE_PER_DEVICE - discounted_unit_price, unit_discount_value = _apply_percentage_discount(unit_price, discount_percent) + discounted_unit_price, unit_discount_value = apply_percentage_discount(unit_price, discount_percent) price_label = texts.format_price(discounted_unit_price) original_label = ( texts.format_price(unit_price) if unit_discount_value and unit_price != discounted_unit_price else None diff --git a/app/services/subscription_service.py b/app/services/subscription_service.py index a1eef3ff..58f9c4b2 100644 --- a/app/services/subscription_service.py +++ b/app/services/subscription_service.py @@ -14,6 +14,7 @@ from app.database.models import PromoGroup, Subscription, SubscriptionStatus, Us from app.external.remnawave_api import RemnaWaveAPI, RemnaWaveAPIError, RemnaWaveUser, TrafficLimitStrategy, UserStatus from app.utils.pricing_utils import ( calculate_months_from_days, + resolve_discount_percent, ) from app.utils.subscription_utils import ( resolve_hwid_device_limit_for_payload, @@ -23,24 +24,6 @@ from app.utils.subscription_utils import ( logger = structlog.get_logger(__name__) -def _resolve_discount_percent( - user: User | None, - promo_group: PromoGroup | None, - category: str, - *, - period_days: int | None = None, -) -> int: - if user is not None: - try: - return user.get_promo_discount(category, period_days) - except AttributeError: - pass - - if promo_group is not None: - return promo_group.get_discount_percent(category, period_days) - - return 0 - def get_traffic_reset_strategy(tariff=None): """Получает стратегию сброса трафика. @@ -813,7 +796,7 @@ class SubscriptionService: months_in_period = calculate_months_from_days(period_days) base_price_original = PERIOD_PRICES.get(period_days, 0) - period_discount_percent = _resolve_discount_percent( + period_discount_percent = resolve_discount_percent( user, promo_group, 'period', @@ -825,7 +808,7 @@ class SubscriptionService: promo_group = promo_group or (user.get_primary_promo_group() if user else None) traffic_price_per_month = settings.get_traffic_price(traffic_gb) - traffic_discount_percent = _resolve_discount_percent( + traffic_discount_percent = resolve_discount_percent( user, promo_group, 'traffic', @@ -837,7 +820,7 @@ class SubscriptionService: server_prices = [] total_servers_price = 0 - servers_discount_percent = _resolve_discount_percent( + servers_discount_percent = resolve_discount_percent( user, promo_group, 'servers', @@ -865,7 +848,7 @@ class SubscriptionService: additional_devices = max(0, devices - settings.DEFAULT_DEVICE_LIMIT) devices_price_per_month = additional_devices * settings.PRICE_PER_DEVICE - devices_discount_percent = _resolve_discount_percent( + devices_discount_percent = resolve_discount_percent( user, promo_group, 'devices', diff --git a/app/utils/formatting.py b/app/utils/formatting.py index 7bba1a03..7df11e0b 100644 --- a/app/utils/formatting.py +++ b/app/utils/formatting.py @@ -21,12 +21,14 @@ def format_price_kopeks(kopeks: int, compact: bool = False) -> str: def format_period(days: int) -> str: """Форматирует период.""" - if days == 1: - return '1 день' - if days < 5: - return f'{days} дня' - if days < 21 or days % 10 >= 5 or days % 10 == 0: - return f'{days} дней' - if days % 10 == 1: - return f'{days} день' - return f'{days} дня' + mod100 = days % 100 + mod10 = days % 10 + if 11 <= mod100 <= 19: + word = 'дней' + elif mod10 == 1: + word = 'день' + elif 2 <= mod10 <= 4: + word = 'дня' + else: + word = 'дней' + return f'{days} {word}' diff --git a/app/utils/pricing_utils.py b/app/utils/pricing_utils.py index 8bf0267c..3fbbf283 100644 --- a/app/utils/pricing_utils.py +++ b/app/utils/pricing_utils.py @@ -28,7 +28,7 @@ def calculate_prorated_price(monthly_price: int, end_date: datetime, min_charge_ days_remaining = max(1, (end_date - now).days) days_to_charge = max(min_charge_days, days_remaining) - total_price = int(monthly_price * days_to_charge / 30) + total_price = monthly_price * days_to_charge // 30 if monthly_price > 0: total_price = max(100, total_price) # Минимум 1 рубль @@ -163,14 +163,20 @@ async def compute_simple_subscription_price( elif raw_squad: resolved_uuids.append(str(raw_squad)) - from app.database.crud.server_squad import get_server_squad_by_uuid + from app.database.crud.server_squad import get_server_squads_by_uuids server_breakdown: list[dict[str, Any]] = [] servers_price_original = 0 servers_discount_total = 0 + if resolved_uuids: + servers = await get_server_squads_by_uuids(db, resolved_uuids) + server_map = {s.squad_uuid: s for s in servers} + else: + server_map = {} + for squad_uuid in resolved_uuids: - server = await get_server_squad_by_uuid(db, squad_uuid) + server = server_map.get(squad_uuid) if not server: logger.warning('SIMPLE_SUBSCRIPTION_PRICE_SERVER_NOT_FOUND | squad', squad_uuid=squad_uuid) server_breakdown.append( diff --git a/app/webapi/routes/miniapp.py b/app/webapi/routes/miniapp.py index 5a2d0c09..9d3de893 100644 --- a/app/webapi/routes/miniapp.py +++ b/app/webapi/routes/miniapp.py @@ -6463,7 +6463,9 @@ async def purchase_tariff_endpoint( except (TypeError, ValueError): pass if discount_percent > 0: - price_kopeks = int(base_price_kopeks * (100 - discount_percent) / 100) + from app.services.pricing_engine import PricingEngine + + price_kopeks = PricingEngine.apply_discount(base_price_kopeks, discount_percent) # Apply personal promo_offer discount on top of group discount consume_promo_offer = False diff --git a/tests/test_pricing_engine.py b/tests/test_pricing_engine.py index c28a343f..1021e267 100644 --- a/tests/test_pricing_engine.py +++ b/tests/test_pricing_engine.py @@ -1,3 +1,5 @@ +import itertools + import pytest from app.services.pricing_engine import PricingEngine, RenewalPricing @@ -75,16 +77,46 @@ class TestStackedDiscounts: from unittest.mock import AsyncMock, MagicMock, patch -_server_id_counter = 0 +class TestPeriodDaysValidation: + @pytest.mark.asyncio + async def test_negative_period_days_raises(self): + engine = PricingEngine() + db = AsyncMock() + subscription = MagicMock() + subscription.tariff_id = None + subscription.tariff = None + with pytest.raises(ValueError, match='Invalid period_days'): + await engine.calculate_renewal_price(db, subscription, -1) + + @pytest.mark.asyncio + async def test_zero_period_days_raises(self): + engine = PricingEngine() + db = AsyncMock() + subscription = MagicMock() + subscription.tariff_id = None + subscription.tariff = None + with pytest.raises(ValueError, match='Invalid period_days'): + await engine.calculate_renewal_price(db, subscription, 0) + + @pytest.mark.asyncio + async def test_float_period_days_raises(self): + engine = PricingEngine() + db = AsyncMock() + subscription = MagicMock() + subscription.tariff_id = None + subscription.tariff = None + with pytest.raises(ValueError, match='Invalid period_days'): + await engine.calculate_renewal_price(db, subscription, 30.0) + + +_server_id_seq = itertools.count(1) def _make_server( price_kopeks=5000, is_available=True, is_full=False, allowed_promo_groups=None, server_id=None, squad_uuid=None ): - global _server_id_counter if server_id is None: - _server_id_counter += 1 - server_id = _server_id_counter + server_id = next(_server_id_seq) server = MagicMock() server.id = server_id server.squad_uuid = squad_uuid @@ -371,6 +403,29 @@ class TestCalculateRenewalPriceTariffMode: assert result.final_total == 10000 assert result.breakdown.get('extra_devices') == 0 + @pytest.mark.asyncio + async def test_tariff_user_none(self): + """When user=None, no discounts are applied.""" + engine = PricingEngine() + db = AsyncMock() + subscription = MagicMock() + subscription.tariff_id = 1 + subscription.tariff = MagicMock() + subscription.tariff.period_prices = {'30': 20000} + subscription.tariff.device_limit = 1 + subscription.tariff.device_price_kopeks = None + subscription.tariff.id = 1 + subscription.device_limit = 1 + with ( + patch('app.services.pricing_engine.get_user_active_promo_discount_percent', return_value=0), + patch('app.services.pricing_engine.settings') as ms, + ): + ms.PRICE_PER_DEVICE = 5000 + result = await engine.calculate_renewal_price(db, subscription, 30, user=None) + assert result.final_total == 20000 + assert result.promo_group_discount == 0 + assert result.promo_offer_discount == 0 + class TestCalculateRenewalPriceClassicMode: @pytest.mark.asyncio @@ -693,6 +748,33 @@ class TestCalculateRenewalPriceClassicMode: assert result.promo_group_discount == 3700 assert result.final_total == 9000 + 4800 + 3500 + 8000 + @pytest.mark.asyncio + async def test_classic_user_none(self): + """When user=None, no discounts are applied.""" + engine = PricingEngine() + db = AsyncMock() + subscription = MagicMock() + subscription.tariff_id = None + subscription.tariff = None + subscription.connected_squads = [] + subscription.traffic_limit_gb = 0 + subscription.purchased_traffic_gb = 0 + subscription.device_limit = 1 + with ( + patch('app.services.pricing_engine.get_user_active_promo_discount_percent', return_value=0), + patch('app.services.pricing_engine.settings') as ms, + patch('app.services.pricing_engine.CLASSIC_PERIOD_PRICES', {30: 15000}), + patch('app.services.pricing_engine.PERIOD_PRICES', {}), + ): + ms.get_traffic_price.return_value = 0 + ms.PRICE_PER_DEVICE = 0 + ms.DEFAULT_DEVICE_LIMIT = 1 + ms.is_traffic_fixed.return_value = False + result = await engine.calculate_renewal_price(db, subscription, 30, user=None) + assert result.final_total == 15000 + assert result.promo_group_discount == 0 + assert result.promo_offer_discount == 0 + class TestServerPromoGroupFiltering: @pytest.mark.asyncio @@ -864,3 +946,25 @@ class TestOriginalPriceIdentity: # original should equal base_original + servers_original + traffic_original + devices_original expected_original = 10000 + 4000 + 3000 + 5000 # 22000 assert original == expected_original + + @pytest.mark.asyncio + async def test_original_total_property_tariff(self): + """original_total property returns correct value.""" + engine = PricingEngine() + db = AsyncMock() + tariff = MagicMock() + tariff.id = 1 + tariff.period_prices = {'30': 20000} + tariff.device_price_kopeks = None + tariff.device_limit = 1 + sub = MagicMock() + sub.tariff_id = 1 + sub.tariff = tariff + sub.device_limit = 1 + user = MagicMock() + promo_group = MagicMock() + promo_group.get_discount_percent = MagicMock(return_value=10) + user.promo_group = promo_group + with patch('app.services.pricing_engine.get_user_active_promo_discount_percent', return_value=5): + result = await engine.calculate_renewal_price(db, sub, 30, user=user) + assert result.original_total == 20000 # undiscounted subtotal