Update the notifications config article #7557 #2026
Conversation
Summary by CodeRabbit
WalkthroughRevises Studio documentation to rename notifications configuration references from notifications.xml to notification-config.xml, restructures and expands notification templates and examples, and wraps added explanatory content in HTML details/summary blocks. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
source/reference/modules/studio.rst (2)
2961-2973: Fix path in caption (same inconsistency as above)Use workflow/notification-config.xml to match the default property and actual location.
- :caption: *CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/notification-config.xml* + :caption: *CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/workflow/notification-config.xml*
2984-3019: Fix path in caption (same inconsistency as above)Align caption path with workflow/notification-config.xml.
- :caption: *CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/notification-config.xml* + :caption: *CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/workflow/notification-config.xml*
🧹 Nitpick comments (3)
source/reference/modules/studio.rst (3)
3077-3080: Verify variable naming/source consistency (reviewer/submitter fields)This template mixes snake_case (first_name, last_name) and camelCase (elsewhere you use approver.firstName). If the backing model uses consistent keys, this will break.
Please confirm the actual variables passed to FTL in Studio 5.x and standardize (all camelCase or whatever Studio emits). Based on learnings
3097-3108: Good: schedule/target/reviewer comment surfaced; add variables to docsYou reference publishPackage.* fields not listed in “Template Variables.” Document these (packageType, schedule, target, reviewerComment, submitterComment, etc.) and new file.* fields (page, browserUri, action, path, packageId, liveError, stagingError).
3178-3216: Template variable consistency (reviewer. fields) and links look fine otherwise*After fixing “&&”, please also verify reviewer.first_name/last_name names as above.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
source/reference/modules/studio.rst(12 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T20:40:32.613Z
Learnt from: alhambrav
PR: craftercms/docs#2042
File: source/reference/modules/studio.rst:0-0
Timestamp: 2025-09-02T20:40:32.613Z
Learning: When reviewing CrafterCMS Studio documentation, property names in configuration examples should match the actual Studio source code implementation, even if the underlying Git config keys are different. Studio may have a translation layer that maps Studio property names to actual Git configuration keys.
Applied to files:
source/reference/modules/studio.rst
🔇 Additional comments (8)
source/reference/modules/studio.rst (8)
2-2: Meta update looks goodUpdated last-updated to 5.0.0 is consistent with the new content.
100-102: Good addition to the config properties indexNew “Notification Service Configuration” entry improves discoverability.
105-105: Description reads wellWording aligns with the section content below.
3023-3030: Nice use of collapsible detailsThe expandable block improves readability.
3051-3052: LGTM: clearer itemization of deployment failuresThe extra details per file are helpful.
3067-3067: Subject logic: good conditional for scheduled vs approvedLooks correct.
3086-3095: Inline link around pages is correctConditional anchor wrapping by file.page is solid.
3267-3314: Great addition: Notification Service Configuration defaultsSection is clear and useful. Ensure the configuration file path here matches all captions above (workflow/notification-config.xml) to avoid user confusion.
| :caption: *CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/notification-config.xml* | ||
There was a problem hiding this comment.
Fix path: notification config location inconsistent with defaults
Captions point to config/studio/notification-config.xml but the default property below uses workflow/notification-config.xml. Align to workflow/… to prevent confusion.
Apply this diff:
- :caption: *CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/notification-config.xml*
+ :caption: *CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/workflow/notification-config.xml*Also update the sentence above that references “Notification Configuration” if it implies the old path. Based on learnings
📝 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.
| :caption: *CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/notification-config.xml* | |
| :caption: *CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/workflow/notification-config.xml* |
🤖 Prompt for AI Agents
In source/reference/modules/studio.rst around lines 2883-2884, the caption path
currently points to config/studio/notification-config.xml while the default
property and intended location use workflow/notification-config.xml; update the
caption line to use
*CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/workflow/notification-config.xml*
and also edit the nearby sentence above that references “Notification
Configuration” so its example/path text reflects the
workflow/notification-config.xml location to keep both descriptions consistent.
| :caption: *CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/notification-config.xml* | ||
| :linenos: | ||
There was a problem hiding this comment.
Fix path in caption (same inconsistency as above)
Update to workflow/notification-config.xml.
- :caption: *CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/notification-config.xml*
+ :caption: *CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/workflow/notification-config.xml*📝 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.
| :caption: *CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/notification-config.xml* | |
| :linenos: | |
| :caption: *CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/workflow/notification-config.xml* | |
| :linenos: |
🤖 Prompt for AI Agents
In source/reference/modules/studio.rst around lines 3032 to 3034 the :caption:
path is incorrect (it currently points to
*CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/notification-config.xml*);
update the caption to the correct path
*CRAFTER_HOME/data/repos/sites/SITENAME/sandbox/config/studio/workflow/notification-config.xml*
so it matches the intended location.
| <#if deploymentError?has_content> | ||
| Error:<br/> | ||
| <pre>${deploymentError}</pre> | ||
| </#if> |
There was a problem hiding this comment.
Prefer explicit string conversion for deploymentError
Earlier docs say deploymentError must be addressed via toString(); template uses ${deploymentError}. Make it explicit to avoid renderer surprises.
- <pre>${deploymentError}</pre>
+ <pre>${deploymentError?string}</pre>Alternatively: ${deploymentError.toString()}.
📝 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 deploymentError?has_content> | |
| Error:<br/> | |
| <pre>${deploymentError}</pre> | |
| </#if> | |
| <#if deploymentError?has_content> | |
| Error:<br/> | |
| <pre>${deploymentError?string}</pre> | |
| </#if> |
🤖 Prompt for AI Agents
In source/reference/modules/studio.rst around lines 3055 to 3058, the template
directly interpolates ${deploymentError} but earlier docs require using
toString(); update the template to perform an explicit string conversion (e.g.,
call deploymentError.toString() or otherwise coerce to a string) before
rendering so the renderer receives a definite string and avoids surprises;
ensure you guard for null/undefined (call toString() only when deploymentError
is present) consistent with the surrounding has_content check.
| <#else> | ||
| ${submitter.first_name!submitter.username} ${submitter.last_name} has submitted items for your review: | ||
| </#if> | ||
| <#if files?? && files?size gt 0> |
There was a problem hiding this comment.
Fix Freemarker operator inside CDATA: use &&, not &&
Inside you must use raw “&&”. The escaped form won’t be parsed by FTL.
- <#if files?? && files?size gt 0>
+ <#if files?? && files?size gt 0>📝 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 files?? && files?size gt 0> | |
| <#if files?? && files?size gt 0> |
🤖 Prompt for AI Agents
In source/reference/modules/studio.rst around line 3131 the Freemarker
boolean-and operator is incorrectly escaped as "&&" inside a CDATA
block; replace the escaped sequence with the raw operator "&&" so Freemarker
will parse the condition correctly (i.e., change "&&" to "&&" within the
<![CDATA[ ]]> context).
| <#else> | ||
| ${reviewer.first_name!submitter.username} ${reviewer.last_name} has reviewed the following content that requires some revision before it can be approved: | ||
| </#if> | ||
| <#if files?? && files?size gt 0> |
There was a problem hiding this comment.
Fix Freemarker operator inside CDATA here too
Same issue as above; switch to “&&”.
- <#if publishPackage.schedule??>
+ <#if publishPackage.schedule??>Note: And change:
- <#if files?? && files?size gt 0>
+ <#if files?? && files?size gt 0>Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In source/reference/modules/studio.rst around line 3185, the Freemarker logical
AND operator is HTML-escaped as "&&" inside the CDATA; replace the
escaped sequence with the literal "&&" so the conditional reads "<#if files?? &&
files?size gt 0>" (ensure it remains inside CDATA unescaped).
2 participants
Ticket reference or full description of what's in the PR
Update the notifications config article craftercms/craftercms#7557