✨(recipients) handle MAX_RECIPIENTS_PER_MESSAGE #456
Conversation
WalkthroughAdds configurable recipient limits (global, domain, mailbox), enforces them in draft create/update, exposes new management permissions/endpoints and OpenAPI schemas, persists custom_limits on models, updates serializers/permissions, and adds frontend UI/modals, tests, and E2E coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant FE as Frontend UI
participant API as Backend API
participant DB as Database
participant Cache as React Query Cache
Admin->>FE: Open Domain Settings modal
FE->>API: GET /api/v1.0/maildomains/{id}/
API->>DB: Fetch MailDomain + custom_limits
DB-->>API: Return MailDomainAdmin
API-->>FE: MailDomainAdmin with custom_limits
FE->>FE: Validate input ≤ MAX_RECIPIENTS_PER_MESSAGE
Admin->>FE: Submit new limit
FE->>API: PATCH /api/v1.0/maildomains/{id}/ { custom_limits: {...} }
API->>API: Permission check (CanManageSettings)
API->>DB: Update MailDomain.custom_limits
DB-->>API: Success
API-->>FE: Return updated MailDomainAdmin
FE->>Cache: Invalidate maildomain queries
FE->>Admin: Show success toast / close modal
%% Message compose flow
participant User as End User
participant Form as Message Form
participant DraftAPI as Draft API
User->>Form: Select mailbox
Form->>API: GET mailbox (abilities, max_recipients_per_message)
API->>DB: Resolve effective limit via mailbox.get_max_recipients_per_message()
DB-->>API: Return limit
API-->>Form: maxRecipients
Form->>Form: Enforce total(to+cc+bcc) ≤ maxRecipients
User->>Form: Submit draft
Form->>DraftAPI: POST /messages/drafts
DraftAPI->>API: create_draft() validation (enforce same limit)
DraftAPI->>DB: Persist draft
DB-->>DraftAPI: Return draft created
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
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/models.py (1)
304-316: Missing validation forcustom_limitsinMailDomain.clean.The
MailDomain.cleanmethod validatescustom_attributesbut notcustom_limits. In contrast,Mailbox.clean(lines 535-552) validates itscustom_limitsfield. This inconsistency could allow invalid data inMailDomain.custom_limits.Consider adding validation similar to
Mailbox.clean:def clean(self): """Validate custom attributes and limits.""" # Custom attributes schema try: jsonschema.validate( self.custom_attributes, settings.SCHEMA_CUSTOM_ATTRIBUTES_MAILDOMAIN ) except jsonschema.ValidationError as exception: raise ValidationError( {"custom_attributes": exception.message} ) from exception + # Validate custom_limits structure + if self.custom_limits: + if not isinstance(self.custom_limits, dict): + raise ValidationError( + {"custom_limits": "custom_limits must be a dictionary"} + ) + if "max_recipients_per_message" in self.custom_limits: + value = self.custom_limits["max_recipients_per_message"] + if value is not None and (not isinstance(value, int) or value < 1): + raise ValidationError( + { + "custom_limits": "max_recipients_per_message must be a positive integer" + } + ) + super().clean()src/backend/core/api/serializers.py (1)
1049-1073: Addcustom_limitstoread_only_fieldsor handle it inupdate().The
custom_limitsfield is added tofieldsbut not toread_only_fields, making it appear writable. However, theupdate()method (lines 1162-1212) doesn't processcustom_limits- it only handles metadata fields likefull_name,name, andcustom_attributes. This inconsistency could confuse API consumers who expect updates to persist.Since the dedicated
/settings/endpoint handlescustom_limitsupdates, consider adding it toread_only_fields:read_only_fields = [ "id", "domain_name", "is_identity", "accesses", # List of users and their roles "created_at", "updated_at", "can_reset_password", "contact", + "custom_limits", ]
🧹 Nitpick comments (29)
src/e2e/src/__tests__/message-max-recipients.spec.ts (1)
17-33: Consider making the expected limit configurable.The test hardcodes
100as the expected limit. If the test environment configuration forMAX_DEFAULT_RECIPIENTS_PER_MESSAGEchanges, this test will fail. Consider reading the expected value from test fixtures or environment configuration to make the test more maintainable.src/frontend/src/features/forms/components/message-form/index.tsx (2)
29-29: Remove unused import.
useConfigRetrieveis imported but never used in this file. OnlyuseConfigis used.-import { useConfigRetrieve } from "@/features/api/gen/config/config";
230-235: Consider extracting duplicate limit calculation logic.The logic for computing the recipient limit from mailbox/config is duplicated in the initial state (lines 230-235) and the useEffect (lines 255-266). Extract to a helper function for maintainability.
const getMaxRecipientsLimit = (mailbox: Mailbox | undefined, config: AppConfig) => { const defaultLimit = Number(config.MAX_DEFAULT_RECIPIENTS_PER_MESSAGE ?? config.MAX_RECIPIENTS_PER_MESSAGE); const mailboxLimit = mailbox?.max_recipients_per_message; return mailboxLimit ? Number(mailboxLimit) : defaultLimit; };Also applies to: 255-266
src/frontend/src/features/forms/components/react-hook-form/rhf-contact-combobox.tsx (2)
1-1: Remove unused import.
useConfigRetrieveis imported but not used in this file.-import { useConfigRetrieve, useContactsList } from "@/features/api/gen"; +import { useContactsList } from "@/features/api/gen";
69-74: Potential validation loop concern.Calling
trigger(['to', 'cc', 'bcc'])on every change in any recipient field will trigger validation for all three fields. If each field's onChange triggers all three, this could cause redundant validation cycles. The approach works but may cause minor performance overhead.Since the parent
message-formalready has a similar trigger (line 275), consider whether both are necessary.src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-limits/index.tsx (1)
33-41: Validation and RHF wiring are correct; consider minor cleanups for readability and form controlOverall flow (defaulting from
mailbox.custom_limits, validating against global max, and submitting viauseMaildomainsMailboxesSettingsUpdate) looks sound.Two small improvements to keep things maintainable:
- Avoid repeated
Number(value)conversions and tighten the guard:- const value = data.max_recipients_per_message.trim(); - if (value !== '' && (isNaN(Number(value)) || Number(value) < 1 || !Number.isInteger(Number(value)))) { + const value = data.max_recipients_per_message.trim(); + if (value === '') { + // handled as "use default" below + } else { + const parsed = Number(value); + if (!Number.isInteger(parsed) || parsed < 1) { + form.setError('max_recipients_per_message', { + message: t('Please enter a valid positive integer or leave empty.'), + }); + return; + } + // Validate that the value does not exceed the global maximum + if (parsed > MAX_RECIPIENTS_PER_MESSAGE) { + form.setError('max_recipients_per_message', { + message: t('The limit cannot exceed the global maximum of {{max}} recipients.', { + max: MAX_RECIPIENTS_PER_MESSAGE, + }), + }); + return; + } + }
- RHF +
Inputcontrol: you currently bothregisterthe field and pass an explicitvalue={maxRecipientsValue}. Withreact-hook-form, the usual pattern is either:
- let
registercontrol the value (drop thevalueprop), or- use
ControllerifInputis not fully compatible withregister.If
Inputworks withregister, you can simplify to:- const { handleSubmit, register, formState: { errors }, watch } = form; - const maxRecipientsValue = watch('max_recipients_per_message'); + const { handleSubmit, register, formState: { errors } } = form; ... - {...register('max_recipients_per_message')} + {...register('max_recipients_per_message')} ... - value={maxRecipientsValue}This avoids potential controlled/uncontrolled warnings and keeps the form logic closer to standard RHF usage.
Also applies to: 48-65, 97-127
src/backend/core/tests/models/test_mailbox.py (1)
106-122: Mailboxmanage_settingscoverage is good; consider adding a positive-case testThe extra
abilities["manage_settings"] is Falseassertions across no‑access/viewer/editor/admin/sender cases, plus the clarified admin docstring, make the permission model around mailbox settings much clearer and aligned with “domain admin only” semantics.You might optionally add a dedicated test exercising the case where a domain admin (or superuser) does get
manage_settings=Trueon a mailbox, mirroring the domain-level tests, so the positive path is also explicitly covered.Also applies to: 123-145, 146-168, 169-193, 194-215
src/backend/core/tests/models/test_maildomain.py (1)
130-162: Domainmanage_settingstests align with implementation; double-check superusergetsemanticsThe new assertions that
abilities["manage_settings"]is False for admins and True for superusers correctly reflectMailDomain.get_abilities(settings reserved to superusers).In the superuser test you also assert
abilities["get"] is False, which matches the current implementation (CAN_READis based solely on domain role, notis_superuser). If the intent is that superusers may update/delete/configure domains but not “read” them viaget_abilities, this is fine; otherwise you might want to adjustget_abilitiesto treat superusers as readable too (and update this assertion accordingly).Also applies to: 163-177
src/e2e/src/__tests__/message-max-recipients-admin.spec.ts (5)
46-47: Remove debug console.log statements before merge.These debug statements should be removed as they add noise to test output.
- console.log(limitMatch); - console.log(limit);
51-51: Fix grammatical error in test description.The test name is grammatically incorrect and doesn't clearly convey the expected behavior.
- test("should super user can customize domain max recipients per message more than the global limit", async ({ page, browserName }) => { + test("should not allow super user to exceed global max recipients limit", async ({ page, browserName }) => {
65-65: ReplacewaitForTimeoutwith explicit wait conditions.Using fixed timeouts is a test anti-pattern that can cause flaky tests. Prefer waiting for specific conditions.
- await page.waitForTimeout(50); + await maxRecipientsPerMessageInput.waitFor({ state: "visible" });Also applies to: 78-78
68-68: Remove debug console.log statement.- console.log(maxRecipientsPerMessageInput);
84-95: Incomplete test with TODO placeholder.This test is incomplete and only contains setup steps. Either implement the full test or remove it to avoid confusion in test reports.
Would you like me to help implement the full test case for domain admin customizing mailbox max recipients?
src/backend/core/mda/draft.py (2)
110-112: Consider using i18n for the error message.The error message uses an f-string, but other validation errors in this file use
_()for translation. For consistency with the rest of the codebase:- raise drf.exceptions.ValidationError( - f"Too many recipients: {total_recipients} (maximum is {max_recipients})." - ) + raise drf.exceptions.ValidationError( + _("Too many recipients: %(total)s (maximum is %(max)s).") + % {"total": total_recipients, "max": max_recipients} + )
272-274: Consider using i18n for consistency.Same as in
create_draft- consider using the translation function for consistency:- raise drf.exceptions.ValidationError( - f"Too many recipients: {total_after_update} (maximum is {max_recipients})." - ) + raise drf.exceptions.ValidationError( + _("Too many recipients: %(total)s (maximum is %(max)s).") + % {"total": total_after_update, "max": max_recipients} + )src/frontend/src/features/layouts/components/admin/modal-create-update-mailbox/index.tsx (1)
72-72: Avoidas anycast - consider proper typing.Casting to
anybypasses TypeScript's type safety. Consider adding proper typing for the config:- const { SCHEMA_CUSTOM_ATTRIBUTES_USER } = useConfig() as any; + const { SCHEMA_CUSTOM_ATTRIBUTES_USER } = useConfig() as { SCHEMA_CUSTOM_ATTRIBUTES_USER?: { properties?: Record<string, ItemJsonSchema> } };Alternatively, extend the
Configtype in the config provider to include this property with its correct type.src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (2)
35-35: Avoidas anycast for type safety.The
MailDomainAdmintype may need to be extended to includecustom_limits. Consider updating the generated types or using a more specific type assertion:- max_recipients_per_message: (domain as any)?.custom_limits?.max_recipients_per_message?.toString() ?? '', + max_recipients_per_message: domain.custom_limits?.max_recipients_per_message?.toString() ?? '',This requires ensuring
custom_limitsis properly typed inMailDomainAdmin.Verify that the
MailDomainAdmintype includescustom_limitsin the generated API types. If not, the OpenAPI schema may need to be updated to include this field.
120-123: Potential double-translation issue.If
erroralready contains a translated message from the API response (line 100-101), wrapping it int(error)could cause unexpected behavior. Consider:- <Banner type="error"> - {t(error)} - </Banner> + <Banner type="error"> + {error} + </Banner>The error message is already constructed with
t()in the catch block's fallback (line 100).src/frontend/src/pages/domain/index.tsx (3)
2-2: Unused import:useModal.The
useModalhook is imported but not used in this file.-import { Button, DataGrid, usePagination, useModal } from "@openfun/cunningham-react"; +import { Button, DataGrid, usePagination } from "@openfun/cunningham-react";
104-123: Consider simplifying the IIFE pattern for modal rendering.The immediately-invoked function expression works but is unconventional in React JSX. The
freshDomainlookup could be moved to auseMemoor inline within the component for consistency with howModalMaildomainManageAccessesis rendered above.That said, the pattern ensures fresh data is used when the modal opens, which is a valid approach for this use case.
34-39: Consider extracting the query invalidation logic to reduce duplication.The same invalidation pattern appears in both
refetchMaildomains(line 34-39) andhandleCreateDomain(line 179-182). A shared helper would improve maintainability.// Could be extracted to a shared utility const invalidateMaildomainsList = () => { queryClient.invalidateQueries({ queryKey: getMaildomainsListQueryOptions().queryKey, exact: false, }); };Also applies to: 176-182
src/backend/core/tests/api/test_admin_maildomains_update.py (1)
50-52: Strengthen the assertion for verifying limits were NOT updated.The assertion
maildomain.custom_limits.get("max_recipients_per_message") != 50would pass ifcustom_limitsis an empty dict (returnsNone). Consider asserting the original state explicitly for clarity.# Verify the limit was NOT updated maildomain.refresh_from_db() - assert maildomain.custom_limits.get("max_recipients_per_message") != 50 + assert maildomain.custom_limits == {} or maildomain.custom_limits.get("max_recipients_per_message") is NoneAlternatively, capture the original value before the request and assert it remains unchanged.
src/backend/core/models.py (1)
623-652: Role initialization change fromNoneto0.Changing
role = Nonetorole = 0is functionally equivalent here since0is falsy andbool(role)evaluates the same. However, this subtly changes semantics:role = 0could be confused with a valid role value in systems using 0-indexed enums.Since
MailboxRoleChoicesstarts at 1 (VIEWER=1), this is safe, but consider documenting or using a named constant for clarity.src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx (1)
218-225: Remove unnecessary type assertion forupdated_atfield.The
MailboxAdmininterface already includesupdated_atas a properly typed field (readonly updated_at: string), so theas anycast is unnecessary:- key={`${editedMailbox.id}-${(editedMailbox as any).updated_at}`} + key={`${editedMailbox.id}-${editedMailbox.updated_at}`}src/backend/core/api/serializers.py (1)
976-991: Consider validating for non-positive values.The validation correctly prevents exceeding the global maximum, but doesn't check for negative or zero values. If
max_recipients_per_message: -1ormax_recipients_per_message: 0is submitted, it would pass validation but could cause unexpected behavior downstream.def validate_custom_limits(self, value): """Validate that custom_limits does not exceed global maximum.""" if value and isinstance(value, dict): max_recipients = value.get("max_recipients_per_message") if max_recipients is not None and isinstance(max_recipients, int): + if max_recipients <= 0: + raise serializers.ValidationError( + _("The limit must be a positive integer.") + ) if max_recipients > settings.MAX_RECIPIENTS_PER_MESSAGE: raise serializers.ValidationError( _( "The limit cannot exceed the global maximum of %(max)s recipients." ) % {"max": settings.MAX_RECIPIENTS_PER_MESSAGE} ) return valuesrc/backend/core/api/openapi.json (4)
221-235: Config: recipient-limit keys mostly consistent, consider requiredness of defaultThe new
MAX_RECIPIENTS_PER_MESSAGEandMAX_DEFAULT_RECIPIENTS_PER_MESSAGEentries and descriptions are clear and aligned with the PR goal. IfMAX_DEFAULT_RECIPIENTS_PER_MESSAGEis guaranteed to be present in the response, it would be more accurate to also include it in therequiredlist; otherwise, consider explicitly noting that it may be omitted so client generators don’t over-assume its presence.Also applies to: 237-250
3669-3735: Mailbox/settings/endpoint matches the intended custom_limits workflowThe new
/maildomains/{maildomain_pk}/mailboxes/{id}/settings/PATCHendpoint cleanly separates mailbox settings updates viaPatchedMailboxSettingsUpdateRequestand returnsMailboxSettingsUpdate, which fits the “settings-only” contract described in the PR. Optionally, you might later add JSON schemas for the400/403responses for better client DX, but this is not blocking.
6194-6209: MailboxAdmin.custom_limits: consider making this explicitly readOnlyExposing
custom_limitsonMailboxAdminis useful for admin UIs, but since you’ve introduced a dedicated/settings/endpoint for modifying these values, you may want to mark this property asreadOnly: truehere to avoid implying that it’s writable via the general mailbox adminPATCHendpoint. That keeps the write surface clearly funneled through/settings/.
6348-6356: MailboxAdminUpdateMetadataRequest: avoid suggesting custom_limits is updatable here if only/settings/should write itAdding
custom_limitstoMailboxAdminUpdateMetadataRequestsuggests clients can also update limits via the main mailboxPATCHmetadata payload, which partially contradicts the dedicated/settings/endpoint and theMailboxAdminCreatenote. If the backend ignores this field in metadata updates, consider removing it from this request schema to keep the documented write path unique; if you do intend to support both, it’s fine but worth being aware that you now have two public write surfaces.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
src/frontend/src/features/api/gen/maildomains/maildomains.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/config_retrieve200.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_abilities.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_update.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_update_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_write.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_write_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_abilities.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin_create.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin_update_metadata_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_settings_update.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_mail_domain_admin_update_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_mailbox_settings_update_request.tsis excluded by!**/gen/**
📒 Files selected for processing (30)
src/backend/core/api/openapi.json(18 hunks)src/backend/core/api/permissions.py(1 hunks)src/backend/core/api/serializers.py(9 hunks)src/backend/core/api/viewsets/config.py(4 hunks)src/backend/core/api/viewsets/maildomain.py(5 hunks)src/backend/core/enums.py(2 hunks)src/backend/core/mda/draft.py(3 hunks)src/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.py(1 hunks)src/backend/core/models.py(9 hunks)src/backend/core/tests/api/test_admin_maildomains_list.py(3 hunks)src/backend/core/tests/api/test_admin_maildomains_update.py(1 hunks)src/backend/core/tests/api/test_config.py(2 hunks)src/backend/core/tests/api/test_mailboxes.py(4 hunks)src/backend/core/tests/api/test_messages_create.py(2 hunks)src/backend/core/tests/models/test_mailbox.py(6 hunks)src/backend/core/tests/models/test_maildomain.py(2 hunks)src/backend/messages/settings.py(1 hunks)src/e2e/src/__tests__/message-max-recipients-admin.spec.ts(1 hunks)src/e2e/src/__tests__/message-max-recipients.spec.ts(1 hunks)src/frontend/src/features/forms/components/message-form/index.tsx(10 hunks)src/frontend/src/features/forms/components/react-hook-form/rhf-contact-combobox.tsx(2 hunks)src/frontend/src/features/forms/components/react-hook-form/rhf-json-schema-field.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/domains-view/settings-domain-action.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx(7 hunks)src/frontend/src/features/layouts/components/admin/modal-create-update-mailbox/index.tsx(4 hunks)src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-limits/index.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx(1 hunks)src/frontend/src/features/providers/config.tsx(1 hunks)src/frontend/src/hooks/use-ability.ts(3 hunks)src/frontend/src/pages/domain/index.tsx(6 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Follow Django/PEP 8 style with a 100-character line limit
Use descriptive, snake_case names for variables and functions
Use Django ORM for database access; avoid raw SQL unless necessary for performance
Use Django’s built-in user model and authentication framework
Prefer try-except blocks to handle exceptions in business logic and views
Log expected and unexpected actions with appropriate log levels
Capture and report exceptions to Sentry; use capture_exception() for custom errors
Do not log sensitive information (tokens, passwords, financial/health data, PII)
Files:
src/backend/core/tests/api/test_admin_maildomains_list.pysrc/backend/core/enums.pysrc/backend/core/mda/draft.pysrc/backend/messages/settings.pysrc/backend/core/tests/models/test_maildomain.pysrc/backend/core/models.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/tests/models/test_mailbox.pysrc/backend/core/api/permissions.pysrc/backend/core/tests/api/test_mailboxes.pysrc/backend/core/tests/api/test_admin_maildomains_update.pysrc/backend/core/tests/api/test_messages_create.pysrc/backend/core/tests/api/test_config.pysrc/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.pysrc/backend/core/api/viewsets/maildomain.pysrc/backend/core/api/serializers.py
src/backend/**/{tests.py,tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/{tests.py,tests/**/*.py}: Use Django’s testing tools (pytest-django) to ensure code quality and reliability
Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases
Files:
src/backend/core/tests/api/test_admin_maildomains_list.pysrc/backend/core/tests/models/test_maildomain.pysrc/backend/core/tests/models/test_mailbox.pysrc/backend/core/tests/api/test_mailboxes.pysrc/backend/core/tests/api/test_admin_maildomains_update.pysrc/backend/core/tests/api/test_messages_create.pysrc/backend/core/tests/api/test_config.py
src/backend/**/{settings.py,middleware.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use middleware judiciously for cross-cutting concerns (authentication, logging, caching)
Files:
src/backend/messages/settings.py
src/backend/**/settings.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/settings.py: Leverage Django’s caching framework (e.g., Redis/Memcached) where appropriate
Use Django’s cache framework with a backend like Redis or Memcached to reduce DB load
Optimize static file handling using Django’s staticfiles pipeline (e.g., WhiteNoise)
Files:
src/backend/messages/settings.py
src/backend/**/{models.py,forms.py,views.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Keep business logic in models and forms; keep views thin and focused on request handling
Files:
src/backend/core/models.py
src/backend/**/{models.py,migrations/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Implement database indexing and query optimization (Model Meta indexes, constraints)
Files:
src/backend/core/models.pysrc/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.py
src/backend/{templates/**/*.html,**/serializers.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use Django templates for HTML and DRF serializers for JSON responses
Files:
src/backend/core/api/serializers.py
🧠 Learnings (3)
📚 Learning: 2025-09-02T12:42:29.083Z
Learnt from: Nastaliss
Repo: suitenumerique/messages PR: 313
File: src/frontend/src/pages/domain/index.tsx:0-0
Timestamp: 2025-09-02T12:42:29.083Z
Learning: The domain admin pages in src/frontend/src/pages/domain/ use AdminLayout, not the main layout. Toast notifications for admin pages should be handled via a Toaster instance in the AdminLayout component.
Applied to files:
src/frontend/src/features/layouts/components/admin/domains-view/settings-domain-action.tsxsrc/frontend/src/pages/domain/index.tsxsrc/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx
📚 Learning: 2025-09-02T12:20:22.006Z
Learnt from: Nastaliss
Repo: suitenumerique/messages PR: 313
File: src/frontend/src/features/layouts/components/admin/modal-create-domain/index.tsx:32-37
Timestamp: 2025-09-02T12:20:22.006Z
Learning: In the messages application, `SCHEMA_CUSTOM_ATTRIBUTES_MAILDOMAIN` from the config defaults to an empty object `{}` rather than being potentially undefined, so null checks are not needed when using this configuration value.
Applied to files:
src/frontend/src/features/forms/components/message-form/index.tsxsrc/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.pysrc/frontend/src/features/layouts/components/admin/modal-create-update-mailbox/index.tsx
📚 Learning: 2025-09-02T10:12:12.835Z
Learnt from: CR
Repo: suitenumerique/messages PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-09-02T10:12:12.835Z
Learning: Applies to src/backend/**/views.py : Use Django REST Framework viewsets for API endpoints
Applied to files:
src/backend/core/api/viewsets/maildomain.py
🧬 Code graph analysis (14)
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-limits/index.tsx (5)
src/frontend/src/features/api/gen/models/mailbox_admin.ts (1)
MailboxAdmin(14-39)src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/api/gen/maildomains/maildomains.ts (1)
useMaildomainsMailboxesSettingsUpdate(1869-1901)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(17-45)
src/backend/core/mda/draft.py (3)
src/backend/core/api/serializers.py (1)
get_max_recipients_per_message(292-294)src/backend/core/models.py (1)
get_max_recipients_per_message(735-764)src/backend/core/enums.py (1)
MessageRecipientTypeChoices(31-36)
src/frontend/src/pages/domain/index.tsx (4)
src/frontend/src/hooks/use-ability.ts (1)
Abilities(34-39)src/frontend/src/features/api/gen/maildomains/maildomains.ts (1)
getMaildomainsListQueryOptions(102-131)src/frontend/src/features/api/gen/models/mail_domain_admin.ts (1)
MailDomainAdmin(13-26)src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (1)
ModalUpdateDomain(24-153)
src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (5)
src/frontend/src/features/api/gen/models/mail_domain_admin.ts (1)
MailDomainAdmin(13-26)src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/api/fetch-api.ts (1)
fetchAPI(12-40)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(17-45)
src/backend/core/tests/models/test_maildomain.py (1)
src/backend/core/models.py (3)
get_abilities(225-240)get_abilities(351-389)get_abilities(619-677)
src/backend/core/api/permissions.py (3)
src/backend/core/api/serializers.py (4)
get_abilities(132-141)get_abilities(165-167)get_abilities(288-290)get_abilities(910-912)src/backend/core/models.py (5)
get_abilities(225-240)get_abilities(351-389)get_abilities(619-677)MailDomain(243-426)Mailbox(483-764)src/backend/core/enums.py (2)
MailDomainAbilities(96-101)MailboxAbilities(104-116)
src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx (2)
src/frontend/src/features/api/gen/models/mailbox_admin.ts (1)
MailboxAdmin(14-39)src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-limits/index.tsx (1)
ModalMailboxManageLimits(24-143)
src/frontend/src/features/forms/components/message-form/index.tsx (2)
src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/providers/mailbox.tsx (1)
useMailboxContext(355-361)
src/e2e/src/__tests__/message-max-recipients.spec.ts (1)
src/e2e/src/utils-test.ts (1)
signInKeycloakIfNeeded(6-39)
src/backend/core/tests/api/test_admin_maildomains_update.py (4)
src/backend/core/enums.py (2)
MailDomainAccessRoleChoices(48-51)MailboxRoleChoices(15-21)src/backend/core/factories.py (3)
UserFactory(20-31)MailDomainFactory(51-57)MailDomainAccessFactory(112-122)src/backend/core/models.py (1)
get_max_recipients_per_message(735-764)src/backend/core/api/serializers.py (1)
get_max_recipients_per_message(292-294)
src/backend/core/tests/api/test_messages_create.py (4)
src/backend/core/tests/models/test_mailbox.py (2)
mailbox(23-25)user(17-19)src/backend/core/tests/api/conftest.py (1)
mailbox(59-61)src/backend/core/factories.py (2)
MailboxFactory(60-96)MailDomainFactory(51-57)src/backend/core/enums.py (1)
MailboxRoleChoices(15-21)
src/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.py (1)
src/backend/core/api/fields.py (1)
JSONField(8-25)
src/backend/core/api/viewsets/maildomain.py (2)
src/backend/core/api/permissions.py (1)
CanManageSettings(455-482)src/backend/core/api/serializers.py (3)
MailDomainAdminWriteSerializer(959-973)MailDomainAdminUpdateSerializer(994-1005)MailboxSettingsUpdateSerializer(1008-1016)
src/frontend/src/features/layouts/components/admin/modal-create-update-mailbox/index.tsx (2)
src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/forms/components/zod-json-schema-serializer/index.ts (1)
ItemJsonSchema(4-8)
🔇 Additional comments (49)
src/frontend/src/features/forms/components/react-hook-form/rhf-json-schema-field.tsx (1)
106-112: LGTM! The conversion toNumberand empty-string-to-nullhandling correctly addresses validation errors for numeric inputs.One consideration: if a numeric field is required, returning
nullfor empty string may cause a validation mismatch (expecting number, receiving null). Verify that the Zod schema for required numeric fields uses.nullable()or.optional()to align with this behavior.src/frontend/src/hooks/use-ability.ts (3)
15-16: LGTM! The newCAN_MANAGE_SETTINGSability for mailboxes follows the established pattern.
27-28: LGTM! The newCAN_MANAGE_MAILDOMAIN_SETTINGSability for mail domains is correctly added.
84-87: LGTM! The switch cases for the new settings abilities properly delegate to the boolean check on the resource's abilities.src/frontend/src/features/forms/components/message-form/index.tsx (2)
269-277: Consider addingschemato dependency array or memoizing trigger call.The
form.triggeruses the resolver which may be stale (as noted above). Additionally, this effect runs every timemaxRecipientsLimitchanges, but theformreference is stable so dependencies look correct.
614-614: LGTM!maxRecipientsprop is correctly passed to all recipient fields (to, cc, bcc).Also applies to: 631-631, 647-647
src/frontend/src/features/providers/config.tsx (1)
26-28: Code correctly adds recipient limit configuration. The generatedConfigRetrieve200type includes bothMAX_RECIPIENTS_PER_MESSAGE(required) andMAX_DEFAULT_RECIPIENTS_PER_MESSAGE(optional), with proper documentation. TheAppConfigtype correctly derives these fields, and the default values of 0 appropriately serve as fallbacks until the backend config is loaded.src/backend/core/tests/api/test_admin_maildomains_list.py (1)
360-380: Abilities expectations formanage_settingslook consistentThe added assertions that
abilities["manage_settings"] is Falsefor domain admins in both detail and list responses align with the backendMailDomain.get_abilitiesbehavior (settings reserved to superusers). This keeps API and tests in sync.Also applies to: 382-404
src/backend/core/tests/api/test_config.py (1)
18-32: Config test correctly covers new max-recipient settingsIncluding
MAX_RECIPIENTS_PER_MESSAGEandMAX_DEFAULT_RECIPIENTS_PER_MESSAGEin bothoverride_settingsand the asserted JSON response matches the ConfigView additions and guards against regressions on these public config keys.Also applies to: 44-58
src/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.py (1)
6-22: Migration forcustom_limitsfields looks correctBoth Mailbox and MailDomain get
JSONField(blank=True, default=dict, ...)forcustom_limits, which is appropriate for optional per-entity limits and avoids shared mutable defaults by passing the callabledict. No data migration needed here.src/backend/core/enums.py (1)
101-101: LGTM!The new
CAN_MANAGE_SETTINGSability options are consistently added to bothMailDomainAbilitiesandMailboxAbilities, following the existing naming conventions and patterns.Also applies to: 116-116
src/backend/core/api/permissions.py (1)
455-482: LGTM!The
CanManageSettingspermission class correctly implements object-level permission checks for bothMailDomainandMailboxobjects. The implementation properly:
- Validates user authentication
- Retrieves abilities via
obj.get_abilities(request.user)- Uses type-specific ability enums for the permission check
src/backend/messages/settings.py (1)
252-263: LGTM!The new recipient limit settings are well-documented and follow the existing configuration patterns. The comments clearly explain the hierarchy:
MAX_RECIPIENTS_PER_MESSAGE: absolute system maximumMAX_DEFAULT_RECIPIENTS_PER_MESSAGE: default when no custom limit is setThe model's
get_max_recipients_per_messagemethod correctly enforces these constraints usingmin().src/backend/core/tests/api/test_mailboxes.py (2)
370-371: LGTM!The test correctly verifies that
manage_settingsisFalsefor mailbox admins. Per the model'sget_abilitieslogic, only domain admins can manage mailbox settings, not mailbox-level admins.Also applies to: 390-391
422-422: LGTM!These assertions correctly verify that editor and viewer roles do not have
manage_settingsability, which aligns with the permission model where only domain admins can manage mailbox settings.Also applies to: 470-470
src/backend/core/mda/draft.py (2)
101-112: LGTM on the validation logic.The recipient limit enforcement is correctly implemented:
- Normalizes recipient lists to empty arrays
- Uses
mailbox.get_max_recipients_per_message()for the effective limit- Validates before processing to fail fast
252-274: LGTM on the partial update handling.The recipient limit validation during updates correctly handles partial updates by:
- Computing current counts per recipient type
- Using update values when provided, falling back to current counts
- Validating the total after applying updates
src/frontend/src/features/layouts/components/admin/modal-create-update-mailbox/index.tsx (2)
124-126: LGTM on the explicit type casting.The explicit cast to
[string, ItemJsonSchema][]is a reasonable workaround for TypeScript's type inference limitations withObject.entries(). This ensures type safety when iterating over the custom attributes.Also applies to: 135-136
415-426: LGTM!The JSON schema field rendering is correctly implemented with proper key naming and error state handling.
src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (1)
48-105: LGTM on the overall submit flow.The implementation correctly:
- Validates input as positive integer or empty
- Enforces global maximum limit
- Sends PATCH request with proper payload structure
- Invalidates relevant queries on success
- Handles errors with appropriate fallbacks
- Shows success toast notification
src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx (2)
60-63: LGTM! Clean integration of the new manage settings action.The handler follows the established pattern used by other actions (
handleManageAccess,handleUpdate, etc.), and the wiring toActionsRowis consistent.Also applies to: 150-150
263-268: Verify separator logic for the "Limits" menu item.The
showSeparatoris set to!onResetPassword, meaning the separator appears when reset password is unavailable. This seems intentional to visually group "Edit" and "Limits" together, separating them from "Delete". However, confirm this matches the desired UX.src/frontend/src/pages/domain/index.tsx (1)
199-225: LGTM! Clean permission-gated actions.The
ActionsCellcorrectly checks bothcanManageAccessesandcanManageSettingsseparately, and returnsnullwhen neither permission is available. This follows the pattern established in the codebase.src/backend/core/tests/api/test_admin_maildomains_update.py (3)
242-245: MailboxAccessFactory created without explicit role.This factory call creates mailbox access but doesn't specify a role. Per the factory definition in
factories.py,MailboxAccessdefaults toVIEWER. If the test intends to verify that admin access to the mailbox alone is insufficient (without domain admin), this is fine. Just confirming the intent.
25-186: LGTM! Comprehensive test coverage for maildomain custom_limits.The test class covers:
- Admin cannot update (403)
- Superuser can update (200)
- Clearing values works
- Unauthorized access (404)
- Non-admin forbidden (404)
- Other fields cannot be changed
- Global max enforcement (400)
This aligns well with the permission model where only superusers can manage domain-level settings.
189-363: LGTM! Good coverage for mailbox custom_limits and priority logic.The tests correctly verify:
- Domain admin can update mailbox limits
- Mailbox limits take priority over domain limits
- Domain limits used when mailbox has none
- Global max cannot be exceeded
- Mailbox admin alone cannot update settings (needs domain admin)
- Name updates work separately from settings
src/backend/core/tests/api/test_messages_create.py (4)
810-836: LGTM! Clear test for basic recipient limit enforcement.The test correctly verifies that when
MAX_RECIPIENTS_PER_MESSAGE=2, a draft with 3 recipients (to + cc + bcc) is rejected with 400.
838-934: LGTM! Good coverage for custom limit overrides.These tests correctly verify:
- Mailbox custom_limits overrides global settings
- Domain custom_limits overrides global settings
- Both success and failure cases are tested
936-986: LGTM! Validates priority: mailbox > domain.This test correctly verifies that when both mailbox (limit=2) and domain (limit=5) have custom limits, the mailbox limit takes precedence.
988-1031: LGTM! Tests recipient limit enforcement on draft updates.Verifies that the limit is also enforced when updating an existing draft, not just on creation.
src/backend/core/models.py (3)
735-764: LGTM! Well-documented priority logic for recipient limits.The method correctly implements the priority chain:
- Mailbox custom_limits
- Domain custom_limits
- Global default (capped by global max)
The
min()calls ensure values never exceedMAX_RECIPIENTS_PER_MESSAGE.Note: The
hasattrchecks (lines 745, 751-754) are technically redundant sincecustom_limitsis a model field that always exists (defaulting to{}), but they're harmless and add defensive coding.
387-388: LGTM! Superuser-only access for domain settings management.The comment clearly explains the restriction, and the implementation correctly ties
CAN_MANAGE_SETTINGStouser.is_superuserfor maildomains.
667-667: LGTM! Domain admin check for mailbox settings.
CAN_MANAGE_SETTINGSfor mailboxes correctly derives fromis_domain_admin, allowing domain administrators to manage settings for mailboxes under their domain.src/backend/core/api/viewsets/maildomain.py (4)
37-70: LGTM - UpdateModelMixin and permission/serializer routing look correct.The addition of
UpdateModelMixinand the correspondingget_permissions()andget_serializer_class()overrides properly route update actions to useCanManageSettingspermission andMailDomainAdminUpdateSerializer. The permission check ensures only users with settings management ability can update domain limits.
167-180: LGTM - Permission stacking for update_settings is appropriate.The
get_permissions()method correctly combines existing domain admin permissions withCanManageSettings, ensuring that only domain administrators with settings management ability can access this action.
356-386: LGTM - New update_settings endpoint is well-structured.The endpoint follows DRF patterns correctly: proper OpenAPI documentation, uses the appropriate serializer, and permission checks are enforced via the
get_permissions()override combined withself.get_object(). The partial update approach allows updating only the fields present in the request.
246-289: Removecustom_limitsfrom the OpenAPI schema for partial_update.The OpenAPI schema at line 261 includes
custom_limitsin theMailboxAdminUpdateMetadata, but theMailboxAdminSerializer.update()method does not process this field. Additionally,MailboxAdminCreateSerializerexplicitly excludescustom_limitswith a comment stating it "can only be modified via the dedicated /settings/ endpoint." This confirms thatcustom_limitsupdates should not be accepted viapartial_update. Removecustom_limitsfrom the inline serializer to avoid misleading users into sending data that will be silently ignored.Likely an incorrect or invalid review comment.
src/backend/core/api/serializers.py (4)
220-231: LGTM - max_recipients_per_message field added correctly.The new
max_recipients_per_messagefield is properly declared as a read-onlySerializerMethodFieldand the getter delegates to the model'sget_max_recipients_per_message()method, maintaining separation of concerns.Also applies to: 292-294
994-1016: LGTM - Update serializers are minimal and focused.Both
MailDomainAdminUpdateSerializerandMailboxSettingsUpdateSerializercorrectly inherit fromCustomLimitsUpdateMixinfirst (for proper MRO), expose only the necessary fields, and are appropriately scoped for their single purpose of updatingcustom_limits.
886-894: LGTM - custom_limits correctly exposed in admin serializer.The
custom_limitsfield is added to the read-onlyMailDomainAdminSerializer, allowing administrators to view the current limits. The field remains read-only as updates go through the dedicated update serializer.
1215-1241: LGTM - Clean exclusion of custom_limits from creation response.The list comprehension approach to exclude
custom_limitsfrom the parent's fields is readable, and the docstring clearly documents the design decision that limits can only be modified via the dedicated/settings/endpoint.src/backend/core/api/openapi.json (8)
3041-3141: Maildomain update/partial_update endpoints look correctly wiredThe new
PUT/PATCHoperations for/maildomains/{maildomain_pk}/correctly use the focusedMailDomainAdminUpdateRequest/PatchedMailDomainAdminUpdateRequestpayloads and returnMailDomainAdminUpdate, which keeps the update surface constrained tocustom_limits. This shape looks coherent with the admin use case.
5719-5785: MailDomainAdmin: exposing custom_limits and manage_settings is consistentAdding a read-only
custom_limitsfield and themanage_settingsability (plus updating therequiredlist accordingly) keeps the domain admin surface self-describing and matches the new permissions model. This schema looks internally consistent.
5787-5813: MailDomainAdminUpdate schemas are minimal and focused*The
MailDomainAdminUpdateandMailDomainAdminUpdateRequestschemas restrict updates tocustom_limitsand keepidread-only on the response, which is a good, narrow surface for administrative limit changes.
5815-5865: MailDomainAdminWrite description tweaks align with actual usage*The updated descriptions for
MailDomainAdminWriteandMailDomainAdminWriteRequestclarifying that they serve the “creating admin view” path are accurate and match the correspondingPOST /maildomains/endpoint semantics.Also applies to: 5863-5887
6223-6225: MailboxAdminCreate description clearly documents exclusion of custom_limitsThe note explaining that
custom_limitsis excluded from the create response and is only modifiable via/settings/is helpful and matches the separation of concerns introduced by the new settings endpoint.
6392-6409: MailboxSettingsUpdate schema matches the settings endpoint contract
MailboxSettingsUpdatemirrors the/settings/response needs (read-onlyid, customizablecustom_limits) and keeps the shape small and focused, which is appropriate for this specialized endpoint.
7111-7119: PatchedMailDomainAdminUpdateRequest is consistent with full update variantThe
PatchedMailDomainAdminUpdateRequestschema mirrorsMailDomainAdminUpdateRequestbut omitsrequired, which is correct for partial updates and aligns with the newPATCH /maildomains/{maildomain_pk}/behavior.
7142-7150: PatchedMailboxSettingsUpdateRequest correctly models partial settings updatesThe
PatchedMailboxSettingsUpdateRequestschema is the appropriate partial counterpart ofMailboxSettingsUpdateRequest(viacustom_limitsonly) and works well with the/settings/PATCHendpoint.
src/frontend/src/features/forms/components/react-hook-form/rhf-contact-combobox.tsx
Outdated
Show resolved
Hide resolved
src/frontend/src/features/layouts/components/admin/domains-view/settings-domain-action.tsx
Show resolved
Hide resolved
sdemagny
force-pushed
the
sdem/recipients_limit_per_msg
branch
from
bc5799f to
c2ce499
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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/models.py (1)
304-316: Misleading docstring or missing validation.The docstring claims "Validate custom attributes and limits," but the method only validates
custom_attributes. UnlikeMailbox.clean()(lines 535-551), there's no validation logic forcustom_limitsstructure or values.If validation for
MailDomain.custom_limitsis intentional, update the docstring:- def clean(self): - """Validate custom attributes and limits.""" + def clean(self): + """Validate custom attributes."""Otherwise, consider adding validation similar to
Mailbox.clean():def clean(self): """Validate custom attributes and limits.""" # Custom attributes schema try: jsonschema.validate( self.custom_attributes, settings.SCHEMA_CUSTOM_ATTRIBUTES_MAILDOMAIN ) except jsonschema.ValidationError as exception: raise ValidationError( {"custom_attributes": exception.message} ) from exception + # Validate custom_limits structure + if self.custom_limits: + if not isinstance(self.custom_limits, dict): + raise ValidationError( + {"custom_limits": "custom_limits must be a dictionary"} + ) + if "max_recipients_per_message" in self.custom_limits: + value = self.custom_limits["max_recipients_per_message"] + if value is not None and (not isinstance(value, int) or value < 1): + raise ValidationError( + { + "custom_limits": "max_recipients_per_message must be a positive integer" + } + ) + super().clean()src/backend/core/api/viewsets/maildomain.py (1)
256-265: Inconsistent:custom_limitsin partial_update schema but not supported.The inline serializer schema includes
custom_limits(line 261), suggesting it can be updated via the regularpartial_updateendpoint. However, based on the PR design,custom_limitsshould only be modifiable via the dedicated/settings/endpoint.If
custom_limitsshouldn't be settable via regular partial_update, remove it from the inline schema to avoid confusion:fields={ "full_name": drf_serializers.CharField( required=False, allow_blank=True ), "name": drf_serializers.CharField( required=False, allow_blank=True ), "custom_attributes": drf_serializers.JSONField(required=False), - "custom_limits": drf_serializers.JSONField(required=False), },Or if it should be supported, ensure the serializer/update logic handles it properly.
♻️ Duplicate comments (2)
src/backend/core/api/openapi.json (1)
5888-6001: Mailboxmax_recipients_per_messageis typed as string while representing a numeric limit
Mailbox.max_recipients_per_messageis declared with"type": "string"even though it represents a numeric limit and is conceptually tied to the integer config valuesMAX_RECIPIENTS_PER_MESSAGE/MAX_DEFAULT_RECIPIENTS_PER_MESSAGE. This is the same concern that was raised in a previous review; just confirming it still applies.If the serializer actually returns an integer, the OpenAPI schema should ideally use
"type": "integer"(and remainreadOnly: true) for consistency and better client typing. If the API intentionally returns a string (for example to allow values like"unlimited"), documenting that explicitly in adescriptionand/or using a more constrained schema (e.g.oneOfinteger or enum) would avoid confusion between config and per‑mailbox values.You can quickly check the serializer field type with something like:
#!/bin/bash # Inspect usages and field definition for max_recipients_per_message rg -n "max_recipients_per_message" src/backend/core -C3 || trueThen align the OpenAPI schema with whatever type the serializer actually exposes.
src/frontend/src/features/forms/components/message-form/index.tsx (1)
237-246: Verify resolver updates when schema changes.The schema is correctly memoized, but
zodResolver(schema)is called inline inuseForm(). According to the past review comment and React Hook Form documentation, the resolver itself should be memoized to ensure validation updates when the schema changes:const schema = useMemo(() => getMessageFormSchema(maxRecipientsLimit), [maxRecipientsLimit]); + const resolver = useMemo(() => zodResolver(schema), [schema]); const form = useForm({ - resolver: zodResolver(schema), + resolver, mode: "onChange", reValidateMode: "onChange", shouldFocusError: false, defaultValues: formDefaultValues, });This ensures the form validation uses the current limit when
maxRecipientsLimitchanges.
🧹 Nitpick comments (13)
src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (2)
35-35: Consider updating the TypeScript type instead of using type assertion.The
(domain as any)type assertion bypasses TypeScript's type checking. IfMailDomainAdminshould includecustom_limits, update the type definition in the API-generated types.Apply this diff to remove the type assertion once the type is updated:
- max_recipients_per_message: (domain as any)?.custom_limits?.max_recipients_per_message?.toString() ?? '', + max_recipients_per_message: domain.custom_limits?.max_recipients_per_message?.toString() ?? '',
48-64: Consider using react-hook-form's built-in validation.The manual validation in
onSubmitcould be replaced with react-hook-form's declarative validation rules. This would move validation logic closer to the form field definition.Example refactor:
const form = useForm<UpdateDomainFormData>({ defaultValues: { max_recipients_per_message: (domain as any)?.custom_limits?.max_recipients_per_message?.toString() ?? '', }, }); // Then in the Input component: <Input {...register('max_recipients_per_message', { validate: { positiveInteger: (value) => { if (value && (isNaN(Number(value)) || Number(value) < 1 || !Number.isInteger(Number(value)))) { return t('Please enter a valid positive integer or leave empty.'); } return true; }, withinMax: (value) => { if (value && Number(value) > MAX_RECIPIENTS_PER_MESSAGE) { return t('The limit cannot exceed the global maximum of {{max}} recipients.', { max: MAX_RECIPIENTS_PER_MESSAGE }); } return true; } } })} // ... other props />src/backend/core/api/openapi.json (3)
221-235: Config recipients limit constants look good; consider also requiring the default valueThe new
MAX_RECIPIENTS_PER_MESSAGEandMAX_DEFAULT_RECIPIENTS_PER_MESSAGEentries under/api/v1.0/config/are well described and correctly modeled as integers, and addingMAX_RECIPIENTS_PER_MESSAGEto therequiredlist matches the “cannot be exceeded” semantics.If
MAX_DEFAULT_RECIPIENTS_PER_MESSAGEis also always present in the backend response (even when not explicitly configured), consider adding it to therequiredarray so generated clients can rely on it being non‑optional; otherwise, it’s fine as is but clients should handle its absence.Also applies to: 237-250
3005-3141: Maildomain and mailbox settings endpoints are consistent; optional doc improvementsThe new
PUT/PATCH /api/v1.0/maildomains/{maildomain_pk}/endpoints andPATCH /api/v1.0/maildomains/{maildomain_pk}/mailboxes/{id}/settings/wire cleanly to the dedicatedMailDomainAdminUpdate*andMailboxSettingsUpdate*schemas and clearly scope changes tocustom_limits, which matches the PR intent.If you want richer API docs, you could later mirror neighboring endpoints by adding explicit
400/403/404responses for these operations, but that’s not strictly required for correctness.Also applies to: 3669-3735
5691-5813:custom_limitssurface is coherent; consider tightening schema shape for typed clientsThe addition of
custom_limitsacrossMailDomainAdmin,MailDomainAdminUpdate*,MailboxAdmin,MailboxAdminUpdateMetadataRequest,MailboxSettingsUpdate*, and related endpoints forms a consistent story for domain/mailbox‑level overrides, and the descriptions make their purpose (e.g. max recipients per message) clear.Right now
custom_limitsis intentionally untyped in OpenAPI (notype/nested properties), which keeps it flexible but means generated clients will treat it asany/unstructured JSON. If you expect clients to rely on specific keys likemax_recipients_per_message, it may be worth, in a follow‑up, declaring at least a minimal structure (e.g."type": "object",additionalProperties, and/or an example such as{"max_recipients_per_message": 50}) to improve discoverability and typing while still leaving room for future limits.Also applies to: 5815-5887, 6199-6219, 6222-6225, 6345-6356, 6392-6409, 7111-7119, 7142-7150
src/frontend/src/features/layouts/components/admin/modal-create-update-mailbox/index.tsx (2)
72-72: Consider improving type safety instead of casting toany.Casting
useConfig()toanybypasses TypeScript's type checking. Consider either:
- Updating the
useConfig()return type to includeSCHEMA_CUSTOM_ATTRIBUTES_USER, or- Using a more specific type assertion if the config shape is known.
Apply this pattern to improve type safety:
- const { SCHEMA_CUSTOM_ATTRIBUTES_USER } = useConfig() as any; + const config = useConfig(); + const SCHEMA_CUSTOM_ATTRIBUTES_USER = config.SCHEMA_CUSTOM_ATTRIBUTES_USER as JSONSchema.Schema;
124-126: Type casting pattern suggests a need for better type definitions.The repeated pattern of casting
Object.entries(customAttributes) as [string, ItemJsonSchema][]indicates that the type definitions forSCHEMA_CUSTOM_ATTRIBUTES_USER?.propertiescould be improved. While this works, consider defining a proper type forpropertiesto avoid the need for manual casts.As per coding guidelines and based on learnings,
SCHEMA_CUSTOM_ATTRIBUTES_MAILDOMAINdefaults to an empty object rather than being undefined, so the null checks with??are appropriate here.Also applies to: 135-135
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-limits/index.tsx (1)
35-35: Type casting suggests incomplete type definition.The cast
(mailbox as any)?.custom_limitsbypasses type checking. TheMailboxAdmininterface should include proper typing forcustom_limitsto avoid this cast. Consider updating the type definition or using a more specific type assertion.- max_recipients_per_message: (mailbox as any)?.custom_limits?.max_recipients_per_message?.toString() ?? '', + max_recipients_per_message: mailbox.custom_limits?.max_recipients_per_message?.toString() ?? '',This would work if
MailboxAdmin.custom_limitsis properly typed as{ max_recipients_per_message?: number }.src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx (1)
218-225: Type assertionas anysuggests missing type definition.The
(editedMailbox as any).updated_atcast bypasses type safety. Ifupdated_atis a valid property ofMailboxAdmin, it should be defined in the type. If it's not available, using it in the key could cause issues.Consider verifying that
updated_atexists in theMailboxAdmintype from the generated API types. If it does exist, remove theas anycast:- key={`${editedMailbox.id}-${(editedMailbox as any).updated_at}`} + key={`${editedMailbox.id}-${editedMailbox.updated_at}`}If
updated_atis not part of the type but is returned by the API, the type definition in the API generation should be updated.src/backend/core/tests/api/test_admin_maildomains_update.py (3)
9-10: Minor: Redundant import.
enumsis imported fromcoreon line 9 andMailDomainAccessRoleChoicesis imported fromcore.enumson line 10. You could use just one import style consistently.-from core import enums, factories -from core.enums import MailDomainAccessRoleChoices +from core import factories +from core.enums import MailDomainAccessRoleChoices, MailboxRoleChoicesThen update line 315 to use
MailboxRoleChoices.ADMINinstead ofenums.MailboxRoleChoices.ADMIN.
50-52: Assertion could be more explicit.The assertion
!= 50verifies the limit wasn't set, but doesn't verify what the actual value is. Ifcustom_limitsdefaults to an empty dict or None, a more explicit assertion would be clearer.# Verify the limit was NOT updated maildomain.refresh_from_db() - assert maildomain.custom_limits.get("max_recipients_per_message") != 50 + assert maildomain.custom_limits.get("max_recipients_per_message") is NoneThis makes the test intent clearer - the limit should not have been set at all.
242-245: Specify explicit role for MailboxAccessFactory to improve test clarity.The
MailboxAccessFactoryusesFuzzyChoiceto randomly select from available roles each time. While this doesn't affect the test's assertion (which validates limit priority, not permissions), explicit role specification improves test determinism and clarity. Consider addingrole=MailboxRoleChoices.ADMIN(or another appropriate role) to match the explicitMailDomainAccessFactorydefinition above it.src/backend/core/api/serializers.py (1)
976-991: Consider additional validation for edge cases.The mixin validates that
max_recipients_per_messagedoesn't exceed the global maximum, but doesn't validate:
- Negative values (e.g.,
-5)- Zero values (which may be invalid depending on business logic)
def validate_custom_limits(self, value): """Validate that custom_limits does not exceed global maximum.""" if value and isinstance(value, dict): max_recipients = value.get("max_recipients_per_message") if max_recipients is not None and isinstance(max_recipients, int): + if max_recipients <= 0: + raise serializers.ValidationError( + _("The limit must be a positive integer.") + ) if max_recipients > settings.MAX_RECIPIENTS_PER_MESSAGE: raise serializers.ValidationError( _( "The limit cannot exceed the global maximum of %(max)s recipients." ) % {"max": settings.MAX_RECIPIENTS_PER_MESSAGE} ) return valueThis aligns with the model's
get_max_recipients_per_message()which checksvalue > 0.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
src/frontend/src/features/api/gen/maildomains/maildomains.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/config_retrieve200.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_abilities.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_update.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_update_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_write.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_write_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_abilities.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin_create.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin_update_metadata_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_settings_update.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_mail_domain_admin_update_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_mailbox_settings_update_request.tsis excluded by!**/gen/**
📒 Files selected for processing (30)
src/backend/core/api/openapi.json(18 hunks)src/backend/core/api/permissions.py(1 hunks)src/backend/core/api/serializers.py(9 hunks)src/backend/core/api/viewsets/config.py(4 hunks)src/backend/core/api/viewsets/maildomain.py(5 hunks)src/backend/core/enums.py(2 hunks)src/backend/core/mda/draft.py(3 hunks)src/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.py(1 hunks)src/backend/core/models.py(9 hunks)src/backend/core/tests/api/test_admin_maildomains_list.py(3 hunks)src/backend/core/tests/api/test_admin_maildomains_update.py(1 hunks)src/backend/core/tests/api/test_config.py(2 hunks)src/backend/core/tests/api/test_mailboxes.py(4 hunks)src/backend/core/tests/api/test_messages_create.py(2 hunks)src/backend/core/tests/models/test_mailbox.py(6 hunks)src/backend/core/tests/models/test_maildomain.py(2 hunks)src/backend/messages/settings.py(1 hunks)src/e2e/src/__tests__/message-max-recipients-admin.spec.ts(1 hunks)src/e2e/src/__tests__/message-max-recipients.spec.ts(1 hunks)src/frontend/src/features/forms/components/message-form/index.tsx(10 hunks)src/frontend/src/features/forms/components/react-hook-form/rhf-contact-combobox.tsx(2 hunks)src/frontend/src/features/forms/components/react-hook-form/rhf-json-schema-field.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/domains-view/settings-domain-action.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx(7 hunks)src/frontend/src/features/layouts/components/admin/modal-create-update-mailbox/index.tsx(4 hunks)src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-limits/index.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx(1 hunks)src/frontend/src/features/providers/config.tsx(1 hunks)src/frontend/src/hooks/use-ability.ts(3 hunks)src/frontend/src/pages/domain/index.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/frontend/src/features/providers/config.tsx
- src/backend/core/mda/draft.py
- src/backend/core/enums.py
- src/backend/core/tests/api/test_config.py
- src/frontend/src/features/layouts/components/admin/domains-view/settings-domain-action.tsx
- src/e2e/src/tests/message-max-recipients.spec.ts
- src/backend/messages/settings.py
- src/e2e/src/tests/message-max-recipients-admin.spec.ts
- src/frontend/src/features/forms/components/react-hook-form/rhf-json-schema-field.tsx
- src/backend/core/api/viewsets/config.py
- src/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.py
🧰 Additional context used
📓 Path-based instructions (5)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Follow Django/PEP 8 style with a 100-character line limit
Use descriptive, snake_case names for variables and functions
Use Django ORM for database access; avoid raw SQL unless necessary for performance
Use Django’s built-in user model and authentication framework
Prefer try-except blocks to handle exceptions in business logic and views
Log expected and unexpected actions with appropriate log levels
Capture and report exceptions to Sentry; use capture_exception() for custom errors
Do not log sensitive information (tokens, passwords, financial/health data, PII)
Files:
src/backend/core/tests/api/test_mailboxes.pysrc/backend/core/tests/api/test_admin_maildomains_list.pysrc/backend/core/api/serializers.pysrc/backend/core/tests/api/test_messages_create.pysrc/backend/core/tests/models/test_mailbox.pysrc/backend/core/api/viewsets/maildomain.pysrc/backend/core/api/permissions.pysrc/backend/core/models.pysrc/backend/core/tests/models/test_maildomain.pysrc/backend/core/tests/api/test_admin_maildomains_update.py
src/backend/**/{tests.py,tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/{tests.py,tests/**/*.py}: Use Django’s testing tools (pytest-django) to ensure code quality and reliability
Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases
Files:
src/backend/core/tests/api/test_mailboxes.pysrc/backend/core/tests/api/test_admin_maildomains_list.pysrc/backend/core/tests/api/test_messages_create.pysrc/backend/core/tests/models/test_mailbox.pysrc/backend/core/tests/models/test_maildomain.pysrc/backend/core/tests/api/test_admin_maildomains_update.py
src/backend/{templates/**/*.html,**/serializers.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use Django templates for HTML and DRF serializers for JSON responses
Files:
src/backend/core/api/serializers.py
src/backend/**/{models.py,forms.py,views.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Keep business logic in models and forms; keep views thin and focused on request handling
Files:
src/backend/core/models.py
src/backend/**/{models.py,migrations/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Implement database indexing and query optimization (Model Meta indexes, constraints)
Files:
src/backend/core/models.py
🧠 Learnings (3)
📚 Learning: 2025-09-02T12:42:29.083Z
Learnt from: Nastaliss
Repo: suitenumerique/messages PR: 313
File: src/frontend/src/pages/domain/index.tsx:0-0
Timestamp: 2025-09-02T12:42:29.083Z
Learning: The domain admin pages in src/frontend/src/pages/domain/ use AdminLayout, not the main layout. Toast notifications for admin pages should be handled via a Toaster instance in the AdminLayout component.
Applied to files:
src/frontend/src/pages/domain/index.tsx
📚 Learning: 2025-09-02T12:20:22.006Z
Learnt from: Nastaliss
Repo: suitenumerique/messages PR: 313
File: src/frontend/src/features/layouts/components/admin/modal-create-domain/index.tsx:32-37
Timestamp: 2025-09-02T12:20:22.006Z
Learning: In the messages application, `SCHEMA_CUSTOM_ATTRIBUTES_MAILDOMAIN` from the config defaults to an empty object `{}` rather than being potentially undefined, so null checks are not needed when using this configuration value.
Applied to files:
src/frontend/src/features/layouts/components/admin/modal-create-update-mailbox/index.tsxsrc/backend/core/api/openapi.jsonsrc/frontend/src/features/forms/components/message-form/index.tsx
📚 Learning: 2025-09-02T10:12:12.835Z
Learnt from: CR
Repo: suitenumerique/messages PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-09-02T10:12:12.835Z
Learning: Applies to src/backend/**/views.py : Use Django REST Framework viewsets for API endpoints
Applied to files:
src/backend/core/api/viewsets/maildomain.py
🧬 Code graph analysis (13)
src/frontend/src/features/forms/components/react-hook-form/rhf-contact-combobox.tsx (2)
src/frontend/src/features/forms/components/combobox/index.tsx (2)
ComboBoxProps(8-15)ComboBox(17-230)src/frontend/src/features/providers/mailbox.tsx (1)
useMailboxContext(355-361)
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-limits/index.tsx (5)
src/frontend/src/features/api/gen/models/mailbox_admin.ts (1)
MailboxAdmin(14-39)src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/api/gen/maildomains/maildomains.ts (1)
useMaildomainsMailboxesSettingsUpdate(1869-1901)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(17-45)
src/frontend/src/pages/domain/index.tsx (4)
src/frontend/src/hooks/use-ability.ts (1)
Abilities(34-39)src/frontend/src/features/api/gen/maildomains/maildomains.ts (1)
getMaildomainsListQueryOptions(102-131)src/frontend/src/features/api/gen/models/mail_domain_admin.ts (1)
MailDomainAdmin(13-26)src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (1)
ModalUpdateDomain(24-153)
src/backend/core/api/serializers.py (2)
src/backend/core/models.py (3)
Mailbox(483-764)get_max_recipients_per_message(735-764)MailDomain(243-426)src/backend/core/api/viewsets/config.py (1)
get(133-181)
src/backend/core/tests/api/test_messages_create.py (3)
src/backend/core/tests/api/conftest.py (1)
mailbox(59-61)src/backend/core/factories.py (3)
MailboxAccessFactory(99-109)MailboxFactory(60-96)MailDomainFactory(51-57)src/backend/core/enums.py (1)
MailboxRoleChoices(15-21)
src/frontend/src/features/layouts/components/admin/modal-create-update-mailbox/index.tsx (2)
src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/forms/components/zod-json-schema-serializer/index.ts (1)
ItemJsonSchema(4-8)
src/backend/core/api/viewsets/maildomain.py (2)
src/backend/core/api/permissions.py (1)
CanManageSettings(455-482)src/backend/core/api/serializers.py (3)
MailDomainAdminWriteSerializer(959-973)MailDomainAdminUpdateSerializer(994-1005)MailboxSettingsUpdateSerializer(1008-1016)
src/backend/core/api/permissions.py (3)
src/backend/core/api/serializers.py (4)
get_abilities(132-141)get_abilities(165-167)get_abilities(288-290)get_abilities(910-912)src/backend/core/models.py (5)
get_abilities(225-240)get_abilities(351-389)get_abilities(619-677)MailDomain(243-426)Mailbox(483-764)src/backend/core/enums.py (2)
MailDomainAbilities(96-101)MailboxAbilities(104-116)
src/backend/core/models.py (3)
src/backend/core/api/fields.py (1)
JSONField(8-25)src/backend/core/enums.py (2)
MailDomainAbilities(96-101)MailboxAbilities(104-116)src/backend/core/api/serializers.py (1)
get_max_recipients_per_message(292-294)
src/backend/core/tests/models/test_maildomain.py (1)
src/backend/core/models.py (3)
get_abilities(225-240)get_abilities(351-389)get_abilities(619-677)
src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (5)
src/frontend/src/features/api/gen/models/mail_domain_admin.ts (1)
MailDomainAdmin(13-26)src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/api/fetch-api.ts (1)
fetchAPI(12-40)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(17-45)
src/backend/core/tests/api/test_admin_maildomains_update.py (4)
src/backend/core/enums.py (2)
MailDomainAccessRoleChoices(48-51)MailboxRoleChoices(15-21)src/backend/core/factories.py (2)
MailDomainFactory(51-57)MailboxFactory(60-96)src/backend/core/models.py (8)
save(92-95)save(207-210)save(299-302)save(530-533)save(1022-1074)save(1569-1572)save(1825-1841)get_max_recipients_per_message(735-764)src/backend/core/api/serializers.py (1)
get_max_recipients_per_message(292-294)
src/frontend/src/features/forms/components/message-form/index.tsx (2)
src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/providers/mailbox.tsx (1)
useMailboxContext(355-361)
🔇 Additional comments (40)
src/backend/core/api/permissions.py (1)
455-482: LGTM!The
CanManageSettingspermission class is well-implemented and follows Django REST Framework conventions. It properly checks authentication, delegates to the object'sget_abilitiesmethod, and handles bothMailDomainandMailboxtypes with appropriate fallback behavior.src/frontend/src/hooks/use-ability.ts (1)
15-15: LGTM!The new abilities are correctly defined and integrated into the
useAbilityhook, following the existing pattern for ability checks.Also applies to: 27-27, 84-86
src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (2)
81-86: Query invalidation approach is safe.The broad predicate matching all queries containing
/api/v1.0/maildomainsensures all related queries are refreshed. While this could be more targeted (e.g., using specific query keys), the current approach is safe and straightforward.
24-152: Well-structured modal component.The component demonstrates good practices:
- Proper form state management with react-hook-form
- Clear error handling and user feedback
- Cleanup on modal close
- Integration with translation and config systems
src/backend/core/tests/api/test_messages_create.py (6)
810-837: LGTM!The test correctly validates that drafts exceeding the global recipient limit are rejected with a 400 error.
838-883: LGTM!The test correctly validates that mailbox-level
custom_limitsoverride the globalMAX_RECIPIENTS_PER_MESSAGE, testing both rejection and success cases.
885-934: LGTM!The test correctly validates that maildomain-level
custom_limitsoverride the globalMAX_RECIPIENTS_PER_MESSAGE, with appropriate success and failure cases.
936-986: LGTM!The test correctly validates the precedence order where mailbox
custom_limitstake priority over maildomaincustom_limits. This is a critical behavior to test.
988-1031: LGTM!The test correctly validates that recipient limits are enforced when updating existing drafts, not just during creation.
1033-1078: LGTM! Past review comment addressed.The test has been correctly renamed and all comments updated to reflect that it validates
MAX_DEFAULT_RECIPIENTS_PER_MESSAGE(the default limit of 2) rather than the global maximum of 10. The test clearly demonstrates the fallback behavior when no custom limits are configured.src/backend/core/models.py (6)
282-289: LGTM!The
custom_limitsJSONField is correctly defined with appropriate defaults and help text.
387-388: LGTM!The
CAN_MANAGE_SETTINGSability is correctly restricted to superusers for maildomains, with clear inline documentation.
511-518: LGTM!The
custom_limitsJSONField onMailboxis correctly defined, mirroring the structure onMailDomain.
530-552: LGTM!The
save()andclean()methods properly enforce validation forcustom_limits:
- Ensures
full_clean()is called before saving- Validates the dictionary structure
- Validates that
max_recipients_per_messageis a positive integer when present
619-677: LGTM!The
get_abilities()method correctly:
- Initializes
roleto0(consistent withMailboxRoleChoicesbeing integer-based)- Checks if the user is a domain admin via
MailDomainAccess- Grants
CAN_MANAGE_SETTINGSto domain admins
735-764: LGTM!The
get_max_recipients_per_message()method correctly implements the precedence hierarchy (mailbox → domain → global default) and ensures the effective limit never exceedsMAX_RECIPIENTS_PER_MESSAGE. The logic is clear and well-commented.src/backend/core/tests/models/test_maildomain.py (1)
141-141: LGTM!The test updates comprehensively cover the new
CAN_MANAGE_SETTINGSability:
- Verifies it's
Falsefor users without access- Verifies it's
Falsefor domain admins (correctly restricted to superusers only)- Adds a dedicated test for superuser abilities where
manage_settingsisTrueAlso applies to: 160-177
src/backend/core/tests/api/test_admin_maildomains_list.py (1)
360-403: LGTM! Test expectations correctly reflect the new permission model.The updated assertions and docstrings properly enforce that
manage_settingsis restricted to superusers for mail domains, while domain admins have this ability set toFalse. This aligns with the newCanManageSettingspermission model introduced in the PR.src/backend/core/tests/models/test_mailbox.py (1)
116-210: LGTM! Test expectations correctly updated for mailbox abilities.The assertions properly enforce that
manage_settingsisFalseacross all mailbox access levels (no_access, viewer, editor, admin, sender). The updated docstring and comment clarify that only domain admins can manage settings for mailboxes, which aligns with the new permission model.src/backend/core/tests/api/test_mailboxes.py (1)
370-470: LGTM! Mailbox abilities test expectations correctly updated.The assertions and docstrings properly reflect that
manage_settingsis restricted to domain admins for mailboxes. The test expectations are consistent with the newCanManageSettingspermission model.src/frontend/src/features/layouts/components/admin/modal-create-update-mailbox/index.tsx (1)
415-426: LGTM! Key change and field rendering updated appropriately.The change from
json-schema-field-${name}tojson-schema-field-attr-${name}provides better semantic clarity for custom attribute fields. The removal of thetextprop and reliance onstatefor error display is also correct.src/frontend/src/features/forms/components/message-form/index.tsx (2)
58-102: Well-implemented dynamic recipient limit validation.The implementation correctly:
- Creates a dynamic schema factory with
superRefinefor total recipient validation- Tracks the max recipients limit based on the selected sender mailbox
- Updates the limit when the sender changes
- Re-validates recipient fields when the limit changes
The validation logic properly adds issues to all recipient fields (to, cc, bcc) when the total exceeds the limit.
Also applies to: 229-277
477-477: LGTM!keepErrors: truepreserves validation state correctly.Adding
keepErrors: trueto thereset()call ensures that validation messages are not lost when saving the draft, which is the correct behavior for this use case.src/frontend/src/features/forms/components/react-hook-form/rhf-contact-combobox.tsx (1)
11-16: LGTM! Dynamic recipient limit handling implemented correctly.The changes properly:
- Accept
maxRecipientsas an optional prop- Calculate the limit from props or the selected mailbox
- Use
formState.errorsfor reactivity to externalsetErrorcalls- Provide properly internationalized helper text with the maximum recipients info (addressing the past review comment)
- Trigger validation for all recipient fields on change to update error states
The i18n translation issue from the past review has been correctly resolved using
t()with interpolation.Also applies to: 52-80
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-limits/index.tsx (2)
48-93: Well-implemented validation and update logic.The implementation correctly:
- Validates for empty or positive integer values
- Enforces the global maximum limit with a clear error message
- Uses the dedicated
/settings/endpoint forcustom_limits- Sets
nullfor empty values to use domain/global defaults- Handles success with toast notification and error with banner display
97-99: LGTM! Clear and helpful helper text.The helper text properly informs users about the behavior (empty = use default) and the maximum allowed value, providing good UX guidance.
src/frontend/src/pages/domain/index.tsx (3)
25-25: LGTM! Clean implementation of domain settings management.The changes properly:
- Add a new
MANAGE_SETTINGSaction to the enum- Update the actions column width to accommodate both buttons
- Gate each action button by its respective ability (
CAN_MANAGE_MAILDOMAIN_ACCESSES,CAN_MANAGE_MAILDOMAIN_SETTINGS)- Render action buttons with appropriate labels and iconography
The button layout with flex-row and gap provides good visual separation between actions.
Also applies to: 65-82, 199-227
104-123: Excellent modal rendering with fresh data and proper key.The implementation correctly:
- Fetches fresh domain data from the list before passing to the modal (line 106)
- Uses a composite key
${freshDomain.id}-${freshDomain.updated_at}to force re-render when data changes (line 109)- Calls
refetchMaildomains()on success to refresh the list (line 117)- Properly resets state on close and success
This ensures the modal always displays current data and the list reflects updates.
32-39: LGTM! Query invalidation properly implemented.The
refetchMaildomainsfunction correctly invalidates the maildomains list queries using the query key fromgetMaildomainsListQueryOptions()withexact: falseto match all related queries.src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx (2)
60-63: LGTM!The
handleManageSettingshandler follows the established pattern used by other handlers (handleResetPassword,handleManageAccess,handleUpdate), maintaining consistency.
263-267: LGTM!The new "Limits" menu option is correctly positioned and the
showSeparatorlogic properly handles the conditional presence of the reset password option.src/backend/core/tests/api/test_admin_maildomains_update.py (3)
25-186: Comprehensive test coverage for maildomain custom_limits.The test class covers essential scenarios: admin vs superuser permissions, clearing values, unauthorized access, non-admin users, field isolation, and global max validation. Well-structured tests following Django/pytest conventions.
189-363: Good test coverage for mailbox custom_limits.Tests cover admin permissions, limit precedence (mailbox > domain > global), validation against global max, and permission boundaries. The tests align well with the
get_max_recipients_per_message()model method logic.
250-269: Missing@override_settingsdecorator.This test relies on
MAX_DEFAULT_RECIPIENTS_PER_MESSAGE=100but doesn't use@override_settings. If the global default differs, the assertion on line 258 could fail unexpectedly.+ @override_settings( + MAX_RECIPIENTS_PER_MESSAGE=200, MAX_DEFAULT_RECIPIENTS_PER_MESSAGE=100 + ) def test_domain_limit_used_when_mailbox_has_none(self): """Test that domain limit is used when mailbox has no custom limit.""" - # Create a mailbox with no custom limit mailbox = factories.MailboxFactory() assert mailbox.get_max_recipients_per_message() == 100Wait, I see the decorator is already on line 250-252. The test does have the decorator. Let me re-check...
Actually, looking more carefully at lines 250-252, the decorator IS present. My observation was incorrect.
src/backend/core/api/viewsets/maildomain.py (2)
173-179: Permission composition looks correct.The
update_settingsaction requires both base permissions (IsSuperUser OR IsMailDomainAdmin) ANDCanManageSettings. This ensures domain admins can only update settings if they have the specific ability.
356-386: Well-structured settings update endpoint.The
update_settingsaction is cleanly implemented with proper OpenAPI documentation, dedicated serializer, and clear separation from the general update endpoint.src/backend/core/api/serializers.py (4)
220-231: LGTM!The
max_recipients_per_messagefield is properly added as a read-only SerializerMethodField, delegating to the model'sget_max_recipients_per_message()method which handles the priority logic correctly.Also applies to: 292-294
994-1016: Clean serializer design for settings updates.The
MailDomainAdminUpdateSerializerandMailboxSettingsUpdateSerializerproperly use theCustomLimitsUpdateMixinand restrict writable fields to onlycustom_limits, preventing unintended modifications to other fields.
1235-1240: Good documentation and implementation for excluding custom_limits from creation.The list comprehension excludes
custom_limitsfrom creation response with a clear docstring explaining why. This enforces the design decision that limits can only be set via the dedicated/settings/endpoint.
886-893: LGTM!Adding
custom_limitstoMailDomainAdminSerializerfields allows reading the current limits in the admin view while keeping it read-only (per line 894).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/frontend/src/features/forms/components/message-form/index.tsx (1)
258-269: Consider optimizing effect dependencies.The effect includes
maxRecipientsLimitin the dependency array, but only uses it for comparison on line 266. This causes the effect to re-run whenever the limit changes, even though it only needs to run whenselectedFromMailboxId,mailboxes, orconfigchanges. While the early return prevents issues, you could optimize by removingmaxRecipientsLimitfrom the deps and using a ref or relying on the comparison alone.Optional refactor:
useEffect(() => { if (!selectedFromMailboxId || !mailboxes) return; const senderMailbox = mailboxes.find((mailbox) => mailbox.id === selectedFromMailboxId); const defaultLimit = Number(config.MAX_DEFAULT_RECIPIENTS_PER_MESSAGE ?? config.MAX_RECIPIENTS_PER_MESSAGE); const mailboxLimit = senderMailbox?.max_recipients_per_message; const newLimit = mailboxLimit ? Number(mailboxLimit) : defaultLimit; - if (newLimit !== maxRecipientsLimit) { - setMaxRecipientsLimit(newLimit); - } - }, [selectedFromMailboxId, mailboxes, config, maxRecipientsLimit]); + setMaxRecipientsLimit(newLimit); + }, [selectedFromMailboxId, mailboxes, config]);Note:
setMaxRecipientsLimitwill only trigger a re-render if the value actually changes (React batches state updates with the same value).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/frontend/src/features/forms/components/message-form/index.tsx(10 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-02T12:20:22.006Z
Learnt from: Nastaliss
Repo: suitenumerique/messages PR: 313
File: src/frontend/src/features/layouts/components/admin/modal-create-domain/index.tsx:32-37
Timestamp: 2025-09-02T12:20:22.006Z
Learning: In the messages application, `SCHEMA_CUSTOM_ATTRIBUTES_MAILDOMAIN` from the config defaults to an empty object `{}` rather than being potentially undefined, so null checks are not needed when using this configuration value.
Applied to files:
src/frontend/src/features/forms/components/message-form/index.tsx
🧬 Code graph analysis (1)
src/frontend/src/features/forms/components/message-form/index.tsx (2)
src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/providers/mailbox.tsx (1)
useMailboxContext(355-361)
🔇 Additional comments (7)
src/frontend/src/features/forms/components/message-form/index.tsx (7)
58-102: Well-structured dynamic validation schema.The factory function correctly enforces the total recipient limit across all fields (to + cc + bcc) using
superRefine. The error messages are properly localized and applied consistently to all recipient fields when the limit is exceeded.
229-235: Robust initialization of recipient limit.The state initialization correctly handles edge cases by falling back from the initial mailbox to the selected mailbox, and from mailbox-specific limits to config defaults. The logic ensures a valid limit is always set.
237-241: Excellent fix: resolver now updates with schema changes.The resolver is correctly memoized using
useMemo(() => zodResolver(schema), [schema]), ensuring that React Hook Form picks up validation changes whenevermaxRecipientsLimitchanges. This addresses the issue flagged in the previous review.
243-249: Form configuration correctly implements dynamic validation.The form now uses the memoized resolver and
mode: "onChange"for immediate validation feedback, which is appropriate for enforcing dynamic recipient limits as users type.
271-280: Effective revalidation on limit changes.The effect correctly triggers validation only when recipients are present, preventing unnecessary validation errors on empty forms. The implementation is sound.
480-480: Good UX: preserving validation errors across draft saves.Adding
keepErrors: trueensures that recipient limit validation errors persist after auto-save, providing consistent feedback to users without clearing important warnings.
617-617: Consistent propagation of recipient limit to all fields.The
maxRecipientsLimitis correctly passed to all three recipient input components (to, cc, bcc), ensuring consistent UI behavior and limit enforcement across all recipient fields.Also applies to: 634-634, 650-650
src/frontend/src/features/forms/components/message-form/index.tsx
Outdated
Show resolved
Hide resolved
sdemagny
force-pushed
the
sdem/recipients_limit_per_msg
branch
from
d3eba79 to
b2d5de7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/core/models.py (1)
304-316: Add validation forcustom_limitsinMailDomain.clean().The
MailDomain.clean()method validatescustom_attributesbut does not validate the newcustom_limitsfield. In contrast,Mailbox.clean()(lines 535-552) does validate itscustom_limits. This inconsistency could allow invalid limit configurations at the domain level.Consider adding similar validation:
def clean(self): """Validate custom attributes and limits.""" # Custom attributes schema try: jsonschema.validate( self.custom_attributes, settings.SCHEMA_CUSTOM_ATTRIBUTES_MAILDOMAIN ) except jsonschema.ValidationError as exception: raise ValidationError( {"custom_attributes": exception.message} ) from exception + # Validate custom_limits structure + if self.custom_limits: + if not isinstance(self.custom_limits, dict): + raise ValidationError( + {"custom_limits": "custom_limits must be a dictionary"} + ) + if "max_recipients_per_message" in self.custom_limits: + value = self.custom_limits["max_recipients_per_message"] + if value is not None and (not isinstance(value, int) or value < 1): + raise ValidationError( + { + "custom_limits": "max_recipients_per_message must be a positive integer" + } + ) + super().clean()
♻️ Duplicate comments (2)
src/frontend/src/features/forms/components/message-form/index.tsx (1)
29-29: Remove unused importuseConfigRetrieve.This was flagged in a previous review and appears to still be present. The import is not used anywhere in the file.
import { useConfig } from "@/features/providers/config"; -import { useConfigRetrieve } from "@/features/api/gen/config/config";src/backend/core/api/openapi.json (1)
5888-6000: Mailbox.max_recipients_per_message: string type vs numeric semantics
max_recipients_per_messageis declared as"type": "string"even though it represents a numeric limit and mirrors the integer-valuedMAX_RECIPIENTS_PER_MESSAGEin/config/. If the serializer returns an integer, consider changing this to"type": "integer"(keepingreadOnly: true) so generated clients get correct typing; if it really is a formatted string (e.g."unlimited"), it would help to document that explicitly in the field description to avoid confusion.
🧹 Nitpick comments (5)
src/frontend/src/features/forms/components/react-hook-form/rhf-contact-combobox.tsx (1)
52-54: Consider removing unusedfieldStatedestructuring.
fieldStateis destructured but not used sincefieldErroris computed fromformState.errorsdirectly. This is intentional per the comment, but the unused variable could be removed for clarity.- render={({ field, fieldState }) => { + render={({ field }) => {src/backend/core/models.py (1)
735-764: LGTM with minor simplification opportunity.The
get_max_recipients_per_messagemethod correctly implements the priority chain (mailbox → domain → global default) and properly caps values withmin()to respect the global maximum.The
hasattrchecks on lines 745 and 751-754 are redundant sincecustom_limitsis aJSONFieldwithdefault=dict, guaranteeing the attribute exists. However, this is defensive coding and doesn't affect correctness.src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-limits/index.tsx (1)
117-127: Enhance UX: Addstep="1"to number input.The number input currently allows decimal entry in the browser UI, though validation will catch non-integers. Adding
step="1"provides a better user experience by:
- Showing integer-only spinners in supporting browsers
- Hinting that only whole numbers are expected
Apply this diff:
<Input {...register('max_recipients_per_message')} label={t('Maximum recipients per message')} type="number" + step="1" min={1} max={MAX_RECIPIENTS_PER_MESSAGE} fullWidth state={errors.max_recipients_per_message ? 'error' : 'default'} text={helperText} value={maxRecipientsValue} />src/backend/core/api/openapi.json (2)
221-250: Config: new recipient limits fields look good; consider also requiring the default value
MAX_RECIPIENTS_PER_MESSAGE/MAX_DEFAULT_RECIPIENTS_PER_MESSAGEas integers with clear descriptions align well with the feature. IfMAX_DEFAULT_RECIPIENTS_PER_MESSAGEis always present in the response (even when using global defaults), you might want to add it to therequiredlist alongsideMAX_RECIPIENTS_PER_MESSAGEso clients can rely on it being defined.
6199-6209: MailboxAdmin.custom_limits exposure matches admin use; consider marking it readOnlyAdding
custom_limitsonMailboxAdmingives the admin UI the effective limits alongside other metadata. Since updates go throughMailboxAdminUpdateMetadataRequestand the/settings/endpoint rather than this schema, you might optionally markcustom_limitsasreadOnly: truehere to make it explicit that it’s not writable at this level in generated clients.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
src/frontend/src/features/api/gen/maildomains/maildomains.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/config_retrieve200.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_abilities.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_update.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_update_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_write.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_write_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_abilities.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin_create.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin_update_metadata_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_settings_update.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_mail_domain_admin_update_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_mailbox_settings_update_request.tsis excluded by!**/gen/**
📒 Files selected for processing (29)
src/backend/core/api/openapi.json(18 hunks)src/backend/core/api/permissions.py(1 hunks)src/backend/core/api/serializers.py(9 hunks)src/backend/core/api/viewsets/config.py(4 hunks)src/backend/core/api/viewsets/maildomain.py(5 hunks)src/backend/core/enums.py(2 hunks)src/backend/core/mda/draft.py(3 hunks)src/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.py(1 hunks)src/backend/core/models.py(9 hunks)src/backend/core/tests/api/test_admin_maildomains_list.py(3 hunks)src/backend/core/tests/api/test_admin_maildomains_update.py(1 hunks)src/backend/core/tests/api/test_config.py(2 hunks)src/backend/core/tests/api/test_mailboxes.py(4 hunks)src/backend/core/tests/api/test_messages_create.py(2 hunks)src/backend/core/tests/models/test_mailbox.py(6 hunks)src/backend/core/tests/models/test_maildomain.py(2 hunks)src/backend/messages/settings.py(1 hunks)src/e2e/src/__tests__/message-max-recipients-admin.spec.ts(1 hunks)src/e2e/src/__tests__/message-max-recipients.spec.ts(1 hunks)src/frontend/src/features/forms/components/message-form/index.tsx(10 hunks)src/frontend/src/features/forms/components/react-hook-form/rhf-contact-combobox.tsx(2 hunks)src/frontend/src/features/forms/components/react-hook-form/rhf-json-schema-field.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/domains-view/settings-domain-action.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx(7 hunks)src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-limits/index.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx(1 hunks)src/frontend/src/features/providers/config.tsx(1 hunks)src/frontend/src/hooks/use-ability.ts(3 hunks)src/frontend/src/pages/domain/index.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (15)
- src/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.py
- src/frontend/src/features/forms/components/react-hook-form/rhf-json-schema-field.tsx
- src/frontend/src/hooks/use-ability.ts
- src/e2e/src/tests/message-max-recipients.spec.ts
- src/backend/messages/settings.py
- src/backend/core/tests/api/test_admin_maildomains_update.py
- src/backend/core/api/permissions.py
- src/backend/core/tests/api/test_mailboxes.py
- src/backend/core/enums.py
- src/backend/core/api/viewsets/config.py
- src/backend/core/tests/models/test_mailbox.py
- src/backend/core/tests/api/test_admin_maildomains_list.py
- src/e2e/src/tests/message-max-recipients-admin.spec.ts
- src/backend/core/mda/draft.py
- src/frontend/src/features/providers/config.tsx
🧰 Additional context used
📓 Path-based instructions (5)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Follow Django/PEP 8 style with a 100-character line limit
Use descriptive, snake_case names for variables and functions
Use Django ORM for database access; avoid raw SQL unless necessary for performance
Use Django’s built-in user model and authentication framework
Prefer try-except blocks to handle exceptions in business logic and views
Log expected and unexpected actions with appropriate log levels
Capture and report exceptions to Sentry; use capture_exception() for custom errors
Do not log sensitive information (tokens, passwords, financial/health data, PII)
Files:
src/backend/core/tests/models/test_maildomain.pysrc/backend/core/tests/api/test_messages_create.pysrc/backend/core/api/viewsets/maildomain.pysrc/backend/core/tests/api/test_config.pysrc/backend/core/api/serializers.pysrc/backend/core/models.py
src/backend/**/{tests.py,tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/{tests.py,tests/**/*.py}: Use Django’s testing tools (pytest-django) to ensure code quality and reliability
Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases
Files:
src/backend/core/tests/models/test_maildomain.pysrc/backend/core/tests/api/test_messages_create.pysrc/backend/core/tests/api/test_config.py
src/backend/{templates/**/*.html,**/serializers.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use Django templates for HTML and DRF serializers for JSON responses
Files:
src/backend/core/api/serializers.py
src/backend/**/{models.py,forms.py,views.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Keep business logic in models and forms; keep views thin and focused on request handling
Files:
src/backend/core/models.py
src/backend/**/{models.py,migrations/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Implement database indexing and query optimization (Model Meta indexes, constraints)
Files:
src/backend/core/models.py
🧠 Learnings (3)
📚 Learning: 2025-09-02T12:42:29.083Z
Learnt from: Nastaliss
Repo: suitenumerique/messages PR: 313
File: src/frontend/src/pages/domain/index.tsx:0-0
Timestamp: 2025-09-02T12:42:29.083Z
Learning: The domain admin pages in src/frontend/src/pages/domain/ use AdminLayout, not the main layout. Toast notifications for admin pages should be handled via a Toaster instance in the AdminLayout component.
Applied to files:
src/frontend/src/pages/domain/index.tsx
📚 Learning: 2025-09-02T10:12:12.835Z
Learnt from: CR
Repo: suitenumerique/messages PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-09-02T10:12:12.835Z
Learning: Applies to src/backend/**/views.py : Use Django REST Framework viewsets for API endpoints
Applied to files:
src/backend/core/api/viewsets/maildomain.py
📚 Learning: 2025-09-02T12:20:22.006Z
Learnt from: Nastaliss
Repo: suitenumerique/messages PR: 313
File: src/frontend/src/features/layouts/components/admin/modal-create-domain/index.tsx:32-37
Timestamp: 2025-09-02T12:20:22.006Z
Learning: In the messages application, `SCHEMA_CUSTOM_ATTRIBUTES_MAILDOMAIN` from the config defaults to an empty object `{}` rather than being potentially undefined, so null checks are not needed when using this configuration value.
Applied to files:
src/frontend/src/features/forms/components/message-form/index.tsxsrc/backend/core/api/openapi.json
🧬 Code graph analysis (8)
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-limits/index.tsx (6)
src/frontend/src/features/api/gen/models/mailbox_admin.ts (1)
MailboxAdmin(14-39)src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/api/gen/maildomains/maildomains.ts (1)
useMaildomainsMailboxesSettingsUpdate(1869-1901)src/frontend/src/features/utils/task-import-cache/index.ts (1)
value(20-23)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(17-45)
src/backend/core/tests/models/test_maildomain.py (1)
src/backend/core/models.py (3)
get_abilities(225-240)get_abilities(351-389)get_abilities(619-677)
src/backend/core/api/viewsets/maildomain.py (4)
src/backend/core/api/permissions.py (1)
CanManageSettings(455-482)src/backend/core/api/viewsets/maildomain_access.py (1)
get_serializer_class(34-38)src/backend/core/api/viewsets/mailbox_access.py (1)
get_serializer_class(38-42)src/backend/core/api/serializers.py (2)
MailDomainAdminUpdateSerializer(994-1005)MailboxSettingsUpdateSerializer(1008-1016)
src/frontend/src/features/forms/components/message-form/index.tsx (2)
src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/providers/mailbox.tsx (1)
useMailboxContext(355-361)
src/frontend/src/features/layouts/components/admin/domains-view/settings-domain-action.tsx (3)
src/frontend/src/features/providers/admin-maildomain.tsx (1)
useAdminMailDomain(47-53)src/frontend/src/hooks/use-ability.ts (1)
Abilities(34-39)src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (1)
ModalUpdateDomain(24-153)
src/backend/core/api/serializers.py (1)
src/backend/core/models.py (3)
Mailbox(483-764)get_max_recipients_per_message(735-764)MailDomain(243-426)
src/backend/core/models.py (3)
src/backend/core/api/fields.py (1)
JSONField(8-25)src/backend/core/enums.py (3)
MailDomainAbilities(96-101)MailDomainAccessRoleChoices(48-51)MailboxAbilities(104-116)src/backend/core/api/serializers.py (1)
get_max_recipients_per_message(292-294)
src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx (2)
src/frontend/src/features/api/gen/models/mailbox_admin.ts (1)
MailboxAdmin(14-39)src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-limits/index.tsx (1)
ModalMailboxManageLimits(24-143)
🔇 Additional comments (17)
src/backend/core/tests/api/test_config.py (1)
30-31: LGTM!The test correctly validates that the new
MAX_RECIPIENTS_PER_MESSAGEandMAX_DEFAULT_RECIPIENTS_PER_MESSAGEconfiguration values are exposed via the/api/v1.0/config/endpoint for both authenticated and unauthenticated users.Also applies to: 56-57
src/frontend/src/features/forms/components/message-form/index.tsx (3)
271-280: LGTM!The revalidation effect correctly triggers validation of recipient fields when the max limit changes. The
formobject fromuseFormis stable, so including it in the dependency array is safe.
58-102: LGTM!The schema factory correctly implements cross-field validation for recipient limits using
superRefine. The logic properly:
- Short-circuits when no limit is configured
- Calculates total recipients across to/cc/bcc fields
- Adds validation issues only to fields that have values (except
towhich always gets the error)- Uses i18n for the error message with proper interpolation
229-269: LGTM!The recipient limit state initialization and update logic correctly:
- Prioritizes mailbox-specific limits over global defaults
- Falls back through config defaults consistently
- Updates the limit when the sender mailbox changes
- Properly converts values to numbers
src/backend/core/models.py (1)
535-552: LGTM!The
Mailbox.clean()method properly validates thecustom_limitsstructure:
- Ensures it's a dictionary
- Validates
max_recipients_per_messageis a positive integer when present- Provides clear error messages
As per coding guidelines, this follows Django best practices by keeping validation logic in the model.
src/backend/core/tests/models/test_maildomain.py (2)
141-161: LGTM!The updated tests correctly verify that:
- Users with no access have
manage_settings=False- Domain admins have
manage_settings=False(only superusers can manage domain settings per the implementation)This aligns with the comment in
models.pyline 387-388: "For now, only superusers can manage settings for maildomains."
163-177: Test correctly reflects implementation, but superuser abilities design warrants review.The test accurately asserts the current behavior: a superuser without an explicit domain role has
get=Falsebut can manage the domain (patch/put/post/delete/manage_accesses/manage_mailboxes/manage_settings). This matches the implementation whereCAN_READ: bool(role)depends only on having a role, while all other abilities depend onis_admin(which includes superuser status).However, this design is unusual—superusers can modify and delete a domain but cannot read it. While this behavior is implemented and tested, it lacks explicit documentation explaining the intent. Consider adding a comment in
MailDomain.get_abilities()to clarify whether this is intentional or ifCAN_READshould also respect superuser status for consistency.src/backend/core/api/openapi.json (10)
3005-3141: Maildomain PUT/PATCH endpoints are coherent with custom_limits-only updatesThe new
PUT/PATCHoperations on/api/v1.0/maildomains/{maildomain_pk}/usingMailDomainAdminUpdate*/PatchedMailDomainAdminUpdateRequestcleanly scope updates tocustom_limitsand keep the admin read model (MailDomainAdmin) separate. This shape looks consistent and future-proof.
3669-3735: Mailbox settings endpoint design matches the new custom_limits model
PATCH /api/v1.0/maildomains/{maildomain_pk}/mailboxes/{id}/settings/withPatchedMailboxSettingsUpdateRequest→MailboxSettingsUpdateis a clear, narrowly-scoped settings endpoint and lines up with the newmanage_settingsability. No OpenAPI issues spotted.
5692-5785: MailDomainAdmin: exposing custom_limits + manage_settings ability is consistentAdding
custom_limits(readOnly + required) and themanage_settingsflag underabilitiesaccurately reflects the new per-domain limits and permissions. This keeps the admin surface self-describing for the UI.
5787-5813: MailDomainAdminUpdate DTOs cleanly isolate the settings surface*
MailDomainAdminUpdate/MailDomainAdminUpdateRequestfocusing onid+custom_limits(and the patched variant) are a good way to constrain what can be written while still letting the admin read model stay richer. The schema wiring to the new PUT/PATCH endpoints looks correct.
5815-5865: MailDomainAdminWrite description tweak improves clarity*The updated description on
MailDomainAdminWrite/MailDomainAdminWriteRequest(“creating admin view”) better explains their role and matches the existing create endpoint behavior; nothing problematic here.
6222-6303: MailboxAdminCreate description correctly documents that custom_limits is not settableThe extended description on
MailboxAdminCreate(notingcustom_limitsis excluded at creation and only modifiable via/settings/) aligns with the API design and should help prevent misuse by client implementers.
6345-6356: MailboxAdminUpdateMetadataRequest: metadata.custom_limits hook is aligned with settings modelAdding
custom_attributesandcustom_limitsintoMailboxAdminUpdateMetadataRequestgives a clear place for mailbox-level overrides while keeping the top-level MailboxAdmin shape stable. This fits well with the new admin flows for per-mailbox limits.
6392-6409: MailboxSettingsUpdate schema is minimal and appropriate for the /settings/ endpoint
MailboxSettingsUpdate(id + custom_limits) is a sensible read model for the/settings/PATCH response, mirroring the domain-level update DTO pattern. Schema and required fields look correct.
7111-7119: PatchedMailDomainAdminUpdateRequest correctly mirrors the non-patched variantThe patched request schema for domain settings mirrors
MailDomainAdminUpdateRequestwhile allowing partial updates, which is exactly what the PATCH operation needs. No issues spotted.
7142-7150: PatchedMailboxSettingsUpdateRequest is consistent with MailboxSettingsUpdateThe patched mailbox settings request shape (only
custom_limits, all optional) aligns with the non-patchedMailboxSettingsUpdateschema and the PATCH semantics of the/settings/endpoint.
src/frontend/src/features/forms/components/react-hook-form/rhf-contact-combobox.tsx
Outdated
Show resolved
Hide resolved
| try { | ||
| // Use the dedicated /settings/ endpoint for custom_limits | ||
| await updateMailboxSettings({ | ||
| maildomainPk: domainId, | ||
| id: mailbox.id, | ||
| data: { | ||
| custom_limits: { | ||
| max_recipients_per_message: value === '' ? null : Number(value), | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| onSuccess(); | ||
| addToast( | ||
| <ToasterItem type="info"> | ||
| <Icon name="check" /> | ||
| <span>{t('The mailbox limits have been updated!')}</span> | ||
| </ToasterItem>, { | ||
| toastId: "toast_edit_mailbox_limits_success", | ||
| } | ||
| ); | ||
| handleClose(); | ||
|
|
||
| } catch (err: any) { | ||
| const errorMessage = err?.data?.custom_limits?.[0] || err?.data?.detail || t('An error occurred while updating the mailbox limits.'); | ||
| setError(errorMessage); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Critical: Missing state reset in error path.
When the API call fails, isSubmitting is never reset to false, leaving the Save button permanently disabled. Users cannot retry after an error.
Apply this diff to reset the submitting state in the catch block:
} catch (err: any) {
const errorMessage = err?.data?.custom_limits?.[0] || err?.data?.detail || t('An error occurred while updating the mailbox limits.');
setError(errorMessage);
+ setIsSubmitting(false);
}Alternatively, use a finally block:
} catch (err: any) {
const errorMessage = err?.data?.custom_limits?.[0] || err?.data?.detail || t('An error occurred while updating the mailbox limits.');
setError(errorMessage);
+ } finally {
+ setIsSubmitting(false);
}📝 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.
| try { | |
| // Use the dedicated /settings/ endpoint for custom_limits | |
| await updateMailboxSettings({ | |
| maildomainPk: domainId, | |
| id: mailbox.id, | |
| data: { | |
| custom_limits: { | |
| max_recipients_per_message: value === '' ? null : Number(value), | |
| }, | |
| }, | |
| }); | |
| onSuccess(); | |
| addToast( | |
| <ToasterItem type="info"> | |
| <Icon name="check" /> | |
| <span>{t('The mailbox limits have been updated!')}</span> | |
| </ToasterItem>, { | |
| toastId: "toast_edit_mailbox_limits_success", | |
| } | |
| ); | |
| handleClose(); | |
| } catch (err: any) { | |
| const errorMessage = err?.data?.custom_limits?.[0] || err?.data?.detail || t('An error occurred while updating the mailbox limits.'); | |
| setError(errorMessage); | |
| } | |
| }; | |
| try { | |
| // Use the dedicated /settings/ endpoint for custom_limits | |
| await updateMailboxSettings({ | |
| maildomainPk: domainId, | |
| id: mailbox.id, | |
| data: { | |
| custom_limits: { | |
| max_recipients_per_message: value === '' ? null : Number(value), | |
| }, | |
| }, | |
| }); | |
| onSuccess(); | |
| addToast( | |
| <ToasterItem type="info"> | |
| <Icon name="check" /> | |
| <span>{t('The mailbox limits have been updated!')}</span> | |
| </ToasterItem>, { | |
| toastId: "toast_edit_mailbox_limits_success", | |
| } | |
| ); | |
| handleClose(); | |
| } catch (err: any) { | |
| const errorMessage = err?.data?.custom_limits?.[0] || err?.data?.detail || t('An error occurred while updating the mailbox limits.'); | |
| setError(errorMessage); | |
| setIsSubmitting(false); | |
| } | |
| }; |
| try { | |
| // Use the dedicated /settings/ endpoint for custom_limits | |
| await updateMailboxSettings({ | |
| maildomainPk: domainId, | |
| id: mailbox.id, | |
| data: { | |
| custom_limits: { | |
| max_recipients_per_message: value === '' ? null : Number(value), | |
| }, | |
| }, | |
| }); | |
| onSuccess(); | |
| addToast( | |
| <ToasterItem type="info"> | |
| <Icon name="check" /> | |
| <span>{t('The mailbox limits have been updated!')}</span> | |
| </ToasterItem>, { | |
| toastId: "toast_edit_mailbox_limits_success", | |
| } | |
| ); | |
| handleClose(); | |
| } catch (err: any) { | |
| const errorMessage = err?.data?.custom_limits?.[0] || err?.data?.detail || t('An error occurred while updating the mailbox limits.'); | |
| setError(errorMessage); | |
| } | |
| }; | |
| try { | |
| // Use the dedicated /settings/ endpoint for custom_limits | |
| await updateMailboxSettings({ | |
| maildomainPk: domainId, | |
| id: mailbox.id, | |
| data: { | |
| custom_limits: { | |
| max_recipients_per_message: value === '' ? null : Number(value), | |
| }, | |
| }, | |
| }); | |
| onSuccess(); | |
| addToast( | |
| <ToasterItem type="info"> | |
| <Icon name="check" /> | |
| <span>{t('The mailbox limits have been updated!')}</span> | |
| </ToasterItem>, { | |
| toastId: "toast_edit_mailbox_limits_success", | |
| } | |
| ); | |
| handleClose(); | |
| } catch (err: any) { | |
| const errorMessage = err?.data?.custom_limits?.[0] || err?.data?.detail || t('An error occurred while updating the mailbox limits.'); | |
| setError(errorMessage); | |
| } finally { | |
| setIsSubmitting(false); | |
| } | |
| }; |
🤖 Prompt for AI Agents
In
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-limits/index.tsx
around lines 66 to 93, the catch path never resets the submitting state so the
Save button remains disabled after an error; update the error path to call
setIsSubmitting(false) (or better, move the reset into a finally block) so
isSubmitting is always cleared after the API call, and keep the existing
setError logic intact.
b2d5de7 to
0014c47
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx (1)
31-31: Past review comment about missing state reset is incorrect.A previous review comment flagged that
isSubmittingis never reset tofalsein the catch block (lines 89-92). However,isSubmittingis actually derived from React Query'sisPendingstate, which automatically resets when the mutation completes (success or failure). No manual state management is needed here.
🧹 Nitpick comments (16)
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx (2)
2-2: Add missing semicolon for consistency.The import statement on line 2 is missing a semicolon, while all other imports have them. This inconsistency could cause issues with certain build tools or code formatters.
Apply this diff:
-import React, { useState } from 'react' +import React, { useState } from 'react';
110-114: Verify translation handling for error messages.The error message is wrapped in
t()at line 112, buterrorMessageis constructed at line 90 and may already include translated strings (when it comes from manual validation) or raw API error messages. While this defensive approach ensures API errors are passed through, it could cause issues if translation keys are accidentally double-translated.Consider removing the
t()wrapper since error messages are already handled appropriately in the construction:- <Banner type="error"> - {t(error)} - </Banner> + <Banner type="error"> + {error} + </Banner>src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (3)
2-2: Add missing semicolon for consistency.The import statement is missing a semicolon, while all other imports have them. This is the same issue present in the mailbox settings modal.
Apply this diff:
-import React, { useState } from 'react' +import React, { useState } from 'react';
51-64: Consider extracting duplicated validation logic.The validation logic (lines 51-64) is identical to the mailbox settings modal at
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx(lines 52-64).Consider extracting this into a shared validation utility function:
// utils/validate-recipient-limit.ts export const validateRecipientLimit = ( value: string, maxLimit: number, t: (key: string, params?: any) => string ): { isValid: boolean; error?: string } => { if (value !== '' && (isNaN(Number(value)) || Number(value) < 1 || !Number.isInteger(Number(value)))) { return { isValid: false, error: t('Please enter a valid positive integer or leave empty.') }; } if (value !== '' && Number(value) > maxLimit) { return { isValid: false, error: t('The limit cannot exceed the global maximum of {{max}} recipients.', { max: maxLimit }) }; } return { isValid: true }; };Then use it in both modals to reduce duplication and improve maintainability.
120-124: Verify translation handling for error messages.Similar to the mailbox settings modal, the error message is wrapped in
t()at line 122, buterrorMessageat line 100 may already include translated strings or raw API errors. This is the same pattern as the mailbox modal.Consider removing the
t()wrapper for consistency:- <Banner type="error"> - {t(error)} - </Banner> + <Banner type="error"> + {error} + </Banner>src/backend/core/api/openapi.json (1)
3006-3142: Unify and typecustom_limitsacross domain/mailbox schemas and settings endpointsThe new domain/mailbox surfaces are coherent (custom_limits on
MailDomainAdmin/MailboxAdmin,max_recipients_per_messageonMailbox,manage_settingsabilities, and the/settings/endpoint returningMailboxSettingsUpdate). One thing that could help API consumers:
custom_limitsis currently an untyped free‑form field in multiple places (MailDomainAdmin*,MailboxAdmin,MailboxAdminUpdateMetadataRequest,MailboxSettingsUpdate*). For client codegen and self‑documentation, consider introducing a shared schema (e.g.CustomLimits) with an explicit object shape (at leastmax_recipients_per_message: integer, plusnullable/bounds if applicable) and referencing it everywherecustom_limitsappears instead of repeating an untyped property.This keeps behavior unchanged but improves typing consistency and reduces drift between the various update/read models and the
/settings/path.Also applies to: 3670-3735, 5693-5814, 5890-6001, 6145-6222, 6223-6304, 6347-6357, 6393-6410, 7112-7120, 7143-7151
src/backend/core/mda/draft.py (1)
252-274: Recipient limit validation is correct; consider consolidating count queries.The validation logic correctly handles partial updates by preserving existing counts for fields not being updated. However, lines 256-261 execute three separate
COUNTqueries. Consider consolidating into a single aggregation:- # Current counts per type - current_counts = {} - for kind, kind_choice in [ - ("to", enums.MessageRecipientTypeChoices.TO), - ("cc", enums.MessageRecipientTypeChoices.CC), - ("bcc", enums.MessageRecipientTypeChoices.BCC), - ]: - current_counts[kind] = message.recipients.filter(type=kind_choice).count() + # Current counts per type (single query) + from django.db.models import Count, Case, When, IntegerField + counts = message.recipients.aggregate( + to=Count(Case(When(type=enums.MessageRecipientTypeChoices.TO, then=1), output_field=IntegerField())), + cc=Count(Case(When(type=enums.MessageRecipientTypeChoices.CC, then=1), output_field=IntegerField())), + bcc=Count(Case(When(type=enums.MessageRecipientTypeChoices.BCC, then=1), output_field=IntegerField())), + ) + current_counts = { + "to": counts["to"] or 0, + "cc": counts["cc"] or 0, + "bcc": counts["bcc"] or 0, + }Note: The import should be moved to the top of the file alongside existing Django imports.
src/backend/core/tests/api/test_admin_maildomains_list.py (1)
378-379: Add test verifying superuser can manage settings for maildomains.The assertion correctly validates that domain admins cannot manage settings (lines 378-379, 402-403). However, corresponding superuser tests at lines 428-438 and 463-473 only verify HTTP status codes; they do not assert that
manage_settingsisTruefor superusers. Add an assertion in one of these superuser tests to confirm the permission is granted as expected.src/e2e/src/__tests__/message-max-recipients-admin.spec.ts (4)
2-2: Remove unused import.
getMailboxEmailis imported but never used in this file.import test, { expect } from "@playwright/test"; -import { getMailboxEmail } from "../utils"; import { signInKeycloakIfNeeded } from "../utils-test";
51-51: Fix grammatical error in test name.The test name "should super user can customize..." is grammatically incorrect.
- test("should super user can customize domain max recipients per message more than the global limit", async ({ page, browserName }) => { + test("should prevent super user from setting domain max recipients above the global limit", async ({ page, browserName }) => {
65-68: ReplacewaitForTimeoutwith explicit waits.Using
waitForTimeoutis a Playwright anti-pattern that leads to flaky tests. Consider waiting for a specific condition instead, such as waiting for the input to be visible and enabled.- await page.waitForTimeout(50); const maxRecipientsPerMessageInput = page.getByLabel("Maximum recipients per message"); - expect(maxRecipientsPerMessageInput).toBeVisible({ timeout: 5000 }); - console.log(maxRecipientsPerMessageInput); + await expect(maxRecipientsPerMessageInput).toBeVisible({ timeout: 5000 });Also applies to: 78-79
84-95: Incomplete test with TODO.This test is a placeholder. Consider either implementing it or removing it until it's ready.
Would you like me to help implement this test based on the patterns established in the other tests?
src/frontend/src/pages/domain/index.tsx (2)
94-103: Remove unnecessary non-null assertion.The non-null assertion on
editedDomain!(line 96) is redundant since the condition already checks thateditedDomainis truthy.{canManageMaildomainAccesses && editedDomain && ( <ModalMaildomainManageAccesses - domain={editedDomain!} + domain={editedDomain} isOpen={!!editedDomain && editAction === MailDomainEditAction.MANAGE_ACCESS}
104-123: Consider extracting fresh domain lookup.The IIFE pattern works but is unconventional. Consider extracting the fresh domain lookup for better readability.
+ {(() => { + if (!editedDomain || editAction !== MailDomainEditAction.MANAGE_SETTINGS) return null; + const freshDomain = domains.find(d => d.id === editedDomain.id) || editedDomain; + return ( + <ModalUpdateDomain + key={`${freshDomain.id}-${freshDomain.updated_at}`} + domain={freshDomain} + isOpen={true} + onClose={() => { + setEditedDomain(null) + setEditAction(null) + }} + onSuccess={() => { + refetchMaildomains(); + setEditedDomain(null) + setEditAction(null) + }} + /> + ); + })()}Alternatively, compute
freshDomainonce at the top of the component wheneditedDomainchanges:const freshDomain = editedDomain ? domains.find(d => d.id === editedDomain.id) || editedDomain : null;src/backend/core/models.py (2)
282-305: Consider validatingMailDomain.custom_limitssimilarly to mailbox limitsYou’ve added
MailDomain.custom_limitsand updated thecleandocstring to mention limits, butclean()still only validatescustom_attributes. This means malformed or negativemax_recipients_per_messagevalues can be stored at the domain level (they’re later ignored byget_max_recipients_per_message, but still end up in the DB).It would be cleaner and safer to mirror the
Mailbox.cleanchecks here (dict type, positive integer orNoneformax_recipients_per_message) so domain- and mailbox-level limits have consistent validation.
619-672: Double‑check whether mailbox settings should be manageable by superusers as well
Mailbox.get_abilitiesnow uses the annotatedis_domain_adminflag and setsMailboxAbilities.CAN_MANAGE_SETTINGSbased solely on that flag:is_domain_admin = ... MailboxAbilities.CAN_MANAGE_SETTINGS: is_domain_admin,Given that domain‑level
CAN_MANAGE_SETTINGSincludesuser.is_superuser, this asymmetry means a global superuser without explicitMailDomainAccess.ADMINwill not be able to manage mailbox settings viaCanManageSettings, even though they can for maildomains.If the intent is that superusers can always manage mailbox settings, consider:
MailboxAbilities.CAN_MANAGE_SETTINGS: is_domain_admin or user.is_superuser,Otherwise, this behavior is fine but should be a deliberate choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (17)
src/frontend/src/features/api/gen/maildomains/maildomains.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/config_retrieve200.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_abilities.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_update.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_update_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_write.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_write_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_abilities.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin_create.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin_update_metadata_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_settings_update.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_mail_domain_admin_update_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_mailbox_settings_update_request.tsis excluded by!**/gen/**
📒 Files selected for processing (30)
src/backend/core/api/openapi.json(18 hunks)src/backend/core/api/permissions.py(1 hunks)src/backend/core/api/serializers.py(9 hunks)src/backend/core/api/viewsets/config.py(4 hunks)src/backend/core/api/viewsets/mailbox.py(3 hunks)src/backend/core/api/viewsets/maildomain.py(6 hunks)src/backend/core/enums.py(2 hunks)src/backend/core/mda/draft.py(3 hunks)src/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.py(1 hunks)src/backend/core/models.py(9 hunks)src/backend/core/tests/api/test_admin_maildomains_list.py(3 hunks)src/backend/core/tests/api/test_admin_maildomains_update.py(1 hunks)src/backend/core/tests/api/test_config.py(2 hunks)src/backend/core/tests/api/test_mailboxes.py(4 hunks)src/backend/core/tests/api/test_messages_create.py(2 hunks)src/backend/core/tests/models/test_mailbox.py(6 hunks)src/backend/core/tests/models/test_maildomain.py(2 hunks)src/backend/messages/settings.py(1 hunks)src/e2e/src/__tests__/message-max-recipients-admin.spec.ts(1 hunks)src/e2e/src/__tests__/message-max-recipients.spec.ts(1 hunks)src/frontend/src/features/forms/components/message-form/index.tsx(10 hunks)src/frontend/src/features/forms/components/react-hook-form/rhf-contact-combobox.tsx(2 hunks)src/frontend/src/features/forms/components/react-hook-form/rhf-json-schema-field.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/domains-view/settings-domain-action.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx(7 hunks)src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx(1 hunks)src/frontend/src/features/providers/config.tsx(1 hunks)src/frontend/src/hooks/use-ability.ts(3 hunks)src/frontend/src/pages/domain/index.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/frontend/src/features/forms/components/react-hook-form/rhf-json-schema-field.tsx
- src/backend/core/api/permissions.py
- src/backend/core/api/viewsets/config.py
- src/e2e/src/tests/message-max-recipients.spec.ts
- src/backend/core/tests/api/test_mailboxes.py
- src/backend/core/tests/models/test_mailbox.py
- src/frontend/src/features/layouts/components/admin/domains-view/settings-domain-action.tsx
- src/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.py
- src/frontend/src/features/providers/config.tsx
- src/backend/messages/settings.py
🧰 Additional context used
📓 Path-based instructions (5)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Follow Django/PEP 8 style with a 100-character line limit
Use descriptive, snake_case names for variables and functions
Use Django ORM for database access; avoid raw SQL unless necessary for performance
Use Django’s built-in user model and authentication framework
Prefer try-except blocks to handle exceptions in business logic and views
Log expected and unexpected actions with appropriate log levels
Capture and report exceptions to Sentry; use capture_exception() for custom errors
Do not log sensitive information (tokens, passwords, financial/health data, PII)
Files:
src/backend/core/tests/api/test_config.pysrc/backend/core/mda/draft.pysrc/backend/core/tests/api/test_admin_maildomains_list.pysrc/backend/core/api/viewsets/mailbox.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_admin_maildomains_update.pysrc/backend/core/enums.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/maildomain.pysrc/backend/core/tests/api/test_messages_create.pysrc/backend/core/tests/models/test_maildomain.py
src/backend/**/{tests.py,tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/{tests.py,tests/**/*.py}: Use Django’s testing tools (pytest-django) to ensure code quality and reliability
Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases
Files:
src/backend/core/tests/api/test_config.pysrc/backend/core/tests/api/test_admin_maildomains_list.pysrc/backend/core/tests/api/test_admin_maildomains_update.pysrc/backend/core/tests/api/test_messages_create.pysrc/backend/core/tests/models/test_maildomain.py
src/backend/**/{models.py,forms.py,views.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Keep business logic in models and forms; keep views thin and focused on request handling
Files:
src/backend/core/models.py
src/backend/**/{models.py,migrations/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Implement database indexing and query optimization (Model Meta indexes, constraints)
Files:
src/backend/core/models.py
src/backend/{templates/**/*.html,**/serializers.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use Django templates for HTML and DRF serializers for JSON responses
Files:
src/backend/core/api/serializers.py
🧠 Learnings (4)
📚 Learning: 2025-09-02T12:42:29.083Z
Learnt from: Nastaliss
Repo: suitenumerique/messages PR: 313
File: src/frontend/src/pages/domain/index.tsx:0-0
Timestamp: 2025-09-02T12:42:29.083Z
Learning: The domain admin pages in src/frontend/src/pages/domain/ use AdminLayout, not the main layout. Toast notifications for admin pages should be handled via a Toaster instance in the AdminLayout component.
Applied to files:
src/frontend/src/pages/domain/index.tsx
📚 Learning: 2025-09-02T10:12:12.835Z
Learnt from: CR
Repo: suitenumerique/messages PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-09-02T10:12:12.835Z
Learning: Applies to src/backend/**/views.py : Use Django REST Framework viewsets for API endpoints
Applied to files:
src/backend/core/api/viewsets/mailbox.pysrc/backend/core/api/viewsets/maildomain.py
📚 Learning: 2025-09-02T10:12:12.835Z
Learnt from: CR
Repo: suitenumerique/messages PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-09-02T10:12:12.835Z
Learning: Applies to src/backend/**/views.py : Optimize related object fetching with select_related and prefetch_related
Applied to files:
src/backend/core/models.py
📚 Learning: 2025-09-02T12:20:22.006Z
Learnt from: Nastaliss
Repo: suitenumerique/messages PR: 313
File: src/frontend/src/features/layouts/components/admin/modal-create-domain/index.tsx:32-37
Timestamp: 2025-09-02T12:20:22.006Z
Learning: In the messages application, `SCHEMA_CUSTOM_ATTRIBUTES_MAILDOMAIN` from the config defaults to an empty object `{}` rather than being potentially undefined, so null checks are not needed when using this configuration value.
Applied to files:
src/backend/core/api/openapi.jsonsrc/frontend/src/features/forms/components/message-form/index.tsx
🧬 Code graph analysis (13)
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx (5)
src/frontend/src/features/api/gen/models/mailbox_admin.ts (1)
MailboxAdmin(14-39)src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/api/gen/maildomains/maildomains.ts (1)
useMaildomainsMailboxesSettingsUpdate(1869-1901)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(17-45)
src/frontend/src/features/forms/components/react-hook-form/rhf-contact-combobox.tsx (2)
src/frontend/src/features/forms/components/combobox/index.tsx (2)
ComboBoxProps(8-15)ComboBox(17-230)src/frontend/src/features/providers/mailbox.tsx (1)
useMailboxContext(355-361)
src/backend/core/mda/draft.py (2)
src/backend/core/models.py (1)
get_max_recipients_per_message(739-768)src/backend/core/enums.py (1)
MessageRecipientTypeChoices(31-36)
src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (5)
src/frontend/src/features/api/gen/models/mail_domain_admin.ts (1)
MailDomainAdmin(13-26)src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/api/fetch-api.ts (1)
fetchAPI(12-40)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(17-45)
src/backend/core/api/viewsets/mailbox.py (4)
src/backend/core/models.py (1)
MailDomainAccess(1649-1671)src/backend/core/tests/models/test_mailbox.py (1)
user(17-19)src/backend/core/tests/models/test_maildomain.py (2)
user(16-18)maildomain(22-24)src/backend/core/enums.py (1)
MailDomainAccessRoleChoices(48-51)
src/backend/core/models.py (3)
src/backend/core/api/fields.py (1)
JSONField(8-25)src/backend/core/enums.py (2)
MailDomainAbilities(96-101)MailboxAbilities(104-116)src/backend/core/api/serializers.py (1)
get_max_recipients_per_message(293-295)
src/backend/core/tests/api/test_admin_maildomains_update.py (3)
src/backend/core/enums.py (1)
MailDomainAccessRoleChoices(48-51)src/backend/core/models.py (1)
get_max_recipients_per_message(739-768)src/backend/core/api/serializers.py (1)
get_max_recipients_per_message(293-295)
src/e2e/src/__tests__/message-max-recipients-admin.spec.ts (1)
src/e2e/src/utils-test.ts (1)
signInKeycloakIfNeeded(6-39)
src/backend/core/api/serializers.py (1)
src/backend/core/models.py (19)
Meta(89-90)Meta(199-202)Meta(291-294)Meta(465-477)Meta(520-525)Meta(790-794)Meta(833-836)Meta(1016-1021)Meta(1188-1192)Meta(1209-1213)Meta(1250-1254)Meta(1325-1329)Meta(1556-1568)Meta(1624-1628)Meta(1664-1668)Meta(1718-1722)Mailbox(483-768)get_max_recipients_per_message(739-768)MailDomain(243-426)
src/backend/core/api/viewsets/maildomain.py (4)
src/backend/core/api/permissions.py (1)
CanManageSettings(455-482)src/backend/core/api/serializers.py (2)
MailDomainAdminUpdateSerializer(995-1006)MailboxSettingsUpdateSerializer(1009-1017)src/backend/core/api/viewsets/mailbox.py (1)
get_queryset(24-47)src/backend/core/enums.py (1)
MailDomainAccessRoleChoices(48-51)
src/frontend/src/features/forms/components/message-form/index.tsx (2)
src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/providers/mailbox.tsx (1)
useMailboxContext(355-361)
src/backend/core/tests/api/test_messages_create.py (1)
src/backend/core/api/viewsets/draft.py (2)
post(197-231)put(234-281)
src/backend/core/tests/models/test_maildomain.py (1)
src/backend/core/models.py (3)
get_abilities(225-240)get_abilities(351-389)get_abilities(619-681)
🔇 Additional comments (33)
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx (1)
24-143: Overall implementation looks solid.The component follows React and React Hook Form best practices. The mutation handling, error management, and user feedback (toasts, validation messages) are all implemented correctly. The use of React Query's mutation hook properly manages the loading state.
src/frontend/src/features/forms/components/message-form/index.tsx (6)
69-100: Recipient limit validation is well-implemented.The
superRefinevalidation correctly:
- Sums recipients across all three fields (to, cc, bcc)
- Adds validation errors to each populated recipient field
- Uses i18n for error messages
- Guards against undefined
maxRecipientswith an early returnThis approach ensures users see the same error message on all recipient fields when the limit is exceeded, which provides clear feedback.
228-240: Dynamic limit initialization and schema memoization are correct.The implementation properly:
- Initializes the limit from the form's initial sender mailbox with appropriate fallbacks
- Memoizes the schema based on the current limit
- Memoizes the resolver to update when the schema changes (addressing a previous review comment)
This ensures validation reflects the current mailbox's recipient limit throughout the form lifecycle.
257-279: Dynamic limit update and revalidation logic is well-designed.The two effects work together correctly:
- Sender change effect (lines 257-268): Updates the limit when the user changes the sender mailbox, properly extracting the mailbox-specific limit or falling back to the default.
- Revalidation effect (lines 271-279): Intelligently triggers validation only when the limit changes AND recipients exist, avoiding unnecessary validation runs.
The optimization to check for existing recipients before triggering validation is a nice touch that prevents validation errors from appearing prematurely.
479-479: Preserving errors during draft save is correct.The
keepErrors: trueoption ensures that validation errors remain visible after the draft auto-saves. This is the right behavior because:
- Draft saving is a background operation
- Validation errors (like exceeding recipient limits) need to persist so users can correct them before sending
- Without this, errors would disappear after auto-save, confusing users
This addresses a UX issue where validation feedback would be lost during the auto-save cycle.
616-616: Consistent prop passing to all recipient fields.The
maxRecipientsprop is correctly passed to all three recipient input components (to, cc, bcc). This ensures the UI consistently reflects the current mailbox's recipient limit across all recipient fields.Also applies to: 633-633, 649-649
57-101: Dynamic recipient limit implementation is comprehensive and well-executed.The changes successfully implement dynamic recipient limits that:
- Adapt to the selected sender mailbox
- Provide real-time validation across all recipient fields
- Preserve validation state through auto-save cycles
- Integrate seamlessly with the existing form logic
The implementation follows React best practices, properly manages state and effects, and provides a good user experience by validating only when necessary.
Also applies to: 228-279
src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (2)
66-104: API call and state management are implemented correctly.The manual state management with
setIsSubmittingin try/finally is the correct pattern when usingfetchAPIdirectly. Thefinallyblock ensures the loading state is always reset, unlike the mailbox modal which relies on React Query's automatic state management.The query invalidation using a predicate (lines 81-86) is a pragmatic approach that ensures all maildomains-related queries are refreshed after the update.
24-153: Implementation is solid with correct state management.The component correctly implements manual loading state management with try/finally, appropriate query invalidation, and proper error handling. The structure mirrors the mailbox settings modal, which provides consistency across the admin UI.
The main improvement opportunity is extracting the duplicated validation logic (already noted in a separate comment).
src/backend/core/api/openapi.json (1)
221-235: New recipient limit config constants are consistent and well‑modeled
MAX_RECIPIENTS_PER_MESSAGEandMAX_DEFAULT_RECIPIENTS_PER_MESSAGEare exposed asintegerand markedreadOnlyandrequired, matching their numeric semantics and existing config conventions (e.g. other MAX_* sizes). Descriptions clearly state global vs default behavior; no changes needed here.Also applies to: 237-251
src/backend/core/tests/api/test_admin_maildomains_list.py (1)
402-403: Consistent with detail view test.Same assertion pattern applied to list view, ensuring both endpoints return consistent abilities for domain admins.
src/backend/core/tests/api/test_config.py (1)
30-31: LGTM! Config test correctly covers new recipient limit settings.The test properly validates that
MAX_RECIPIENTS_PER_MESSAGEandMAX_DEFAULT_RECIPIENTS_PER_MESSAGEare exposed in the public API configuration. Values are consistent between override settings and expected response.Also applies to: 56-57
src/backend/core/tests/models/test_maildomain.py (2)
141-141: LGTM! Correctly extends no-access test.Adding the
manage_settingsassertion ensures complete coverage of the abilities response.
160-161: LGTM! Correctly validates admin cannot manage settings.The comment clarifies the permission model: domain admins can perform CRUD and manage accesses/mailboxes, but only superusers can manage settings.
src/backend/core/mda/draft.py (2)
101-112: LGTM! Proper recipient limit enforcement on draft creation.The validation correctly:
- Normalizes recipient lists to avoid
Noneissues- Sums all recipient types (to + cc + bcc)
- Validates against the mailbox's effective limit
- Provides a clear error message with actual vs. maximum counts
184-186: Correct simplification after normalization.Since
to_emails,cc_emails, andbcc_emailsare normalized to empty lists at lines 102-104, the fallbackor []is no longer needed here.src/backend/core/api/viewsets/mailbox.py (1)
37-44: LGTM! Good N+1 query optimization.The
is_domain_adminannotation pre-computes domain admin status during the initial query, avoiding repeated database hits inget_abilities(). TheExistssubquery is the correct approach for boolean annotations. The implementation includes a smart try-except fallback (models.py, lines 628-636) that gracefully handles cases where the annotation may not be present, ensuring consistency across different code paths.src/frontend/src/hooks/use-ability.ts (1)
15-16: LGTM!The new abilities
CAN_MANAGE_SETTINGSandCAN_MANAGE_MAILDOMAIN_SETTINGSare correctly added to their respective enums and properly handled in the switch statement following the existing pattern.Also applies to: 27-28, 84-87
src/backend/core/enums.py (1)
101-101: LGTM!The new
CAN_MANAGE_SETTINGSabilities are correctly added to bothMailDomainAbilitiesandMailboxAbilitiesenums following the existing Django TextChoices pattern with appropriate value/label pairs.Also applies to: 116-116
src/frontend/src/pages/domain/index.tsx (1)
199-226: LGTM!The ActionsCell component correctly checks both
CAN_MANAGE_MAILDOMAIN_ACCESSESandCAN_MANAGE_MAILDOMAIN_SETTINGSabilities before rendering their respective buttons, and returns null early when neither permission is granted.src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx (2)
60-63: LGTM!The
handleManageSettingsfunction and its wiring toActionsRowfollows the established pattern used for other edit actions in this component.Also applies to: 150-151, 237-240
218-225: Remove unnecessary type assertion;updated_atis already defined inMailboxAdmin.The
MailboxAdmintype includesreadonly updated_at: string, so theas anycast on line 219 is unnecessary and can be removed. TypeScript should recognize the field without the assertion.Likely an incorrect or invalid review comment.
src/frontend/src/features/forms/components/react-hook-form/rhf-contact-combobox.tsx (1)
11-79: Recipient limit wiring and form error handling look solidUsing
maxRecipients/selectedMailbox?.max_recipients_per_messageto computeMAX_RECIPIENTS_PER_MESSAGE, surfacing it in a translated helper text, and drivingComboBoxerror state fromformState.errorswithsetValue(..., { shouldDirty: true, shouldValidate: true })plus cross-fieldtrigger(['to', 'cc', 'bcc'])is consistent and correct for enforcing the new recipient limits in the UI. I don’t see issues with this integration.src/backend/core/tests/api/test_messages_create.py (1)
9-1078: Recipient‑limit test coverage is thorough and consistent with model logicThe new tests cleanly exercise global limits, mailbox/domain overrides, precedence rules, and the update‑draft path (both via
override_settingsandcustom_limits), and the expectations line up withMailbox.get_max_recipients_per_message. I don’t see gaps or contradictions here.src/backend/core/tests/api/test_admin_maildomains_update.py (2)
24-186: Good end‑to‑end coverage of maildomain custom_limits permissions and boundsThe domain‑level tests clearly distinguish unauthenticated, non‑admin, admin, and superuser behaviors, and also assert that
custom_limits.max_recipients_per_messagecannot exceed the global max. The permission and validation expectations are consistent with the new serializers, permissions, and model behavior.
132-160: The test behavior is correct and will pass. DRF 3.15.2'sModelSerializersilently ignores input fields not declared inMeta.fieldsby default—it does not return400 Bad Request. The serializer will ignore the"name"field in the request and process only"custom_limits", resulting in the200 OKresponse with the name unchanged as expected. No code changes are required.Likely an incorrect or invalid review comment.
src/backend/core/models.py (2)
511-553: Mailboxcustom_limitsvalidation is consistent and defensiveThe new JSONField plus
clean()checks (dict type and positive integermax_recipients_per_message) provide a solid validation layer for mailbox-level limits, and align with howget_max_recipients_per_messageinterprets them. This is a good place to enforce structure before hitting the database.
739-768: Recipient limit resolution logic matches documented priority
get_max_recipients_per_message()correctly implements:
- Mailbox
custom_limits.max_recipients_per_message(capped by global max)- Domain
custom_limits.max_recipients_per_message(also capped)- Fallback to
min(MAX_DEFAULT_RECIPIENTS_PER_MESSAGE, MAX_RECIPIENTS_PER_MESSAGE)and safely ignores non‑positive or non‑int values. This aligns with the tests and prevents any configured limit from exceeding the global ceiling.
src/backend/core/api/viewsets/maildomain.py (2)
37-69: Maildomain update wiring and permissions align with new abilitiesAdding
UpdateModelMixinand routingupdate/partial_updatethroughMailDomainAdminUpdateSerializerwithCanManageSettingsensures that only superusers (as defined inMailDomain.get_abilities) can changecustom_limits, while list/retrieve continue to use the broaderIsSuperUser | IsMailDomainAdminguard. This matches the intent reflected in the tests.
167-203: Mailbox settings endpoint correctly layers domain admin + settings abilityThe new
/settingsaction onAdminMailDomainMailboxViewSet:
- Uses
MailboxSettingsUpdateSerializerdedicated tocustom_limits.- Restricts access via existing domain‑admin permissions plus
CanManageSettings, which consultsMailbox.get_abilities.- Annotates
user_roleandis_domain_admininget_querysetto avoid N+1 lookups inget_abilities.This design cleanly separates mailbox “settings” from general mailbox updates and should scale well without obvious correctness or performance issues.
Also applies to: 375-406
src/backend/core/api/serializers.py (3)
220-231: Mailbox serializer now exposes effective max recipients cleanlyAdding
max_recipients_per_messageas aSerializerMethodFieldbacked byMailbox.get_max_recipients_per_message()is a good way to surface the resolved limit (mailbox/domain/global) to clients without duplicating logic in the API layer. The schema annotation viaextend_schema_fieldis also appropriate.Also applies to: 292-296
885-895: Custom‑limits serializers and mixin are consistent with model behavior
MailDomainAdminSerializernow includes read‑onlycustom_limits, which matches its use for admin listing/detail.CustomLimitsUpdateMixin.validate_custom_limitsenforces thatmax_recipients_per_messagenever exceedssettings.MAX_RECIPIENTS_PER_MESSAGE, aligning with the model helper’s global cap and the new tests.MailDomainAdminUpdateSerializerandMailboxSettingsUpdateSerializercleanly scope updates tocustom_limitsfor domains and mailboxes respectively, leaving other fields to their existing serializers.Combined, this gives you a clear, settings‑aware update path for limits without leaking unrelated fields into these endpoints.
Also applies to: 960-975, 977-1007, 1009-1017
1051-1064: Mailbox admin serializers correctly expose and separatecustom_limitsand creation response
MailboxAdminSerializernow includescustom_limitsin its fields, so admin clients can see the effective per‑mailbox limits along with other metadata.MailboxAdminCreateSerializerexplicitly omitscustom_limitsfrom the creation response and documents that it must be managed via the dedicated/settings/endpoint, while still adding theone_time_passwordfield for schema purposes.This keeps the creation flow focused and shifts limit management to the specialized settings API, which is aligned with the new permissions and tests.
Also applies to: 1216-1223, 1234-1242
| def test_models_maildomain_get_abilities_superuser(self, user, maildomain): | ||
| """Test MailDomain.get_abilities when user is superuser.""" | ||
| user.is_superuser = True | ||
| user.save() | ||
|
|
||
| abilities = maildomain.get_abilities(user) | ||
|
|
||
| assert abilities["get"] is False | ||
| assert abilities["patch"] is True | ||
| assert abilities["put"] is True | ||
| assert abilities["post"] is True | ||
| assert abilities["delete"] is True | ||
| assert abilities["manage_accesses"] is True | ||
| assert abilities["manage_mailboxes"] is True | ||
| assert abilities["manage_settings"] is True |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n -B 5 -A 15 'CRUDAbilities.CAN_READ' src/backend/core/models.pyRepository: suitenumerique/messages
Length of output: 2118
🏁 Script executed:
rg -n -B 3 -A 10 'is_superuser.*True' src/backend/core/tests/models/test_maildomain.py | head -60Repository: suitenumerique/messages
Length of output: 636
CAN_READ ability should check superuser status for consistency.
Line 170's abilities["get"] is False for a superuser is unintentional. The get_abilities implementation sets CAN_READ = bool(role), which ignores superuser status, while all write operations and CAN_MANAGE_SETTINGS check user.is_superuser. This creates an inconsistency where a superuser can modify/delete a domain but cannot read it. Update CAN_READ to also check superuser status: CAN_READ: bool(role) or user.is_superuser.
🤖 Prompt for AI Agents
In src/backend/core/tests/models/test_maildomain.py around lines 163 to 177, the
test asserts that a superuser has no read ability which is inconsistent with
implementation: update the expectation so CAN_READ accounts for superuser
status; change the assertion to expect that the read ability is True for a
superuser by adjusting the test to reflect CAN_READ = bool(role) or
user.is_superuser.
| console.log(limitMatch); | ||
| console.log(limit); | ||
| expect(limit).toBe(100); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove debug console.log statements.
These debug statements should be removed before merging.
// Check that the help text shows the new limit
const helpText = await page.locator('text=/maximum.*recipients/i').textContent();
const limitMatch = helpText?.match(/\d+/);
const limit = limitMatch ? parseInt(limitMatch[0]) : null;
- console.log(limitMatch);
- console.log(limit);
expect(limit).toBe(100);📝 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.
| console.log(limitMatch); | |
| console.log(limit); | |
| expect(limit).toBe(100); | |
| // Check that the help text shows the new limit | |
| const helpText = await page.locator('text=/maximum.*recipients/i').textContent(); | |
| const limitMatch = helpText?.match(/\d+/); | |
| const limit = limitMatch ? parseInt(limitMatch[0]) : null; | |
| expect(limit).toBe(100); |
🤖 Prompt for AI Agents
In src/e2e/src/__tests__/message-max-recipients-admin.spec.ts around lines 46 to
48, remove the two debug console.log statements (console.log(limitMatch);
console.log(limit);) so the test contains only assertions; ensure no other debug
prints remain and run the test to confirm behavior unchanged.
| }, | ||
| { | ||
| label: t('Settings'), | ||
| icon: <Icon name="tune" size={IconSize.SMALL} />, | ||
| callback: onManageSettings, | ||
| showSeparator: !onResetPassword, | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how CAN_MANAGE_SETTINGS is used in the codebase
rg -n "CAN_MANAGE_SETTINGS" --type=ts --type=tsx -C3Repository: suitenumerique/messages
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Also search for domain settings ability checks
rg -n "CAN_MANAGE_MAILDOMAIN_SETTINGS" --type=ts --type=tsx -C3Repository: suitenumerique/messages
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Find the domain page ActionsCell component mentioned in the review
fd "ActionsCell" --type=fRepository: suitenumerique/messages
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Examine the mailbox-data-grid file to see current permission checks
cat -n src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx | sed -n '250,280p'Repository: suitenumerique/messages
Length of output: 1585
🏁 Script executed:
#!/bin/bash
# Search for CAN_MANAGE_SETTINGS without type restriction
rg "CAN_MANAGE_SETTINGS" -C3Repository: suitenumerique/messages
Length of output: 6332
🏁 Script executed:
#!/bin/bash
# Search for CAN_MANAGE_MAILDOMAIN_SETTINGS
rg "CAN_MANAGE_MAILDOMAIN_SETTINGS" -C3Repository: suitenumerique/messages
Length of output: 2882
🏁 Script executed:
#!/bin/bash
# Find domain-related ActionsCell or actions components
rg "ActionsCell" -C2Repository: suitenumerique/messages
Length of output: 924
🏁 Script executed:
#!/bin/bash
# Look at the mailbox-data-grid component definition and props
head -50 src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsxRepository: suitenumerique/messages
Length of output: 2477
🏁 Script executed:
#!/bin/bash
# Search for onManageSettings callback to understand where it's passed from
rg "onManageSettings" -C2Repository: suitenumerique/messages
Length of output: 3656
🏁 Script executed:
#!/bin/bash
# Get the full ActionsRow component to see what props are available
rg -n "const ActionsRow" -A 50 src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx | head -80Repository: suitenumerique/messages
Length of output: 2395
🏁 Script executed:
#!/bin/bash
# Check how ActionsRow is called - what props are passed to it
rg -n "ActionsRow" src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsxRepository: suitenumerique/messages
Length of output: 292
🏁 Script executed:
#!/bin/bash
# Get the ActionsRowProps type definition and the place where ActionsRow is called
cat -n src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx | sed -n '232,239p'Repository: suitenumerique/messages
Length of output: 303
🏁 Script executed:
#!/bin/bash
# Get the section where ActionsRow is rendered (line 145 area)
cat -n src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx | sed -n '140,160p'Repository: suitenumerique/messages
Length of output: 1051
🏁 Script executed:
#!/bin/bash
# Check the full component context - what abilities are checked at the component level
cat -n src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx | sed -n '25,80p'Repository: suitenumerique/messages
Length of output: 2888
Add ability check for Settings action to match domain page pattern.
The Settings option should check CAN_MANAGE_SETTINGS ability before being shown, consistent with how the domain page checks CAN_MANAGE_MAILDOMAIN_SETTINGS. Currently, the Settings action is always displayed when canManageMailboxes is true, regardless of the user's settings management permission. Use the existing pattern of conditionally passing callbacks (like onResetPassword) to gate the Settings option.
🤖 Prompt for AI Agents
In
src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx
around lines 262 to 268, the "Settings" action is shown whenever
canManageMailboxes is true but does not check the CAN_MANAGE_SETTINGS ability;
update it to follow the existing pattern used for other actions (e.g.,
onResetPassword) by conditionally passing the callback only when the user has
CAN_MANAGE_SETTINGS. Concretely, wrap or replace onManageSettings with a
conditional (like canManageSettings ? onManageSettings : undefined) or gate the
action using the same ability variable used elsewhere so the Settings menu item
only renders when the user has CAN_MANAGE_SETTINGS, and keep showSeparator logic
unchanged.
Define global env var to configure max recipients allowed per message Implement frontend restrictions Add custom_limits on mailbox and maildomain Add playwright tests
sdemagny
force-pushed
the
sdem/recipients_limit_per_msg
branch
from
a508e39 to
2169aab
Compare
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)
304-316: Validatecustom_limitsstructure and values.The docstring mentions validating "custom attributes and limits," but only
custom_attributesis validated. This is inconsistent withMailbox.clean()(lines 535-552), which validatescustom_limitsstructure. Without validation, invalid data could be saved incustom_limits, leading to runtime errors inget_max_recipients_per_message().Apply this diff to add
custom_limitsvalidation:def clean(self): - """Validate custom attributes and limits.""" - # Custom attributes schema + """Validate custom attributes and limits.""" + # Validate custom_attributes schema try: jsonschema.validate( self.custom_attributes, settings.SCHEMA_CUSTOM_ATTRIBUTES_MAILDOMAIN ) except jsonschema.ValidationError as exception: raise ValidationError( {"custom_attributes": exception.message} ) from exception + # Validate custom_limits structure + if self.custom_limits: + if not isinstance(self.custom_limits, dict): + raise ValidationError( + {"custom_limits": "custom_limits must be a dictionary"} + ) + if "max_recipients_per_message" in self.custom_limits: + value = self.custom_limits["max_recipients_per_message"] + if value is not None and (not isinstance(value, int) or value < 1): + raise ValidationError( + { + "custom_limits": "max_recipients_per_message must be a positive integer" + } + ) + super().clean()
♻️ Duplicate comments (2)
src/backend/core/tests/models/test_maildomain.py (1)
163-177: CAN_READ inconsistency for superuser should be addressed.Line 170 asserts
abilities["get"] is Falsefor a superuser, but a superuser haspatch,put,post,delete,manage_accesses,manage_mailboxes, andmanage_settingsallTrue. This creates an inconsistency where a superuser can modify or delete a domain but cannot read it.Based on the
get_abilitiesimplementation insrc/backend/core/models.py(lines 350-388),CAN_READis set tobool(role)without checking superuser status. Consider updating the model to include superuser in the read check:CAN_READ: bool(role) or user.is_superuser,This would align read permissions with write permissions for superusers.
src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx (1)
262-268: Add ability check for Settings action.The Settings option should check
CAN_MANAGE_SETTINGSability before being shown, consistent with how the domain page checksCAN_MANAGE_MAILDOMAIN_SETTINGS(src/frontend/src/pages/domain/index.tsx lines 202, 215-224). Currently, the Settings action is always displayed whencanManageMailboxesis true, regardless of the user's settings management permission.To fix this, you should:
- Check the mailbox's
CAN_MANAGE_SETTINGSability in the parent component (where abilities are available)- Conditionally pass the
onManageSettingscallback (similar toonResetPasswordpattern on line 147)- Only render the Settings option in the dropdown when the callback is provided
For example, in the parent component around line 150:
onManageSettings={mailbox.abilities[Abilities.CAN_MANAGE_SETTINGS] ? () => handleManageSettings(row) : undefined}And in ActionsRow, make the props optional and filter out undefined callbacks in the options array.
🧹 Nitpick comments (11)
src/e2e/src/__tests__/message-max-recipients-admin.spec.ts (3)
65-65: Avoid using hardcodedwaitForTimeout.
waitForTimeout(50)is fragile and can cause flaky tests. Consider using a more reliable wait condition such as waiting for the input to be enabled or for a specific network request to complete.- await page.waitForTimeout(50); + // Wait for modal animation to complete by checking input is interactable
83-84: Same concern: avoid hardcodedwaitForTimeout.This pattern appears again. Consider extracting a helper function if you need to wait for modal transitions, or use Playwright's auto-waiting by interacting with elements directly.
89-100: Complete or remove the placeholder test.The test
"should domain admin can customize mailbox max recipients per message"is incomplete with a TODO comment. Incomplete tests can mask missing coverage in CI reports. Either:
- Complete the implementation
- Mark it as
test.skipwith a tracking issue- Remove it until ready
Would you like me to help implement this test or create an issue to track it?
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx (1)
116-127: Consider addingaria-describedbyfor accessibility.The Input component has helper text, but ensure it's properly associated with the input for screen readers. If the
Inputcomponent from@openfun/cunningham-reactdoesn't handle this automatically, consider adding explicitaria-describedby.src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (3)
24-29: Consider using a generated hook instead of manual state and fetchAPI.The mailbox settings modal (
modal-mailbox-manage-settings) usesuseMaildomainsMailboxesSettingsUpdatehook which manages pending state automatically. This component uses manualisSubmittingstate and directfetchAPIcalls.For consistency and to leverage automatic cache invalidation, consider using or generating a hook for domain settings updates similar to the mailbox pattern.
80-86: Fragile query invalidation using string matching.The predicate-based invalidation using string matching on query keys (
key.includes('/api/v1.0/maildomains')) is fragile and could break if query key formats change. Consider using the query key factories from the generated API code for more robust invalidation.// Instead of string matching, use the generated query key patterns import { getMaildomainsListQueryKey } from '@/features/api/gen'; await queryClient.invalidateQueries({ queryKey: getMaildomainsListQueryKey(), // or similar });
48-64: Consider extracting shared validation logic.The validation logic for
max_recipients_per_message(positive integer or empty, not exceeding global max) is duplicated between this component andmodal-mailbox-manage-settings. Consider extracting this into a shared utility function.// utils/validation.ts export const validateMaxRecipients = ( value: string, maxGlobal: number, t: TFunction ): string | null => { const trimmed = value.trim(); if (trimmed === '') return null; const num = Number(trimmed); if (isNaN(num) || num < 1 || !Number.isInteger(num)) { return t('Please enter a valid positive integer or leave empty.'); } if (num > maxGlobal) { return t('The limit cannot exceed the global maximum of {{max}} recipients.', { max: maxGlobal }); } return null; };src/backend/core/models.py (1)
530-533: Consider removing redundantsave()method.
BaseModel.save()(lines 92-95) already callsfull_clean()before saving, so this override is redundant. SinceMailboxextendsBaseModel, the parent implementation already provides this behavior.Apply this diff to remove the redundant method:
- def save(self, *args, **kwargs): - """Enforce validation before saving.""" - self.full_clean() - super().save(*args, **kwargs) - def clean(self):src/backend/core/api/openapi.json (2)
3455-3460: Optional: mark nested mailboxidparameters as UUIDs for better typingThese
idpath parameters currently expose only"type": "string", whereas other mailbox/domain identifiers in the spec typically also declare"format": "uuid". Consider adding theformatfor consistency and stronger generated client types.Also applies to: 3497-3503, 3553-3560, 3592-3598
5787-5813: Consider typingcustom_limitsconsistently in update request/response schemasIn
MailDomainAdminUpdate*,MailboxAdminUpdateMetadataRequest,MailboxSettingsUpdate, and their patched request variants,custom_limitsis currently untyped (notype/properties), while the read models (MailDomainAdmin.custom_limits,MailboxAdmin.custom_limits) define a concrete object withmax_recipients_per_message. For stronger client codegen and validation, consider reusing a sharedCustomLimitsschema (or mirroring the object definition) here as well.Also applies to: 6356-6367, 6403-6420, 7122-7130, 7153-7161
src/frontend/src/pages/domain/index.tsx (1)
2-2: Remove unused import.The
useModalimport is not used anywhere in the file. Please remove it to keep the code clean.Apply this diff:
-import { Button, DataGrid, usePagination, useModal } from "@openfun/cunningham-react"; +import { Button, DataGrid, usePagination } from "@openfun/cunningham-react";
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (19)
src/frontend/src/features/api/gen/maildomains/maildomains.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/config_retrieve200.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_abilities.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_custom_limits.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_update.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_update_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_write.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mail_domain_admin_write_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_abilities.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin_create.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin_custom_limits.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin_update_metadata_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_settings_update.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_mail_domain_admin_update_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_mailbox_settings_update_request.tsis excluded by!**/gen/**
📒 Files selected for processing (31)
env.d/development/backend.e2e(1 hunks)src/backend/core/api/openapi.json(22 hunks)src/backend/core/api/permissions.py(1 hunks)src/backend/core/api/serializers.py(12 hunks)src/backend/core/api/viewsets/config.py(4 hunks)src/backend/core/api/viewsets/mailbox.py(3 hunks)src/backend/core/api/viewsets/maildomain.py(6 hunks)src/backend/core/enums.py(2 hunks)src/backend/core/mda/draft.py(3 hunks)src/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.py(1 hunks)src/backend/core/models.py(9 hunks)src/backend/core/tests/api/test_admin_maildomains_list.py(3 hunks)src/backend/core/tests/api/test_admin_maildomains_update.py(1 hunks)src/backend/core/tests/api/test_config.py(2 hunks)src/backend/core/tests/api/test_mailboxes.py(4 hunks)src/backend/core/tests/api/test_messages_create.py(2 hunks)src/backend/core/tests/models/test_mailbox.py(6 hunks)src/backend/core/tests/models/test_maildomain.py(2 hunks)src/backend/messages/settings.py(1 hunks)src/e2e/src/__tests__/message-max-recipients-admin.spec.ts(1 hunks)src/e2e/src/__tests__/message-max-recipients.spec.ts(1 hunks)src/frontend/src/features/forms/components/message-form/index.tsx(10 hunks)src/frontend/src/features/forms/components/react-hook-form/rhf-contact-combobox.tsx(2 hunks)src/frontend/src/features/forms/components/react-hook-form/rhf-json-schema-field.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/domains-view/settings-domain-action.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx(7 hunks)src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx(1 hunks)src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx(1 hunks)src/frontend/src/features/providers/config.tsx(1 hunks)src/frontend/src/hooks/use-ability.ts(3 hunks)src/frontend/src/pages/domain/index.tsx(6 hunks)
✅ Files skipped from review due to trivial changes (1)
- env.d/development/backend.e2e
🚧 Files skipped from review as they are similar to previous changes (15)
- src/backend/core/api/permissions.py
- src/frontend/src/features/layouts/components/admin/domains-view/settings-domain-action.tsx
- src/backend/core/tests/api/test_admin_maildomains_update.py
- src/backend/core/tests/api/test_config.py
- src/frontend/src/features/forms/components/react-hook-form/rhf-json-schema-field.tsx
- src/backend/core/tests/models/test_mailbox.py
- src/e2e/src/tests/message-max-recipients.spec.ts
- src/frontend/src/features/forms/components/react-hook-form/rhf-contact-combobox.tsx
- src/backend/core/tests/api/test_mailboxes.py
- src/backend/core/enums.py
- src/backend/messages/settings.py
- src/frontend/src/features/providers/config.tsx
- src/backend/core/tests/api/test_admin_maildomains_list.py
- src/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.py
- src/backend/core/api/viewsets/config.py
🧰 Additional context used
📓 Path-based instructions (5)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Follow Django/PEP 8 style with a 100-character line limit
Use descriptive, snake_case names for variables and functions
Use Django ORM for database access; avoid raw SQL unless necessary for performance
Use Django’s built-in user model and authentication framework
Prefer try-except blocks to handle exceptions in business logic and views
Log expected and unexpected actions with appropriate log levels
Capture and report exceptions to Sentry; use capture_exception() for custom errors
Do not log sensitive information (tokens, passwords, financial/health data, PII)
Files:
src/backend/core/mda/draft.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_messages_create.pysrc/backend/core/api/viewsets/maildomain.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/mailbox.pysrc/backend/core/tests/models/test_maildomain.py
src/backend/**/{models.py,forms.py,views.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Keep business logic in models and forms; keep views thin and focused on request handling
Files:
src/backend/core/models.py
src/backend/**/{models.py,migrations/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Implement database indexing and query optimization (Model Meta indexes, constraints)
Files:
src/backend/core/models.py
src/backend/**/{tests.py,tests/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/{tests.py,tests/**/*.py}: Use Django’s testing tools (pytest-django) to ensure code quality and reliability
Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases
Files:
src/backend/core/tests/api/test_messages_create.pysrc/backend/core/tests/models/test_maildomain.py
src/backend/{templates/**/*.html,**/serializers.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use Django templates for HTML and DRF serializers for JSON responses
Files:
src/backend/core/api/serializers.py
🧠 Learnings (4)
📚 Learning: 2025-09-02T12:20:22.006Z
Learnt from: Nastaliss
Repo: suitenumerique/messages PR: 313
File: src/frontend/src/features/layouts/components/admin/modal-create-domain/index.tsx:32-37
Timestamp: 2025-09-02T12:20:22.006Z
Learning: In the messages application, `SCHEMA_CUSTOM_ATTRIBUTES_MAILDOMAIN` from the config defaults to an empty object `{}` rather than being potentially undefined, so null checks are not needed when using this configuration value.
Applied to files:
src/frontend/src/features/forms/components/message-form/index.tsxsrc/backend/core/api/openapi.json
📚 Learning: 2025-09-02T10:12:12.835Z
Learnt from: CR
Repo: suitenumerique/messages PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-09-02T10:12:12.835Z
Learning: Applies to src/backend/**/views.py : Optimize related object fetching with select_related and prefetch_related
Applied to files:
src/backend/core/models.py
📚 Learning: 2025-09-02T12:42:29.083Z
Learnt from: Nastaliss
Repo: suitenumerique/messages PR: 313
File: src/frontend/src/pages/domain/index.tsx:0-0
Timestamp: 2025-09-02T12:42:29.083Z
Learning: The domain admin pages in src/frontend/src/pages/domain/ use AdminLayout, not the main layout. Toast notifications for admin pages should be handled via a Toaster instance in the AdminLayout component.
Applied to files:
src/frontend/src/pages/domain/index.tsx
📚 Learning: 2025-09-02T10:12:12.835Z
Learnt from: CR
Repo: suitenumerique/messages PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-09-02T10:12:12.835Z
Learning: Applies to src/backend/**/views.py : Use Django REST Framework viewsets for API endpoints
Applied to files:
src/backend/core/api/viewsets/maildomain.pysrc/backend/core/api/viewsets/mailbox.py
🧬 Code graph analysis (11)
src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx (2)
src/frontend/src/features/api/gen/models/mailbox_admin.ts (1)
MailboxAdmin(15-43)src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx (1)
ModalMailboxManageSettings(24-143)
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx (5)
src/frontend/src/features/api/gen/models/mailbox_admin.ts (1)
MailboxAdmin(15-43)src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/api/gen/maildomains/maildomains.ts (1)
useMaildomainsMailboxesSettingsUpdate(1869-1901)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(17-45)
src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (5)
src/frontend/src/features/api/gen/models/mail_domain_admin.ts (1)
MailDomainAdmin(14-30)src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/api/fetch-api.ts (1)
fetchAPI(12-40)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(17-45)
src/frontend/src/features/forms/components/message-form/index.tsx (2)
src/frontend/src/features/providers/config.tsx (1)
useConfig(69-75)src/frontend/src/features/providers/mailbox.tsx (1)
useMailboxContext(355-361)
src/backend/core/models.py (4)
src/backend/core/api/fields.py (1)
JSONField(8-25)src/backend/core/enums.py (3)
MailDomainAbilities(96-101)MailDomainAccessRoleChoices(48-51)MailboxAbilities(104-116)src/backend/core/api/serializers.py (1)
get_max_recipients_per_message(293-295)src/backend/core/api/viewsets/placeholder.py (1)
get(45-66)
src/backend/core/tests/api/test_messages_create.py (2)
src/backend/core/factories.py (3)
MailboxAccessFactory(99-109)MailboxFactory(60-96)MailDomainFactory(51-57)src/backend/core/enums.py (1)
MailboxRoleChoices(15-21)
src/e2e/src/__tests__/message-max-recipients-admin.spec.ts (1)
src/e2e/src/utils-test.ts (1)
signInKeycloakIfNeeded(6-39)
src/frontend/src/pages/domain/index.tsx (4)
src/frontend/src/hooks/use-ability.ts (1)
Abilities(34-39)src/frontend/src/features/api/gen/maildomains/maildomains.ts (1)
getMaildomainsListQueryOptions(102-131)src/frontend/src/features/api/gen/models/mail_domain_admin.ts (1)
MailDomainAdmin(14-30)src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (1)
ModalUpdateDomain(24-153)
src/backend/core/api/serializers.py (1)
src/backend/core/models.py (18)
Meta(89-90)Meta(199-202)Meta(291-294)Meta(465-477)Meta(520-525)Meta(790-794)Meta(833-836)Meta(1016-1021)Meta(1188-1192)Meta(1209-1213)Meta(1250-1254)Meta(1325-1329)Meta(1556-1568)Meta(1624-1628)Meta(1664-1668)Meta(1718-1722)Mailbox(483-768)MailDomain(243-426)
src/backend/core/api/viewsets/mailbox.py (4)
src/backend/core/models.py (1)
MailDomainAccess(1649-1671)src/backend/core/tests/models/test_mailbox.py (1)
user(17-19)src/backend/core/tests/models/test_maildomain.py (2)
user(16-18)maildomain(22-24)src/backend/core/enums.py (1)
MailDomainAccessRoleChoices(48-51)
src/backend/core/tests/models/test_maildomain.py (2)
src/backend/core/models.py (3)
get_abilities(225-240)get_abilities(351-389)get_abilities(619-681)src/backend/core/api/serializers.py (4)
get_abilities(132-141)get_abilities(165-167)get_abilities(288-290)get_abilities(932-934)
🔇 Additional comments (47)
src/backend/core/tests/api/test_messages_create.py (6)
810-836: LGTM! Good test coverage for basic recipient limit enforcement.The test correctly verifies that a draft with recipients exceeding
MAX_RECIPIENTS_PER_MESSAGEis rejected with a 400 response.
838-883: LGTM! Proper testing of mailbox-level override.The test correctly verifies that
custom_limits.max_recipients_per_messageon a mailbox takes precedence over the global setting. Both the rejection (3 recipients > limit of 2) and success (2 recipients = limit) cases are covered.
885-934: LGTM! Proper testing of domain-level override.The test correctly verifies that
custom_limits.max_recipients_per_messageon a mail domain takes precedence over the global setting when no mailbox-level override exists.
936-986: LGTM! Proper testing of precedence hierarchy.The test correctly verifies that mailbox
custom_limitstakes priority over domaincustom_limits, ensuring the full precedence chain (mailbox > domain > global) works as expected.
988-1031: LGTM! Draft update limit enforcement is properly tested.The test verifies that recipient limits are enforced not only on draft creation but also on draft updates, preventing circumvention of limits via incremental updates.
1033-1078: LGTM! Test naming and comments are now accurate.The test correctly verifies that
MAX_DEFAULT_RECIPIENTS_PER_MESSAGEis enforced when no custom limits exist at the mailbox or domain level.src/backend/core/tests/models/test_maildomain.py (2)
141-141: LGTM! Correctly asserts no manage_settings for users without access.The assertion properly verifies that users without any role cannot manage settings.
160-161: LGTM! Correctly restricts manage_settings to superusers only.The comment and assertion properly document that even domain admins cannot manage settings—only superusers can.
src/backend/core/mda/draft.py (3)
101-112: LGTM! Clean recipient limit enforcement in draft creation.The implementation:
- Normalizes
Nonevalues to empty lists early- Calculates total recipients across all fields (to + cc + bcc)
- Raises a clear validation error when the limit is exceeded
The error message is descriptive and includes both the actual count and the maximum allowed.
184-186: LGTM! Simplified recipient storage.Direct assignment of recipient lists without fallback logic is cleaner since normalization happens earlier at lines 102-104.
252-274: LGTM! Correct handling of partial updates for recipient limits.The implementation properly:
- Counts current recipients per type from the database
- Computes the total after update by using new values when provided or existing counts when not
- Validates against the same
max_recipientslimit used in creationThis prevents circumventing limits by incrementally adding recipients across multiple update calls.
src/frontend/src/features/forms/components/message-form/index.tsx (5)
57-101: LGTM! Well-designed dynamic schema with recipient limit validation.The schema factory pattern is appropriate here since the max recipients limit can vary per mailbox. The
superRefineimplementation:
- Only validates when
maxRecipientsis defined- Correctly sums all recipient fields
- Adds errors only to fields that have values (avoiding confusing errors on hidden cc/bcc fields)
228-240: LGTM! Proper state management for dynamic validation.The implementation correctly:
- Initializes the limit from the selected mailbox or falls back to config defaults
- Memoizes the schema to regenerate only when the limit changes
- Memoizes the resolver to update when the schema changes (addressing the previous review feedback)
257-279: LGTM! Reactive limit updates with proper revalidation.The two effects work together correctly:
- First effect updates
maxRecipientsLimitwhen sender mailbox changes- Second effect triggers revalidation only when there are recipients to validate
The conditional revalidation prevents unnecessary validation runs on initial render.
268-268: Potential stale closure issue withmaxRecipientsLimitin dependency array.The effect at lines 257-268 includes
maxRecipientsLimitin its dependency array, which means it re-runs when the limit changes. However, since this effect is responsible for settingmaxRecipientsLimit, including it as a dependency is technically correct but could be confusing. The current implementation works because of thenewLimit !== maxRecipientsLimitguard.
479-479:keepErrors: truemay preserve stale validation errors.When
form.resetis called withkeepErrors: true, validation errors from before the reset will persist even if the values haven't changed. This could lead to stale error messages being displayed after a successful auto-save if validation was triggered between saves.Consider whether this is the intended behavior. If errors should be cleared on successful save, remove
keepErrors: true.src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx (2)
1-11: LGTM! Clean imports and component setup.The component properly imports all necessary dependencies and uses the generated API hook for the settings update.
48-93: LGTM! Proper validation and error handling.The submit handler:
- Validates input as positive integer or empty
- Validates against global maximum
- Uses the dedicated
/settings/endpoint via the generated hook- Handles success with toast notification and modal close
- Handles errors by displaying them in a banner
Since
isPendingis managed by theuseMaildomainsMailboxesSettingsUpdatehook (line 31), there's no manual state to reset on error—the hook handles this automatically.src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (1)
102-104: LGTM! Proper state reset in finally block.The
finallyblock ensuresisSubmittingis reset regardless of success or failure, preventing the Save button from being permanently disabled after an error.src/backend/core/models.py (7)
282-289: LGTM!The
custom_limitsfield definition follows the same pattern ascustom_attributesand includes appropriate help text.
387-388: LGTM!The restriction to superusers for managing mail domain settings is clearly documented and appropriately implemented.
511-518: LGTM!The
custom_limitsfield definition is consistent with the pattern established inMailDomain.
535-552: LGTM!The
custom_limitsvalidation is thorough and handles both structure and value constraints correctly.
623-656: LGTM!The role initialization to
0provides safer default behavior for integer comparisons. Theis_domain_admincheck follows the same annotation-first pattern asuser_role, which addresses the N+1 query concern when the viewset properly annotates the queryset.
671-671: LGTM!The
CAN_MANAGE_SETTINGSability correctly usesis_domain_admin, consistent with the access control model.
739-768: LGTM!The priority cascade (mailbox → domain → global) is correctly implemented, with appropriate type checking and enforcement of the global maximum at each level.
src/backend/core/api/openapi.json (8)
221-235: Config schema: recipient limit constants are well introducedThe two new integer properties (
MAX_RECIPIENTS_PER_MESSAGE,MAX_DEFAULT_RECIPIENTS_PER_MESSAGE) with clear descriptions and inclusion inrequiredlook consistent with the existing size limits block and appropriate for frontend consumption.Also applies to: 237-251
3042-3142: Maildomain PUT/PATCH endpoints for custom_limits look coherentAdding full and partial update operations on
/maildomains/{maildomain_pk}/using the dedicatedMailDomainAdminUpdate*schemas is consistent with the admin surface and keeps custom_limits updates clearly scoped.
3662-3725: New/mailboxes/{id}/settings/endpoint is wired cleanly to settings schemasThe mailbox settings PATCH endpoint under a maildomain, using
PatchedMailboxSettingsUpdateRequestand returningMailboxSettingsUpdate, matches the intended “custom_limits-only” semantics and reuses existing auth/tag patterns; the documented 400/403 responses are sensible.
5714-5785: MailDomainAdmin: custom_limits shape and manage_settings ability are consistentExposing
custom_limitsas a nullable object with a typedmax_recipients_per_messagefield, plus adding amanage_settingsability (and marking both as required) aligns well with the new settings feature and gives clients a clear, strongly-typed read model.
5815-5817: Maildomain create admin descriptions are clearer nowThe updated descriptions on
MailDomainAdminWriteandMailDomainAdminWriteRequestbetter communicate their “create admin view” role without altering the schema surface; no issues spotted.Also applies to: 5863-5865
5918-5921: Mailbox schema: max_recipients_per_message and manage_settings ability look correct
max_recipients_per_messageis now an integer, readOnly field and included inrequired, and themanage_settingsability is added and required alongside existing abilities; this matches the numeric semantics of the limits and the new settings permission model.Also applies to: 5970-5988, 5993-5999
6206-6226: MailboxAdmin.read: custom_limits exposure is aligned with domain-level shapeThe new nullable
custom_limitsobject withmax_recipients_per_messageinMailboxAdmin, plus adding it torequired, is consistent withMailDomainAdminand gives admin UIs a predictable place to read mailbox-level limits.
6234-6236: MailboxAdminCreate doc correctly defers custom_limits to /settings/The note clarifying that
custom_limitscannot be set at creation time and must be managed via the/settings/endpoint matches the new API design and should help avoid misuse by clients.src/backend/core/api/viewsets/mailbox.py (1)
3-3: LGTM! Efficient domain admin status annotation.The
is_domain_adminannotation correctly usesExiststo check domain admin status in a single query, avoiding N+1 query issues. This aligns well with the new settings management permissions introduced across the PR.Also applies to: 27-27, 36-44
src/frontend/src/hooks/use-ability.ts (1)
15-15: LGTM! Abilities correctly added for settings management.The new
CAN_MANAGE_SETTINGSandCAN_MANAGE_MAILDOMAIN_SETTINGSabilities are properly integrated into the ability checking system, following the existing patterns consistently.Also applies to: 27-27, 84-86
src/frontend/src/pages/domain/index.tsx (2)
104-123: LGTM! Fresh domain lookup ensures modal has latest data.The pattern of fetching
freshDomainfrom the current domains list (line 106) ensures the modal always receives the most up-to-date domain data. The key usingupdated_at(line 109) correctly forces remounting when the domain is updated.
199-227: LGTM! ActionsCell properly gates actions by abilities.The component correctly checks
CAN_MANAGE_MAILDOMAIN_ACCESSESandCAN_MANAGE_MAILDOMAIN_SETTINGSabilities before rendering their respective buttons. The early return when neither ability is present (line 204) is good practice.src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx (1)
60-63: LGTM! Modal integration follows established patterns.The settings modal integration correctly follows the same pattern as other modal actions in this component, with proper key management for remounting and appropriate refetch behavior.
Also applies to: 218-225
src/backend/core/api/viewsets/maildomain.py (3)
7-7: LGTM! Update support correctly added with proper permissions.The addition of
UpdateModelMixinand the use ofCanManageSettingspermission for update actions properly enables domain settings updates with appropriate authorization checks.Also applies to: 41-41, 59-68
167-203: LGTM! Queryset annotations and permissions properly configured.The annotations for
user_roleandis_domain_adminfollow the same efficient pattern used in src/backend/core/api/viewsets/mailbox.py, and the permission/serializer selection forupdate_settingsis correctly implemented.
375-405: LGTM! Settings update action properly implemented.The
update_settingsaction is well-structured with clear OpenAPI documentation, proper permission enforcement throughget_permissions, and a clean implementation that delegates validation to the serializer.src/backend/core/api/serializers.py (5)
7-7: LGTM! max_recipients_per_message field correctly exposed.The read-only field properly delegates to the model's
get_max_recipients_per_message()method, following Django REST Framework best practices for computed fields.Also applies to: 220-220, 224-231, 292-295
998-1017: LGTM! Validation correctly enforces limits.The
CustomLimitsUpdateMixinproperly validatesmax_recipients_per_message:
- Allows
None(to use inherited limits)- Enforces positive values
- Enforces global maximum from settings
- Provides clear, translatable error messages
1020-1043: LGTM! Update serializers are focused and well-structured.Both
MailDomainAdminUpdateSerializerandMailboxSettingsUpdateSerializercorrectly inherit the validation mixin and expose only the necessary fields for settings updates, following the single-responsibility principle.
874-904: LGTM! custom_limits properly exposed as read-only in admin serializers.Both
MailDomainAdminSerializerandMailboxAdminSerializercorrectly exposecustom_limitsas read-only fields with detailed OpenAPI schemas. The comment on line 1120 appropriately directs users to the/settings/endpoint for updates.Also applies to: 908-915, 1074-1094, 1109-1120
1267-1288: LGTM! Creation serializer correctly excludes custom_limits.The design decision to exclude
custom_limitsfrom the creation response and require the dedicated/settings/endpoint for configuration is well-documented and implemented cleanly.
| expect(finalValue).toBe(initialValue); | ||
| }); | ||
|
|
||
| test("should domain admin can customize mailbox max recipients per message ", async ({ page, browserName }) => { |
There was a problem hiding this comment.
Grammar issue in test name.
The test name has a grammatical error: "should domain admin can" → "should allow domain admin to".
- test("should domain admin can customize mailbox max recipients per message ", async ({ page, browserName }) => {
+ test("should allow domain admin to customize mailbox max recipients per message", async ({ page, browserName }) => {📝 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.
| test("should domain admin can customize mailbox max recipients per message ", async ({ page, browserName }) => { | |
| test("should allow domain admin to customize mailbox max recipients per message", async ({ page, browserName }) => { |
🤖 Prompt for AI Agents
In src/e2e/src/__tests__/message-max-recipients-admin.spec.ts around line 89,
the test title string contains a grammatical error: replace "should domain admin
can customize mailbox max recipients per message" with "should allow domain
admin to customize mailbox max recipients per message" by updating the test(...)
description accordingly to use correct grammar and infinitive form.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/backend/core/api/viewsets/maildomain.py (2)
173-180: Consider simplifying the permission composition.The current implementation correctly enforces that users must satisfy
(IsSuperUser | IsMailDomainAdmin) AND CanManageSettings, but the list comprehension with+could be more explicit.🔎 Consider this alternative for clarity:
def get_permissions(self): """Override permissions for specific actions.""" if self.action == "update_settings": - # Domain admin + ability to manage settings - return [permission() for permission in self.permission_classes] + [ - core_permissions.CanManageSettings() - ] + # Domain admin + ability to manage settings + return [ + core_permissions.IsSuperUser() | core_permissions.IsMailDomainAdmin(), + core_permissions.CanManageSettings(), + ] return super().get_permissions()
372-402: Consider adding logging for settings updates.The
update_settingsaction successfully implements the settings management endpoint, but it lacks logging. According to the coding guidelines, expected actions should be logged with appropriate levels.Additionally, for consistency with other mutation endpoints in this viewset (
createandpartial_update), consider adding@transaction.atomic.🔎 Consider these improvements:
+@transaction.atomic @action(detail=True, methods=["patch"], url_path="settings") def update_settings(self, request, maildomain_pk=None, pk=None): """ Update mailbox settings (custom_limits). Only domain administrators can manage mailbox settings. This endpoint is separate from the general mailbox update endpoint to enforce proper permission checks at the viewset level. """ mailbox = self.get_object() serializer = self.get_serializer(mailbox, data=request.data, partial=True) serializer.is_valid(raise_exception=True) serializer.save() + logger.info( + "Mailbox settings updated for mailbox %s by user %s", + mailbox.id, + request.user.id, + ) return Response(serializer.data, status=status.HTTP_200_OK)Based on coding guidelines, ...
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/backend/core/api/viewsets/maildomain.py(6 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Follow Django/PEP 8 style with a 100-character line limit
Use descriptive, snake_case names for variables and functions
Use Django ORM for database access; avoid raw SQL unless necessary for performance
Use Django’s built-in user model and authentication framework
Prefer try-except blocks to handle exceptions in business logic and views
Log expected and unexpected actions with appropriate log levels
Capture and report exceptions to Sentry; use capture_exception() for custom errors
Do not log sensitive information (tokens, passwords, financial/health data, PII)
Files:
src/backend/core/api/viewsets/maildomain.py
🧠 Learnings (1)
📚 Learning: 2025-09-02T10:12:12.835Z
Learnt from: CR
Repo: suitenumerique/messages PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-09-02T10:12:12.835Z
Learning: Applies to src/backend/**/views.py : Use Django REST Framework viewsets for API endpoints
Applied to files:
src/backend/core/api/viewsets/maildomain.py
🔇 Additional comments (5)
src/backend/core/api/viewsets/maildomain.py (5)
7-7: LGTM!The new imports are properly utilized in the queryset annotations below (lines 187-199).
41-41: LGTM!The addition of
UpdateModelMixincombined with appropriate permission and serializer handling for update actions follows Django REST Framework best practices and properly enables settings management on mail domains.Also applies to: 59-68
277-277: LGTM!Adding
custom_limitsto the OpenAPI schema properly documents the new field availability in the API contract.
167-171: LGTM!The serializer selection logic properly returns the specialized serializer for the
update_settingsaction.
184-200: No action needed — the annotations are properly utilized.The
user_roleandis_domain_adminannotations are actively used as claimed:
Mailbox.get_abilities()directly accessesself.user_role(line 640) andself.is_domain_admin(line 629) to compute user abilities- The serializers reference
user_roleviaget_user_role()method and in field lists- Both annotations have corresponding test coverage validating their functionality
- The code includes appropriate
try/exceptblocks checking for annotated attributes, showing intentional optimization for the N+1 query problemThe annotations are necessary optimizations that reduce database queries within
get_abilities()computation.Likely an incorrect or invalid review comment.
Define global env var to configure max recipients allowed per message
Implement frontend restrictions
Add custom_limits on mailbox and maildomain
Add playwright tests
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.