✨(templates) add Email templates for signatures and replies #303
+6,106
−219
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sdemagny
force-pushed
the
sdem/proposition1_template
branch
7 times, most recently
from
9b4dc0c to
2f3206e
Compare
sylvinus
reviewed
Aug 5, 2025
sylvinus
reviewed
Aug 5, 2025
sylvinus
reviewed
Aug 5, 2025
sylvinus
reviewed
Aug 5, 2025
sylvinus
reviewed
Aug 5, 2025
sylvinus
reviewed
Aug 5, 2025
sdemagny
force-pushed
the
sdem/proposition1_template
branch
from
ab63ddf to
1c70072
Compare
jbpenrath
force-pushed
the
sdem/proposition1_template
branch
from
1c70072 to
396df94
Compare
jbpenrath
reviewed
Aug 7, 2025
sylvinus
reviewed
Aug 8, 2025
jbpenrath
reviewed
Aug 11, 2025
sdemagny
force-pushed
the
sdem/proposition1_template
branch
from
396df94 to
8bc60b5
Compare
jbpenrath
reviewed
Aug 20, 2025
jbpenrath
reviewed
Aug 20, 2025
jbpenrath
reviewed
Aug 20, 2025
jbpenrath
reviewed
Aug 20, 2025
jbpenrath
reviewed
Aug 20, 2025
jbpenrath
reviewed
Aug 20, 2025
sdemagny
force-pushed
the
sdem/proposition1_template
branch
2 times, most recently
from
1c29abf to
f61c38b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
♻️ Duplicate comments (21)
src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx (3)
51-53: Remove non-null assertion on selectedMailDomain; render a guard UI instead.This will crash when no domain is selected. Gate the form and show a friendly placeholder.
<div className="modal-compose-signature"> - <SignatureComposeForm domain={selectedMailDomain!} defaultValue={signature} onSuccess={handleSuccess} /> + {selectedMailDomain ? ( + <SignatureComposeForm + domain={selectedMailDomain} + defaultValue={signature} + onSuccess={handleSuccess} + /> + ) : ( + <p className="c__modal__text--centered"> + {t('admin_maildomains_signature.compose_modal.no_domain_selected')} + </p> + )} </div>
64-71: Zod: usemessagekey, noterror, so custom messages display.const signatureComposerSchema = z.object({ - name: z.string().min(1, { error: "admin_maildomains_signature.compose_modal.form.errors.name" }), + name: z.string().min(1, { message: "admin_maildomains_signature.compose_modal.form.errors.name" }), is_active: z.boolean(), is_forced: z.boolean(), - htmlBody: z.string().min(1, { error: "admin_maildomains_signature.compose_modal.form.errors.content" }), - textBody: z.string().min(1, { error: "admin_maildomains_signature.compose_modal.form.errors.content" }), - rawBody: z.string().min(1, { error: "admin_maildomains_signature.compose_modal.form.errors.content" }), + htmlBody: z.string().min(1, { message: "admin_maildomains_signature.compose_modal.form.errors.content" }), + textBody: z.string().min(1, { message: "admin_maildomains_signature.compose_modal.form.errors.content" }), + rawBody: z.string().min(1, { message: "admin_maildomains_signature.compose_modal.form.errors.content" }), });
92-126: Only call onSuccess after a successful mutation; show an error toast on failure.const onSubmit = async (data: SignatureComposerFormData) => { try { if (defaultValue?.id) { await updateSignature({ @@ }); } else { await createSignature({ @@ }); } - } catch (error) { - console.error(error); - } - onSuccess?.(); + onSuccess?.(); + } catch (error) { + console.error(error); + addToast( + <ToasterItem type="error"> + <span>{t("admin_maildomains_signature.toasts.error_save")}</span> + </ToasterItem> + ); + } }src/frontend/src/features/layouts/components/admin/signatures-view/signature-data-grid.tsx (2)
45-59: Handle delete errors and show a per-row spinner instead of a global one.Protect UX against silent failures and avoid blocking all rows while one deletes.
- const [selectedSignature, setSelectedSignature] = useState<ReadOnlyMessageTemplate | undefined>(); + const [selectedSignature, setSelectedSignature] = useState<ReadOnlyMessageTemplate | undefined>(); + const [deletingId, setDeletingId] = useState<string | null>(null); @@ const handleDeleteRow = async (signature: ReadOnlyMessageTemplate) => { const decision = await modals.deleteConfirmationModal({ title: <span className="c__modal__text--centered">{t('admin_maildomains_signature.compose_modal.delete_modal.title', { signature: signature.name })}</span>, children: t('admin_maildomains_signature.compose_modal.delete_modal.message'), }); if (decision === 'delete') { - await deleteSignature({ id: signature.id }); - invalidateMessageTemplates(); - addToast( - <ToasterItem type="info"> - <span>{t("admin_maildomains_signature.toasts.delete")}</span> - </ToasterItem>, - ); + setDeletingId(signature.id); + try { + await deleteSignature({ id: signature.id }); + await invalidateMessageTemplates(); + addToast( + <ToasterItem type="info"> + <span>{t("admin_maildomains_signature.toasts.delete")}</span> + </ToasterItem>, + ); + } catch (e) { + addToast( + <ToasterItem type="error"> + <span>{t("admin_maildomains_signature.toasts.error_delete")}</span> + </ToasterItem>, + ); + } finally { + setDeletingId(null); + } } } @@ - <Button + <Button color="danger" size="small" - icon={isDeleting ? <Spinner size="sm" /> : <Icon name="delete" size={IconSize.SMALL} />} + icon={deletingId === row.id ? <Spinner size="sm" /> : <Icon name="delete" size={IconSize.SMALL} />} onClick={() => handleDeleteRow(row)} - disabled={isDeleting} + disabled={deletingId === row.id} aria-label={t("admin_maildomains_signature.actions.delete")} >Also applies to: 118-123, 29-33
60-67: Wrap toggleActive in try/catch and await invalidation.Avoids silent failures and ensures UI refresh ordering.
- const toggleActive = async (signature: ReadOnlyMessageTemplate) => { - await updateSignature({ - id: signature.id, - data: { is_active: !signature.is_active, maildomain_id: domain.id }, - }); - invalidateMessageTemplates(); - addUpdateSucceededToast(); - } + const toggleActive = async (signature: ReadOnlyMessageTemplate) => { + try { + await updateSignature({ + id: signature.id, + data: { is_active: !signature.is_active, maildomain_id: domain.id }, + }); + await invalidateMessageTemplates(); + addUpdateSucceededToast(); + } catch { + addToast(<ToasterItem type="error"><span>{t("admin_maildomains_signature.toasts.error_update")}</span></ToasterItem>); + } + }src/frontend/src/features/forms/components/message-composer/index.tsx (5)
43-46: Docs vs behavior + field-name mismatch (update comment).Comment says blur-updates and htmlBody/textBody/draftBody, but code updates onChange and uses messageHtmlBody/messageTextBody/messageDraftBody. Align the comment.
- * 3 hidden inputs (`htmlBody`, `textBody` and `draftBody`) are rendered to store - * the HTML, text and raw content of the message. Their values are updated - * when the editor is blurred. Those inputs must be used in the parent form + * 3 hidden inputs (`messageHtmlBody`, `messageTextBody` and `messageDraftBody`) are rendered to store + * the HTML, text and raw content of the message. Their values are updated + * on every content change and on mount. Those inputs must be used in the parent form * to retrieve all the content of the message.
67-71: Guard JSON.parse to avoid crashes on malformed defaultValue.Wrap parse in try/catch and fall back to an empty paragraph.
- const getInitialContent = () => { - const initialContent = defaultValue - ? JSON.parse(defaultValue) - : [{ type: "paragraph", content: "" }]; + const getInitialContent = () => { + const initialContent = (() => { + if (!defaultValue) return [{ type: "paragraph", content: "" }]; + try { + return JSON.parse(defaultValue); + } catch { + return [{ type: "paragraph", content: "" }]; + } + })();
31-37: Don’t hard-code quoted-message mode to "forward"; make it a prop.Expose quotedMode?: "reply" | "forward" (default "reply") and pass it to the quoted block.
type MessageComposerProps = FieldProps & { mailboxId?: string; blockNoteOptions?: Partial<MessageComposerBlockNoteSchema> defaultValue?: string; quotedMessage?: Message; disabled?: boolean; + quotedMode?: "reply" | "forward"; } -export const MessageComposer = ({ mailboxId, blockNoteOptions, defaultValue, quotedMessage, disabled = false, ...props }: MessageComposerProps) => { +export const MessageComposer = ({ mailboxId, blockNoteOptions, defaultValue, quotedMessage, quotedMode = "reply", disabled = false, ...props }: MessageComposerProps) => { … - mode: "forward", + mode: quotedMode,Follow-up: ensure the parent passes quotedMode based on context (reply/forward) where MessageComposer is used.
Also applies to: 48-48, 78-78
104-110: Mark all fields dirty and debounce async handleChange to avoid races.
- Add { shouldDirty: true } for text/html (you already do for draft).
- Debounce handleChange or add a last-call token to prevent out-of-order writes on rapid edits.
- form.setValue("messageTextBody", markdown); - form.setValue("messageHtmlBody", html); + form.setValue("messageTextBody", markdown, { shouldDirty: true }); + form.setValue("messageHtmlBody", html, { shouldDirty: true });Example (outside this hunk) to debounce and ensure last-call-wins:
// near component body const changeSeq = useRef(0); const debouncedHandleChange = useMemo(() => debounce(async () => { const seq = ++changeSeq.current; const markdown = await editor.blocksToMarkdownLossy(editor.document); const html = await editor.blocksToHTML(editor.document); if (seq !== changeSeq.current) return; // stale write form.setValue("messageDraftBody", JSON.stringify(editor.document), { shouldDirty: true }); form.setValue("messageTextBody", markdown, { shouldDirty: true }); form.setValue("messageHtmlBody", html, { shouldDirty: true }); }, 250), [editor, form]);Then wire onChange: debouncedHandleChange.
151-156: Consider debouncing the onChange handler.Converting to markdown+HTML per keystroke is heavy; wire a debounced handler to reduce load.
src/backend/core/api/serializers.py (1)
993-1005: Enforce mutual exclusivity between mailbox_id and maildomain_id on createCreation currently allows both; align with intent and fail fast.
def validate(self, attrs): """Validate template data.""" - # exactly one maildomain_id or mailbox_id is required + # exactly one of maildomain_id or mailbox_id is required mailbox_id = attrs.get("mailbox_id") maildomain_id = attrs.get("maildomain_id") if not self.instance: # Only for creation if not mailbox_id and not maildomain_id: raise serializers.ValidationError( "Either mailbox_id or maildomain_id is required." ) + if mailbox_id and maildomain_id: + raise serializers.ValidationError( + "Provide either mailbox_id or maildomain_id, not both." + )src/backend/core/api/viewsets/message_template.py (1)
153-160: Handle invalid type safely (avoid KeyError → 500)Wrap enum lookup; return 400 or empty set.
if _type: - queryset = queryset.filter(type=MessageTemplateTypeChoices[_type.upper()]) + try: + queryset = queryset.filter(type=MessageTemplateTypeChoices[_type.upper()]) + except KeyError: + return MessageTemplate.objects.none()src/backend/core/migrations/0006_blob_maildomain_alter_blob_mailbox_and_more.py (1)
39-41: help_text lists types not present in enum“forward, auto reply” are not in MessageTemplateTypeChoices. Update the model and regenerate this migration to keep docs accurate.
src/backend/core/models.py (3)
1491-1498: Fix type help_text to match enum valuesRemove “forward, auto reply” to avoid misleading docs.
- help_text=_( - "Type of template (reply, forward, new message, auto reply, signature)" - ), + help_text=_("Type of template (reply, new_message, signature)"),
1290-1295: Use ValidationError and model clean() for ownership invariantRaising ValueError surfaces as 500. Move the check to clean(); BaseModel.save already calls full_clean().
-def save(self, *args, **kwargs): - # at least one of mailbox or maildomain must be set - if not self.mailbox and not self.maildomain: - raise ValueError("Blob must have at least a mailbox or maildomain") - super().save(*args, **kwargs) +def clean(self): + # at least one of mailbox or maildomain must be set + if not self.mailbox and not self.maildomain: + raise ValidationError({"__all__": "Blob must have at least a mailbox or maildomain"}) + super().clean()Also add a CheckConstraint at DB level (see migration comment).
1568-1585: abilities shape inconsistent with API schema (list vs dict)AbilitiesModelSerializer documents an object of booleans; returning a list will break clients.
-def get_abilities(self, user): - """Get abilities for this template based on user permissions.""" - abilities = [] - - # Check if user has access to any of the mailboxes or maildomains - if ( - user.is_superuser - or (self.mailbox and self.mailbox.accesses.filter(user=user).exists()) - or (self.maildomain and self.maildomain.accesses.filter(user=user).exists()) - ): - abilities.extend( - [ - UserAbilities.CAN_VIEW_DOMAIN_ADMIN, - UserAbilities.CAN_CREATE_MAILDOMAINS, - ] - ) - - return abilities +def get_abilities(self, user): + """Return CRUD-like abilities consistent with other models.""" + can_access = bool( + user.is_superuser + or (self.mailbox and self.mailbox.accesses.filter(user=user).exists()) + or (self.maildomain and self.maildomain.accesses.filter(user=user).exists()) + ) + is_admin = bool(user.is_superuser) + return { + CRUDAbilities.CAN_READ: can_access, + CRUDAbilities.CAN_CREATE: is_admin, + CRUDAbilities.CAN_UPDATE: is_admin or can_access, + CRUDAbilities.CAN_PARTIALLY_UPDATE: is_admin or can_access, + CRUDAbilities.CAN_DELETE: is_admin, + }If you still need the UserAbilities flags, expose them under a separate field.
src/backend/core/api/openapi.json (1)
2770-2797: Add typed query params for filters on list (and page).Filters are documented but not declared; add explicit parameters, including pagination. Also use
is_forced(new name) notis_default.Apply:
"/api/v1.0/message-templates/": { "get": { "operationId": "message_templates_list", "description": "ViewSet for managing message templates and signatures.\n\nThis ViewSet provides endpoints for creating, reading, updating, and deleting\nmessage templates and signatures. Templates can be used for replies, new messages,\nand signatures.\n\nFiltering:\n- type: Filter by template type (reply, new_message, signature)\n- is_active: Filter by active status (true/false)\n- mailbox_id: Filter by mailbox UUID\n- maildomain_id: Filter by mail domain UUID\n- is_forced: Filter by forced status (true/false) - works with mailbox or maildomain", + "parameters": [ + { "in": "query", "name": "type", "schema": { "$ref": "#/components/schemas/MessageTemplateTypeChoices" }, "description": "Filter by template type." }, + { "in": "query", "name": "is_active", "schema": { "type": "boolean" }, "description": "Filter by active status." }, + { "in": "query", "name": "mailbox_id", "schema": { "type": "string", "format": "uuid" }, "description": "Filter by mailbox UUID." }, + { "in": "query", "name": "maildomain_id", "schema": { "type": "string", "format": "uuid" }, "description": "Filter by mail domain UUID." }, + { "in": "query", "name": "is_forced", "schema": { "type": "boolean" }, "description": "Filter by forced status." }, + { "in": "query", "name": "page", "required": false, "schema": { "type": "integer" }, "description": "Pagination page." } + ], "tags": [ "message-templates" ],src/backend/core/tests/api/test_send_message_signature.py (4)
133-137: Decode blob bytes before asserting (avoid brittle str(bytes)).- message = models.Message.objects.get(id=draft_message.id) - assert ( - "<p>Best regards,<br>John Doe<br>Software Engineer<br>Engineering</p>" - in str(message.blob.get_content()) - ) + message = models.Message.objects.get(id=draft_message.id) + content = message.blob.get_content().decode() + assert "<p>Best regards,<br>John Doe<br>Software Engineer<br>Engineering</p>" in content
204-208: Same: decode bytes in text-body assertion.- message = models.Message.objects.get(id=draft_message.id) - assert ( - "Best regards,\\nJohn Doe\\nSoftware Engineer\\nEngineering\\n" - in str(message.blob.get_content()) - ) + message = models.Message.objects.get(id=draft_message.id) + content = message.blob.get_content().decode() + assert "Best regards,\nJohn Doe\nSoftware Engineer\nEngineering" in content
243-247: Same: decode bytes for HTML-only case.- message = models.Message.objects.get(id=draft_message.id) - assert ( - "<p>Best regards,<br>John Doe<br>Software Engineer<br>Engineering</p>" - in str(message.blob.get_content()) - ) + message = models.Message.objects.get(id=draft_message.id) + content = message.blob.get_content().decode() + assert "<p>Best regards,<br>John Doe<br>Software Engineer<br>Engineering</p>" in content
292-296: Same: decode bytes for reply case text assertion.- message = models.Message.objects.get(id=draft_message.id) - assert ( - "Best regards,\\nJohn Doe\\nSoftware Engineer\\nEngineering\\n" - in str(message.blob.get_content()) - ) + message = models.Message.objects.get(id=draft_message.id) + content = message.blob.get_content().decode() + assert "Best regards,\nJohn Doe\nSoftware Engineer\nEngineering" in content
🧹 Nitpick comments (11)
src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx (2)
1-13: Import useEffect (needed for form interlock below).import { zodResolver } from "@hookform/resolvers/zod"; +import { useEffect } from "react";
28-30: Optional: narrow cache invalidation to the list query key used by the list hook.Reduces unnecessary refetches if other template queries exist.
If
useMessageTemplatesListexposes aqueryKey, switch toinvalidateQueries({ queryKey: thatKey }). Otherwise, keep current.src/frontend/src/features/layouts/components/admin/signatures-view/signature-data-grid.tsx (1)
31-33: Optional: narrow cache invalidation to the current list query.If the list hook exposes a
queryKey, preferinvalidateQueries({ queryKey })from that hook to avoid unrelated refetches.Would you like me to inspect the generated API hooks and propose a centralized
messageTemplatesKeyshelper?src/backend/core/api/serializers.py (1)
1006-1016: Comment and behavior mismatchDocstring says “both html_body and text_body being updated” but the condition is “either field present”. Consider rewording for clarity.
src/backend/core/api/viewsets/message_template.py (2)
171-185: Document 400 responses for render actionThe endpoint returns 400 on validation/render errors; reflect it in OpenAPI.
@extend_schema( responses={ description="Template rendered with provided context", response={ "type": "object", "properties": { "html_body": {"type": "string"}, "text_body": {"type": "string"}, }, }, ), + 400: OpenApiResponse(description="Bad request"), },
191-207: Validate provided mailbox_id/maildomain_id against template ownershipYou require one of the IDs but don’t verify it matches the template; this can hide client bugs.
mailbox_id = request.query_params.get("mailbox_id") maildomain_id = request.query_params.get("maildomain_id") if not mailbox_id and not maildomain_id: return Response( {"error": "At least one of mailbox_id or maildomain_id is required"}, status=status.HTTP_400_BAD_REQUEST, ) # get template template = self.get_object() +if mailbox_id and str(template.mailbox_id) != mailbox_id: + return Response({"error": "Template not linked to this mailbox"}, status=400) +if maildomain_id and str(template.maildomain_id) != maildomain_id: + return Response({"error": "Template not linked to this maildomain"}, status=400) try: rendered = template.render_template(request.user) return Response(rendered)src/backend/core/migrations/0006_blob_maildomain_alter_blob_mailbox_and_more.py (1)
14-53: Add DB-level constraints for ownership invariantsEnforce “at least one of mailbox or maildomain” on Blob and MessageTemplate with CheckConstraint in a follow-up migration.
Example Meta.constraints to add in models.py, then makemigrations:
models.CheckConstraint( name="blob_owner_present", check=~(models.Q(mailbox__isnull=True) & models.Q(maildomain__isnull=True)), ) # and similar for MessageTemplatesrc/backend/core/models.py (1)
1524-1530: Nit: clarify is_forced help_textSmall grammar tweak for clarity.
- help_text=_( - "Whether this template is forced no other same type template can be used" - ), + help_text=_("If true, this template is the only allowed one of its type for the owner"),src/backend/core/api/openapi.json (2)
5661-5736: Clarifyraw_blobshape in read model.Expose it as a UUID for consistency with write models.
"raw_blob": { - "type": "string", + "type": "string", + "format": "uuid", "nullable": true, - "description": "Get raw blob.", + "description": "Blob ID containing the raw template content.", "readOnly": true },
2999-3044: Align render endpoint doc with behavior (no input context).Either accept context via POST body, or update text to reflect server-side context resolution and add 403.
- "get": { - "operationId": "message_templates_render_retrieve", - "description": "Render a template with the provided context variables.", + "get": { + "operationId": "message_templates_render_retrieve", + "description": "Render a template using server-side context (current user, mailbox/domain).", "parameters": [ ... "responses": { "200": { "content": { "application/json": { "schema": { "type": "object", "properties": { - "html_body": { "type": "string" }, - "text_body": { "type": "string" } + "html_body": { "type": "string", "nullable": true }, + "text_body": { "type": "string", "nullable": true } } } } }, "description": "Template rendered with provided context" }, + "403": { "description": "Permission denied" }, "404": { "description": "Template not found" } }If you intend clients to supply merge variables, switch to POST and add a
{ "context": { ... } }requestBody schema.src/backend/core/tests/api/test_send_message_signature.py (1)
96-108: Add a negative test: cannot inject/use a signature not accessible to sender.Covers the outstanding hardening item (check access before replacements).
Proposed new test (illustrative):
def test_send_message_rejects_unowned_signature(user, mailbox_access, mailbox, draft_message): other_mailbox = factories.MailboxFactory() rogue_signature = factories.MessageTemplateFactory( type=models.MessageTemplateTypeChoices.SIGNATURE, is_active=True, mailbox=other_mailbox, html_body="<p>Should not render</p>", text_body="Should not render", ) client = APIClient(); client.force_authenticate(user=user) with patch("core.api.viewsets.send.send_message_task") as mock_task: mock_task.delay.return_value = MagicMock(id="task-123") resp = client.post( reverse("send-message"), { "messageId": str(draft_message.id), "senderId": str(mailbox.id), "textBody": f"Hello\n<SignatureID>{rogue_signature.id}</SignatureID>\n", "htmlBody": f"<p>Hello</p><p><SignatureID>{rogue_signature.id}</SignatureID></p>", }, ) assert resp.status_code in (status.HTTP_400_BAD_REQUEST, status.HTTP_403_FORBIDDEN)I can adapt the expected status once the view enforces this check.
Also applies to: 258-267
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
src/frontend/src/features/api/gen/message-templates/message-templates.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message_template.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message_template_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_message_template_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/read_only_message_template.tsis excluded by!**/gen/**
📒 Files selected for processing (16)
src/backend/core/admin.py(1 hunks)src/backend/core/api/openapi.json(4 hunks)src/backend/core/api/permissions.py(1 hunks)src/backend/core/api/serializers.py(2 hunks)src/backend/core/api/viewsets/message_template.py(1 hunks)src/backend/core/factories.py(1 hunks)src/backend/core/mda/outbound.py(3 hunks)src/backend/core/migrations/0006_blob_maildomain_alter_blob_mailbox_and_more.py(1 hunks)src/backend/core/models.py(6 hunks)src/backend/core/tests/api/test_message_template.py(1 hunks)src/backend/core/tests/api/test_send_message_signature.py(1 hunks)src/backend/core/tests/mda/test_outbound.py(1 hunks)src/frontend/src/features/forms/components/message-composer/index.tsx(1 hunks)src/frontend/src/features/i18n/translations.json(6 hunks)src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/signatures-view/signature-data-grid.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/backend/core/factories.py
- src/backend/core/tests/api/test_message_template.py
- src/backend/core/api/permissions.py
- src/backend/core/tests/mda/test_outbound.py
- src/frontend/src/features/i18n/translations.json
- src/backend/core/admin.py
- src/backend/core/mda/outbound.py
🧰 Additional context used
🧬 Code graph analysis (7)
src/frontend/src/features/forms/components/message-composer/index.tsx (6)
src/frontend/src/features/blocknote/signature-block/index.tsx (3)
BlockSignature(91-131)BlockSignatureConfigProps(132-132)SignatureTemplateSelector(19-86)src/frontend/src/features/blocknote/quoted-message-block/index.tsx (1)
QuotedMessageBlock(5-41)src/frontend/src/features/api/gen/message-templates/message-templates.ts (1)
useMessageTemplatesList(184-212)src/frontend/src/features/api/gen/models/message_template_type_choices.ts (2)
MessageTemplateTypeChoices(9-10)MessageTemplateTypeChoices(13-17)src/frontend/src/features/blocknote/blocknote-view-field/index.tsx (1)
BlockNoteViewField(11-27)src/frontend/src/features/blocknote/toolbar.tsx (1)
Toolbar(6-30)
src/backend/core/api/viewsets/message_template.py (3)
src/backend/core/api/serializers.py (2)
MessageTemplateSerializer(948-1092)ReadOnlyMessageTemplateSerializer(921-945)src/backend/core/models.py (4)
Mailbox(411-549)MailDomain(236-408)MessageTemplate(1455-1628)render_template(1587-1618)src/backend/core/api/permissions.py (1)
IsAllowedToManageMessageTemplate(354-465)
src/backend/core/api/serializers.py (3)
src/backend/core/enums.py (1)
MessageTemplateTypeChoices(108-113)src/backend/core/models.py (25)
get_content(1296-1310)Meta(84-85)Meta(194-197)Meta(276-279)Meta(440-445)Meta(567-571)Meta(595-598)Meta(775-780)Meta(947-951)Meta(968-972)Meta(1009-1013)Meta(1068-1072)Meta(1281-1285)Meta(1342-1346)Meta(1382-1386)Meta(1436-1440)Meta(1532-1536)MessageTemplate(1455-1628)Mailbox(411-549)MailDomain(236-408)Blob(1226-1310)create_blob(465-491)create_blob(1161-1223)content_type(1352-1354)delete(909-929)src/frontend/src/features/api/gen/models/message_template.ts (1)
MessageTemplate(14-42)
src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx (8)
src/frontend/src/features/api/gen/models/read_only_message_template.ts (1)
ReadOnlyMessageTemplate(14-45)src/frontend/src/features/providers/admin-maildomain.tsx (1)
useAdminMailDomain(47-53)src/frontend/src/features/ui/components/toaster/index.tsx (2)
addToast(63-76)ToasterItem(16-61)src/frontend/src/features/api/gen/message-templates/message-templates.ts (2)
useMessageTemplatesCreate(304-324)useMessageTemplatesUpdate(612-632)src/frontend/src/features/api/gen/models/message_template_type_choices.ts (2)
MessageTemplateTypeChoices(9-10)MessageTemplateTypeChoices(13-17)src/frontend/src/features/forms/components/react-hook-form/rhf-input.tsx (1)
RhfInput(10-33)src/frontend/src/features/layouts/components/admin/modal-compose-signature/signature-composer.tsx (1)
SignatureComposer(37-110)src/frontend/src/features/forms/components/react-hook-form/rhf-checkbox.tsx (1)
RhfCheckbox(10-33)
src/backend/core/tests/api/test_send_message_signature.py (4)
src/backend/core/factories.py (8)
UserFactory(18-29)MailboxFactory(58-94)MailboxAccessFactory(97-107)ThreadFactory(123-130)ThreadAccessFactory(133-143)ContactFactory(146-155)MessageFactory(158-184)MessageTemplateFactory(220-231)src/backend/core/enums.py (3)
MailboxRoleChoices(15-21)ThreadAccessRoleChoices(24-28)MessageTemplateTypeChoices(108-113)src/backend/core/api/viewsets/send.py (1)
post(97-155)src/backend/core/models.py (8)
Message(1019-1155)get_content(1296-1310)save(87-90)save(202-205)save(284-287)save(785-837)save(1290-1294)save(1541-1566)
src/backend/core/models.py (4)
src/backend/core/enums.py (3)
MessageTemplateTypeChoices(108-113)CompressionTypeChoices(54-58)UserAbilities(75-79)src/backend/core/tests/api/test_attachments.py (1)
blob(160-166)src/backend/core/api/serializers.py (4)
create(877-879)create(1018-1043)update(881-883)update(1045-1092)src/backend/core/api/viewsets/message_template.py (1)
render_template(188-208)
src/frontend/src/features/layouts/components/admin/signatures-view/signature-data-grid.tsx (6)
src/frontend/src/features/api/gen/message-templates/message-templates.ts (3)
useMessageTemplatesList(184-212)useMessageTemplatesPartialUpdate(725-749)useMessageTemplatesDestroy(838-861)src/frontend/src/features/api/gen/models/message_template_type_choices.ts (2)
MessageTemplateTypeChoices(9-10)MessageTemplateTypeChoices(13-17)src/frontend/src/features/api/gen/models/read_only_message_template.ts (1)
ReadOnlyMessageTemplate(14-45)src/frontend/src/features/ui/components/toaster/index.tsx (2)
addToast(63-76)ToasterItem(16-61)src/frontend/src/features/ui/components/banner/index.tsx (1)
Banner(14-41)src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx (1)
ModalComposeSignature(23-56)
⏰ 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). (3)
- GitHub Check: build-front
- GitHub Check: check-api-state
- GitHub Check: test-back
🔇 Additional comments (3)
src/frontend/src/features/forms/components/message-composer/index.tsx (1)
162-165: Hidden inputs wiring looks good.Names align with the form registration and updated docs above.
src/backend/core/models.py (2)
486-491: Good change: centralize blob creation via managerDelegating to Blob.objects.create_blob improves consistency and compression handling.
1158-1224: BlobManager.create_blob: solid; minor note on defaultsManager defaults to ZSTD while model default is NONE; acceptable since manager is the primary path. No change required.
Comment on lines
+123
to
+146
| useEffect(() => { | ||
| const signatureBlock = editor.getBlock('signature'); | ||
| const defaultSignatureTemplateId = activeSignatures.find(signature => signature.is_forced)?.id | ||
| if (defaultSignatureTemplateId && !(signatureBlock?.props as BlockSignatureConfigProps | undefined)?.templateId) { | ||
| if (!signatureBlock) { | ||
| editor.insertBlocks( | ||
| [{ id: "signature", type: "signature", props: { templateId: defaultSignatureTemplateId, mailboxId: mailboxId } }], | ||
| editor.document[0].id, | ||
| "after" | ||
| ); | ||
| } | ||
| else { | ||
| editor.replaceBlocks( | ||
| ["signature"], | ||
| [{ id: "signature", type: "signature", props: { templateId: defaultSignatureTemplateId, mailboxId: mailboxId } }], | ||
| ); | ||
| } | ||
| } | ||
| // In case there is a signature block but the templateId does not match an active signature, we remove the block. | ||
| if (signatureBlock) { | ||
| const isSignatureActive = activeSignatures.findIndex(signature => signature.id === (signatureBlock.props as BlockSignatureConfigProps).templateId); | ||
| if (isSignatureActive === -1) editor.removeBlocks(["signature"]); | ||
| } | ||
| }, [isLoadingSignatures]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce forced signature and fix effect deps.
- If a forced signature exists, replace any non-matching signature block, not only when templateId is missing.
- Depend on activeSignatures/mailboxId so the effect re-runs if templates change.
useEffect(() => {
const signatureBlock = editor.getBlock('signature');
- const defaultSignatureTemplateId = activeSignatures.find(signature => signature.is_forced)?.id
- if (defaultSignatureTemplateId && !(signatureBlock?.props as BlockSignatureConfigProps | undefined)?.templateId) {
- if (!signatureBlock) {
+ const defaultSignatureTemplateId = activeSignatures.find(signature => signature.is_forced)?.id;
+ const currentTemplateId = (signatureBlock?.props as BlockSignatureConfigProps | undefined)?.templateId;
+ // If a forced signature exists and differs from the current one, enforce it.
+ if (defaultSignatureTemplateId && currentTemplateId !== defaultSignatureTemplateId) {
+ if (!signatureBlock) {
editor.insertBlocks(
[{ id: "signature", type: "signature", props: { templateId: defaultSignatureTemplateId, mailboxId: mailboxId } }],
editor.document[0].id,
"after"
);
- }
- else {
+ } else {
editor.replaceBlocks(
["signature"],
[{ id: "signature", type: "signature", props: { templateId: defaultSignatureTemplateId, mailboxId: mailboxId } }],
);
}
}
// In case there is a signature block but the templateId does not match an active signature, we remove the block.
if (signatureBlock) {
const isSignatureActive = activeSignatures.findIndex(signature => signature.id === (signatureBlock.props as BlockSignatureConfigProps).templateId);
if (isSignatureActive === -1) editor.removeBlocks(["signature"]);
}
- }, [isLoadingSignatures]);
+ }, [activeSignatures, mailboxId]);Optional: disable signature switching in the toolbar UI when a forced signature exists to avoid UX churn.
📝 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.
Suggested change
| useEffect(() => { | |
| const signatureBlock = editor.getBlock('signature'); | |
| const defaultSignatureTemplateId = activeSignatures.find(signature => signature.is_forced)?.id | |
| if (defaultSignatureTemplateId && !(signatureBlock?.props as BlockSignatureConfigProps | undefined)?.templateId) { | |
| if (!signatureBlock) { | |
| editor.insertBlocks( | |
| [{ id: "signature", type: "signature", props: { templateId: defaultSignatureTemplateId, mailboxId: mailboxId } }], | |
| editor.document[0].id, | |
| "after" | |
| ); | |
| } | |
| else { | |
| editor.replaceBlocks( | |
| ["signature"], | |
| [{ id: "signature", type: "signature", props: { templateId: defaultSignatureTemplateId, mailboxId: mailboxId } }], | |
| ); | |
| } | |
| } | |
| // In case there is a signature block but the templateId does not match an active signature, we remove the block. | |
| if (signatureBlock) { | |
| const isSignatureActive = activeSignatures.findIndex(signature => signature.id === (signatureBlock.props as BlockSignatureConfigProps).templateId); | |
| if (isSignatureActive === -1) editor.removeBlocks(["signature"]); | |
| } | |
| }, [isLoadingSignatures]); | |
| useEffect(() => { | |
| const signatureBlock = editor.getBlock('signature'); | |
| const defaultSignatureTemplateId = activeSignatures.find(signature => signature.is_forced)?.id; | |
| const currentTemplateId = (signatureBlock?.props as BlockSignatureConfigProps | undefined)?.templateId; | |
| // If a forced signature exists and differs from the current one, enforce it. | |
| if (defaultSignatureTemplateId && currentTemplateId !== defaultSignatureTemplateId) { | |
| if (!signatureBlock) { | |
| editor.insertBlocks( | |
| [{ id: "signature", type: "signature", props: { templateId: defaultSignatureTemplateId, mailboxId: mailboxId } }], | |
| editor.document[0].id, | |
| "after" | |
| ); | |
| } else { | |
| editor.replaceBlocks( | |
| ["signature"], | |
| [{ id: "signature", type: "signature", props: { templateId: defaultSignatureTemplateId, mailboxId: mailboxId } }], | |
| ); | |
| } | |
| } | |
| // In case there is a signature block but the templateId does not match an active signature, we remove the block. | |
| if (signatureBlock) { | |
| const isSignatureActive = activeSignatures.findIndex(signature => signature.id === (signatureBlock.props as BlockSignatureConfigProps).templateId); | |
| if (isSignatureActive === -1) editor.removeBlocks(["signature"]); | |
| } | |
| }, [activeSignatures, mailboxId]); |
Comment on lines
+158
to
+161
| <Toolbar> | ||
| <SignatureTemplateSelector templates={activeSignatures} isLoading={isLoadingSignatures} mailboxId={mailboxId} /> | ||
| </Toolbar> | ||
| </BlockNoteViewField> |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Back-end guardrail reminder for signature replacement.
Ensure the server verifies that the chosen signature belongs to the sender before replacement and performs this check before processing quoted parts (matches PR checklist).
🏁 Script executed:
#!/bin/bash
# Verify server-side guards exist (ownership check and pre-quote replacement).
rg -nPI 'insert_signature|_insert_signature|SignatureID|template_id' -C3 backend --type=py || trueLength of output: 147
🏁 Script executed:
#!/bin/bash
# Global search for any server-side signature handling to verify guardrails
rg -n 'signature' -C3 --glob '!**/node_modules/**'Length of output: 170798
Add server-side permission check for signature insertion
In src/backend/core/mda/outbound.py, _get_signatures currently does MessageTemplate.objects.get(id=signature_id) without scoping to the sender’s mailbox or user; modify it (and any upstream calls in prepare_outbound_message) to filter by signature_id and the requesting user’s mailbox/maildomain and raise on mismatch.
🤖 Prompt for AI Agents
In src/frontend/src/features/forms/components/message-composer/index.tsx around
lines 158 to 161, the review requests adding a server-side permission check for
signature insertion: update the backend function
src/backend/core/mda/outbound.py::_get_signatures so it no longer does a blind
MessageTemplate.objects.get(id=signature_id) — instead query MessageTemplate
filtered by id=signature_id AND the requesting mailbox/user or mailbox's domain
(e.g. mailbox__id=..., mailbox__domain=... or owner=user) so only templates
belonging to that mailbox/user are returned; have _get_signatures raise a
NotFound/PermissionDenied error on mismatch; propagate the mailbox/user context
into prepare_outbound_message (and any upstream callsites) so the scoped query
can be performed; ensure callers handle the raised exception and return an
appropriate error to the client.
Comment on lines
+75
to
+91
| const SignatureComposeForm = ({ domain, defaultValue, onSuccess }: SignatureComposerFormProps) => { | ||
| const { t } = useTranslation(); | ||
| const form = useForm<SignatureComposerFormData>({ | ||
| resolver: zodResolver(signatureComposerSchema), | ||
| defaultValues: { | ||
| name: defaultValue?.name ?? "", | ||
| is_active: defaultValue?.is_active ?? true, | ||
| is_forced: defaultValue?.is_forced ?? false, | ||
| htmlBody: defaultValue?.html_body, | ||
| textBody: defaultValue?.text_body, | ||
| rawBody: defaultValue?.raw_blob ?? undefined, | ||
| } | ||
| }); | ||
| const { mutateAsync: createSignature, isPending } = useMessageTemplatesCreate(); | ||
| const { mutateAsync: updateSignature, isPending: isUpdating } = useMessageTemplatesUpdate(); | ||
| const isSubmitting = isPending || isUpdating; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
“Active” should disable and clear “Forced”.
Meets the PR checklist item: when inactive, force cannot be enabled.
const SignatureComposeForm = ({ domain, defaultValue, onSuccess }: SignatureComposerFormProps) => {
const { t } = useTranslation();
const form = useForm<SignatureComposerFormData>({
@@
});
+ const isActive = form.watch("is_active");
+ useEffect(() => {
+ if (!isActive) {
+ form.setValue("is_forced", false, { shouldDirty: true });
+ }
+ }, [isActive]);📝 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.
Suggested change
| const SignatureComposeForm = ({ domain, defaultValue, onSuccess }: SignatureComposerFormProps) => { | |
| const { t } = useTranslation(); | |
| const form = useForm<SignatureComposerFormData>({ | |
| resolver: zodResolver(signatureComposerSchema), | |
| defaultValues: { | |
| name: defaultValue?.name ?? "", | |
| is_active: defaultValue?.is_active ?? true, | |
| is_forced: defaultValue?.is_forced ?? false, | |
| htmlBody: defaultValue?.html_body, | |
| textBody: defaultValue?.text_body, | |
| rawBody: defaultValue?.raw_blob ?? undefined, | |
| } | |
| }); | |
| const { mutateAsync: createSignature, isPending } = useMessageTemplatesCreate(); | |
| const { mutateAsync: updateSignature, isPending: isUpdating } = useMessageTemplatesUpdate(); | |
| const isSubmitting = isPending || isUpdating; | |
| const SignatureComposeForm = ({ domain, defaultValue, onSuccess }: SignatureComposerFormProps) => { | |
| const { t } = useTranslation(); | |
| const form = useForm<SignatureComposerFormData>({ | |
| resolver: zodResolver(signatureComposerSchema), | |
| defaultValues: { | |
| name: defaultValue?.name ?? "", | |
| is_active: defaultValue?.is_active ?? true, | |
| is_forced: defaultValue?.is_forced ?? false, | |
| htmlBody: defaultValue?.html_body, | |
| textBody: defaultValue?.text_body, | |
| rawBody: defaultValue?.raw_blob ?? undefined, | |
| } | |
| }); | |
| // Ensure that if “Active” is turned off, “Forced” is cleared | |
| const isActive = form.watch("is_active"); | |
| useEffect(() => { | |
| if (!isActive) { | |
| form.setValue("is_forced", false, { shouldDirty: true }); | |
| } | |
| }, [isActive]); | |
| const { mutateAsync: createSignature, isPending } = useMessageTemplatesCreate(); | |
| const { mutateAsync: updateSignature, isPending: isUpdating } = useMessageTemplatesUpdate(); | |
| const isSubmitting = isPending || isUpdating; | |
| // …rest of the component… | |
| } |
🤖 Prompt for AI Agents
In
src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx
around lines 75-91, the form currently allows "is_forced" to remain true when
"is_active" is set to false; update the component so that when is_active becomes
false the form clears and disables the "is_forced" field and the submitted
payload always contains is_forced: false. Implement this by adding a useEffect
that watches form.watch("is_active") and calls form.setValue("is_forced", false)
whenever it becomes false, ensure the UI checkbox for is_forced is rendered
disabled when is_active is false, and additionally coerce is_forced to false in
the create/update submit handlers before calling mutateAsync to guarantee the
backend never receives a forced flag for an inactive signature.
Comment on lines
+153
to
+158
| <RhfCheckbox | ||
| label={t('admin_maildomains_signature.compose_modal.form.labels.is_forced')} | ||
| name="is_forced" | ||
| text={t('admin_maildomains_signature.compose_modal.form.helper_text.is_forced')} | ||
| fullWidth | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Disable “Forced” checkbox when inactive.
- <RhfCheckbox
+ <RhfCheckbox
label={t('admin_maildomains_signature.compose_modal.form.labels.is_forced')}
name="is_forced"
text={t('admin_maildomains_signature.compose_modal.form.helper_text.is_forced')}
+ disabled={!form.watch('is_active')}
fullWidth
/>📝 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.
Suggested change
| <RhfCheckbox | |
| label={t('admin_maildomains_signature.compose_modal.form.labels.is_forced')} | |
| name="is_forced" | |
| text={t('admin_maildomains_signature.compose_modal.form.helper_text.is_forced')} | |
| fullWidth | |
| /> | |
| <RhfCheckbox | |
| label={t('admin_maildomains_signature.compose_modal.form.labels.is_forced')} | |
| name="is_forced" | |
| text={t('admin_maildomains_signature.compose_modal.form.helper_text.is_forced')} | |
| disabled={!form.watch('is_active')} | |
| fullWidth | |
| /> |
🤖 Prompt for AI Agents
In
src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx
around lines 153-158, the "Forced" RhfCheckbox must be disabled when the
signature is inactive; update the component to read the active state from the
form (use useFormContext/useWatch or equivalent to watch the
'is_active'/'active' field) and pass disabled={!isActive} (or
disabled={watch('is_active') === false}) to RhfCheckbox so the checkbox is not
interactive when the signature is inactive; keep the label/helper_text the same.
Comment on lines
+68
to
+75
| const toggleDefault = async (signature: ReadOnlyMessageTemplate) => { | ||
| await updateSignature({ | ||
| id: signature.id, | ||
| data: { is_forced: !signature.is_forced, maildomain_id: domain.id }, | ||
| }); | ||
| invalidateMessageTemplates(); | ||
| addUpdateSucceededToast(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Don’t allow “Forced” on inactive signatures; gate the toggle and disable the cell.
Matches the PR checklist item (“Active” disables “Default”).
- const toggleDefault = async (signature: ReadOnlyMessageTemplate) => {
- await updateSignature({
- id: signature.id,
- data: { is_forced: !signature.is_forced, maildomain_id: domain.id },
- });
- invalidateMessageTemplates();
- addUpdateSucceededToast();
- }
+ const toggleDefault = async (signature: ReadOnlyMessageTemplate) => {
+ if (!signature.is_active) {
+ addToast(<ToasterItem type="error"><span>{t("admin_maildomains_signature.errors.cannot_force_inactive")}</span></ToasterItem>);
+ return;
+ }
+ try {
+ await updateSignature({
+ id: signature.id,
+ data: { is_forced: !signature.is_forced, maildomain_id: domain.id },
+ });
+ await invalidateMessageTemplates();
+ addUpdateSucceededToast();
+ } catch {
+ addToast(<ToasterItem type="error"><span>{t("admin_maildomains_signature.toasts.error_update")}</span></ToasterItem>);
+ }
+ }
@@
- <Checkbox checked={row.is_forced} onChange={() => toggleDefault(row)} disabled={isUpdating} />
+ <Checkbox
+ checked={row.is_forced}
+ onChange={() => toggleDefault(row)}
+ disabled={!row.is_active || isUpdating}
+ />Also applies to: 88-96
🤖 Prompt for AI Agents
In
src/frontend/src/features/layouts/components/admin/signatures-view/signature-data-grid.tsx
around lines 68-75 (and similarly lines 88-96), the toggle handler and cell UI
currently allow setting "Forced" on signatures regardless of active state;
update the code so the toggle is gated by signature.active (or
signature.is_active) and the cell is rendered disabled when the signature is
inactive. Specifically, check the signature's active flag before calling
updateSignature (no-op and optionally show a disabled state/tooltip if
inactive), and pass a disabled prop to the toggle cell renderer when signature
is inactive so the cell is visually and interactively disabled. Ensure
invalidateMessageTemplates and addUpdateSucceededToast are only called after a
successful update.
f9d4e73 to
0a93c11
Compare
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)
src/backend/core/factories.py (1)
220-232: Factory violates model constraint: must set mailbox or maildomain.MessageTemplate.save() raises when both are null; this factory will fail in tests unless every call overrides relations.
Apply this diff to set a sane default and still allow overrides:
class MessageTemplateFactory(factory.django.DjangoModelFactory): @@ name = factory.Sequence(lambda n: f"Template {n}") html_body = factory.Faker("sentence", nb_words=10) text_body = factory.Faker("text", max_nb_chars=100) + mailbox = factory.SubFactory(MailboxFactory) # default to satisfy model constraint type = factory.fuzzy.FuzzyChoice( [choice[0] for choice in models.MessageTemplateTypeChoices.choices] ) + is_active = Truesrc/backend/core/mda/outbound.py (1)
108-143: Prevent third‑party signature injection: insert signature before quoting replies/forwards.Currently replacement runs after embedding the quoted original, so a malicious incoming message containing ... could trigger unintended replacement. Move insertion to happen on user-authored bodies before create_reply_message/create_forward_message.
Apply this diff:
@@ - # Handle reply and forward message embedding - if message.parent: + # Replace the signature in the user-authored bodies before quoting + text_body, html_body = _insert_signature(text_body, html_body, user, mailbox_sender) + + # Handle reply and forward message embedding + if message.parent: @@ - # Update the bodies with properly formatted reply content + # Update the bodies with properly formatted reply content (only the quoted/nesting part) if nested_data.get("textBody"): text_body = nested_data["textBody"][0]["content"] if nested_data.get("htmlBody"): html_body = nested_data["htmlBody"][0]["content"] @@ - # Replace the signature in the text and html bodies - text_body, html_body = _insert_signature(text_body, html_body, user) + # Signature was already inserted prior to quoting (see above)
♻️ Duplicate comments (16)
src/frontend/src/features/forms/components/message-composer/index.tsx (5)
43-46: Docs vs behavior + field-name mismatch (still present).Comment says values update on blur and names htmlBody/textBody/draftBody, but code updates onChange/on mount and uses messageHtmlBody/messageTextBody/messageDraftBody. Align the comment.
- * 3 hidden inputs (`htmlBody`, `textBody` and `draftBody`) are rendered to store - * the HTML, text and raw content of the message. Their values are updated - * when the editor is blurred. Those inputs must be used in the parent form + * 3 hidden inputs (`messageHtmlBody`, `messageTextBody` and `messageDraftBody`) are rendered to store + * the HTML, text and raw content of the message. Their values are updated + * on every content change and on mount. Those inputs must be used in the parent form * to retrieve all the content of the message.
31-37: Expose quotedMode prop; stop hard-coding “forward”.Lets composer show correct “replied/forwarded” banner.
type MessageComposerProps = FieldProps & { mailboxId?: string; blockNoteOptions?: Partial<MessageComposerBlockNoteSchema> defaultValue?: string; quotedMessage?: Message; disabled?: boolean; + quotedMode?: "reply" | "forward"; } -export const MessageComposer = ({ mailboxId, blockNoteOptions, defaultValue, quotedMessage, disabled = false, ...props }: MessageComposerProps) => { +export const MessageComposer = ({ mailboxId, blockNoteOptions, defaultValue, quotedMessage, quotedMode = "reply", disabled = false, ...props }: MessageComposerProps) => { @@ props: { - mode: "forward", + mode: quotedMode, messageId: quotedMessage.id, subject: quotedMessage.subject, recipients: quotedMessage.to.map((to) => to.email).join(", "), sender: quotedMessage.sender.email, received_at: quotedMessage.created_at }Follow-up: pass quotedMode from the parent (reply/forward context).
Also applies to: 48-49, 78-85
68-71: Guard JSON.parse to avoid crashes on malformed defaultValue.- const initialContent = defaultValue - ? JSON.parse(defaultValue) - : [{ type: "paragraph", content: "" }]; + const initialContent = (() => { + if (!defaultValue) return [{ type: "paragraph", content: "" }]; + try { + return JSON.parse(defaultValue); + } catch { + return [{ type: "paragraph", content: "" }]; + } + })();
6-6: Preserve signature placeholder: use BlockNote HTML exporter (not Markdown→HTML).Current Markdown→HTML path drops custom blocks’ toExternalHTML (e.g., ), breaking server-side replacement. Export HTML via BlockNote, and mark fields dirty.
-import MailHelper from '@/features/utils/mail-helper';const handleChange = async () => { - const markdown = await editor.blocksToMarkdownLossy(editor.document); - const html = await MailHelper.markdownToHtml(markdown); + const markdown = await editor.blocksToMarkdownLossy(editor.document); + // Use BlockNote HTML export so custom blocks' toExternalHTML are preserved. + const html = await editor.blocksToFullHTML(editor.document); form.setValue("messageDraftBody", JSON.stringify(editor.document), { shouldDirty: true }); - form.setValue("messageTextBody", markdown); - form.setValue("messageHtmlBody", html); + form.setValue("messageTextBody", markdown, { shouldDirty: true }); + form.setValue("messageHtmlBody", html, { shouldDirty: true }); }If blocksToFullHTML isn’t available in your version, use blocksToHTML/blocksToHTMLLossy.
Also applies to: 104-110
123-147: Enforce forced signature and fix effect deps.Re-run when signatures/mailbox change; replace mismatched signatures, and remove stale ones.
- useEffect(() => { - const signatureBlock = editor.getBlock('signature'); - const defaultSignatureTemplateId = activeSignatures.find(signature => signature.is_forced)?.id - if (defaultSignatureTemplateId && !(signatureBlock?.props as BlockSignatureConfigProps | undefined)?.templateId) { - if (!signatureBlock) { - editor.insertBlocks( - [{ id: "signature", type: "signature", props: { templateId: defaultSignatureTemplateId, mailboxId: mailboxId } }], - editor.document[0].id, - "after" - ); - } - else { - editor.replaceBlocks( - ["signature"], - [{ id: "signature", type: "signature", props: { templateId: defaultSignatureTemplateId, mailboxId: mailboxId } }], - ); - } - } - // In case there is a signature block but the templateId does not match an active signature, we remove the block. - if (signatureBlock) { - const isSignatureActive = activeSignatures.findIndex(signature => signature.id === (signatureBlock.props as BlockSignatureConfigProps).templateId); - if (isSignatureActive === -1) editor.removeBlocks(["signature"]); - } - }, [isLoadingSignatures]); + useEffect(() => { + const signatureBlock = editor.getBlock('signature'); + const defaultSignatureTemplateId = activeSignatures.find(s => s.is_forced)?.id; + const currentTemplateId = (signatureBlock?.props as BlockSignatureConfigProps | undefined)?.templateId; + // Enforce forced signature if present and different from current. + if (defaultSignatureTemplateId && currentTemplateId !== defaultSignatureTemplateId) { + if (!signatureBlock) { + editor.insertBlocks( + [{ id: "signature", type: "signature", props: { templateId: defaultSignatureTemplateId, mailboxId } }], + editor.document[0].id, + "after" + ); + } else { + editor.replaceBlocks( + ["signature"], + [{ id: "signature", type: "signature", props: { templateId: defaultSignatureTemplateId, mailboxId } }], + ); + } + } + // Remove stale signature if its template is no longer active. + if (signatureBlock) { + const isActive = activeSignatures.some(s => s.id === (signatureBlock.props as BlockSignatureConfigProps).templateId); + if (!isActive) editor.removeBlocks(["signature"]); + } + }, [activeSignatures, mailboxId]);src/backend/core/api/serializers.py (2)
1018-1043: Handle lookup errors and validate raw_blob JSON before blob creation.
- Wrap Mailbox/MailDomain lookups to return 400s, not 500s.
- Validate raw_blob JSON since content_type is application/json.
def create(self, validated_data): """Create template with relationships.""" mailbox_id = validated_data.pop("mailbox_id", None) maildomain_id = validated_data.pop("maildomain_id", None) raw_blob_content = validated_data.pop("raw_blob", None) # Set mailbox or maildomain directly - if mailbox_id: - validated_data["mailbox"] = models.Mailbox.objects.get(id=mailbox_id) - elif maildomain_id: - validated_data["maildomain"] = models.MailDomain.objects.get( - id=maildomain_id - ) + try: + if mailbox_id: + validated_data["mailbox"] = models.Mailbox.objects.get(id=mailbox_id) + elif maildomain_id: + validated_data["maildomain"] = models.MailDomain.objects.get(id=maildomain_id) + except models.Mailbox.DoesNotExist: + raise serializers.ValidationError({"mailbox_id": "Mailbox not found"}) + except models.MailDomain.DoesNotExist: + raise serializers.ValidationError({"maildomain_id": "Maildomain not found"}) # Create raw_blob relationship if content provided if raw_blob_content: + # Validate JSON + import json + try: + json.loads(raw_blob_content) + except json.JSONDecodeError as e: + raise serializers.ValidationError({"raw_blob": f"Invalid JSON: {e.msg}"}) blob = models.Blob.objects.create_blob( content=raw_blob_content.encode("utf-8"), content_type="application/json", mailbox_id=mailbox_id, maildomain_id=maildomain_id, ) validated_data["raw_blob"] = blob template = super().create(validated_data) return template
1045-1091: Apply relationship updates, avoid ownerless blobs, and validate JSON on update.
- New mailbox_id/maildomain_id are validated but not assigned.
- Blob creation can fail with ValueError if both owners omitted; fallback to instance owners.
- Validate JSON like in create().
def update(self, instance, validated_data): """Update template with relationships.""" mailbox_id = validated_data.pop("mailbox_id", None) maildomain_id = validated_data.pop("maildomain_id", None) raw_blob_content = validated_data.pop("raw_blob", None) @@ elif maildomain_id is not None and instance.mailbox: maildomain = models.MailDomain.objects.get(id=maildomain_id) if instance.mailbox.domain != maildomain: raise serializers.ValidationError( "Mailbox domain does not match maildomain." ) + # Apply relationship updates + try: + if mailbox_id is not None: + validated_data["mailbox"] = models.Mailbox.objects.get(id=mailbox_id) + if maildomain_id is not None: + validated_data["maildomain"] = models.MailDomain.objects.get(id=maildomain_id) + except models.Mailbox.DoesNotExist: + raise serializers.ValidationError({"mailbox_id": "Mailbox not found"}) + except models.MailDomain.DoesNotExist: + raise serializers.ValidationError({"maildomain_id": "Maildomain not found"}) # Handle raw_blob update if raw_blob_content is not None: try: if instance.raw_blob: instance.raw_blob.delete() except models.Blob.DoesNotExist: pass if raw_blob_content: # Only create blob if content is not empty - blob = models.Blob.objects.create_blob( + # Validate JSON + import json + try: + json.loads(raw_blob_content) + except json.JSONDecodeError as e: + raise serializers.ValidationError({"raw_blob": f"Invalid JSON: {e.msg}"}) + + # Fallback ownership to existing instance when not provided + owner_mailbox_id = mailbox_id if mailbox_id is not None else instance.mailbox_id + owner_maildomain_id = maildomain_id if maildomain_id is not None else instance.maildomain_id + blob = models.Blob.objects.create_blob( content=raw_blob_content.encode("utf-8"), content_type="application/json", - mailbox_id=mailbox_id, - maildomain_id=maildomain_id, + mailbox_id=owner_mailbox_id, + maildomain_id=owner_maildomain_id, ) validated_data["raw_blob"] = blob else: validated_data["raw_blob"] = None template = super().update(instance, validated_data) return templatesrc/backend/core/models.py (3)
1491-1498: Help text doesn’t match enum.Remove “forward, auto reply” to avoid confusion.
- help_text=_( - "Type of template (reply, forward, new message, auto reply, signature)" - ), + help_text=_("Type of template (reply, new message, signature)"),
1290-1295: Raise ValidationError (not ValueError) and move checks to clean().Throwing ValueError bubbles as 500s. Use model validation to surface 400s; BaseModel.save() already calls full_clean().
class Blob(BaseModel): @@ - def save(self, *args, **kwargs): - # at least one of mailbox or maildomain must be set - if not self.mailbox and not self.maildomain: - raise ValueError("Blob must have at least a mailbox or maildomain") - super().save(*args, **kwargs) + def clean(self): + if not self.mailbox and not self.maildomain: + raise ValidationError({"__all__": "Blob must have at least a mailbox or maildomain"}) + super().clean()class MessageTemplate(BaseModel): @@ - def save(self, *args, **kwargs): - # at least one of mailbox or maildomain must be set - if not self.mailbox and not self.maildomain: - raise ValueError( - "MessageTemplate must have at least a mailbox or maildomain" - ) + def clean(self): + if not self.mailbox and not self.maildomain: + raise ValidationError({"__all__": "MessageTemplate must have at least a mailbox or maildomain"}) + super().clean() - if self.is_forced: + def save(self, *args, **kwargs): + if self.is_forced: # If this template is being set as forced, unforce other templates of the same type # in the same maildomain or mailbox @@ super().save(*args, **kwargs)Optionally add DB CheckConstraints for both models’ ownership.
Also applies to: 1541-1567
1568-1586: abilities shape is inconsistent; will break AbilitiesModelSerializer.Other models return dicts keyed by CRUDAbilities (and domain abilities). Here a list is returned, conflicting with OpenAPI and consumer expectations. Return a dict.
- def get_abilities(self, user): - """Get abilities for this template based on user permissions.""" - abilities = [] - # Check if user has access to any of the mailboxes or maildomains - if ( - user.is_superuser - or (self.mailbox and self.mailbox.accesses.filter(user=user).exists()) - or (self.maildomain and self.maildomain.accesses.filter(user=user).exists()) - ): - abilities.extend( - [ - UserAbilities.CAN_VIEW_DOMAIN_ADMIN, - UserAbilities.CAN_CREATE_MAILDOMAINS, - ] - ) - return abilities + def get_abilities(self, user): + """Return CRUD-like abilities consistent with other models.""" + can_access = bool( + getattr(user, "is_superuser", False) + or (self.mailbox and self.mailbox.accesses.filter(user=user).exists()) + or (self.maildomain and self.maildomain.accesses.filter(user=user).exists()) + ) + is_admin = bool(getattr(user, "is_superuser", False)) + return { + CRUDAbilities.CAN_READ: can_access, + CRUDAbilities.CAN_CREATE: is_admin, + CRUDAbilities.CAN_UPDATE: is_admin or can_access, + CRUDAbilities.CAN_PARTIALLY_UPDATE: is_admin or can_access, + CRUDAbilities.CAN_DELETE: is_admin, + }src/backend/core/api/viewsets/message_template.py (1)
153-155: Guard type filter against invalid values to avoid 500s.MessageTemplateTypeChoices[_type.upper()] raises KeyError on invalid input.
Apply:
- if _type: - queryset = queryset.filter(type=MessageTemplateTypeChoices[_type.upper()]) + if _type: + try: + template_type = MessageTemplateTypeChoices[_type.upper()] + except KeyError: + from rest_framework.exceptions import ValidationError + raise ValidationError( + f"Invalid type '{_type}'. Allowed: " + + ", ".join([c.label for c in MessageTemplateTypeChoices]) + ) + queryset = queryset.filter(type=template_type)src/backend/core/api/openapi.json (4)
2770-2797: Add typed query parameters for message-templates list.Filters are documented but not declared; add a parameters array so clients can use them reliably.
"/api/v1.0/message-templates/": { "get": { "operationId": "message_templates_list", "description": "ViewSet for managing message templates and signatures.\n\nThis ViewSet provides endpoints for creating, reading, updating, and deleting\nmessage templates and signatures. Templates can be used for replies, new messages,\nand signatures.\n\nFiltering:\n- type: Filter by template type (reply, new_message, signature)\n- is_active: Filter by active status (true/false)\n- mailbox_id: Filter by mailbox UUID\n- maildomain_id: Filter by mail domain UUID\n- is_forced: Filter by forced status (true/false) - works with mailbox or maildomain", + "parameters": [ + { "in": "query", "name": "type", "schema": { "$ref": "#/components/schemas/MessageTemplateTypeChoices" }, "description": "Filter by template type." }, + { "in": "query", "name": "is_active", "schema": { "type": "boolean" }, "description": "Filter by active status." }, + { "in": "query", "name": "mailbox_id", "schema": { "type": "string", "format": "uuid" }, "description": "Filter by mailbox UUID." }, + { "in": "query", "name": "maildomain_id", "schema": { "type": "string", "format": "uuid" }, "description": "Filter by mail domain UUID." }, + { "in": "query", "name": "is_forced", "schema": { "type": "boolean" }, "description": "Filter by forced/enforced status." } + ], "tags": [ "message-templates" ],
5217-5289: Make raw_blob a Blob UUID and clarify is_forced wording.Current typing suggests opaque content; contract elsewhere uses Blob records. Align schema with UUID and clear semantics.
"raw_blob": { - "type": "string", - "description": "Raw blob content" + "type": "string", + "format": "uuid", + "description": "Blob ID containing the raw template content" }, "is_forced": { "type": "boolean", "default": false, - "description": "Set as forced template" + "description": "Force usage of this template within its scope (mailbox or maildomain)" },
5290-5343: POST/PUT contract: require html_body, text_body, and raw_blob together; type IDs as UUIDs; enforce mailbox/maildomain XOR.Prevents partial/inconsistent updates and mismatched scoping.
"MessageTemplateRequest": { "type": "object", "description": "Serialize message templates for POST/PUT/PATCH operations.", "properties": { @@ "raw_blob": { - "type": "string", - "minLength": 1, - "description": "Raw blob content" + "type": "string", + "format": "uuid", + "minLength": 1, + "description": "Blob ID containing the raw template content" }, @@ "mailbox_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Mailbox UUID" }, "maildomain_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Maildomain UUID" } }, - "required": [ - "name", - "raw_blob", - "type" - ] + "required": ["name","type","raw_blob","html_body","text_body"], + "oneOf": [ + { "required": ["mailbox_id"] }, + { "required": ["maildomain_id"] } + ] },
5593-5641: PATCH contract: all-or-none rule for html_body/text_body/raw_blob; add UUID formats.Guarantees the triplet stays consistent on partial updates.
"PatchedMessageTemplateRequest": { "type": "object", "description": "Serialize message templates for POST/PUT/PATCH operations.", "properties": { @@ "raw_blob": { - "type": "string", - "minLength": 1, - "description": "Raw blob content" + "type": "string", + "format": "uuid", + "minLength": 1, + "description": "Blob ID containing the raw template content" }, @@ "mailbox_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Mailbox UUID" }, "maildomain_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Maildomain UUID" } - } + }, + "oneOf": [ + { "not": { "anyOf": [ { "required": ["raw_blob"] }, { "required": ["html_body"] }, { "required": ["text_body"] } ] } }, + { "required": ["raw_blob","html_body","text_body"] } + ] },src/backend/core/tests/api/test_send_message_signature.py (1)
133-137: Decode blob bytes before assertions (avoid brittle str(bytes)).Use .decode() to assert on actual content; repr() of bytes is unstable and includes escapes.
- message = models.Message.objects.get(id=draft_message.id) - assert ( - "<p>Best regards,<br>John Doe<br>Software Engineer<br>Engineering</p>" - in str(message.blob.get_content()) - ) + message = models.Message.objects.get(id=draft_message.id) + content = message.blob.get_content().decode() + assert "<p>Best regards,<br>John Doe<br>Software Engineer<br>Engineering</p>" in content- message = models.Message.objects.get(id=draft_message.id) - assert ( - "Best regards,\\nJohn Doe\\nSoftware Engineer\\nEngineering\\n" - in str(message.blob.get_content()) - ) + message = models.Message.objects.get(id=draft_message.id) + content = message.blob.get_content().decode() + assert "Best regards,\nJohn Doe\nSoftware Engineer\nEngineering" in content- message = models.Message.objects.get(id=draft_message.id) - assert ( - "<p>Best regards,<br>John Doe<br>Software Engineer<br>Engineering</p>" - in str(message.blob.get_content()) - ) + message = models.Message.objects.get(id=draft_message.id) + content = message.blob.get_content().decode() + assert "<p>Best regards,<br>John Doe<br>Software Engineer<br>Engineering</p>" in content- message = models.Message.objects.get(id=draft_message.id) - assert ( - "Best regards,\\nJohn Doe\\nSoftware Engineer\\nEngineering\\n" - in str(message.blob.get_content()) - ) + message = models.Message.objects.get(id=draft_message.id) + content = message.blob.get_content().decode() + assert "Best regards,\nJohn Doe\nSoftware Engineer\nEngineering" in contentAlso applies to: 204-208, 243-247, 292-296
🧹 Nitpick comments (13)
src/frontend/src/features/forms/components/message-composer/index.tsx (3)
62-66: Minor grammar fix in comment.- * If the user is replying or forwarding a message, a quoted-message block is append + * If the user is replying or forwarding a message, a quoted-message block is appended
116-117: Avoid unhandled promise in effect.- handleChange(); + void handleChange();
158-161: Hide toolbar when disabled.Prevents confusing, non-functional controls.
- <Toolbar> - <SignatureTemplateSelector templates={activeSignatures} isLoading={isLoadingSignatures} mailboxId={mailboxId} /> - </Toolbar> + {!disabled && ( + <Toolbar> + <SignatureTemplateSelector templates={activeSignatures} isLoading={isLoadingSignatures} mailboxId={mailboxId} /> + </Toolbar> + )}src/backend/core/migrations/0006_blob_maildomain_alter_blob_mailbox_and_more.py (1)
31-52: Consider DB constraints for “forced” uniqueness and ownership checks.Add partial unique constraints to guarantee at most one forced template per (mailbox,type) and per (maildomain,type), and add CheckConstraints enforcing at least one owner for Blob and MessageTemplate.
src/backend/core/models.py (2)
1158-1224: Minor: prefer self.model.create and align defaults.Using self.create (or self.model.objects.create) keeps custom managers subclass-friendly; also consider aligning Blob.compression default with manager default to avoid surprises when bypassing the manager.
1281-1286: Optional: add DB constraints for invariants.
- Blob: CheckConstraint(mailbox IS NOT NULL OR maildomain IS NOT NULL).
- MessageTemplate: same ownership check; and two partial UniqueConstraints to guarantee one forced per (mailbox,type) and per (maildomain,type).
Also applies to: 1532-1537
src/backend/core/api/viewsets/message_template.py (3)
46-51: Use read-only serializer for retrieve as well.Currently only list uses ReadOnly serializer; consider also using it for retrieve to keep API shapes consistent.
Apply:
- if self.action == "list": + if self.action in {"list", "retrieve"}: return ReadOnlyMessageTemplateSerializer
112-147: Reject ambiguous filters when both mailbox_id and maildomain_id are provided.Silently choosing one is confusing for clients.
Two options:
- Strict (preferred):
+ if mailbox_id and maildomain_id: + from rest_framework.exceptions import ValidationError + raise ValidationError("Provide either mailbox_id or maildomain_id, not both.")
- Lenient:
+ if mailbox_id and maildomain_id: + return MessageTemplate.objects.none()
191-199: Drop unused mailbox_id/maildomain_id requirement in render action.You require a param but don’t use it; get_object() already scopes by permissions.
Apply:
- mailbox_id = request.query_params.get("mailbox_id") - maildomain_id = request.query_params.get("maildomain_id") - if not mailbox_id and not maildomain_id: - return Response( - {"error": "At least one of mailbox_id or maildomain_id is required"}, - status=status.HTTP_400_BAD_REQUEST, - ) + # Optional: keep query params for future context, but not requiredsrc/backend/core/api/openapi.json (2)
2842-2851: Mark template path IDs as UUIDs.Path params for message-templates endpoints should declare format: uuid for consistency with the rest of the API.
- "schema": { "type": "string" } + "schema": { "type": "string", "format": "uuid" }Apply to id in retrieve, update, partial_update, destroy, and render.
Also applies to: 2877-2885, 2926-2934, 2974-2982, 3004-3011
5661-5734: Polish ReadOnly schema: consistent wording and UUID format for raw_blob.Minor doc/typing consistency.
"raw_blob": { - "type": "string", + "type": "string", + "format": "uuid", "nullable": true, - "description": "Get raw blob.", + "description": "Blob ID containing the raw template content", "readOnly": true }, "is_forced": { "type": "boolean", - "description": "Whether this template is forced no other same type template can be used" + "description": "When true, this template is enforced within its scope (mailbox or maildomain)" },src/backend/core/tests/api/test_send_message_signature.py (2)
96-107: Add a negative test: reject/invalidate unauthorized SignatureID.Security item still open in PR: ensure the signature ID is accessible to the sender before replacement. Add a test where SignatureID points to a template from a different mailbox/domain and assert 403 or that the placeholder remains untouched.
I can draft this test if you confirm expected behavior (403 vs. ignore).
119-128: Also assert placeholder tag is removed from stored content.Quick sanity check that the marker isn’t leaked to recipients.
Example follow-up assert after decoding:
assert "<SignatureID>" not in content and "</SignatureID>" not in content
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
src/frontend/src/features/api/gen/message-templates/message-templates.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message_template.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message_template_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_message_template_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/read_only_message_template.tsis excluded by!**/gen/**
📒 Files selected for processing (16)
src/backend/core/admin.py(1 hunks)src/backend/core/api/openapi.json(4 hunks)src/backend/core/api/permissions.py(1 hunks)src/backend/core/api/serializers.py(2 hunks)src/backend/core/api/viewsets/message_template.py(1 hunks)src/backend/core/factories.py(1 hunks)src/backend/core/mda/outbound.py(3 hunks)src/backend/core/migrations/0006_blob_maildomain_alter_blob_mailbox_and_more.py(1 hunks)src/backend/core/models.py(6 hunks)src/backend/core/tests/api/test_message_template.py(1 hunks)src/backend/core/tests/api/test_send_message_signature.py(1 hunks)src/backend/core/tests/mda/test_outbound.py(1 hunks)src/frontend/src/features/forms/components/message-composer/index.tsx(1 hunks)src/frontend/src/features/i18n/translations.json(6 hunks)src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/signatures-view/signature-data-grid.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/backend/core/tests/api/test_message_template.py
- src/backend/core/admin.py
- src/frontend/src/features/layouts/components/admin/signatures-view/signature-data-grid.tsx
- src/backend/core/tests/mda/test_outbound.py
- src/frontend/src/features/i18n/translations.json
- src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx
🧰 Additional context used
🧬 Code graph analysis (8)
src/backend/core/factories.py (2)
src/backend/core/models.py (17)
Meta(84-85)Meta(194-197)Meta(276-279)Meta(440-445)Meta(567-571)Meta(595-598)Meta(775-780)Meta(947-951)Meta(968-972)Meta(1009-1013)Meta(1068-1072)Meta(1281-1285)Meta(1342-1346)Meta(1382-1386)Meta(1436-1440)Meta(1532-1536)MessageTemplate(1455-1628)src/backend/core/enums.py (1)
MessageTemplateTypeChoices(108-113)
src/frontend/src/features/forms/components/message-composer/index.tsx (6)
src/frontend/src/features/blocknote/signature-block/index.tsx (3)
BlockSignature(91-131)BlockSignatureConfigProps(132-132)SignatureTemplateSelector(19-86)src/frontend/src/features/blocknote/quoted-message-block/index.tsx (1)
QuotedMessageBlock(5-41)src/frontend/src/features/api/gen/message-templates/message-templates.ts (1)
useMessageTemplatesList(184-212)src/frontend/src/features/api/gen/models/message_template_type_choices.ts (2)
MessageTemplateTypeChoices(9-10)MessageTemplateTypeChoices(13-17)src/frontend/src/features/blocknote/blocknote-view-field/index.tsx (1)
BlockNoteViewField(11-27)src/frontend/src/features/blocknote/toolbar.tsx (1)
Toolbar(6-30)
src/backend/core/tests/api/test_send_message_signature.py (3)
src/backend/core/tests/mda/test_outbound.py (2)
fixture_user(126-134)fixture_signature_template(138-148)src/backend/core/factories.py (8)
UserFactory(18-29)MailboxFactory(58-94)MailboxAccessFactory(97-107)ThreadFactory(123-130)ThreadAccessFactory(133-143)ContactFactory(146-155)MessageFactory(158-184)MessageTemplateFactory(220-231)src/backend/core/api/viewsets/send.py (1)
post(97-155)
src/backend/core/api/serializers.py (4)
src/backend/core/enums.py (1)
MessageTemplateTypeChoices(108-113)src/frontend/src/features/api/gen/models/message_template_type_choices.ts (2)
MessageTemplateTypeChoices(9-10)MessageTemplateTypeChoices(13-17)src/backend/core/models.py (25)
get_content(1296-1310)Meta(84-85)Meta(194-197)Meta(276-279)Meta(440-445)Meta(567-571)Meta(595-598)Meta(775-780)Meta(947-951)Meta(968-972)Meta(1009-1013)Meta(1068-1072)Meta(1281-1285)Meta(1342-1346)Meta(1382-1386)Meta(1436-1440)Meta(1532-1536)MessageTemplate(1455-1628)Mailbox(411-549)MailDomain(236-408)Blob(1226-1310)create_blob(465-491)create_blob(1161-1223)content_type(1352-1354)delete(909-929)src/frontend/src/features/api/gen/models/message_template.ts (1)
MessageTemplate(14-42)
src/backend/core/api/viewsets/message_template.py (3)
src/backend/core/api/serializers.py (2)
MessageTemplateSerializer(948-1092)ReadOnlyMessageTemplateSerializer(921-945)src/backend/core/models.py (4)
Mailbox(411-549)MailDomain(236-408)MessageTemplate(1455-1628)render_template(1587-1618)src/backend/core/api/permissions.py (1)
IsAllowedToManageMessageTemplate(354-465)
src/backend/core/api/permissions.py (2)
src/backend/core/enums.py (2)
MailboxRoleChoices(15-21)MailDomainAccessRoleChoices(48-51)src/backend/core/models.py (2)
MailboxAccess(552-574)MailDomainAccess(1367-1389)
src/backend/core/mda/outbound.py (1)
src/backend/core/models.py (5)
User(122-233)MessageTemplate(1455-1628)render_template(1587-1618)Mailbox(411-549)Message(1019-1155)
src/backend/core/models.py (2)
src/backend/core/enums.py (3)
MessageTemplateTypeChoices(108-113)CompressionTypeChoices(54-58)UserAbilities(75-79)src/backend/core/api/serializers.py (4)
create(877-879)create(1018-1043)update(881-883)update(1045-1092)
⏰ 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-back
🔇 Additional comments (5)
src/backend/core/api/serializers.py (2)
921-931: Read serializer looks good.get_raw_blob returns decoded content and keeps raw_blob read-only. No issues.
963-974: OK to override raw_blob in representation.Consistent with read-only serializer behavior.
src/backend/core/migrations/0006_blob_maildomain_alter_blob_mailbox_and_more.py (1)
28-29: OneToOne change for Message.draft_blob may break existing data.If any blob is referenced by multiple messages as draft_blob today, this migration will fail. Add a data migration to de-duplicate or validate, or preflight-check in staging.
src/backend/core/models.py (1)
486-491: Good: centralize blob creation.Switching to Blob.objects.create_blob() keeps compression/hash logic consistent.
src/backend/core/api/permissions.py (1)
354-466: Permission logic looks solid and aligned with roles.Object writes require EDITOR/SENDER/ADMIN on mailbox or ADMIN on maildomain; reads are filtered by queryset. LGTM.
src/backend/core/api/serializers.py
Outdated
Comment on lines
993
to
1021
| def validate(self, attrs): | ||
| """Validate template data.""" | ||
| # exactly one maildomain_id or mailbox_id is required | ||
| mailbox_id = attrs.get("mailbox_id") | ||
| maildomain_id = attrs.get("maildomain_id") | ||
|
|
||
| if not self.instance: # Only for creation | ||
| if not mailbox_id and not maildomain_id: | ||
| raise serializers.ValidationError( | ||
| "Either mailbox_id or maildomain_id is required." | ||
| ) | ||
|
|
||
| # For creation or when both html_body and text_body are being updated | ||
| if "html_body" in attrs or "text_body" in attrs: | ||
| html_body = attrs.get("html_body", "") | ||
| text_body = attrs.get("text_body", "") | ||
|
|
||
| # If we're updating both fields and both are empty, that's invalid | ||
| if not html_body and not text_body: | ||
| raise serializers.ValidationError( | ||
| "At least one of html_body or text_body must be provided." | ||
| ) | ||
|
|
||
| return attrs |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enforce mutual exclusivity and atomic updates for html/text/raw_blob.
- Creation currently allows both mailbox_id and maildomain_id. Disallow both.
- PR objective: require html_body, text_body, and raw_blob to be updated together. Add atomic validation.
def validate(self, attrs):
"""Validate template data."""
# exactly one maildomain_id or mailbox_id is required
mailbox_id = attrs.get("mailbox_id")
maildomain_id = attrs.get("maildomain_id")
if not self.instance: # Only for creation
- if not mailbox_id and not maildomain_id:
+ if not mailbox_id and not maildomain_id:
raise serializers.ValidationError(
"Either mailbox_id or maildomain_id is required."
)
+ if mailbox_id and maildomain_id:
+ raise serializers.ValidationError(
+ "Provide either mailbox_id or maildomain_id, not both."
+ )
- # For creation or when both html_body and text_body are being updated
- if "html_body" in attrs or "text_body" in attrs:
+ # Atomic update: if any of {html_body, text_body, raw_blob} is present, require all.
+ atomic_fields = {"html_body", "text_body", "raw_blob"}
+ present = atomic_fields.intersection(attrs.keys())
+ if present and present != atomic_fields:
+ missing = ", ".join(sorted(atomic_fields - present))
+ raise serializers.ValidationError(
+ f"html_body, text_body and raw_blob must be updated together. Missing: {missing}."
+ )
+
+ # Also enforce at least one of html/text has content when updating bodies
+ if "html_body" in attrs or "text_body" in attrs:
html_body = attrs.get("html_body", "")
text_body = attrs.get("text_body", "")
# If we're updating both fields and both are empty, that's invalid
if not html_body and not text_body:
raise serializers.ValidationError(
"At least one of html_body or text_body must be provided."
)
return attrs📝 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.
Suggested change
| def validate(self, attrs): | |
| """Validate template data.""" | |
| # exactly one maildomain_id or mailbox_id is required | |
| mailbox_id = attrs.get("mailbox_id") | |
| maildomain_id = attrs.get("maildomain_id") | |
| if not self.instance: # Only for creation | |
| if not mailbox_id and not maildomain_id: | |
| raise serializers.ValidationError( | |
| "Either mailbox_id or maildomain_id is required." | |
| ) | |
| # For creation or when both html_body and text_body are being updated | |
| if "html_body" in attrs or "text_body" in attrs: | |
| html_body = attrs.get("html_body", "") | |
| text_body = attrs.get("text_body", "") | |
| # If we're updating both fields and both are empty, that's invalid | |
| if not html_body and not text_body: | |
| raise serializers.ValidationError( | |
| "At least one of html_body or text_body must be provided." | |
| ) | |
| return attrs | |
| def validate(self, attrs): | |
| """Validate template data.""" | |
| # exactly one maildomain_id or mailbox_id is required | |
| mailbox_id = attrs.get("mailbox_id") | |
| maildomain_id = attrs.get("maildomain_id") | |
| if not self.instance: # Only for creation | |
| if not mailbox_id and not maildomain_id: | |
| raise serializers.ValidationError( | |
| "Either mailbox_id or maildomain_id is required." | |
| ) | |
| if mailbox_id and maildomain_id: | |
| raise serializers.ValidationError( | |
| "Provide either mailbox_id or maildomain_id, not both." | |
| ) | |
| # Atomic update: if any of {html_body, text_body, raw_blob} is present, require all. | |
| atomic_fields = {"html_body", "text_body", "raw_blob"} | |
| present = atomic_fields.intersection(attrs.keys()) | |
| if present and present != atomic_fields: | |
| missing = ", ".join(sorted(atomic_fields - present)) | |
| raise serializers.ValidationError( | |
| f"html_body, text_body and raw_blob must be updated together. Missing: {missing}." | |
| ) | |
| # Also enforce at least one of html/text has content when updating bodies | |
| if "html_body" in attrs or "text_body" in attrs: | |
| html_body = attrs.get("html_body", "") | |
| text_body = attrs.get("text_body", "") | |
| # If both body fields are provided but empty, that's invalid | |
| if not html_body and not text_body: | |
| raise serializers.ValidationError( | |
| "At least one of html_body or text_body must be provided." | |
| ) | |
| return attrs |
🤖 Prompt for AI Agents
In src/backend/core/api/serializers.py around lines 993 to 1016, update
validation so that on creation mailbox_id and maildomain_id are mutually
exclusive (error if both provided) and to enforce atomic updates for content
fields: whenever any of html_body, text_body, or raw_blob is present in attrs
(on creation or update), require that all three keys are provided (and non-empty
if creation) and raise a ValidationError if one or more are missing; also remove
the previous partial-check logic that allowed updating html/text independently
so the three fields are treated as an atomic group.
src/backend/core/mda/outbound.py
Outdated
Comment on lines
42
to
115
| def _get_signatures(signature_id: str, user: models.User) -> dict[str, str]: | ||
| """Render the user signature with the given user.""" | ||
| signature = models.MessageTemplate.objects.get(id=signature_id) | ||
|
|
||
| try: | ||
| rendered = signature.render_template(user) | ||
| return rendered | ||
| except (KeyError, ValueError, TypeError): | ||
| return None | ||
|
|
There was a problem hiding this comment.
Harden _get_signatures: handle DoesNotExist/ValueError and enforce sender access/type.
Also fix the return type to Optional and require SIGNATURE, is_active, and that the template belongs to the sender mailbox or its domain.
Apply this diff:
-from core.enums import MessageDeliveryStatusChoices
+from core.enums import MessageDeliveryStatusChoices, MessageTemplateTypeChoices
@@
-def _get_signatures(signature_id: str, user: models.User) -> dict[str, str]:
- """Render the user signature with the given user."""
- signature = models.MessageTemplate.objects.get(id=signature_id)
-
- try:
- rendered = signature.render_template(user)
- return rendered
- except (KeyError, ValueError, TypeError):
- return None
+def _get_signatures(
+ signature_id: str, user: models.User, sender_mailbox: models.Mailbox
+) -> Optional[dict[str, str]]:
+ """Render the user signature if accessible for the sender."""
+ try:
+ signature = models.MessageTemplate.objects.get(id=signature_id)
+ except (models.MessageTemplate.DoesNotExist, ValueError, TypeError):
+ return None
+
+ # Enforce type and visibility for the sender context
+ if (
+ signature.type != MessageTemplateTypeChoices.SIGNATURE
+ or not signature.is_active
+ or not (
+ (signature.mailbox and signature.mailbox_id == sender_mailbox.id)
+ or (signature.maildomain and signature.maildomain_id == sender_mailbox.domain_id)
+ )
+ ):
+ return None
+
+ try:
+ return signature.render_template(user)
+ except (KeyError, ValueError, TypeError):
+ return NoneCommittable suggestion skipped: line range outside the PR's diff.
src/backend/core/mda/outbound.py
Outdated
Comment on lines
53
to
212
| def _insert_signature( | ||
| text_body: str, html_body: str, user: models.User | ||
| ) -> tuple[str, str]: | ||
| """Replace the signature in the text and html bodies.""" | ||
| # Handle signature by replacing the `<SignatureID>` with the signature | ||
| match = None | ||
| if html_body: | ||
| match = re.search(r"<p><SignatureID>(.*?)</SignatureID></p>", html_body) | ||
| elif text_body: | ||
| match = re.search(r"\n<SignatureID>(.*?)</SignatureID>\n", text_body) | ||
|
|
||
| if match: | ||
| signature_id = match.group(1) | ||
| signatures = _get_signatures(signature_id, user) | ||
| if signatures: | ||
| # replace placeholder with signature | ||
| if text_body: | ||
| text_body = text_body.replace( | ||
| "<SignatureID>" + signature_id + "</SignatureID>", | ||
| signatures["text_body"], | ||
| ) | ||
| if html_body: | ||
| html_body = html_body.replace( | ||
| "<SignatureID>" + signature_id + "</SignatureID>", | ||
| signatures["html_body"], | ||
| ) | ||
| return text_body, html_body |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace all placeholders in both bodies; avoid HTML-only, first-match-only behavior; pass sender for access checks.
Current logic misses text placeholders when HTML exists, handles only one match, and lacks sender scoping.
Apply this diff:
-def _insert_signature(
- text_body: str, html_body: str, user: models.User
-) -> tuple[str, str]:
- """Replace the signature in the text and html bodies."""
- # Handle signature by replacing the `<SignatureID>` with the signature
- match = None
- if html_body:
- match = re.search(r"<p><SignatureID>(.*?)</SignatureID></p>", html_body)
- elif text_body:
- match = re.search(r"\n<SignatureID>(.*?)</SignatureID>\n", text_body)
-
- if match:
- signature_id = match.group(1)
- signatures = _get_signatures(signature_id, user)
- if signatures:
- # replace placeholder with signature
- if text_body:
- text_body = text_body.replace(
- "<SignatureID>" + signature_id + "</SignatureID>",
- signatures["text_body"],
- )
- if html_body:
- html_body = html_body.replace(
- "<SignatureID>" + signature_id + "</SignatureID>",
- signatures["html_body"],
- )
- return text_body, html_body
+def _insert_signature(
+ text_body: str, html_body: str, user: models.User, sender_mailbox: models.Mailbox
+) -> tuple[str, str]:
+ """Replace signature placeholders in both bodies (supports multiple occurrences)."""
+ html = html_body or ""
+ text = text_body or ""
+
+ html_pattern = re.compile(
+ r"(?:<p[^>]*>\s*)?<SignatureID>(?P<ID>[^<]+)</SignatureID>(?:\s*</p>)?",
+ re.IGNORECASE,
+ )
+ text_pattern = re.compile(r"<SignatureID>(?P<ID>[^<]+)</SignatureID>", re.IGNORECASE)
+
+ def _replace_all(pattern, content, is_html: bool):
+ for m in list(pattern.finditer(content)):
+ sig = _get_signatures(m.group("ID"), user, sender_mailbox)
+ if sig:
+ replacement = sig["html_body"] if is_html else sig["text_body"]
+ content = content.replace(m.group(0), replacement)
+ return content
+
+ if html:
+ html = _replace_all(html_pattern, html, is_html=True)
+ if text:
+ text = _replace_all(text_pattern, text, is_html=False)
+
+ return (text if text_body is not None else None), (html if html_body is not None else None)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/backend/core/mda/outbound.py around lines 53 to 79, the function only
searches HTML (or text) and only replaces the first match and calls
_get_signatures without the sender, causing missed text placeholders,
single-match behavior, and missing access checks; change it to scan both
html_body and text_body for all occurrences of "<SignatureID>...</SignatureID>"
(use re.findall or re.finditer), collect unique signature_ids, call
_get_signatures(signature_id, user, sender) (or add sender param) for each id
once, and then replace every placeholder occurrence in both text_body and
html_body with the corresponding signatures["text_body"] /
signatures["html_body"] so all placeholders are replaced and signature retrieval
is scoped to the sender.
0a93c11 to
037deb2
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/core/mda/outbound.py (1)
15-16: Import template type enum to enforce SIGNATURE type.-from core.enums import MessageDeliveryStatusChoices +from core.enums import MessageDeliveryStatusChoices, MessageTemplateTypeChoices
♻️ Duplicate comments (14)
src/frontend/src/features/i18n/translations.json (2)
170-172: Add missing composer toolbar i18n keys (use in toolbar/components).Extend message_composer with variables/loading/signatures/signature_item so UI can drop hard-coded strings. Mirrors prior feedback.
"message_composer": { - "start_typing": "Start typing..." + "start_typing": "Start typing...", + "variables": "Variables", + "loading_variables": "Loading variables...", + "signatures": "Signatures", + "signature_item": "Signature: {{name}}" },
675-677: Mirror EN composer additions in FR locale.Keep parity for toolbar strings. Mirrors prior feedback.
"message_composer": { - "start_typing": "Commencer à écrire..." + "start_typing": "Commencer à écrire...", + "variables": "Variables", + "loading_variables": "Chargement des variables...", + "signatures": "Signatures", + "signature_item": "Signature : {{name}}" },src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx (5)
51-53: Remove non-null assertion; guard when no domain is selected.Avoid runtime crash. Render a friendly placeholder. Mirrors prior feedback.
- <SignatureComposeForm domain={selectedMailDomain!} defaultValue={signature} onSuccess={handleSuccess} /> + {selectedMailDomain ? ( + <SignatureComposeForm domain={selectedMailDomain} defaultValue={signature} onSuccess={handleSuccess} /> + ) : ( + <p className="c__modal__text--centered"> + {t('admin_maildomains_signature.compose_modal.no_domain_selected')} + </p> + )}
64-71: Zod: use “message”, not “error”, for custom messages.Otherwise errors won’t surface. Mirrors prior feedback.
- name: z.string().min(1, { error: "admin_maildomains_signature.compose_modal.form.errors.name" }), + name: z.string().min(1, { message: "admin_maildomains_signature.compose_modal.form.errors.name" }), @@ - htmlBody: z.string().min(1, { error: "admin_maildomains_signature.compose_modal.form.errors.content" }), - textBody: z.string().min(1, { error: "admin_maildomains_signature.compose_modal.form.errors.content" }), - rawBody: z.string().min(1, { error: "admin_maildomains_signature.compose_modal.form.errors.content" }), + htmlBody: z.string().min(1, { message: "admin_maildomains_signature.compose_modal.form.errors.content" }), + textBody: z.string().min(1, { message: "admin_maildomains_signature.compose_modal.form.errors.content" }), + rawBody: z.string().min(1, { message: "admin_maildomains_signature.compose_modal.form.errors.content" }),
75-91: When inactive, clear and disable “Forced”.Keep state consistent and meet checklist. Mirrors prior feedback.
const SignatureComposeForm = ({ domain, defaultValue, onSuccess }: SignatureComposerFormProps) => { const { t } = useTranslation(); const form = useForm<SignatureComposerFormData>({ @@ }); + const isActive = form.watch("is_active"); + useEffect(() => { + if (!isActive) { + form.setValue("is_forced", false, { shouldDirty: true }); + } + }, [isActive]);Additional import required at top of file:
import { useEffect } from "react";
92-126: Call onSuccess only on success; show toast on failure; coerce is_forced when inactive.Prevents false positives and bad UX.
- const onSubmit = async (data: SignatureComposerFormData) => { - try { + const onSubmit = async (data: SignatureComposerFormData) => { + try { + const payload = { + maildomain_id: domain.id, + name: data.name, + type: MessageTemplateTypeChoices.signature, + is_active: data.is_active, + is_forced: data.is_active ? data.is_forced : false, + html_body: data.htmlBody, + text_body: data.textBody, + raw_blob: data.rawBody, + }; if (defaultValue?.id) { await updateSignature({ - id: defaultValue.id, - data: { - maildomain_id: domain.id, - name: data.name, - type: MessageTemplateTypeChoices.signature, - is_active: data.is_active, - is_forced: data.is_forced, - html_body: data.htmlBody, - text_body: data.textBody, - raw_blob: data.rawBody, - } + id: defaultValue.id, + data: payload }); } else { await createSignature({ - data: { - maildomain_id: domain.id, - name: data.name, - type: MessageTemplateTypeChoices.signature, - is_active: data.is_active, - is_forced: data.is_forced, - html_body: data.htmlBody, - text_body: data.textBody, - raw_blob: data.rawBody, - } + data: payload }); } - } catch (error) { - console.error(error); - } - onSuccess?.(); + onSuccess?.(); + } catch (error) { + console.error(error); + addToast( + <ToasterItem type="error"> + <span>{t("api.error.unexpected")}</span> + </ToasterItem> + ); + } }
153-158: Disable “Forced” checkbox when inactive.Prevents invalid state toggling. Mirrors prior feedback.
<RhfCheckbox label={t('admin_maildomains_signature.compose_modal.form.labels.is_forced')} name="is_forced" text={t('admin_maildomains_signature.compose_modal.form.helper_text.is_forced')} + disabled={!form.watch('is_active')} fullWidth />src/frontend/src/features/forms/components/message-composer/index.tsx (5)
43-46: Fix docs: field names and update timing.Comment should reflect messageHtmlBody/messageTextBody/messageDraftBody and onChange updates. Mirrors prior feedback.
- * 3 hidden inputs (`htmlBody`, `textBody` and `draftBody`) are rendered to store - * the HTML, text and raw content of the message. Their values are updated - * when the editor is blurred. Those inputs must be used in the parent form + * 3 hidden inputs (`messageHtmlBody`, `messageTextBody` and `messageDraftBody`) are rendered to store + * the HTML, text and raw content of the message. Their values are updated + * on every content change and on mount. Those inputs must be used in the parent form * to retrieve all the content of the message.
31-37: Expose quotedMode prop (don’t hard-code “forward”).Allows correct “reply” vs “forward” rendering. Mirrors prior feedback.
type MessageComposerProps = FieldProps & { mailboxId?: string; blockNoteOptions?: Partial<MessageComposerBlockNoteSchema> defaultValue?: string; quotedMessage?: Message; disabled?: boolean; + quotedMode?: "reply" | "forward"; } -export const MessageComposer = ({ mailboxId, blockNoteOptions, defaultValue, quotedMessage, disabled = false, ...props }: MessageComposerProps) => { +export const MessageComposer = ({ mailboxId, blockNoteOptions, defaultValue, quotedMessage, quotedMode = "reply", disabled = false, ...props }: MessageComposerProps) => { @@ return initialContent.concat([{ type: "quoted-message", content: undefined, props: { - mode: "forward", + mode: quotedMode, messageId: quotedMessage.id,Also applies to: 48-49, 78-83
67-71: Guard JSON.parse on defaultValue.Prevents crashes on malformed JSON. Mirrors prior feedback.
- const initialContent = defaultValue - ? JSON.parse(defaultValue) - : [{ type: "paragraph", content: "" }]; + const initialContent = (() => { + if (!defaultValue) return [{ type: "paragraph", content: "" }]; + try { + return JSON.parse(defaultValue); + } catch { + return [{ type: "paragraph", content: "" }]; + } + })();
123-146: Enforce forced signature and fix effect deps.Replace any non-matching signature when a forced one exists; re-run when templates/mailbox change. Mirrors prior feedback.
useEffect(() => { const signatureBlock = editor.getBlock('signature'); - const defaultSignatureTemplateId = activeSignatures.find(signature => signature.is_forced)?.id - if (defaultSignatureTemplateId && !(signatureBlock?.props as BlockSignatureConfigProps | undefined)?.templateId) { - if (!signatureBlock) { + const defaultSignatureTemplateId = activeSignatures.find(signature => signature.is_forced)?.id; + const currentTemplateId = (signatureBlock?.props as BlockSignatureConfigProps | undefined)?.templateId; + if (defaultSignatureTemplateId && currentTemplateId !== defaultSignatureTemplateId) { + if (!signatureBlock) { editor.insertBlocks( [{ id: "signature", type: "signature", props: { templateId: defaultSignatureTemplateId, mailboxId: mailboxId } }], editor.document[0].id, "after" ); - } - else { + } else { editor.replaceBlocks( ["signature"], [{ id: "signature", type: "signature", props: { templateId: defaultSignatureTemplateId, mailboxId: mailboxId } }], ); } } // In case there is a signature block but the templateId does not match an active signature, we remove the block. if (signatureBlock) { const isSignatureActive = activeSignatures.findIndex(signature => signature.id === (signatureBlock.props as BlockSignatureConfigProps).templateId); if (isSignatureActive === -1) editor.removeBlocks(["signature"]); } - }, [isLoadingSignatures]); + }, [activeSignatures, mailboxId]);
104-110: Preserve signature block export: use BlockNote HTML exporter.Markdown→HTML drops custom toExternalHTML (…), breaking server-side replacement. Use BlockNote HTML export and mark fields dirty.
- const markdown = await editor.blocksToMarkdownLossy(editor.document); - const html = await MailHelper.markdownToHtml(markdown); + const markdown = await editor.blocksToMarkdownLossy(editor.document); + // Preserve custom blocks' toExternalHTML (e.g., signature) + const html = await editor.blocksToFullHTML(editor.document); form.setValue("messageDraftBody", JSON.stringify(editor.document), { shouldDirty: true }); - form.setValue("messageTextBody", markdown); - form.setValue("messageHtmlBody", html); + form.setValue("messageTextBody", markdown, { shouldDirty: true }); + form.setValue("messageHtmlBody", html, { shouldDirty: true });src/backend/core/mda/outbound.py (2)
117-212: Replace-first-only and HTML-priority cause broken text parts; scan/replace all in both bodies.Also thread sender_mailbox to access check. This fixes placeholders left in text/plain and supports multiple occurrences.
-def _insert_signature( - text_body: str, html_body: str, user: models.User -) -> tuple[str, str]: - """Replace the signature in the text and html bodies. - - This function is hardened to: - 1. Validate signature IDs before processing - 2. Check user access permissions to signature templates - 3. Use safe string replacement methods - 4. Log security-relevant events - """ - # Handle signature by replacing the `<SignatureID>` with the signature - match = None - if html_body: - match = re.search(r"<p><SignatureID>(.*?)</SignatureID></p>", html_body) - elif text_body: - match = re.search(r"\n<SignatureID>(.*?)</SignatureID>\n", text_body) - - if match: - signature_id = match.group(1) - - # Additional validation: ensure signature_id contains only safe characters - if not _is_valid_uuid(signature_id): - logger.warning( - "User %s attempted to use malformed signature ID: %s", - user.id, - signature_id, - ) - # Remove invalid signature placeholders for security - if text_body: - text_body = re.sub( - r"\n<SignatureID>" + re.escape(signature_id) + r"</SignatureID>\n", - "\n", - text_body, - count=1, - ) - if html_body: - html_body = re.sub( - r"<p><SignatureID>" - + re.escape(signature_id) - + r"</SignatureID></p>", - "", - html_body, - count=1, - ) - return text_body, html_body - - signatures = _get_signatures(signature_id, user) - if signatures: - # Use regex-based replacement to ensure we only replace the exact matched pattern - # This prevents injection attacks by only replacing the specific SignatureID tag we found - if text_body: - text_body = re.sub( - r"\n<SignatureID>" + re.escape(signature_id) + r"</SignatureID>\n", - "\n" + signatures["text_body"] + "\n", - text_body, - count=1, # Only replace the first occurrence - ) - if html_body: - html_body = re.sub( - r"<p><SignatureID>" - + re.escape(signature_id) - + r"</SignatureID></p>", - "<p>" + signatures["html_body"] + "</p>", - html_body, - count=1, # Only replace the first occurrence - ) - - logger.debug( - "Successfully inserted signature %s for user %s", signature_id, user.id - ) - else: - # Remove invalid signature placeholders for security - if text_body: - text_body = re.sub( - r"\n<SignatureID>" + re.escape(signature_id) + r"</SignatureID>\n", - "\n", - text_body, - count=1, - ) - if html_body: - html_body = re.sub( - r"<p><SignatureID>" - + re.escape(signature_id) - + r"</SignatureID></p>", - "", - html_body, - count=1, - ) - logger.warning( - "Failed to get signature %s for user %s, placeholder removed", - signature_id, - user.id, - ) - - return text_body, html_body +def _insert_signature( + text_body: str, html_body: str, user: models.User, sender_mailbox: models.Mailbox +) -> tuple[str, str]: + """Replace signature placeholders in both bodies (supports multiple occurrences).""" + html = html_body or "" + text = text_body or "" + + # Allow optional <p> wrapper and whitespace + html_pattern = re.compile(r"(?:<p[^>]*>\s*)?<SignatureID>(?P<ID>[^<]+)</SignatureID>(?:\s*</p>)?", re.IGNORECASE) + text_pattern = re.compile(r"<SignatureID>(?P<ID>[^<]+)</SignatureID>", re.IGNORECASE) + + def _replace_all(pattern: re.Pattern, content: str, is_html: bool) -> str: + for m in list(pattern.finditer(content)): + sig_id = m.group("ID") + if not _is_valid_uuid(sig_id): + # strip invalid placeholder + content = content.replace(m.group(0), "" if is_html else "\n") + continue + sig = _get_signatures(sig_id, user, sender_mailbox) + if sig and (sig.get("html_body") if is_html else sig.get("text_body")) is not None: + replacement = sig["html_body"] if is_html else sig["text_body"] + content = content.replace(m.group(0), replacement) + else: + # remove placeholder if not accessible/not found + content = content.replace(m.group(0), "" if is_html else "\n") + return content + + if html: + html = _replace_all(html_pattern, html, is_html=True) + if text: + text = _replace_all(text_pattern, text, is_html=False) + + return text if text_body is not None else None, html if html_body is not None else None
52-115: Harden _get_signatures: enforce type, sender scoping, and keep Optional return.Limit to active SIGNATURE templates scoped to the sender’s mailbox/domain to prevent cross-tenant misuse.
-def _get_signatures(signature_id: str, user: models.User) -> Optional[dict[str, str]]: +def _get_signatures(signature_id: str, user: models.User, sender_mailbox: models.Mailbox) -> Optional[dict[str, str]]: @@ - signature = models.MessageTemplate.objects.get(id=signature_id) + signature = models.MessageTemplate.objects.get(id=signature_id) @@ - # Check if user has access to this signature template - if not signature.is_active: + # Check type and active + if not signature.is_active or signature.type != MessageTemplateTypeChoices.SIGNATURE: logger.warning( "User %s attempted to access inactive signature %s", user.id, signature_id, ) return None @@ - if signature.mailbox: - # Check if user has access to the mailbox - if signature.mailbox.accesses.filter(user=user).exists(): - user_has_access = True - elif signature.maildomain: - # Check if user has access to the maildomain - if signature.maildomain.accesses.filter(user=user).exists(): - user_has_access = True - # Also check if user has access through mailbox access to this domain - elif signature.maildomain.mailbox_set.filter(accesses__user=user).exists(): - user_has_access = True + if signature.mailbox: + user_has_access = ( + signature.mailbox_id == sender_mailbox.id + and signature.mailbox.accesses.filter(user=user).exists() + ) + elif signature.maildomain: + user_has_access = ( + signature.maildomain_id == sender_mailbox.domain_id + and ( + signature.maildomain.accesses.filter(user=user).exists() + or signature.maildomain.mailbox_set.filter(accesses__user=user).exists() + ) + )
🧹 Nitpick comments (19)
src/frontend/src/features/i18n/translations.json (2)
458-500: Add missing error toast key and “no domain selected” placeholder.Needed by modal guard/error handling.
"compose_modal": { - "title": "Create a new signature for {{domain}}", + "title": "Create a new signature for {{domain}}", + "no_domain_selected": "No domain selected.", @@ "toasts": { "success_create": "Signature created!", "success_update": "Signature updated!", - "delete": "Signature deleted!" + "delete": "Signature deleted!", + "error_save": "Failed to save signature." }
958-1000: Add FR keys for modal guard and error toast.Match EN additions.
"compose_modal": { - "title": "Créer une nouvelle signature pour {{domain}}", + "title": "Créer une nouvelle signature pour {{domain}}", + "no_domain_selected": "Aucun domaine sélectionné.", @@ "toasts": { "success_create": "Signature créée !", "success_update": "Signature mise à jour !", - "delete": "Signature supprimée !" + "delete": "Signature supprimée !", + "error_save": "Échec de l’enregistrement de la signature." }src/backend/core/migrations/0006_blob_maildomain_alter_blob_mailbox_and_more.py (1)
39-41: Fix mismatched help_text for type and tighten wording for is_forcedThe help_text lists “forward, auto reply” which aren’t in MessageTemplateTypeChoices; also clarify is_forced wording.
- ('type', models.SmallIntegerField(choices=[(1, 'reply'), (2, 'new_message'), (3, 'signature')], default=1, help_text='Type of template (reply, forward, new message, auto reply, signature)', verbose_name='type')), + ('type', models.SmallIntegerField(choices=[(1, 'reply'), (2, 'new_message'), (3, 'signature')], default=1, help_text='Type of template (reply, new_message, signature)', verbose_name='type')), - ('is_forced', models.BooleanField(default=False, help_text='Whether this template is forced no other same type template can be used', verbose_name='is forced')), + ('is_forced', models.BooleanField(default=False, help_text='If true, this is the enforced template for its type within the same mailbox or maildomain.', verbose_name='is forced')),src/backend/core/tests/api/test_message_template.py (3)
634-650: Consider asserting HTML-escaping behavior on renderOnce we escape injected HTML in html_body (see model comment), add a test that custom_attributes containing HTML are escaped in html_body but not altered semantically in text_body.
579-590: Guard against brittle error-text assertionsIf you rely on exact messages, prefer
.startswith/inchecks (you already useinelsewhere). Looks good here; just a heads-up to keep them resilient.
172-190: Order-dependent assertionsRelying on list ordering by name is OK but fragile if ordering changes; consider asserting sets or explicitly ordering by name in the API when
mailbox_idis passed.src/backend/core/models.py (2)
1491-1498: Align help_text with enum valuesRemove “forward, auto reply” from help_text.
- help_text=_( - "Type of template (reply, forward, new message, auto reply, signature)" - ), + help_text=_("Type of template (reply, new_message, signature)"),
1568-1586: Return CRUD-style abilities for consistencyOther models return a CRUDAbilities dict; this one returns a list of UserAbilities. Consider aligning for frontend consistency.
- def get_abilities(self, user): - """Get abilities for this template based on user permissions.""" - abilities = [] - if ( - user.is_superuser - or (self.mailbox and self.mailbox.accesses.filter(user=user).exists()) - or (self.maildomain and self.maildomain.accesses.filter(user=user).exists()) - ): - abilities.extend( - [ - UserAbilities.CAN_VIEW_DOMAIN_ADMIN, - UserAbilities.CAN_CREATE_MAILDOMAINS, - ] - ) - return abilities + def get_abilities(self, user): + """Return CRUD-like abilities consistent with other models.""" + can_access = bool( + user + and getattr(user, "is_authenticated", False) + and ( + user.is_superuser + or (self.mailbox and self.mailbox.accesses.filter(user=user).exists()) + or (self.maildomain and self.maildomain.accesses.filter(user=user).exists()) + ) + ) + is_admin = bool(user and user.is_superuser) + return { + CRUDAbilities.CAN_READ: can_access, + CRUDAbilities.CAN_CREATE: is_admin, + CRUDAbilities.CAN_UPDATE: is_admin or can_access, + CRUDAbilities.CAN_PARTIALLY_UPDATE: is_admin or can_access, + CRUDAbilities.CAN_DELETE: is_admin, + }src/backend/core/tests/mda/test_outbound.py (11)
64-99: Make SMTP payload assertion robust (bytes vs str).Guard against smtplib returning str vs bytes to avoid type errors.
- assert call_args[2].endswith(draft_message.blob.get_content()) + payload = call_args[2] if isinstance(call_args[2], (bytes, bytearray)) else call_args[2].encode() + assert payload.endswith(draft_message.blob.get_content())
205-209: Decode MIME instead of str(bytes) to avoid brittle assertions.Using str(bytes) relies on escapes and may break with encodings; decode once and assert on text.
- message = models.Message.objects.get(id=message.id) - assert ( - "Best regards,<br>John Doe<br>Software Engineer<br>Engineering</p>" - in str(message.blob.get_content()) - ) + message.refresh_from_db() + content = message.blob.get_content().decode("utf-8", "replace") + assert "Best regards,<br>John Doe<br>Software Engineer<br>Engineering</p>" in content
224-227: Normalize newlines; assert on decoded text.Quoted-printable/Base64 or CRLF may make the current assertion flaky.
- message = models.Message.objects.get(id=message.id) - assert "Best regards,\\nJohn Doe\\nSoftware Engineer\\nEngineering\\n" in str( - message.blob.get_content() - ) + message.refresh_from_db() + content = message.blob.get_content().decode("utf-8", "replace") + assert "Best regards,\nJohn Doe\nSoftware Engineer\nEngineering\n" in content
242-245: Verify both text and HTML parts; avoid str(bytes).- message = models.Message.objects.get(id=message.id) - assert "Best regards,\\nJohn Doe\\nSoftware Engineer\\nEngineering\\n" in str( - message.blob.get_content() - ) + message.refresh_from_db() + content = message.blob.get_content().decode("utf-8", "replace") + assert "Best regards,\nJohn Doe\nSoftware Engineer\nEngineering\n" in content + assert "Best regards,<br>John Doe<br>Software Engineer<br>Engineering</p>" in content
277-283: Reply + signature: decode and assert on text (and optionally HTML).- message = models.Message.objects.get(id=message.id) - assert "Best regards,\\nJohn Doe\\nSoftware Engineer\\nEngineering\\n" in str( - message.blob.get_content() - ) + message.refresh_from_db() + content = message.blob.get_content().decode("utf-8", "replace") + assert "Best regards,\nJohn Doe\nSoftware Engineer\nEngineering\n" in content
295-303: Also assert content is preserved; decode before checks.Confirm placeholder removal didn’t drop user content.
- result = outbound.prepare_outbound_message( + result = outbound.prepare_outbound_message( mailbox_sender, message, text_body, html_body, user ) assert result is True - # The signature placeholder should be removed for security - message = models.Message.objects.get(id=message.id) - assert "invalid-uuid-format" not in str(message.blob.get_content()) + # The signature placeholder should be removed for security + message.refresh_from_db() + content = message.blob.get_content().decode("utf-8", "replace") + assert "invalid-uuid-format" not in content + assert "Hello world!" in content
317-326: Fix contradictory comment; decode and drop redundant str().Comment says “remain unchanged” but test expects stripping. Align the comment and simplify.
- # The malicious content should remain unchanged since it's not a valid UUID + # The SignatureID tag must be stripped because the ID is not a valid UUID (defense-in-depth) - message = models.Message.objects.get(id=message.id) - content = str(message.blob.get_content()) + message.refresh_from_db() + content = message.blob.get_content().decode("utf-8", "replace") # The SignatureID tag should be removed because the ID is not a valid UUID # Note: In MIME content, quotes are escaped, so we need to look for the escaped version assert ( "<SignatureID><script>alert(\\'xss\\')</script></SignatureID>" - not in str(content) + not in content )
178-183: Use consistent enum source (enums vs models).Elsewhere in this file you use enums.ThreadAccessRoleChoices; keep it consistent.
- role=models.ThreadAccessRoleChoices.EDITOR, + role=enums.ThreadAccessRoleChoices.EDITOR,
388-419: Clarify expectation or enable full substitution for this test.Docstring says “inserted before reply” (good), and comment says “actual template values” but the assertion keeps {job_title}/{department}. Either:
- Keep partial substitution and update the assertion message, or
- Enable full substitution via override_settings and assert final values.
+ @override_settings(SCHEMA_CUSTOM_ATTRIBUTES_USER=SCHEMA_CUSTOM_ATTRIBUTES) def test_signature_insertion_before_reply_processing( @@ - assert result is True - message = models.Message.objects.get(id=message.id) - # Verify signature was inserted (with actual template values) - assert "Best regards,\\nJohn Doe\\n{job_title}\\n{department}" in str( - message.blob.get_content() - ) + assert result is True + message.refresh_from_db() + content = message.blob.get_content().decode("utf-8", "replace") + # Verify signature was inserted (with actual template values) + assert "Best regards,\nJohn Doe\nSoftware Engineer\nEngineering" in content @@ - assert str(signature_template.id) not in str(message.blob.get_content()) + assert str(signature_template.id) not in content
383-386: Decode before assertion for consistency.- message = models.Message.objects.get(id=message.id) - assert str(inactive_signature.id) not in str(message.blob.get_content()) + message.refresh_from_db() + content = message.blob.get_content().decode("utf-8", "replace") + assert str(inactive_signature.id) not in content
435-441: Decode before assertion; keep content check stable.- message = models.Message.objects.get(id=message.id) - content = str(message.blob.get_content()) + message.refresh_from_db() + content = message.blob.get_content().decode("utf-8", "replace") @@ - assert "Hello world!" in content + assert "Hello world!" in content
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (5)
src/frontend/src/features/api/gen/message-templates/message-templates.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message_template.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message_template_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_message_template_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/read_only_message_template.tsis excluded by!**/gen/**
📒 Files selected for processing (16)
src/backend/core/admin.py(1 hunks)src/backend/core/api/openapi.json(4 hunks)src/backend/core/api/permissions.py(1 hunks)src/backend/core/api/serializers.py(2 hunks)src/backend/core/api/viewsets/message_template.py(1 hunks)src/backend/core/factories.py(1 hunks)src/backend/core/mda/outbound.py(3 hunks)src/backend/core/migrations/0006_blob_maildomain_alter_blob_mailbox_and_more.py(1 hunks)src/backend/core/models.py(6 hunks)src/backend/core/tests/api/test_message_template.py(1 hunks)src/backend/core/tests/api/test_send_message_signature.py(1 hunks)src/backend/core/tests/mda/test_outbound.py(2 hunks)src/frontend/src/features/forms/components/message-composer/index.tsx(1 hunks)src/frontend/src/features/i18n/translations.json(6 hunks)src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/signatures-view/signature-data-grid.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/backend/core/admin.py
- src/backend/core/api/viewsets/message_template.py
- src/backend/core/api/permissions.py
- src/frontend/src/features/layouts/components/admin/signatures-view/signature-data-grid.tsx
- src/backend/core/api/serializers.py
- src/backend/core/api/openapi.json
- src/backend/core/factories.py
- src/backend/core/tests/api/test_send_message_signature.py
🧰 Additional context used
🧬 Code graph analysis (6)
src/backend/core/tests/api/test_message_template.py (3)
src/backend/core/tests/api/test_send_message_signature.py (1)
fixture_user(34-42)src/backend/core/factories.py (6)
UserFactory(18-29)MailDomainFactory(49-55)MailboxFactory(58-94)MessageTemplateFactory(220-231)MailboxAccessFactory(97-107)MailDomainAccessFactory(110-120)src/backend/core/models.py (10)
MessageTemplate(1455-1628)Blob(1226-1310)get_content(1296-1310)delete(909-929)save(87-90)save(202-205)save(284-287)save(785-837)save(1290-1294)save(1541-1566)
src/frontend/src/features/forms/components/message-composer/index.tsx (6)
src/frontend/src/features/blocknote/signature-block/index.tsx (3)
BlockSignature(91-131)BlockSignatureConfigProps(132-132)SignatureTemplateSelector(19-86)src/frontend/src/features/blocknote/quoted-message-block/index.tsx (1)
QuotedMessageBlock(5-41)src/frontend/src/features/api/gen/message-templates/message-templates.ts (1)
useMessageTemplatesList(184-212)src/frontend/src/features/api/gen/models/message_template_type_choices.ts (2)
MessageTemplateTypeChoices(9-10)MessageTemplateTypeChoices(13-17)src/frontend/src/features/blocknote/blocknote-view-field/index.tsx (1)
BlockNoteViewField(11-27)src/frontend/src/features/blocknote/toolbar.tsx (1)
Toolbar(6-30)
src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx (8)
src/frontend/src/features/api/gen/models/read_only_message_template.ts (1)
ReadOnlyMessageTemplate(14-45)src/frontend/src/features/providers/admin-maildomain.tsx (1)
useAdminMailDomain(47-53)src/frontend/src/features/ui/components/toaster/index.tsx (2)
addToast(63-76)ToasterItem(16-61)src/frontend/src/features/api/gen/message-templates/message-templates.ts (2)
useMessageTemplatesCreate(304-324)useMessageTemplatesUpdate(612-632)src/frontend/src/features/api/gen/models/message_template_type_choices.ts (2)
MessageTemplateTypeChoices(9-10)MessageTemplateTypeChoices(13-17)src/frontend/src/features/forms/components/react-hook-form/rhf-input.tsx (1)
RhfInput(10-33)src/frontend/src/features/layouts/components/admin/modal-compose-signature/signature-composer.tsx (1)
SignatureComposer(37-110)src/frontend/src/features/forms/components/react-hook-form/rhf-checkbox.tsx (1)
RhfCheckbox(10-33)
src/backend/core/mda/outbound.py (1)
src/backend/core/models.py (5)
User(122-233)MessageTemplate(1455-1628)render_template(1587-1618)Mailbox(411-549)Message(1019-1155)
src/backend/core/models.py (5)
src/frontend/src/features/api/gen/models/message_template_type_choices.ts (2)
MessageTemplateTypeChoices(9-10)MessageTemplateTypeChoices(13-17)src/backend/core/enums.py (3)
MessageTemplateTypeChoices(108-113)CompressionTypeChoices(54-58)UserAbilities(75-79)src/backend/core/api/serializers.py (4)
create(877-879)create(1018-1043)update(881-883)update(1045-1092)src/frontend/src/features/api/gen/models/message_template.ts (1)
MessageTemplate(14-42)src/backend/core/api/viewsets/message_template.py (1)
render_template(188-208)
src/backend/core/tests/mda/test_outbound.py (5)
src/backend/core/tests/api/test_send_message_signature.py (1)
fixture_user(34-42)src/backend/core/factories.py (7)
UserFactory(18-29)MessageTemplateFactory(220-231)MailboxFactory(58-94)MailboxAccessFactory(97-107)ThreadFactory(123-130)ContactFactory(146-155)MessageFactory(158-184)src/backend/core/enums.py (2)
MessageTemplateTypeChoices(108-113)MailboxRoleChoices(15-21)src/backend/core/mda/outbound.py (1)
prepare_outbound_message(215-372)src/backend/core/models.py (8)
Message(1019-1155)get_content(1296-1310)save(87-90)save(202-205)save(284-287)save(785-837)save(1290-1294)save(1541-1566)
⏰ 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). (4)
- GitHub Check: build-front
- GitHub Check: check-api-state
- GitHub Check: lint-back
- GitHub Check: test-back
🔇 Additional comments (3)
src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx (1)
83-86: Verify raw_blob/text/html defaults align with API shapes.Confirm that defaultValue.raw_blob is the JSON document content, not a blob ID; otherwise the editor will crash on JSON.parse. I can help adjust serializer or mapping if needed.
src/backend/core/models.py (1)
486-491: Good reuse via BlobManagerNice consolidation to Blob.objects.create_blob with mailbox context.
src/backend/core/tests/mda/test_outbound.py (1)
158-165: Confirm required role; SENDER may be more appropriate.Preparing/signing for outbound often requires send permission. If so, set SENDER here.
- role=models.MailboxRoleChoices.EDITOR, + role=models.MailboxRoleChoices.SENDER,If EDITOR is intentional for this path, ignore.
Comment on lines
215
to
221
| def prepare_outbound_message( | ||
| mailbox_sender: models.Mailbox, | ||
| message: models.Message, | ||
| text_body: str, | ||
| html_body: str, | ||
| user: models.User, | ||
| ) -> bool: |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify all call sites updated for new prepare_outbound_message signature.
Ensure send.py (and others) pass the user arg; after this change, also ensure _insert_signature sender_mailbox param is wired only here.
🏁 Script executed:
#!/bin/bash
# Find prepare_outbound_message calls and show surrounding context
rg -nPU -C3 '\bprepare_outbound_message\s*\(' --type=pyLength of output: 7388
Fix prepare_outbound_message call in send.py
src/backend/core/api/viewsets/send.py:131 – prepare_outbound_message now requires html_body and user; update the call to pass request.data.get("htmlBody") and request.user.
🤖 Prompt for AI Agents
In src/backend/core/mda/outbound.py around lines 215 to 221, the
prepare_outbound_message function signature now requires html_body and user but
the call in src/backend/core/api/viewsets/send.py at ~line 131 still passes only
text_body; update that call to pass request.data.get("htmlBody") as the
html_body argument and request.user as the user argument so the function
receives all required parameters.
src/backend/core/migrations/0006_blob_maildomain_alter_blob_mailbox_and_more.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/backend/core/api/serializers.py (1)
1065-1111: Update path drops relationship changes and can create ownerless blobs
- mailbox_id/maildomain_id are validated but never applied to instance.
- Creating a blob without owners (IDs not provided on update) violates Blob.save invariant and 500s.
@@ - # Use atomic transaction to ensure all content fields are updated together + # Apply relationship updates (assign objects) + try: + if mailbox_id is not None: + validated_data["mailbox"] = models.Mailbox.objects.get(id=mailbox_id) + if maildomain_id is not None: + validated_data["maildomain"] = models.MailDomain.objects.get(id=maildomain_id) + except models.Mailbox.DoesNotExist: + raise serializers.ValidationError({"mailbox_id": "Mailbox not found"}) + except models.MailDomain.DoesNotExist: + raise serializers.ValidationError({"maildomain_id": "Maildomain not found"}) + + # Use atomic transaction to ensure all content fields are updated together with transaction.atomic(): @@ - if raw_blob_content: # Only create blob if content is not empty - blob = models.Blob.objects.create_blob( - content=raw_blob_content.encode("utf-8"), - content_type="application/json", - mailbox_id=mailbox_id, - maildomain_id=maildomain_id, - ) + if raw_blob_content: # Only create blob if content is not empty + # Fallback to existing owners when not provided in payload + owner_mailbox_id = ( + mailbox_id if mailbox_id is not None else instance.mailbox_id + ) + owner_maildomain_id = ( + maildomain_id if maildomain_id is not None else instance.maildomain_id + ) + if not owner_mailbox_id and not owner_maildomain_id: + raise serializers.ValidationError( + "Cannot create raw_blob without mailbox_id or maildomain_id." + ) + blob = models.Blob.objects.create_blob( + content=raw_blob_content.encode("utf-8"), + content_type="application/json", + mailbox_id=owner_mailbox_id, + maildomain_id=owner_maildomain_id, + ) validated_data["raw_blob"] = blobsrc/backend/core/tests/api/test_message_template.py (3)
946-950: And here as wellUse MailDomainAccessRoleChoices for maildomain access.
- factories.MailDomainAccessFactory( - maildomain=maildomain, - user=user, - role=models.MailboxRoleChoices.VIEWER, - ) + factories.MailDomainAccessFactory( + maildomain=maildomain, + user=user, + role=models.MailDomainAccessRoleChoices.ADMIN, + )
398-404: Wrong enum for MailDomain access roleUse MailDomainAccessRoleChoices; MailboxRoleChoices is for mailbox accesses.
- factories.MailDomainAccessFactory( - maildomain=maildomain, - user=user, - role=models.MailboxRoleChoices.VIEWER, - ) + factories.MailDomainAccessFactory( + maildomain=maildomain, + user=user, + role=models.MailDomainAccessRoleChoices.ADMIN, + )
845-851: Same enum mix-up hereSwitch to the MailDomain access enum.
- factories.MailDomainAccessFactory( - maildomain=maildomain, - user=user, - role=models.MailboxRoleChoices.VIEWER, - ) + factories.MailDomainAccessFactory( + maildomain=maildomain, + user=user, + role=models.MailDomainAccessRoleChoices.ADMIN, + )
🧹 Nitpick comments (4)
src/backend/core/api/serializers.py (3)
955-960: Use UUIDField for IDs to get built-in validationCharField accepts anything; UUIDField will 400 on invalid UUIDs before hitting the DB.
- mailbox_id = serializers.CharField( + mailbox_id = serializers.UUIDField( required=False, write_only=True, help_text="Mailbox UUID" ) - maildomain_id = serializers.CharField( + maildomain_id = serializers.UUIDField( required=False, write_only=True, help_text="Maildomain UUID" )
928-931: Defensive decode of blob contentAvoid UnicodeDecodeError by using errors="replace".
- return obj.raw_blob.get_content().decode("utf-8") if obj.raw_blob else None + if not obj.raw_blob: + return None + return obj.raw_blob.get_content().decode("utf-8", errors="replace")
966-975: Same here for to_representation raw_blob decodeUse a safe decode to prevent unexpected 500s on non-UTF8 payloads.
- if instance.raw_blob: - data["raw_blob"] = instance.raw_blob.get_content().decode("utf-8") - else: - data["raw_blob"] = None + if instance.raw_blob: + data["raw_blob"] = instance.raw_blob.get_content().decode("utf-8", errors="replace") + else: + data["raw_blob"] = Nonesrc/backend/core/tests/api/test_message_template.py (1)
588-590: Avoid brittle error-structure assertionValidationError formatting can vary; assert against response.data string for stability.
- assert "Mailbox domain does not match maildomain." in response.json()[0] + assert "Mailbox domain does not match maildomain." in str(response.data)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/backend/core/api/serializers.py(2 hunks)src/backend/core/tests/api/test_message_template.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/backend/core/api/serializers.py (2)
src/backend/core/enums.py (1)
MessageTemplateTypeChoices(108-113)src/backend/core/models.py (25)
get_content(1296-1310)Meta(84-85)Meta(194-197)Meta(276-279)Meta(440-445)Meta(567-571)Meta(595-598)Meta(775-780)Meta(947-951)Meta(968-972)Meta(1009-1013)Meta(1068-1072)Meta(1281-1285)Meta(1342-1346)Meta(1382-1386)Meta(1436-1440)Meta(1532-1536)MessageTemplate(1455-1628)Mailbox(411-549)MailDomain(236-408)Blob(1226-1310)create_blob(465-491)create_blob(1161-1223)content_type(1352-1354)delete(909-929)
src/backend/core/tests/api/test_message_template.py (3)
src/backend/core/factories.py (6)
UserFactory(18-29)MailDomainFactory(49-55)MailboxFactory(58-94)MessageTemplateFactory(220-231)MailboxAccessFactory(97-107)MailDomainAccessFactory(110-120)src/backend/core/enums.py (3)
MessageTemplateTypeChoices(108-113)MailboxRoleChoices(15-21)MailDomainAccessRoleChoices(48-51)src/backend/core/models.py (9)
MessageTemplate(1455-1628)Blob(1226-1310)get_content(1296-1310)save(87-90)save(202-205)save(284-287)save(785-837)save(1290-1294)save(1541-1566)
⏰ 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). (3)
- GitHub Check: build-front
- GitHub Check: check-api-state
- GitHub Check: test-back
🔇 Additional comments (1)
src/backend/core/tests/api/test_message_template.py (1)
1-1350: Approve code changes: Tests are comprehensive and anrgscan confirms no enum mix-ups remain.
Comment on lines
+994
to
+1021
| def validate(self, attrs): | ||
| """Validate template data.""" | ||
| # exactly one maildomain_id or mailbox_id is required | ||
| mailbox_id = attrs.get("mailbox_id") | ||
| maildomain_id = attrs.get("maildomain_id") | ||
|
|
||
| if not self.instance: # Only for creation | ||
| if not mailbox_id and not maildomain_id: | ||
| raise serializers.ValidationError( | ||
| "Either mailbox_id or maildomain_id is required." | ||
| ) | ||
|
|
||
| # For creation or when updating content fields | ||
| if "html_body" in attrs or "text_body" in attrs or "raw_blob" in attrs: | ||
| html_body = attrs.get("html_body", "") | ||
| text_body = attrs.get("text_body", "") | ||
| raw_blob = attrs.get("raw_blob", "") | ||
|
|
||
| # If any content field is provided, all three must be provided together | ||
| # This ensures atomic updates and prevents inconsistent states | ||
| if html_body or text_body or raw_blob: | ||
| if not html_body or not text_body or not raw_blob: | ||
| raise serializers.ValidationError( | ||
| "When providing content fields, all three fields (html_body, text_body, raw_blob) " | ||
| "must be provided together to ensure consistency." | ||
| ) | ||
|
|
||
| return attrs |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Atomic content update check should be key-based (not truthiness) and message clarified
Right now, empty strings bypass the "all three" rule, and the comment says "exactly one" while code allows both mailbox_id and maildomain_id. Switch to key-presence validation and clarify the message.
- # exactly one maildomain_id or mailbox_id is required
+ # at least one of maildomain_id or mailbox_id is required
@@
- if not self.instance: # Only for creation
- if not mailbox_id and not maildomain_id:
- raise serializers.ValidationError(
- "Either mailbox_id or maildomain_id is required."
- )
+ if not self.instance: # Only for creation
+ if not mailbox_id and not maildomain_id:
+ raise serializers.ValidationError(
+ "At least one of mailbox_id or maildomain_id is required."
+ )
@@
- # For creation or when updating content fields
- if "html_body" in attrs or "text_body" in attrs or "raw_blob" in attrs:
- html_body = attrs.get("html_body", "")
- text_body = attrs.get("text_body", "")
- raw_blob = attrs.get("raw_blob", "")
-
- # If any content field is provided, all three must be provided together
- # This ensures atomic updates and prevents inconsistent states
- if html_body or text_body or raw_blob:
- if not html_body or not text_body or not raw_blob:
- raise serializers.ValidationError(
- "When providing content fields, all three fields (html_body, text_body, raw_blob) "
- "must be provided together to ensure consistency."
- )
+ # Atomic content update: if any of the keys is present, require all three keys.
+ atomic_fields = {"html_body", "text_body", "raw_blob"}
+ present = atomic_fields.intersection(attrs.keys())
+ if present and present != atomic_fields:
+ missing = ", ".join(sorted(atomic_fields - present))
+ raise serializers.ValidationError(
+ f"html_body, text_body and raw_blob must be updated together. Missing: {missing}."
+ )📝 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.
Suggested change
| def validate(self, attrs): | |
| """Validate template data.""" | |
| # exactly one maildomain_id or mailbox_id is required | |
| mailbox_id = attrs.get("mailbox_id") | |
| maildomain_id = attrs.get("maildomain_id") | |
| if not self.instance: # Only for creation | |
| if not mailbox_id and not maildomain_id: | |
| raise serializers.ValidationError( | |
| "Either mailbox_id or maildomain_id is required." | |
| ) | |
| # For creation or when updating content fields | |
| if "html_body" in attrs or "text_body" in attrs or "raw_blob" in attrs: | |
| html_body = attrs.get("html_body", "") | |
| text_body = attrs.get("text_body", "") | |
| raw_blob = attrs.get("raw_blob", "") | |
| # If any content field is provided, all three must be provided together | |
| # This ensures atomic updates and prevents inconsistent states | |
| if html_body or text_body or raw_blob: | |
| if not html_body or not text_body or not raw_blob: | |
| raise serializers.ValidationError( | |
| "When providing content fields, all three fields (html_body, text_body, raw_blob) " | |
| "must be provided together to ensure consistency." | |
| ) | |
| return attrs | |
| def validate(self, attrs): | |
| """Validate template data.""" | |
| # at least one of maildomain_id or mailbox_id is required | |
| mailbox_id = attrs.get("mailbox_id") | |
| maildomain_id = attrs.get("maildomain_id") | |
| if not self.instance: # Only for creation | |
| if not mailbox_id and not maildomain_id: | |
| raise serializers.ValidationError( | |
| "At least one of mailbox_id or maildomain_id is required." | |
| ) | |
| # Atomic content update: if any of the keys is present, require all three keys. | |
| atomic_fields = {"html_body", "text_body", "raw_blob"} | |
| present = atomic_fields.intersection(attrs.keys()) | |
| if present and present != atomic_fields: | |
| missing = ", ".join(sorted(atomic_fields - present)) | |
| raise serializers.ValidationError( | |
| f"html_body, text_body and raw_blob must be updated together. Missing: {missing}." | |
| ) | |
| return attrs |
🤖 Prompt for AI Agents
In src/backend/core/api/serializers.py around lines 994 to 1021, change the
validation so presence checks use key membership in attrs (e.g., "mailbox_id" in
attrs) instead of truthiness: enforce exactly one of mailbox_id or maildomain_id
is provided (XOR on key presence) and update the error message accordingly; for
content fields, detect if any of the keys "html_body", "text_body", or
"raw_blob" are present in attrs and, if so, require all three keys to be present
(not just non-empty values), and update the ValidationError message to clearly
state that all three keys must be provided together when any content key is
supplied.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/core/models.py (1)
1282-1287: Add DB-level ownership check on BlobMirror app-level validation with a CheckConstraint so orphan blobs can’t be inserted.
class Meta: db_table = "messages_blob" verbose_name = _("blob") verbose_name_plural = _("blobs") ordering = ["-created_at"] + constraints = [ + models.CheckConstraint( + check=(models.Q(mailbox__isnull=False) | models.Q(maildomain__isnull=False)), + name="blob_has_owner", + ), + ]
♻️ Duplicate comments (8)
src/backend/core/models.py (4)
1492-1500: Fix help_text to match enum valuesHelp text lists forward/auto reply which aren’t in MessageTemplateTypeChoices.
type = models.SmallIntegerField( _("type"), choices=MessageTemplateTypeChoices.choices, default=MessageTemplateTypeChoices.REPLY, help_text=_( - "Type of template (reply, forward, new message, auto reply, signature)" + "Type of template (reply, new_message, signature)" ), )
1589-1607: Make abilities shape consistent with other models (CRUD dict)Other models return CRUDAbilities dicts. Returning a list here is inconsistent and brittle for clients.
- def get_abilities(self, user): - """Get abilities for this template based on user permissions.""" - abilities = [] - - # Check if user has access to any of the mailboxes or maildomains - if ( - user.is_superuser - or (self.mailbox and self.mailbox.accesses.filter(user=user).exists()) - or (self.maildomain and self.maildomain.accesses.filter(user=user).exists()) - ): - abilities.extend( - [ - UserAbilities.CAN_VIEW_DOMAIN_ADMIN, - UserAbilities.CAN_CREATE_MAILDOMAINS, - ] - ) - - return abilities + def get_abilities(self, user): + """Return CRUD-like abilities consistent with other models.""" + can_access = False + if user and getattr(user, "is_authenticated", False): + if user.is_superuser: + can_access = True + else: + can_access = ( + (self.mailbox and self.mailbox.accesses.filter(user=user).exists()) + or (self.maildomain and self.maildomain.accesses.filter(user=user).exists()) + ) + is_admin = bool(user and user.is_superuser) + return { + CRUDAbilities.CAN_READ: can_access, + CRUDAbilities.CAN_CREATE: is_admin, + CRUDAbilities.CAN_UPDATE: is_admin or can_access, + CRUDAbilities.CAN_PARTIALLY_UPDATE: is_admin or can_access, + CRUDAbilities.CAN_DELETE: is_admin, + }
1608-1641: Good: HTML escaping on substitution prevents injectionEscaping only for HTML path while keeping text plain is the right security/usability balance.
1291-1296: Use ValidationError via clean(); avoid 500s and extra DB hitssave() raises ValueError and accesses self.mailbox/self.maildomain (can trigger queries). Move validation to clean() using *_id fields so DRF/admin return 400s; rely on BaseModel.save() to run full_clean().
- def save(self, *args, **kwargs): - # at least one of mailbox or maildomain must be set - if not self.mailbox and not self.maildomain: - raise ValueError("Blob must have at least a mailbox or maildomain") - super().save(*args, **kwargs) + def clean(self): + if not self.mailbox_id and not self.maildomain_id: + raise ValidationError({"__all__": "Blob must have at least a mailbox or maildomain"}) + super().clean()src/backend/core/api/openapi.json (4)
2770-2797: Add typed query params for documented filters.GET /message-templates/ documents filters but doesn’t declare query parameters. Add them for client-gen and validation.
"/api/v1.0/message-templates/": { "get": { "operationId": "message_templates_list", "description": "ViewSet for managing message templates and signatures.\n\nThis ViewSet provides endpoints for creating, reading, updating, and deleting\nmessage templates and signatures. Templates can be used for replies, new messages,\nand signatures.\n\nFiltering:\n- type: Filter by template type (reply, new_message, signature)\n- is_active: Filter by active status (true/false)\n- mailbox_id: Filter by mailbox UUID\n- maildomain_id: Filter by mail domain UUID\n- is_forced: Filter by forced status (true/false) - works with mailbox or maildomain", + "parameters": [ + { "in": "query", "name": "type", "schema": { "$ref": "#/components/schemas/MessageTemplateTypeChoices" }, "description": "Filter by template type." }, + { "in": "query", "name": "is_active", "schema": { "type": "boolean" }, "description": "Filter by active status." }, + { "in": "query", "name": "mailbox_id", "schema": { "type": "string", "format": "uuid" }, "description": "Filter by mailbox UUID." }, + { "in": "query", "name": "maildomain_id", "schema": { "type": "string", "format": "uuid" }, "description": "Filter by mail domain UUID." }, + { "in": "query", "name": "is_forced", "schema": { "type": "boolean" }, "description": "Filter by forced status." } + ],
5217-5288: raw_blob must be a Blob UUID; clarify is_forced wording.Align with Blob FK and semantics.
"raw_blob": { - "type": "string", - "description": "Raw blob content" + "type": "string", + "format": "uuid", + "description": "Blob ID containing the raw template content" }, "is_forced": { "type": "boolean", "default": false, - "description": "Set as forced template" + "description": "Force usage of this template within its scope (mailbox or maildomain)" },
5289-5341: Request schema: enforce trio consistency, UUID formats, and mailbox/maildomain exclusivity.Create/PUT must send html_body, text_body, raw_blob together; IDs should be UUID; scope must be one-of.
"MessageTemplateRequest": { "type": "object", "description": "Serialize message templates for POST/PUT/PATCH operations.", "properties": { "raw_blob": { - "type": "string", - "minLength": 1, - "description": "Raw blob content" + "type": "string", + "format": "uuid", + "minLength": 1, + "description": "Blob ID containing the raw template content" }, "mailbox_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Mailbox UUID" }, "maildomain_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Maildomain UUID" } }, - "required": [ - "name", - "type" - ] + "required": ["name","type","raw_blob","html_body","text_body"], + "oneOf": [ + { "required": ["mailbox_id"] }, + { "required": ["maildomain_id"] } + ] },
5591-5639: PATCH schema: all-or-none rule for html/text/raw + UUID formats.Ensure partial updates keep trio in sync; add UUID formats.
"PatchedMessageTemplateRequest": { "type": "object", "description": "Serialize message templates for POST/PUT/PATCH operations.", "properties": { "raw_blob": { - "type": "string", - "minLength": 1, - "description": "Raw blob content" + "type": "string", + "format": "uuid", + "minLength": 1, + "description": "Blob ID containing the raw template content" }, "mailbox_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Mailbox UUID" }, "maildomain_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Maildomain UUID" } - } + }, + "oneOf": [ + { "not": { "anyOf": [ { "required": ["raw_blob"] }, { "required": ["html_body"] }, { "required": ["text_body"] } ] } }, + { "required": ["raw_blob","html_body","text_body"] } + ] },
🧹 Nitpick comments (11)
src/backend/core/models.py (1)
1162-1225: Guard compression type to avoid AttributeError and invalid enum valuesEnsure compression is a CompressionTypeChoices instance before use (e.g., for .label) and fail fast on bad inputs.
def create_blob( @@ - # Calculate SHA256 hash of the original content + # Normalize/validate compression enum early + if not isinstance(compression, CompressionTypeChoices): + try: + compression = CompressionTypeChoices(compression) + except Exception as exc: + raise ValueError(f"Unsupported compression type: {compression}") from exc + + # Calculate SHA256 hash of the original content sha256_hash = hashlib.sha256(content).digest()src/backend/core/tests/api/test_message_template.py (3)
583-590: Harden assertion against DRF error shapeDRF typically returns {"non_field_errors": [...]}; indexing response.json()[0] can break. Assert on the serialized string or the non_field_errors key.
- assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "Mailbox domain does not match maildomain." in response.json()[0] + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "Mailbox domain does not match maildomain." in str(response.data)
786-816: Add an XSS-escape rendering testGiven escaping in render_template, add a case where user.full_name contains HTML to ensure it’s escaped in html_body and unchanged in text_body.
def test_render_template_escapes_html(user, mailbox): user.full_name = '<b>Alice & Co.</b>' user.save() factories.MailboxAccessFactory(mailbox=mailbox, user=user, role=models.MailboxRoleChoices.VIEWER) template = factories.MessageTemplateFactory( html_body="<p>{full_name}</p>", text_body="{full_name}", mailbox=mailbox ) client = APIClient(); client.force_authenticate(user=user) resp = client.get(reverse("message-templates-detail", kwargs={"pk": template.id}) + "render/", {"mailbox_id": str(mailbox.id)}) assert resp.status_code == status.HTTP_200_OK assert resp.data["html_body"] == "<p><b>Alice & Co.</b></p>" assert resp.data["text_body"] == "<b>Alice & Co.</b>"
124-190: Ordering assumptions rely on model Meta.orderingAssertions depend on alphabetical ordering by name; if API changes ordering, tests will flake. Consider asserting sets of names instead of positions.
src/backend/core/api/openapi.json (7)
2839-2872: Clean up description (filters irrelevant on item GET).Remove the “Filtering” block from retrieve; it applies to list only.
- "description": "ViewSet for managing message templates and signatures.\n\nThis ViewSet provides endpoints for creating, reading, updating, and deleting\nmessage templates and signatures. Templates can be used for replies, new messages,\nand signatures.\n\nFiltering:\n- type: Filter by template type (reply, new_message, signature)\n- is_active: Filter by active status (true/false)\n- mailbox_id: Filter by mailbox UUID\n- maildomain_id: Filter by mail domain UUID\n- is_forced: Filter by forced status (true/false) - works with mailbox or maildomain", + "description": "Retrieve a specific message template or signature.",
2873-2921: Clean up description (filters irrelevant on PUT).Same issue on update.
- "description": "ViewSet for managing message templates and signatures.\n\nThis ViewSet provides endpoints for creating, reading, updating, and deleting\nmessage templates and signatures. Templates can be used for replies, new messages,\nand signatures.\n\nFiltering:\n- type: Filter by template type (reply, new_message, signature)\n- is_active: Filter by active status (true/false)\n- mailbox_id: Filter by mailbox UUID\n- maildomain_id: Filter by mail domain UUID\n- is_forced: Filter by forced status (true/false) - works with mailbox or maildomain", + "description": "Replace a message template or signature.",
2922-2969: Clean up description (filters irrelevant on PATCH).- "description": "ViewSet for managing message templates and signatures.\n\nThis ViewSet provides endpoints for creating, reading, updating, and deleting\nmessage templates and signatures. Templates can be used for replies, new messages,\nand signatures.\n\nFiltering:\n- type: Filter by template type (reply, new_message, signature)\n- is_active: Filter by active status (true/false)\n- mailbox_id: Filter by mailbox UUID\n- maildomain_id: Filter by mail domain UUID\n- is_forced: Filter by forced status (true/false) - works with mailbox or maildomain", + "description": "Partially update a message template or signature.",
2970-2997: Clean up description (filters irrelevant on DELETE).- "description": "ViewSet for managing message templates and signatures.\n\nThis ViewSet provides endpoints for creating, reading, updating, and deleting\nmessage templates and signatures. Templates can be used for replies, new messages,\nand signatures.\n\nFiltering:\n- type: Filter by template type (reply, new_message, signature)\n- is_active: Filter by active status (true/false)\n- mailbox_id: Filter by mailbox UUID\n- maildomain_id: Filter by mail domain UUID\n- is_forced: Filter by forced status (true/false) - works with mailbox or maildomain", + "description": "Delete a message template or signature.",
2839-2851: Mark path parameter ids as UUIDs.All message-template path ids lack format: "uuid".
{ "in": "path", "name": "id", "schema": { - "type": "string" + "type": "string", "format": "uuid" }, "required": true }Apply to retrieve, update, partial_update, destroy, and render.
Also applies to: 2876-2885, 2926-2934, 2974-2982, 3003-3011
5659-5732: Read-only schema: improve semantics; make is_forced readOnly.Clarify wording; prevent clients from assuming they can set it via read model.
"is_forced": { - "type": "boolean", - "description": "Whether this template is forced no other same type template can be used" + "type": "boolean", + "readOnly": true, + "description": "This template is enforced within its scope; other templates of the same type cannot be used." },
2798-2836: POST description mentions filters; remove to avoid confusion.Filters are for list; POST shouldn’t document them.
- "description": "ViewSet for managing message templates and signatures.\n\nThis ViewSet provides endpoints for creating, reading, updating, and deleting\nmessage templates and signatures. Templates can be used for replies, new messages,\nand signatures.\n\nFiltering:\n- type: Filter by template type (reply, new_message, signature)\n- is_active: Filter by active status (true/false)\n- mailbox_id: Filter by mailbox UUID\n- maildomain_id: Filter by mail domain UUID\n- is_forced: Filter by forced status (true/false) - works with mailbox or maildomain", + "description": "Create a message template or signature.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
src/frontend/src/features/api/gen/models/message_template.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message_template_request.tsis excluded by!**/gen/**
📒 Files selected for processing (3)
src/backend/core/api/openapi.json(4 hunks)src/backend/core/models.py(7 hunks)src/backend/core/tests/api/test_message_template.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/backend/core/tests/api/test_message_template.py (3)
src/backend/core/factories.py (6)
UserFactory(18-29)MailDomainFactory(49-55)MailboxFactory(58-94)MessageTemplateFactory(220-231)MailboxAccessFactory(97-107)MailDomainAccessFactory(110-120)src/backend/core/enums.py (3)
MessageTemplateTypeChoices(108-113)MailboxRoleChoices(15-21)MailDomainAccessRoleChoices(48-51)src/backend/core/models.py (10)
MessageTemplate(1456-1651)Blob(1227-1311)get_content(1297-1311)delete(910-930)save(88-91)save(203-206)save(285-288)save(786-838)save(1291-1295)save(1560-1571)
src/backend/core/models.py (4)
src/backend/core/enums.py (3)
MessageTemplateTypeChoices(108-113)CompressionTypeChoices(54-58)UserAbilities(75-79)src/backend/core/tests/api/test_attachments.py (1)
blob(160-166)src/backend/core/api/serializers.py (4)
create(878-880)create(1023-1063)update(882-884)update(1065-1115)src/backend/core/api/viewsets/message_template.py (1)
render_template(188-208)
⏰ 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). (3)
- GitHub Check: check-api-state
- GitHub Check: build-front
- GitHub Check: test-back
🔇 Additional comments (4)
src/backend/core/models.py (4)
487-492: Good: delegate blob creation to managerUsing Blob.objects.create_blob centralizes hashing/compression and reduces duplication.
1539-1555: Nice: partial uniques + ownership CHECK on MessageTemplateConstraints correctly enforce one forced per scope+type and prevent ownerless rows.
1560-1572: Ordering of “unforce others” is acceptableWrapped in transaction.atomic() and unique constraints exist; if validation fails, roll back. Keep as-is.
1643-1651: OK: formatted content fallbackPreferring html_body then text_body keeps signatures usable in both contexts.
Comment on lines
+2998
to
+3044
| "/api/v1.0/message-templates/{id}/render/": { | ||
| "get": { | ||
| "operationId": "message_templates_render_retrieve", | ||
| "description": "Render a template with the provided context variables.", | ||
| "parameters": [ | ||
| { | ||
| "in": "path", | ||
| "name": "id", | ||
| "schema": { | ||
| "type": "string" | ||
| }, | ||
| "required": true | ||
| } | ||
| ], | ||
| "tags": [ | ||
| "message-templates" | ||
| ], | ||
| "security": [ | ||
| { | ||
| "cookieAuth": [] | ||
| } | ||
| ], | ||
| "responses": { | ||
| "200": { | ||
| "content": { | ||
| "application/json": { | ||
| "schema": { | ||
| "type": "object", | ||
| "properties": { | ||
| "html_body": { | ||
| "type": "string" | ||
| }, | ||
| "text_body": { | ||
| "type": "string" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "description": "Template rendered with provided context" | ||
| }, | ||
| "404": { | ||
| "description": "Template not found" | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Render API can’t accept “provided context variables” as there’s no input.
Either remove that claim or add a requestBody for context.
"get": {
- "operationId": "message_templates_render_retrieve",
- "description": "Render a template with the provided context variables.",
+ "operationId": "message_templates_render_retrieve",
+ "description": "Render a template. If context variables are provided, they are applied.",
+ "requestBody": {
+ "required": false,
+ "content": {
+ "application/json": {
+ "schema": {
+ "type": "object",
+ "properties": {
+ "context": { "type": "object", "additionalProperties": true, "description": "Template variables." }
+ }
+ }
+ }
+ }
+ },Optionally add 400 for invalid context types.
📝 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.
Suggested change
| "/api/v1.0/message-templates/{id}/render/": { | |
| "get": { | |
| "operationId": "message_templates_render_retrieve", | |
| "description": "Render a template with the provided context variables.", | |
| "parameters": [ | |
| { | |
| "in": "path", | |
| "name": "id", | |
| "schema": { | |
| "type": "string" | |
| }, | |
| "required": true | |
| } | |
| ], | |
| "tags": [ | |
| "message-templates" | |
| ], | |
| "security": [ | |
| { | |
| "cookieAuth": [] | |
| } | |
| ], | |
| "responses": { | |
| "200": { | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "html_body": { | |
| "type": "string" | |
| }, | |
| "text_body": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "description": "Template rendered with provided context" | |
| }, | |
| "404": { | |
| "description": "Template not found" | |
| } | |
| } | |
| } | |
| }, | |
| "/api/v1.0/message-templates/{id}/render/": { | |
| "get": { | |
| "operationId": "message_templates_render_retrieve", | |
| "description": "Render a template. If context variables are provided, they are applied.", | |
| "requestBody": { | |
| "required": false, | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "context": { | |
| "type": "object", | |
| "additionalProperties": true, | |
| "description": "Template variables." | |
| } | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "parameters": [ | |
| { | |
| "in": "path", | |
| "name": "id", | |
| "schema": { | |
| "type": "string" | |
| }, | |
| "required": true | |
| } | |
| ], | |
| "tags": [ | |
| "message-templates" | |
| ], | |
| "security": [ | |
| { | |
| "cookieAuth": [] | |
| } | |
| ], | |
| "responses": { | |
| "200": { | |
| "content": { | |
| "application/json": { | |
| "schema": { | |
| "type": "object", | |
| "properties": { | |
| "html_body": { | |
| "type": "string" | |
| }, | |
| "text_body": { | |
| "type": "string" | |
| } | |
| } | |
| } | |
| } | |
| }, | |
| "description": "Template rendered with provided context" | |
| }, | |
| "404": { | |
| "description": "Template not found" | |
| } | |
| } | |
| } | |
| }, |
🤖 Prompt for AI Agents
In src/backend/core/api/openapi.json around lines 2998 to 3044, the GET
operation description claims the endpoint renders a template "with the provided
context variables" but there is no requestBody defined; update the spec to
either remove that phrase or (preferred) change the operation to accept a
requestBody (e.g., application/json) describing the context object schema and
mark required=false if optional, add a 400 response for invalid context types,
and adjust operationId/parameters as needed; ensure the security and response
schemas remain intact and validate the updated OpenAPI JSON against the spec.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
src/backend/core/api/serializers.py (3)
994-1021: Atomic content validation is bypassable; fix key-based check and error text.
- Current truthiness check lets clients send only html_body="" (no text/raw) without error.
- Comment says “exactly one … is required” but code and models allow both; update message.
Apply:
- # exactly one maildomain_id or mailbox_id is required + # at least one of maildomain_id or mailbox_id is required @@ - if not mailbox_id and not maildomain_id: - raise serializers.ValidationError( - "Either mailbox_id or maildomain_id is required." - ) + if not mailbox_id and not maildomain_id: + raise serializers.ValidationError( + "At least one of mailbox_id or maildomain_id is required." + ) @@ - # For creation or when updating content fields - if "html_body" in attrs or "text_body" in attrs or "raw_blob" in attrs: - html_body = attrs.get("html_body", "") - text_body = attrs.get("text_body", "") - raw_blob = attrs.get("raw_blob", "") - - # If any content field is provided, all three must be provided together - # This ensures atomic updates and prevents inconsistent states - if html_body or text_body or raw_blob: - if not html_body or not text_body or not raw_blob: - raise serializers.ValidationError( - "When providing content fields, all three fields (html_body, text_body, raw_blob) " - "must be provided together to ensure consistency." - ) + # Atomic content updates: if any of the keys is present, require all three keys. + atomic_fields = {"html_body", "text_body", "raw_blob"} + present = atomic_fields.intersection(attrs.keys()) + if present and present != atomic_fields: + missing = ", ".join(sorted(atomic_fields - present)) + raise serializers.ValidationError( + f"html_body, text_body and raw_blob must be updated together. Missing: {missing}." + )
1030-1049: Creation path can 500 on bad FK IDs; return field-scoped ValidationErrors.Uncaught DoesNotExist from Mailbox/MailDomain will bubble up as 500.
Apply:
- if mailbox_id is not None and maildomain_id is not None: + try: + if mailbox_id is not None and maildomain_id is not None: # check if domain is the same as the mailbox domain - mailbox = models.Mailbox.objects.get(id=mailbox_id) + mailbox = models.Mailbox.objects.get(id=mailbox_id) if mailbox.domain.id != maildomain_id: raise serializers.ValidationError( "Mailbox domain does not match maildomain." ) validated_data["mailbox"] = mailbox - validated_data["maildomain"] = models.MailDomain.objects.get( - id=maildomain_id - ) - elif mailbox_id is not None: - validated_data["mailbox"] = models.Mailbox.objects.get(id=mailbox_id) - elif maildomain_id is not None: - validated_data["maildomain"] = models.MailDomain.objects.get( - id=maildomain_id - ) + validated_data["maildomain"] = models.MailDomain.objects.get(id=maildomain_id) + elif mailbox_id is not None: + validated_data["mailbox"] = models.Mailbox.objects.get(id=mailbox_id) + elif maildomain_id is not None: + validated_data["maildomain"] = models.MailDomain.objects.get(id=maildomain_id) + except models.Mailbox.DoesNotExist: + raise serializers.ValidationError({"mailbox_id": "Mailbox not found"}) + except models.MailDomain.DoesNotExist: + raise serializers.ValidationError({"maildomain_id": "Maildomain not found"})
1065-1115: Update path doesn’t persist owner changes and may create ownerless blobs.
- New mailbox_id/maildomain_id are validated but never assigned ⇒ relationship updates are dropped.
- Creating a new blob uses payload IDs only; when omitted, both are None ⇒ violates Blob ownership constraint.
Apply:
- # Update mailbox or maildomain if provided + # Update mailbox or maildomain if provided (validate consistency first) @@ - elif maildomain_id is not None and instance.mailbox: + elif maildomain_id is not None and instance.mailbox: maildomain = models.MailDomain.objects.get(id=maildomain_id) if instance.mailbox.domain != maildomain: raise serializers.ValidationError( "Mailbox domain does not match maildomain." ) + + # Apply relationship updates (assign objects) + try: + if mailbox_id is not None: + validated_data["mailbox"] = models.Mailbox.objects.get(id=mailbox_id) + if maildomain_id is not None: + validated_data["maildomain"] = models.MailDomain.objects.get(id=maildomain_id) + except models.Mailbox.DoesNotExist: + raise serializers.ValidationError({"mailbox_id": "Mailbox not found"}) + except models.MailDomain.DoesNotExist: + raise serializers.ValidationError({"maildomain_id": "Maildomain not found"}) @@ - if raw_blob_content: # Only create blob if content is not empty - blob = models.Blob.objects.create_blob( + if raw_blob_content: # Only create blob if content is not empty + # Fallback ownership to instance when IDs are not provided + owner_mailbox_id = mailbox_id if mailbox_id is not None else instance.mailbox_id + owner_maildomain_id = maildomain_id if maildomain_id is not None else instance.maildomain_id + blob = models.Blob.objects.create_blob( content=raw_blob_content.encode("utf-8"), content_type="application/json", - mailbox_id=mailbox_id, - maildomain_id=maildomain_id, + mailbox_id=owner_mailbox_id, + maildomain_id=owner_maildomain_id, ) validated_data["raw_blob"] = blob else: validated_data["raw_blob"] = None
🧹 Nitpick comments (4)
src/backend/core/api/serializers.py (1)
1050-1061: Validate raw_blob JSON before storing as application/json.If the API accepts invalid JSON, downstream editors/parsers will break.
Apply around both create/update paths:
+ # Validate JSON content if provided + import json + try: + json.loads(raw_blob_content) + except json.JSONDecodeError as e: + raise serializers.ValidationError({"raw_blob": f"Invalid JSON: {e.msg}"})Also applies to: 1092-1112
src/backend/core/models.py (3)
1532-1538: Clarify is_forced help_text.Current phrasing is awkward.
- help_text=_( - "Whether this template is forced no other same type template can be used" - ), + help_text=_( + "Whether this template is forced; no other template of the same type can be used in the same scope" + ),
1567-1579: Demote “other forced” after saving to reduce race windows.Save the instance first, then unforce others in the same transaction. Keeps state consistent if save fails and plays nicer with the partial unique constraints.
- def save(self, *args, **kwargs): - with transaction.atomic(): - if self.is_forced: - qs = MessageTemplate.objects.filter( - type=self.type, is_forced=True - ).exclude(id=self.id) - if self.mailbox_id: - qs = qs.filter(mailbox_id=self.mailbox_id) - if self.maildomain_id: - qs = qs.filter(maildomain_id=self.maildomain_id) - qs.update(is_forced=False) - super().save(*args, **kwargs) + def save(self, *args, **kwargs): + with transaction.atomic(): + super().save(*args, **kwargs) + if self.is_forced: + qs = MessageTemplate.objects.filter(type=self.type, is_forced=True).exclude(id=self.id) + if self.mailbox_id: + qs = qs.filter(mailbox_id=self.mailbox_id) + if self.maildomain_id: + qs = qs.filter(maildomain_id=self.maildomain_id) + qs.update(is_forced=False)
1540-1562: Indexing tip for query hotspots.If templates are frequently fetched by (mailbox_id or maildomain_id, type, is_active), consider adding a composite index to speed lookups alongside the partial uniques.
Example:
class Meta: indexes = [ models.Index(fields=("mailbox", "type", "is_active")), models.Index(fields=("maildomain", "type", "is_active")), ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
src/backend/core/api/serializers.py(2 hunks)src/backend/core/models.py(6 hunks)src/backend/core/tests/api/test_message_template.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend/core/tests/api/test_message_template.py
🧰 Additional context used
🧬 Code graph analysis (2)
src/backend/core/api/serializers.py (2)
src/backend/core/enums.py (1)
MessageTemplateTypeChoices(108-113)src/backend/core/models.py (25)
get_content(1306-1320)Meta(85-86)Meta(195-198)Meta(277-280)Meta(441-446)Meta(568-572)Meta(596-599)Meta(776-781)Meta(948-952)Meta(969-973)Meta(1010-1014)Meta(1069-1073)Meta(1282-1294)Meta(1352-1356)Meta(1392-1396)Meta(1446-1450)Meta(1540-1562)MessageTemplate(1465-1639)Mailbox(412-550)MailDomain(237-409)Blob(1227-1320)create_blob(466-492)create_blob(1162-1224)content_type(1362-1364)delete(910-930)
src/backend/core/models.py (4)
src/backend/core/enums.py (2)
MessageTemplateTypeChoices(108-113)CompressionTypeChoices(54-58)src/backend/core/tests/api/test_attachments.py (1)
blob(160-166)src/backend/core/api/serializers.py (4)
create(878-880)create(1023-1063)update(882-884)update(1065-1115)src/backend/core/api/viewsets/message_template.py (1)
render_template(188-208)
⏰ 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). (5)
- GitHub Check: docker-publish-mta-in / docker-build-push
- GitHub Check: check-api-state
- GitHub Check: build-front
- GitHub Check: test-back
- GitHub Check: lint-back
🔇 Additional comments (5)
src/backend/core/api/serializers.py (1)
966-975: to_representation override looks good.Ensures clients always receive decoded blob content; avoids double-fetch on the client.
src/backend/core/models.py (4)
1596-1629: Good: HTML-escape placeholder values in render_template.Prevents HTML injection via user attributes while keeping text_body untouched.
1287-1305: Good: DB constraint + clean() enforce Blob ownership invariants.Prevents ownerless blobs at both app and DB layers.
1162-1225: BlobManager.create_blob is solid (hashing, compression, logging).Appropriate defaults and metrics; aligns with Mailbox.create_blob usage.
487-493: Nice reuse: Mailbox.create_blob delegates to BlobManager.Keeps hashing/compression logic centralized.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
src/backend/core/api/openapi.json (4)
2771-2797: Re-add typed query parameters for filters on message-templates list.The GET description documents filters but the parameters array is missing (regression vs earlier review). Add explicit query params for client generation.
"/api/v1.0/message-templates/": { "get": { "operationId": "message_templates_list", "description": "ViewSet for managing message templates and signatures.\n\nThis ViewSet provides endpoints for creating, reading, updating, and deleting\nmessage templates and signatures. Templates can be used for replies, new messages,\nand signatures.\n\nFiltering:\n- type: Filter by template type (reply, new_message, signature)\n- is_active: Filter by active status (true/false)\n- mailbox_id: Filter by mailbox UUID\n- maildomain_id: Filter by mail domain UUID\n- is_forced: Filter by forced status (true/false) - works with mailbox or maildomain", + "parameters": [ + { "in": "query", "name": "type", "schema": { "$ref": "#/components/schemas/MessageTemplateTypeChoices" }, "description": "Filter by template type." }, + { "in": "query", "name": "is_active", "schema": { "type": "boolean" }, "description": "Filter by active status." }, + { "in": "query", "name": "mailbox_id", "schema": { "type": "string", "format": "uuid" }, "description": "Filter by mailbox UUID." }, + { "in": "query", "name": "maildomain_id", "schema": { "type": "string", "format": "uuid" }, "description": "Filter by mail domain UUID." }, + { "in": "query", "name": "is_forced", "schema": { "type": "boolean" }, "description": "Filter by forced status." } + ],
5279-5331: Enforce contracts on create: html/text/raw trio, UUID formats, and scope exclusivity.Require html_body, text_body, and raw_blob together; type IDs as UUIDs; allow exactly one of mailbox_id or maildomain_id.
"MessageTemplateRequest": { "type": "object", "description": "Serialize message templates for POST/PUT/PATCH operations.", "properties": { "raw_blob": { - "type": "string", - "minLength": 1, - "description": "Raw blob content" + "type": "string", + "format": "uuid", + "minLength": 1, + "description": "Blob ID containing the raw template content" }, "mailbox_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Mailbox UUID" }, "maildomain_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Maildomain UUID" }, "is_forced": { "type": "boolean", "default": false, - "description": "Set as forced template" + "description": "Force usage of this template within its scope (mailbox or maildomain)" } }, - "required": [ - "name", - "type" - ] + "required": ["name","type","raw_blob","html_body","text_body"], + "oneOf": [ + { "required": ["mailbox_id"] }, + { "required": ["maildomain_id"] } + ] },
5217-5278: Fix MessageTemplate field formats and wording.
- raw_blob must be a Blob UUID, not opaque text.
- Clarify is_forced semantics per product behavior.
"raw_blob": { - "type": "string", - "description": "Raw blob content" + "type": "string", + "format": "uuid", + "description": "Blob ID containing the raw template content" }, "is_forced": { "type": "boolean", "default": false, - "description": "Set as forced template" + "description": "Force usage of this template within its scope (mailbox or maildomain)" },
5581-5629: PATCH contract: all-or-none for html/text/raw and UUID formats.For partial updates, require the trio together if any is present; also add UUID formats to IDs.
"PatchedMessageTemplateRequest": { "type": "object", "description": "Serialize message templates for POST/PUT/PATCH operations.", "properties": { "raw_blob": { - "type": "string", - "minLength": 1, - "description": "Raw blob content" + "type": "string", + "format": "uuid", + "minLength": 1, + "description": "Blob ID containing the raw template content" }, "mailbox_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Mailbox UUID" }, "maildomain_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Maildomain UUID" } - } + }, + "oneOf": [ + { "not": { "anyOf": [ { "required": ["raw_blob"] }, { "required": ["html_body"] }, { "required": ["text_body"] } ] } }, + { "required": ["raw_blob","html_body","text_body"] } + ] },
🧹 Nitpick comments (2)
src/backend/core/api/openapi.json (2)
2843-2851: Type the path parameter id as UUID across message-templates operations.Consistency with other endpoints improves client typing and validation.
- "schema": { "type": "string" } + "schema": { "type": "string", "format": "uuid" }Also applies to: 2876-2885, 2926-2934, 2974-2981
5649-5712: Tighten ReadOnlyMessageTemplate typing; optional copy edit.Set raw_blob format to uuid and clarify is_forced wording. Keeping raw_blob in required while nullable is acceptable but reads oddly—consider dropping from required if null is common.
"raw_blob": { - "type": "string", + "type": "string", + "format": "uuid", "nullable": true, - "description": "Get raw blob.", + "description": "Blob ID for the raw template content.", "readOnly": true }, "is_forced": { "type": "boolean", - "description": "Whether this template is forced no other same type template can be used" + "description": "If true, this template is enforced within its scope and cannot be overridden." },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (3)
src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message_template.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/read_only_message_template.tsis excluded by!**/gen/**
📒 Files selected for processing (1)
src/backend/core/api/openapi.json(4 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). (3)
- GitHub Check: build-front
- GitHub Check: check-api-state
- GitHub Check: test-back
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/backend/messages/settings.py (1)
96-110: Confirm intent: Compose-specific defaults should live in Development, not Base.If you need "postgresql" service host locally, move those defaults under Development to avoid impacting prod/CI.
src/backend/core/models.py (3)
1159-1225: Minor: use self.create instead of Blob.objects.create inside the manager.Avoid bypassing the current manager and keep things testable.
Apply this diff:
- blob = Blob.objects.create( + blob = self.create( sha256=sha256_hash, size=original_size, content_type=content_type, compression=compression, raw_content=compressed_content, **kwargs, )
1564-1566: Optional: use partial indexes on is_active=True for better selectivity.If lookups nearly always filter on active templates, partial indexes reduce size and speed scans. Requires migration churn.
Apply this diff:
- indexes = [ - models.Index(fields=("mailbox", "type", "is_active")), - models.Index(fields=("maildomain", "type", "is_active")), - ] + indexes = [ + models.Index( + fields=("mailbox", "type"), + condition=models.Q(is_active=True), + name="mt_active_mailbox_type_idx", + ), + models.Index( + fields=("maildomain", "type"), + condition=models.Q(is_active=True), + name="mt_active_maildomain_type_idx", + ), + ]
1571-1583: Demoting other forced templates pre-save is OK; consider locking to reduce races.Inside the atomic block, a select_for_update() on the forced set can minimize concurrent flip-flops; UNIQUE still protects correctness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/backend/core/api/serializers.py(2 hunks)src/backend/core/api/viewsets/message_template.py(1 hunks)src/backend/core/migrations/0007_alter_messagetemplate_type_blob_blob_has_owner_and_more.py(1 hunks)src/backend/core/migrations/0008_alter_messagetemplate_is_forced_and_more.py(1 hunks)src/backend/core/models.py(6 hunks)src/backend/messages/settings.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/backend/core/api/serializers.py
- src/backend/core/api/viewsets/message_template.py
🧰 Additional context used
🧬 Code graph analysis (3)
src/backend/core/migrations/0007_alter_messagetemplate_type_blob_blob_has_owner_and_more.py (1)
src/backend/core/migrations/0008_alter_messagetemplate_is_forced_and_more.py (1)
Migration(6-26)
src/backend/core/migrations/0008_alter_messagetemplate_is_forced_and_more.py (1)
src/backend/core/migrations/0007_alter_messagetemplate_type_blob_blob_has_owner_and_more.py (1)
Migration(6-34)
src/backend/core/models.py (4)
src/backend/core/enums.py (2)
MessageTemplateTypeChoices(108-113)CompressionTypeChoices(54-58)src/backend/core/tests/api/test_attachments.py (1)
blob(160-166)src/backend/core/api/serializers.py (4)
create(878-880)create(1023-1063)update(882-884)update(1065-1115)src/backend/core/api/viewsets/message_template.py (1)
render_template(188-208)
⏰ 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). (4)
- GitHub Check: build-front
- GitHub Check: lint-back
- GitHub Check: test-back
- GitHub Check: check-api-state
🔇 Additional comments (8)
src/backend/core/migrations/0008_alter_messagetemplate_is_forced_and_more.py (1)
12-26: Indexes for active lookups: LGTM.The added (mailbox|maildomain, type, is_active) indexes align with common queries.
src/backend/core/migrations/0007_alter_messagetemplate_type_blob_blob_has_owner_and_more.py (1)
12-34: Owner checks and partial uniques: LGTM.The CHECK and conditional UNIQUE constraints correctly enforce ownership and “single forced per scope+type.”
src/backend/core/models.py (6)
17-19: Needed imports added.transaction and escape are appropriate for the new logic.
487-492: Good: delegate blob creation to the manager.Centralizing compression/hash logic in BlobManager improves consistency.
1264-1294: Blob ownership and DB CHECK: LGTM.Allowing mailbox or maildomain (or both) with a CHECK is coherent with the serializers.
1299-1305: Validation at model level: LGTM.Raising ValidationError ensures DRF/admin return 400s.
1465-1567: MessageTemplate model, constraints, and indexes: solid.Owner CHECK + partial UNIQUEs match the intended invariants.
1600-1633: Escaped HTML substitution: LGTM.Values are escaped for HTML and raw for text; simple placeholder replacement is acceptable here.
sdemagny
force-pushed
the
sdem/proposition1_template
branch
from
7c7d8d4 to
e29bd44
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (10)
src/backend/core/api/serializers.py (5)
1030-1049: Uncaught DoesNotExist leads to 500 on bad IDsWrap FK lookups; return field-scoped ValidationError. Also use domain_id for efficiency.
- if mailbox_id is not None and maildomain_id is not None: - # check if domain is the same as the mailbox domain - mailbox = models.Mailbox.objects.get(id=mailbox_id) - if mailbox.domain.id != maildomain_id: - raise serializers.ValidationError( - "Mailbox domain does not match maildomain." - ) - validated_data["mailbox"] = mailbox - validated_data["maildomain"] = models.MailDomain.objects.get( - id=maildomain_id - ) - elif mailbox_id is not None: - validated_data["mailbox"] = models.Mailbox.objects.get(id=mailbox_id) - elif maildomain_id is not None: - validated_data["maildomain"] = models.MailDomain.objects.get( - id=maildomain_id - ) + try: + if mailbox_id is not None and maildomain_id is not None: + mailbox = models.Mailbox.objects.get(id=mailbox_id) + if mailbox.domain_id != maildomain_id: + raise serializers.ValidationError("Mailbox domain does not match maildomain.") + validated_data["mailbox"] = mailbox + validated_data["maildomain"] = models.MailDomain.objects.get(id=maildomain_id) + elif mailbox_id is not None: + validated_data["mailbox"] = models.Mailbox.objects.get(id=mailbox_id) + elif maildomain_id is not None: + validated_data["maildomain"] = models.MailDomain.objects.get(id=maildomain_id) + except models.Mailbox.DoesNotExist: + raise serializers.ValidationError({"mailbox_id": "Mailbox not found"}) + except models.MailDomain.DoesNotExist: + raise serializers.ValidationError({"maildomain_id": "Maildomain not found"})
1071-1091: Update validates owner FKs but never applies themNew mailbox_id/maildomain_id are checked but not assigned; relationship updates won’t persist.
# Update mailbox or maildomain if provided (validate consistency first) if mailbox_id is not None and maildomain_id is not None: @@ raise serializers.ValidationError( "Mailbox domain does not match maildomain." ) - elif mailbox_id is not None and instance.maildomain: + elif mailbox_id is not None and instance.maildomain: mailbox = models.Mailbox.objects.get(id=mailbox_id) if instance.maildomain != mailbox.domain: raise serializers.ValidationError( "Mailbox domain does not match maildomain." ) - elif maildomain_id is not None and instance.mailbox: + elif maildomain_id is not None and instance.mailbox: maildomain = models.MailDomain.objects.get(id=maildomain_id) if instance.mailbox.domain != maildomain: raise serializers.ValidationError( "Mailbox domain does not match maildomain." ) + + # Apply owner relationship updates (with proper error handling) + try: + if mailbox_id is not None: + validated_data["mailbox"] = models.Mailbox.objects.get(id=mailbox_id) + if maildomain_id is not None: + validated_data["maildomain"] = models.MailDomain.objects.get(id=maildomain_id) + except models.Mailbox.DoesNotExist: + raise serializers.ValidationError({"mailbox_id": "Mailbox not found"}) + except models.MailDomain.DoesNotExist: + raise serializers.ValidationError({"maildomain_id": "Maildomain not found"})
1092-1115: Ownerless blob risk on update + missing JSON validationWhen mailbox_id/maildomain_id aren’t in the payload, you pass None to create_blob, violating blob_has_owner and causing a 500. Also no JSON validation.
- if raw_blob_content is not None: + if raw_blob_content is not None: try: if instance.raw_blob: instance.raw_blob.delete() except models.Blob.DoesNotExist: pass - if raw_blob_content: # Only create blob if content is not empty - blob = models.Blob.objects.create_blob( + if raw_blob_content: # Only create blob if content is not empty + # Validate JSON content + import json + try: + json.loads(raw_blob_content) + except json.JSONDecodeError as e: + raise serializers.ValidationError({"raw_blob": f"Invalid JSON: {e.msg}"}) + + # Fallback ownership to existing instance when not provided + owner_mailbox_id = ( + mailbox_id if mailbox_id is not None else instance.mailbox_id + ) + owner_maildomain_id = ( + maildomain_id if maildomain_id is not None else instance.maildomain_id + ) + blob = models.Blob.objects.create_blob( content=raw_blob_content.encode("utf-8"), content_type="application/json", - mailbox_id=mailbox_id, - maildomain_id=maildomain_id, + mailbox_id=owner_mailbox_id, + maildomain_id=owner_maildomain_id, ) validated_data["raw_blob"] = blob else: validated_data["raw_blob"] = None
1006-1020: Atomic content update check is ineffective (truthiness-based); enforce key presenceCurrent logic allows partial updates when empty strings are sent. Enforce presence of all three keys when any appears to meet the PR objective.
- # For creation or when updating content fields - if "html_body" in attrs or "text_body" in attrs or "raw_blob" in attrs: - html_body = attrs.get("html_body", "") - text_body = attrs.get("text_body", "") - raw_blob = attrs.get("raw_blob", "") - - # If any content field is provided, all three must be provided together - # This ensures atomic updates and prevents inconsistent states - if html_body or text_body or raw_blob: - if not html_body or not text_body or not raw_blob: - raise serializers.ValidationError( - "When providing content fields, all three fields (html_body, text_body, raw_blob) " - "must be provided together to ensure consistency." - ) + # Atomic content update: if any key is present, require all three keys + atomic_fields = {"html_body", "text_body", "raw_blob"} + present = atomic_fields.intersection(attrs.keys()) + if present and present != atomic_fields: + missing = ", ".join(sorted(atomic_fields - present)) + raise serializers.ValidationError( + f"html_body, text_body and raw_blob must be updated together. Missing: {missing}." + )
1051-1061: Validate JSON before creating blobYou’re storing as application/json; reject invalid JSON up front and avoid garbage blobs.
- if raw_blob_content: - blob = models.Blob.objects.create_blob( + if raw_blob_content: + # Validate JSON content + import json + try: + json.loads(raw_blob_content) + except json.JSONDecodeError as e: + raise serializers.ValidationError({"raw_blob": f"Invalid JSON: {e.msg}"}) + blob = models.Blob.objects.create_blob( content=raw_blob_content.encode("utf-8"), content_type="application/json", mailbox_id=mailbox_id, maildomain_id=maildomain_id, ) validated_data["raw_blob"] = blobsrc/backend/core/api/openapi.json (5)
2770-2797: Add typed query parameters for documented filters on list endpoint.Filters are described but not typed; this hurts client generation and validation. Add explicit query params (with UUID/boolean formats).
"/api/v1.0/message-templates/": { "get": { "operationId": "message_templates_list", "description": "ViewSet for managing message templates and signatures.\n\nThis ViewSet provides endpoints for creating, reading, updating, and deleting\nmessage templates and signatures. Templates can be used for replies, new messages,\nand signatures.\n\nFiltering:\n- type: Filter by template type (reply, new_message, signature)\n- is_active: Filter by active status (true/false)\n- mailbox_id: Filter by mailbox UUID\n- maildomain_id: Filter by mail domain UUID\n- is_forced: Filter by forced status (true/false) - works with mailbox or maildomain", + "parameters": [ + { "in": "query", "name": "type", "schema": { "$ref": "#/components/schemas/MessageTemplateTypeChoices" }, "description": "Filter by template type." }, + { "in": "query", "name": "is_active", "schema": { "type": "boolean" }, "description": "Filter by active status." }, + { "in": "query", "name": "mailbox_id", "schema": { "type": "string", "format": "uuid" }, "description": "Filter by mailbox UUID." }, + { "in": "query", "name": "maildomain_id", "schema": { "type": "string", "format": "uuid" }, "description": "Filter by mail domain UUID." }, + { "in": "query", "name": "is_forced", "schema": { "type": "boolean" }, "description": "Filter by forced status within scope." } + ], "tags": [ "message-templates" ],
2998-3044: Switch render endpoint to POST and accept context payload; GET-with-body isn’t portable.Also type the path id as UUID and add 400 for invalid context.
- "/api/v1.0/message-templates/{id}/render/": { - "get": { - "operationId": "message_templates_render_retrieve", - "description": "Render a template with the provided context variables.", + "/api/v1.0/message-templates/{id}/render/": { + "post": { + "operationId": "message_templates_render_create", + "description": "Render a template. If context variables are provided, they are applied.", "parameters": [ { "in": "path", "name": "id", "schema": { - "type": "string" + "type": "string", + "format": "uuid" }, "required": true } ], "tags": [ "message-templates" ], + "requestBody": { + "required": false, + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "context": { "type": "object", "additionalProperties": true, "description": "Template variables." } + } + } + } + } + }, "security": [ { "cookieAuth": [] } ], "responses": { "200": { "content": { "application/json": { "schema": { "type": "object", "properties": { "html_body": { "type": "string" }, "text_body": { "type": "string" } } } } }, "description": "Template rendered with provided context" }, + "400": { "description": "Invalid context." }, "404": { "description": "Template not found" } } } },
5217-5278: Fix raw_blob typing and clarify is_forced semantics in MessageTemplate schema.Use UUID for blob reference and accurate description to match backend and DB model.
"MessageTemplate": { "type": "object", "description": "Serialize message templates for POST/PUT/PATCH operations.", "properties": { @@ - "raw_blob": { - "type": "string", - "description": "Raw blob content" - }, + "raw_blob": { + "type": "string", + "format": "uuid", + "nullable": true, + "description": "Blob ID containing the raw template content" + }, @@ - "is_forced": { - "type": "boolean", - "default": false, - "description": "Set as forced template" - }, + "is_forced": { + "type": "boolean", + "default": false, + "description": "Force usage of this template within its scope (mailbox or maildomain)" + },
5280-5331: Harden request contract: require html/text/raw together; type IDs as UUID; enforce mailbox/maildomain exclusivity.Aligns with PR goals and prevents partial state.
"MessageTemplateRequest": { "type": "object", "description": "Serialize message templates for POST/PUT/PATCH operations.", "properties": { @@ - "raw_blob": { - "type": "string", - "minLength": 1, - "description": "Raw blob content" - }, + "raw_blob": { + "type": "string", + "format": "uuid", + "minLength": 1, + "description": "Blob ID containing the raw template content" + }, @@ - "is_forced": { + "is_forced": { "type": "boolean", "default": false, - "description": "Set as forced template" + "description": "Force usage of this template within its scope (mailbox or maildomain)" }, "mailbox_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Mailbox UUID" }, "maildomain_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Maildomain UUID" } }, - "required": [ - "name", - "type" - ] + "required": ["name","type","raw_blob","html_body","text_body"], + "oneOf": [ + { "required": ["mailbox_id"] }, + { "required": ["maildomain_id"] } + ] },
5581-5629: PATCH: all-or-none rule for html/text/raw and UUID formats.Permit partial updates but keep the trio consistent when present.
"PatchedMessageTemplateRequest": { "type": "object", "description": "Serialize message templates for POST/PUT/PATCH operations.", "properties": { @@ - "raw_blob": { - "type": "string", - "minLength": 1, - "description": "Raw blob content" - }, + "raw_blob": { + "type": "string", + "format": "uuid", + "minLength": 1, + "description": "Blob ID containing the raw template content" + }, @@ - "mailbox_id": { - "type": "string", + "mailbox_id": { + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Mailbox UUID" }, - "maildomain_id": { - "type": "string", + "maildomain_id": { + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Maildomain UUID" } - } + }, + "oneOf": [ + { "not": { "anyOf": [ { "required": ["raw_blob"] }, { "required": ["html_body"] }, { "required": ["text_body"] } ] } }, + { "required": ["raw_blob","html_body","text_body"] } + ] },
🧹 Nitpick comments (6)
src/backend/core/api/serializers.py (2)
955-960: Prefer UUIDField for FK IDsUse serializers.UUIDField to validate IDs early and avoid passing arbitrary strings to .get().
- mailbox_id = serializers.CharField( + mailbox_id = serializers.UUIDField( required=False, write_only=True, help_text="Mailbox UUID" ) - maildomain_id = serializers.CharField( + maildomain_id = serializers.UUIDField( required=False, write_only=True, help_text="Maildomain UUID" )
996-999: Comment contradicts model rules (both owners allowed if same domain)Model clean() allows both mailbox and maildomain (same domain). Update the comment to avoid confusion.
- # exactly one maildomain_id or mailbox_id is required + # at least one of maildomain_id or mailbox_id is required (both allowed if same domain)src/backend/core/api/openapi.json (1)
2843-2851: Type path parameter id as UUID for all MessageTemplate item routes.Improves client typing and validation.
{ "in": "path", "name": "id", "schema": { - "type": "string" + "type": "string", + "format": "uuid" }, "required": true }Also applies to: 2876-2885, 2926-2934, 2974-2982
src/backend/core/models.py (3)
487-492: Optionally also persist maildomain on blobs created via Mailbox.create_blob.
Adds symmetry with serializer path and can simplify owner-scoped queries.Apply:
return Blob.objects.create_blob( content=content, content_type=content_type, compression=compression, mailbox=self, + maildomain=self.domain, )
1159-1225: Reduce blob creation log verbosity; switch info→debug.
Blob creation can be high volume; debug is usually enough.- logger.info( + logger.debug( "Created blob %s: %d bytes, %s compression, %s content type", blob.id, original_size, compression.label, content_type, )
1571-1583: Map unique-constraint races to a clean 400; keep atomicity.
Concurrent “forced” saves can raise IntegrityError. Catch and re-raise ValidationError for DRF/admin UX.-from django.db import models, transaction +from django.db import models, transaction, IntegrityErrordef save(self, *args, **kwargs): with transaction.atomic(): - if self.is_forced: + if self.is_forced: qs = MessageTemplate.objects.filter( type=self.type, is_forced=True ).exclude(id=self.id) if self.mailbox_id: qs = qs.filter(mailbox_id=self.mailbox_id) if self.maildomain_id: qs = qs.filter(maildomain_id=self.maildomain_id) qs.update(is_forced=False) - super().save(*args, **kwargs) + try: + super().save(*args, **kwargs) + except IntegrityError as e: + # Likely violated partial UNIQUE on forced per scope+type + raise ValidationError( + {"is_forced": "Only one forced template per scope and type is allowed."} + ) from eAlso applies to: 17-17
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
src/frontend/src/features/api/gen/models/read_only_message_template.tsis excluded by!**/gen/**
📒 Files selected for processing (6)
src/backend/core/api/openapi.json(4 hunks)src/backend/core/api/serializers.py(2 hunks)src/backend/core/api/viewsets/message_template.py(1 hunks)src/backend/core/migrations/0007_alter_messagetemplate_type_blob_blob_has_owner_and_more.py(1 hunks)src/backend/core/migrations/0008_alter_messagetemplate_is_forced_and_more.py(1 hunks)src/backend/core/models.py(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/backend/core/api/viewsets/message_template.py
- src/backend/core/migrations/0007_alter_messagetemplate_type_blob_blob_has_owner_and_more.py
- src/backend/core/migrations/0008_alter_messagetemplate_is_forced_and_more.py
🧰 Additional context used
🧬 Code graph analysis (2)
src/backend/core/api/serializers.py (2)
src/backend/core/enums.py (1)
MessageTemplateTypeChoices(108-113)src/backend/core/models.py (25)
get_content(1306-1320)Meta(85-86)Meta(195-198)Meta(277-280)Meta(441-446)Meta(568-572)Meta(596-599)Meta(776-781)Meta(948-952)Meta(969-973)Meta(1010-1014)Meta(1069-1073)Meta(1282-1294)Meta(1352-1356)Meta(1392-1396)Meta(1446-1450)Meta(1540-1566)MessageTemplate(1465-1643)Mailbox(412-550)MailDomain(237-409)Blob(1227-1320)create_blob(466-492)create_blob(1162-1224)content_type(1362-1364)delete(910-930)
src/backend/core/models.py (6)
src/frontend/src/features/api/gen/models/message_template_type_choices.ts (2)
MessageTemplateTypeChoices(9-10)MessageTemplateTypeChoices(13-17)src/backend/core/enums.py (2)
MessageTemplateTypeChoices(108-113)CompressionTypeChoices(54-58)src/backend/core/tests/api/test_attachments.py (1)
blob(160-166)src/backend/core/api/serializers.py (4)
create(878-880)create(1023-1063)update(882-884)update(1065-1115)src/frontend/src/features/api/gen/models/message_template.ts (1)
MessageTemplate(13-36)src/backend/core/api/viewsets/message_template.py (1)
render_template(188-208)
⏰ 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). (2)
- GitHub Check: build-front
- GitHub Check: test-back
🔇 Additional comments (12)
src/backend/core/api/serializers.py (3)
1-1: LGTM: module-level changesImport and pylint directive look fine.
Also applies to: 4-4
922-947: LGTM: read-only template serializerReadable, safe decoding via Blob.get_content().
966-975: LGTM: consistent output for raw_blobRepresentation override is correct and keeps API coherent with read-only serializer.
src/backend/core/models.py (9)
17-19: Good: imports required for atomic updates and safe HTML escaping.
37-37: Enum import aligns with choices usage.
1264-1278: Blob ownership fields look correct.
Allows either mailbox or maildomain ownership.
1280-1280: Custom manager registration is appropriate.
1287-1294: DB-level ownership CHECK is solid.
1299-1305: Model clean() enforces owner presence.
1468-1513: Fields/readability LGTM.
Name, html/text bodies, raw_blob, type, and is_active cover expected use cases.
1514-1531: Owner FKs LGTM.
Scope can be mailbox or maildomain.
1635-1644: get_formatted_content() is fine.
Returns HTML preferred, else text.
Comment on lines
+1532
to
+1538
| is_forced = models.BooleanField( | ||
| _("is forced"), | ||
| default=False, | ||
| help_text=_( | ||
| "Whether this template is forced; no other template of the same type can be used in the same scope" | ||
| ), | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Clarify forced vs active state.
Forced templates should always be active; enforce via validation/DB check (see below).
🤖 Prompt for AI Agents
In src/backend/core/models.py around lines 1532 to 1538, the new is_forced
BooleanField must imply the template is active; implement enforcement by adding
model-level validation and a DB constraint: add a clean() method (or override
save()) to set self.is_active = True when self.is_forced is True and raise
ValidationError if someone tries to save with is_forced=True and
is_active=False, and add a CheckConstraint (or Unique/Conditional constraint if
supported) to the model Meta to prevent rows where is_forced is True and
is_active is False at the database level so the invariant is enforced both in
application logic and DB.
Comment on lines
+1584
to
+1599
| def clean(self): | ||
| errors = {} | ||
| if not self.mailbox and not self.maildomain: | ||
| errors["__all__"] = ( | ||
| "MessageTemplate must have at least a mailbox or maildomain" | ||
| ) | ||
| if ( | ||
| self.mailbox | ||
| and self.maildomain | ||
| and self.mailbox.domain_id != self.maildomain_id | ||
| ): | ||
| errors["mailbox"] = "Mailbox domain does not match maildomain." | ||
| if errors: | ||
| raise ValidationError(errors) | ||
| super().clean() | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tighten validation: forced⇒active; optional: raw_blob owner match and non-empty content.
- Essential: is_forced requires is_active.
- Optional: if raw_blob set, its owner should match mailbox/maildomain; and require at least one of html_body/text_body/raw_blob.
def clean(self):
errors = {}
if not self.mailbox and not self.maildomain:
errors["__all__"] = (
"MessageTemplate must have at least a mailbox or maildomain"
)
if (
self.mailbox
and self.maildomain
and self.mailbox.domain_id != self.maildomain_id
):
errors["mailbox"] = "Mailbox domain does not match maildomain."
+ if self.is_forced and not self.is_active:
+ errors["is_forced"] = "Forced templates must be active."
+ # Optional guards
+ if self.raw_blob_id:
+ if self.mailbox_id and self.raw_blob.mailbox_id != self.mailbox_id:
+ errors["raw_blob"] = "Raw blob owner (mailbox) must match the template owner."
+ if self.maildomain_id and self.raw_blob.maildomain_id != self.maildomain_id:
+ errors["raw_blob"] = "Raw blob owner (maildomain) must match the template owner."
+ if not (self.html_body or self.text_body or self.raw_blob_id):
+ errors["__all__"] = errors.get("__all__") or "At least one of html_body, text_body or raw_blob is required."
if errors:
raise ValidationError(errors)
super().clean()📝 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.
Suggested change
| def clean(self): | |
| errors = {} | |
| if not self.mailbox and not self.maildomain: | |
| errors["__all__"] = ( | |
| "MessageTemplate must have at least a mailbox or maildomain" | |
| ) | |
| if ( | |
| self.mailbox | |
| and self.maildomain | |
| and self.mailbox.domain_id != self.maildomain_id | |
| ): | |
| errors["mailbox"] = "Mailbox domain does not match maildomain." | |
| if errors: | |
| raise ValidationError(errors) | |
| super().clean() | |
| def clean(self): | |
| errors = {} | |
| if not self.mailbox and not self.maildomain: | |
| errors["__all__"] = ( | |
| "MessageTemplate must have at least a mailbox or maildomain" | |
| ) | |
| if ( | |
| self.mailbox | |
| and self.maildomain | |
| and self.mailbox.domain_id != self.maildomain_id | |
| ): | |
| errors["mailbox"] = "Mailbox domain does not match maildomain." | |
| # Enforce that forced templates must also be active | |
| if self.is_forced and not self.is_active: | |
| errors["is_forced"] = "Forced templates must be active." | |
| # Optional: if using a raw_blob, its owner must match this template's owner | |
| if self.raw_blob_id: | |
| if self.mailbox_id and self.raw_blob.mailbox_id != self.mailbox_id: | |
| errors["raw_blob"] = "Raw blob owner (mailbox) must match the template owner." | |
| if self.maildomain_id and self.raw_blob.maildomain_id != self.maildomain_id: | |
| errors["raw_blob"] = "Raw blob owner (maildomain) must match the template owner." | |
| # Ensure the template has at least one form of content | |
| if not (self.html_body or self.text_body or self.raw_blob_id): | |
| errors["__all__"] = errors.get("__all__") or "At least one of html_body, text_body or raw_blob is required." | |
| if errors: | |
| raise ValidationError(errors) | |
| super().clean() |
🤖 Prompt for AI Agents
In src/backend/core/models.py around lines 1584 to 1599, tighten
MessageTemplate.clean validation: add a check that if self.is_forced is True
then self.is_active must also be True and add an error on "is_forced" if not; if
self.raw_blob is set verify its owner equals the mailbox owner (when mailbox is
set) or equals the maildomain owner (when maildomain is set) and add an error on
"raw_blob" if it doesn't match; require at least one of html_body, text_body, or
raw_blob to be present/non-empty and add a "__all__" error if none are provided;
preserve existing mailbox/maildomain checks and raise ValidationError(errors) if
any errors exist, then call super().clean() at the end.
- Add MessageTemplate model with html_body and text_body fields - Add MessageTemplateViewSet with CRUD operations and render action - Add MessageTemplateAdmin with inlines for relationships - Add admin action for template duplication This commit introduces a complete email template system with support for both HTML and text content, flexible relationships with mailboxes and mail domains, and a RESTful API for template management.
Allow to manage maildomain's signature from the administration. Then in the Message composer allow the user to select a signature then render a preview in the MessageEditor
- remove the m2m relationships between MessageTemplate and maildomains + mailboxes - rename the current is_default to is_forced and fix the behaviour - harden _insert signature
sdemagny
force-pushed
the
sdem/proposition1_template
branch
from
a25b566 to
624178f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (23)
src/frontend/src/features/blocknote/signature-block/index.tsx (3)
104-125: Fix rules-of-hooks violation and sanitize HTML to prevent XSS.Move the hook into a child component and sanitize
preview.html_body. This also removes the eslint-disable.- render: ({ block : { props }}) => { - // eslint-disable-next-line react-hooks/rules-of-hooks - const { data: { data: preview = null } = {}, isLoading } = useMessageTemplatesRenderRetrieve(props.templateId, { - request: { - params: { - mailbox_id: props.mailboxId - } - } - }); - - if (isLoading) { - return <Spinner size="sm" />; - } - - if (!preview?.html_body) { - return null; - } - - return ( - <div dangerouslySetInnerHTML={{ __html: preview.html_body }} /> - ) - }, + render: ({ block: { props } }) => { + const SignaturePreview: React.FC<{ templateId: string; mailboxId?: string }> = ({ templateId, mailboxId }) => { + const { data: { data: preview = null } = {}, isLoading } = + useMessageTemplatesRenderRetrieve(templateId, { + request: { params: { mailbox_id: mailboxId } }, + }); + if (isLoading) return <Spinner size="sm" />; + if (!preview?.html_body) return null; + /* biome-ignore lint/security/noDangerouslySetInnerHtml: HTML is sanitized with DOMPurify. */ + return <div dangerouslySetInnerHTML={{ __html: DOMPurify.sanitize(preview.html_body) }} />; + }; + return <SignaturePreview templateId={props.templateId} mailboxId={props.mailboxId} />; + },
126-129: Emit a literal placeholder in toExternalHTML (avoid JSX escaping).Ensure the backend sees
<p><SignatureID>{id}</SignatureID></p>as raw HTML.- toExternalHTML: ({ block : { props }}) => { - // This will be parsed by the backend to insert the signature in the message body - return <p>{`<SignatureID>${props.templateId}</SignatureID>`}</p> - } + toExternalHTML: ({ block: { props } }) => { + // Must be literal HTML: <p><SignatureID>{id}</SignatureID></p> + const p = document.createElement("p"); + p.innerHTML = `<SignatureID>${props.templateId}</SignatureID>`; + return p; + }Please confirm the backend regex still matches exactly
<p><SignatureID>(.*?)</SignatureID></p>and thattoExternalHTMLruns client-side (document available).
1-7: Import DOMPurify for safe HTML rendering.import { createReactBlockSpec, useBlockNoteEditor, useComponentsContext, useEditorContentOrSelectionChange } from "@blocknote/react"; import { Icon, IconSize, Spinner } from "@gouvfr-lasuite/ui-kit"; import { useEffect, useState } from "react"; import { Props } from "@blocknote/core"; import { ReadOnlyMessageTemplate, useMessageTemplatesRenderRetrieve } from "@/features/api/gen"; import { MessageComposerBlockSchema, MessageComposerInlineContentSchema, MessageComposerStyleSchema, PartialMessageComposerBlockSchema } from "@/features/forms/components/message-composer"; +import DOMPurify from "dompurify";src/frontend/src/features/layouts/components/admin/signatures-view/signature-data-grid.tsx (2)
29-33: Add per-row delete spinner, error handling, and await cache invalidationCurrent delete flow uses a global isDeleting flag and lacks error handling. This can freeze all rows and silently fail. Track a per-row deletingId, wrap in try/catch, and await invalidateQueries to avoid stale UI.
@@ - const [selectedSignature, setSelectedSignature] = useState<ReadOnlyMessageTemplate | undefined>(); + const [selectedSignature, setSelectedSignature] = useState<ReadOnlyMessageTemplate | undefined>(); + const [deletingId, setDeletingId] = useState<string | null>(null); @@ - if (decision === 'delete') { - await deleteSignature({ id: signature.id }); - invalidateMessageTemplates(); - addToast( - <ToasterItem type="info"> - <span>{t("admin_maildomains_signature.toasts.delete")}</span> - </ToasterItem>, - ); - } + if (decision === 'delete') { + setDeletingId(signature.id); + try { + await deleteSignature({ id: signature.id }); + await invalidateMessageTemplates(); + addToast( + <ToasterItem type="info"> + <span>{t("admin_maildomains_signature.toasts.delete")}</span> + </ToasterItem>, + ); + } catch (e) { + addToast( + <ToasterItem type="error"> + <span>{t("admin_maildomains_signature.toasts.error_delete")}</span> + </ToasterItem>, + ); + } finally { + setDeletingId(null); + } + } @@ - icon={isDeleting ? <Spinner size="sm" /> : <Icon name="delete" size={IconSize.SMALL} />} + icon={deletingId === row.id ? <Spinner size="sm" /> : <Icon name="delete" size={IconSize.SMALL} />} onClick={() => handleDeleteRow(row)} - disabled={isDeleting} + disabled={deletingId === row.id}Also applies to: 45-59, 118-121
68-75: Don’t allow “Forced” on inactive signatures; gate the toggle and disable the cellPrevent forcing when a signature is inactive and reflect that in the UI. Add error toast on blocked action.
- const toggleDefault = async (signature: ReadOnlyMessageTemplate) => { - await updateSignature({ - id: signature.id, - data: { is_forced: !signature.is_forced, maildomain_id: domain.id }, - }); - invalidateMessageTemplates(); - addUpdateSucceededToast(); - } + const toggleDefault = async (signature: ReadOnlyMessageTemplate) => { + if (!signature.is_active) { + addToast(<ToasterItem type="error"><span>{t("admin_maildomains_signature.errors.cannot_force_inactive")}</span></ToasterItem>); + return; + } + try { + await updateSignature({ + id: signature.id, + data: { is_forced: !signature.is_forced, maildomain_id: domain.id }, + }); + await invalidateMessageTemplates(); + addUpdateSucceededToast(); + } catch { + addToast(<ToasterItem type="error"><span>{t("admin_maildomains_signature.toasts.error_update")}</span></ToasterItem>); + } + } @@ - <Checkbox checked={row.is_forced} onChange={() => toggleDefault(row)} disabled={isUpdating} /> + <Checkbox + checked={!!row.is_forced} + onChange={() => toggleDefault(row)} + disabled={!row.is_active || isUpdating} + />Also applies to: 91-96
src/frontend/src/features/i18n/translations.json (1)
170-172: Add composer toolbar strings (used by variables/signatures UI) and migrate hard-coded labelsThese are missing and block full i18n coverage for the composer toolbars.
"message_composer": { - "start_typing": "Start typing..." + "start_typing": "Start typing...", + "variables": "Variables", + "loading_variables": "Loading variables...", + "signatures": "Signatures", + "signature_item": "Signature: {{name}}" },"message_composer": { - "start_typing": "Commencer à écrire..." + "start_typing": "Commencer à écrire...", + "variables": "Variables", + "loading_variables": "Chargement des variables...", + "signatures": "Signatures", + "signature_item": "Signature : {{name}}" },Optionally, update UI components (inline-template-variable, signature-block, etc.) to consume these keys.
Also applies to: 675-677
src/backend/core/api/permissions.py (1)
414-432: Write-path checks look correct.Mailbox: EDITOR/SENDER/ADMIN; Maildomain: ADMIN. Good.
src/backend/core/api/serializers.py (5)
994-1021: Atomic content check is key-presence sensitive; current truthiness logic allows partial updates with empty strings.Make the rule purely key-based and fix the misleading comment.
- """Validate template data.""" - # exactly one maildomain_id or mailbox_id is required + """Validate template data.""" + # at least one of maildomain_id or mailbox_id is required on creation @@ - # For creation or when updating content fields - if "html_body" in attrs or "text_body" in attrs or "raw_blob" in attrs: - html_body = attrs.get("html_body", "") - text_body = attrs.get("text_body", "") - raw_blob = attrs.get("raw_blob", "") - - # If any content field is provided, all three must be provided together - # This ensures atomic updates and prevents inconsistent states - if html_body or text_body or raw_blob: - if not html_body or not text_body or not raw_blob: - raise serializers.ValidationError( - "When providing content fields, all three fields (html_body, text_body, raw_blob) " - "must be provided together to ensure consistency." - ) + # Atomic content update: if any of the keys is present, require all three keys. + atomic_fields = {"html_body", "text_body", "raw_blob"} + present = atomic_fields.intersection(attrs.keys()) + if present and present != atomic_fields: + missing = ", ".join(sorted(atomic_fields - present)) + raise serializers.ValidationError( + f"html_body, text_body and raw_blob must be updated together. Missing: {missing}." + )
1030-1049: Creation path can 500 on invalid/unknown IDs; wrap lookups and return field-scoped errors.- # Set mailbox or maildomain directly - # Update mailbox or maildomain if provided - if mailbox_id is not None and maildomain_id is not None: - # check if domain is the same as the mailbox domain - mailbox = models.Mailbox.objects.get(id=mailbox_id) - if mailbox.domain.id != maildomain_id: - raise serializers.ValidationError( - "Mailbox domain does not match maildomain." - ) - validated_data["mailbox"] = mailbox - validated_data["maildomain"] = models.MailDomain.objects.get( - id=maildomain_id - ) - elif mailbox_id is not None: - validated_data["mailbox"] = models.Mailbox.objects.get(id=mailbox_id) - elif maildomain_id is not None: - validated_data["maildomain"] = models.MailDomain.objects.get( - id=maildomain_id - ) + # Set mailbox or maildomain directly (with validation) + try: + if mailbox_id is not None and maildomain_id is not None: + mailbox = models.Mailbox.objects.get(id=mailbox_id) + if mailbox.domain_id != maildomain_id: + raise serializers.ValidationError("Mailbox domain does not match maildomain.") + validated_data["mailbox"] = mailbox + validated_data["maildomain"] = models.MailDomain.objects.get(id=maildomain_id) + elif mailbox_id is not None: + validated_data["mailbox"] = models.Mailbox.objects.get(id=mailbox_id) + elif maildomain_id is not None: + validated_data["maildomain"] = models.MailDomain.objects.get(id=maildomain_id) + except (models.Mailbox.DoesNotExist, ValueError): + raise serializers.ValidationError({"mailbox_id": "Mailbox not found"}) + except (models.MailDomain.DoesNotExist, ValueError): + raise serializers.ValidationError({"maildomain_id": "Maildomain not found"})
1050-1061: Validate raw_blob JSON before creating Blob; otherwise invalid JSON gets stored as application/json.with transaction.atomic(): # Create raw_blob relationship if content provided if raw_blob_content: + # Validate JSON content + import json + try: + json.loads(raw_blob_content) + except json.JSONDecodeError as e: + raise serializers.ValidationError({"raw_blob": f"Invalid JSON: {e.msg}"}) blob = models.Blob.objects.create_blob( content=raw_blob_content.encode("utf-8"), content_type="application/json", mailbox_id=mailbox_id, maildomain_id=maildomain_id, ) validated_data["raw_blob"] = blob
1071-1091: Update path validates relationships but does not apply them; also missing DoesNotExist handling.Assign the resolved FKs into validated_data and surface field-specific errors.
# Update mailbox or maildomain if provided (validate consistency first) if mailbox_id is not None and maildomain_id is not None: @@ raise serializers.ValidationError( "Mailbox domain does not match maildomain." ) - elif mailbox_id is not None and instance.maildomain: + elif mailbox_id is not None and instance.maildomain: mailbox = models.Mailbox.objects.get(id=mailbox_id) if instance.maildomain != mailbox.domain: raise serializers.ValidationError( "Mailbox domain does not match maildomain." ) elif maildomain_id is not None and instance.mailbox: maildomain = models.MailDomain.objects.get(id=maildomain_id) if instance.mailbox.domain != maildomain: raise serializers.ValidationError( "Mailbox domain does not match maildomain." ) + + # Apply relationship updates + try: + if mailbox_id is not None: + validated_data["mailbox"] = models.Mailbox.objects.get(id=mailbox_id) + if maildomain_id is not None: + validated_data["maildomain"] = models.MailDomain.objects.get(id=maildomain_id) + except (models.Mailbox.DoesNotExist, ValueError): + raise serializers.ValidationError({"mailbox_id": "Mailbox not found"}) + except (models.MailDomain.DoesNotExist, ValueError): + raise serializers.ValidationError({"maildomain_id": "Maildomain not found"})
1092-1115: Blob update: validate JSON and ensure ownership (fallback to instance owners when IDs not provided).Without fallback, you can create ownerless blobs.
with transaction.atomic(): # Handle raw_blob update first if raw_blob_content is not None: try: if instance.raw_blob: instance.raw_blob.delete() except models.Blob.DoesNotExist: pass if raw_blob_content: # Only create blob if content is not empty - blob = models.Blob.objects.create_blob( + # Validate JSON content + import json + try: + json.loads(raw_blob_content) + except json.JSONDecodeError as e: + raise serializers.ValidationError({"raw_blob": f"Invalid JSON: {e.msg}"}) + # Determine blob ownership + owner_mailbox_id = mailbox_id if mailbox_id is not None else instance.mailbox_id + owner_maildomain_id = ( + maildomain_id if maildomain_id is not None else instance.maildomain_id + ) + blob = models.Blob.objects.create_blob( content=raw_blob_content.encode("utf-8"), content_type="application/json", - mailbox_id=mailbox_id, - maildomain_id=maildomain_id, + mailbox_id=owner_mailbox_id, + maildomain_id=owner_maildomain_id, ) validated_data["raw_blob"] = blob else: validated_data["raw_blob"] = Nonesrc/backend/core/api/openapi.json (5)
2771-2797: Add typed query parameters for message-templates list.Filters are documented in the description but not declared as OpenAPI parameters, which breaks client generation and server-side validation hints. Add explicit query params.
"/api/v1.0/message-templates/": { "get": { "operationId": "message_templates_list", "description": "ViewSet for managing message templates and signatures.\n\nThis ViewSet provides endpoints for creating, reading, updating, and deleting\nmessage templates and signatures. Templates can be used for replies, new messages,\nand signatures.\n\nFiltering:\n- type: Filter by template type (reply, new_message, signature)\n- is_active: Filter by active status (true/false)\n- mailbox_id: Filter by mailbox UUID\n- maildomain_id: Filter by mail domain UUID\n- is_forced: Filter by forced status (true/false) - works with mailbox or maildomain", + "parameters": [ + { "in": "query", "name": "type", "schema": { "$ref": "#/components/schemas/MessageTemplateTypeChoices" }, "description": "Filter by template type." }, + { "in": "query", "name": "is_active", "schema": { "type": "boolean" }, "description": "Filter by active status." }, + { "in": "query", "name": "mailbox_id", "schema": { "type": "string", "format": "uuid" }, "description": "Filter by mailbox UUID." }, + { "in": "query", "name": "maildomain_id", "schema": { "type": "string", "format": "uuid" }, "description": "Filter by mail domain UUID." }, + { "in": "query", "name": "is_forced", "schema": { "type": "boolean" }, "description": "Filter by forced status within scope." } + ], "tags": [ "message-templates" ],
2999-3044: Render endpoint can’t accept context via GET; switch to POST with a requestBody.GET bodies are not portable and there’s no requestBody to pass variables. Use POST and define the payload.
- "/api/v1.0/message-templates/{id}/render/": { - "get": { - "operationId": "message_templates_render_retrieve", - "description": "Render a template with the provided context variables.", + "/api/v1.0/message-templates/{id}/render/": { + "post": { + "operationId": "message_templates_render_create", + "description": "Render a template. If context variables are provided, they are applied.", "parameters": [ { "in": "path", "name": "id", - "schema": { - "type": "string" - }, + "schema": { "type": "string", "format": "uuid" }, "required": true } ], "tags": [ "message-templates" ], + "requestBody": { + "required": false, + "content": { + "application/json": { + "schema": { + "type": "object", + "properties": { + "context": { "type": "object", "additionalProperties": true, "description": "Template variables." } + } + } + } + } + }, "security": [ { "cookieAuth": [] } ], "responses": { "200": { "content": { "application/json": { "schema": { "type": "object", "properties": { "html_body": { "type": "string" }, "text_body": { "type": "string" } } } } }, "description": "Template rendered with provided context" }, + "400": { "description": "Invalid context." }, "404": { "description": "Template not found" } } } },
5217-5278: Fix raw_blob typing and clarify is_forced semantics in MessageTemplate schema.raw_blob should be a Blob FK UUID, not arbitrary text. Align description; improve is_forced wording.
"MessageTemplate": { "type": "object", "description": "Serialize message templates for POST/PUT/PATCH operations.", "properties": { "raw_blob": { - "type": "string", - "description": "Raw blob content" + "type": "string", + "format": "uuid", + "description": "Blob ID containing the raw template content" }, "is_forced": { "type": "boolean", "default": false, - "description": "Set as forced template" + "description": "Force usage of this template within its scope (mailbox or maildomain)" },
5279-5331: Enforce html/text/raw all-or-none; type IDs as UUID; mailbox/maildomain exclusivity.This aligns with PR goals and prevents inconsistent payloads.
"MessageTemplateRequest": { "type": "object", "description": "Serialize message templates for POST/PUT/PATCH operations.", "properties": { "raw_blob": { - "type": "string", - "minLength": 1, - "description": "Raw blob content" + "type": "string", + "format": "uuid", + "minLength": 1, + "description": "Blob ID containing the raw template content" }, "mailbox_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Mailbox UUID" }, "maildomain_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Maildomain UUID" } }, - "required": [ - "name", - "type" - ] + "required": ["name","type","raw_blob","html_body","text_body"], + "oneOf": [ + { "required": ["mailbox_id"] }, + { "required": ["maildomain_id"] } + ] },Optionally add a schema-level rule to disallow both mailbox_id and maildomain_id simultaneously (the oneOf handles it for most generators).
5581-5629: PATCH contract: require html/text/raw trio together when any one is present; add UUID formats.Keeps partial updates coherent with create/update rules.
"PatchedMessageTemplateRequest": { "type": "object", "description": "Serialize message templates for POST/PUT/PATCH operations.", "properties": { "raw_blob": { - "type": "string", - "minLength": 1, - "description": "Raw blob content" + "type": "string", + "format": "uuid", + "minLength": 1, + "description": "Blob ID containing the raw template content" }, "mailbox_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Mailbox UUID" }, "maildomain_id": { - "type": "string", + "type": "string", + "format": "uuid", "writeOnly": true, "minLength": 1, "description": "Maildomain UUID" } - } + }, + "oneOf": [ + { "not": { "anyOf": [ { "required": ["raw_blob"] }, { "required": ["html_body"] }, { "required": ["text_body"] } ] } }, + { "required": ["raw_blob","html_body","text_body"] } + ] },src/frontend/src/features/utils/mail-helper.tsx (1)
44-46: Make wrapper stripping robust to attribute order and trailing whitespace.Current regex misses cases with extra attributes or reordered attributes. Capture inner HTML safely.
- return renderToString(<Markdown>{markdown}</Markdown>) - .replace(/(^<div data-id="react-email-markdown">|<\/div>$)/g, '') - .trim(); + const html = renderToString(<Markdown>{markdown}</Markdown>); + return html + .replace(/^<div[^>]*\bdata-id=(["'])react-email-markdown\1[^>]*>([\s\S]*?)<\/div>\s*$/i, '$2') + .trim();src/frontend/src/features/forms/components/message-form/index.tsx (1)
540-546: Pass reply/forward mode to composer so quoted block mode is correct.Composer currently hardcodes quoted block mode to “forward”; replies render incorrectly.
- <MessageComposer + <MessageComposer mailboxId={defaultSenderId} defaultValue={form.getValues('messageDraftBody')} fullWidth state={form.formState.errors?.messageDraftBody ? "error" : "default"} text={form.formState.errors?.messageDraftBody?.message} quotedMessage={mode !== "new" ? parentMessage : undefined} + quotedMode={mode} disabled={!canWriteMessages} />Additionally update MessageComposer to accept and use quotedMode:
// src/frontend/src/features/forms/components/message-composer/index.tsx export const MessageComposer = ({ quotedMode = "reply", ... }: MessageComposerProps & { quotedMode?: "new"|"reply"|"reply_all"|"forward" }) => { const getInitialContent = () => { const initialContent = /* existing logic */; if (!quotedMessage) return initialContent; const mode = quotedMode === "forward" ? "forward" : "reply"; return initialContent.concat([{ type: "quoted-message", content: undefined, props: { mode, /* ... */ } }]); }; };src/backend/core/mda/outbound.py (4)
117-213: Replace-at-most-one + HTML-first misses placeholders; support both bodies and multiple occurrences; pass sender mailbox.
The current logic: (1) processes only the first match; (2) skips text when html exists; (3) requires strict wrappers; (4) lacks sender scoping to _get_signatures.Apply:
-def _insert_signature( - text_body: str, html_body: str, user: models.User -) -> tuple[str, str]: - """Replace the signature in the text and html bodies. +def _insert_signature( + text_body: str, html_body: str, user: models.User, sender_mailbox: models.Mailbox +) -> tuple[str, str]: + """Replace signature placeholders in both bodies (supports multiple occurrences). @@ - # Handle signature by replacing the `<SignatureID>` with the signature - match = None - if html_body: - match = re.search(r"<p><SignatureID>(.*?)</SignatureID></p>", html_body) - elif text_body: - match = re.search(r"\n<SignatureID>(.*?)</SignatureID>\n", text_body) - - if match: - signature_id = match.group(1) - - # Additional validation: ensure signature_id contains only safe characters - if not _is_valid_uuid(signature_id): - logger.warning( - "User %s attempted to use malformed signature ID: %s", - user.id, - signature_id, - ) - # Remove invalid signature placeholders for security - if text_body: - text_body = re.sub( - r"\n<SignatureID>" + re.escape(signature_id) + r"</SignatureID>\n", - "\n", - text_body, - count=1, - ) - if html_body: - html_body = re.sub( - r"<p><SignatureID>" - + re.escape(signature_id) - + r"</SignatureID></p>", - "", - html_body, - count=1, - ) - return text_body, html_body - - signatures = _get_signatures(signature_id, user) - if signatures: - # Use regex-based replacement to ensure we only replace the exact matched pattern - # This prevents injection attacks by only replacing the specific SignatureID tag we found - if text_body: - text_body = re.sub( - r"\n<SignatureID>" + re.escape(signature_id) + r"</SignatureID>\n", - "\n" + signatures["text_body"] + "\n", - text_body, - count=1, # Only replace the first occurrence - ) - if html_body: - html_body = re.sub( - r"<p><SignatureID>" - + re.escape(signature_id) - + r"</SignatureID></p>", - "<p>" + signatures["html_body"] + "</p>", - html_body, - count=1, # Only replace the first occurrence - ) - - logger.debug( - "Successfully inserted signature %s for user %s", signature_id, user.id - ) - else: - # Remove invalid signature placeholders for security - if text_body: - text_body = re.sub( - r"\n<SignatureID>" + re.escape(signature_id) + r"</SignatureID>\n", - "\n", - text_body, - count=1, - ) - if html_body: - html_body = re.sub( - r"<p><SignatureID>" - + re.escape(signature_id) - + r"</SignatureID></p>", - "", - html_body, - count=1, - ) - logger.warning( - "Failed to get signature %s for user %s, placeholder removed", - signature_id, - user.id, - ) - - return text_body, html_body + # Patterns accept optional <p> wrapper/whitespace (HTML) and raw tag (text) + html = html_body or "" + text = text_body or "" + html_pat = re.compile( + r"(?:<p[^>]*>\s*)?<SignatureID>(?P<ID>[^<]+)</SignatureID>(?:\s*</p>)?", + re.IGNORECASE, + ) + text_pat = re.compile(r"<SignatureID>(?P<ID>[^<]+)</SignatureID>", re.IGNORECASE) + + cache: dict[str, Optional[dict[str, str]]] = {} + + def resolve(sig_id: str) -> Optional[dict[str, str]]: + if sig_id not in cache: + if not _is_valid_uuid(sig_id): + logger.warning("User %s malformed SignatureID %s", user.id, sig_id) + cache[sig_id] = None + else: + cache[sig_id] = _get_signatures(sig_id, user, sender_mailbox) + return cache[sig_id] + + def html_repl(m: re.Match) -> str: + sig_id = m.group("ID") + sig = resolve(sig_id) + if not sig: + logger.warning("Signature %s not usable for user %s; stripping", sig_id, user.id) + return "" + return sig["html_body"] or "" + + def text_repl(m: re.Match) -> str: + sig_id = m.group("ID") + sig = resolve(sig_id) + if not sig: + return "" + return sig["text_body"] or "" + + if html: + html = html_pat.sub(html_repl, html) + if text: + text = text_pat.sub(text_repl, text) + + return text if text_body is not None else None, html if html_body is not None else None
241-244: Pass sender mailbox to signature replacer.
Needed for access scoping in _get_signatures.- text_body, html_body = _insert_signature(text_body, html_body, user) + text_body, html_body = _insert_signature(text_body, html_body, user, mailbox_sender)
220-221: Update prepare_outbound_message call in send viewset
At src/backend/core/api/viewsets/send.py:131, passhtml_bodyanduseraftertext_body—e.g.- prepare_outbound_message(mailbox_sender, message, request.data.get("textBody")) + prepare_outbound_message( + mailbox_sender, + message, + request.data.get("textBody"), + request.data.get("htmlBody"), + request.user + )
52-115: Scope signatures to the sender mailbox and enforce SIGNATURE type.
Currently any template the user can access may be injected, crossing mailbox/domain boundaries. Enforce type == SIGNATURE and that the template belongs to the sender mailbox or its domain.Apply:
-from core.enums import MessageDeliveryStatusChoices +from core.enums import MessageDeliveryStatusChoices, MessageTemplateTypeChoices-def _get_signatures(signature_id: str, user: models.User) -> Optional[dict[str, str]]: +def _get_signatures( + signature_id: str, user: models.User, sender_mailbox: models.Mailbox +) -> Optional[dict[str, str]]: @@ - signature = models.MessageTemplate.objects.get(id=signature_id) + signature = models.MessageTemplate.objects.get(id=signature_id) @@ - # Check if user has access to this signature template - if not signature.is_active: + # Check type and activation + if ( + not signature.is_active + or signature.type != MessageTemplateTypeChoices.SIGNATURE + ): logger.warning( - "User %s attempted to access inactive signature %s", + "User %s attempted to use non-signature or inactive template %s", user.id, signature_id, ) return None @@ - # Verify user has access to the mailbox or maildomain associated with this template - user_has_access = False - - if signature.mailbox: - # Check if user has access to the mailbox - if signature.mailbox.accesses.filter(user=user).exists(): - user_has_access = True - elif signature.maildomain: - # Check if user has access to the maildomain - if signature.maildomain.accesses.filter(user=user).exists(): - user_has_access = True - # Also check if user has access through mailbox access to this domain - elif signature.maildomain.mailbox_set.filter(accesses__user=user).exists(): - user_has_access = True - - if not user_has_access: + # Enforce scope: signature must belong to the sender mailbox or its domain + in_scope = ( + (signature.mailbox_id is not None and signature.mailbox_id == sender_mailbox.id) + or (signature.maildomain_id is not None and signature.maildomain_id == sender_mailbox.domain_id) + ) + if not in_scope: logger.warning( - "User %s attempted to access unauthorized signature %s", + "User %s attempted to use out-of-scope signature %s", user.id, signature_id, ) return NoneIf you also want a user-level guard: additionally assert sender_mailbox.accesses.filter(user=user).exists() before rendering.
🧹 Nitpick comments (17)
src/frontend/src/styles/main.scss (2)
54-56: Consider code-splitting BlockNote styles to reduce global CSS payload.
If the editor isn’t used on most pages, import these in the feature/page bundle instead of main.scss.Apply this diff if you choose to defer-load via the editor feature entrypoint:
-// Blocknote custom blocks and components -@use "./../features/blocknote/blocknote-view-field"; -@use "./../features/blocknote/inline-template-variable";
54-56: Scope BlockNote selectors under a single root class to avoid global bleed.
Verify that these partials nest under a namespaced root (e.g., .bn-editor or .signature-editor). If not, wrap them.src/backend/core/migrations/0007_alter_messagetemplate_type_blob_blob_has_owner_and_more.py (2)
14-17: Add a DB-level CHECK fortype(defense-in-depth).Choices validate at the app layer only. Add a check constraint so direct DB writes can’t insert invalid
typevalues.Apply this diff:
@@ migrations.AlterField( model_name='messagetemplate', name='type', field=models.SmallIntegerField(choices=[(1, 'reply'), (2, 'new_message'), (3, 'signature')], default=1, help_text='Type of template (reply, new_message, signature)', verbose_name='type'), ), + migrations.AddConstraint( + model_name='messagetemplate', + constraint=models.CheckConstraint( + name='messagetemplate_type_valid', + condition=models.Q(('type__in', [1, 2, 3])), + ), + ),
27-33: Revisit partial UNIQUE scope to match “forced” semantics.As written, the domain-level UNIQUE (on (maildomain, type) WHERE is_forced) will also catch rows that have a mailbox set, effectively permitting only one forced template per domain/type total. If you want multiple mailboxes in the same domain to each have their own forced template (and only block duplicates within the same mailbox), restrict each UNIQUE to its scope:
Apply this diff if that’s the intended behavior:
@@ - migrations.AddConstraint( - model_name='messagetemplate', - constraint=models.UniqueConstraint(condition=models.Q(('is_forced', True)), fields=('mailbox', 'type'), name='uniq_forced_template_mailbox_type'), - ), + migrations.AddConstraint( + model_name='messagetemplate', + constraint=models.UniqueConstraint( + condition=models.Q(('is_forced', True), ('mailbox__isnull', False)), + fields=('mailbox', 'type'), + name='uniq_forced_template_mailbox_type', + ), + ), @@ - migrations.AddConstraint( - model_name='messagetemplate', - constraint=models.UniqueConstraint(condition=models.Q(('is_forced', True)), fields=('maildomain', 'type'), name='uniq_forced_template_maildomain_type'), - ), + migrations.AddConstraint( + model_name='messagetemplate', + constraint=models.UniqueConstraint( + condition=models.Q(('is_forced', True), ('mailbox__isnull', True)), + fields=('maildomain', 'type'), + name='uniq_forced_template_maildomain_type', + ), + ),If, instead, a domain-level forced template must preclude any mailbox-level forced templates in that domain, keep the current version.
src/frontend/src/features/blocknote/signature-block/index.tsx (6)
23-25: Fix misleading comment and keep naming consistent.- // Tracks whether the text & background are both blue. + // Tracks the currently selected signature template ID. const [isSelected, setIsSelected] = useState<string>();
34-41: Add missing deps to useEffect (or explicitly justify skip).- useEffect(() => { + useEffect(() => { if(!isSelected) { const signatureBlock = editor.getBlock('signature'); if (signatureBlock) { setIsSelected((signatureBlock.props as BlockSignatureConfigProps).templateId); } } - }, []); + }, [editor, isSelected]);
67-79: Prefer appending the signature at the end of the document instead of after the first block.This avoids surprising placement in non-empty editors.
} else { - editor.insertBlocks( - [{ id: "signature", type: "signature", props: { templateId: template.id, mailboxId: mailboxId } }] as unknown as PartialMessageComposerBlockSchema[], - editor.document[0].id, - "after" - ); + const lastIdx = Math.max(0, editor.document.length - 1); + const anchorId = editor.document[lastIdx].id; + editor.insertBlocks( + [{ id: "signature", type: "signature", props: { templateId: template.id, mailboxId } }] as unknown as PartialMessageComposerBlockSchema[], + anchorId, + "after" + ); }
67-76: Avoid hardcoded block ID "signature" to reduce collision risk.Using a fixed ID assumes uniqueness. Consider letting the editor assign an ID, or generate a namespaced ID (e.g.,
signature-${mailboxId || 'global'}). If you keep the fixed ID, ensure no other blocks share it.
91-102: Drop unused prop "username" from the block schema (YAGNI).Not used in render/export; remove to simplify the schema.
propSchema: { templateId: { default: "" }, mailboxId: { default: "" }, - username: { default: "" }, }
51-66: Minor UX: add aria-label and localize labels.
- Provide an accessible label on the Select.
- Replace hardcoded English with i18n keys if available.
- <Components.FormattingToolbar.Select + <Components.FormattingToolbar.Select + aria-label="Choose signature" key={"templateVariableSelector"} @@ - text: "Signatures", + text: "Signatures", @@ - text: `Signature : ${template.name}`, + text: `Signature : ${template.name}`,src/frontend/src/features/layouts/components/admin/signatures-view/signature-data-grid.tsx (1)
81-85: Coalesce optional booleans and add ARIA labels for accessibilityAvoid passing undefined to Checkbox.checked, and add aria-labels to improve SR usability.
- <Checkbox checked={row.is_active} onChange={() => toggleActive(row)} disabled={isUpdating} /> + <Checkbox + checked={!!row.is_active} + onChange={() => toggleActive(row)} + disabled={isUpdating} + aria-label={t("admin_maildomains_signature.datagrid_headers.is_active")} + /> @@ - <Checkbox checked={row.is_forced} onChange={() => toggleDefault(row)} disabled={isUpdating} /> + <Checkbox + checked={!!row.is_forced} + onChange={() => toggleDefault(row)} + disabled={!row.is_active || isUpdating} + aria-label={t("admin_maildomains_signature.datagrid_headers.is_forced")} + />Also applies to: 91-96
src/backend/core/api/permissions.py (1)
377-387: Pre-check list access against user roles to avoid enumerating unrelated IDs.Small optimization: return False when the user lacks access to the requested mailbox/domain instead of relying only on queryset filtering.
if view.action == "list": mailbox_id = request.query_params.get("mailbox_id") maildomain_id = request.query_params.get("maildomain_id") if not mailbox_id and not maildomain_id: return False - - # Allow access to list action - the ViewSet will filter results based on user permissions - # This allows users to see empty lists when they don't have access to the specified mailbox/domain - return True + if mailbox_id: + return models.MailboxAccess.objects.filter( + mailbox_id=mailbox_id, user=request.user + ).exists() + if maildomain_id: + return models.MailDomainAccess.objects.filter( + maildomain_id=maildomain_id, + user=request.user, + role=models.MailDomainAccessRoleChoices.ADMIN, + ).exists()src/backend/core/api/serializers.py (1)
955-960: Use UUIDField for IDs to validate early and avoid ValueError on .get() calls.- mailbox_id = serializers.CharField( + mailbox_id = serializers.UUIDField( required=False, write_only=True, help_text="Mailbox UUID" ) - maildomain_id = serializers.CharField( + maildomain_id = serializers.UUIDField( required=False, write_only=True, help_text="Maildomain UUID" )src/backend/core/api/openapi.json (3)
2843-2851: Declare path parameter id as UUID across message-templates/{id} endpoints.Path ids are typed as plain strings; set format to uuid for accurate contracts and client typings.
"parameters": [ { "in": "path", "name": "id", - "schema": { - "type": "string" - }, + "schema": { "type": "string", "format": "uuid" }, "required": true } ],Apply the same change in GET/PUT/PATCH/DELETE variants on this path.
Also applies to: 2876-2885, 2926-2934, 2974-2982
2840-2842: Trim list-only “Filtering” notes from item endpoints’ descriptions.These descriptions repeat list filters on per-id operations and can confuse API consumers.
No diff provided to keep noise low; please drop the filter bullets from the four item operations’ descriptions.
Also applies to: 2875-2876, 2924-2925, 2971-2972
5649-5712: Tighten ReadOnlyMessageTemplate: UUID format for raw_blob; don’t require nullable field.raw_blob is nullable but listed as required, which is inconsistent. Also add uuid format and refine is_forced description.
"ReadOnlyMessageTemplate": { "type": "object", "description": "Serialize message templates for read-only operations.", "properties": { "raw_blob": { - "type": "string", - "nullable": true, - "description": "Get raw blob.", - "readOnly": true + "type": "string", + "format": "uuid", + "nullable": true, + "description": "Blob ID containing the raw template content (if present).", + "readOnly": true }, "is_forced": { "type": "boolean", - "description": "Whether this template is forced; no other template of the same type can be used in the same scope" + "description": "Force usage of this template within its scope (mailbox or maildomain)" } }, "required": [ "created_at", "id", "name", - "raw_blob", "type", "updated_at" ] },src/frontend/src/features/forms/components/message-form/index.tsx (1)
200-203: Detect “forgot attachments” from text body, not the JSON draft.Scanning the BlockNote JSON can lead to misses; check the plain text for keywords.
- const showAttachmentsForgetAlert = useMemo(() => { - return MailHelper.areAttachmentsMentionedInDraft(messageDraftBody) && attachments.length === 0 && driveAttachments.length === 0; - }, [messageDraftBody, attachments, driveAttachments]); + const messageTextBody = useWatch({ control: form.control, name: "messageTextBody" }) || ""; + const showAttachmentsForgetAlert = useMemo(() => { + return MailHelper.areAttachmentsMentionedInDraft(messageTextBody) && attachments.length === 0 && driveAttachments.length === 0; + }, [messageTextBody, attachments, driveAttachments]);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (9)
src/frontend/src/features/api/gen/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/message-templates/message-templates.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message_template.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message_template_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message_template_type_choices.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message_templates_render_retrieve200.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_message_template_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/read_only_message_template.tsis excluded by!**/gen/**
📒 Files selected for processing (47)
src/backend/core/admin.py(1 hunks)src/backend/core/api/openapi.json(4 hunks)src/backend/core/api/permissions.py(1 hunks)src/backend/core/api/serializers.py(2 hunks)src/backend/core/api/viewsets/message_template.py(1 hunks)src/backend/core/api/viewsets/send.py(1 hunks)src/backend/core/enums.py(1 hunks)src/backend/core/factories.py(1 hunks)src/backend/core/mda/outbound.py(3 hunks)src/backend/core/migrations/0006_blob_maildomain_alter_blob_mailbox_and_more.py(1 hunks)src/backend/core/migrations/0007_alter_messagetemplate_type_blob_blob_has_owner_and_more.py(1 hunks)src/backend/core/migrations/0008_alter_messagetemplate_is_forced_and_more.py(1 hunks)src/backend/core/models.py(6 hunks)src/backend/core/tests/api/test_message_template.py(1 hunks)src/backend/core/tests/api/test_send_message_signature.py(1 hunks)src/backend/core/tests/mda/test_outbound.py(2 hunks)src/backend/core/urls.py(2 hunks)src/frontend/src/features/blocknote/blocknote-view-field/_index.scss(1 hunks)src/frontend/src/features/blocknote/blocknote-view-field/index.tsx(1 hunks)src/frontend/src/features/blocknote/inline-template-variable/_index.scss(1 hunks)src/frontend/src/features/blocknote/inline-template-variable/index.tsx(1 hunks)src/frontend/src/features/blocknote/signature-block/index.tsx(1 hunks)src/frontend/src/features/blocknote/toolbar.tsx(2 hunks)src/frontend/src/features/forms/components/message-composer/index.tsx(1 hunks)src/frontend/src/features/forms/components/message-editor/index.tsx(0 hunks)src/frontend/src/features/forms/components/message-form/index.tsx(9 hunks)src/frontend/src/features/i18n/translations.json(6 hunks)src/frontend/src/features/layouts/components/admin/admin-layout.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/mailboxes-view/page-content.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/modal-compose-signature/_index.scss(1 hunks)src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/modal-compose-signature/signature-composer.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/page-content.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/signatures-view/_index.scss(1 hunks)src/frontend/src/features/layouts/components/admin/signatures-view/compose-signature-action.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/signatures-view/page-content.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/signatures-view/signature-data-grid.tsx(1 hunks)src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss(0 hunks)src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/index.tsx(1 hunks)src/frontend/src/features/layouts/components/main/index.tsx(0 hunks)src/frontend/src/features/layouts/components/main/layout/index.tsx(2 hunks)src/frontend/src/features/utils/mail-helper.test.tsx(1 hunks)src/frontend/src/features/utils/mail-helper.tsx(1 hunks)src/frontend/src/pages/_app.tsx(1 hunks)src/frontend/src/pages/domain/[maildomainId]/signatures.tsx(1 hunks)src/frontend/src/styles/globals.scss(1 hunks)src/frontend/src/styles/main.scss(2 hunks)
💤 Files with no reviewable changes (3)
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss
- src/frontend/src/features/layouts/components/main/index.tsx
- src/frontend/src/features/forms/components/message-editor/index.tsx
✅ Files skipped from review due to trivial changes (1)
- src/frontend/src/features/blocknote/inline-template-variable/_index.scss
🚧 Files skipped from review as they are similar to previous changes (31)
- src/frontend/src/features/layouts/components/admin/signatures-view/compose-signature-action.tsx
- src/frontend/src/features/layouts/components/admin/admin-layout.tsx
- src/frontend/src/features/layouts/components/admin/signatures-view/page-content.tsx
- src/frontend/src/features/layouts/components/main/layout/index.tsx
- src/frontend/src/features/forms/components/message-composer/index.tsx
- src/frontend/src/features/blocknote/toolbar.tsx
- src/frontend/src/features/blocknote/inline-template-variable/index.tsx
- src/frontend/src/features/layouts/components/admin/page-content.tsx
- src/frontend/src/styles/globals.scss
- src/backend/core/enums.py
- src/frontend/src/pages/_app.tsx
- src/frontend/src/features/layouts/components/admin/modal-compose-signature/_index.scss
- src/backend/core/api/viewsets/send.py
- src/backend/core/migrations/0008_alter_messagetemplate_is_forced_and_more.py
- src/frontend/src/pages/domain/[maildomainId]/signatures.tsx
- src/frontend/src/features/layouts/components/admin/modal-compose-signature/signature-composer.tsx
- src/frontend/src/features/blocknote/blocknote-view-field/index.tsx
- src/backend/core/tests/api/test_send_message_signature.py
- src/frontend/src/features/blocknote/blocknote-view-field/_index.scss
- src/backend/core/tests/api/test_message_template.py
- src/backend/core/admin.py
- src/backend/core/tests/mda/test_outbound.py
- src/backend/core/factories.py
- src/frontend/src/features/utils/mail-helper.test.tsx
- src/backend/core/api/viewsets/message_template.py
- src/frontend/src/features/layouts/components/admin/signatures-view/_index.scss
- src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx
- src/backend/core/migrations/0006_blob_maildomain_alter_blob_mailbox_and_more.py
- src/backend/core/urls.py
- src/backend/core/models.py
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/index.tsx
🧰 Additional context used
🧬 Code graph analysis (7)
src/backend/core/api/serializers.py (2)
src/backend/core/enums.py (1)
MessageTemplateTypeChoices(108-113)src/backend/core/models.py (25)
get_content(1306-1320)Meta(85-86)Meta(195-198)Meta(277-280)Meta(441-446)Meta(568-572)Meta(596-599)Meta(776-781)Meta(948-952)Meta(969-973)Meta(1010-1014)Meta(1069-1073)Meta(1282-1294)Meta(1352-1356)Meta(1392-1396)Meta(1446-1450)Meta(1540-1566)MessageTemplate(1465-1643)Mailbox(412-550)MailDomain(237-409)Blob(1227-1320)create_blob(466-492)create_blob(1162-1224)content_type(1362-1364)delete(910-930)
src/frontend/src/features/layouts/components/admin/signatures-view/signature-data-grid.tsx (6)
src/frontend/src/features/api/gen/message-templates/message-templates.ts (3)
useMessageTemplatesList(184-212)useMessageTemplatesPartialUpdate(725-749)useMessageTemplatesDestroy(838-861)src/frontend/src/features/api/gen/models/message_template_type_choices.ts (2)
MessageTemplateTypeChoices(9-10)MessageTemplateTypeChoices(13-17)src/frontend/src/features/api/gen/models/read_only_message_template.ts (1)
ReadOnlyMessageTemplate(13-39)src/frontend/src/features/ui/components/toaster/index.tsx (2)
addToast(63-76)ToasterItem(16-61)src/frontend/src/features/ui/components/banner/index.tsx (1)
Banner(14-41)src/frontend/src/features/layouts/components/admin/modal-compose-signature/index.tsx (1)
ModalComposeSignature(23-56)
src/backend/core/migrations/0007_alter_messagetemplate_type_blob_blob_has_owner_and_more.py (1)
src/backend/core/migrations/0008_alter_messagetemplate_is_forced_and_more.py (1)
Migration(6-26)
src/backend/core/api/permissions.py (2)
src/backend/core/enums.py (2)
MailboxRoleChoices(15-21)MailDomainAccessRoleChoices(48-51)src/backend/core/models.py (2)
MailboxAccess(553-575)MailDomainAccess(1377-1399)
src/frontend/src/features/blocknote/signature-block/index.tsx (3)
src/frontend/src/features/api/gen/models/read_only_message_template.ts (1)
ReadOnlyMessageTemplate(13-39)src/frontend/src/features/forms/components/message-composer/index.tsx (4)
MessageComposerBlockSchema(26-26)MessageComposerInlineContentSchema(27-27)MessageComposerStyleSchema(28-28)PartialMessageComposerBlockSchema(29-29)src/frontend/src/features/api/gen/message-templates/message-templates.ts (1)
useMessageTemplatesRenderRetrieve(1022-1054)
src/frontend/src/features/forms/components/message-form/index.tsx (1)
src/frontend/src/features/forms/components/message-composer/index.tsx (1)
MessageComposer(48-167)
src/backend/core/mda/outbound.py (1)
src/backend/core/models.py (5)
User(123-234)MessageTemplate(1465-1643)render_template(1600-1633)Mailbox(412-550)Message(1020-1156)
🪛 ast-grep (0.38.6)
src/frontend/src/features/blocknote/signature-block/index.tsx
[warning] 122-122: Usage of dangerouslySetInnerHTML detected. This bypasses React's built-in XSS protection. Always sanitize HTML content using libraries like DOMPurify before injecting it into the DOM to prevent XSS attacks.
Context: dangerouslySetInnerHTML
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://reactjs.org/docs/dom-elements.html#dangerouslysetinnerhtml
- https://cwe.mitre.org/data/definitions/79.html
(react-unsafe-html-injection)
🪛 Biome (2.1.2)
src/frontend/src/features/blocknote/signature-block/index.tsx
[error] 123-123: Avoid passing content using the dangerouslySetInnerHTML prop.
Setting content using code can expose users to cross-site scripting (XSS) attacks
(lint/security/noDangerouslySetInnerHtml)
⏰ 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). (3)
- GitHub Check: check-api-state
- GitHub Check: build-front
- GitHub Check: test-back
Comment on lines
+458
to
+500
| "admin_maildomains_signature": { | ||
| "loading": "Loading signatures...", | ||
| "no_signatures": "No signatures found", | ||
| "errors": { | ||
| "failed_to_load_signatures": "Error while loading signatures" | ||
| }, | ||
| "actions": { | ||
| "modify": "Modify" | ||
| }, | ||
| "datagrid_headers": { | ||
| "name": "Name", | ||
| "is_active": "Active", | ||
| "is_forced": "Forced", | ||
| "actions": "Actions" | ||
| }, | ||
| "compose_modal": { | ||
| "title": "Create a new signature for {{domain}}", | ||
| "delete_modal": { | ||
| "title": "Delete signature \"{{signature}}\"", | ||
| "message": "Are you sure you want to delete this signature? This action is irreversible!" | ||
| }, | ||
| "form": { | ||
| "labels": { | ||
| "name": "Name", | ||
| "is_active": "Active", | ||
| "is_forced": "Forced signature" | ||
| }, | ||
| "errors": { | ||
| "name": "Name is required", | ||
| "content": "Content is required" | ||
| }, | ||
| "helper_text": { | ||
| "is_active": "While the signature is disabled, it will not be available to the users.", | ||
| "is_forced": "The forced signature will be the only one usable for new messages." | ||
| } | ||
| } | ||
| }, | ||
| "toasts": { | ||
| "success_create": "Signature created!", | ||
| "success_update": "Signature updated!", | ||
| "delete": "Signature deleted!" | ||
| } | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing keys used by error handling and gating logic
UI changes above rely on these i18n keys. Add them to both locales.
"admin_maildomains_signature": {
"loading": "Loading signatures...",
"no_signatures": "No signatures found",
"errors": {
- "failed_to_load_signatures": "Error while loading signatures"
+ "failed_to_load_signatures": "Error while loading signatures",
+ "cannot_force_inactive": "Cannot force an inactive signature."
},
@@
"toasts": {
"success_create": "Signature created!",
"success_update": "Signature updated!",
- "delete": "Signature deleted!"
+ "delete": "Signature deleted!",
+ "error_update": "Failed to update signature.",
+ "error_delete": "Failed to delete signature."
}
}, "admin_maildomains_signature": {
"loading": "Chargement des signatures...",
"no_signatures": "Aucune signature trouvée",
"errors": {
- "failed_to_load_signatures": "Erreur lors du chargement des signatures"
+ "failed_to_load_signatures": "Erreur lors du chargement des signatures",
+ "cannot_force_inactive": "Impossible de forcer une signature inactive."
},
@@
"toasts": {
"success_create": "Signature créée !",
"success_update": "Signature mise à jour !",
- "delete": "Signature supprimée !"
+ "delete": "Signature supprimée !",
+ "error_update": "Échec de la mise à jour de la signature.",
+ "error_delete": "Échec de la suppression de la signature."
}
},Also applies to: 958-1000
🤖 Prompt for AI Agents
In src/frontend/src/features/i18n/translations.json around lines 458-500 (and
similarly update lines 958-1000 for the second locale), the UI relies on
additional i18n keys for error handling and gating that are currently missing;
add the missing keys (matching the structure under
"admin_maildomains_signature": errors, gating/flags, any referenced helper or
status keys used by the UI) to both locale blocks, ensuring keys and
placeholders exactly match those referenced in the components (e.g., add error
messages, gating flags/labels, and any toast/placeholder keys), and keep the
same nested JSON shape so the components can read them without runtime key
errors.
Comment on lines
+60
to
+67
| const toggleActive = async (signature: ReadOnlyMessageTemplate) => { | ||
| await updateSignature({ | ||
| id: signature.id, | ||
| data: { is_active: !signature.is_active, maildomain_id: domain.id }, | ||
| }); | ||
| invalidateMessageTemplates(); | ||
| addUpdateSucceededToast(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard invariants when deactivating and handle errors (active → forced must become false)
If an active, forced signature is toggled inactive, also unset is_forced in the same mutation to satisfy the “Active disables Default/Forced” rule and backend constraints. Add try/catch and await invalidation.
- const toggleActive = async (signature: ReadOnlyMessageTemplate) => {
- await updateSignature({
- id: signature.id,
- data: { is_active: !signature.is_active, maildomain_id: domain.id },
- });
- invalidateMessageTemplates();
- addUpdateSucceededToast();
- }
+ const toggleActive = async (signature: ReadOnlyMessageTemplate) => {
+ const nextActive = !Boolean(signature.is_active);
+ try {
+ await updateSignature({
+ id: signature.id,
+ data: {
+ is_active: nextActive,
+ // If we deactivate a forced signature, also drop the forced flag.
+ ...(signature.is_active && signature.is_forced ? { is_forced: false } : {}),
+ maildomain_id: domain.id,
+ },
+ });
+ await invalidateMessageTemplates();
+ addUpdateSucceededToast();
+ } catch {
+ addToast(<ToasterItem type="error"><span>{t("admin_maildomains_signature.toasts.error_update")}</span></ToasterItem>);
+ }
+ }📝 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.
Suggested change
| const toggleActive = async (signature: ReadOnlyMessageTemplate) => { | |
| await updateSignature({ | |
| id: signature.id, | |
| data: { is_active: !signature.is_active, maildomain_id: domain.id }, | |
| }); | |
| invalidateMessageTemplates(); | |
| addUpdateSucceededToast(); | |
| } | |
| const toggleActive = async (signature: ReadOnlyMessageTemplate) => { | |
| const nextActive = !Boolean(signature.is_active); | |
| try { | |
| await updateSignature({ | |
| id: signature.id, | |
| data: { | |
| is_active: nextActive, | |
| // If we deactivate a forced signature, also drop the forced flag. | |
| ...(signature.is_active && signature.is_forced ? { is_forced: false } : {}), | |
| maildomain_id: domain.id, | |
| }, | |
| }); | |
| await invalidateMessageTemplates(); | |
| addUpdateSucceededToast(); | |
| } catch { | |
| addToast( | |
| <ToasterItem type="error"> | |
| <span>{t("admin_maildomains_signature.toasts.error_update")}</span> | |
| </ToasterItem>, | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
src/frontend/src/features/layouts/components/admin/signatures-view/signature-data-grid.tsx
around lines 60 to 67, guard the invariants and handle errors when toggling a
signature: if you're turning an active signature off, include is_forced: false
in the same update mutation so "active disables forced" is respected; wrap the
update in try/catch, await the invalidateMessageTemplates() call after a
successful update, call addUpdateSucceededToast() only on success, and on error
log or show a failure toast and do not call the success toast.
624178f to
3180d9d
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/backend/core/models.py (2)
1584-1599: Tighten validation: forced⇒active; raw_blob owner match; require some content.
Prevents inconsistent flags and cross-tenant blob references; ensures template has material to render.Apply:
def clean(self): errors = {} if not self.mailbox and not self.maildomain: errors["__all__"] = ( "MessageTemplate must have at least a mailbox or maildomain" ) if ( self.mailbox and self.maildomain and self.mailbox.domain_id != self.maildomain_id ): errors["mailbox"] = "Mailbox domain does not match maildomain." + if self.is_forced and not self.is_active: + errors["is_forced"] = "Forced templates must be active." + # Optional but recommended: owner consistency and non-empty content + if self.raw_blob_id: + if self.mailbox_id and self.raw_blob.mailbox_id != self.mailbox_id: + errors["raw_blob"] = "Raw blob owner (mailbox) must match the template owner." + if self.maildomain_id and self.raw_blob.maildomain_id != self.maildomain_id: + errors["raw_blob"] = "Raw blob owner (maildomain) must match the template owner." + if not (self.html_body or self.text_body or self.raw_blob_id): + errors["__all__"] = errors.get("__all__") or "At least one of html_body, text_body or raw_blob is required." if errors: raise ValidationError(errors) super().clean()
1540-1562: Enforce “forced implies active” at DB level.
Prevents inconsistent states and matches UI expectation (“Active” disables “Default”).Apply:
constraints = [ models.CheckConstraint( check=( models.Q(mailbox__isnull=False) | models.Q(maildomain__isnull=False) ), name="messagetemplate_has_owner", ), + models.CheckConstraint( + check=(models.Q(is_forced=False) | models.Q(is_active=True)), + name="forced_template_must_be_active", + ), models.UniqueConstraint( fields=("mailbox", "type"), condition=models.Q(is_forced=True), name="uniq_forced_template_mailbox_type", ), models.UniqueConstraint( fields=("maildomain", "type"), condition=models.Q(is_forced=True), name="uniq_forced_template_maildomain_type", ), ]
🧹 Nitpick comments (2)
src/backend/core/models.py (2)
1619-1629: Avoid placeholder-collision bugs by replacing longest keys first.
Prevents {name} from clobbering {full_name}.Apply:
- # Simple placeholder substitution - for key, value in context.items(): - placeholder = f"{{{key}}}" - rendered_html_body = rendered_html_body.replace( - placeholder, escape(str(value)) - ) - rendered_text_body = rendered_text_body.replace(placeholder, str(value)) + # Simple placeholder substitution (longest keys first) + for key in sorted(context.keys(), key=len, reverse=True): + value = context[key] + placeholder = f"{{{key}}}" + rendered_html_body = rendered_html_body.replace(placeholder, escape(str(value))) + rendered_text_body = rendered_text_body.replace(placeholder, str(value))
1190-1199: Optional: reconsider compression logs and metrics.
Log reduction ratio only when compressed; include guard for very small/negative deltas.Apply:
- logger.debug( - "Compressed blob from %d bytes to %d bytes (%.1f%% reduction)", - original_size, - len(compressed_content), - (1 - len(compressed_content) / original_size) * 100, - ) + if original_size: + logger.debug( + "Compressed blob from %d to %d bytes (%.1f%% delta)", + original_size, + len(compressed_content), + (len(compressed_content) - original_size) * 100.0 / original_size, + )Also applies to: 1216-1224
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (4)
src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message_template.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/message_template_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/read_only_message_template.tsis excluded by!**/gen/**
📒 Files selected for processing (7)
src/backend/core/api/openapi.json(4 hunks)src/backend/core/api/serializers.py(2 hunks)src/backend/core/api/viewsets/message_template.py(1 hunks)src/backend/core/migrations/0007_alter_messagetemplate_type_blob_blob_has_owner_and_more.py(1 hunks)src/backend/core/migrations/0008_alter_messagetemplate_is_forced_and_more.py(1 hunks)src/backend/core/models.py(6 hunks)src/backend/core/tests/api/test_message_template.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- src/backend/core/tests/api/test_message_template.py
- src/backend/core/api/openapi.json
- src/backend/core/api/viewsets/message_template.py
- src/backend/core/migrations/0008_alter_messagetemplate_is_forced_and_more.py
- src/backend/core/migrations/0007_alter_messagetemplate_type_blob_blob_has_owner_and_more.py
- src/backend/core/api/serializers.py
🧰 Additional context used
🧬 Code graph analysis (1)
src/backend/core/models.py (4)
src/backend/core/enums.py (2)
MessageTemplateTypeChoices(108-113)CompressionTypeChoices(54-58)src/backend/core/tests/api/test_attachments.py (1)
blob(160-166)src/backend/core/api/serializers.py (4)
create(878-880)create(1023-1063)update(882-884)update(1065-1115)src/backend/core/api/viewsets/message_template.py (1)
render_template(195-215)
⏰ 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). (5)
- GitHub Check: docker-publish-mta-in / docker-build-push
- GitHub Check: check-api-state
- GitHub Check: build-front
- GitHub Check: test-back
- GitHub Check: lint-back
🔇 Additional comments (5)
src/backend/core/models.py (5)
17-18: LGTM: correct imports for transaction and HTML escaping.Also applies to: 37-37
487-492: Good delegation to BlobManager.
This removes duplication and centralizes compression/hash logic.
1265-1281: Blob ownership validation and DB Check look solid.
Constraint + clean() enforce “at least one owner”. Manager alignment via kwargs is good.Also applies to: 1287-1294, 1299-1305
1564-1566: Indexes and formatted-content fallback look good.
Good for common lookups; sensible html-or-text preference.Also applies to: 1635-1644
1192-1194: Resolve: MESSAGES_BLOB_ZSTD_LEVEL is defined
The setting is declared in src/backend/messages/settings.py with a default value, so no KeyError will occur.
Comment on lines
+1159
to
+1225
| class BlobManager(models.Manager): | ||
| """Custom Manager for Blob model.""" | ||
|
|
||
| def create_blob( | ||
| self, | ||
| content: bytes, | ||
| content_type: str, | ||
| compression: Optional[CompressionTypeChoices] = CompressionTypeChoices.ZSTD, | ||
| **kwargs, | ||
| ) -> "Blob": | ||
| """ | ||
| Create a new blob with automatic SHA256 calculation and compression. | ||
| Args: | ||
| content: Raw binary content to store | ||
| content_type: MIME type of the content | ||
| compression: Compression type to use (defaults to ZSTD) | ||
| Returns: | ||
| The created Blob instance | ||
| Raises: | ||
| ValueError: If content is empty | ||
| """ | ||
| if not content: | ||
| raise ValueError("Content cannot be empty") | ||
|
|
||
| # Calculate SHA256 hash of the original content | ||
| sha256_hash = hashlib.sha256(content).digest() | ||
|
|
||
| # Store the original size | ||
| original_size = len(content) | ||
|
|
||
| # Apply compression if requested | ||
| compressed_content = content | ||
| if compression == CompressionTypeChoices.ZSTD: | ||
| compressed_content = pyzstd.compress( | ||
| content, level_or_option=settings.MESSAGES_BLOB_ZSTD_LEVEL | ||
| ) | ||
| logger.debug( | ||
| "Compressed blob from %d bytes to %d bytes (%.1f%% reduction)", | ||
| original_size, | ||
| len(compressed_content), | ||
| (1 - len(compressed_content) / original_size) * 100, | ||
| ) | ||
| elif compression == CompressionTypeChoices.NONE: | ||
| compressed_content = content | ||
| else: | ||
| raise ValueError(f"Unsupported compression type: {compression}") | ||
|
|
||
| # Create the blob | ||
| blob = Blob.objects.create( | ||
| sha256=sha256_hash, | ||
| size=original_size, | ||
| content_type=content_type, | ||
| compression=compression, | ||
| raw_content=compressed_content, | ||
| **kwargs, | ||
| ) | ||
|
|
||
| logger.info( | ||
| "Created blob %s: %d bytes, %s compression, %s content type", | ||
| blob.id, | ||
| original_size, | ||
| compression.label, | ||
| content_type, | ||
| ) | ||
|
|
||
| return blob | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use ValidationError and quiet logs in BlobManager.create_blob.
Avoid 500s from ValueError; reduce info-level noise for high-volume code paths.
Apply:
@@
- if not content:
- raise ValueError("Content cannot be empty")
+ if not content:
+ raise ValidationError({"content": "Content cannot be empty"})
@@
- else:
- raise ValueError(f"Unsupported compression type: {compression}")
+ else:
+ raise ValidationError({"compression": f"Unsupported compression type: {compression}"})
@@
- logger.info(
+ logger.debug(
"Created blob %s: %d bytes, %s compression, %s content type",
blob.id,
original_size,
compression.label,
content_type,
)📝 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.
Suggested change
| class BlobManager(models.Manager): | |
| """Custom Manager for Blob model.""" | |
| def create_blob( | |
| self, | |
| content: bytes, | |
| content_type: str, | |
| compression: Optional[CompressionTypeChoices] = CompressionTypeChoices.ZSTD, | |
| **kwargs, | |
| ) -> "Blob": | |
| """ | |
| Create a new blob with automatic SHA256 calculation and compression. | |
| Args: | |
| content: Raw binary content to store | |
| content_type: MIME type of the content | |
| compression: Compression type to use (defaults to ZSTD) | |
| Returns: | |
| The created Blob instance | |
| Raises: | |
| ValueError: If content is empty | |
| """ | |
| if not content: | |
| raise ValueError("Content cannot be empty") | |
| # Calculate SHA256 hash of the original content | |
| sha256_hash = hashlib.sha256(content).digest() | |
| # Store the original size | |
| original_size = len(content) | |
| # Apply compression if requested | |
| compressed_content = content | |
| if compression == CompressionTypeChoices.ZSTD: | |
| compressed_content = pyzstd.compress( | |
| content, level_or_option=settings.MESSAGES_BLOB_ZSTD_LEVEL | |
| ) | |
| logger.debug( | |
| "Compressed blob from %d bytes to %d bytes (%.1f%% reduction)", | |
| original_size, | |
| len(compressed_content), | |
| (1 - len(compressed_content) / original_size) * 100, | |
| ) | |
| elif compression == CompressionTypeChoices.NONE: | |
| compressed_content = content | |
| else: | |
| raise ValueError(f"Unsupported compression type: {compression}") | |
| # Create the blob | |
| blob = Blob.objects.create( | |
| sha256=sha256_hash, | |
| size=original_size, | |
| content_type=content_type, | |
| compression=compression, | |
| raw_content=compressed_content, | |
| **kwargs, | |
| ) | |
| logger.info( | |
| "Created blob %s: %d bytes, %s compression, %s content type", | |
| blob.id, | |
| original_size, | |
| compression.label, | |
| content_type, | |
| ) | |
| return blob | |
| class BlobManager(models.Manager): | |
| """Custom Manager for Blob model.""" | |
| def create_blob( | |
| self, | |
| content: bytes, | |
| content_type: str, | |
| compression: Optional[CompressionTypeChoices] = CompressionTypeChoices.ZSTD, | |
| **kwargs, | |
| ) -> "Blob": | |
| """ | |
| Create a new blob with automatic SHA256 calculation and compression. | |
| Args: | |
| content: Raw binary content to store | |
| content_type: MIME type of the content | |
| compression: Compression type to use (defaults to ZSTD) | |
| Returns: | |
| The created Blob instance | |
| Raises: | |
| ValidationError: If content is empty or compression type unsupported | |
| """ | |
| if not content: | |
| raise ValidationError({"content": "Content cannot be empty"}) | |
| # Calculate SHA256 hash of the original content | |
| sha256_hash = hashlib.sha256(content).digest() | |
| # Store the original size | |
| original_size = len(content) | |
| # Apply compression if requested | |
| compressed_content = content | |
| if compression == CompressionTypeChoices.ZSTD: | |
| compressed_content = pyzstd.compress( | |
| content, level_or_option=settings.MESSAGES_BLOB_ZSTD_LEVEL | |
| ) | |
| logger.debug( | |
| "Compressed blob from %d bytes to %d bytes (%.1f%% reduction)", | |
| original_size, | |
| len(compressed_content), | |
| (1 - len(compressed_content) / original_size) * 100, | |
| ) | |
| elif compression == CompressionTypeChoices.NONE: | |
| compressed_content = content | |
| else: | |
| raise ValidationError({"compression": f"Unsupported compression type: {compression}"}) | |
| # Create the blob | |
| blob = Blob.objects.create( | |
| sha256=sha256_hash, | |
| size=original_size, | |
| content_type=content_type, | |
| compression=compression, | |
| raw_content=compressed_content, | |
| **kwargs, | |
| ) | |
| logger.debug( | |
| "Created blob %s: %d bytes, %s compression, %s content type", | |
| blob.id, | |
| original_size, | |
| compression.label, | |
| content_type, | |
| ) | |
| return blob |
Comment on lines
+1571
to
+1582
| def save(self, *args, **kwargs): | ||
| with transaction.atomic(): | ||
| if self.is_forced: | ||
| qs = MessageTemplate.objects.filter( | ||
| type=self.type, is_forced=True | ||
| ).exclude(id=self.id) | ||
| if self.mailbox_id: | ||
| qs = qs.filter(mailbox_id=self.mailbox_id) | ||
| if self.maildomain_id: | ||
| qs = qs.filter(maildomain_id=self.maildomain_id) | ||
| qs.update(is_forced=False) | ||
| super().save(*args, **kwargs) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate before demoting others; lock contenders to reduce races.
Call full_clean() before updating peers to avoid side effects if validation fails; lock rows you demote.
Apply:
def save(self, *args, **kwargs):
with transaction.atomic():
- if self.is_forced:
+ # Validate self first to avoid demoting others on invalid data
+ self.full_clean()
+ if self.is_forced:
- qs = MessageTemplate.objects.filter(
+ qs = MessageTemplate.objects.select_for_update().filter(
type=self.type, is_forced=True
).exclude(id=self.id)
if self.mailbox_id:
qs = qs.filter(mailbox_id=self.mailbox_id)
if self.maildomain_id:
qs = qs.filter(maildomain_id=self.maildomain_id)
qs.update(is_forced=False)
super().save(*args, **kwargs)📝 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.
Suggested change
| def save(self, *args, **kwargs): | |
| with transaction.atomic(): | |
| if self.is_forced: | |
| qs = MessageTemplate.objects.filter( | |
| type=self.type, is_forced=True | |
| ).exclude(id=self.id) | |
| if self.mailbox_id: | |
| qs = qs.filter(mailbox_id=self.mailbox_id) | |
| if self.maildomain_id: | |
| qs = qs.filter(maildomain_id=self.maildomain_id) | |
| qs.update(is_forced=False) | |
| super().save(*args, **kwargs) | |
| def save(self, *args, **kwargs): | |
| with transaction.atomic(): | |
| # Validate self first to avoid demoting others on invalid data | |
| self.full_clean() | |
| if self.is_forced: | |
| qs = MessageTemplate.objects.select_for_update().filter( | |
| type=self.type, is_forced=True | |
| ).exclude(id=self.id) | |
| if self.mailbox_id: | |
| qs = qs.filter(mailbox_id=self.mailbox_id) | |
| if self.maildomain_id: | |
| qs = qs.filter(maildomain_id=self.maildomain_id) | |
| qs.update(is_forced=False) | |
| super().save(*args, **kwargs) |
🤖 Prompt for AI Agents
In src/backend/core/models.py around lines 1571-1582, call self.full_clean()
before altering other MessageTemplate rows and acquire row locks on the
contenders to prevent races: validate the instance first (self.full_clean()),
then inside the existing transaction.atomic() build the queryset of other forced
templates (filtering mailbox_id/maildomain_id and excluding self.id) but use
select_for_update() to lock those rows before calling
qs.update(is_forced=False); finally call super().save(*args, **kwargs). Ensure
validation happens prior to demotion and that the lock and update occur inside
the same transaction.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Refactor
Tests
Style