forgejo_admin/westside-emails
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat: MJML email template system with jersey reminder #2

Merged
forgejo_admin merged 2 commits from 1-feat-mjml-email-template-system-with-jer into main 2026-03-21 17:31:32 +00:00
Conversation 4 Commits 2 Files changed 13 +3447
forgejo_admin commented 2026-03-21 16:55:40 +00:00
Owner
Copy link

Summary

MJML email template system for Westside Kings & Queens. First template is the girls jersey ordering reminder -- branded, professional, with real jersey images from MinIO CDN. Templates compile to HTML with literal {{variable}} placeholders that basketball-api replaces at send time via simple string substitution.

Changes

  • package.json -- mjml dependency, build and preview scripts
  • scripts/build.js -- Node build script compiling src/.mjml to dist/.html
  • src/base-layout.mjml -- shared Westside brand wrapper (logo, red accent header, dark theme, footer)
  • src/jersey-reminder.mjml -- jersey ordering reminder with three options ($90 reversible, $130 jersey+warmup, $0 opt-out), real jersey photos from MinIO CDN, gentle tone
  • sample-data/jersey-reminder.json -- sample data for preview rendering
  • preview/index.html -- browser preview that loads compiled HTML with sample data
  • .gitignore -- excludes node_modules and dist
  • package-lock.json -- lockfile for reproducible builds

Test Plan

  • npm install && npm run build exits 0, produces dist/jersey-reminder.html and dist/base-layout.html
  • dist/jersey-reminder.html contains {{name}} and {{jersey_url}} placeholders
  • npm run preview opens browser preview at localhost:3001
  • Jersey images at MinIO CDN (IMG_4164.jpeg, IMG_4165.jpeg) return HTTP 200

Review Checklist

  • Passed automated review-fix loop
  • No secrets committed
  • No unnecessary file changes
  • Commit messages are descriptive

Related

## Summary MJML email template system for Westside Kings & Queens. First template is the girls jersey ordering reminder -- branded, professional, with real jersey images from MinIO CDN. Templates compile to HTML with literal `{{variable}}` placeholders that basketball-api replaces at send time via simple string substitution. ## Changes - `package.json` -- mjml dependency, build and preview scripts - `scripts/build.js` -- Node build script compiling src/*.mjml to dist/*.html - `src/base-layout.mjml` -- shared Westside brand wrapper (logo, red accent header, dark theme, footer) - `src/jersey-reminder.mjml` -- jersey ordering reminder with three options ($90 reversible, $130 jersey+warmup, $0 opt-out), real jersey photos from MinIO CDN, gentle tone - `sample-data/jersey-reminder.json` -- sample data for preview rendering - `preview/index.html` -- browser preview that loads compiled HTML with sample data - `.gitignore` -- excludes node_modules and dist - `package-lock.json` -- lockfile for reproducible builds ## Test Plan - [x] `npm install && npm run build` exits 0, produces dist/jersey-reminder.html and dist/base-layout.html - [x] `dist/jersey-reminder.html` contains `{{name}}` and `{{jersey_url}}` placeholders - [ ] `npm run preview` opens browser preview at localhost:3001 - [x] Jersey images at MinIO CDN (IMG_4164.jpeg, IMG_4165.jpeg) return HTTP 200 ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #1 - `plan-wkq` -- Phase 11 discovered scope - Parent issue: forgejo_admin/basketball-api#134
- base-layout.mjml: shared Westside brand wrapper with logo, red accent, footer
- jersey-reminder.mjml: girls jersey ordering reminder with three options ($90/$130/opt-out),
  jersey images from MinIO CDN, gentle tone, no pressure
- Build script compiles src/*.mjml to dist/*.html via node
- Preview system loads compiled HTML with sample data in browser
- Brand colors: black #1a1a1a, red #c41230, white #ffffff
- {{name}} and {{jersey_url}} placeholders for simple string replacement at send time

Closes #1

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-03-21 17:03:11 +00:00
Author
Owner
Copy link

Review-fix loop

No issues found. All files are new (greenfield repo).

Verified:

  • npm run build exits 0, compiles both base-layout.mjml and jersey-reminder.mjml to dist/
  • dist/jersey-reminder.html contains {{name}} (1 occurrence) and {{jersey_url}} (2 occurrences) placeholders
  • Jersey images at MinIO CDN (IMG_4164.jpeg, IMG_4165.jpeg) return HTTP 200
  • Westside logo at MinIO CDN returns HTTP 200
  • Brand colors match spec: #1a1a1a (black), #c41230 (red), #ffffff (white)
  • Gentle tone in copy -- no pressure language, opt-out friendly
## Review-fix loop **No issues found.** All files are new (greenfield repo). Verified: - `npm run build` exits 0, compiles both `base-layout.mjml` and `jersey-reminder.mjml` to `dist/` - `dist/jersey-reminder.html` contains `{{name}}` (1 occurrence) and `{{jersey_url}}` (2 occurrences) placeholders - Jersey images at MinIO CDN (`IMG_4164.jpeg`, `IMG_4165.jpeg`) return HTTP 200 - Westside logo at MinIO CDN returns HTTP 200 - Brand colors match spec: `#1a1a1a` (black), `#c41230` (red), `#ffffff` (white) - Gentle tone in copy -- no pressure language, opt-out friendly
forgejo_admin commented 2026-03-21 17:16:22 +00:00
Author
Owner
Copy link

PR #2 Review

DOMAIN REVIEW

Tech stack: Node.js, MJML (email templating), vanilla HTML/JS preview system.

This is a new repo (westside-emails) introducing an MJML-based email template compilation pipeline. The architecture is: MJML source files compile to HTML via npm run build, producing dist/*.html files with {{variable}} placeholders that basketball-api will consume for string substitution before sending via Gmail OAuth.

Build script (scripts/build.js):

  • Clean, well-structured Node script. Uses mjml with validationLevel: "soft", which is appropriate for development but means warnings do not fail the build. Files with errors are still written to disk -- the script increments an errors counter but never exits non-zero for compilation warnings. This is acceptable for a template repo where the human previews output, but worth noting.
  • Uses synchronous fs.readFileSync/fs.writeFileSync -- fine for a build tool, not a server.

Base layout (src/base-layout.mjml):

  • Defines the shared Westside branding: dark theme (#0a0a0a / #1a1a1a / #141414), red accent (#c41230), logo from MinIO CDN, footer with org name and location.
  • Contains a {{content}} placeholder in the body section.

Jersey reminder (src/jersey-reminder.mjml):

  • CRITICAL: This template does NOT use <mj-include path="./base-layout.mjml" /> or any include mechanism. Instead, it duplicates the entire header (logo, org name, red accent bar), all <mj-head> attributes/styles, and the footer verbatim from base-layout.mjml. The base layout file exists but is never referenced by the actual template.
  • This means base-layout.mjml compiles to its own dist/base-layout.html (with a raw {{content}} placeholder), but jersey-reminder.mjml is a fully standalone file that copy-pastes the layout. When branding changes (logo URL, colors, footer text), both files must be updated independently -- exactly the divergence risk that base layouts are designed to prevent.
  • Template content is well-written: three clear pricing options ($90/$130/$0), appropriate alt text on images, fallback URL for non-clickable email clients.
  • Uses {{name}} and {{jersey_url}} placeholders correctly.

Preview system (preview/index.html):

  • Clean vanilla JS implementation. Template registry pattern is extensible for future templates.
  • renderTemplate() uses regex replacement for {{key}} patterns -- correct and sufficient for simple string substitution.
  • Good error handling: shows red error message if HTML file not found, prompts user to run npm run build.
  • iframe srcdoc approach avoids CORS issues with local file serving.

Sample data (sample-data/jersey-reminder.json):

  • Contains a Tailscale URL: https://westsidekingsandqueens.tail5b443a.ts.net/jersey?token=sample-token-abc123. This is sample/preview data, not a real credential, and the file is clearly labeled as sample data. Acceptable.

Package configuration:

  • MJML pinned to ^4.15.3 (caret range allows minor/patch updates). package-lock.json is committed for reproducibility. Good.
  • serve as devDependency for preview. Clean dependency footprint.

BLOCKERS

1. DRY violation: base-layout.mjml exists but is not used (NOT a security/auth path, but a significant architecture concern)

src/base-layout.mjml defines the shared branding layout with a {{content}} slot. However, src/jersey-reminder.mjml duplicates the entire header, mj-head attributes, styles, and footer instead of using <mj-include>. This means:

  • base-layout.mjml compiles to an unusable HTML file (raw {{content}} in output)
  • Future templates will either copy-paste from jersey-reminder (compounding duplication) or use the base layout (creating two divergent patterns)
  • Branding changes require updating every standalone template

MJML supports <mj-include path="./base-layout.mjml" /> natively. The jersey-reminder template should compose with the base layout rather than duplicating it. This is the stated architectural goal in issue #134 ("brand wrapper") and the repo's own file naming convention implies composition.

2. No test coverage for build output

The Test Plan states "Build succeeds and outputs both compiled HTML files" and "Placeholders present: {{name}} and {{jersey_url}}" -- but these are manual verification steps, not automated tests. There are no test scripts, no CI pipeline (.woodpecker.yml is absent), and no assertions that:

  • The build produces expected output files
  • Placeholders survive compilation
  • HTML is valid
  • Template count matches source count

For a template system that basketball-api will consume programmatically, build correctness should be verified automatically. At minimum, a smoke test that runs npm run build and asserts output files exist with expected placeholders would prevent silent regressions. Per BLOCKER criteria: new functionality with zero test coverage.

NITS

  1. README is minimal. A template repo benefits from documenting: how to add a new template, the placeholder convention, how basketball-api consumes the output, and the preview workflow. The current README is one line.

  2. Build script exit code. scripts/build.js exits 0 even when templates have compilation warnings. Consider process.exit(1) when errors > 0 so CI (when added) can catch regressions.

  3. No .woodpecker.yml. This repo lives on Forgejo but has no CI pipeline. Even a simple pipeline that runs npm ci && npm run build would catch broken templates on push.

  4. base-layout.mjml compiles to a broken HTML file. Since it contains {{content}} as literal text (not a valid MJML construct), it produces an HTML file with a raw {{content}} string in the output. If it is meant only as an include fragment, it should either be excluded from the build glob (e.g., prefix with _ and filter in build.js) or moved to a partials/ directory.

  5. Hardcoded MinIO CDN URLs in templates. The logo and jersey image URLs point to minio-api.tail5b443a.ts.net. If the MinIO hostname changes, every template needs updating. Consider a {{cdn_base_url}} placeholder or a build-time variable, though this may be acceptable given the current scale.

  6. Preview iframe height is hardcoded to 1200px. The jersey-reminder email is long with three product cards. This works now but may clip or leave excess whitespace for shorter/longer future templates. Not blocking.

SOP COMPLIANCE

  • Branch named after issue: 1-feat-mjml-email-template-system-with-jer references issue #1
  • PR body has Summary, Changes, Test Plan, Related sections
  • Related section references plan slug: "References plan-wkq Phase 11"
  • No secrets committed (sample token in sample-data is clearly labeled sample data)
  • No unnecessary file changes (all files are part of the template system)
  • Tests exist and pass: No automated tests or CI pipeline

PROCESS OBSERVATIONS

  • Deployment frequency: This is a new repo with a clean initial contribution. The template compilation approach is sound and will enable faster email iteration without API deployments -- a DORA deployment frequency win.
  • Change failure risk: The DRY violation (duplicated layout) increases change failure risk for future template additions. A developer adding a second template will face a choice between copy-pasting 200+ lines or figuring out why base-layout exists but is unused.
  • Documentation gap: The README does not explain the template authoring workflow, which will slow onboarding for future template contributors.
  • CI gap: No Woodpecker pipeline means build breakage will not be caught until someone manually runs npm run build. For a system that basketball-api depends on at runtime, this is a reliability concern.

VERDICT: NOT APPROVED

Two blockers must be addressed:

  1. DRY violation: jersey-reminder.mjml must compose with base-layout.mjml via <mj-include> rather than duplicating the entire layout. The base layout file exists for this purpose -- use it.
  2. Zero test coverage: Add at minimum a build smoke test (script or CI step) that verifies npm run build produces expected output files with expected placeholders. Adding a .woodpecker.yml pipeline would be ideal but a test script in package.json is the minimum bar.
## PR #2 Review ### DOMAIN REVIEW **Tech stack:** Node.js, MJML (email templating), vanilla HTML/JS preview system. This is a new repo (`westside-emails`) introducing an MJML-based email template compilation pipeline. The architecture is: MJML source files compile to HTML via `npm run build`, producing `dist/*.html` files with `{{variable}}` placeholders that basketball-api will consume for string substitution before sending via Gmail OAuth. **Build script (`scripts/build.js`):** - Clean, well-structured Node script. Uses `mjml` with `validationLevel: "soft"`, which is appropriate for development but means warnings do not fail the build. Files with errors are still written to disk -- the script increments an `errors` counter but never exits non-zero for compilation warnings. This is acceptable for a template repo where the human previews output, but worth noting. - Uses synchronous `fs.readFileSync`/`fs.writeFileSync` -- fine for a build tool, not a server. **Base layout (`src/base-layout.mjml`):** - Defines the shared Westside branding: dark theme (#0a0a0a / #1a1a1a / #141414), red accent (#c41230), logo from MinIO CDN, footer with org name and location. - Contains a `{{content}}` placeholder in the body section. **Jersey reminder (`src/jersey-reminder.mjml`):** - CRITICAL: This template does NOT use `<mj-include path="./base-layout.mjml" />` or any include mechanism. Instead, it duplicates the entire header (logo, org name, red accent bar), all `<mj-head>` attributes/styles, and the footer verbatim from `base-layout.mjml`. The base layout file exists but is never referenced by the actual template. - This means `base-layout.mjml` compiles to its own `dist/base-layout.html` (with a raw `{{content}}` placeholder), but `jersey-reminder.mjml` is a fully standalone file that copy-pastes the layout. When branding changes (logo URL, colors, footer text), both files must be updated independently -- exactly the divergence risk that base layouts are designed to prevent. - Template content is well-written: three clear pricing options ($90/$130/$0), appropriate alt text on images, fallback URL for non-clickable email clients. - Uses `{{name}}` and `{{jersey_url}}` placeholders correctly. **Preview system (`preview/index.html`):** - Clean vanilla JS implementation. Template registry pattern is extensible for future templates. - `renderTemplate()` uses regex replacement for `{{key}}` patterns -- correct and sufficient for simple string substitution. - Good error handling: shows red error message if HTML file not found, prompts user to run `npm run build`. - iframe `srcdoc` approach avoids CORS issues with local file serving. **Sample data (`sample-data/jersey-reminder.json`):** - Contains a Tailscale URL: `https://westsidekingsandqueens.tail5b443a.ts.net/jersey?token=sample-token-abc123`. This is sample/preview data, not a real credential, and the file is clearly labeled as sample data. Acceptable. **Package configuration:** - MJML pinned to `^4.15.3` (caret range allows minor/patch updates). `package-lock.json` is committed for reproducibility. Good. - `serve` as devDependency for preview. Clean dependency footprint. ### BLOCKERS **1. DRY violation: `base-layout.mjml` exists but is not used (NOT a security/auth path, but a significant architecture concern)** `src/base-layout.mjml` defines the shared branding layout with a `{{content}}` slot. However, `src/jersey-reminder.mjml` duplicates the entire header, mj-head attributes, styles, and footer instead of using `<mj-include>`. This means: - `base-layout.mjml` compiles to an unusable HTML file (raw `{{content}}` in output) - Future templates will either copy-paste from jersey-reminder (compounding duplication) or use the base layout (creating two divergent patterns) - Branding changes require updating every standalone template MJML supports `<mj-include path="./base-layout.mjml" />` natively. The jersey-reminder template should compose with the base layout rather than duplicating it. This is the stated architectural goal in issue #134 ("brand wrapper") and the repo's own file naming convention implies composition. **2. No test coverage for build output** The Test Plan states "Build succeeds and outputs both compiled HTML files" and "Placeholders present: `{{name}}` and `{{jersey_url}}`" -- but these are manual verification steps, not automated tests. There are no test scripts, no CI pipeline (`.woodpecker.yml` is absent), and no assertions that: - The build produces expected output files - Placeholders survive compilation - HTML is valid - Template count matches source count For a template system that basketball-api will consume programmatically, build correctness should be verified automatically. At minimum, a smoke test that runs `npm run build` and asserts output files exist with expected placeholders would prevent silent regressions. Per BLOCKER criteria: new functionality with zero test coverage. ### NITS 1. **README is minimal.** A template repo benefits from documenting: how to add a new template, the placeholder convention, how basketball-api consumes the output, and the preview workflow. The current README is one line. 2. **Build script exit code.** `scripts/build.js` exits 0 even when templates have compilation warnings. Consider `process.exit(1)` when `errors > 0` so CI (when added) can catch regressions. 3. **No `.woodpecker.yml`.** This repo lives on Forgejo but has no CI pipeline. Even a simple pipeline that runs `npm ci && npm run build` would catch broken templates on push. 4. **`base-layout.mjml` compiles to a broken HTML file.** Since it contains `{{content}}` as literal text (not a valid MJML construct), it produces an HTML file with a raw `{{content}}` string in the output. If it is meant only as an include fragment, it should either be excluded from the build glob (e.g., prefix with `_` and filter in build.js) or moved to a `partials/` directory. 5. **Hardcoded MinIO CDN URLs in templates.** The logo and jersey image URLs point to `minio-api.tail5b443a.ts.net`. If the MinIO hostname changes, every template needs updating. Consider a `{{cdn_base_url}}` placeholder or a build-time variable, though this may be acceptable given the current scale. 6. **Preview iframe height is hardcoded to 1200px.** The jersey-reminder email is long with three product cards. This works now but may clip or leave excess whitespace for shorter/longer future templates. Not blocking. ### SOP COMPLIANCE - [x] Branch named after issue: `1-feat-mjml-email-template-system-with-jer` references issue #1 - [x] PR body has Summary, Changes, Test Plan, Related sections - [x] Related section references plan slug: "References plan-wkq Phase 11" - [x] No secrets committed (sample token in sample-data is clearly labeled sample data) - [x] No unnecessary file changes (all files are part of the template system) - [ ] Tests exist and pass: No automated tests or CI pipeline ### PROCESS OBSERVATIONS - **Deployment frequency:** This is a new repo with a clean initial contribution. The template compilation approach is sound and will enable faster email iteration without API deployments -- a DORA deployment frequency win. - **Change failure risk:** The DRY violation (duplicated layout) increases change failure risk for future template additions. A developer adding a second template will face a choice between copy-pasting 200+ lines or figuring out why base-layout exists but is unused. - **Documentation gap:** The README does not explain the template authoring workflow, which will slow onboarding for future template contributors. - **CI gap:** No Woodpecker pipeline means build breakage will not be caught until someone manually runs `npm run build`. For a system that basketball-api depends on at runtime, this is a reliability concern. ### VERDICT: NOT APPROVED Two blockers must be addressed: 1. **DRY violation:** `jersey-reminder.mjml` must compose with `base-layout.mjml` via `<mj-include>` rather than duplicating the entire layout. The base layout file exists for this purpose -- use it. 2. **Zero test coverage:** Add at minimum a build smoke test (script or CI step) that verifies `npm run build` produces expected output files with expected placeholders. Adding a `.woodpecker.yml` pipeline would be ideal but a `test` script in `package.json` is the minimum bar.
Refactor jersey-reminder.mjml to use mj-include for the shared head
attributes, header, and footer instead of duplicating them from
base-layout.mjml. Extract three partials (head.mjml, header.mjml,
footer.mjml) into src/partials/ that both templates now include.

Add scripts/test.js with 8 assertions: strict MJML compilation,
dist output verification, placeholder marker checks ({{name}},
{{jersey_url}}), and DRY verification that partials exist and
mj-include directives are used. Wire up npm test in package.json.

Add .woodpecker.yaml CI pipeline (install, build, test steps).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-03-21 17:22:12 +00:00
Author
Owner
Copy link

QA Blocker Fixes

Blocker 1 -- DRY violation (resolved)

Extracted three shared partials into src/partials/:

  • head.mjml -- mj-attributes (font family, colors, line-height) and mj-style (cta-link, footer-link)
  • header.mjml -- logo image, "Westside Kings & Queens" title, red accent divider
  • footer.mjml -- org name and location

Both base-layout.mjml and jersey-reminder.mjml now use <mj-include path="./partials/..." /> instead of duplicating these blocks. The jersey reminder template dropped from 217 lines to 166, with zero inline duplication of brand elements.

Blocker 2 -- Zero test coverage (resolved)

  • scripts/test.js -- 8 assertions covering strict MJML compilation, dist output existence, placeholder markers ({{name}}, {{jersey_url}}), and DRY verification (partials exist, mj-include directives used)
  • package.json -- added "test": "node scripts/build.js && node scripts/test.js" script
  • .woodpecker.yaml -- CI pipeline with install, build, and test steps using node:20-alpine

All 8 tests pass. Build output verified: compiled HTML preserves all content, placeholders, header, and footer.

## QA Blocker Fixes ### Blocker 1 -- DRY violation (resolved) Extracted three shared partials into `src/partials/`: - `head.mjml` -- mj-attributes (font family, colors, line-height) and mj-style (cta-link, footer-link) - `header.mjml` -- logo image, "Westside Kings & Queens" title, red accent divider - `footer.mjml` -- org name and location Both `base-layout.mjml` and `jersey-reminder.mjml` now use `<mj-include path="./partials/..." />` instead of duplicating these blocks. The jersey reminder template dropped from 217 lines to 166, with zero inline duplication of brand elements. ### Blocker 2 -- Zero test coverage (resolved) - **`scripts/test.js`** -- 8 assertions covering strict MJML compilation, dist output existence, placeholder markers (`{{name}}`, `{{jersey_url}}`), and DRY verification (partials exist, mj-include directives used) - **`package.json`** -- added `"test": "node scripts/build.js && node scripts/test.js"` script - **`.woodpecker.yaml`** -- CI pipeline with install, build, and test steps using `node:20-alpine` All 8 tests pass. Build output verified: compiled HTML preserves all content, placeholders, header, and footer.
forgejo_admin commented 2026-03-21 17:27:20 +00:00
Author
Owner
Copy link

PR #2 Review

Re-review after blocker fixes. Previous review found two blockers: (1) DRY violation in jersey-reminder.mjml, (2) no tests or CI. Both addressed in commit 60d2792.

Tech stack: Node.js, MJML email templating, Woodpecker CI. Applying generic code quality + Node/CI domain checks.

DOMAIN REVIEW

MJML Architecture (13 files, +3447 lines)

The template system is well-structured. Two templates (jersey-reminder.mjml, base-layout.mjml) share three partials via mj-include:

  • src/partials/head.mjml -- shared mj-attributes (font family, colors, background) and mj-style
  • src/partials/header.mjml -- logo image from MinIO CDN, org name, red accent divider
  • src/partials/footer.mjml -- org name + location

The build pipeline (scripts/build.js) compiles all src/*.mjml to dist/*.html with soft validation. The test suite (scripts/test.js) has 8 assertions across 4 categories:

  1. Strict compilation validation (all MJML files)
  2. Build output existence (dist/*.html files)
  3. Placeholder integrity ({{name}}, {{jersey_url}} survive compilation)
  4. DRY enforcement (>= 3 partials exist, >= 3 mj-include directives in jersey-reminder)

The DRY test in category 4 is a smart guardrail -- it will catch future regressions if someone inlines partials again.

Build script review (scripts/build.js):

  • Uses validationLevel: "soft" which allows compilation despite warnings. This is appropriate for build output (produce HTML even with non-critical warnings) while tests use "strict" to catch issues. Good separation.
  • Properly exits with error if no source files found.
  • Note: files with compilation errors still produce output (line written after error logging). This is intentional for soft validation but worth documenting.

Test script review (scripts/test.js):

  • Clean pass/fail tracking with exit code 1 on any failure.
  • Test 2 depends on npm run build having been run first -- this is handled by the package.json test script: "test": "node scripts/build.js && node scripts/test.js".
  • DRY verification test hardcodes the filename jersey-reminder.mjml. When future templates are added, this test should be generalized. Acceptable for now with one template.

CI pipeline (.woodpecker.yaml):

  • Three-step pipeline: install -> build -> test on node:20-alpine.
  • Each step uses a separate image instance. The npm ci in step 1 populates node_modules/, and steps 2-3 will also have access since Woodpecker shares the workspace volume by default. Correct.
  • Note: The repo is not yet registered with Woodpecker CI (404 on pipeline listing). The .woodpecker.yaml is ready but CI activation is a separate step.

Preview system (preview/index.html):

  • Vanilla JS template renderer with iframe srcdoc. Clean implementation.
  • Template registry pattern (const templates = { ... }) makes it easy to add new templates.
  • Regex-based {{key}} replacement is simple and sufficient for the use case. No risk of injection since sample data is developer-controlled JSON files.

Email content quality:

  • Jersey images reference MinIO CDN URLs (minio-api.tail5b443a.ts.net/assets/westside/jerseys/). These are real, hosted assets.
  • Logo references minio-api.tail5b443a.ts.net/assets/westside/branding/logo.jpeg. Consistent CDN usage.
  • alt text present on all images. Good accessibility.
  • Tone is friendly, pressure-free. Appropriate for parent-facing communication.

BLOCKERS

None. Both previously identified blockers have been resolved:

  1. DRY violation -- FIXED. jersey-reminder.mjml now uses 3 mj-include directives for head.mjml, header.mjml, and footer.mjml. The base-layout.mjml also uses the same 3 includes. Branding changes propagate from one place.

  2. No tests or CI -- FIXED. 8-assertion test suite added (scripts/test.js), wired via "test": "node scripts/build.js && node scripts/test.js" in package.json. Woodpecker CI pipeline configured in .woodpecker.yaml. Test categories cover compilation, output, placeholders, and DRY enforcement.

NITS

  1. CI not yet activated. The .woodpecker.yaml is present but the repo is not registered with Woodpecker (returns 404). This should be activated before merge so the pipeline validates the branch. If activation is planned as a follow-up, track it.

  2. base-layout.mjml is unused. It defines a {{content}} slot pattern but no template inherits from it -- jersey-reminder.mjml uses the partials directly instead of extending base-layout.mjml. This file should either be used as the actual base (future templates extend it) or removed to avoid confusion. If it's a reference/scaffold for future templates, a comment at the top saying so would help.

  3. Hardcoded MinIO hostname. Image URLs use minio-api.tail5b443a.ts.net directly in MJML source. If the CDN hostname ever changes, every template needs updating. Consider documenting this as a known constraint, or using a placeholder like {{cdn_base}} that the build or send-time process replaces.

  4. head.mjml partial has mj-section background #1a1a1a but jersey-reminder.mjml sections use #141414. The partial sets <mj-section background-color="#1a1a1a" /> as default, but every section in the jersey template overrides it to #141414. The default in the partial could be updated to #141414 to match actual usage and reduce override noise.

  5. Test script hardcodes jersey-reminder.mjml filename. Test 3 (placeholder check) and Test 4 (DRY check) reference this specific file. When a second template is added, these tests should be generalized to loop over all templates with a corresponding sample-data JSON.

SOP COMPLIANCE

  • Branch named after issue (1-feat-mjml-email-template-system-with-jer -- references issue #1 in westside-emails)
  • PR body has: Summary, Changes, Test Plan, Related (Review Checklist used instead of separate sections -- acceptable)
  • Related references plan slug (plan-wkq -- Phase 11 discovered scope)
  • Related references parent issue (forgejo_admin/basketball-api#134)
  • No secrets committed (checked all files -- no API keys, tokens, or credentials)
  • No unnecessary file changes (all 13 files serve the template system)
  • Commit messages are descriptive (feat: for initial, fix: for blocker resolution)
  • .gitignore excludes dist/ and node_modules/

PROCESS OBSERVATIONS

  • Deployment frequency: This is a new repo (westside-emails) that decouples email templates from basketball-api Python code. Good architectural separation -- email copy changes no longer require API redeployment. Positive DORA impact on DF.
  • Change failure risk: Low. MJML is a compile-time system. The test suite catches compilation errors and DRY regressions before merge. The missing CI activation is the main gap -- once Woodpecker is wired, the pipeline provides automated validation.
  • Integration gap: The basketball-api side of this (template loader, replacing f-string HTML) is tracked in issue #134 and PR #135. This PR is the template repo half. The two PRs should be merged in sequence: templates first, then the API integration.

VERDICT: APPROVED

## PR #2 Review Re-review after blocker fixes. Previous review found two blockers: (1) DRY violation in jersey-reminder.mjml, (2) no tests or CI. Both addressed in commit `60d2792`. **Tech stack:** Node.js, MJML email templating, Woodpecker CI. Applying generic code quality + Node/CI domain checks. ### DOMAIN REVIEW **MJML Architecture (13 files, +3447 lines)** The template system is well-structured. Two templates (`jersey-reminder.mjml`, `base-layout.mjml`) share three partials via `mj-include`: - `src/partials/head.mjml` -- shared mj-attributes (font family, colors, background) and mj-style - `src/partials/header.mjml` -- logo image from MinIO CDN, org name, red accent divider - `src/partials/footer.mjml` -- org name + location The build pipeline (`scripts/build.js`) compiles all `src/*.mjml` to `dist/*.html` with soft validation. The test suite (`scripts/test.js`) has 8 assertions across 4 categories: 1. Strict compilation validation (all MJML files) 2. Build output existence (dist/*.html files) 3. Placeholder integrity (`{{name}}`, `{{jersey_url}}` survive compilation) 4. DRY enforcement (>= 3 partials exist, >= 3 mj-include directives in jersey-reminder) The DRY test in category 4 is a smart guardrail -- it will catch future regressions if someone inlines partials again. **Build script review (`scripts/build.js`):** - Uses `validationLevel: "soft"` which allows compilation despite warnings. This is appropriate for build output (produce HTML even with non-critical warnings) while tests use `"strict"` to catch issues. Good separation. - Properly exits with error if no source files found. - Note: files with compilation errors still produce output (line written after error logging). This is intentional for soft validation but worth documenting. **Test script review (`scripts/test.js`):** - Clean pass/fail tracking with exit code 1 on any failure. - Test 2 depends on `npm run build` having been run first -- this is handled by the `package.json` test script: `"test": "node scripts/build.js && node scripts/test.js"`. - DRY verification test hardcodes the filename `jersey-reminder.mjml`. When future templates are added, this test should be generalized. Acceptable for now with one template. **CI pipeline (`.woodpecker.yaml`):** - Three-step pipeline: install -> build -> test on `node:20-alpine`. - Each step uses a separate image instance. The `npm ci` in step 1 populates `node_modules/`, and steps 2-3 will also have access since Woodpecker shares the workspace volume by default. Correct. - Note: The repo is **not yet registered with Woodpecker CI** (404 on pipeline listing). The `.woodpecker.yaml` is ready but CI activation is a separate step. **Preview system (`preview/index.html`):** - Vanilla JS template renderer with iframe srcdoc. Clean implementation. - Template registry pattern (`const templates = { ... }`) makes it easy to add new templates. - Regex-based `{{key}}` replacement is simple and sufficient for the use case. No risk of injection since sample data is developer-controlled JSON files. **Email content quality:** - Jersey images reference MinIO CDN URLs (`minio-api.tail5b443a.ts.net/assets/westside/jerseys/`). These are real, hosted assets. - Logo references `minio-api.tail5b443a.ts.net/assets/westside/branding/logo.jpeg`. Consistent CDN usage. - `alt` text present on all images. Good accessibility. - Tone is friendly, pressure-free. Appropriate for parent-facing communication. ### BLOCKERS **None.** Both previously identified blockers have been resolved: 1. **DRY violation -- FIXED.** `jersey-reminder.mjml` now uses 3 `mj-include` directives for `head.mjml`, `header.mjml`, and `footer.mjml`. The `base-layout.mjml` also uses the same 3 includes. Branding changes propagate from one place. 2. **No tests or CI -- FIXED.** 8-assertion test suite added (`scripts/test.js`), wired via `"test": "node scripts/build.js && node scripts/test.js"` in `package.json`. Woodpecker CI pipeline configured in `.woodpecker.yaml`. Test categories cover compilation, output, placeholders, and DRY enforcement. ### NITS 1. **CI not yet activated.** The `.woodpecker.yaml` is present but the repo is not registered with Woodpecker (returns 404). This should be activated before merge so the pipeline validates the branch. If activation is planned as a follow-up, track it. 2. **`base-layout.mjml` is unused.** It defines a `{{content}}` slot pattern but no template inherits from it -- `jersey-reminder.mjml` uses the partials directly instead of extending `base-layout.mjml`. This file should either be used as the actual base (future templates extend it) or removed to avoid confusion. If it's a reference/scaffold for future templates, a comment at the top saying so would help. 3. **Hardcoded MinIO hostname.** Image URLs use `minio-api.tail5b443a.ts.net` directly in MJML source. If the CDN hostname ever changes, every template needs updating. Consider documenting this as a known constraint, or using a placeholder like `{{cdn_base}}` that the build or send-time process replaces. 4. **`head.mjml` partial has `mj-section` background `#1a1a1a` but `jersey-reminder.mjml` sections use `#141414`.** The partial sets `<mj-section background-color="#1a1a1a" />` as default, but every section in the jersey template overrides it to `#141414`. The default in the partial could be updated to `#141414` to match actual usage and reduce override noise. 5. **Test script hardcodes `jersey-reminder.mjml` filename.** Test 3 (placeholder check) and Test 4 (DRY check) reference this specific file. When a second template is added, these tests should be generalized to loop over all templates with a corresponding sample-data JSON. ### SOP COMPLIANCE - [x] Branch named after issue (`1-feat-mjml-email-template-system-with-jer` -- references issue #1 in westside-emails) - [x] PR body has: Summary, Changes, Test Plan, Related (Review Checklist used instead of separate sections -- acceptable) - [x] Related references plan slug (`plan-wkq` -- Phase 11 discovered scope) - [x] Related references parent issue (`forgejo_admin/basketball-api#134`) - [x] No secrets committed (checked all files -- no API keys, tokens, or credentials) - [x] No unnecessary file changes (all 13 files serve the template system) - [x] Commit messages are descriptive (`feat:` for initial, `fix:` for blocker resolution) - [x] `.gitignore` excludes `dist/` and `node_modules/` ### PROCESS OBSERVATIONS - **Deployment frequency:** This is a new repo (`westside-emails`) that decouples email templates from `basketball-api` Python code. Good architectural separation -- email copy changes no longer require API redeployment. Positive DORA impact on DF. - **Change failure risk:** Low. MJML is a compile-time system. The test suite catches compilation errors and DRY regressions before merge. The missing CI activation is the main gap -- once Woodpecker is wired, the pipeline provides automated validation. - **Integration gap:** The `basketball-api` side of this (template loader, replacing f-string HTML) is tracked in issue #134 and PR #135. This PR is the template repo half. The two PRs should be merged in sequence: templates first, then the API integration. ### VERDICT: APPROVED
forgejo_admin deleted branch 1-feat-mjml-email-template-system-with-jer 2026-03-21 17:31:32 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
forgejo_admin/westside-emails!2
No description provided.