Localize Spark starter copy defaults#25
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
|
|
||
| ROOT = Path(__file__).resolve().parents[1] | ||
|
|
||
| LOCALIZED_DEFAULT_OVERRIDE_SETTINGS = { |
There was a problem hiding this comment.
SUGGESTION: LOCALIZED_DEFAULT_OVERRIDE_SETTINGS is a hand-maintained set duplicated from the schema/partials. If a future contributor adds a new starter-copy setting (schema field with a localized {% t %} fallback in a template) but forgets to update this list, the test won't catch it — the contract is opt-in rather than derived. Consider generating this set from {% t %} usage combined with template-introspection (e.g., look for patterns like settings.X|default:default_X in templates), or at minimum add a comment in the schema/partials pointing back to this constant so the linkage is discoverable.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @@ -561,7 +556,6 @@ | |||
| "name": "promo_banner_heading", | |||
| "label": "Heading", | |||
There was a problem hiding this comment.
WARNING: Removing the default for promo_banner_heading is inconsistent with the rest of this PR. Every other removed default is paired with a {% t %} fallback in the corresponding template (section_hero.html, section_featured_products.html, section_featured_categories.html, section_on_sale.html, block_cart_upsell.html, etc.). Here, partials/section_promo_banner.html still uses the heading as a section-render gate and has no localized fallback — so dropping the default means the banner disappears for new installs.
Either drop promo_banner_heading from STARTER_COPY_SETTINGS (i.e., restore the default) or update the template to render a localized fallback heading.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| {% if settings.enable_upsells %} | ||
| <div class="spark-upsells" data-upsell-section> | ||
| {% if settings.upsell_section_title %} | ||
| {% t "side_cart.default.upsell_section_title" as default_upsell_section_title %} |
There was a problem hiding this comment.
WARNING: The previous {% if settings.upsell_section_title %} guard was removed, and the new default_upsell_section_title is unconditionally populated from the locale file. Net effect: whenever settings.enable_upsells is true, the <h3>...</h3> heading renders even if no upsell products are configured and the merchant never set a title. That always-on heading is a behavior change from the prior implementation, which suppressed the heading when no title was set.
Consider keeping the heading suppressed when there are no upsell products (e.g., gate on the upsell product settings) or restore the original guard if the always-on heading is unintentional.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| class LocalizationContractTests(unittest.TestCase): | ||
| def test_template_translation_keys_exist_in_every_locale(self): | ||
| template_dirs = ["layouts", "partials", "templates"] | ||
| key_pattern = re.compile(r"{%\s*t\s+[\"']([^\"']+)[\"']") |
There was a problem hiding this comment.
SUGGESTION: The translation-key regex {%\s*t\s+["\']([^"\']+)["\'] will silently miss {% t %} invocations written with Django whitespace control ({%- t "key" %} or {% t "key" -%}). All current callers use the plain form, so tests pass today, but a future contributor using {%- t %} would have their key silently dropped from the contract check, allowing missing-key drift to slip back in.
Consider tightening the regex to accept an optional - after {% and before %}: {%-?\s*t\s+["\']([^"\']+)["\'].
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 1 New Issue Found | Recommendation: Address before merge Overview
Resolution of previous review (commit
|
| Previous File | Resolution |
|---|---|
partials/section_promo_banner.html (CRITICAL) |
{% if settings.promo_banner_heading %} guard removed; section now always renders with localized default. ✓ |
partials/block_cart_upsell.html (WARNING) |
Entire <div class="spark-upsells"> wrapped in {% if settings.upsell_product_1 or settings.upsell_product_2 or settings.upsell_product_3 %} — heading no longer renders with zero products. ✓ |
partials/recommended_products.html (WARNING) |
Locale recommended_products_header updated to "Recommended products" across all 12 locales; semantic copy regression fixed. ✓ |
configs/settings_schema.json (WARNING) |
promo_banner_heading now backed by {% t "homepage.heading.promo_banner" as default_promo_banner_heading %} fallback. ✓ |
partials/section_featured_product.html (WARNING) |
featured_product_header now uses default:default_featured_product_heading from {% t "homepage.heading.featured_product" %}. ✓ |
tests/test_localization_contracts.py regex (SUGGESTION) |
TRANSLATION_TAG_PATTERN now {%-?\s*t\s+… — handles {%- t "key" %} whitespace-control variants. ✓ |
tests/test_localization_contracts.py hand-maintained list (SUGGESTION) |
New test_localized_default_override_settings_match_template_usage derives the set from templates and asserts equality with the hand-maintained set. ✓ |
New issues from incremental review (commit 0509bec)
SUGGESTION
| File | Line | Issue |
|---|---|---|
tests/test_localization_contracts.py |
77 | localized_default_settings_from_templates scopes assignment and default-pattern matching to the same file's text. If a future contributor puts {% t "..." as default_X %} in a shared parent partial and `settings.Y |
Notes on review coverage
- Dead locale key (not in diff, surfaced via summary):
homepage.placeholder.promo_banneris no longer referenced by any template — the placeholder skeleton inpartials/section_promo_banner.htmlwas removed in this PR. The key still exists in all 12 locale files (e.g.,locales/en.default.json:294). This is dead code introduced by this PR (the template using it was deleted). The testtest_template_translation_keys_exist_in_every_localedoes not detect unused keys; only missing ones. Recommend pruninghomepage.placeholder.promo_bannerfrom all 12 locale files in a follow-up. layouts/base.html,partials/announcement_bar.html,partials/side_cart.html,partials/block_cart_progress_bar.html,partials/recommended_products.html,partials/section_featured_product.html,partials/section_promo_banner.html, and the 12 locale files were all reviewed; no other issues found.
Fix these issues in Kilo Cloud
Previous Review Summary (commit 9fd5102)
Current summary above is authoritative. Previous snapshots are kept for context only.
Previous review (commit 9fd5102)
Status: 7 Issues Found | Recommendation: Address before merge
Overview
| Severity | Count |
|---|---|
| CRITICAL | 1 |
| WARNING | 4 |
| SUGGESTION | 2 |
Issue Details (click to expand)
CRITICAL
| File | Line | Issue |
|---|---|---|
partials/section_promo_banner.html |
3 | Schema promo_banner_heading default removed + settings_data.json starter value removed, but this template still gates the entire section on {% if settings.promo_banner_heading %}. New installs will never render the promo banner. (Inline-comment blocked — file not in PR diff; surfaced via summary.) |
WARNING
| File | Line | Issue |
|---|---|---|
partials/block_cart_upsell.html |
10 | Removed {% if settings.upsell_section_title %} guard; default_upsell_section_title is always populated from locale, so the heading now always renders when enable_upsells is true, even with zero upsell products configured. |
partials/recommended_products.html |
11 | Removing the Recommended Products schema default shifts the fallback to store.catalogue.recommended_products_header ("Customers who viewed this item also bought") — a meaningful semantic copy change for upgrading merchants. (Inline-comment blocked — file not in PR diff.) |
configs/settings_schema.json |
557 | promo_banner_heading default removal is inconsistent: every other removed default pairs with a {% t %} fallback in its template; this one doesn't. |
partials/section_featured_product.html |
6 | featured_product_header still has no localized fallback even though its sibling featured_product_cta_text (line 38) does — inconsistent with the PR's intent. (Inline-comment blocked — file not in PR diff.) |
SUGGESTION
| File | Line | Issue |
|---|---|---|
tests/test_localization_contracts.py |
66 | Translation-key regex won't match {%- t "key" %} (whitespace-control variants) — future contributors could silently bypass the missing-key check. |
tests/test_localization_contracts.py |
9 | LOCALIZED_DEFAULT_OVERRIDE_SETTINGS is hand-maintained rather than derived; future starter-copy settings can drift out of the contract silently. |
Files Reviewed (17 files)
configs/settings_data.json- 1 issue (summary)configs/settings_schema.json- 1 issue (inline)locales/de.json- cleanlocales/en.default.json- cleanlocales/es.json- cleanlocales/fi.json- cleanlocales/fr.json- cleanlocales/it.json- cleanlocales/nl.json- cleanlocales/no.json- cleanlocales/pt.json- cleanlocales/sv.json- cleanlocales/th.json- cleanpartials/block_cart_upsell.html- 1 issue (inline)partials/section_featured_categories.html- cleanpartials/section_featured_products.html- cleantests/test_localization_contracts.py- 2 issues (inline)
Notes on review coverage
partials/section_promo_banner.html,partials/recommended_products.html, andpartials/section_featured_product.htmlwere not modified by this PR but are affected by the schema/setting changes; their issues are reported in the summary because they fall outside the diff hunks and cannot receive inline comments.
Reviewed by minimax-m3 · Input: 125.9K · Output: 17.5K · Cached: 3M
| yield from (ROOT / dirname).rglob("*.html") | ||
|
|
||
|
|
||
| def localized_default_settings_from_templates(): |
There was a problem hiding this comment.
SUGGESTION: localized_default_settings_from_templates scopes LOCALIZED_DEFAULT_ASSIGNMENT_PATTERN and SETTING_DEFAULT_PATTERN to the same file's text. If a future contributor puts the {% t "..." as default_X %} assignment in a shared parent partial and the settings.Y|default:default_X reference in a child include, the test will not detect Y (false negative) and the exact-equality assertion will fail (false positive). Consider collecting assignments and references across all template files and joining them once, so the test mirrors the runtime scoping of the variables rather than the file-scoping of re.findall.
| def localized_default_settings_from_templates(): | |
| def localized_default_settings_from_templates(): | |
| settings = set() | |
| localized_default_vars = set() | |
| for path in template_paths(): | |
| text = path.read_text() | |
| localized_default_vars.update(LOCALIZED_DEFAULT_ASSIGNMENT_PATTERN.findall(text)) | |
| for path in template_paths(): | |
| text = path.read_text() | |
| for setting_name, default_var in SETTING_DEFAULT_PATTERN.findall(text): | |
| if default_var in localized_default_vars: | |
| settings.add(setting_name) | |
| return settings |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
1 participant
Summary
{% t %}stringsValidation
python3 -m unittest discover -s testspython3 -m json.toolfor schema, settings data, and locale filesgit diff --check