[WIP] max recipient quota #459
Conversation
Define global env var to configure max recipients allowed per message Implement frontend restrictions Add custom_limits on mailbox and maildomain Add playwright tests
WalkthroughThis PR implements a comprehensive recipient quota system for managing email sending limits at mailbox and domain scopes. It includes a Redis-backed quota service, API endpoints for quota status and custom limit configuration, database schema changes, comprehensive validation logic, management commands, and frontend UI components for display and administration of quotas with period-based tracking (daily/monthly). Changes
Sequence DiagramssequenceDiagram
actor User
participant Frontend
participant API
participant QuotaService as Quota Service<br/>(Redis)
participant DB as Database
User->>Frontend: Click "Send Message"
Frontend->>API: POST /send (recipients)
API->>API: check_and_update_recipient_quota()
alt Quota Check
API->>QuotaService: check_and_increment(mailbox_id,<br/>mailbox_limit, domain_id,<br/>domain_limit, count)
QuotaService->>QuotaService: Atomic Lua script:<br/>verify both quotas
alt Quotas OK
QuotaService->>QuotaService: Increment counts<br/>(with TTL refresh)
QuotaService-->>API: (True, "", remaining)
API->>DB: Create & Send Message
DB-->>API: Success
API-->>Frontend: 200 OK
Frontend->>User: Message sent
else Quota Exceeded
QuotaService-->>API: (False, "mailbox"|"domain",<br/>remaining)
API-->>Frontend: 403 Permission Error<br/>(with quota details)
Frontend->>User: Quota exceeded warning
end
end
sequenceDiagram
actor Admin
participant Frontend
participant API
participant QuotaService as Quota Service<br/>(Redis)
participant DB as Database
Admin->>Frontend: View Quota Display
Frontend->>Frontend: Render QuotaBadge
Frontend->>API: GET /mailboxes/{id}/quota/
API->>DB: Fetch mailbox + period config
API->>QuotaService: get_status(entity_type,<br/>entity_id, period, limit)
QuotaService->>QuotaService: Calculate period_start,<br/>fetch current_count
QuotaService-->>API: {recipient_count, quota_limit,<br/>remaining, usage_percentage}
API->>API: Format period_display
API-->>Frontend: RecipientQuota (JSON)
Frontend->>Frontend: Render badge with<br/>colors (green/yellow/red)
Frontend->>Admin: Display quota status
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly Related PRs
Suggested Reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| # not at configuration time. | ||
|
|
||
| except ValueError as e: | ||
| raise serializers.ValidationError(str(e)) from e |
Check warning
Code scanning / CodeQL
Information exposure through an exception Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 1 day ago
To fix this information exposure issue, the code should avoid returning the raw exception message from parse_max_recipients to the user. Instead, catch the ValueError and raise a serializers.ValidationError with a generic and user-friendly error message that does not leak stack trace or internal details. To preserve debugging information, the original exception can be logged on the server, using Python’s standard logging library. The change should be made in the validate_custom_limits method, replacing the current error path. This fix only requires editing the block surrounding the exception handling in src/backend/core/api/serializers.py. Add an import for logging in this file if not already present.
| @@ -3,7 +3,7 @@ | ||
| # pylint: disable=too-many-lines | ||
|
|
||
| import json | ||
|
|
||
| import logging | ||
| from django.conf import settings | ||
| from django.db import transaction | ||
| from django.db.models import Count, Exists, OuterRef, Q | ||
| @@ -1055,7 +1055,8 @@ | ||
| # not at configuration time. | ||
|
|
||
| except ValueError as e: | ||
| raise serializers.ValidationError(str(e)) from e | ||
| logging.exception("Invalid max_recipients format in custom_limits validation") | ||
| raise serializers.ValidationError(_("Invalid format for recipients limit.")) | ||
|
|
||
| return value | ||
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🧹 Nitpick comments (33)
src/frontend/src/features/forms/components/react-hook-form/rhf-contact-combobox.tsx (3)
8-8: Remove unused import.
useConfigis imported but never used in this file.🔎 Apply this diff to remove the unused import:
-import { useConfig } from "@/features/providers/config";
52-54: Remove unusedfieldStatedestructuring.
fieldStateis destructured from the render props but is never used since you're accessingformState.errorsdirectly. Consider removing it to avoid confusion.🔎 Apply this diff:
- render={({ field, fieldState }) => { + render={({ field }) => {
70-75: Consider making the related fields configurable or filtering out the current field.The hardcoded
['to', 'cc', 'bcc']couples this component to a specific form structure. Additionally, triggering validation on the current field (props.name) is redundant sinceshouldValidate: truealready handles it.🔎 Apply this diff to filter out the current field:
onChange={(value) => { // Update the value of the field with validation setValue(props.name, value, { shouldDirty: true, shouldValidate: true }); // Trigger validation for other recipient fields to update their error state - trigger(['to', 'cc', 'bcc']); + const recipientFields = ['to', 'cc', 'bcc'].filter(f => f !== props.name); + trigger(recipientFields); }}Alternatively, consider accepting a
relatedFieldsprop to make this component more reusable across different form structures.src/backend/messages/settings.py (1)
252-263: Consider documenting the relationship between MAX and DEFAULT settings.The naming convention distinguishes
MAX_RECIPIENTS_PER_MESSAGE(system-wide hard limit) fromMAX_DEFAULT_RECIPIENTS_PER_MESSAGE(default per entity). While the comments explain this, the similar names could cause confusion.Consider:
- Adding a comment block at the top explaining the two-tier limit system
- Ensuring the settings check that DEFAULT ≤ MAX
💡 Suggested documentation enhancement
+ # Outgoing email limits - Two-tier system: + # 1. MAX_* settings define system-wide hard limits (cannot be exceeded) + # 2. MAX_DEFAULT_* settings define default limits for entities without custom limits + # Note: MAX_DEFAULT_* values should be ≤ MAX_* values # Outgoing email limitssrc/e2e/src/__tests__/message-max-recipients.spec.ts (1)
17-32: Consider removing hard-coded limit assertion or using a shared constant.The test hard-codes an expectation of
limit === 50with a FIXME comment acknowledging it's set in another test file. This creates tight coupling between tests and makes them brittle.Consider one of these approaches:
- Remove the specific value assertion and only verify a limit is displayed
- Extract the limit to a shared test constant
- Dynamically fetch the limit from the API/config
🔎 Option 1 - Remove specific value assertion:
// Extract the limit number from help text const limitMatch = helpText?.match(/\d+/); expect(limitMatch).toBeTruthy(); const limit = parseInt(limitMatch![0]); - // FIXME: this is value set in message-max-recipients-admin.spec.ts - expect(limit).toBe(50); + // Verify limit is a reasonable positive number + expect(limit).toBeGreaterThan(0);src/backend/core/checks.py (1)
15-22: Consider addingsocket_timeoutfor robustness.The Redis client in the quota service (see
src/backend/core/services/quota_redis.pylines 24-43) includessocket_timeout=2to prevent hanging on unresponsive connections. Adding this here would ensure the startup check doesn't block indefinitely.🔎 Suggested change:
client = redis.Redis( host=settings.REDIS_QUOTAS_HOST, port=settings.REDIS_QUOTAS_PORT, db=settings.REDIS_QUOTAS_DB, password=settings.REDIS_QUOTAS_PASSWORD, socket_connect_timeout=2, + socket_timeout=2, decode_responses=True, )src/backend/core/api/viewsets/mailbox.py (1)
123-136: Consider logging or raising for unexpected period values.The fallback at line 134 silently defaults to
nowfor unknown period values. While the code handles "y" (year), theQuotaPeriodChoicesenum only defines "d" (day) and "m" (month). If an unexpected period is returned fromget_max_recipients(), the silent fallback could mask configuration issues.🔎 Suggested change:
+ import logging + logger = logging.getLogger(__name__) + # Calculate period_start based on period type now = timezone.now() if period == "d": period_start = now.replace(hour=0, minute=0, second=0, microsecond=0) elif period == "m": period_start = now.replace(day=1, hour=0, minute=0, second=0, microsecond=0) elif period == "y": period_start = now.replace( month=1, day=1, hour=0, minute=0, second=0, microsecond=0 ) else: + logger.warning("Unknown quota period '%s' for mailbox %s", period, mailbox.id) period_start = nowsrc/backend/core/api/viewsets/send.py (1)
28-91: Consider extracting duplicated period display logic.The
period_displaydictionary is defined identically in two places (lines 62-64 and 78-80). Extract it to a constant or helper to reduce duplication.🔎 Suggested refactor:
+PERIOD_DISPLAY_MAP = {"d": "day", "m": "month", "y": "year"} + + def check_and_update_recipient_quota( mailbox: models.Mailbox, recipient_count: int ) -> None: ... if not success: if failed_entity == "mailbox": - period_display = {"d": "day", "m": "month", "y": "year"}.get( - mailbox_period, mailbox_period - ) + period_display = PERIOD_DISPLAY_MAP.get(mailbox_period, mailbox_period) ... # Domain quota exceeded - period_display = {"d": "day", "m": "month", "y": "year"}.get( - domain_period, domain_period - ) + period_display = PERIOD_DISPLAY_MAP.get(domain_period, domain_period)src/e2e/src/__tests__/message-max-recipients-admin.spec.ts (1)
65-65: ReplacewaitForTimeoutwith explicit waits to avoid flaky tests.Using
waitForTimeout(50)is a code smell in e2e tests that can lead to flakiness. Consider waiting for the input to be visible or for a network request to complete instead.🔎 Suggested fix:
- await page.waitForTimeout(50); const maxRecipientsPerMessageInput = page.getByLabel("Maximum recipients per message"); await expect(maxRecipientsPerMessageInput).toBeVisible({ timeout: 5000 });The
waitForTimeout(50)on line 65 can be removed since the next line already waits for visibility. Similarly for line 83.src/frontend/src/features/ui/components/quota-badge/index.tsx (1)
106-122: Consider using a non-interactive element for the tooltip target.Using a
<button>element purely as a tooltip target without any click handler may confuse assistive technologies. Consider using a<span>withtabIndex={0}androle="status"for better semantics.🔎 Suggested fix:
<Tooltip content={tooltipText} placement="top"> - <button - type="button" + <span + tabIndex={0} + role="status" className={clsx("quota-badge-container", { "quota-badge": !compact, "quota-badge--warning": !compact && isLow && !isCritical, "quota-badge--critical": !compact && isCritical, })} > {indicator} {quota.recipient_count}/{quota.quota_limit} <Icon name="person" type={IconType.OUTLINED} size={IconSize.SMALL} /> {periodShortLabel} {!compact && ` (${quota.usage_percentage}%)`} - </button> + </span> </Tooltip>src/frontend/src/features/forms/components/message-form/index.tsx (1)
32-40: Extract sharedRecipientQuotatype to avoid duplication.The
RecipientQuotatype is duplicated between this file andsrc/frontend/src/features/ui/components/quota-badge/index.tsx. Consider extracting it to a shared types file.src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (6)
26-30: Untranslated strings in constant definition.The
labelvalues inPERIOD_OPTIONSare hardcoded in English. While you do translate them at render time (line 183-186), consider defining these with translation keys for consistency, or add a comment clarifying that translation happens at usage.
35-52: Consider extracting shared parsing utilities.The
parseMaxRecipientsandparseGlobalMaxRecipientsfunctions are duplicated in bothmodal-maildomain-manage-settingsandmodal-mailbox-manage-settings. Consider extracting these into a shared utility module to reduce duplication and ensure consistent behavior.
85-101: Consider using a validation schema instead of manual validation in onSubmit.The validation logic is duplicated between onSubmit and could be handled declaratively using react-hook-form's resolver with a schema (e.g., Zod). This would provide real-time validation feedback and reduce imperative code.
133-137: Using raw fetchAPI instead of generated mutation hook.Unlike
ModalMailboxManageSettingswhich uses the generateduseMaildomainsMailboxesSettingsUpdatehook, this component usesfetchAPIdirectly. Consider using a generated hook for consistency, better TypeScript support, and automatic error handling patterns.
167-172: Error type isany- consider proper error typing.The catch block uses
anytype which loses type safety. Consider defining an error interface or using a type guard to safely access nested error properties.🔎 Suggested improvement:
- } catch (err: any) { - const errorMessage = err?.data?.custom_limits?.[0] || err?.data?.detail || t('An error occurred while updating the domain settings.'); + } catch (err: unknown) { + let errorMessage = t('An error occurred while updating the domain settings.'); + if (err && typeof err === 'object' && 'data' in err) { + const data = (err as { data?: { custom_limits?: string[]; detail?: string } }).data; + errorMessage = data?.custom_limits?.[0] || data?.detail || errorMessage; + } setError(errorMessage); } finally {
217-241: Inline styles reduce maintainability.The flex layout uses inline styles. Consider using CSS classes or styled-components for better maintainability and consistency with the rest of the codebase.
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx (1)
33-53: Significant code duplication with ModalUpdateDomain.The parsing functions
parseMaxRecipientsandparseGlobalMaxRecipients, thePERIOD_OPTIONSconstant, the form structure, and validation logic are nearly identical tomodal-maildomain-manage-settings. Consider extracting shared logic into:
- A shared utility file for parsing functions
- A shared constants file for period options
- Potentially a custom hook for the common form logic
src/backend/core/services/quota_redis.py (2)
73-87: TTL calculation for daily period may be negative near end of day.If the current time is very close to 23:59:59 (e.g., at 23:59:59.5),
end_of_day - nowcould be near zero or negative due to microseconds being set to 0. While the 3600s buffer mitigates this, consider usingend_of_day + timedelta(days=1)for robustness.🔎 Alternative approach:
if period == "d": # Expire at end of day + 1 hour buffer now = timezone.now() - end_of_day = now.replace(hour=23, minute=59, second=59, microsecond=0) - return int((end_of_day - now).total_seconds()) + 3600 + # Calculate seconds until midnight of the next day + buffer + tomorrow = (now + timedelta(days=1)).replace(hour=0, minute=0, second=0, microsecond=0) + return int((tomorrow - now).total_seconds()) + 3600
277-278: Module-level singleton may cause issues in tests.The singleton instance
quota_serviceis created at module import time. This can make testing difficult as the same instance is shared across tests. Consider providing a factory function or allowing dependency injection for testing purposes.src/backend/core/management/commands/redis_quotas_info.py (1)
69-76: scan_iter iteration may consume excessive memory for large key sets.The
list(redis.scan_iter(...))collects all matching keys into memory. Withcount=1000, it limits keys per SCAN call but not total keys collected. For very large deployments, consider counting incrementally without materializing the full list:🔎 Memory-efficient alternative:
try: for label, pattern in patterns: - # Use scan_iter with count limit to avoid blocking - keys = list(redis.scan_iter(pattern, count=1000)) - if keys: - self.stdout.write(f"{label}: {len(keys)}") + # Count keys without loading all into memory + count = sum(1 for _ in redis.scan_iter(pattern, count=1000)) + if count: + self.stdout.write(f"{label}: {count}") except Exception as e:src/backend/core/models.py (1)
361-383: Consider extracting validation logic to reduce duplication.The
custom_limitsvalidation logic inMailDomain.clean()is duplicated inMailbox.clean(). Consider extracting to a shared helper function.🔎 Suggested refactor:
def validate_custom_limits(custom_limits: dict) -> None: """Validate custom_limits structure for domain or mailbox.""" if not custom_limits: return if not isinstance(custom_limits, dict): raise ValidationError({"custom_limits": "custom_limits must be a dictionary"}) if "max_recipients_per_message" in custom_limits: value = 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"} ) if "max_recipients" in custom_limits: value = custom_limits["max_recipients"] if value is not None: try: parse_max_recipients(value) except ValueError as e: raise ValidationError({"custom_limits": str(e)}) from esrc/frontend/src/pages/domain/index.tsx (1)
113-132: Consider simplifying the IIFE pattern.The immediately-invoked function expression (IIFE) pattern for rendering the modal is functional but unconventional. A simpler conditional rendering would be more readable.
🔎 Suggested refactor:
{editedDomain && editAction === MailDomainEditAction.MANAGE_SETTINGS && ( <ModalUpdateDomain key={`${(domains.find(d => d.id === editedDomain.id) || editedDomain).id}-${(domains.find(d => d.id === editedDomain.id) || editedDomain).updated_at}`} domain={domains.find(d => d.id === editedDomain.id) || editedDomain} isOpen={true} onClose={() => { setEditedDomain(null) setEditAction(null) }} onSuccess={() => { refetchMaildomains(); setEditedDomain(null) setEditAction(null) }} /> )}Or extract
freshDomaincomputation outside the JSX for clarity.src/backend/core/api/viewsets/maildomain.py (1)
146-201: Consider extracting period calculation logic to reduce duplication.The period_start calculation and period_display mapping (lines 179-191) are duplicated in the mailbox quota action. Consider extracting to a shared helper.
🔎 Suggested helper function:
def get_period_metadata(period: str) -> dict: """Calculate period start and display name for quota response.""" now = timezone.now() if period == "d": period_start = now.replace(hour=0, minute=0, second=0, microsecond=0) elif period == "m": period_start = now.replace(day=1, hour=0, minute=0, second=0, microsecond=0) elif period == "y": period_start = now.replace( month=1, day=1, hour=0, minute=0, second=0, microsecond=0 ) else: period_start = now period_display = {"d": "day", "m": "month", "y": "year"}.get(period, period) return { "period": period, "period_display": period_display, "period_start": period_start, }src/backend/core/api/serializers.py (2)
1007-1061: Consider adding a docstring to document the mixin's purpose and assumptions.The validation logic is well-implemented and handles both per-message and periodic recipient limits. However, a class docstring would help clarify:
- The mixin's purpose (reusable custom_limits validation)
- The assumption that the using serializer must have
Meta.modelset to eithermodels.Mailboxormodels.MailDomainRegarding the static analysis warning on line 1058: This is a false positive. The exception messages from
parse_max_recipientsare user-facing validation errors (e.g., "must be in format 'number/period'"), not sensitive information.📝 Suggested docstring
class CustomLimitsUpdateMixin: - """Mixin for validating custom_limits field.""" + """Mixin for validating custom_limits field. + + Provides validation for custom_limits dictionaries containing: + - max_recipients_per_message: integer limit per message + - max_recipients: string format "number/period" (e.g., "500/d") + + The using serializer must have Meta.model set to either + models.Mailbox or models.MailDomain to determine applicable global limits. + """
1668-1701: LGTM! Comprehensive quota status serializer.The serializer properly exposes all relevant quota information with clear field descriptions. Since it's read-only and not used for create/update operations, the current implementation is functional.
Optionally, for consistency with the
ImportBaseSerializerpattern used elsewhere in this file (lines 1343-1352), you could explicitly disable create/update methods, though this is not essential given the read-only nature.src/backend/core/api/openapi.json (7)
221-275: Config: new MAX_ recipient settings are well integrated; consider tightening string formats*The new global and default recipient-limit settings under
/api/v1.0/config/line up with the quota feature and are correctly markedreadOnlyandrequired, so clients can rely on them being present. If you want stronger client-side validation, you could optionally add apatternfor the"number/period"strings (e.g.^[0-9]+/[dmy]$) onMAX_*RECIPIENTS_FOR_*to document the expected format more formally.
2907-2941: Quota endpoints: shape is consistent; maybe add error responses laterThe new mailbox- and maildomain-level quota endpoints (
/mailboxes/{id}/quota/,/maildomains/{maildomain_pk}/mailboxes/{id}/quota/,/maildomains/{maildomain_pk}/quota/) consistently return the sharedRecipientQuotaschema and reuse existing auth/tagging conventions, which is good for consumers. You might eventually want to document403/404responses as well (to mirror other admin endpoints), but that’s a nice-to-have, not a blocker.Also applies to: 3647-3691, 4137-4173
3066-3202: MailDomainAdmin update surface looks coherent;custom_limitstype could be more explicitAdding
PUT/PATCHon/maildomains/{maildomain_pk}/with dedicatedMailDomainAdminUpdate*schemas cleanly scopes writes tocustom_limitsand avoids overloading the read model. Right nowcustom_limitsin those update schemas is untyped (only adescription), while the read-onlyMailDomainAdmin.custom_limitshas a fully specified object shape; if you want generated clients to understand the structure, consider reusing or referencing the same object schema there.Also applies to: 5935-5961, 7275-7283
6291-6384: MailboxAdmin:custom_limitsread model and write paths look consistent, but there are two ways to updateExposing
custom_limitsonMailboxAdminwith the same object shape as domains is clear, and the note onMailboxAdminCreateabout using a dedicated/settings/endpoint avoids suggesting they’re settable at creation time. You now support updating mailbox limits both viametadata.custom_limitsinMailboxAdminUpdateMetadataRequestand via/mailboxes/{id}/settings/; if possible, it might be cleaner long term to steer new clients towards the settings endpoint and treat the metadata route as legacy/compat only.Also applies to: 6386-6467, 6509-6520
3510-3551: Mailboxidpath params under maildomains loseformat: uuidhintFor several
/maildomains/{maildomain_pk}/mailboxes/{id}/…operations, theidpath parameter is now just"type": "string"withoutformat: "uuid", whereasmaildomain_pk(and many other ids) still advertise the UUID format. This isn’t functionally wrong, but if the underlying field is still a UUID, restoring theformatwould make the schema more informative for generated clients and validators.Also applies to: 3553-3609, 3610-3646, 3693-3765
3767-3831: Mailbox settings endpoint and schemas are minimal but serviceableThe
/maildomains/{maildomain_pk}/mailboxes/{id}/settings/PATCH endpoint, withMailboxSettingsUpdate/PatchedMailboxSettingsUpdateRequest, cleanly isolates mailbox settings (custom_limits) updates and keeps the response small. Similar to the domain update schemas,custom_limitshere is untyped (description-only); consider defining it as an object with the same properties as the read models if you want client codegen to expose structured fields instead of a generic “any”.Also applies to: 6556-6573, 7306-7314
7442-7486: RecipientQuota schema is clear; enums and bounds could further document constraints
RecipientQuotanicely captures all the fields a UI needs (period, counts, remaining, and usage_percentage). As optional polish, you might tighten it by: (a) turningperiodinto an enum (d,m,y) and (b) addingminimum/maximumonusage_percentage(0–100) so clients and validators can enforce the same invariants you describe in the docstring.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (21)
src/frontend/src/features/api/gen/mailboxes/mailboxes.tsis excluded by!**/gen/**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/**src/frontend/src/features/api/gen/models/recipient_quota.tsis excluded by!**/gen/**
📒 Files selected for processing (41)
compose.yaml(2 hunks)env.d/development/backend.e2e(1 hunks)src/backend/core/api/openapi.json(26 hunks)src/backend/core/api/permissions.py(1 hunks)src/backend/core/api/serializers.py(14 hunks)src/backend/core/api/viewsets/config.py(4 hunks)src/backend/core/api/viewsets/mailbox.py(4 hunks)src/backend/core/api/viewsets/maildomain.py(8 hunks)src/backend/core/api/viewsets/send.py(3 hunks)src/backend/core/apps.py(1 hunks)src/backend/core/checks.py(1 hunks)src/backend/core/enums.py(3 hunks)src/backend/core/management/commands/redis_quotas_info.py(1 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(13 hunks)src/backend/core/services/quota_redis.py(1 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.scss(1 hunks)src/frontend/src/features/forms/components/message-form/index.tsx(12 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(8 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/features/ui/components/quota-badge/_index.scss(1 hunks)src/frontend/src/features/ui/components/quota-badge/index.tsx(1 hunks)src/frontend/src/hooks/use-ability.ts(3 hunks)src/frontend/src/pages/domain/index.tsx(6 hunks)src/frontend/src/styles/main.scss(1 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/mda/draft.pysrc/backend/core/tests/api/test_mailboxes.pysrc/backend/core/tests/api/test_admin_maildomains_list.pysrc/backend/messages/settings.pysrc/backend/core/api/permissions.pysrc/backend/core/services/quota_redis.pysrc/backend/core/checks.pysrc/backend/core/apps.pysrc/backend/core/api/viewsets/mailbox.pysrc/backend/core/api/viewsets/send.pysrc/backend/core/management/commands/redis_quotas_info.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/tests/api/test_messages_create.pysrc/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.pysrc/backend/core/tests/api/test_admin_maildomains_update.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_config.pysrc/backend/core/enums.pysrc/backend/core/tests/models/test_mailbox.pysrc/backend/core/api/serializers.pysrc/backend/core/tests/models/test_maildomain.pysrc/backend/core/api/viewsets/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_mailboxes.pysrc/backend/core/tests/api/test_admin_maildomains_list.pysrc/backend/core/tests/api/test_messages_create.pysrc/backend/core/tests/api/test_admin_maildomains_update.pysrc/backend/core/tests/api/test_config.pysrc/backend/core/tests/models/test_mailbox.pysrc/backend/core/tests/models/test_maildomain.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,migrations/**/*.py}
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Implement database indexing and query optimization (Model Meta indexes, constraints)
Files:
src/backend/core/migrations/0012_mailbox_custom_attributes_mailbox_custom_limits.pysrc/backend/core/models.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/{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.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/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 : Use Django REST Framework viewsets for API endpoints
Applied to files:
src/backend/core/api/viewsets/mailbox.pysrc/backend/core/api/viewsets/maildomain.py
🧬 Code graph analysis (20)
src/backend/core/mda/draft.py (3)
src/backend/core/api/serializers.py (1)
get_max_recipients_per_message(294-296)src/backend/core/models.py (1)
get_max_recipients_per_message(852-881)src/backend/core/enums.py (1)
MessageRecipientTypeChoices(31-36)
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(73-79)src/frontend/src/features/api/gen/maildomains/maildomains.ts (1)
useMaildomainsMailboxesSettingsUpdate(2077-2109)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/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(73-79)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/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(54-256)
src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx (3)
src/frontend/src/features/api/gen/models/mailbox_admin.ts (1)
MailboxAdmin(15-43)src/frontend/src/features/ui/components/quota-badge/index.tsx (1)
QuotaBadge(30-123)src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx (1)
ModalMailboxManageSettings(55-248)
src/backend/core/api/permissions.py (2)
src/backend/core/models.py (5)
get_abilities(270-285)get_abilities(420-458)get_abilities(732-794)MailDomain(288-530)Mailbox(587-928)src/backend/core/enums.py (2)
MailDomainAbilities(96-101)MailboxAbilities(104-116)
src/e2e/src/__tests__/message-max-recipients.spec.ts (1)
src/e2e/src/utils-test.ts (1)
signInKeycloakIfNeeded(6-39)
src/backend/core/checks.py (1)
src/backend/core/services/quota_redis.py (1)
redis(25-44)
src/backend/core/api/viewsets/mailbox.py (3)
src/backend/core/models.py (3)
MailDomainAccess(1809-1831)get_max_recipients(497-530)get_max_recipients(883-928)src/backend/core/api/serializers.py (1)
RecipientQuotaSerializer(1668-1701)src/backend/core/services/quota_redis.py (1)
get_status(242-274)
src/backend/core/api/viewsets/send.py (2)
src/backend/core/models.py (3)
Mailbox(587-928)get_max_recipients(497-530)get_max_recipients(883-928)src/backend/core/services/quota_redis.py (1)
check_and_increment(135-224)
src/e2e/src/__tests__/message-max-recipients-admin.spec.ts (1)
src/e2e/src/utils-test.ts (1)
signInKeycloakIfNeeded(6-39)
src/backend/core/management/commands/redis_quotas_info.py (1)
src/backend/core/services/quota_redis.py (1)
redis(25-44)
src/frontend/src/pages/domain/index.tsx (4)
src/frontend/src/features/api/gen/maildomains/maildomains.ts (1)
getMaildomainsListQueryOptions(103-132)src/frontend/src/features/api/gen/models/mail_domain_admin.ts (1)
MailDomainAdmin(14-30)src/frontend/src/features/ui/components/quota-badge/index.tsx (1)
QuotaBadge(30-123)src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (1)
ModalUpdateDomain(54-256)
src/backend/core/tests/api/test_messages_create.py (5)
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/api/viewsets/send.py (1)
post(146-206)src/backend/core/api/viewsets/draft.py (1)
post(197-231)
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/tests/api/test_admin_maildomains_update.py (4)
src/backend/core/factories.py (6)
UserFactory(20-31)MailDomainFactory(51-57)MailDomainAccessFactory(112-122)MailboxFactory(60-96)MailboxAccessFactory(99-109)ContactFactory(148-157)src/backend/core/enums.py (2)
MailDomainAccessRoleChoices(48-51)MailboxRoleChoices(15-21)src/backend/core/models.py (8)
save(137-140)save(252-255)save(344-347)save(634-637)save(1186-1238)save(1733-1736)save(1989-2005)get_max_recipients_per_message(852-881)src/backend/core/api/serializers.py (1)
get_max_recipients_per_message(294-296)
src/backend/core/models.py (4)
src/backend/core/enums.py (1)
QuotaPeriodChoices(126-130)src/backend/core/api/fields.py (1)
JSONField(8-25)src/backend/core/api/viewsets/config.py (1)
get(173-231)src/backend/core/api/serializers.py (1)
get_max_recipients_per_message(294-296)
src/backend/core/api/serializers.py (1)
src/backend/core/models.py (4)
parse_max_recipients(52-93)Mailbox(587-928)get_max_recipients_per_message(852-881)MailDomain(288-530)
src/backend/core/tests/models/test_maildomain.py (1)
src/backend/core/models.py (3)
get_abilities(270-285)get_abilities(420-458)get_abilities(732-794)
🪛 GitHub Check: CodeQL
src/backend/core/api/serializers.py
[warning] 1058-1058: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 Ruff (0.14.8)
src/backend/core/services/quota_redis.py
28-28: import should be at the top-level of a file
(PLC0415)
30-30: import should be at the top-level of a file
(PLC0415)
src/backend/core/apps.py
21-21: import should be at the top-level of a file
(PLC0415)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (python)
| redis-quotas: | ||
| image: redis:7-alpine | ||
| command: > | ||
| redis-server | ||
| --port 6380 | ||
| --maxmemory-policy noeviction | ||
| --appendonly yes | ||
| --appendfsync everysec | ||
| --save 900 1 | ||
| --save 300 10 | ||
| --save 60 10000 | ||
| ports: | ||
| - "8916:6380" | ||
| volumes: | ||
| - redis-quotas-data:/data | ||
| healthcheck: | ||
| test: ["CMD", "redis-cli", "-p", "6380", "ping"] | ||
| interval: 5s | ||
| timeout: 3s | ||
| retries: 5 | ||
| restart: unless-stopped |
There was a problem hiding this comment.
Port conflict: redis-quotas and socks-proxy both use host port 8916.
Line 334 maps socks-proxy to 8916:1080 and line 34 maps redis-quotas to 8916:6380. This creates a port conflict where only one service can bind to host port 8916, causing the other to fail.
🔎 Apply this diff to resolve the port conflict:
ports:
- - "8916:6380"
+ - "8918:6380"Note: Choose an available port number that doesn't conflict with existing services.
📝 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.
| redis-quotas: | |
| image: redis:7-alpine | |
| command: > | |
| redis-server | |
| --port 6380 | |
| --maxmemory-policy noeviction | |
| --appendonly yes | |
| --appendfsync everysec | |
| --save 900 1 | |
| --save 300 10 | |
| --save 60 10000 | |
| ports: | |
| - "8916:6380" | |
| volumes: | |
| - redis-quotas-data:/data | |
| healthcheck: | |
| test: ["CMD", "redis-cli", "-p", "6380", "ping"] | |
| interval: 5s | |
| timeout: 3s | |
| retries: 5 | |
| restart: unless-stopped | |
| redis-quotas: | |
| image: redis:7-alpine | |
| command: > | |
| redis-server | |
| --port 6380 | |
| --maxmemory-policy noeviction | |
| --appendonly yes | |
| --appendfsync everysec | |
| --save 900 1 | |
| --save 300 10 | |
| --save 60 10000 | |
| ports: | |
| - "8918:6380" | |
| volumes: | |
| - redis-quotas-data:/data | |
| healthcheck: | |
| test: ["CMD", "redis-cli", "-p", "6380", "ping"] | |
| interval: 5s | |
| timeout: 3s | |
| retries: 5 | |
| restart: unless-stopped |
🤖 Prompt for AI Agents
In compose.yaml around lines 22 to 42 there is a host port conflict:
redis-quotas maps host port 8916:6380 while socks-proxy also maps host port
8916:1080 (line ~334), so only one can bind. Fix by changing one service's host
port to an unused port (for example change redis-quotas or socks-proxy host side
from 8916 to an available port like 8917 or another free port), update any docs
or consumer configs that expect the old host port, and ensure the chosen port
does not collide with other services.
src/backend/core/checks.py
Outdated
| try: | ||
| client = redis.Redis( | ||
| host=settings.REDIS_QUOTAS_HOST, | ||
| port=settings.REDIS_QUOTAS_PORT, | ||
| db=settings.REDIS_QUOTAS_DB, | ||
| password=settings.REDIS_QUOTAS_PASSWORD, | ||
| socket_connect_timeout=2, | ||
| decode_responses=True, | ||
| ) | ||
|
|
||
| # Test ping | ||
| if not client.ping(): | ||
| raise ConnectionError("Ping failed") | ||
|
|
||
| # Test write/read | ||
| test_key = "health:check:test" | ||
| client.set(test_key, "ok", ex=10) | ||
| value = client.get(test_key) | ||
| client.delete(test_key) | ||
|
|
||
| if value != "ok": | ||
| raise ValueError("Write/read test failed") | ||
|
|
||
| client.close() |
There was a problem hiding this comment.
Resource leak: client.close() not called on exception path.
If an exception occurs after the Redis client is created (line 15) but before client.close() (line 37), the connection is not properly closed. Use a try/finally block to ensure cleanup.
🔎 Apply this diff to ensure proper cleanup:
try:
client = redis.Redis(
host=settings.REDIS_QUOTAS_HOST,
port=settings.REDIS_QUOTAS_PORT,
db=settings.REDIS_QUOTAS_DB,
password=settings.REDIS_QUOTAS_PASSWORD,
socket_connect_timeout=2,
decode_responses=True,
)
- # Test ping
- if not client.ping():
- raise ConnectionError("Ping failed")
-
- # Test write/read
- test_key = "health:check:test"
- client.set(test_key, "ok", ex=10)
- value = client.get(test_key)
- client.delete(test_key)
-
- if value != "ok":
- raise ValueError("Write/read test failed")
-
- client.close()
+ try:
+ # Test ping
+ if not client.ping():
+ raise ConnectionError("Ping failed")
+
+ # Test write/read
+ test_key = "health:check:test"
+ client.set(test_key, "ok", ex=10)
+ value = client.get(test_key)
+ client.delete(test_key)
+
+ if value != "ok":
+ raise ValueError("Write/read test failed")
+ finally:
+ client.close()
except (redis.RedisError, ConnectionError, ValueError) as e:🤖 Prompt for AI Agents
In src/backend/core/checks.py around lines 14 to 37, the Redis client is created
but client.close() is only called on the success path, which leaks the
connection if an exception occurs; wrap the client usage in a try/finally (or
use a context manager) so client.close() is always executed in the finally
block, moving the ping/write/read/delete logic into the try and calling
client.close() from finally to guarantee cleanup on exceptions.
| class QuotaPeriodChoices(models.TextChoices): | ||
| """Defines the possible quota period types.""" | ||
|
|
||
| DAY = "d", "day" | ||
| MONTH = "m", "month" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if yearly period is used in the codebase
rg -n '"y"' --type=py src/backend/core/ | head -30Repository: suitenumerique/messages
Length of output: 1288
🏁 Script executed:
#!/bin/bash
# Search for mailbox.py and examine its content
find src/backend -name "mailbox.py" -type fRepository: suitenumerique/messages
Length of output: 107
🏁 Script executed:
#!/bin/bash
# Search for get_max_recipients function
rg -n "get_max_recipients" --type=py src/backend/Repository: suitenumerique/messages
Length of output: 1613
🏁 Script executed:
#!/bin/bash
# Examine the enums.py file to see the full QuotaPeriodChoices definition
cat -n src/backend/core/enums.py | grep -A 20 "QuotaPeriodChoices"Repository: suitenumerique/messages
Length of output: 579
🏁 Script executed:
#!/bin/bash
# Examine mailbox.py around lines 129-136 to see the exact code
sed -n '110,150p' src/backend/core/api/viewsets/mailbox.pyRepository: suitenumerique/messages
Length of output: 1387
Add YEAR period to QuotaPeriodChoices enum for consistency.
The QuotaPeriodChoices enum currently defines only DAY and MONTH, but the mailbox.py viewset and other endpoints handle yearly ("y") periods (see lines 129-136), and get_max_recipients() can return "y" as a period value. Add YEAR = "y", "year" to keep the enum definition in sync with actual usage.
🤖 Prompt for AI Agents
In src/backend/core/enums.py around lines 126 to 131, QuotaPeriodChoices
currently defines only DAY and MONTH but the codebase uses a yearly period
("y"); add YEAR = "y", "year" as a new member of QuotaPeriodChoices so the enum
matches usage elsewhere (e.g., mailbox viewset and get_max_recipients) and
update any related imports/tests if they validate enum members.
| def get_max_recipients(self) -> tuple[int, str]: | ||
| """Return the effective max recipients per period for a mail domain. | ||
| Priority: | ||
| 1. MailDomain custom_limits.max_recipients | ||
| 2. Global setting MAX_DEFAULT_RECIPIENTS_FOR_DOMAIN | ||
| 3. Capped by MAX_RECIPIENTS_FOR_DOMAIN (cannot be exceeded) | ||
| Returns: | ||
| Tuple of (limit, period) where period is 'd', 'm', or 'y' | ||
| """ | ||
| # Parse global settings for domain | ||
| domain_max_limit, domain_max_period = parse_max_recipients( | ||
| settings.MAX_RECIPIENTS_FOR_DOMAIN | ||
| ) | ||
| domain_default_limit, domain_default_period = parse_max_recipients( | ||
| settings.MAX_DEFAULT_RECIPIENTS_FOR_DOMAIN | ||
| ) | ||
|
|
||
| # MailDomain-level override | ||
| if hasattr(self, "custom_limits") and self.custom_limits: | ||
| value = self.custom_limits.get("max_recipients") | ||
| if value: | ||
| try: | ||
| limit, period = parse_max_recipients(value) | ||
| if period == domain_max_period: | ||
| return min(limit, domain_max_limit), period | ||
| return limit, period | ||
| except ValueError: | ||
| pass # Invalid format, fall through to global | ||
|
|
||
| # Global fallback - ensure it never exceeds the domain maximum | ||
| effective_limit = min(domain_default_limit, domain_max_limit) | ||
| return effective_limit, domain_default_period |
There was a problem hiding this comment.
Docstring mentions 'y' (year) period but it's not supported.
The docstring on line 506 states the period can be 'd', 'm', or 'y', but QuotaPeriodChoices only defines DAY = "d" and MONTH = "m". The 'y' period would fail validation in parse_max_recipients.
🔎 Apply this diff to fix the docstring:
Returns:
- Tuple of (limit, period) where period is 'd', 'm', or 'y'
+ Tuple of (limit, period) where period is 'd' or 'm'🤖 Prompt for AI Agents
In src/backend/core/models.py around lines 497 to 530, the docstring for
get_max_recipients incorrectly lists the period options as 'd', 'm', or 'y'
though QuotaPeriodChoices and parse_max_recipients only support 'd' and 'm';
update the docstring to list only 'd' and 'm' (or, if year support is intended,
add 'y' to QuotaPeriodChoices and adjust parsing/validation) so the documented
periods match the actual supported choices.
| def get_max_recipients(self) -> tuple[int, str]: | ||
| """Return the effective max recipients per period for a mailbox. | ||
| Priority: | ||
| 1. Mailbox custom_limits.max_recipients | ||
| 2. Global setting MAX_DEFAULT_RECIPIENTS_FOR_MAILBOX | ||
| 3. Capped by MAX_RECIPIENTS_FOR_MAILBOX (cannot be exceeded) | ||
| Note: Domain's max_recipients is NOT inherited - it's for the aggregate | ||
| domain quota. Both mailbox and domain quotas are enforced independently | ||
| at send time via Redis. | ||
| Returns: | ||
| Tuple of (limit, period) where period is 'd', 'm', or 'y' | ||
| """ | ||
| # Parse global settings for mailbox | ||
| mailbox_max_limit, mailbox_max_period = parse_max_recipients( | ||
| settings.MAX_RECIPIENTS_FOR_MAILBOX | ||
| ) | ||
| mailbox_default_limit, mailbox_default_period = parse_max_recipients( | ||
| settings.MAX_DEFAULT_RECIPIENTS_FOR_MAILBOX | ||
| ) | ||
|
|
||
| # Mailbox-level override | ||
| if hasattr(self, "custom_limits") and self.custom_limits: | ||
| value = self.custom_limits.get("max_recipients") | ||
| if value: | ||
| try: | ||
| limit, period = parse_max_recipients(value) | ||
| # Period must match mailbox max period | ||
| if period == mailbox_max_period: | ||
| return min(limit, mailbox_max_limit), period | ||
| # If periods differ, use the mailbox value but log a warning | ||
| logger.warning( | ||
| "Mailbox %s has max_recipients period '%s' different from global '%s'", | ||
| str(self), | ||
| period, | ||
| mailbox_max_period, | ||
| ) | ||
| return limit, period | ||
| except ValueError: | ||
| pass # Invalid format, fall through to global | ||
|
|
||
| # Global fallback for mailbox - ensure it never exceeds the mailbox maximum | ||
| effective_limit = min(mailbox_default_limit, mailbox_max_limit) | ||
| return effective_limit, mailbox_default_period |
There was a problem hiding this comment.
Docstring mentions 'y' (year) period but it's not supported.
Same issue as in MailDomain.get_max_recipients() - the docstring on line 896 mentions 'y' but this period is not supported.
🔎 Apply this diff to fix the docstring:
Returns:
- Tuple of (limit, period) where period is 'd', 'm', or 'y'
+ Tuple of (limit, period) where period is 'd' or 'm'🤖 Prompt for AI Agents
In src/backend/core/models.py around lines 883 to 928, the get_max_recipients
docstring incorrectly lists 'y' (year) as a supported period even though the
code does not support it; update the docstring to remove 'y' (only list the
actual supported periods, e.g., 'd' and 'm'), making it consistent with
MailDomain.get_max_recipients(), and ensure any example or return description
reflects the correct allowed period values.
| {error && ( | ||
| <Banner type="error"> | ||
| {t(error)} | ||
| </Banner> |
There was a problem hiding this comment.
Same i18n issue with dynamic error message.
Same as the domain modal - t(error) won't translate API error messages that aren't predefined translation keys.
🔎 Suggested fix:
{error && (
<Banner type="error">
- {t(error)}
+ {error}
</Banner>
)}📝 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.
| {error && ( | |
| <Banner type="error"> | |
| {t(error)} | |
| </Banner> | |
| {error && ( | |
| <Banner type="error"> | |
| {error} | |
| </Banner> | |
| )} |
🤖 Prompt for AI Agents
In
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx
around lines 189-192, the Banner currently renders t(error) which incorrectly
treats arbitrary API error messages as i18n keys; change the rendering to detect
whether the error is a known translation key (use the i18n exists function) and
call t(error) only in that case, otherwise render the raw error message (or a
generic translated message with the error injected via interpolation, e.g.
t('apiError.generic', { message: error })); ensure you handle non-string errors
(extract .message) and avoid passing unknown keys to t.
| const form = useForm<UpdateDomainFormData>({ | ||
| defaultValues: { | ||
| max_recipients_per_message: domain.custom_limits?.max_recipients_per_message?.toString() ?? '', | ||
| max_recipients_limit: parsedMaxRecipients.limit, | ||
| max_recipients_period: parsedMaxRecipients.period, | ||
| }, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "ModalUpdateDomain|modal-maildomain-manage-settings" --type=tsx --type=ts -C 3Repository: suitenumerique/messages
Length of output: 94
🏁 Script executed:
#!/bin/bash
# Search for ModalUpdateDomain usage
rg "ModalUpdateDomain|modal-maildomain-manage-settings" -C 3
# Also find the component file
find . -type f -name "*modal*maildomain*" 2>/dev/null | head -20Repository: suitenumerique/messages
Length of output: 5623
🏁 Script executed:
#!/bin/bash
# Read the modal component implementation
echo "=== MODAL COMPONENT ==="
wc -l src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx
# Read full component file
echo ""
cat src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsxRepository: suitenumerique/messages
Length of output: 9412
🏁 Script executed:
#!/bin/bash
# Get context around the first usage in domain/index.tsx
echo "=== USAGE 1: src/frontend/src/pages/domain/index.tsx ==="
rg -n "ModalUpdateDomain" src/frontend/src/pages/domain/index.tsx -B 10 -A 10
echo ""
echo "=== USAGE 2: src/frontend/src/features/layouts/components/admin/domains-view/settings-domain-action.tsx ==="
rg -n "ModalUpdateDomain" src/frontend/src/features/layouts/components/admin/domains-view/settings-domain-action.tsx -B 10 -A 10Repository: suitenumerique/messages
Length of output: 3268
🏁 Script executed:
#!/bin/bash
# Search for all usages of ModalUpdateDomain in the entire codebase
rg "ModalUpdateDomain" --type ts --type js 2>/dev/null || rg "ModalUpdateDomain"Repository: suitenumerique/messages
Length of output: 923
Form may display stale values when reopening modal with different domain data.
React Hook Form caches defaultValues, which are only evaluated once when the form is initialized. The component currently lacks a mechanism to update the form when the domain prop changes. While the primary usage in src/frontend/src/pages/domain/index.tsx mitigates this via the key prop (causing component remounting), the secondary usage in settings-domain-action.tsx has no such protection. If a user selects a different domain between modal close and reopen, the form will display the previous domain's cached values.
Add a useEffect hook to call form.reset(domain) when the domain prop changes, or ensure all usages include a key prop that changes with the domain (e.g., key={domain.id}).
| {error && ( | ||
| <Banner type="error"> | ||
| {t(error)} | ||
| </Banner> |
There was a problem hiding this comment.
Passing dynamic error messages to t() may not work as expected.
On line 199, t(error) passes a dynamic error string to the translation function. If error comes from the API and isn't a known translation key, it won't be translated. Consider displaying the error directly without wrapping in t(), or ensure the backend returns translation keys.
🔎 Suggested fix:
{error && (
<Banner type="error">
- {t(error)}
+ {error}
</Banner>
)}📝 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.
| {error && ( | |
| <Banner type="error"> | |
| {t(error)} | |
| </Banner> | |
| {error && ( | |
| <Banner type="error"> | |
| {error} | |
| </Banner> | |
| )} |
🤖 Prompt for AI Agents
In
src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx
around lines 197 to 200, the code calls t(error) which assumes the API returns a
translation key; if it returns a raw message this will fail to show correctly.
Change the rendering to display the API error string directly (e.g., show error
without wrapping in t()), or, if you must translate, map/validate the API value
to a known translation key before calling t(). Ensure the Banner receives the
final user-facing string (either the raw error or a translated key).
| &:focus { | ||
| outline: none; | ||
| } |
There was a problem hiding this comment.
Restore focus indicators for keyboard accessibility.
Removing the focus outline completely (outline: none) breaks keyboard navigation and violates WCAG accessibility guidelines. Users navigating with keyboards need visible focus indicators to understand where they are on the page.
🔎 Apply this diff to provide a visible focus indicator:
&:focus {
- outline: none;
+ outline: 2px solid var(--c--theme--colors--primary-500);
+ outline-offset: 2px;
}📝 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.
| &:focus { | |
| outline: none; | |
| } | |
| &:focus { | |
| outline: 2px solid var(--c--theme--colors--primary-500); | |
| outline-offset: 2px; | |
| } |
🤖 Prompt for AI Agents
In src/frontend/src/features/ui/components/quota-badge/_index.scss around lines
18–20, remove the blanket outline: none; which hides keyboard focus and breaks
accessibility; instead restore a visible, high-contrast focus indicator
(preferably using :focus-visible to target keyboard users) by replacing the rule
with a clear outline or accessible box-shadow (2px or similar, high-contrast
color or using currentColor) that works in both light/dark themes and does not
interfere with the component layout; ensure the new style only applies for
keyboard focus and keep mouse focus untouched.
| ports: | ||
| - "8913:6379" | ||
|
|
||
| redis-quotas: |
There was a problem hiding this comment.
we already have a redis server, let's reuse it
| # not at configuration time. | ||
|
|
||
| except ValueError as e: | ||
| raise serializers.ValidationError(str(e)) from e |
There was a problem hiding this comment.
AI is right: don't serialize errors straight to the user
| ), | ||
| migrations.AddField( | ||
| model_name='maildomain', | ||
| name='custom_limits', |
There was a problem hiding this comment.
why not reuse custom_settings ? (and possibly add Mailbox.custom_settings)
| ) | ||
| # Default maximum allowed recipients per message for a mailbox or maildomain | ||
| # if no custom limit is set. | ||
| MAX_DEFAULT_RECIPIENTS_PER_MESSAGE = values.PositiveIntegerValue( |
There was a problem hiding this comment.
I think we can find a better name than MAX_DEFAULT_* ? It's a bit confusing
| description="Get the recipient quota status for this mailbox.", | ||
| ) | ||
| @action(detail=True, methods=["get"], url_path="quota") | ||
| def quota(self, request, maildomain_pk=None, pk=None): # pylint: disable=unused-argument |
There was a problem hiding this comment.
this code is repeated 3 times in the PR
| domain_limit, domain_period = mailbox.domain.get_max_recipients() | ||
|
|
||
| # Atomic check and increment for mailbox and domain quotas | ||
| success, failed_entity, remaining = quota_service.check_and_increment( |
There was a problem hiding this comment.
the quota check should not be in the viewset, but in mda/outbound
| def clean(self): | ||
| """Validate custom limits.""" | ||
| # Validate custom_limits structure | ||
| if self.custom_limits: |
sdemagny
force-pushed
the
sdem/max_recipients_and_quota
branch
from
ec74692 to
1cd50e7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (11)
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx (1)
189-192: Dynamic error message won't translate correctly.Using
t(error)treats the API error string as a translation key, which won't work for dynamic API messages. Display the error directly instead.🔎 Suggested fix
{error && ( <Banner type="error"> - {t(error)} + {error} </Banner> )}src/backend/core/enums.py (1)
126-131: Add YEAR period to QuotaPeriodChoices for consistency.The
QuotaPeriodChoicesenum defines only DAY and MONTH, but the codebase handles yearly periods ("y") in multiple places (e.g.,get_max_recipients()returns "y", viewsets map "y" to "year" for display). AddYEAR = "y", "year"to keep the enum synchronized with actual usage.🔎 Suggested fix
class QuotaPeriodChoices(models.TextChoices): """Defines the possible quota period types.""" DAY = "d", "day" MONTH = "m", "month" + YEAR = "y", "year"src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (2)
66-72: Form may display stale values when reopening modal with different domain.React Hook Form's
defaultValuesare only evaluated once on mount. If thedomainprop changes while the component remains mounted, the form will display the previous domain's cached values. Add auseEffectto reset the form whendomainchanges.🔎 Suggested fix
+import { useEffect } from 'react'; const form = useForm<UpdateDomainFormData>({ defaultValues: { max_recipients_per_message: domain.custom_limits?.max_recipients_per_message?.toString() ?? '', max_recipients_limit: parsedMaxRecipients.limit, max_recipients_period: parsedMaxRecipients.period, }, }); +// Reset form when domain changes +useEffect(() => { + const parsed = parseMaxRecipients(domain.custom_limits?.max_recipients); + form.reset({ + max_recipients_per_message: domain.custom_limits?.max_recipients_per_message?.toString() ?? '', + max_recipients_limit: parsed.limit, + max_recipients_period: parsed.period, + }); +}, [domain.id, form]);
197-200: Dynamic API error strings passed tot()won't translate.If
errorcomes from the API and isn't a known i18n key, wrapping it int()has no effect. Display the error directly or ensure the backend returns translation keys.🔎 Suggested fix
{error && ( <Banner type="error"> - {t(error)} + {error} </Banner> )}src/backend/core/services/quota.py (1)
13-14: PeriodType missing "y" (yearly) period.This was flagged in a previous review. The docstrings in
models.pymention 'y' period support, butPeriodTypeonly defines "d" and "m".src/frontend/src/pages/domain/index.tsx (1)
2-2: Remove unuseduseModalimport.This was flagged in a previous review.
src/backend/core/models.py (3)
361-384: Duplicated validation logic with Mailbox.clean.As noted in a previous review, this custom_limits validation logic is repeated in both
MailDomain.cleanandMailbox.clean(lines 641-663). Consider extracting to a shared helper function.
497-530: Docstring mentions 'y' (year) period but it's not supported.This was flagged in a previous review. The docstring on line 506 states the period can be
'd', 'm', or 'y', butQuotaPeriodChoicesonly definesDAY = "d"andMONTH = "m".
883-928: Docstring mentions 'y' (year) period but it's not supported.Same issue as in
MailDomain.get_max_recipients()- the docstring on line 896 mentions'y'but this period is not supported.src/backend/core/api/viewsets/maildomain.py (1)
178-189: Extract period_start calculation to a helper function.This period calculation logic is repeated in the mailbox quota action (lines 495-506) and was already flagged in a previous review comment. Consider extracting it to a shared helper function to reduce duplication and improve maintainability.
🔎 Suggested refactor
Create a helper function at module level:
def calculate_period_start(period: str) -> datetime: """Calculate the start of the current period based on period type.""" now = timezone.now() if period == "d": return now.replace(hour=0, minute=0, second=0, microsecond=0) elif period == "m": return now.replace(day=1, hour=0, minute=0, second=0, microsecond=0) elif period == "y": return now.replace(month=1, day=1, hour=0, minute=0, second=0, microsecond=0) return nowThen use it in both quota actions:
- now = timezone.now() - if period == "d": - period_start = now.replace(hour=0, minute=0, second=0, microsecond=0) - elif period == "m": - period_start = now.replace(day=1, hour=0, minute=0, second=0, microsecond=0) - elif period == "y": - period_start = now.replace( - month=1, day=1, hour=0, minute=0, second=0, microsecond=0 - ) - else: - period_start = now + period_start = calculate_period_start(period)src/backend/core/api/serializers.py (1)
1007-1060: LGTM on validation logic; note existing security concern at line 1058.The custom limits validation is comprehensive and correctly handles:
- Positive integer checks
- Global maximum enforcement
- Period-based quota comparisons
- Mailbox vs domain-specific limits
However, line 1058 has an existing security concern (already flagged) about exposing internal exception details via
str(e).
🧹 Nitpick comments (20)
src/frontend/src/features/forms/components/message-form/_index.scss (1)
175-189: Well-structured quota display styling.The implementation is clean and follows good SCSS practices. The inline-flex layout with proper alignment and spacing will create an effective quota indicator badge.
One optional consideration: the hardcoded spacing values (4px gap, 2px 8px padding, 4px border-radius) could use design system tokens like
var(--c--theme--spacings--xxs)for better consistency with the rest of the stylesheet, though the current approach is perfectly acceptable for fine-tuned component styling.src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx (4)
159-162: Consider more robust error extraction.The error extraction chain
err?.data?.custom_limits?.[0]assumes a specific error structure. Consider handling additional error formats for better resilience.🔎 Suggested improvement
- } catch (err: any) { - const errorMessage = err?.data?.custom_limits?.[0] || err?.data?.detail || t('An error occurred while updating the mailbox settings.'); + } catch (err: unknown) { + const apiError = err as { data?: { custom_limits?: string[]; detail?: string; non_field_errors?: string[] } }; + const errorMessage = + apiError?.data?.custom_limits?.[0] || + apiError?.data?.non_field_errors?.[0] || + apiError?.data?.detail || + t('An error occurred while updating the mailbox settings.'); setError(errorMessage); }
196-206: Avoid mixingregisterspread with explicitvalueprop.Using both
{...register('max_recipients_per_message')}andvalue={maxRecipientsValue}can cause controlled/uncontrolled component conflicts. Theregisterfunction manages the input value via ref, while the explicitvalueprop creates a controlled input. Remove thevalueprop or useControllerfrom react-hook-form for controlled inputs.🔎 Suggested fix - Option 1: Remove explicit value prop
<Input {...register('max_recipients_per_message')} label={t('Maximum recipients per message')} type="number" min={1} max={MAX_RECIPIENTS_PER_MESSAGE} fullWidth state={errors.max_recipients_per_message ? 'error' : 'default'} text={helperTextPerMessage} - value={maxRecipientsValue} />
212-221: Same register/value conflict applies here.Remove the explicit
valueprop to let react-hook-form manage the input state.🔎 Suggested fix
<Input {...register('max_recipients_limit')} label={t('Maximum recipients per period')} type="number" min={1} fullWidth state={errors.max_recipients_limit ? 'error' : 'default'} text={helperTextQuota} - value={maxRecipientsLimitValue} />
210-232: Consider extracting inline styles to CSS classes.The inline styles for the flex layout work but reduce maintainability. Consider using CSS classes consistent with other modal components in the codebase.
src/backend/core/api/viewsets/send.py (1)
28-91: Well-structured quota check with atomic Redis operations.The function correctly uses the atomic
check_and_incrementfromquota_serviceto avoid race conditions. The error messages are translatable and provide useful context (remaining count, period, attempted count).Consider extracting the period display mapping to a shared constant or using
QuotaPeriodChoicesto avoid duplication (it appears on lines 62-63 and 78-79).🔎 Suggested refactor to reduce duplication
+PERIOD_DISPLAY_MAP = {"d": "day", "m": "month", "y": "year"} + def check_and_update_recipient_quota( mailbox: models.Mailbox, recipient_count: int ) -> None: ... if not success: if failed_entity == "mailbox": - period_display = {"d": "day", "m": "month", "y": "year"}.get( - mailbox_period, mailbox_period - ) + period_display = PERIOD_DISPLAY_MAP.get(mailbox_period, mailbox_period) ... # Domain quota exceeded - period_display = {"d": "day", "m": "month", "y": "year"}.get( - domain_period, domain_period - ) + period_display = PERIOD_DISPLAY_MAP.get(domain_period, domain_period)src/backend/core/api/viewsets/mailbox.py (1)
91-146: Quota endpoint implementation is correct.The endpoint properly retrieves quota limits, fetches usage from Redis via
quota_service.get_status, and calculatesperiod_startbased on the period type.Minor observation: The
elsebranch on line 133-134 setsperiod_start = nowfor unknown periods. While unlikely to occur given the controlledget_max_recipients()return values, logging an unexpected period might help with debugging.🔎 Optional: Log unexpected period values
+import logging + +logger = logging.getLogger(__name__) + elif period == "y": period_start = now.replace( month=1, day=1, hour=0, minute=0, second=0, microsecond=0 ) else: + logger.warning("Unexpected quota period '%s' for mailbox %s", period, mailbox.id) period_start = nowsrc/frontend/src/features/ui/components/quota-badge/index.tsx (3)
8-16: Consider importingRecipientQuotafrom generated types.This type duplicates the interface already defined in
src/frontend/src/features/api/gen/models/recipient_quota.ts. Using the generated type ensures consistency and avoids drift if the API schema changes.🔎 Suggested change
+import { RecipientQuota } from "@/features/api/gen/models/recipient_quota"; + -type RecipientQuota = { - period: string; - period_display: string; - period_start: string; - recipient_count: number; - quota_limit: number; - remaining: number; - usage_percentage: number; -};
106-122: Use a semantic element instead of<button>for the tooltip trigger.The
<button>element implies an interactive control, but clicking the badge doesn't perform an action—it only shows a tooltip on hover. This may confuse screen reader users who expect buttons to be actionable.Consider using a
<span>withtabIndex={0}and appropriate ARIA attributes for accessibility.🔎 Suggested change
<Tooltip content={tooltipText} placement="top"> - <button - type="button" + <span + tabIndex={0} + role="status" + aria-label={tooltipText} className={clsx("quota-badge-container", { "quota-badge": !compact, "quota-badge--warning": !compact && isLow && !isCritical, "quota-badge--critical": !compact && isCritical, })} > {indicator} {quota.recipient_count}/{quota.quota_limit} <Icon name="person" type={IconType.OUTLINED} size={IconSize.SMALL} /> {periodShortLabel} {!compact && ` (${quota.usage_percentage}%)`} - </button> + </span> </Tooltip>
125-127: Remove trailing empty lines.There are extra blank lines at the end of the file.
src/frontend/src/features/layouts/components/admin/modal-maildomain-manage-settings/index.tsx (2)
167-169: Error extraction may miss structured validation errors.The error extraction
err?.data?.custom_limits?.[0]assumescustom_limitserrors are an array. If the backend returns nested field errors (e.g.,{ custom_limits: { max_recipients: ["error"] } }), this won't extract them properly. Consider a more robust error parsing approach.🔎 Example of more robust error extraction
const extractError = (err: any): string => { const data = err?.data; if (!data) return t('An error occurred while updating the domain settings.'); // Handle custom_limits nested errors if (data.custom_limits) { if (typeof data.custom_limits === 'string') return data.custom_limits; if (Array.isArray(data.custom_limits)) return data.custom_limits[0]; // Handle nested field errors const fieldErrors = Object.values(data.custom_limits).flat(); if (fieldErrors.length > 0) return String(fieldErrors[0]); } return data.detail || t('An error occurred while updating the domain settings.'); };
85-119: Consider using react-hook-form's built-in validation.The manual validation in
onSubmitduplicates what react-hook-form can handle declaratively via theregisteroptions. This would provide real-time validation feedback and reduce boilerplate.🔎 Example using register validation
<Input {...register('max_recipients_per_message', { validate: (value) => { if (value === '') return true; const num = Number(value); if (isNaN(num) || num < 1 || !Number.isInteger(num)) { return t('Please enter a valid positive integer or leave empty.'); } if (num > MAX_RECIPIENTS_PER_MESSAGE) { return t('The limit cannot exceed the global maximum of {{max}} recipients.', { max: MAX_RECIPIENTS_PER_MESSAGE }); } return true; } })} // ... />src/backend/core/management/commands/redis_quotas_info.py (1)
70-74: Thecountparameter doesn't limit total results.
scan_iter(pattern, count=1000)sets the batch size hint, not a cap on total keys returned. For very large keyspaces, this could be slow or memory-intensive when converting to a list.Consider adding a limit or using a streaming approach if large keyspaces are expected:
🔎 Suggested improvement
try: for label, pattern in patterns: # Use scan_iter with count limit to avoid blocking - keys = list(redis.scan_iter(pattern, count=1000)) - if keys: - self.stdout.write(f"{label}: {len(keys)}") + count = 0 + for _ in redis.scan_iter(pattern, count=1000): + count += 1 + if count >= 100000: # Safety limit + break + if count: + suffix = "+" if count >= 100000 else "" + self.stdout.write(f"{label}: {count}{suffix}")src/backend/core/services/quota.py (1)
265-274: Minor:usage_percentagecould exceed 100 before capping.The calculation
int((current_count / limit) * 100)withmin(100, ...)is correct, but consider handling the edge case wherecurrent_countexceedslimit(e.g., if incremented via external means):🔎 Optional improvement for clarity
current_count = self.get_count(entity_type, entity_id, period) remaining = max(0, limit - current_count) - usage_percentage = int((current_count / limit) * 100) if limit > 0 else 0 + usage_percentage = min(100, int((current_count / limit) * 100)) if limit > 0 else 0 return { "recipient_count": current_count, "quota_limit": limit, "remaining": remaining, - "usage_percentage": min(100, usage_percentage), # Cap at 100% + "usage_percentage": usage_percentage, }src/frontend/src/pages/domain/index.tsx (1)
113-132: Consider extracting the IIFE into a component or simplifying.The immediately-invoked function expression (IIFE) pattern for conditional rendering is unusual in React. This could be simplified to a regular conditional with the variable declared inline, or extracted to a small component.
🔎 Simplified approach
- {editedDomain && editAction === MailDomainEditAction.MANAGE_SETTINGS && (() => { - // Get fresh domain data from the list - 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) - }} - /> - ); - })()} + {editedDomain && editAction === MailDomainEditAction.MANAGE_SETTINGS && ( + <ModalUpdateDomain + key={`${editedDomain.id}-${editedDomain.updated_at}`} + domain={domains.find(d => d.id === editedDomain.id) || editedDomain} + isOpen={true} + onClose={() => { + setEditedDomain(null) + setEditAction(null) + }} + onSuccess={() => { + refetchMaildomains(); + setEditedDomain(null) + setEditAction(null) + }} + /> + )}src/backend/core/models.py (1)
862-876: Thehasattrchecks forcustom_limitsare unnecessary.Since
custom_limitsis a model field withdefault=dict, it will always exist on model instances. Thehasattrcheck is defensive but misleading.🔎 Simplified approach
- # Mailbox-level override - if hasattr(self, "custom_limits") and self.custom_limits: + # Mailbox-level override + if self.custom_limits: value = self.custom_limits.get("max_recipients_per_message") if isinstance(value, int) and value > 0: return min(value, settings.MAX_RECIPIENTS_PER_MESSAGE) - # MailDomain-level override - if ( - self.domain - and hasattr(self.domain, "custom_limits") - and self.domain.custom_limits - ): + # MailDomain-level override + if self.domain and self.domain.custom_limits: value = self.domain.custom_limits.get("max_recipients_per_message")src/backend/core/api/openapi.json (4)
2907-2941: Quota endpoints +RecipientQuotaschema are well-shaped; consider tightening the schema a bit.The three new quota endpoints:
/api/v1.0/mailboxes/{id}/quota//api/v1.0/maildomains/{maildomain_pk}/mailboxes/{id}/quota//api/v1.0/maildomains/{maildomain_pk}/quota/all consistently return
RecipientQuota, use cookie auth, and are tagged under the expected resources. TheRecipientQuotaschema itself looks good and has all the fields a UI needs (counts, remaining, percentage, period info).Two minor schema improvements you may want to consider, to make the contract crisper for generated clients:
- If
periodonly ever takes a small set of values ("d","m", maybe"y"), declaring anenumon that field would document the allowed codes and improve type-safety.- If any of these quota endpoints can ever yield “no quota configured” or “unlimited” semantics, it might be worth specifying how that’s represented (e.g. a very large
quota_limitvs. nullable) and reflecting that withnullable: trueor docs, so clients don’t guess.Nothing blocking here; just potential tightening of the spec.
Also applies to: 3647-3691, 4137-4173, 7442-7486
3066-3202: Domain-levelcustom_limits& settings endpoints: behavior is clear, but write schemas are very loose.Nice separation of concerns:
MailDomainAdminexposes a read-only, strongly-typedcustom_limitsobject (withmax_recipients_per_messageandmax_recipients) plus a newabilities.manage_settingsflag./api/v1.0/maildomains/{maildomain_pk}/now has dedicatedPUT/PATCHoperations that only deal with settings, usingMailDomainAdminUpdateRequest/PatchedMailDomainAdminUpdateRequestfor input andMailDomainAdminUpdatefor output.Two points to consider:
custom_limitswrite shape is “any”
InMailDomainAdminUpdate,MailDomainAdminUpdateRequest, andPatchedMailDomainAdminUpdateRequest,custom_limitshas only adescription, with notypeorproperties. That makes it effectivelyanyfor OpenAPI clients, whereas the read model inMailDomainAdminis a well-defined object. Reusing the same object shape (or a sharedMailDomainCustomLimitsschema) for both read and write would give much better type-safety and codegen support.Update response vs. main representation
The update endpoints returnMailDomainAdminUpdate(id + custom_limits only) instead of the fullMailDomainAdmin. That’s valid if the UI either doesn’t use the response or re-fetches viaGET, but it’s a slight departure from the “return full resource” pattern. If frontends end up wanting the full updated domain after a settings change, switching the response schema toMailDomainAdminmight simplify their flow.None of this is functionally wrong; it’s mostly about keeping the OpenAPI contract as precise and DRY as possible.
Also applies to: 5857-5933, 5935-5961, 7275-7283
3518-3519: Path params for mailbox ids under maildomains could keep theformat: uuidhint.For the nested mailbox admin endpoints under
/api/v1.0/maildomains/{maildomain_pk}/mailboxes/{id}/..., theidpath parameter is now documented as just{"type": "string"}(noformat: "uuid"), while other mailbox-related paths and the schema forMailboxAdmin.idstill useformat: "uuid".This is perfectly valid OpenAPI, but for consistency and better client generation (and request validation, where used), it would be slightly nicer to keep the
format: "uuid"on theseidparams as well.Also applies to: 3561-3562, 3618-3619, 3701-3702
3767-3830: Mailbox-level limits & settings endpoints are well modeled; watch for duplication and untypedcustom_limitson writes.Mailbox-side changes hang together well:
Mailboxnow exposes an effectivemax_recipients_per_message(readOnly, required), which is very useful to clients.- Mailbox abilities gained
manage_settings, mirroring the domain-level capability.MailboxAdminhas a read-only, fully-typedcustom_limitsblock analogous to the domain-level one.- You introduced a dedicated settings endpoint
/api/v1.0/maildomains/{maildomain_pk}/mailboxes/{id}/settings/
usingPatchedMailboxSettingsUpdateRequest(input) andMailboxSettingsUpdate(output), focused purely oncustom_limits.Two design nits to keep in mind:
Multiple write paths for
custom_limits
MailboxAdminUpdateMetadataRequest’smetadatanow also includes acustom_limitsfield. That meanscustom_limitscan potentially be changed both via the new/settings/endpoint and via the generic metadata update. If the intention is to centralize quota changes via/settings/, consider droppingcustom_limitsfrom the metadata schema (or making it readOnly there) to avoid ambiguous or divergent behavior.Untyped
custom_limitsin write schemas
As with the domain-level settings,MailboxSettingsUpdate,PatchedMailboxSettingsUpdateRequest, and themetadata.custom_limitsfield inMailboxAdminUpdateMetadataRequestall leavecustom_limitsuntyped (notypeorproperties). Meanwhile, the read-onlycustom_limitsinMailboxAdminis strongly typed. Reusing that same object shape (or a sharedMailboxCustomLimitsschema) for both read and write would make the contract clearer and help client code avoid shape mismatches.The overall structure is sound; these tweaks would mainly improve API ergonomics and consistency.
Also applies to: 6066-6069, 6117-6120, 6134-6136, 6146-6147, 6353-6379, 6513-6520, 6556-6573, 7306-7314
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (7)
src/frontend/src/features/api/gen/mailboxes/mailboxes.tsis excluded by!**/gen/**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_custom_limits.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/recipient_quota.tsis excluded by!**/gen/**
📒 Files selected for processing (23)
compose.yaml(2 hunks)src/backend/core/api/openapi.json(26 hunks)src/backend/core/api/serializers.py(14 hunks)src/backend/core/api/viewsets/config.py(4 hunks)src/backend/core/api/viewsets/mailbox.py(4 hunks)src/backend/core/api/viewsets/maildomain.py(8 hunks)src/backend/core/api/viewsets/send.py(3 hunks)src/backend/core/apps.py(1 hunks)src/backend/core/enums.py(3 hunks)src/backend/core/management/commands/redis_quotas_info.py(1 hunks)src/backend/core/models.py(13 hunks)src/backend/core/services/quota.py(1 hunks)src/backend/messages/settings.py(1 hunks)src/frontend/src/features/forms/components/message-form/_index.scss(1 hunks)src/frontend/src/features/forms/components/message-form/index.tsx(12 hunks)src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx(8 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/features/ui/components/quota-badge/_index.scss(1 hunks)src/frontend/src/features/ui/components/quota-badge/index.tsx(1 hunks)src/frontend/src/pages/domain/index.tsx(6 hunks)src/frontend/src/styles/main.scss(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- src/frontend/src/features/ui/components/quota-badge/_index.scss
- src/frontend/src/features/providers/config.tsx
- src/frontend/src/styles/main.scss
- src/frontend/src/features/forms/components/message-form/index.tsx
- src/frontend/src/features/layouts/components/admin/mailboxes-view/mailbox-data-grid.tsx
- compose.yaml
- src/backend/messages/settings.py
🧰 Additional context used
📓 Path-based instructions (4)
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/management/commands/redis_quotas_info.pysrc/backend/core/api/viewsets/config.pysrc/backend/core/enums.pysrc/backend/core/services/quota.pysrc/backend/core/models.pysrc/backend/core/api/viewsets/send.pysrc/backend/core/apps.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/mailbox.pysrc/backend/core/api/viewsets/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 (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/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 : Use Django REST Framework viewsets for API endpoints
Applied to files:
src/backend/core/api/viewsets/mailbox.pysrc/backend/core/api/viewsets/maildomain.py
🧬 Code graph analysis (8)
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(73-79)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/ui/components/quota-badge/index.tsx (4)
src/frontend/src/features/api/gen/models/recipient_quota.ts (1)
RecipientQuota(12-27)src/backend/core/api/viewsets/mailbox.py (1)
quota(97-146)src/backend/core/api/viewsets/maildomain.py (2)
quota(152-201)quota(469-518)src/frontend/src/features/api/fetch-api.ts (1)
fetchAPI(12-40)
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(73-79)src/frontend/src/features/api/gen/maildomains/maildomains.ts (1)
useMaildomainsMailboxesSettingsUpdate(2077-2109)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/management/commands/redis_quotas_info.py (3)
src/backend/core/api/viewsets/mailbox.py (1)
quota(97-146)src/backend/core/api/viewsets/maildomain.py (2)
quota(152-201)quota(469-518)src/backend/core/services/quota.py (1)
redis(25-44)
src/backend/core/models.py (3)
src/backend/core/enums.py (3)
QuotaPeriodChoices(126-130)MailDomainAbilities(96-101)MailboxAbilities(104-116)src/backend/core/api/fields.py (1)
JSONField(8-25)src/backend/core/api/serializers.py (1)
get_max_recipients_per_message(294-296)
src/backend/core/api/viewsets/send.py (2)
src/backend/core/models.py (3)
Mailbox(587-928)get_max_recipients(497-530)get_max_recipients(883-928)src/backend/core/services/quota.py (1)
check_and_increment(135-224)
src/backend/core/api/serializers.py (1)
src/backend/core/models.py (4)
parse_max_recipients(52-93)Mailbox(587-928)get_max_recipients_per_message(852-881)MailDomain(288-530)
src/backend/core/api/viewsets/mailbox.py (3)
src/backend/core/models.py (2)
get_max_recipients(497-530)get_max_recipients(883-928)src/backend/core/api/serializers.py (1)
RecipientQuotaSerializer(1668-1701)src/backend/core/services/quota.py (1)
get_status(242-274)
🪛 GitHub Check: CodeQL
src/backend/core/api/serializers.py
[warning] 1058-1058: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 Ruff (0.14.8)
src/backend/core/services/quota.py
28-28: import should be at the top-level of a file
(PLC0415)
30-30: import should be at the top-level of a file
(PLC0415)
src/backend/core/apps.py
21-21: import should be at the top-level of a file
(PLC0415)
🔇 Additional comments (35)
src/frontend/src/features/forms/components/message-form/_index.scss (1)
191-199: LGTM! Clear and consistent modifier classes.The warning and critical state modifiers follow a clean, consistent pattern and use semantic theme colors. The 800/100 color pairings (dark text on light background) should provide good contrast, and the use of theme variables ensures consistency with the broader design system.
src/frontend/src/features/layouts/components/admin/modal-mailbox-manage-settings/index.tsx (4)
1-25: LGTM!Imports and type definitions are well-organized and appropriate for the component's requirements.
27-53: LGTM!The parsing helpers handle edge cases well with sensible defaults. The PERIOD_OPTIONS pattern of translating labels at render time is correct.
55-85: LGTM!Form initialization properly handles existing values from
mailbox.custom_limits, andhandleClosecorrectly resets both form and error state.
114-121: Verify cross-period validation logic.The validation only checks against the global maximum when
periodValue === globalMaxRecipients.period. This means if the global setting is500/d(daily) and a user selects a monthly period, no upper-bound validation is applied. If this is intentional to allow flexibility, consider adding a comment explaining the design decision.src/backend/core/enums.py (2)
101-101: LGTM!The
CAN_MANAGE_SETTINGSability follows the existing pattern for domain abilities.
116-116: LGTM!The
CAN_MANAGE_SETTINGSability for mailboxes is consistent with the domain ability addition.src/backend/core/api/viewsets/send.py (1)
178-180: Quota check placement noted.The quota check is performed before
prepare_outbound_message, which ensures recipients are validated early. Note that a previous reviewer suggested moving this logic tomda/outboundfor better separation of concerns—consider this for future refactoring if the quota enforcement needs to apply to other sending paths.src/backend/core/api/viewsets/mailbox.py (1)
38-47: Good use ofExistsfor N+1 query optimization.Annotating
is_domain_adminusingExistssubquery is an efficient approach to preload domain admin status, avoiding separate queries inget_abilities().src/backend/core/api/viewsets/config.py (3)
6-9: LGTM!The import formatting change is clean and follows Python conventions for multi-line imports.
94-146: LGTM!The OpenAPI schema is well-structured with clear, descriptive documentation for each quota setting. The distinction between maximum limits (cannot be exceeded) and default limits (if no custom limit is set) is clearly communicated.
201-216: LGTM!The settings are correctly exposed in the response. These settings are defined in
src/backend/messages/settings.pyper the AI summary, ensuring they exist at runtime.src/backend/core/management/commands/redis_quotas_info.py (1)
13-90: Well-structured diagnostic command.Good error handling with appropriate severity levels:
- Server info failure terminates early (critical dependency)
- Memory/persistence/keyspace failures emit warnings and continue
- Connection test provides clear pass/fail feedback
The command follows Django management command conventions properly.
src/backend/core/services/quota.py (3)
28-42: Lazy imports are intentional here.The static analysis hint about imports not being at the top-level is a false positive. The lazy-loading pattern is deliberate to avoid circular imports and defer Redis connection until first use.
171-203: Well-designed atomic Lua script for dual-quota enforcement.The Lua script correctly:
- Gets current values with fallback to 0
- Checks both quotas before any increment
- Only increments both if both pass
- Returns structured failure info with remaining capacity
This prevents race conditions and avoids partial updates that would require rollback.
79-87: TTL calculation is correct.The daily TTL computes time until end of day plus a 1-hour buffer, and monthly uses 32 days to cover the longest month plus safety margin. Both approaches ensure keys expire after the period ends.
src/frontend/src/pages/domain/index.tsx (2)
66-73: LGTM!The QuotaBadge column is properly integrated with appropriate sizing and compact mode for the grid display.
208-234: LGTM!The ActionsCell properly gates each button on separate permissions (
CAN_MANAGE_MAILDOMAIN_ACCESSESandCAN_MANAGE_MAILDOMAIN_SETTINGS), and returns null when neither permission is granted.src/backend/core/models.py (3)
52-94: LGTM!The
parse_max_recipientsfunction provides robust validation:
- Validates format ("number/period")
- Ensures positive integer limit
- Validates period against
QuotaPeriodChoices- Clear error messages for each failure case
736-749: LGTM!The
get_abilitiesmethod properly:
- Checks for pre-annotated
is_domain_adminto avoid N+1 queries- Falls back to a direct query if not annotated
- Uses
is_domain_adminto gateCAN_MANAGE_SETTINGSpermission
634-665: Explicitsaveandcleanmethods follow Django conventions.The Mailbox model now properly validates
custom_limitsbefore saving, consistent with theBaseModelpattern used elsewhere.src/backend/core/api/viewsets/maildomain.py (5)
43-43: LGTM! UpdateModelMixin integration is well-structured.The addition of
UpdateModelMixinwith corresponding permission checks (CanManageSettings) and serializer selection (MailDomainAdminUpdateSerializer) follows Django REST Framework best practices and maintains proper separation of concerns.Also applies to: 61-62, 69-70
226-239: LGTM! Permission and serializer logic is properly structured.The
update_settingsaction correctly combines base permissions (IsSuperUser | IsMailDomainAdmin) withCanManageSettings, and uses the appropriate serializer.
244-259: LGTM! Query optimization with annotations is well-implemented.The use of
SubqueryandExistsannotations effectively prevents N+1 queries when checking user roles and domain admin status, improving performance for mailbox listings.
431-461: LGTM! Settings update action follows DRF conventions.The implementation correctly delegates validation to the serializer and maintains separation of concerns. The dedicated endpoint ensures proper permission enforcement at the viewset level.
463-518: LGTM! Mailbox quota action correctly mirrors domain quota pattern.The implementation properly retrieves mailbox-specific quota limits and status. The structure is consistent with the domain quota endpoint for a unified API experience.
src/backend/core/api/serializers.py (7)
221-232: LGTM! max_recipients_per_message exposure is well-integrated.The field correctly exposes the mailbox's maximum recipients per message limit as a read-only value, with proper OpenAPI schema documentation.
Also applies to: 293-296
875-913: LGTM! custom_limits serialization is properly documented.The field correctly exposes domain-level custom limits with comprehensive OpenAPI schema annotations that clearly document the expected format for max_recipients ("number/period").
1063-1075: LGTM! Update serializer is properly scoped.The serializer correctly limits update operations to
custom_limitsonly, inheriting validation fromCustomLimitsUpdateMixinfor consistency.
1077-1086: LGTM! Mailbox settings serializer follows consistent pattern.The serializer mirrors the domain update serializer structure, ensuring consistent behavior and validation across both entity types.
1117-1145: LGTM! Mailbox custom_limits exposure is well-documented.The field properly exposes mailbox-level custom limits as read-only, with a helpful comment directing users to the
/settings/endpoint for updates, maintaining clear API boundaries.Also applies to: 1171-1171
1318-1340: LGTM! Creation serializer correctly excludes custom_limits.The exclusion of
custom_limitsfrom the creation response is a sound design decision, properly documented and implemented. This enforces the use of the dedicated/settings/endpoint for quota configuration.
1668-1701: LGTM! RecipientQuotaSerializer provides comprehensive quota status.The serializer effectively presents all necessary quota information with clear field documentation, supporting both domain and mailbox quota endpoints consistently.
src/backend/core/api/openapi.json (2)
225-255: Config quota keys look coherent; just confirm they are always populated and non-null.The new public settings (
MAX_RECIPIENTS_PER_MESSAGE,MAX_DEFAULT_RECIPIENTS_PER_MESSAGE,MAX_RECIPIENTS_FOR_DOMAIN,MAX_DEFAULT_RECIPIENTS_FOR_DOMAIN,MAX_RECIPIENTS_FOR_MAILBOX,MAX_DEFAULT_RECIPIENTS_FOR_MAILBOX) and their descriptions/readOnly flags are consistent with the quota model and nicely distinguish “hard” vs “default” limits.They are also all marked as required in the
/api/v1.0/config/schema. That’s fine as long as backend config always provides concrete (non-null) values for all six keys in every environment; otherwise the OpenAPI contract will diverge from reality. Worth double-checking the config service/env defaults for that.Also applies to: 268-274
5857-5874: Abilities & required fields for settings and limits: contract looks good; just verify serializers match.You’ve:
- Added
custom_limitstoMailDomainAdminandMailboxAdminas readOnly, nullable-but-required objects.- Added
manage_settingsto bothMailDomainAdmin.abilitiesandMailbox.abilities, and marked it as required.- Added
max_recipients_per_messagetoMailboxas a required, non-null, readOnly integer.This is a clean contract and will be very helpful to UIs, but it does require that the corresponding serializers always emit:
custom_limits(even ifnull) on every domain/mailbox admin response, and- a non-null
max_recipients_per_messageandabilities.manage_settingson every mailbox response.Given these are now in
required, if any of those fields can be missing orNonein practice, codegen clients may choke. It’s worth quickly confirming that the serializers forMailDomainAdmin,MailboxAdmin, andMailboxare wired to always provide these fields with the documented nullability.Also applies to: 5906-5910, 5919-5921, 5928-5929, 6066-6069, 6146-6147
| # Import checks to register them | ||
| from . import checks # noqa: F401 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Examine the apps.py file to verify the code at lines 20-21 and line 32
echo "=== Examining src/backend/core/apps.py ==="
if [ -f "src/backend/core/apps.py" ]; then
echo "File exists. Reading content:"
cat -n "src/backend/core/apps.py"
echo ""
echo "=== Checking for checks.py module ==="
if [ -f "src/backend/core/checks.py" ]; then
echo "checks.py exists. Content:"
head -50 "src/backend/core/checks.py"
else
echo "checks.py not found"
fi
else
echo "File src/backend/core/apps.py does not exist"
fiRepository: suitenumerique/messages
Length of output: 1411
🏁 Script executed:
#!/bin/bash
# Search entire codebase for checks.py
echo "=== Searching for checks.py in entire repository ==="
fd -t f "checks\.py" .
echo ""
echo "=== Searching for 'checks' module references ==="
rg "from \. import checks|import.*checks" --type py -A 2 -B 2
echo ""
echo "=== Listing all files in src/backend/core/ directory ==="
ls -la src/backend/core/Repository: suitenumerique/messages
Length of output: 1986
Create src/backend/core/checks.py module—import statement will fail at runtime.
The import statement at line 21 references a checks module that does not exist in the codebase. When Django initializes the app and calls AppConfig.ready(), this will raise ModuleNotFoundError. The import statement tries to access a module that cannot be found.
The comment correctly identifies that checks should be registered in a file that's loaded when your application is loaded; for example, in the AppConfig.ready() method, but the referenced module must actually exist before this code can work.
Create src/backend/core/checks.py with the necessary Django system check registrations.
🧰 Tools
🪛 Ruff (0.14.8)
21-21: import should be at the top-level of a file
(PLC0415)
| const { data: quota, isLoading, isError } = useQuery<RecipientQuota>({ | ||
| queryKey: [`${entityType}-quota`, entityId, parentDomainId], | ||
| queryFn: async () => { | ||
| const response = await fetchAPI<{ data: RecipientQuota }>(apiUrl); | ||
| return response.data; | ||
| }, | ||
| enabled: !!entityId, | ||
| staleTime: 5000, // Cache for 5 seconds | ||
| retry: 1, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine fetch-api.ts
fd -t f "fetch-api\." src/frontend --exec head -50 {}Repository: suitenumerique/messages
Length of output: 1388
🏁 Script executed:
# Find backend quota endpoint
find src/backend -type f -name "*.py" | xargs grep -l "quota" | head -5Repository: suitenumerique/messages
Length of output: 283
🏁 Script executed:
# Search for RecipientQuota type definition in frontend
fd -t f "\.(tsx?|ts)$" src/frontend | xargs grep -l "RecipientQuota" | head -5Repository: suitenumerique/messages
Length of output: 372
🏁 Script executed:
# Check the quota-related serializer and endpoint
cat src/backend/core/api/serializers.py | grep -A 20 "RecipientQuota"Repository: suitenumerique/messages
Length of output: 938
🏁 Script executed:
# Search for quota viewset or endpoint
rg -n "quota" src/backend/core/api/viewsets/ -A 10 | head -60Repository: suitenumerique/messages
Length of output: 4620
🏁 Script executed:
# Check the RecipientQuota type definition
cat src/frontend/src/features/api/gen/models/recipient_quota.tsRepository: suitenumerique/messages
Length of output: 832
🏁 Script executed:
# Find the quota endpoint view
rg -n "quota" src/backend/core/api/viewsets/mailbox.py -B 2 -A 15 | head -50Repository: suitenumerique/messages
Length of output: 1877
🏁 Script executed:
# Check how the response is returned
rg -n "RecipientQuotaSerializer" src/backend/core/api/viewsets/ -B 2 -A 5Repository: suitenumerique/messages
Length of output: 3603
🏁 Script executed:
# Check the complete quota function in mailbox viewset
sed -n '97,146p' src/backend/core/api/viewsets/mailbox.pyRepository: suitenumerique/messages
Length of output: 1949
🏁 Script executed:
# Check how the component calls fetchAPI
sed -n '51,60p' src/frontend/src/features/ui/components/quota-badge/index.tsxRepository: suitenumerique/messages
Length of output: 468
🏁 Script executed:
# Check if there are other similar uses of fetchAPI in the codebase to understand the pattern
rg -n "fetchAPI<" src/frontend --type=tsx -B 1 -A 1 | head -40Repository: suitenumerique/messages
Length of output: 94
🏁 Script executed:
# Check if there are other similar uses of fetchAPI in the codebase
rg -n "fetchAPI<" src/frontend -A 2 -B 1 | head -60Repository: suitenumerique/messages
Length of output: 5294
🏁 Script executed:
# Verify the exact structure by checking what MultipartInitResponse looks like
rg -n "MultipartInitResponse" src/frontend --type=ts -B 1 -A 5 | head -30Repository: suitenumerique/messages
Length of output: 1642
Fix the fetchAPI generic type to match the actual return structure.
The fetchAPI function returns { status, data, headers }, but the current generic specifies <{ data: RecipientQuota }>. Change to <{ status: number, data: RecipientQuota }> or extract the entire response structure properly. This aligns with other uses in the codebase (e.g., MultipartInitResponse) and avoids misleading type annotations.
🤖 Prompt for AI Agents
In src/frontend/src/features/ui/components/quota-badge/index.tsx around lines 51
to 60, the fetchAPI generic is incorrect: it currently uses <{ data:
RecipientQuota }> but fetchAPI actually returns an object like { status, data,
headers }. Update the generic to <{ status: number; data: RecipientQuota;
headers?: Record<string,string> }> (or the exact response type used across the
codebase) and return response.data as before so the typing matches other usages
and avoids misleading annotations.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.