Skip to content

Add consistent email footer across all templates#891

Open
krusche wants to merge 5 commits into
developfrom
feature/consistent-email-footer
Open

Add consistent email footer across all templates#891
krusche wants to merge 5 commits into
developfrom
feature/consistent-email-footer

Conversation

@krusche

@krusche krusche commented Mar 3, 2026

Copy link
Copy Markdown
Member

Summary

  • Every email now gets a unified footer: a configurable sign-off (defaulting to "Regards, The Thesis Management Team.") followed by a notification settings link, both appended centrally by MailBuilder.send()
  • Add emailSignature field to ResearchGroupSettings so each research group can customize their sign-off
  • Replace all hardcoded "Best regards, The Thesis Coordination Team" sign-offs and broken ${config.signature} references with the ${emailSignature} Thymeleaf variable
  • Remove per-template notification link blocks — the link is now appended centrally after rendering
  • Add Liquibase migration 40 to update the schema and existing template data in deployed databases

Test plan

  • All 711 server tests pass (./gradlew test)
  • All 134 e2e tests pass (./execute-e2e-local.sh), including new assertEmailFooter assertions in 6 spec files
  • Verify via Mailpit UI that emails show consistent footer with notification settings link
  • Verify custom emailSignature from research group settings overrides the default

🤖 Generated with Claude Code

Summary by CodeRabbit

Release Notes

  • New Features
    • Research groups can now configure custom email signatures that are automatically appended to all outgoing notifications for a personalized touch
    • Email footers containing notification settings links are now consistently applied across all automated system emails
    • Improved email presentation with unified footer formatting and signature support throughout the application

Review Change Stack

Every email now gets a unified footer: a configurable sign-off
(defaulting to "Regards, The Thesis Management Team.") followed by a
notification settings link appended centrally by MailBuilder.send().

- Add emailSignature field to ResearchGroupSettings for per-group
  customization
- Set default emailSignature template variable in MailBuilder constructor
- Append notification settings link centrally in MailBuilder.send()
  after Thymeleaf rendering
- Replace hardcoded "Best regards" sign-offs and broken
  ${config.signature} references with ${emailSignature} in all templates
- Remove per-template notification link blocks (now handled centrally)
- Add Liquibase migration 36 for schema + existing template data updates
- Add assertEmailFooter e2e helper and footer assertions in 6 test specs

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Mar 3, 2026

Copy link
Copy Markdown

Walkthrough

E2E tests across email workflows now validate footer presence via a new assertEmailFooter utility. The client interface adds emailSignature field to research group settings, enabling custom email signatures in outgoing messages.

Changes

Email footer testing and signature infrastructure

Layer / File(s) Summary
Email footer assertion utility
client/e2e/mailpit.ts
New exported assertEmailFooter() helper validates emails contain the notification settings footer and /settings/notifications URL.
E2E footer validation across workflows
client/e2e/application-review-workflow.spec.ts, client/e2e/application-workflow.spec.ts, client/e2e/presentation-workflow.spec.ts, client/e2e/proposal-feedback-workflow.spec.ts, client/e2e/thesis-grading-workflow.spec.ts, client/e2e/thesis-workflow.spec.ts
Six test workflows import assertEmailFooter and apply footer validation to application rejection/acceptance, thesis creation, presentation scheduling, proposal feedback, final grades, and thesis completion emails. Footer assertions run immediately after verifying emails were sent from the application.
Research group email signature settings contract
client/src/requests/responses/researchGroupSettings.ts
IResearchGroupSettingsEmail interface adds optional `emailSignature?: string

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and accurately summarizes the main change: adding a consistent email footer across all email templates in the system.
Docstring Coverage ✅ Passed Docstring coverage is 83.87% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/consistent-email-footer

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
server/src/main/resources/db/changelog/changes/28_data_export.sql (1)

14-32: ⚠️ Potential issue | 🟠 Major

Modification of existing changeset; also ON CONFLICT prevents updates.

Two concerns:

  1. Modifying changeset 28 after it's been applied will cause Liquibase checksum failures
  2. The ON CONFLICT DO NOTHING clause means existing DATA_EXPORT_READY templates won't receive the emailSignature placeholder update anyway

Ensure changeset 36 handles the UPDATE for existing rows. The modification here only affects fresh deployments.

As per coding guidelines: "Never modify existing changesets that have been applied; create new ones."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server/src/main/resources/db/changelog/changes/28_data_export.sql` around
lines 14 - 32, Do not modify changeset 28; revert that file to its original
INSERT-only content and create a new changeset (e.g., changeset 36) that
performs an UPDATE on email_templates for template_case = 'DATA_EXPORT_READY'
and language = 'en' (and research_group_id IS NULL) to append or inject the <div
th:utext="${emailSignature}"></div> into body_html so existing rows get the
placeholder; ensure the new changeset uses an UPDATE statement (not INSERT ...
ON CONFLICT DO NOTHING) targeting the email_templates rows and include a clear
id/author so Liquibase checksum history remains intact.
server/src/main/resources/db/changelog/changes/19_add_interview_invitation_email_template.sql (1)

1-135: ⚠️ Potential issue | 🟠 Major

Modification of existing changeset will cause checksum failures.

Same issue as changeset 14 — this changeset (19) has likely been applied to deployed databases. Modifying the template bodies at lines 26, 55, 103, and 133 will break Liquibase checksum validation.

The migration should be handled by a new changeset (e.g., changeset 36) that UPDATEs existing rows rather than modifying historical INSERTs.

As per coding guidelines: "Never modify existing changesets that have been applied; create new ones."

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@server/src/main/resources/db/changelog/changes/19_add_interview_invitation_email_template.sql`
around lines 1 - 135, You changed an already-applied Liquibase changeset
(changeset 19) which will break checksums; instead create a new changeset (e.g.,
changeset 36) that issues UPDATE statements to modify the
body_html/subject/description for the existing template rows. Locate the
templates by their template_case values ('INTERVIEW_INVITATION',
'INTERVIEW_INVITATION_REMINDER', 'INTERVIEW_SLOT_BOOKED_CONFORMATION',
'INTERVIEW_SLOT_BOOKED_CANCELLATION') and write UPDATE ... SET body_html =
'...', subject = '...', description = '...' WHERE template_case = '...'; add
this new SQL changeset file and do not alter the original INSERTs in changeset
19.
🧹 Nitpick comments (3)
client/e2e/application-workflow.spec.ts (1)

146-161: Consider asserting sender/footer for the supervisor notification too.

For consistency with the other recipient checks in this test, add assertSentFromApp and assertEmailFooter for supervisorChairEmail.

♻️ Suggested change
     const supervisorChairEmail = findBySubject(supervisorEmails, 'New Thesis Application')
     expect(
       supervisorChairEmail,
       'Supervisor (supervisor@test.local) should receive "New Thesis Application" email',
     ).toBeDefined()
+    assertSentFromApp(supervisorChairEmail!)
+    assertEmailFooter(supervisorChairEmail!)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@client/e2e/application-workflow.spec.ts` around lines 146 - 161, Add
assertions to verify the supervisor notification's sender and footer like you do
for the examiner: after locating supervisorChairEmail (from waitForNewMessages
and findBySubject) call assertSentFromApp(supervisorChairEmail) and
assertEmailFooter(supervisorChairEmail). This will mirror the existing checks on
examinerChairEmail and ensure the supervisor email's sender and footer are
validated as well.
server/src/main/java/de/tum/cit/aet/thesis/service/MailingService.java (1)

64-75: @Autowired is redundant on single-constructor classes.

Spring Boot auto-wires the sole constructor without the annotation.

♻️ Optional: Remove redundant annotation
-	`@Autowired`
 	public MailingService(
 			JavaMailSender javaMailSender,
 			UploadService uploadService,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server/src/main/java/de/tum/cit/aet/thesis/service/MailingService.java`
around lines 64 - 75, The `@Autowired` annotation on the MailingService
constructor is redundant because Spring will auto-wire the single constructor;
remove the `@Autowired` annotation from the MailingService constructor (the
constructor that takes JavaMailSender, UploadService, MailConfig,
EmailTemplateRepository) so the class relies on constructor injection without
the unnecessary annotation.
server/src/main/java/de/tum/cit/aet/thesis/utility/MailBuilder.java (1)

590-595: Consider extracting the hardcoded footer HTML.

The notification settings footer is inlined here, making future changes error-prone if the same link/text is referenced elsewhere.

♻️ Extract to a constant
+	private static final String NOTIFICATION_FOOTER_TEMPLATE =
+			"<hr/><div style=\"text-align: center; font-size: 10px;\">"
+			+ "You can (un)subscribe to similar emails in your "
+			+ "<a href=\"%s/settings/notifications\">notification settings in Thesis Management</a>.</div>";
+
 	public void send(JavaMailSender mailSender, UploadService uploadService) {
 		// ... existing code ...
 
 				String renderedHtml = config.getTemplateEngine().process(templateHtml, templateContext);
-				renderedHtml += "<hr/><div style=\"text-align: center; font-size: 10px;\">"
-						+ "You can (un)subscribe to similar emails in your "
-						+ "<a href=\"" + config.getClientHost() + "/settings/notifications\">"
-						+ "notification settings in Thesis Management</a>.</div>";
+				renderedHtml += String.format(NOTIFICATION_FOOTER_TEMPLATE, config.getClientHost());
 				messageBody.setContent(renderedHtml, "text/html; charset=utf-8");
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@server/src/main/java/de/tum/cit/aet/thesis/utility/MailBuilder.java` around
lines 590 - 595, The hardcoded notification-settings footer is inlined when
building renderedHtml in MailBuilder (where templateHtml, renderedHtml and
messageBody are used); extract that HTML snippet into a single reusable private
static final String (e.g. NOTIFICATION_SETTINGS_FOOTER) in the MailBuilder class
and replace the inline concatenation ("<hr/><div...<a href=\"" +
config.getClientHost() + "/settings/notifications\">...") with appending that
constant (compose the href using config.getClientHost() if it must be dynamic or
store a format string constant and String.format it), keeping the same
"text/html; charset=utf-8" content type on messageBody.setContent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In
`@server/src/main/java/de/tum/cit/aet/thesis/controller/ResearchGroupSettingsController.java`:
- Around line 83-84: Sanitize the email signature before saving: in
ResearchGroupSettingsController where you set toSave.setEmailSignature(...),
trim and null-check the incoming newSettings.emailSettings().emailSignature(),
then call HtmlUtils.htmlEscape(...) on the trimmed value and persist that
escaped string (or null if empty) so stored signatures cannot inject raw HTML
into outbound templates; follow the same pattern used for
applicantName/thesisTitle in MailingService.

In
`@server/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsEmailDTO.java`:
- Around line 5-11: Add the required JSON include annotation to the DTO by
annotating the ResearchGroupSettingsEmailDTO record with
`@JsonInclude`(JsonInclude.Include.NON_EMPTY) (importing
com.fasterxml.jackson.annotation.JsonInclude) so that null/empty fields like
applicationNotificationEmail and emailSignature are omitted during
serialization; ensure the annotation is placed on the record declaration (above
ResearchGroupSettingsEmailDTO) and verify imports are updated, leaving the
fromEntity(...) factory method unchanged.

In
`@server/src/main/resources/db/changelog/changes/36_consistent_email_footer.sql`:
- Around line 40-44: The changeset
consistent-email-footer:36-add-signature-to-templates-without is too broad
because it updates every row in email_templates whose body_html lacks
emailSignature or "Best regards"; narrow the scope by adding explicit
template_case filtering so only templates that should have a signature are
modified (e.g., add AND template_case IN (...) or AND template_case NOT IN (...)
to the WHERE clause), referencing the email_templates table and body_html column
to locate the UPDATE and adjust the WHERE to target intended template types
rather than relying solely on absence detection.

---

Outside diff comments:
In
`@server/src/main/resources/db/changelog/changes/19_add_interview_invitation_email_template.sql`:
- Around line 1-135: You changed an already-applied Liquibase changeset
(changeset 19) which will break checksums; instead create a new changeset (e.g.,
changeset 36) that issues UPDATE statements to modify the
body_html/subject/description for the existing template rows. Locate the
templates by their template_case values ('INTERVIEW_INVITATION',
'INTERVIEW_INVITATION_REMINDER', 'INTERVIEW_SLOT_BOOKED_CONFORMATION',
'INTERVIEW_SLOT_BOOKED_CANCELLATION') and write UPDATE ... SET body_html =
'...', subject = '...', description = '...' WHERE template_case = '...'; add
this new SQL changeset file and do not alter the original INSERTs in changeset
19.

In `@server/src/main/resources/db/changelog/changes/28_data_export.sql`:
- Around line 14-32: Do not modify changeset 28; revert that file to its
original INSERT-only content and create a new changeset (e.g., changeset 36)
that performs an UPDATE on email_templates for template_case =
'DATA_EXPORT_READY' and language = 'en' (and research_group_id IS NULL) to
append or inject the <div th:utext="${emailSignature}"></div> into body_html so
existing rows get the placeholder; ensure the new changeset uses an UPDATE
statement (not INSERT ... ON CONFLICT DO NOTHING) targeting the email_templates
rows and include a clear id/author so Liquibase checksum history remains intact.

---

Nitpick comments:
In `@client/e2e/application-workflow.spec.ts`:
- Around line 146-161: Add assertions to verify the supervisor notification's
sender and footer like you do for the examiner: after locating
supervisorChairEmail (from waitForNewMessages and findBySubject) call
assertSentFromApp(supervisorChairEmail) and
assertEmailFooter(supervisorChairEmail). This will mirror the existing checks on
examinerChairEmail and ensure the supervisor email's sender and footer are
validated as well.

In `@server/src/main/java/de/tum/cit/aet/thesis/service/MailingService.java`:
- Around line 64-75: The `@Autowired` annotation on the MailingService constructor
is redundant because Spring will auto-wire the single constructor; remove the
`@Autowired` annotation from the MailingService constructor (the constructor that
takes JavaMailSender, UploadService, MailConfig, EmailTemplateRepository) so the
class relies on constructor injection without the unnecessary annotation.

In `@server/src/main/java/de/tum/cit/aet/thesis/utility/MailBuilder.java`:
- Around line 590-595: The hardcoded notification-settings footer is inlined
when building renderedHtml in MailBuilder (where templateHtml, renderedHtml and
messageBody are used); extract that HTML snippet into a single reusable private
static final String (e.g. NOTIFICATION_SETTINGS_FOOTER) in the MailBuilder class
and replace the inline concatenation ("<hr/><div...<a href=\"" +
config.getClientHost() + "/settings/notifications\">...") with appending that
constant (compose the href using config.getClientHost() if it must be dynamic or
store a format string constant and String.format it), keeping the same
"text/html; charset=utf-8" content type on messageBody.setContent.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 242ab64 and 9a9353a.

📒 Files selected for processing (20)
  • client/e2e/application-review-workflow.spec.ts
  • client/e2e/application-workflow.spec.ts
  • client/e2e/mailpit.ts
  • client/e2e/presentation-workflow.spec.ts
  • client/e2e/proposal-feedback-workflow.spec.ts
  • client/e2e/thesis-grading-workflow.spec.ts
  • client/e2e/thesis-workflow.spec.ts
  • client/src/requests/responses/researchGroupSettings.ts
  • server/src/main/java/de/tum/cit/aet/thesis/controller/ResearchGroupSettingsController.java
  • server/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsEmailDTO.java
  • server/src/main/java/de/tum/cit/aet/thesis/entity/ResearchGroupSettings.java
  • server/src/main/java/de/tum/cit/aet/thesis/service/MailingService.java
  • server/src/main/java/de/tum/cit/aet/thesis/utility/MailBuilder.java
  • server/src/main/resources/db/changelog/changes/14_add_automatic_rejection_reminder_email_template.sql
  • server/src/main/resources/db/changelog/changes/19_add_interview_invitation_email_template.sql
  • server/src/main/resources/db/changelog/changes/28_data_export.sql
  • server/src/main/resources/db/changelog/changes/36_consistent_email_footer.sql
  • server/src/main/resources/db/changelog/db.changelog-master.xml
  • server/src/main/resources/db/changelog/manual/insert_tum_aet_email_templates.sql
  • server/src/main/resources/db/changelog/manual/insert_tum_default_email_templates.sql

Comment thread server/src/main/resources/db/changelog/changes/36_consistent_email_footer.sql Outdated

@Claudia-Anthropica Claudia-Anthropica left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krusche Nice approach — centralizing the footer in MailBuilder.send() and making the signature configurable per research group is clean. Migration 36 handles deployed databases correctly. However, the modifications to past Liquibase migrations (14, 19, 28) will break checksum validation on any database that already applied them. See inline comment.

- Renumber the consistent-email-footer migration to 40 (after develop's 36-39)
  and update its changeset IDs accordingly.
- Resolve the seed-template SQL conflicts in favour of develop's role-naming
  ('advisor and supervisor', APPLICATION_ACCEPTED_NO_ADVISOR, etc.) while
  consistently substituting the literal 'Best regards / Coordination Team'
  sign-off with the new <div th:utext="${emailSignature}"></div> placeholder.
- Revert the in-place edits to migrations 14 / 19 / 28: those changesets
  have already been applied to deployed databases so editing them would
  trip Liquibase's MD5 checksum guard. Migration 40 already covers the
  data-side rewrite via REPLACE.
- Add @JsonInclude(NON_EMPTY) to ResearchGroupSettingsEmailDTO to match
  the codebase's DTO serialization convention.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 9, 2026 10:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR standardizes email footers by introducing a shared notification-settings footer appended in MailBuilder.send(), and adds a per–research group configurable email signature that templates can render via ${emailSignature}.

Changes:

  • Add email_signature to research_group_settings and expose it via DTO/controller + client types.
  • Update stored/default email templates to use ${emailSignature} and remove per-template notification settings blocks.
  • Append a centralized notification-settings footer to every rendered email and add e2e assertions for it.

Reviewed changes

Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
server/src/main/resources/db/changelog/manual/insert_tum_default_email_templates.sql Replaces hardcoded sign-off with ${emailSignature} in default TUM templates.
server/src/main/resources/db/changelog/manual/insert_tum_aet_email_templates.sql Replaces ${config.signature} and removes per-template notification blocks in AET templates.
server/src/main/resources/db/changelog/db.changelog-master.xml Registers Liquibase migration 40_consistent_email_footer.sql.
server/src/main/resources/db/changelog/changes/40_consistent_email_footer.sql Adds email_signature column and bulk-updates existing email_templates contents.
server/src/main/java/de/tum/cit/aet/thesis/utility/MailBuilder.java Adds default emailSignature, withSignature(...), and appends centralized notification footer after render.
server/src/main/java/de/tum/cit/aet/thesis/service/MailingService.java Applies research-group signature to mail builders where a group is available.
server/src/main/java/de/tum/cit/aet/thesis/entity/ResearchGroupSettings.java Adds emailSignature field mapped to email_signature column.
server/src/main/java/de/tum/cit/aet/thesis/dto/ResearchGroupSettingsEmailDTO.java Extends email settings DTO with emailSignature.
server/src/main/java/de/tum/cit/aet/thesis/controller/ResearchGroupSettingsController.java Persists emailSignature from settings payload (trim + null when blank).
client/src/requests/responses/researchGroupSettings.ts Extends client type for research group email settings with emailSignature.
client/e2e/mailpit.ts Adds assertEmailFooter helper to check presence of centralized footer/link.
client/e2e/thesis-workflow.spec.ts Uses assertEmailFooter in thesis creation workflow.
client/e2e/thesis-grading-workflow.spec.ts Uses assertEmailFooter in grading workflow.
client/e2e/proposal-feedback-workflow.spec.ts Uses assertEmailFooter in proposal workflows.
client/e2e/presentation-workflow.spec.ts Uses assertEmailFooter in presentation workflow.
client/e2e/application-workflow.spec.ts Uses assertEmailFooter in application submission workflow.
client/e2e/application-review-workflow.spec.ts Uses assertEmailFooter in review/accept/reject workflows.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread server/src/main/java/de/tum/cit/aet/thesis/service/MailingService.java Outdated
Comment thread server/src/main/java/de/tum/cit/aet/thesis/utility/MailBuilder.java
Comment thread server/src/main/resources/db/changelog/db.changelog-master.xml

@Claudia-Anthropica Claudia-Anthropica left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krusche Most of the previous feedback is addressed — 14/19/28 are reverted, DTO has @JsonInclude, signature is read in the controller. But the merge commit re-introduced the same Liquibase checksum issue on a different changeset: insert_tum_default_email_templates.sql is included via sqlFile from changeset ramona:21-seed-default-email-templates, so modifying it will trip the MD5 guard on production startup. Easy fix — revert that file too, migration 40 already covers the data side. See inline.

@krusche krusche requested a review from Claudia-Anthropica May 9, 2026 12:33
krusche and others added 2 commits May 9, 2026 15:46
Five threads from review:

1. CRITICAL — Liquibase checksum (Claudia). The seed file
   `insert_tum_default_email_templates.sql` is loaded via `sqlFile` from
   the already-applied `ramona:21-seed-default-email-templates`
   changeset, with no `runOnChange`. Modifying its content changes the
   MD5 Liquibase validates on startup, breaking deployed databases.
   Revert the file to develop's content. Migration 40 already rewrites
   the literal "Best regards / Coordination Team" sign-off to
   `${emailSignature}` via REPLACE for both fresh and existing DBs, so
   the end state is identical.

2. Notification-link href in MailBuilder.send() interpolated
   `config.getClientHost()` directly into an HTML attribute. HTML-escape
   the URL via Spring's HtmlUtils and add `rel="noopener noreferrer
   nofollow"` for parity with the other Anchor tags in the codebase.

3. prepareMinimalApplicationMailBuilder did not include the
   `${emailSignature}` placeholder, so emails sent through the minimal
   path skipped the per-research-group signature. Append the placeholder
   so the consistent-footer behaviour holds for all mail paths.

4. (Self) Server-rendered HTML signature is admin-supplied — a known
   trust-boundary choice for a per-group signature feature, intentionally
   not sanitized so admins can embed images/links.

5. (Doc-only) PR description references migration #36; the migration was
   renumbered to #40 during the develop merge — to be updated on the PR.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

@krusche krusche left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the th:utext signature thread (admin-controlled HTML rendering): this is an intentional design choice. The signature is set via the research group settings API which is already restricted to group admins, who likewise control the email templates themselves. HTML embedding (images, links, formatted contact info) is the explicit goal of the feature — sanitizing to a plain-text or strict allowlist would defeat the purpose. Marking the thread as resolved.

@Claudia-Anthropica Claudia-Anthropica left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@krusche All previous feedback addressed — already-applied changesets (14/19/28) and the seed SQL referenced by changeset 21 are back to develop's content, the DTO has @JsonInclude(NON_EMPTY), and migration 40's REPLACE handles the data transform for deployed databases. Footer URL is HTML-escaped centrally in MailBuilder.send(). The th:utext signature design choice is reasonable given the API's group-admin restriction. Nice work.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
client/src/requests/responses/researchGroupSettings.ts (1)

8-8: Grading scheme changes appear unrelated to this PR's email footer objectives.

This field addition is not mentioned in the PR objectives, which focus exclusively on email footer and signature functionality. Consider moving grading scheme changes to a separate PR for clearer change tracking.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/src/requests/responses/researchGroupSettings.ts` at line 8, The
addition of the gradingSchemeSettings field (gradingSchemeSettings?:
IResearchGroupSettingsGradingScheme) in
client/src/requests/responses/researchGroupSettings.ts is unrelated to the email
footer/signature work; remove this field from the current diff or extract it
into a separate branch/PR so the email-footer PR only contains signature/footer
changes; specifically revert or delete the gradingSchemeSettings line from the
ResearchGroupSettings response interface and, if needed, open a new PR that
introduces IResearchGroupSettingsGradingScheme and gradingSchemeSettings with
its own description and tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@client/src/requests/responses/researchGroupSettings.ts`:
- Line 8: The addition of the gradingSchemeSettings field
(gradingSchemeSettings?: IResearchGroupSettingsGradingScheme) in
client/src/requests/responses/researchGroupSettings.ts is unrelated to the email
footer/signature work; remove this field from the current diff or extract it
into a separate branch/PR so the email-footer PR only contains signature/footer
changes; specifically revert or delete the gradingSchemeSettings line from the
ResearchGroupSettings response interface and, if needed, open a new PR that
introduces IResearchGroupSettingsGradingScheme and gradingSchemeSettings with
its own description and tests.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 09f99808-5c6c-49d1-b6f5-453bf5050ed0

📥 Commits

Reviewing files that changed from the base of the PR and between 9a9353a and 3cb4550.

📒 Files selected for processing (8)
  • client/e2e/application-review-workflow.spec.ts
  • client/e2e/application-workflow.spec.ts
  • client/e2e/mailpit.ts
  • client/e2e/presentation-workflow.spec.ts
  • client/e2e/proposal-feedback-workflow.spec.ts
  • client/e2e/thesis-grading-workflow.spec.ts
  • client/e2e/thesis-workflow.spec.ts
  • client/src/requests/responses/researchGroupSettings.ts
✅ Files skipped from review due to trivial changes (1)
  • client/e2e/thesis-grading-workflow.spec.ts

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants