Move MJML content to separate template file#955
Move MJML content to separate template file#955dthaler wants to merge 4 commits intoorcasound:mainfrom
Conversation
Co-authored-by: dthaler <6547784+dthaler@users.noreply.github.com>
Co-authored-by: dthaler <6547784+dthaler@users.noreply.github.com>
WalkthroughReplaced inline MJML email strings with external EEx MJML templates loaded at runtime via a new private read_template/1; added two MJML templates and tests validating their presence and key placeholders. Compile and rendering flow (Zappa + EEx) and public APIs unchanged. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Notifier
participant Email as Notifications.Email
participant FS as FileSystem
participant Zappa as MJML Compiler
participant EEx as EEx Engine
participant Mailer
Notifier->>Email: build_email(type, assigns)
Email->>Email: read_template(name)
Email->>FS: read templates/{name}.mjml.eex
FS-->>Email: template string
Email->>Zappa: compile!(mjml_string)
Zappa-->>Email: compiled HTML with EEx tags
Email->>EEx: eval_string(assigns incl. unsubscribe_url)
EEx-->>Email: final HTML
Email->>Mailer: deliver(email, HTML)
Mailer-->>Notifier: result
note over Email: unsubscribe block rendered only if token present
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (10)
server/lib/orcasite/notifications/templates/confirmed_candidate.mjml.eex (3)
20-21: Add descriptive alt text for accessibility.Provide alt text for the “don’t miss” image.
- <mj-image src="https://orcasite.s3.us-west-2.amazonaws.com/email_assets/orcasound_dont_miss.jpg"></mj-image> + <mj-image src="https://orcasite.s3.us-west-2.amazonaws.com/email_assets/orcasound_dont_miss.jpg" alt="Don't miss the live orca audio stream"></mj-image>
28-30: Remove stray “ecm” query flag.
?ecm&looks accidental and could pollute analytics.- If you miss the concert, <br /> watch the <a href="https://orcasound.net/blog?ecm&utm_source=email&utm_medium=email&utm_campaign=notifications" style="color: #007C89;">Orcasound blog</a> for recordings & bioacoustic analysis! + If you miss the concert, <br /> watch the <a href="https://orcasound.net/blog?utm_source=email&utm_medium=email&utm_campaign=notifications" style="color: #007C89;">Orcasound blog</a> for recordings & bioacoustic analysis!
45-47: Stale copyright year; make it dynamic.Keeps templates future‑proof.
EEx runs in your pipeline; confirm mixing EEx with MJML/Handlebars is OK in prod.
- Copyright © 2023 Orcasound, All rights reserved. <br /> + Copyright © <%= Date.utc_today().year %> Orcasound, All rights reserved. <br />server/lib/orcasite/notifications/email.ex (1)
73-79: Handle MJML compilation errors explicitly.
elem(1)discards error status. Surface failures to avoid silently sending broken HTML.def compile_mjml(mjml, assigns) do mjml |> Zappa.compile!() |> EEx.eval_string(Map.to_list(assigns)) - |> Mjml.to_html() - |> elem(1) + |> Mjml.to_html() + |> case do + {:ok, html} -> html + {:error, reason} -> raise "MJML render failed: #{inspect(reason)}" + end endserver/test/orcasite/notifications/email_template_test.exs (3)
6-7: Make test paths independent of CWD.Resolve from test file dir to avoid failures when run outside app root.
- template_path = Path.expand("lib/orcasite/notifications/templates/new_detection.mjml.eex") + template_path = Path.expand("../lib/orcasite/notifications/templates/new_detection.mjml.eex", __DIR__)
16-17: Same here for confirmed_candidate path.- template_path = Path.expand("lib/orcasite/notifications/templates/confirmed_candidate.mjml.eex") + template_path = Path.expand("../lib/orcasite/notifications/templates/confirmed_candidate.mjml.eex", __DIR__)
25-38: Strengthen assertions to catch regressions and link issues.
- Ensure per‑row “Review” link uses
notif_meta["candidate_id"].- Guard against accidental
http://links in templates.test "templates contain required variables" do # Test new_detection template variables - {:ok, new_detection_content} = File.read(Path.expand("lib/orcasite/notifications/templates/new_detection.mjml.eex")) + {:ok, new_detection_content} = File.read(Path.expand("../lib/orcasite/notifications/templates/new_detection.mjml.eex", __DIR__)) assert String.contains?(new_detection_content, "{{ node_name }}"), "should contain node_name variable" assert String.contains?(new_detection_content, "{{ node }}"), "should contain node variable" assert String.contains?(new_detection_content, "meta[\"description\"]"), "should contain description variable" assert String.contains?(new_detection_content, "{{unsubscribe_url}}"), "should contain unsubscribe_url variable" + assert String.contains?(new_detection_content, "reports/{{notif_meta[\"candidate_id\"]}}"), "review link should use notif_meta candidate_id" # Test confirmed_candidate template variables - {:ok, confirmed_candidate_content} = File.read(Path.expand("lib/orcasite/notifications/templates/confirmed_candidate.mjml.eex")) + {:ok, confirmed_candidate_content} = File.read(Path.expand("../lib/orcasite/notifications/templates/confirmed_candidate.mjml.eex", __DIR__)) assert String.contains?(confirmed_candidate_content, "{{ node }}"), "should contain node variable" assert String.contains?(confirmed_candidate_content, "meta[\"message\"]"), "should contain message variable" assert String.contains?(confirmed_candidate_content, "{{ unsubscribe_url }}"), "should contain unsubscribe_url variable" + refute confirmed_candidate_content =~ ~r{\shref="http://}i, "all links should be https" endOptionally add a render smoke test:
test "templates render without errors" do assigns = %{ node: "haro-strait", node_name: "Haro Strait", unsubscribe_token: "t", notifications_since_count: 0, meta: %{} } assert is_binary(Orcasite.Notifications.Email.mjml_new_detection_body(assigns)) assert is_binary(Orcasite.Notifications.Email.mjml_confirmed_candidate_body(assigns)) endserver/lib/orcasite/notifications/templates/new_detection.mjml.eex (3)
23-27: Avoid non‑portable#ifboolean expressions.
node && meta["start_time"] && meta["category"]is not standard Handlebars. Prefer nested#iffor compatibility.- {{#if node && meta["start_time"] && meta["category"] }} + {{#if node}} + {{#if meta["start_time"]}} + {{#if meta["category"]}} <mj-text font-size="20px" font-family="helvetica"> <a href="https://live.orcasound.net/bouts/new/{{ node }}?time={{ meta["start_time"] }}&category={{ meta["category"] }}&utm_source=email&utm_medium=email&utm_campaign=notifications">Start a new bout</a> </mj-text> - {{/if}} + {{/if}}{{/if}}{{/if}}
29-33: Likely non‑portable comparison in#if.
> 0may not be supported by your Handlebars engine. Consider relying on truthiness of the count or a precomputed boolean.- {{#if notifications_since_count > 0}} + {{#if notifications_since_count}} <mj-text font-size="20px" font-family="helvetica"> There have been {{ notifications_since_count }} other detections since the last notification. </mj-text>
56-58: Minor: normalize placeholder style.Use consistent spacing in
{{ ... }}for readability.- Listen here: <a href="https://live.orcasound.net/listen/{{node}}?utm_source=email&utm_medium=email&utm_campaign=notifications">https://live.orcasound.net/listen/{{ node }}</a> + Listen here: <a href="https://live.orcasound.net/listen/{{ node }}?utm_source=email&utm_medium=email&utm_campaign=notifications">https://live.orcasound.net/listen/{{ node }}</a>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
server/lib/orcasite/notifications/email.ex(3 hunks)server/lib/orcasite/notifications/templates/confirmed_candidate.mjml.eex(1 hunks)server/lib/orcasite/notifications/templates/new_detection.mjml.eex(1 hunks)server/test/orcasite/notifications/email_template_test.exs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
🔇 Additional comments (1)
server/lib/orcasite/notifications/email.ex (1)
31-33: Confirm Handlebars features are supported by your renderer.Templates use
#ifwith expressions andeach as |x|. Ensure Zappa/MJML pipeline (or downstream provider) evaluates these; otherwise, emails may ship with raw{{ }}.Would you like a quick ExUnit that renders both templates via
compile_mjml/2with minimal assigns to catch runtime issues?Also applies to: 43-45
| defp read_template(name) do | ||
| Path.expand("lib/orcasite/notifications/templates/#{name}.mjml.eex") | ||
| |> File.read!() | ||
| end |
There was a problem hiding this comment.
Make template path robust to CWD/release layout.
Hardcoding "lib/..." depends on process CWD. Resolve relative to this module instead.
- defp read_template(name) do
- Path.expand("lib/orcasite/notifications/templates/#{name}.mjml.eex")
- |> File.read!()
- end
+ defp read_template(name) do
+ Path.expand("templates/#{name}.mjml.eex", __DIR__)
+ |> File.read!()
+ end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| defp read_template(name) do | |
| Path.expand("lib/orcasite/notifications/templates/#{name}.mjml.eex") | |
| |> File.read!() | |
| end | |
| defp read_template(name) do | |
| Path.expand("templates/#{name}.mjml.eex", __DIR__) | |
| |> File.read!() | |
| end |
🤖 Prompt for AI Agents
In server/lib/orcasite/notifications/email.ex around lines 68–71, the template
path is hardcoded to "lib/..." which depends on the process CWD; change it to
build the path relative to this module file (use __DIR__ or similar) and join
into the templates folder so it works in releases/other CWDs—i.e. construct the
full path from the current file directory plus "templates/#{name}.mjml.eex"
(using Path.join/1 or Path.expand with __DIR__), then read that file with
File.read!().
server/lib/orcasite/notifications/templates/confirmed_candidate.mjml.eex
Outdated
Show resolved
Hide resolved
| {{#if notif_meta["candidate_id"] }} | ||
| <a href="https://live.orcasound.net/reports/{{meta["candidate_id"]}}?utm_source=email&utm_medium=email&utm_campaign=notifications">Review</a> | ||
| {{/if}} |
There was a problem hiding this comment.
Wrong variable in Review link inside the loop.
Links will point to the outer meta id, not the per‑row notif_meta id.
- <a href="https://live.orcasound.net/reports/{{meta["candidate_id"]}}?utm_source=email&utm_medium=email&utm_campaign=notifications">Review</a>
+ <a href="https://live.orcasound.net/reports/{{notif_meta["candidate_id"]}}?utm_source=email&utm_medium=email&utm_campaign=notifications">Review</a>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {{#if notif_meta["candidate_id"] }} | |
| <a href="https://live.orcasound.net/reports/{{meta["candidate_id"]}}?utm_source=email&utm_medium=email&utm_campaign=notifications">Review</a> | |
| {{/if}} | |
| {{#if notif_meta["candidate_id"] }} | |
| <a href="https://live.orcasound.net/reports/{{notif_meta["candidate_id"]}}?utm_source=email&utm_medium=email&utm_campaign=notifications">Review</a> | |
| {{/if}} |
🤖 Prompt for AI Agents
In server/lib/orcasite/notifications/templates/new_detection.mjml.eex around
lines 46 to 48, the Review link uses the outer meta["candidate_id"] instead of
the per-row notif_meta["candidate_id"]; change the URL interpolation to use
notif_meta["candidate_id"] (so the href becomes
.../reports/{{notif_meta["candidate_id"]}}?...), ensuring it matches the
existing {{#if notif_meta["candidate_id"] }} guard and preserves the utm query
params and quoting style.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…e.mjml.eex Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR refactors email notification templates by extracting hardcoded MJML content from Elixir functions into separate template files, making email content more accessible to non-developers while maintaining existing functionality.
- Extracted MJML email templates from Elixir code into dedicated
.mjml.eexfiles - Added template loading infrastructure with
read_template/1helper function - Implemented comprehensive test coverage for template existence and content validation
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| server/lib/orcasite/notifications/email.ex | Replaced hardcoded MJML strings with calls to read_template() function |
| server/lib/orcasite/notifications/templates/new_detection.mjml.eex | New template file containing MJML for new detection notifications |
| server/lib/orcasite/notifications/templates/confirmed_candidate.mjml.eex | New template file containing MJML for confirmed candidate notifications |
| server/test/orcasite/notifications/email_template_test.exs | Added test suite to verify template files exist and contain required content |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| <td style="padding: 0 0 0 15px;">{{ notif_meta["description"] }}</td> | ||
| <td style="padding: 0 0 0 15px;"> | ||
| {{#if notif_meta["candidate_id"] }} | ||
| <a href="https://live.orcasound.net/reports/{{meta["candidate_id"]}}?utm_source=email&utm_medium=email&utm_campaign=notifications">Review</a> |
There was a problem hiding this comment.
The variable reference is incorrect. It should use notif_meta["candidate_id"] instead of meta["candidate_id"] since this is inside the {{#each notifications_since as |notif_meta|}} loop.
| <a href="https://live.orcasound.net/reports/{{meta["candidate_id"]}}?utm_source=email&utm_medium=email&utm_campaign=notifications">Review</a> | |
| <a href="https://live.orcasound.net/reports/{{notif_meta["candidate_id"]}}?utm_source=email&utm_medium=email&utm_campaign=notifications">Review</a> |
server/lib/orcasite/notifications/templates/confirmed_candidate.mjml.eex
Show resolved
Hide resolved
|
Good idea and I don't have any major issues with this PR (though there are some AI comments that might be worth addressing), but I think we should take this opportunity to try out |
This PR addresses the issue where email bodies were hardcoded in
server/lib/orcasite/notifications/email.ex, making it difficult for non-Elixir-developers to collaborate on email content.Changes Made
Extracted hardcoded MJML content from the
mjml_new_detection_body/1andmjml_confirmed_candidate_body/1functions into separate template files:lib/orcasite/notifications/templates/new_detection.mjml.eex(2,902 characters)lib/orcasite/notifications/templates/confirmed_candidate.mjml.eex(2,955 characters)Implemented template loading infrastructure:
read_template/1helper function that usesPath.expand/1to read template files from the lib directoryread_template/1instead of heredoc strings{{ variable }}format)Added comprehensive test coverage in
test/orcasite/notifications/email_template_test.exsthat verifies:Benefits
Summary by CodeRabbit
New Features
Refactor
Tests