(SP: 3) [SHOP] CP-01 commercial policy refactor: decouple locale, enforce UAH storefront, align checkout, and admin pricing cleanup#441
Merged
ViktorSvertoka merged 11 commits intodevelopfrom Apr 2, 2026
Conversation
liudmylasovetovs
requested review from
AM1007,
LesiaUKR and
ViktorSvertoka
as code owners
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 9 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughCentralizes storefront commercial policy (UAH/UA defaults and provider capabilities), localizes admin and checkout UI, requires UAH pricing, delegates provider/currency resolution to server policy, updates checkout/cart/shipping routes, and adds extensive tests and docs reflecting these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (Browser)
participant CartAPI as /api/shop/cart/rehydrate
participant CheckoutAPI as /api/shop/checkout
participant Policy as CommercialPolicy (server)
participant Orders as Orders Service
participant Payment as Payment Provider
Client->>CartAPI: POST items
CartAPI->>Policy: resolveStandardStorefrontCurrency()
CartAPI-->>Client: quote (pricingFingerprint, currency=UAH)
Client->>CheckoutAPI: POST checkout (items, pricingFingerprint, legalConsent)
CheckoutAPI->>Policy: resolveStandardStorefrontProviderCapabilities()
CheckoutAPI->>Orders: createOrderWithItems(..., storefrontCurrency=UAH, providerCandidates)
Orders->>Payment: init payment (chosen provider/method)
Payment-->>Orders: payment intent / invoice
Orders-->>CheckoutAPI: order created
CheckoutAPI-->>Client: order response (currency=UAH, totals)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 240533f1d5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
frontend/lib/tests/shop/checkout-stripe-payments-disabled.test.ts (1)
376-394: Minor redundancy:legalConsentis specified twice.Line 384 explicitly includes
legalConsent: TEST_LEGAL_CONSENTin the body, but thepostCheckouthelper already injectslegalConsent: TEST_LEGAL_CONSENTat lines 230. The spread at line 234 (...args.body) will overwrite the first one, so this works correctly but is redundant.💡 Remove redundant legalConsent from test body
body: { paymentProvider: 'stripe', paymentMethod: 'stripe_card', paymentCurrency: 'UAH', - legalConsent: TEST_LEGAL_CONSENT, items: [{ productId, quantity: 1 }], },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/checkout-stripe-payments-disabled.test.ts` around lines 376 - 394, The test payload in the postCheckout call redundantly sets legalConsent: TEST_LEGAL_CONSENT even though the postCheckout helper already injects legalConsent via TEST_LEGAL_CONSENT; remove the duplicate key from the body object in the test to avoid redundancy (update the test that calls postCheckout in checkout-stripe-payments-disabled.test.ts and remove the explicit legalConsent property from the body passed to postCheckout while keeping TEST_LEGAL_CONSENT referenced in the helper).frontend/lib/tests/shop/admin-product-form-messages.test.ts (1)
24-69: Consider adding locale identification to assertions for easier debugging.When a key is missing, the test fails without indicating which locale caused the failure. Adding context to assertions would improve debuggability.
💡 Suggested improvement
describe('admin product form i18n messages', () => { - it('keeps product form labels under shop.admin.products.form for all supported locales', () => { - for (const messages of [en, uk, pl]) { + it.each([ + ['en', en], + ['uk', uk], + ['pl', pl], + ] as const)('keeps product form labels for %s locale', (localeName, messages) => { expect( getAtPath(messages as Record<string, unknown>, [ 'shop', 'admin', 'products', 'form', 'fields', 'title', ]) - ).toBeTruthy(); + ).toBeTruthy(); // ... remaining assertions - } }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/admin-product-form-messages.test.ts` around lines 24 - 69, The test should surface which locale is missing a key; change the loop to iterate with locale names (e.g. for (const [localeName, messages] of Object.entries({ en, uk, pl })) ) and for each asserted path call getAtPath once into a variable (e.g. const val = getAtPath(messages as Record<string, unknown>, [...])) and if (!val) throw new Error(`Missing key shop.admin.products.form... in locale ${localeName}`) before/instead of expect(val).toBeTruthy(); update all four assertions this way so failures include the locale (references: en, uk, pl, getAtPath, test "keeps product form labels under shop.admin.products.form for all supported locales").frontend/app/[locale]/shop/checkout/error/page.tsx (1)
23-29: Pass the route locale explicitly intogenerateMetadata()to follow the recommendednext-intlpattern.While this currently works due to
dynamic = 'force-dynamic', the official next-intl documentation recommends explicitly passing the locale from params rather than relying on implicit request-locale resolution. This makes metadata generation more robust and prevents issues if the page's rendering mode changes in the future.♻️ Suggested refactor
-export async function generateMetadata(): Promise<Metadata> { - const t = await getTranslations('shop.checkout.errorPage'); +export async function generateMetadata({ + params, +}: { + params: Promise<{ locale: string }>; +}): Promise<Metadata> { + const { locale } = await params; + const t = await getTranslations({ + locale, + namespace: 'shop.checkout.errorPage', + }); return { title: t('metaTitle'), description: t('metaDescription'), }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/checkout/error/page.tsx around lines 23 - 29, The generateMetadata function should accept the route params and pass the route locale explicitly into getTranslations instead of relying on implicit resolution; update the generateMetadata signature to receive ({ params }) (or ({ params: { locale } })) and call getTranslations with that locale and the 'shop.checkout.errorPage' namespace so title/description are generated using params.locale (refer to generateMetadata and getTranslations to locate where to change).frontend/app/[locale]/shop/cart/provider-policy.ts (2)
7-20: Consider adding a brief comment explaining whycurrencyis accepted but unused.The
void args.currencypattern indicates intentional API alignment where currency was previously used for provider selection. A brief comment would clarify this is intentional for the policy refactor:export function resolveInitialProvider(args: { stripeEnabled: boolean; monobankEnabled: boolean; currency: string | null | undefined; }): CheckoutProvider { + // currency accepted for API compatibility; provider selection is capability-based post CP-01 void args.currency;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/cart/provider-policy.ts around lines 7 - 20, Add a short inline comment above or next to the existing void args.currency in resolveInitialProvider explaining that currency is intentionally unused but accepted to preserve the API/shape (e.g., historical provider-selection or future-proofing for currency-based logic); reference the function name resolveInitialProvider and the parameter args.currency so reviewers understand this is purposeful and not a leftover bug.
17-19: Line 19 is redundant given the fallback logic.Lines 17-19:
if (canUseMonobank) return 'monobank'; if (canUseStripe) return 'stripe'; return 'stripe';Line 19 (
return 'stripe') is only reached when both providers are disabled. Consider whether returning'stripe'as a fallback when neither is enabled is the intended behavior, or if this should throw/return a different value.Alternative: Make fallback explicit
if (canUseMonobank) return 'monobank'; if (canUseStripe) return 'stripe'; - return 'stripe'; + return 'stripe'; // Fallback when no provider is enabled; checkout will fail at validation🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/cart/provider-policy.ts around lines 17 - 19, The current provider-selection logic redundantly returns 'stripe' as a final fallback and masks the case when both canUseMonobank and canUseStripe are false; update the logic around the canUseMonobank and canUseStripe checks to make the fallback explicit: either remove the redundant final return and throw an error (or return null/undefined) when neither provider is available, or keep a deliberate fallback by returning 'stripe' but add a comment and/or a boolean guard to document intent; reference the canUseMonobank and canUseStripe conditions in the provider-selection function and update callers to handle the new error/null case if you choose to remove the implicit 'stripe' fallback.frontend/app/[locale]/shop/cart/capabilities.ts (1)
1-14: Consider caching or combining calls ifresolveStandardStorefrontProviderCapabilities()is expensive.Each capability function independently calls
resolveStandardStorefrontProviderCapabilities(). If this resolver performs I/O or non-trivial computation, consider either:
- Exporting a single resolver that returns all capabilities at once for consumers that need multiple values
- Ensuring the underlying implementation caches results per request
For now, this is acceptable if the resolver is cheap (simple env reads).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/cart/capabilities.ts around lines 1 - 14, The three small accessors (resolveStripeCheckoutEnabled, resolveMonobankCheckoutEnabled, resolveMonobankGooglePayEnabled) each call resolveStandardStorefrontProviderCapabilities(), which may be expensive; modify callers to either use a single cached call to resolveStandardStorefrontProviderCapabilities() and read the three properties, or change the module to export a single resolver that returns all capabilities (and update callers to destructure the returned object), or implement per-request memoization inside resolveStandardStorefrontProviderCapabilities() so repeated calls within the same request return a cached result—update usages of the three functions accordingly to avoid redundant expensive calls.frontend/lib/shop/commercial-policy.server.ts (1)
14-16: Use one truthy parser for both env flags.
PAYMENTS_ENABLEDonly accepts exact"true", whileSHOP_MONOBANK_GPAY_ENABLEDacceptstrue/1/yes/on. Equivalent configs can therefore disable Monobank checkout while still enabling the Google Pay sub-flag.♻️ Proposed cleanup
function isFlagEnabled(value: string | undefined): boolean { - return (value ?? '').trim() === 'true'; + const normalized = (value ?? '').trim().toLowerCase(); + return ( + normalized === 'true' || + normalized === '1' || + normalized === 'yes' || + normalized === 'on' + ); } @@ - const rawMonobankGooglePay = ( - readServerEnv('SHOP_MONOBANK_GPAY_ENABLED') ?? '' - ) - .trim() - .toLowerCase(); const monobankGooglePayEnabled = monobankCheckoutEnabled && - (rawMonobankGooglePay === 'true' || - rawMonobankGooglePay === '1' || - rawMonobankGooglePay === 'yes' || - rawMonobankGooglePay === 'on'); + isFlagEnabled(readServerEnv('SHOP_MONOBANK_GPAY_ENABLED'));Also applies to: 28-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/shop/commercial-policy.server.ts` around lines 14 - 16, The helper isFlagEnabled currently only treats the exact string "true" as truthy; change it to normalize the input (trim and toLowerCase) and check against a set of truthy values like {'true','1','yes','on'} so both PAYMENTS_ENABLED and SHOP_MONOBANK_GPAY_ENABLED (and other uses in the same file around the 28-49 region) use the same parser; update all callers to use the single isFlagEnabled function to ensure consistent flag behavior across the file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@frontend/lib/services/products/mutations/update.ts`:
- Around line 83-125: The read of productPrices (existingPriceRows) and the
merged-price validations (building merged, calling assertMergedPricesPolicy and
enforceSaleBadgeRequiresOriginal) must be moved inside the same db.transaction
that performs the upsert/update so they validate the exact write snapshot;
relocate the select-from(productPrices) + merge logic into the transaction
callback, perform a row lock if supported by the DB client before reading, and
run assertMergedPricesPolicy and enforceSaleBadgeRequiresOriginal inside that
transaction before applying any updates to ensure checks are atomic with the
write (references: productPrices, existingPriceRows, merged, mergedRows,
assertMergedPricesPolicy, enforceSaleBadgeRequiresOriginal, db.transaction).
In `@frontend/lib/shop/commercial-policy.ts`:
- Around line 28-40: The function
resolveStandardStorefrontCheckoutProviderCandidates currently short-circuits a
provided requestedProvider before deriving a provider from requestedMethod;
change the logic in resolveStandardStorefrontCheckoutProviderCandidates to first
infer provider(s) from args.requestedMethod via
inferCurrentCheckoutProviderFromMethod, then apply currency/policy filtering and
only after that intersect with args.requestedProvider (if present) so
requestedProvider cannot bypass method-based restrictions; update the parallel
logic at the other occurrence (the block around lines 78-95) similarly, and add
regression tests asserting that requestedProvider: 'monobank' with currency:
'USD' and requestedMethod: 'monobank_invoice' with currency: 'USD' both yield
the tightened (currency-filtered) results.
- Around line 49-65: The helpers
resolveCurrentStandardStorefrontCurrencyFromLocale and
resolveCurrentStandardStorefrontShippingCountryFromLocale still derive currency
and shipping country from the locale, causing non-UK locales to produce null
countries and break shipping logic in callers like buildCheckoutShippingPayload,
resolveShippingAvailability and resolveLocaleAndCurrency; change these helpers
(or the wrappers resolveCurrencyFromLocale/localeToCountry) to stop using
normalizeLocaleTag(locale) as the decision and instead return values from
STANDARD_STOREFRONT_COMMERCIAL_POLICY (currency and shippingCountry)
unconditionally for the standard storefront path so callers always get the fixed
policy values rather than locale-driven ones.
In `@frontend/lib/tests/shop/commercial-policy-delegation.test.ts`:
- Around line 4-7: The beforeEach currently calls vi.clearAllMocks() and
vi.resetModules() but does not remove registrations from vi.doMock(), so mocked
policy stubs can bleed between tests; update the beforeEach in
commercial-policy-delegation.test.ts to explicitly unmock any modules you mock
dynamically (e.g., call vi.unmock('<moduleId>') for the policy stub(s) you use)
after vi.resetModules(), or programmatically iterate known mocked module ids and
call vi.unmock on them so vi.doMock registrations are cleared before each test.
In `@frontend/lib/tests/shop/product-admin-create-pricing-policy.test.ts`:
- Around line 138-158: The test currently uses expect(...).rejects.toMatchObject
which can be satisfied by any object with matching shape; update the assertion
to explicitly check the thrown error class from the service layer by asserting
the promise rejects to an instance of PriceConfigError (using
expect(...).rejects.toBeInstanceOf(PriceConfigError)) and then separately assert
the error properties (e.g., await expect(...).rejects.toMatchObject({ code:
'PRICE_CONFIG_ERROR', currency: 'UAH' })); locate the assertion around
createProduct in the test and replace the single toMatchObject check with these
two awaits referencing createProduct and PriceConfigError.
In `@frontend/lib/tests/shop/public-storefront-read-policy.test.ts`:
- Around line 61-90: The test should assert the exact number of calls and their
precise arguments to prevent hidden legacy fallback queries; replace the three
toHaveBeenNthCalledWith checks with a strict assertion like expecting
shopQueryMocks.getActiveProductsPage toHaveBeenCalledTimes(3) and then compare
shopQueryMocks.getActiveProductsPage.mock.calls (or the equivalent) to an
explicit array of the three expected argument objects (each containing currency:
'UAH', limit: 24, offset: 0, category: undefined, type: undefined, color:
undefined, size: undefined, sort: 'newest') so any extra/legacy call will fail;
target the getActiveProductsPage mock in this test when making the change.
---
Nitpick comments:
In `@frontend/app/`[locale]/shop/cart/capabilities.ts:
- Around line 1-14: The three small accessors (resolveStripeCheckoutEnabled,
resolveMonobankCheckoutEnabled, resolveMonobankGooglePayEnabled) each call
resolveStandardStorefrontProviderCapabilities(), which may be expensive; modify
callers to either use a single cached call to
resolveStandardStorefrontProviderCapabilities() and read the three properties,
or change the module to export a single resolver that returns all capabilities
(and update callers to destructure the returned object), or implement
per-request memoization inside resolveStandardStorefrontProviderCapabilities()
so repeated calls within the same request return a cached result—update usages
of the three functions accordingly to avoid redundant expensive calls.
In `@frontend/app/`[locale]/shop/cart/provider-policy.ts:
- Around line 7-20: Add a short inline comment above or next to the existing
void args.currency in resolveInitialProvider explaining that currency is
intentionally unused but accepted to preserve the API/shape (e.g., historical
provider-selection or future-proofing for currency-based logic); reference the
function name resolveInitialProvider and the parameter args.currency so
reviewers understand this is purposeful and not a leftover bug.
- Around line 17-19: The current provider-selection logic redundantly returns
'stripe' as a final fallback and masks the case when both canUseMonobank and
canUseStripe are false; update the logic around the canUseMonobank and
canUseStripe checks to make the fallback explicit: either remove the redundant
final return and throw an error (or return null/undefined) when neither provider
is available, or keep a deliberate fallback by returning 'stripe' but add a
comment and/or a boolean guard to document intent; reference the canUseMonobank
and canUseStripe conditions in the provider-selection function and update
callers to handle the new error/null case if you choose to remove the implicit
'stripe' fallback.
In `@frontend/app/`[locale]/shop/checkout/error/page.tsx:
- Around line 23-29: The generateMetadata function should accept the route
params and pass the route locale explicitly into getTranslations instead of
relying on implicit resolution; update the generateMetadata signature to receive
({ params }) (or ({ params: { locale } })) and call getTranslations with that
locale and the 'shop.checkout.errorPage' namespace so title/description are
generated using params.locale (refer to generateMetadata and getTranslations to
locate where to change).
In `@frontend/lib/shop/commercial-policy.server.ts`:
- Around line 14-16: The helper isFlagEnabled currently only treats the exact
string "true" as truthy; change it to normalize the input (trim and toLowerCase)
and check against a set of truthy values like {'true','1','yes','on'} so both
PAYMENTS_ENABLED and SHOP_MONOBANK_GPAY_ENABLED (and other uses in the same file
around the 28-49 region) use the same parser; update all callers to use the
single isFlagEnabled function to ensure consistent flag behavior across the
file.
In `@frontend/lib/tests/shop/admin-product-form-messages.test.ts`:
- Around line 24-69: The test should surface which locale is missing a key;
change the loop to iterate with locale names (e.g. for (const [localeName,
messages] of Object.entries({ en, uk, pl })) ) and for each asserted path call
getAtPath once into a variable (e.g. const val = getAtPath(messages as
Record<string, unknown>, [...])) and if (!val) throw new Error(`Missing key
shop.admin.products.form... in locale ${localeName}`) before/instead of
expect(val).toBeTruthy(); update all four assertions this way so failures
include the locale (references: en, uk, pl, getAtPath, test "keeps product form
labels under shop.admin.products.form for all supported locales").
In `@frontend/lib/tests/shop/checkout-stripe-payments-disabled.test.ts`:
- Around line 376-394: The test payload in the postCheckout call redundantly
sets legalConsent: TEST_LEGAL_CONSENT even though the postCheckout helper
already injects legalConsent via TEST_LEGAL_CONSENT; remove the duplicate key
from the body object in the test to avoid redundancy (update the test that calls
postCheckout in checkout-stripe-payments-disabled.test.ts and remove the
explicit legalConsent property from the body passed to postCheckout while
keeping TEST_LEGAL_CONSENT referenced in the helper).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3c34a5a3-9d82-4424-8655-74234928cb46
📒 Files selected for processing (54)
frontend/app/[locale]/admin/shop/products/_components/ProductForm.tsxfrontend/app/[locale]/shop/cart/CartPageClient.tsxfrontend/app/[locale]/shop/cart/capabilities.tsfrontend/app/[locale]/shop/cart/provider-policy.tsfrontend/app/[locale]/shop/checkout/error/page.tsxfrontend/app/[locale]/shop/checkout/payment/StripePaymentClient.tsxfrontend/app/api/shop/cart/rehydrate/route.tsfrontend/app/api/shop/checkout/route.tsfrontend/app/api/shop/shipping/methods/route.tsfrontend/app/api/shop/shipping/np/cities/route.tsfrontend/app/api/shop/shipping/np/warehouses/route.tsfrontend/docs/shop/commercial-policy-batch-cp-01-acceptance.mdfrontend/docs/shop/commercial-policy-batch-cp-01.mdfrontend/lib/services/orders/checkout.tsfrontend/lib/services/products/mutations/create.tsfrontend/lib/services/products/mutations/update.tsfrontend/lib/services/products/prices.tsfrontend/lib/shop/checkout-display-currency.tsfrontend/lib/shop/commercial-policy.server.tsfrontend/lib/shop/commercial-policy.tsfrontend/lib/shop/currency.tsfrontend/lib/shop/data.tsfrontend/lib/shop/locale.tsfrontend/lib/shop/payments.tsfrontend/lib/tests/shop/admin-product-form-messages.test.tsfrontend/lib/tests/shop/admin-product-patch-price-config-error-contract.test.tsfrontend/lib/tests/shop/cart-public-provider-policy.test.tsfrontend/lib/tests/shop/cart-rehydrate-route-policy.test.tsfrontend/lib/tests/shop/catalog-merchandising-cleanup.test.tsfrontend/lib/tests/shop/checkout-currency-policy.test.tsfrontend/lib/tests/shop/checkout-display-currency.test.tsfrontend/lib/tests/shop/checkout-legal-consent-contract.test.tsfrontend/lib/tests/shop/checkout-monobank-happy-path.test.tsfrontend/lib/tests/shop/checkout-monobank-idempotency-contract.test.tsfrontend/lib/tests/shop/checkout-monobank-parse-validation.test.tsfrontend/lib/tests/shop/checkout-no-payments.test.tsfrontend/lib/tests/shop/checkout-stripe-payments-disabled.test.tsfrontend/lib/tests/shop/commercial-policy-delegation.test.tsfrontend/lib/tests/shop/commercial-policy.test.tsfrontend/lib/tests/shop/currency.test.tsfrontend/lib/tests/shop/order-items-snapshot-immutable.test.tsfrontend/lib/tests/shop/product-admin-create-pricing-policy.test.tsfrontend/lib/tests/shop/product-admin-merged-prices-policy.test.tsfrontend/lib/tests/shop/product-photo-plan-fixes.test.tsfrontend/lib/tests/shop/product-prices-write-authority-phase8.test.tsfrontend/lib/tests/shop/public-product-visibility.test.tsfrontend/lib/tests/shop/public-storefront-read-policy.test.tsfrontend/lib/tests/shop/shipping-methods-route-p2.test.tsfrontend/lib/tests/shop/shipping-np-cities-route-p2.test.tsfrontend/lib/tests/shop/shipping-np-warehouses-route-p2.test.tsfrontend/lib/validation/shop.tsfrontend/messages/en.jsonfrontend/messages/pl.jsonfrontend/messages/uk.json
frontend/lib/tests/shop/product-admin-create-pricing-policy.test.ts
Outdated
Show resolved
Hide resolved
…licy, and harden tests
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
frontend/lib/tests/shop/currency.test.ts (1)
53-81: Consider table-driven locale assertions to reduce repetition.This block is correct, but the repeated uk/en/pl setup across three tests can be simplified for easier extension and maintenance.
♻️ Optional refactor sketch
+const LOCALES = ['uk', 'en', 'pl'] as const; + +function expectSameAcrossLocales(render: (locale: (typeof LOCALES)[number]) => string) { + const [base, ...rest] = LOCALES.map(render); + for (const value of rest) expect(value).toBe(base); + return base; +} + describe('UAH storefront formatting', () => { it('formats UAH identically across uk / en / pl in Ukrainian storefront style', () => { - const uk = formatMoney(200000, 'UAH', 'uk'); - const en = formatMoney(200000, 'UAH', 'en'); - const pl = formatMoney(200000, 'UAH', 'pl'); - - expect(en).toBe(uk); - expect(pl).toBe(uk); + const uk = expectSameAcrossLocales((locale) => formatMoney(200000, 'UAH', locale)); expect(normalizeRenderedSpacing(uk)).toBe('2 000,00 ₴'); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/currency.test.ts` around lines 53 - 81, Replace the repeated uk/en/pl assertions with a table-driven loop: for each test (formatMoney, formatMoneyCode, formatPrice) create an array of locales const locales = ['uk','en','pl'] and compute the canonical value using the first locale (e.g., canonical = formatMoney(200000,'UAH','uk')), then iterate over locales and assert each locale's output equals canonical; use normalizeRenderedSpacing(canonical) for the final expectation that checks the exact rendered string. Reference the functions formatMoney, formatMoneyCode, formatPrice and helper normalizeRenderedSpacing when locating where to consolidate the assertions.frontend/lib/tests/shop/product-admin-create-pricing-policy.test.ts (2)
31-40: Do not swallow teardown failures inafterEach.Silently ignoring delete errors can mask DB cleanup regressions and leak state across tests.
♻️ Suggested teardown hardening
afterEach(async () => { for (const productId of createdProductIds.splice(0)) { - await db - .delete(productPrices) - .where(eq(productPrices.productId, productId)) - .catch(() => undefined); - await db - .delete(products) - .where(eq(products.id, productId)) - .catch(() => undefined); + await db.delete(productPrices).where(eq(productPrices.productId, productId)); + await db.delete(products).where(eq(products.id, productId)); } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/product-admin-create-pricing-policy.test.ts` around lines 31 - 40, The afterEach teardown currently swallows DB delete errors by using .catch(() => undefined) on the db.delete calls (in the afterEach that iterates over createdProductIds), which hides failures; change the teardown to let errors surface so tests fail on cleanup issues — remove the silent .catch handlers on the db.delete(...).where(eq(productPrices.productId,...)) and db.delete(...).where(eq(products.id,...)) calls (or replace them with a try/catch that logs the error and rethrows) so any deletion failure in afterEach is reported by the test runner.
93-101: Guardlegacybefore dereferencing fields.If no row is returned, this currently throws a
TypeErrorinstead of a clear test failure message.🧪 Suggested assertion guard
const [legacy] = await db .select({ price: products.price, originalPrice: products.originalPrice, currency: products.currency, }) .from(products) .where(eq(products.id, created.id)) .limit(1); + expect(legacy).toBeDefined(); + if (!legacy) throw new Error('Expected product legacy row to exist'); + expect(legacy.currency).toBe('USD'); expect(String(legacy.price)).toBe(String(toDbMoney(5100))); expect(legacy.originalPrice).toBeNull();Also applies to: 130-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/product-admin-create-pricing-policy.test.ts` around lines 93 - 101, The test dereferences the result tuple "legacy" returned from the DB select without checking whether a row was returned, which can cause a TypeError; add an explicit guard/assertion right after the select (the const [legacy] = await db.select(...).from(products).where(eq(products.id, created.id)).limit(1) call) such as asserting legacy is defined (with a clear failure message) before accessing legacy.price/originalPrice/currency, and apply the same guard to the other identical select/assertion block later in the file that also destructures into "legacy".frontend/app/[locale]/shop/checkout/error/page.tsx (1)
233-238: Remove unnecessaryanytype casts.The
(order as any)casts bypass type checking despite the fields being properly typed. SincegetOrderSummaryreturnsOrderSummaryWithMinor(which includes bothcurrencyandtotalAmountMinorvia theorderSummarySchema), these fields are accessible without casting:const totalMinor = - typeof (order as any).totalAmountMinor === 'number' - ? (order as any).totalAmountMinor + typeof order.totalAmountMinor === 'number' + ? order.totalAmountMinor : null; - const currency = resolveCheckoutDisplayCurrency((order as any).currency); + const currency = resolveCheckoutDisplayCurrency(order.currency);The
currencyresolution logic is correct—resolveCheckoutDisplayCurrencysafely handles null/undefined by falling back to the standard storefront currency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/checkout/error/page.tsx around lines 233 - 238, Remove the unnecessary (order as any) casts and use the typed OrderSummaryWithMinor returned by getOrderSummary: access order.totalAmountMinor (check typeof order.totalAmountMinor === 'number' to set totalMinor) and access order.currency directly when calling resolveCheckoutDisplayCurrency; the orderSummarySchema/getOrderSummary guarantee these fields exist so remove the casts around totalAmountMinor and currency while keeping the existing null/undefined-safe logic in resolveCheckoutDisplayCurrency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/app/`[locale]/shop/checkout/error/page.tsx:
- Around line 233-238: Remove the unnecessary (order as any) casts and use the
typed OrderSummaryWithMinor returned by getOrderSummary: access
order.totalAmountMinor (check typeof order.totalAmountMinor === 'number' to set
totalMinor) and access order.currency directly when calling
resolveCheckoutDisplayCurrency; the orderSummarySchema/getOrderSummary guarantee
these fields exist so remove the casts around totalAmountMinor and currency
while keeping the existing null/undefined-safe logic in
resolveCheckoutDisplayCurrency.
In `@frontend/lib/tests/shop/currency.test.ts`:
- Around line 53-81: Replace the repeated uk/en/pl assertions with a
table-driven loop: for each test (formatMoney, formatMoneyCode, formatPrice)
create an array of locales const locales = ['uk','en','pl'] and compute the
canonical value using the first locale (e.g., canonical =
formatMoney(200000,'UAH','uk')), then iterate over locales and assert each
locale's output equals canonical; use normalizeRenderedSpacing(canonical) for
the final expectation that checks the exact rendered string. Reference the
functions formatMoney, formatMoneyCode, formatPrice and helper
normalizeRenderedSpacing when locating where to consolidate the assertions.
In `@frontend/lib/tests/shop/product-admin-create-pricing-policy.test.ts`:
- Around line 31-40: The afterEach teardown currently swallows DB delete errors
by using .catch(() => undefined) on the db.delete calls (in the afterEach that
iterates over createdProductIds), which hides failures; change the teardown to
let errors surface so tests fail on cleanup issues — remove the silent .catch
handlers on the db.delete(...).where(eq(productPrices.productId,...)) and
db.delete(...).where(eq(products.id,...)) calls (or replace them with a
try/catch that logs the error and rethrows) so any deletion failure in afterEach
is reported by the test runner.
- Around line 93-101: The test dereferences the result tuple "legacy" returned
from the DB select without checking whether a row was returned, which can cause
a TypeError; add an explicit guard/assertion right after the select (the const
[legacy] = await db.select(...).from(products).where(eq(products.id,
created.id)).limit(1) call) such as asserting legacy is defined (with a clear
failure message) before accessing legacy.price/originalPrice/currency, and apply
the same guard to the other identical select/assertion block later in the file
that also destructures into "legacy".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f70c320b-6dbd-4f86-b00f-1a6a29097ddd
📒 Files selected for processing (12)
frontend/app/[locale]/shop/checkout/error/page.tsxfrontend/lib/services/products/mutations/update.tsfrontend/lib/shop/commercial-policy.server.tsfrontend/lib/shop/commercial-policy.tsfrontend/lib/tests/shop/admin-product-form-messages.test.tsfrontend/lib/tests/shop/checkout-stripe-payments-disabled.test.tsfrontend/lib/tests/shop/commercial-policy-delegation.test.tsfrontend/lib/tests/shop/commercial-policy.test.tsfrontend/lib/tests/shop/currency.test.tsfrontend/lib/tests/shop/product-admin-create-pricing-policy.test.tsfrontend/lib/tests/shop/public-cart-env-contract.test.tsfrontend/lib/tests/shop/public-storefront-read-policy.test.ts
✅ Files skipped from review due to trivial changes (3)
- frontend/lib/tests/shop/public-cart-env-contract.test.ts
- frontend/lib/tests/shop/admin-product-form-messages.test.ts
- frontend/lib/tests/shop/commercial-policy.test.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- frontend/lib/tests/shop/checkout-stripe-payments-disabled.test.ts
- frontend/lib/services/products/mutations/update.ts
- frontend/lib/tests/shop/commercial-policy-delegation.test.ts
- frontend/lib/tests/shop/public-storefront-read-policy.test.ts
- frontend/lib/shop/commercial-policy.server.ts
- frontend/lib/shop/commercial-policy.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
frontend/lib/tests/shop/currency.test.ts (1)
59-61: Consider adding locale context to loop assertions for easier debugging.When a loop assertion fails, the default error message won't indicate which locale caused the failure. Adding context would make test failures easier to diagnose.
♻️ Proposed refactor for clearer assertion messages
for (const locale of otherLocales) { - expect(formatMoney(200000, 'UAH', locale)).toBe(canonical); + expect( + formatMoney(200000, 'UAH', locale), + `formatMoney should match canonical for locale "${locale}"` + ).toBe(canonical); }Same pattern can be applied to the other loops at lines 70-72 and 84-90.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/lib/tests/shop/currency.test.ts` around lines 59 - 61, The loop assertions using formatMoney(200000, 'UAH', locale) against canonical lack locale context on failure; update the loop over otherLocales to include the failing locale in the assertion output (for example, compute const result = formatMoney(...) and throw or fail with a message containing the locale when result !== canonical, or convert the loop to a test.each/table-driven test that includes locale in the test name) and apply the same change to the other similar loops that call formatMoney in this test file so any failing assertion reports which locale caused it.frontend/app/[locale]/shop/checkout/error/page.tsx (1)
90-93: Consider simplifying Promise detection.The duck-typing approach with
(searchParams as any).thenworks but is somewhat awkward. Since Next.js 15 always providessearchParamsas a Promise in async Server Components, you could simplify to always await if the type union is only for documentation purposes.Alternatively, a cleaner type guard could be used if you need to support both patterns.
♻️ Optional: Simpler approach if always Promise
- const resolvedSearchParams: SearchParams | undefined = - searchParams && typeof (searchParams as any).then === 'function' - ? await (searchParams as Promise<SearchParams>) - : (searchParams as SearchParams | undefined); + const resolvedSearchParams = searchParams ? await searchParams : undefined;This assumes
searchParamstype is updated to justPromise<SearchParams> | undefined.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/app/`[locale]/shop/checkout/error/page.tsx around lines 90 - 93, The duck-typing check for searchParams is awkward—replace the conditional with a simpler approach: if your component always receives a Promise (Next.js 15 async Server Component), change the handling of searchParams so resolvedSearchParams is obtained by awaiting the promise (await searchParams) or update the declared type to Promise<SearchParams> | undefined and simply await it; otherwise implement a tiny type guard like isPromise(obj): obj != null && typeof (obj as any).then === 'function' and use that to await only when needed. Ensure you update the usage around the resolvedSearchParams variable and reference the searchParams identifier and resolvedSearchParams assignment site when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@frontend/app/`[locale]/shop/checkout/error/page.tsx:
- Around line 90-93: The duck-typing check for searchParams is awkward—replace
the conditional with a simpler approach: if your component always receives a
Promise (Next.js 15 async Server Component), change the handling of searchParams
so resolvedSearchParams is obtained by awaiting the promise (await searchParams)
or update the declared type to Promise<SearchParams> | undefined and simply
await it; otherwise implement a tiny type guard like isPromise(obj): obj != null
&& typeof (obj as any).then === 'function' and use that to await only when
needed. Ensure you update the usage around the resolvedSearchParams variable and
reference the searchParams identifier and resolvedSearchParams assignment site
when making the change.
In `@frontend/lib/tests/shop/currency.test.ts`:
- Around line 59-61: The loop assertions using formatMoney(200000, 'UAH',
locale) against canonical lack locale context on failure; update the loop over
otherLocales to include the failing locale in the assertion output (for example,
compute const result = formatMoney(...) and throw or fail with a message
containing the locale when result !== canonical, or convert the loop to a
test.each/table-driven test that includes locale in the test name) and apply the
same change to the other similar loops that call formatMoney in this test file
so any failing assertion reports which locale caused it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e0bbb34a-7dde-4377-8ee6-0bc8efbb0a6c
📒 Files selected for processing (3)
frontend/app/[locale]/shop/checkout/error/page.tsxfrontend/lib/tests/shop/currency.test.tsfrontend/lib/tests/shop/product-admin-create-pricing-policy.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/lib/tests/shop/product-admin-create-pricing-policy.test.ts
ViktorSvertoka
approved these changes
Apr 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR implements Batch CP-01 — Commercial Policy Refactor on Stabilized Shop Baseline.
The goal of this batch is to remove the incorrect coupling between
localeand commercial behavior (currency, payment providers, shipping), while preserving all stabilized shop flows.Key outcome:
localenow controls language/content onlyRelated Issue
Issue: #<issue_number>
Changes
PR-A — Policy memo + test repair
LEGAL_CONSENT_REQUIREDPR-B — Centralized commercial policy
commercial-policy.tscommercial-policy.server.tsPR-C — Storefront read-path switch
PR-D — Checkout/server enforcement
UAHacross all localesPR-E — Acceptance & regression gate
PR-F — Admin pricing cleanup
Additional improvements
uk-UAformatting everywhere)Database Changes (if applicable)
How Has This Been Tested?
Test strategy
Screenshots (if applicable)
Checklist
Before submitting
Reviewers
Summary by CodeRabbit
New Features
Improvements
Documentation