Add ProConnect card #14
Conversation
WalkthroughAdds service dependency and ProConnect activation logic, exposes operator/service config and organization mail-domain/status through APIs, updates admin/webhooks, adds a migration for service requirements, implements new frontend service UI (including ProConnect flows), and adds tests for the new behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend as Browser UI
participant API as Backend API
participant DB as Database
participant Webhook as Webhook Sender
User->>Frontend: Open organization services page
Frontend->>API: GET operator, organization, services
API->>DB: fetch operator, org, services, subscriptions
DB-->>API: return data (includes config, mail_domain, mail_domain_status, can_activate)
API-->>Frontend: serialized payload
User->>Frontend: Select IDP or toggle activation
Frontend->>API: PATCH subscription (is_active / metadata.idp_id)
API->>API: validate required_services & ProConnect prerequisites
alt validation fails
API-->>Frontend: 400 with errors
else
API->>DB: persist subscription change
DB-->>API: saved
API->>Webhook: send_webhooks (include idp_name, is_active, org mail_domain)
API-->>Frontend: 200/201 success
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
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 |
NathanVss
force-pushed
the
feat/pc-service
branch
from
38ebcef to
e574dcd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
src/frontend/package.json (1)
24-24: Inconsistent version pinning for react-hook-form.Most production dependencies use exact versions (e.g.,
"next": "15.5.4","react": "19.1.0"), butreact-hook-formuses a caret range (^7.67.0). Consider pinning to an exact version for consistency and reproducible builds.- "react-hook-form": "^7.67.0", + "react-hook-form": "7.67.0",src/backend/core/models.py (1)
497-502: Consider defensive handling for email parsing.While
EmailFieldprovides validation, ifadresse_messagerieis somehow malformed (e.g., no@), this will raise anIndexError. The risk is low given Django's validation, but defensive coding would be safer.@property def adresse_messagerie_domain(self): """Get the mail domain for the organization.""" if not self.adresse_messagerie: return None - return self.adresse_messagerie.split("@")[1] + parts = self.adresse_messagerie.split("@") + return parts[1] if len(parts) > 1 else Nonesrc/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx (2)
353-355: Clarify the purpose of the no-oponSubmit.The
onSubmitfunction is memoized but returnsvoid 0(effectivelyundefined). The actual submission logic appears to be incanActivateSubscription. Consider adding a comment or removing if unused.
240-242: Optimistic UI update may cause flickering on error.
setChecked(true)is called afteronChangeSubscription, but the mutation is asynchronous. If the mutation fails, the UI will briefly show the checked state before reverting. Consider settingcheckedonly on success.if (decision === "yes") { + setChecked(true); onChangeSubscription( { is_active: true }, { + onSuccess: () => { + // State already updated optimistically + }, onError: () => { setChecked(false); }, } ); - setChecked(true); }This is already partially implemented, but the current placement ensures the optimistic update happens first, which is correct. The suggestion is to make the intent clearer with a comment.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
src/backend/core/admin.py(2 hunks)src/backend/core/api/serializers.py(6 hunks)src/backend/core/api/viewsets/service.py(2 hunks)src/backend/core/migrations/0009_service_required_services.py(1 hunks)src/backend/core/models.py(5 hunks)src/frontend/package.json(1 hunks)src/frontend/src/features/api/Repository.ts(3 hunks)src/frontend/src/features/i18n/translations.json(2 hunks)src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx(7 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Use Django’s ORM for database interactions; avoid raw SQL unless strictly necessary for performance
Use Django’s built-in user model and authentication framework for user management
Log expected and unexpected application actions with appropriate log levels
Respect log level semantics: DEBUG (diagnostics), INFO (normal events), WARN (unexpected but continuing), ERROR (functional failure)
Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Files:
src/backend/core/admin.pysrc/backend/core/migrations/0009_service_required_services.pysrc/backend/core/api/viewsets/service.pysrc/backend/core/models.pysrc/backend/core/api/serializers.py
src/backend/**/{models,forms,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
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Define appropriate database indexes and optimize query patterns via model Meta and QuerySets
Files:
src/backend/core/models.py
src/backend/**/serializers.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use DRF serializers for JSON responses
Files:
src/backend/core/api/serializers.py
🧠 Learnings (1)
📚 Learning: 2025-10-03T23:40:57.889Z
Learnt from: CR
Repo: suitenumerique/st-deploycenter PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-10-03T23:40:57.889Z
Learning: Applies to src/backend/**/serializers.py : Use DRF serializers for JSON responses
Applied to files:
src/backend/core/api/serializers.py
🧬 Code graph analysis (4)
src/frontend/src/features/api/Repository.ts (1)
src/backend/core/models.py (4)
Operator(187-281)MailDomainStatus(334-337)Organization(328-548)Service(639-765)
src/backend/core/api/viewsets/service.py (2)
src/backend/core/models.py (2)
Organization(328-548)ServiceSubscription(768-821)src/frontend/src/features/api/Repository.ts (2)
Organization(33-45)ServiceSubscription(47-54)
src/backend/core/models.py (1)
src/frontend/src/features/api/Repository.ts (1)
Organization(33-45)
src/backend/core/api/serializers.py (1)
src/backend/core/models.py (3)
mail_domain(513-516)get_mail_domain_status(518-545)can_activate(756-765)
🪛 Ruff (0.14.7)
src/backend/core/models.py
509-509: print found
Remove print
(T201)
🔇 Additional comments (15)
src/frontend/src/features/i18n/translations.json (1)
29-29: LGTM!The new
cannot_activatetranslations are well-crafted and consistent between English and French locales.Also applies to: 77-77
src/backend/core/api/viewsets/service.py (1)
196-208: LGTM!The metadata handling in the upsert flow is correctly implemented - extracting from validated data with an appropriate default and passing it to the subscription creation.
src/backend/core/migrations/0009_service_required_services.py (1)
1-18: LGTM!The migration correctly adds the
required_servicesManyToManyField to the Service model with appropriate metadata. This aligns with the model definition for service dependency management.src/backend/core/admin.py (2)
267-272: LGTM!Good separation of the
configfield into its own fieldset forOperatorAdmin. This improves admin UI organization.
400-401: LGTM!Adding
required_servicesto the ServiceAdmin fieldset properly exposes the new ManyToMany relationship for managing service dependencies in the admin interface.src/frontend/src/features/api/Repository.ts (2)
27-31: LGTM!The
MailDomainStatusenum correctly mirrors the backendOrganization.MailDomainStatusenum values.
56-57: LGTM!The
SERVICE_TYPE_PROCONNECTconstant and newServicetype fields (type,can_activate) properly extend the frontend types to support the ProConnect activation flow.Also applies to: 63-63, 70-70
src/backend/core/models.py (2)
334-337: LGTM!The
MailDomainStatusenum usingStrEnumis clean and provides clear status values that serialize naturally to JSON.
728-734: LGTM!The self-referential
ManyToManyFieldwithsymmetrical=Falseis correctly set up for modeling service dependencies.src/backend/core/api/serializers.py (2)
378-384: LGTM!The
to_representationoverride correctly enriches the organization response with mail domain data from the model's new utility method.
341-343: LGTM!The list comprehension approach for
read_only_fieldsis clear and maintainable.src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx (4)
174-176: Placeholder implementation:canActivateSubscriptionalways returnstrue.This hook's
canActivateSubscriptionalways resolves totrue, making it a no-op. The actual validation logic is only inProConnectServiceBlock. If this is intentional for regular services, consider documenting it. Otherwise, this may need implementation.
48-62: LGTM!The sorting logic correctly prioritizes ProConnect services first, then sorts by display priority in descending order. The null-coalescing to
-Infinityensures services without priority are placed at the end.
137-145: LGTM!Clean dispatcher pattern that routes to the appropriate component based on service type.
383-390: LGTM!Creative use of
handleSubmitto validate the form before activation. The Promise wrapper cleanly integrates react-hook-form validation with the async activation flow.
src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx
Outdated
Show resolved
Hide resolved
src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/backend/core/models.py (2)
505-510: Remove debugThis code has two issues already flagged in previous reviews:
- Debug
- Unsafe URL parsing with
split("//")[1]can raiseIndexErrorfor malformed URLsBased on coding guidelines, use proper logging and robust URL parsing:
+from urllib.parse import urlparse + ... def site_internet_domain(self): """Get the website domain for the organization.""" if not self.site_internet: return None - print(self.site_internet) - return self.site_internet.split("//")[1].replace("/", "") + try: + parsed = urlparse(self.site_internet) + return parsed.netloc or None + except Exception as e: + logger.warning( + "Invalid URL format for organization %s: %s", + self.id, + self.site_internet + ) + return None
757-766: Logic bug:can_activatereturnsTrueif ANY required service is active instead of ALL.This critical issue was already flagged in previous reviews. Using
.exists()returnsTrueif at least one required service has an active subscription. The method must verify that all required services are active.Apply this diff:
def can_activate(self, organization: Organization): """ Check if the service can be activated for the given organization. """ - if not self.required_services.count(): + required_count = self.required_services.count() + if not required_count: return True - return self.required_services.all().filter( + active_required_count = self.required_services.filter( subscriptions__organization=organization, subscriptions__is_active=True - ).exists() + ).distinct().count() + return active_required_count == required_countsrc/backend/core/api/viewsets/service.py (1)
101-104: UnhandledDoesNotExistexception for invalid organization ID.This issue was already flagged in previous reviews. The direct call to
Organization.objects.get()will raiseOrganization.DoesNotExistfor invalid IDs, causing a 500 error instead of a proper 404 response.Apply this diff:
+from django.shortcuts import get_object_or_404 + def get_serializer_context(self): - """Add operator_id and organization to serializer context.""" + """Add operator_id and organization to serializer context.""" context = super().get_serializer_context() context["operator_id"] = self.kwargs["operator_id"] - organization = models.Organization.objects.get( - id=self.kwargs["organization_id"] + organization = get_object_or_404( + models.Organization, id=self.kwargs["organization_id"] ) context["organization"] = organization return contextsrc/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx (1)
360-362: Add null check foroperator.configbefore accessingidps.This issue was already flagged in previous reviews.
operator?.config.idpsassumesconfigexists whenoperatorexists. Ifconfigisnullorundefined, this will throw a runtime error.Apply this diff:
const getIdp = (idp_id: string) => { - return operator?.config.idps?.find((idp) => idp.id == idp_id); + return operator?.config?.idps?.find((idp) => idp.id === idp_id); };Note: Also changed
==to===for strict equality comparison.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/backend/core/api/viewsets/service.py(2 hunks)src/backend/core/models.py(6 hunks)src/backend/core/webhooks.py(2 hunks)src/frontend/src/features/i18n/translations.json(2 hunks)src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx(7 hunks)src/frontend/src/pages/operators/[operator_id]/organizations/index.scss(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Use Django’s ORM for database interactions; avoid raw SQL unless strictly necessary for performance
Use Django’s built-in user model and authentication framework for user management
Log expected and unexpected application actions with appropriate log levels
Respect log level semantics: DEBUG (diagnostics), INFO (normal events), WARN (unexpected but continuing), ERROR (functional failure)
Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Files:
src/backend/core/webhooks.pysrc/backend/core/api/viewsets/service.pysrc/backend/core/models.py
src/backend/**/{models,forms,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
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Define appropriate database indexes and optimize query patterns via model Meta and QuerySets
Files:
src/backend/core/models.py
🧬 Code graph analysis (3)
src/backend/core/webhooks.py (1)
src/backend/core/models.py (2)
idp_name(884-895)mail_domain(513-516)
src/backend/core/api/viewsets/service.py (1)
src/backend/core/models.py (2)
Organization(328-548)ServiceSubscription(769-898)
src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx (3)
src/backend/core/models.py (4)
Service(639-766)Organization(328-548)ServiceSubscription(769-898)MailDomainStatus(334-337)src/frontend/src/hooks/useQueries.tsx (1)
useMutationUpdateOrganizationServiceSubscription(87-117)src/frontend/src/features/layouts/components/GlobalLayout.tsx (1)
useOperatorContext(57-63)
🪛 Ruff (0.14.7)
src/backend/core/models.py
509-509: print found
Remove print
(T201)
🔇 Additional comments (16)
src/frontend/src/features/i18n/translations.json (1)
29-50: LGTM! Well-structured translation additions.The new translation keys for ProConnect (cannot_activate, idp, mail_domain) are complete and consistent across both English and French locales. The nested structure follows existing patterns.
Also applies to: 95-116
src/backend/core/models.py (5)
334-337: LGTM! Proper enum definition.The MailDomainStatus enum is well-defined using StrEnum with clear status values.
518-545: LGTM! Sound RPNT validation logic.The mail domain status computation correctly validates RPNT criteria using set operations. Note that this method depends on
adresse_messagerie_domainandsite_internet_domain(), which have separate issues that need fixing.
728-735: LGTM! Proper service dependency relationship.The
required_servicesManyToManyField is correctly configured withsymmetrical=Falsefor directed dependency relationships.
822-856: LGTM! Thorough validation of required services.The validation correctly ensures all required services have active subscriptions before allowing activation, with clear error messages listing missing services.
858-873: LGTM! Appropriate ProConnect-specific validation.The validation correctly enforces mail domain and IDP requirements for ProConnect subscriptions before activation.
src/backend/core/webhooks.py (1)
224-224: LGTM! Webhook context enrichment aligns with model changes.The addition of
subscription_idp_nameandorganization_mail_domainto the webhook payload appropriately exposes the new ProConnect-related data for external systems.Also applies to: 234-234
src/backend/core/api/viewsets/service.py (1)
198-205: LGTM! Explicit metadata handling in subscription creation.The explicit extraction of
metadatafrom validated data with a default empty dict is appropriate for the upsert behavior.src/frontend/src/pages/operators/[operator_id]/organizations/index.scss (1)
189-253: LGTM! Well-structured styling additions for ProConnect UI.The new CSS classes for service attributes, warnings, and modals are well-organized with appropriate visual hierarchy and interactive states.
src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx (7)
57-71: LGTM! Efficient ProConnect prioritization logic.The memoized sorting correctly places ProConnect services first, then orders by display priority.
130-140: LGTM! Appropriate loading states.The rendering guard correctly waits for both organization and operator data before rendering services.
156-195: LGTM! Well-structured service block hook.The hook appropriately encapsulates subscription state and mutation logic, with extensibility for custom activation validation.
374-488: LGTM! Robust form validation for ProConnect activation.The form validation correctly enforces IDP and mail domain requirements before allowing activation, with clear error messaging.
490-501: LGTM! Appropriate mail domain status warnings.The conditional warnings for mail domain status use properly internationalized translation keys that are defined in the translations file.
381-389: LGTM! Proper IDP pre-configuration flow.The
saveIdpChangefunction correctly allows IDP selection before subscription activation by creating an inactive subscription if needed.
508-537: LGTM! Clean and reusable attribute component.The
ServiceAttributecomponent is well-structured with appropriate support for both static display and interactive editing.
src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
src/backend/core/api/serializers.py (2)
287-290: Re‑exposingOperator.configrisks leaking sensitive keys; either remove or filter it.Including
configinOperatorSerializeragain will expose all operator configuration, which previously contained values likeexternal_management_api_key. This is a direct security/privacy risk; either removeconfigfromfieldsor implement a filtered representation that strips secrets (keys containing e.g.key,secret,token,credential). As per coding guidelines, sensitive information should not be exposed via APIs.
419-423: Guard against missingorganizationin serializer context to avoidKeyError.Accessing
self.context["organization"]directly will raiseKeyErrorif the serializer is instantiated without that key (e.g., reuse in another view), leading to a 500. Either validate presence explicitly or fall back gracefully.For example:
def to_representation(self, instance): data = super().to_representation(instance) organization = self.context.get("organization") if organization is None: # Either be strict: # raise ValueError("OrganizationServiceSerializer requires 'organization' in context") data["can_activate"] = False else: data["can_activate"] = instance.can_activate(organization) return dataThis keeps the API robust while still exposing the flag when context is correctly set.
src/frontend/src/features/api/Repository.ts (1)
11-24: MakeOperator.configandidpsnullable/optional to match backend JSONField.Backend allows
Operator.configto benulland not necessarily containidps. Typing it as always-present{ idps: OperatorIdp[] }makes downstream code likeoperator?.config.idpsunsafe whenconfigisnullor missingidps, and also hides thatidpsmay be absent.Consider aligning the type with the backend shape:
export type Operator = { id: string; name: string; url: string; scope: string; is_active: boolean; - config: { - idps: OperatorIdp[]; - }; + config?: { + idps?: OperatorIdp[]; + } | null; };Then, at call sites, prefer
operator.config?.idps ?? []oroperator.config?.idps?.find(...)so accesses stay safe when config or idps is missing.src/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx (1)
41-49: Guard againstoperator.configbeing null and use strict equality for IDP lookup.
Operator.configis a nullable JSON field on the backend, so usingoperator?.config.idpscan still throw whenconfigisnull, and==risks silent coercions.Recommend:
const getIdp = (idp_id: string) => { - return operator?.config.idps?.find((idp) => idp.id == idp_id); + return operator?.config?.idps?.find((idp) => idp.id === idp_id); }; ... - options={ - operator?.config.idps?.map((idp) => ({ - label: idp.name, - value: idp.id, - })) ?? [] - } + options={ + operator?.config?.idps?.map((idp) => ({ + label: idp.name, + value: idp.id, + })) ?? [] + }This matches the nullable
configshape and avoids unexpected matches from loose equality.Also applies to: 120-133
🧹 Nitpick comments (6)
src/backend/core/api/serializers.py (2)
305-320: Confirm thatService.configis safe to expose as‑is (no secrets embedded).Adding
configtoServiceSerializermeans all service configuration becomes part of the public API response for this endpoint. IfService.configcan contain credentials, tokens, or other sensitive/internal‑only values (now or in the future), implement a filtered representation similar to what is needed forOperator.config, or split public vs private config.
339-344: Computedread_only_fieldscleanly makes onlymetadatawritable.Deriving
read_only_fieldsfromfieldsand excludingmetadataensures all other fields (created_at,updated_at,is_active,service,operator) are read‑only in this composite serializer. This matches the intended “only metadata is mutable” behavior and stays resilient iffieldschanges.You could make intent clearer by spelling out the list (even if slightly more verbose) to avoid surprises if
fieldsis refactored later.src/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx (2)
68-76: Preserve existing subscription metadata when changing IDP (if other keys exist).
saveIdpChangecurrently sendsmetadata: { idp_id: idp?.id }, overwriting any other metadata fields the backend might have stored for this subscription.If metadata is expected to hold additional information, consider shallow-merging:
const saveIdpChange = (idp?: OperatorIdp) => { - const data: Partial<ServiceSubscription> = { - metadata: { idp_id: idp?.id }, - }; + const previousMetadata = props.service.subscription?.metadata ?? {}; + const data: Partial<ServiceSubscription> = { + metadata: { ...previousMetadata, idp_id: idp?.id }, + };The rest of the function can stay as is.
80-92: Minor UX tweaks: avoid incidental form submit in modal and harden external links.Two small refinements:
- The close button in the IDP modal is declared as
type="submit", which will submit the surrounding form even though it only closes the modal:- <Button type="submit" onClick={idpModal.close}> + <Button type="button" onClick={idpModal.close}>
- For links that open in a new tab, add
rel="noreferrer noopener"for safety:- <a target="_blank" href={props.service.config?.help_center_url}> + <a + target="_blank" + rel="noreferrer noopener" + href={props.service.config?.help_center_url} + >(You may want to mirror the same pattern in
ServiceBlock’s main “goto” link.)Also applies to: 94-115, 193-205
src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx (1)
108-116: Loading guard is safe, consider future error handling.
isServicesLoading || !organization || !operatorensures the dispatcher is only rendered when all required data and context are present, which protectsuseOperatorContextconsumers.If
useOrganization/useOrganizationServicescan end in an error state (404/500), you may eventually want to differentiate “still loading” from “failed to load” instead of showing a perpetual spinner.src/frontend/src/features/ui/components/service/ServiceBlock.tsx (1)
171-183: Addrel="noreferrer noopener"to external “goto” links.Since the “goto” link opens services in a new tab, it’s safer to include
relattributes:- <a target="_blank" href={service.url}> + <a target="_blank" rel="noreferrer noopener" href={service.url}>Consider mirroring this pattern in other external links (e.g., ProConnect help center).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/backend/core/api/serializers.py(7 hunks)src/backend/core/webhooks.py(2 hunks)src/frontend/src/features/api/Repository.ts(3 hunks)src/frontend/src/features/i18n/translations.json(2 hunks)src/frontend/src/features/ui/components/service/ServiceAttribute.tsx(1 hunks)src/frontend/src/features/ui/components/service/ServiceBlock.tsx(1 hunks)src/frontend/src/features/ui/components/service/ServiceBlockDispatcher.tsx(1 hunks)src/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx(1 hunks)src/frontend/src/features/ui/components/service/implementations/RegularServiceBlock.tsx(1 hunks)src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/frontend/src/features/ui/components/service/ServiceAttribute.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend/core/webhooks.py
🧰 Additional context used
📓 Path-based instructions (2)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Use Django’s ORM for database interactions; avoid raw SQL unless strictly necessary for performance
Use Django’s built-in user model and authentication framework for user management
Log expected and unexpected application actions with appropriate log levels
Respect log level semantics: DEBUG (diagnostics), INFO (normal events), WARN (unexpected but continuing), ERROR (functional failure)
Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Files:
src/backend/core/api/serializers.py
src/backend/**/serializers.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use DRF serializers for JSON responses
Files:
src/backend/core/api/serializers.py
🧠 Learnings (2)
📚 Learning: 2025-10-03T23:40:57.889Z
Learnt from: CR
Repo: suitenumerique/st-deploycenter PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-10-03T23:40:57.889Z
Learning: Applies to src/backend/**/serializers.py : Use DRF serializers for JSON responses
Applied to files:
src/backend/core/api/serializers.py
📚 Learning: 2025-10-03T23:40:57.889Z
Learnt from: CR
Repo: suitenumerique/st-deploycenter PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-10-03T23:40:57.889Z
Learning: Applies to src/backend/**/*.py : Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Applied to files:
src/backend/core/api/serializers.py
🧬 Code graph analysis (7)
src/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx (6)
src/frontend/src/features/api/Repository.ts (4)
Service(58-74)Organization(33-45)OperatorIdp(11-14)ServiceSubscription(47-54)src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx (1)
Organization(16-122)src/frontend/src/features/layouts/components/GlobalLayout.tsx (1)
useOperatorContext(57-63)src/frontend/src/features/ui/components/service/ServiceBlock.tsx (2)
useServiceBlock(11-53)ServiceBlock(55-189)src/frontend/src/features/ui/components/service/ServiceAttribute.tsx (1)
ServiceAttribute(1-30)src/backend/core/models.py (1)
MailDomainStatus(334-337)
src/frontend/src/features/ui/components/service/implementations/RegularServiceBlock.tsx (2)
src/frontend/src/features/api/Repository.ts (2)
Service(58-74)Organization(33-45)src/frontend/src/features/ui/components/service/ServiceBlock.tsx (2)
useServiceBlock(11-53)ServiceBlock(55-189)
src/frontend/src/features/ui/components/service/ServiceBlock.tsx (3)
src/frontend/src/features/api/Repository.ts (4)
Service(58-74)Organization(33-45)updateOrganizationServiceSubscription(159-174)ServiceSubscription(47-54)src/frontend/src/hooks/useQueries.tsx (1)
useMutationUpdateOrganizationServiceSubscription(87-117)src/frontend/src/features/layouts/components/GlobalLayout.tsx (1)
useOperatorContext(57-63)
src/frontend/src/features/ui/components/service/ServiceBlockDispatcher.tsx (3)
src/frontend/src/features/api/Repository.ts (3)
Service(58-74)Organization(33-45)SERVICE_TYPE_PROCONNECT(56-56)src/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx (1)
ProConnectServiceBlock(41-211)src/frontend/src/features/ui/components/service/implementations/RegularServiceBlock.tsx (1)
RegularServiceBlock(7-13)
src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx (2)
src/frontend/src/features/api/Repository.ts (1)
SERVICE_TYPE_PROCONNECT(56-56)src/frontend/src/features/ui/components/service/ServiceBlockDispatcher.tsx (1)
ServiceBlockDispatcher(9-17)
src/frontend/src/features/api/Repository.ts (1)
src/backend/core/models.py (5)
Operator(187-281)MailDomainStatus(334-337)Organization(328-548)Service(639-766)ServiceSubscription(769-898)
src/backend/core/api/serializers.py (1)
src/backend/core/models.py (3)
mail_domain(513-516)get_mail_domain_status(518-545)can_activate(757-766)
🔇 Additional comments (8)
src/backend/core/api/serializers.py (3)
326-330: Writablemetadata/is_activehere looks intentional and consistent.
ServiceSubscriptionSerializernow exposesmetadataandis_activeas writable while keeping timestamps read‑only, which matches common patterns for subscription state + freeform metadata. No correctness issues spotted.
379-385: Appending mail domain info into_representationis straightforward and correct.Using
get_mail_domain_status()to injectmail_domainandmail_domain_statusinto the serialized output aligns with the model helpers and avoids extra DB hits. No functional or privacy issues evident.
393-397: Keepingcan_activateout ofMeta.fieldsbut adding it into_representationis a good choice.Not declaring
can_activateinfieldswhile still adding it at render time ensures it’s output‑only and can’t be posted/updated by clients, which is appropriate for a computed capability flag.src/frontend/src/features/api/Repository.ts (1)
27-45: Mail-domain and ProConnect typings look consistent with backend.
MailDomainStatusvalues,Organization.mail_domain/mail_domain_status,SERVICE_TYPE_PROCONNECT, andService.type/can_activate/config.help_center_urlall line up cleanly with the backend model descriptions and downstream usage in the UI (ProConnect flows, tooltips, help center link). No changes needed here.Also applies to: 56-73
src/frontend/src/features/ui/components/service/implementations/RegularServiceBlock.tsx (1)
1-13: Straightforward wrapper aroundServiceBlocklooks good.Nice separation: regular services reuse
useServiceBlock+ServiceBlockwithout adding logic, leaving specialization to the ProConnect implementation.src/frontend/src/features/ui/components/service/ServiceBlockDispatcher.tsx (1)
1-17: Type-based dispatch between ProConnect and regular services is clear and minimal.The dispatcher cleanly centralizes the
SERVICE_TYPE_PROCONNECTcheck and keeps both implementations’ props in sync. Looks good.src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx (1)
35-49: Service ordering (ProConnect first, then by priority) is correctly implemented.Using
SERVICE_TYPE_PROCONNECTchecks followed bydisplay_priority ?? -Infinityyields the intended order (ProConnect at the top, then descending priority, with undefined priorities last). Copyingservices.resultsbefore sorting preserves the original data.src/frontend/src/features/ui/components/service/ServiceBlock.tsx (1)
11-53: Hook abstraction for service subscription state looks solid.Centralizing subscription state, mutation wiring, and
canActivateSubscriptioninuseServiceBlockkeeps the UI component lean and reusable. Syncingcheckedonservice.subscriptionchanges ensures React Query updates propagate correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/backend/core/models.py (3)
498-503: Handle malformed email addresses to prevent IndexError.The previous review flagged this: if
adresse_messageriedoesn't contain@(e.g., from data import or corruption),split("@")[1]will raiseIndexError. While Django'sEmailFieldvalidates at save time, defensive coding requires handling edge cases.Apply this diff to add robust error handling:
@property def adresse_messagerie_domain(self): """Get the mail domain for the organization.""" if not self.adresse_messagerie: return None - return self.adresse_messagerie.split("@")[1] + try: + return self.adresse_messagerie.split("@")[1] + except IndexError: + logger.warning( + "Invalid email format for organization %s: %s", + self.id, + self.adresse_messagerie + ) + return NoneBased on coding guidelines, log unexpected application actions with appropriate levels.
770-779: Logic bug: can_activate checks if ANY required service is subscribed instead of ALL.This critical issue was flagged in the previous review but remains unresolved. Using
.exists()returnsTrueif at least one required service has an active subscription. The method must verify that all required services are active.Apply this diff to fix the logic:
def can_activate(self, organization: Organization): """ Check if the service can be activated for the given organization. """ - if not self.required_services.count(): + required_count = self.required_services.count() + if not required_count: return True - return self.required_services.all().filter( + active_required_count = self.required_services.filter( subscriptions__organization=organization, subscriptions__is_active=True - ).exists() + ).distinct().count() + return active_required_count == required_count
896-908: Handle missing idps key to prevent KeyError.This issue was flagged in the previous review but remains unresolved. Line 903 directly accesses
self.operator.config["idps"]without using.get(). If theconfigdict exists but doesn't have anidpskey, this will raiseKeyError.Apply this diff:
@property def idp_name(self): """ Get the name of the IDP for the ProConnect subscription. """ if not self.metadata.get("idp_id"): return None - if not self.operator.config["idps"]: + idps = self.operator.config.get("idps") + if not idps: return None - for idp in self.operator.config["idps"]: + for idp in idps: if idp["id"] == self.metadata.get("idp_id"): return idp["name"] return None
🧹 Nitpick comments (2)
src/backend/core/models.py (1)
506-523: Handle empty domain edge case.The urlparse approach is excellent (addressing the previous review), but if
parsed.netlocis empty (e.g., for malformed URLs like "example.com" without a scheme), the method returns an empty string instead ofNone.Apply this diff to ensure consistent
Nonereturn for invalid cases:@property def site_internet_domain(self): """ Get the website domain for the organization. Not sure that this method is completely exhaustive. """ if not self.site_internet: return None parsed = urlparse(self.site_internet) domain = parsed.netloc + if not domain: + return None # Remove port number if present (e.g., "example.com:8080" -> "example.com") if ":" in domain: domain = domain.split(":")[0] # Remove www. prefix if present if domain.startswith("www."): return domain[4:] return domainsrc/backend/core/tests/models/test_organization.py (1)
78-105: Consider adding tests for critical validation logic.The current tests provide good coverage for domain extraction. However, consider adding tests for the following critical validation logic introduced in this PR:
Organization.get_mail_domain_status()with various RPNT criteria combinationsService.can_activate()to verify the ALL-vs-ANY logic (especially important given the bug flagged in the models review)ServiceSubscription.validate_required_services()andvalidate_proconnect_subscription()These validations enforce business rules that could break silently without test coverage.
Do you want me to generate test cases for these validation methods?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/backend/core/models.py(6 hunks)src/backend/core/tests/models/test_organization.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Use Django’s ORM for database interactions; avoid raw SQL unless strictly necessary for performance
Use Django’s built-in user model and authentication framework for user management
Log expected and unexpected application actions with appropriate log levels
Respect log level semantics: DEBUG (diagnostics), INFO (normal events), WARN (unexpected but continuing), ERROR (functional failure)
Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Files:
src/backend/core/tests/models/test_organization.pysrc/backend/core/models.py
src/backend/**/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/tests/**/*.py: Use Django’s testing tools with pytest-django for reliable test suites
Unit tests should focus on a single use-case, keep assertions minimal, and cover all plausible scenarios
Files:
src/backend/core/tests/models/test_organization.py
src/backend/**/{models,forms,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
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Define appropriate database indexes and optimize query patterns via model Meta and QuerySets
Files:
src/backend/core/models.py
🧬 Code graph analysis (2)
src/backend/core/tests/models/test_organization.py (2)
src/backend/core/factories.py (1)
OrganizationFactory(69-102)src/backend/core/models.py (2)
adresse_messagerie_domain(499-503)site_internet_domain(507-523)
src/backend/core/models.py (1)
src/frontend/src/features/api/Repository.ts (2)
Organization(33-45)ServiceSubscription(47-54)
🪛 Ruff (0.14.7)
src/backend/core/tests/models/test_organization.py
73-73: Redefinition of unused test_site_internet_domain_classic_trailing_slash from line 68
(F811)
🔇 Additional comments (8)
src/backend/core/models.py (7)
335-339: LGTM!The
MailDomainStatusenum is well-defined with clear values that align with the frontend types.
525-558: LGTM, but depends on fixing property issues.The
mail_domainproperty andget_mail_domain_statusmethod implement sound RPNT validation logic. However, ensure theadresse_messagerie_domainandsite_internet_domainproperties are fixed (per other review comments) to prevent propagation of IndexError or empty string issues.
741-748: LGTM!The
required_servicesManyToManyField is correctly configured withsymmetrical=Falsefor dependency relationships.
835-868: LGTM!The
validate_required_servicesmethod correctly verifies that all required services have active subscriptions using set comparison, and provides clear error messages listing missing services.
871-886: LGTM, but depends on fixing domain extraction.The ProConnect validation logic is correct. However, line 882's check on
self.organization.mail_domaindepends onadresse_messagerie_domainnot raisingIndexError(see earlier review comment).
888-894: LGTM!The
cleanmethod correctly invokes validation before saving, following Django best practices.
6-6: StrEnum requirement is already met. The project specifies Python 3.13+ as the minimum version (requires-python = ">=3.13,<4.0"insrc/backend/pyproject.toml), which exceeds the Python 3.11 requirement forStrEnum. No action needed.src/backend/core/tests/models/test_organization.py (1)
1-44: LGTM!The test setup and
adresse_messagerie_domaintests provide good coverage of empty, valid, and invalid email formats.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/backend/core/tests/models/test_organization.py (1)
74-77: Duplicate test method - only one will run.This is flagged by static analysis (Ruff F811). The test method
test_site_internet_domain_classic_trailing_slashis defined twice (lines 69 and 74). Python silently overrides the first definition, so only the second test runs.
🧹 Nitpick comments (2)
src/backend/core/tests/api/test_services.py (1)
698-732: Add blank line before function definition.PEP 8 recommends two blank lines before top-level function definitions.
+ def test_api_organization_proconnect_service_cannot_activate_if_idp_is_not_set():src/backend/core/tests/models/test_organization.py (1)
20-28: Consider usingis Noneinstead of== None.PEP 8 recommends using
is Nonefor None comparisons since None is a singleton.def test_messagerie_domain_empty(self): """The messagerie domain should be None if the email is not set.""" organization = factories.OrganizationFactory(adresse_messagerie=None) - assert organization.adresse_messagerie_domain == None + assert organization.adresse_messagerie_domain is None def test_messagerie_domain_empty_str(self): """The messagerie domain should be None if the email is an empty string.""" organization = factories.OrganizationFactory(adresse_messagerie="") - assert organization.adresse_messagerie_domain == None + assert organization.adresse_messagerie_domain is NoneSimilar changes apply to lines 52 and 57 for
site_internet_domaintests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
bin/pytest(1 hunks)src/backend/core/api/serializers.py(6 hunks)src/backend/core/tests/api/test_organizations.py(3 hunks)src/backend/core/tests/api/test_services.py(1 hunks)src/backend/core/tests/models/test_organization.py(1 hunks)src/backend/core/tests/utils.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/backend/core/api/serializers.py
🧰 Additional context used
📓 Path-based instructions (2)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Use Django’s ORM for database interactions; avoid raw SQL unless strictly necessary for performance
Use Django’s built-in user model and authentication framework for user management
Log expected and unexpected application actions with appropriate log levels
Respect log level semantics: DEBUG (diagnostics), INFO (normal events), WARN (unexpected but continuing), ERROR (functional failure)
Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Files:
src/backend/core/tests/api/test_services.pysrc/backend/core/tests/utils.pysrc/backend/core/tests/api/test_organizations.pysrc/backend/core/tests/models/test_organization.py
src/backend/**/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/tests/**/*.py: Use Django’s testing tools with pytest-django for reliable test suites
Unit tests should focus on a single use-case, keep assertions minimal, and cover all plausible scenarios
Files:
src/backend/core/tests/api/test_services.pysrc/backend/core/tests/utils.pysrc/backend/core/tests/api/test_organizations.pysrc/backend/core/tests/models/test_organization.py
🧠 Learnings (1)
📚 Learning: 2025-10-03T23:40:57.889Z
Learnt from: CR
Repo: suitenumerique/st-deploycenter PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-10-03T23:40:57.889Z
Learning: Applies to src/backend/**/tests/**/*.py : Use Django’s testing tools with pytest-django for reliable test suites
Applied to files:
bin/pytest
🧬 Code graph analysis (3)
src/backend/core/tests/api/test_services.py (1)
src/backend/core/models.py (1)
idp_name(897-908)
src/backend/core/tests/api/test_organizations.py (1)
src/backend/core/models.py (2)
Organization(329-561)MailDomainStatus(335-338)
src/backend/core/tests/models/test_organization.py (1)
src/backend/core/models.py (4)
adresse_messagerie_domain(499-503)site_internet_domain(507-523)get_mail_domain_status(531-558)MailDomainStatus(335-338)
🪛 Ruff (0.14.7)
src/backend/core/tests/models/test_organization.py
74-74: Redefinition of unused test_site_internet_domain_classic_trailing_slash from line 69
(F811)
🔇 Additional comments (8)
bin/pytest (1)
9-9: LGTM!The addition of
-vvvfor maximum verbosity will aid in diagnosing test failures, particularly useful given the expanded test coverage in this PR. The flag placement after"$@"preserves user-provided arguments.src/backend/core/tests/utils.py (1)
22-23: LGTM!The enhanced assertion message provides clearer context for debugging test failures by showing the expected value, actual value, and the key where the mismatch occurred.
src/backend/core/tests/api/test_services.py (4)
489-510: Good test coverage for can_activate without required services.The test properly verifies that services without required dependencies can be activated by default.
513-552: Good coverage for required services constraint.The test properly verifies both the
can_activateflag and the 400 error response with a descriptive message when required services are not activated.
662-697: Good coverage for ProConnect mail domain requirement.The test properly verifies that ProConnect services require a mail domain for activation.
795-814: Good test for IDP name resolution.The test verifies that the
idp_nameproperty correctly resolves the IDP name from the operator's config based on theidp_idin subscription metadata. This aligns with theidp_nameproperty implementation insrc/backend/core/models.py(lines 896-907).src/backend/core/tests/models/test_organization.py (1)
110-131: Good coverage for get_mail_domain_status logic.The tests thoroughly verify the RPNT-based mail domain status resolution, including edge cases for empty RPNT, unmatched criteria, valid website with invalid email, and fully valid scenarios. The test at line 130 correctly uses different domains to verify the implementation uses
adresse_messagerie_domainwhen both are valid.src/backend/core/tests/api/test_organizations.py (1)
356-390: Good test extension for mail_domain and mail_domain_status.The test correctly verifies that the API exposes
mail_domainandmail_domain_statusfields. The RPNT values["1.1", "1.2", "2.1", "2.2", "2.3"]satisfy both website and email validation criteria, correctly resulting inMailDomainStatus.VALIDand the domain extracted fromadresse_messagerie.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/backend/core/api/serializers.py (2)
287-319: Re‑evaluate exposing rawconfigon Operator and Service serializers – risk of leaking secrets.Adding
configto bothOperatorSerializerandServiceSerializermeans any sensitive keys stored there (e.g.external_management_api_keyfrom previous tests/fixtures) become part of the public API response. That’s effectively equivalent to logging secrets and violates the usual “no credentials in API payloads” expectation.Unless you are certain that:
operator.confignever contains secrets, andservice.configis strictly non‑sensitive display/configuration data,you should either remove these fields from the API, or filter out sensitive keys before serializing.
Example: the safer option is to keep
configinternal onOperatorSerializer:class OperatorSerializer(serializers.ModelSerializer): @@ class Meta: model = models.Operator - fields = ["id", "name", "url", "scope", "is_active", "user_role", "config"] + fields = ["id", "name", "url", "scope", "is_active", "user_role"] read_only_fields = fieldsIf you truly need to expose some parts of
config, consider a dedicated, sanitized representation (e.g. aSerializerMethodFieldthat drops any keys containing “key”, “secret”, “token”, “credential”, etc.), and apply similar scrutiny toServiceSerializer.config.
460-464: Guard against missingorganizationin serializer context when computingcan_activate.
OrganizationServiceSerializer.to_representationdirectly accessesself.context["organization"]. If the serializer is ever instantiated without that context key (e.g. reused elsewhere or in tests), this will raise aKeyErrorand return a 500 instead of a controlled error.Either:
- Fail fast with a clear exception, if
organizationis mandatory, or- Gracefully handle the missing context (e.g. default
can_activatetoFalse).Example (fail‑fast with a clear message):
def to_representation(self, instance): """Convert the representation to the desired format.""" - data = super().to_representation(instance) - data["can_activate"] = instance.can_activate(self.context["organization"]) + if "organization" not in self.context: + raise ValueError( + "OrganizationServiceSerializer requires 'organization' in context " + "to compute can_activate" + ) + data = super().to_representation(instance) + data["can_activate"] = instance.can_activate(self.context["organization"]) return data
🧹 Nitpick comments (5)
src/backend/core/tests/api/test_services.py (1)
489-659: Good coverage of required‑services activation; consider minor tightening of assertions and reuse.These four tests exercise the key paths for
can_activate(no required services, missing required services, cross‑organization, and blocked‑but‑updatable subscriptions) and look correct.Two small suggestions:
- In
test_api_organization_service_can_update_subscription_when_cannot_activate, after the secondPATCHthat only updatesmetadata, also assert thatis_activeremainsFalseto fully pin down the behavior.- Given the repeated setup (user/operator/org, service + required service, configs), you could factor a small helper/fixture to keep these tests shorter and more focused on expectations.
src/backend/core/tests/api/test_services_proconnect.py (2)
12-143: ProConnect precondition tests are correct but inline comments are slightly misleading.The three tests around “cannot activate if mail domain/IDP is not set” plus the “can activate when both are set” correctly assert:
can_activateisTrue(no required services restriction), and- activation is actually blocked/enabled via the subscription
PATCHreturning400/201based on mail‑domain/IDP presence.However, the inline comments like “Check that the service cannot be activated by default” contradict the
assert content["can_activate"] is True. To avoid confusion for future readers, consider rewording those comments to reflect thatcan_activateis purely about service dependencies, while ProConnect‑specific mail‑domain/IDP checks are enforced at subscription time.Also, the setup for
rpnt,adresse_messagerie, andsite_internetis duplicated across several tests; a small factory helper or pytest fixture could DRY this up.
167-251: Subscription update / IDP change flow is well covered; confirm single‑request deactivate+IDP‑change behavior.This test thoroughly checks that:
- Changing
idp_idon an active subscription is rejected.- Other metadata can still be updated while active.
- After deactivation,
idp_idcan be changed.- You cannot both change
idp_idand end up active in the same request.One nuance:
_validate_proconnect_subscriptiononly blocks when the resultingis_activeisTrue, so a singlePATCHthat sets{"is_active": False, "metadata": {"idp_id": "new"}}on an active subscription will be accepted. If product requirements truly want a strict “deactivate first, then change IDP in a separate call” policy, you may want an extra test for that case and to tighten the validator accordingly.src/backend/core/api/serializers.py (2)
323-387: ProConnectidp_idupdate guardrails look correct; consider clarifying edge semantics.The new
_validate_proconnect_subscriptionandvalidateimplementation correctly:
- Scope the extra checks to ProConnect services.
- Only run on updates (
instancepresent).- Forbid changing
metadata["idp_id"]when the resulting subscription is active.- Return a clear error structure on violation, matching the new tests.
Two details to double‑check:
- A single
PATCHthat both deactivates and changesidp_id(e.g.{"is_active": False, "metadata": {"idp_id": "new"}}) will be allowed, becauseis_activeis computed from attrs and becomesFalse. If you want “deactivate first, then change IDP” as two distinct steps, you’d need to treat “was active before this request and metadata.idp_id is changing” as invalid regardless of the newis_activevalue.- The error is raised as a top‑level
serializers.ValidationErrorfromvalidate, which is correct for DRF; no change needed there.Behavior is consistent with the current tests; only adjust if product rules require stricter sequencing.
421-427: Organizationmail_domainandmail_domain_statusenrichment is fine; consider making them explicit fields.
to_representationcleanly injectsmail_domainandmail_domain_statusfromget_mail_domain_status(), which matches how the tests and frontend are expected to consume this data.Since these keys are not declared in
Meta.fields, they won’t appear in your OpenAPI schema. If you rely on schema‑driven frontend or documentation, consider adding them as explicit read‑only fields (e.g.SerializerMethodFields) so they’re discoverable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/backend/core/api/serializers.py(6 hunks)src/backend/core/tests/api/test_services.py(1 hunks)src/backend/core/tests/api/test_services_proconnect.py(1 hunks)src/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx
🧰 Additional context used
📓 Path-based instructions (3)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Use Django’s ORM for database interactions; avoid raw SQL unless strictly necessary for performance
Use Django’s built-in user model and authentication framework for user management
Log expected and unexpected application actions with appropriate log levels
Respect log level semantics: DEBUG (diagnostics), INFO (normal events), WARN (unexpected but continuing), ERROR (functional failure)
Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Files:
src/backend/core/tests/api/test_services.pysrc/backend/core/tests/api/test_services_proconnect.pysrc/backend/core/api/serializers.py
src/backend/**/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/tests/**/*.py: Use Django’s testing tools with pytest-django for reliable test suites
Unit tests should focus on a single use-case, keep assertions minimal, and cover all plausible scenarios
Files:
src/backend/core/tests/api/test_services.pysrc/backend/core/tests/api/test_services_proconnect.py
src/backend/**/serializers.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use DRF serializers for JSON responses
Files:
src/backend/core/api/serializers.py
🧠 Learnings (2)
📚 Learning: 2025-10-03T23:40:57.889Z
Learnt from: CR
Repo: suitenumerique/st-deploycenter PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-10-03T23:40:57.889Z
Learning: Applies to src/backend/**/serializers.py : Use DRF serializers for JSON responses
Applied to files:
src/backend/core/api/serializers.py
📚 Learning: 2025-10-03T23:40:57.889Z
Learnt from: CR
Repo: suitenumerique/st-deploycenter PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-10-03T23:40:57.889Z
Learning: Applies to src/backend/**/*.py : Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Applied to files:
src/backend/core/api/serializers.py
🧬 Code graph analysis (3)
src/backend/core/tests/api/test_services.py (1)
src/backend/core/factories.py (3)
OrganizationFactory(69-102)ServiceFactory(116-132)ServiceSubscriptionFactory(146-156)
src/backend/core/tests/api/test_services_proconnect.py (2)
src/backend/core/factories.py (3)
OrganizationFactory(69-102)ServiceFactory(116-132)ServiceSubscriptionFactory(146-156)src/backend/core/models.py (1)
idp_name(897-908)
src/backend/core/api/serializers.py (1)
src/backend/core/models.py (3)
mail_domain(526-529)get_mail_domain_status(531-558)can_activate(770-779)
🔇 Additional comments (1)
src/backend/core/tests/api/test_services_proconnect.py (1)
145-165: idp_name test nicely validates model‑level behavior.
test_api_organization_proconnect_subscription_idp_namedirectly exercises theServiceSubscription.idp_namelogic against a realisticoperator.config["idps"]payload and looks spot‑on. No changes needed.
NathanVss
force-pushed
the
feat/pc-service
branch
2 times, most recently
from
de82624 to
7912cbc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/frontend/src/features/i18n/translations.json (1)
23-50: Add missing Englishhelp_centerblock underorganizations.services.
ProConnectServiceBlockusest("organizations.services.help_center.label"), but the EN tree currently has onlymail_domainunderorganizations.services(nohelp_centersibling), while FR defineshelp_center.label. This will cause EN users to see a raw key or fallback.Add an EN
help_centerblock mirroring FR:"values": { "users": "Users" - }, - "mail_domain": { + }, + "mail_domain": { "label": "NDD", "none": "None", "required": "Mail domain is required", "need_email_setup": "The mail domain is not valid. Please set it to the website domain.", "invalid": "The mail domain is not valid. Please set it to the website domain." - } + }, + "help_center": { + "label": "Help center" + }Optionally, you can also align EN
need_email_setupvsinvalidto mirror the clearer FR wording (one for “needs setup to mail provider”, one for “no valid domain found”).
♻️ Duplicate comments (7)
src/frontend/src/features/api/Repository.ts (1)
11-25: AlignOperator.configtyping with backend nullability.Backend
Operator.configis a nullable JSON field; the current type (config: { idps: OperatorIdp[]; }) still assumes it always exists and always hasidps. Call sites are already using optional chaining (e.g.operator?.config?.idps), so the runtime is prepared fornull, but the TS type doesn’t reflect that.Consider loosening the type:
export type Operator = { id: string; name: string; url: string; scope: string; is_active: boolean; - config: { - idps: OperatorIdp[]; - }; + config?: { + idps?: OperatorIdp[]; + } | null; };This will better match actual API responses and keeps call sites honest about null/undefined.
src/frontend/src/features/i18n/translations.json (1)
95-118: Fix typo in French IDP modal help text.
organizations.services.idp.modal.helpstill uses"Selectionnez un FI..."; in French it should be"Sélectionnez un FI..."with an accentedé. This was flagged previously and is still present; please correct the string.src/backend/core/models.py (3)
505-510: Defensive parsing foradresse_messagerie_domainEven though
adresse_messagerieis anEmailFieldandBaseModel.saveenforcesfull_clean(), being defensive here avoids relying entirely on upstream validation and guards against malformed values introduced by future changes or data migrations.You can safely handle unexpected formats without logging PII:
@property def adresse_messagerie_domain(self): """Get the mail domain for the organization.""" if not self.adresse_messagerie: return None - return self.adresse_messagerie.split("@")[1] + _local, sep, domain = self.adresse_messagerie.partition("@") + if not sep or not domain: + logger.warning( + "Invalid adresse_messagerie format for Organization %s", + self.id, + ) + return None + return domainThis keeps logs free of email addresses while preventing potential
IndexError. As per coding guidelines, avoid logging PII.
773-785:can_activateonly requires one required service instead of allThe current implementation returns
Trueas soon as any required service has an active subscription:if not self.required_services.count(): return True return ( self.required_services.all() .filter(subscriptions__organization=organization, subscriptions__is_active=True) .exists() )Given the semantics of “required services” and the stricter check in
ServiceSubscription.validate_required_services, this should require all required services to be active for the organization, otherwisecan_activatewill disagree with model validation and the UI can show an activatable service that will later be rejected.A safer implementation:
def can_activate(self, organization: Organization): """ Check if the service can be activated for the given organization. """ - if not self.required_services.count(): - return True - return ( - self.required_services.all() - .filter( - subscriptions__organization=organization, subscriptions__is_active=True - ) - .exists() - ) + required_qs = self.required_services.all() + required_count = required_qs.count() + if not required_count: + return True + + active_required_count = ( + required_qs.filter( + subscriptions__organization=organization, + subscriptions__is_active=True, + ) + .distinct() + .count() + ) + return active_required_count == required_countThis keeps
can_activatealigned with the subscription-level validation.
843-855: Guard against missingidpsinoperator.configwhen resolvingidp_name
idp_namecurrently indexesself.operator.config["idps"]directly. Ifconfigexists but has no"idps"key, this will raiseKeyError:if not self.operator.config["idps"]: return None for idp in self.operator.config["idps"]: ...Use
.get(or a local variable) to make this robust:@property def idp_name(self): """ Get the name of the IDP for the ProConnect subscription. """ if not self.metadata.get("idp_id"): return None - if not self.operator.config["idps"]: - return None - for idp in self.operator.config["idps"]: + idps = self.operator.config.get("idps") or [] + if not idps: + return None + for idp in idps: if idp["id"] == self.metadata.get("idp_id"): return idp["name"] return NoneThis matches how
operator.configis used elsewhere and avoids runtime errors when configuration is incomplete. Based on learnings, avoid leaking config issues as 500s.src/backend/core/api/serializers.py (2)
287-290: Re-evaluate exposing rawOperator.configin the public API
OperatorSerializernow includes theconfigfield:class Meta: model = models.Operator fields = ["id", "name", "url", "scope", "is_active", "user_role", "config"]Earlier analysis showed that
Operator.configmay contain sensitive keys like API tokens (e.g.external_management_api_key). Exposing the entire JSON unfiltered to clients risks leaking credentials and violates the “no sensitive data” guideline.Consider either:
- Removing
configfrom the public serializer and exposing only the specific, non-sensitive pieces needed by the frontend via dedicated fields, or- Implementing a filtered representation (e.g., whitelisted keys or filtering out anything whose key contains
key,secret,token,credential).Example whitelist approach:
class OperatorSerializer(serializers.ModelSerializer): user_role = serializers.SerializerMethodField(read_only=True) class Meta: model = models.Operator fields = ["id", "name", "url", "scope", "is_active", "user_role", "config"] read_only_fields = fields def to_representation(self, instance): data = super().to_representation(instance) config = data.get("config") if isinstance(config, dict): allowed_keys = {"idps"} # adjust to what the UI truly needs data["config"] = {k: v for k, v in config.items() if k in allowed_keys} return dataAs per coding guidelines and retrieved learnings, API responses should not include secrets or PII.
459-463: AvoidKeyErrorby validatingorganizationin serializer context
OrganizationServiceSerializer.to_representationassumesself.context["organization"]is always present:data["can_activate"] = instance.can_activate(self.context["organization"])If the serializer is instantiated without that context key (e.g., reused in a different view), this will raise a
KeyErrorand return 500 instead of a clear error.Options:
- Enforce the requirement explicitly:
def to_representation(self, instance): """Convert the representation to the desired format.""" - data = super().to_representation(instance) - data["can_activate"] = instance.can_activate(self.context["organization"]) + data = super().to_representation(instance) + if "organization" not in self.context: + raise ValueError( + "OrganizationServiceSerializer requires 'organization' in context" + ) + organization = self.context["organization"] + data["can_activate"] = instance.can_activate(organization) return data
- Or, if
organizationis optional in some uses, fall back to a safe default (e.g.,can_activate = False) when missing.
🧹 Nitpick comments (10)
bin/pytest (1)
9-9: Consider making verbosity configurable instead of hardcoded.Hardcoding
-vvv(maximum verbosity) can generate excessive output in CI/CD pipelines, making it harder to identify actual failures. Consider making verbosity configurable so developers can increase it when debugging while keeping CI logs concise.You could default to
-vor-vvand let users override via the"$@"passthrough:- pytest "$@" -s -vvv + pytest -s -v "$@"Then developers can run
./bin/pytest -vvvwhen they need maximum verbosity.src/backend/core/admin.py (1)
388-406: Consider using a ManyToMany-friendly widget forrequired_services.Adding
required_servicesdirectly into the main fieldset works, but the default multiple-select can become unwieldy as the number of services grows. You might want to enablefilter_horizontalorfilter_verticalonrequired_servicesinServiceAdminto keep the admin usable for large datasets.src/frontend/src/features/ui/components/service/ServiceAttribute.tsx (1)
1-30: Defaultinteractivebased on presence ofonClickto avoid silently disabled buttons.Right now
disabled={!interactive}means that if a caller passesonClickbut forgetsinteractive, the button is disabled and the handler never fires.You can make usage safer by normalizing the flag:
export const ServiceAttribute = ({ name, value, children, interactive, onClick, }: { @@ - }) => { - return ( + }) => { + const isInteractive = interactive ?? !!onClick; + return ( <div className="dc__service__attribute"> @@ <button className="dc__service__attribute__top__value" - onClick={onClick} - disabled={!interactive} + onClick={onClick} + disabled={!isInteractive} type="button" >Existing call sites that already set
interactivekeep their behavior; new ones get a sensible default.src/frontend/src/features/ui/components/service/implementations/RegularServiceBlock.tsx (1)
1-13:RegularServiceBlockwiring is correct; consider explicitly threading base props.The wrapper correctly delegates to
useServiceBlockandServiceBlock. To reduce coupling ifuseServiceBlock’s return shape changes later, you might explicitly pass the base props through:-export const RegularServiceBlock = (props: { - service: Service; - organization: Organization; - }) => { - const blockProps = useServiceBlock(props.service, props.organization); - return <ServiceBlock {...blockProps} />; - }; +export const RegularServiceBlock = (props: { + service: Service; + organization: Organization; +}) => { + const blockProps = useServiceBlock(props.service, props.organization); + return <ServiceBlock {...blockProps} service={props.service} organization={props.organization} />; +};Not mandatory, but it avoids a hidden dependency on
useServiceBlockalways returningserviceandorganization.src/backend/core/tests/models/test_organization.py (1)
20-181: Tests are correct; minor style cleanups for readability.The test coverage for
adresse_messagerie_domain,site_internet_domain, andget_mail_domain_statuslooks solid and matches the model implementation.Two small, non-blocking polish suggestions:
- Prefer
is Noneover== Nonein assertions (adresse_messagerie_domain/site_internet_domain“empty” cases) to follow common Python style.test_site_internet_domain_classic_trailing_query_paramsandtest_site_internet_domain_classic_trailing_slash_query_paramscurrently use the same URL (https://www.commune.fr/?param=value); if they’re meant to cover different shapes, adjust one URL, otherwise consider merging them.Purely cosmetic, no need to block the PR.
src/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx (2)
102-115: Avoid submitting the form when closing the IDP modalThe modal close button is declared with
type="submit"inside a<form>. Clicking it will submit the parent form and triggerreact-hook-formvalidation even though the user only intends to close the modal.Consider switching it to a plain button:
- <Button type="submit" onClick={idpModal.close}> + <Button type="button" onClick={idpModal.close}>This keeps validation tied to the explicit activation flow (
canActivateSubscription) instead of a passive close action.
193-205: Harden external help-center links opened in a new tabBoth the
<a>and theButtonopenhelp_center_urlwithtarget="_blank"but withoutrel="noopener noreferrer". For untrusted or operator-configurable URLs, this can allow the opened page to reach back intowindow.opener.Suggestion:
- <a target="_blank" href={props.service.config?.help_center_url}> + <a + target="_blank" + rel="noopener noreferrer" + href={props.service.config?.help_center_url} + > … - <Button + <Button color="tertiary" size="nano" href={props.service.config?.help_center_url} - target="_blank" + target="_blank" + rel="noopener noreferrer" />src/frontend/src/pages/operators/[operator_id]/organizations/index.scss (1)
196-202: Remove duplicatemargin-topdeclaration
.dc__service__warningsetsmargin-toptwice (4px then 16px); only the last one is effective, so the first is dead code.You can safely drop the earlier declaration to avoid confusion:
.dc__service__warning { color: var(--c--theme--colors--warning-500); font-size: 12px; font-weight: 400; - margin-top: 4px; margin-top: 16px; }src/backend/core/api/serializers.py (2)
305-320: Double-check that exposingService.configis safe
ServiceSerializernow returnsconfigto clients. Depending on howService.configis populated, this may be fine (purely public display/configuration data) or could leak operational details (e.g., backend URLs, tokens).If there is any chance
configholds sensitive information, mirror the approach suggested forOperatorSerializer: either restrict what goes intoconfig, or filter its representation to a safe subset before sending it to clients.
419-425: Be explicit about serializingmail_domain_status
OrganizationSerializer.to_representationinjects:mail_domain, mail_domain_status = instance.get_mail_domain_status() data["mail_domain"] = mail_domain data["mail_domain_status"] = mail_domain_statusBecause
MailDomainStatusis aStrEnum, JSON rendering will work (values are string-like). For clarity and to decouple the API from the enum implementation, consider serializing the raw value:mail_domain, mail_domain_status = instance.get_mail_domain_status() data["mail_domain"] = mail_domain - data["mail_domain_status"] = mail_domain_status + data["mail_domain_status"] = mail_domain_status.valueThis makes the public API field a plain string (
"valid","need_email_setup","invalid") regardless of internal enum representation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (23)
bin/pytest(1 hunks)src/backend/core/admin.py(2 hunks)src/backend/core/api/serializers.py(6 hunks)src/backend/core/api/viewsets/service.py(2 hunks)src/backend/core/migrations/0009_service_required_services.py(1 hunks)src/backend/core/models.py(6 hunks)src/backend/core/tests/api/test_operators.py(2 hunks)src/backend/core/tests/api/test_organizations.py(3 hunks)src/backend/core/tests/api/test_services.py(1 hunks)src/backend/core/tests/api/test_services_proconnect.py(1 hunks)src/backend/core/tests/models/test_organization.py(1 hunks)src/backend/core/tests/utils.py(1 hunks)src/backend/core/webhooks.py(2 hunks)src/frontend/package.json(1 hunks)src/frontend/src/features/api/Repository.ts(3 hunks)src/frontend/src/features/i18n/translations.json(2 hunks)src/frontend/src/features/ui/components/service/ServiceAttribute.tsx(1 hunks)src/frontend/src/features/ui/components/service/ServiceBlock.tsx(1 hunks)src/frontend/src/features/ui/components/service/ServiceBlockDispatcher.tsx(1 hunks)src/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx(1 hunks)src/frontend/src/features/ui/components/service/implementations/RegularServiceBlock.tsx(1 hunks)src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx(3 hunks)src/frontend/src/pages/operators/[operator_id]/organizations/index.scss(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- src/frontend/src/features/ui/components/service/ServiceBlockDispatcher.tsx
- src/backend/core/api/viewsets/service.py
- src/backend/core/webhooks.py
- src/frontend/src/features/ui/components/service/ServiceBlock.tsx
- src/backend/core/tests/utils.py
- src/backend/core/tests/api/test_operators.py
- src/backend/core/tests/api/test_services_proconnect.py
- src/backend/core/tests/api/test_organizations.py
- src/backend/core/migrations/0009_service_required_services.py
- src/frontend/package.json
🧰 Additional context used
📓 Path-based instructions (5)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Use Django’s ORM for database interactions; avoid raw SQL unless strictly necessary for performance
Use Django’s built-in user model and authentication framework for user management
Log expected and unexpected application actions with appropriate log levels
Respect log level semantics: DEBUG (diagnostics), INFO (normal events), WARN (unexpected but continuing), ERROR (functional failure)
Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Files:
src/backend/core/tests/api/test_services.pysrc/backend/core/models.pysrc/backend/core/admin.pysrc/backend/core/tests/models/test_organization.pysrc/backend/core/api/serializers.py
src/backend/**/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/tests/**/*.py: Use Django’s testing tools with pytest-django for reliable test suites
Unit tests should focus on a single use-case, keep assertions minimal, and cover all plausible scenarios
Files:
src/backend/core/tests/api/test_services.pysrc/backend/core/tests/models/test_organization.py
src/backend/**/{models,forms,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
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Define appropriate database indexes and optimize query patterns via model Meta and QuerySets
Files:
src/backend/core/models.py
src/backend/**/serializers.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use DRF serializers for JSON responses
Files:
src/backend/core/api/serializers.py
🧠 Learnings (3)
📚 Learning: 2025-10-03T23:40:57.889Z
Learnt from: CR
Repo: suitenumerique/st-deploycenter PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-10-03T23:40:57.889Z
Learning: Applies to src/backend/**/tests/**/*.py : Use Django’s testing tools with pytest-django for reliable test suites
Applied to files:
bin/pytest
📚 Learning: 2025-10-03T23:40:57.889Z
Learnt from: CR
Repo: suitenumerique/st-deploycenter PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-10-03T23:40:57.889Z
Learning: Applies to src/backend/**/serializers.py : Use DRF serializers for JSON responses
Applied to files:
src/backend/core/api/serializers.py
📚 Learning: 2025-10-03T23:40:57.889Z
Learnt from: CR
Repo: suitenumerique/st-deploycenter PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-10-03T23:40:57.889Z
Learning: Applies to src/backend/**/*.py : Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Applied to files:
src/backend/core/api/serializers.py
🧬 Code graph analysis (5)
src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx (2)
src/frontend/src/features/api/Repository.ts (1)
SERVICE_TYPE_PROCONNECT(56-56)src/frontend/src/features/ui/components/service/ServiceBlockDispatcher.tsx (1)
ServiceBlockDispatcher(9-17)
src/backend/core/tests/api/test_services.py (1)
src/backend/core/factories.py (8)
UserFactory(16-27)OperatorFactory(47-55)UserOperatorRoleFactory(58-66)OrganizationFactory(69-102)OperatorOrganizationRoleFactory(105-113)ServiceFactory(116-132)OperatorServiceConfigFactory(135-143)ServiceSubscriptionFactory(146-156)
src/frontend/src/features/ui/components/service/implementations/RegularServiceBlock.tsx (2)
src/frontend/src/features/api/Repository.ts (2)
Service(58-74)Organization(33-45)src/frontend/src/features/ui/components/service/ServiceBlock.tsx (2)
useServiceBlock(11-53)ServiceBlock(55-189)
src/backend/core/models.py (1)
src/frontend/src/features/api/Repository.ts (2)
Organization(33-45)ServiceSubscription(47-54)
src/backend/core/tests/models/test_organization.py (2)
src/backend/core/models.py (5)
Organization(329-564)adresse_messagerie_domain(506-510)site_internet_domain(513-529)get_mail_domain_status(537-564)MailDomainStatus(335-342)src/backend/core/factories.py (1)
OrganizationFactory(69-102)
🔇 Additional comments (5)
src/backend/core/admin.py (1)
267-272: Operator admin fieldsets reorganization looks good.Moving
configinto its own “Configuration” fieldset is consistent with the rest of the admin and keeps the financial info section clean.src/backend/core/tests/api/test_services.py (1)
490-660: Strong coverage ofrequired_servicesactivation semantics.The new tests around
can_activateandrequired_servicesnicely cover:
- base case (no required services →
can_activate=True),- blocked activation with clear 400 error when a required service is inactive,
- allowing metadata-only updates when activation is blocked,
- ensuring activation depends on the required service being active for the same organization.
The setups are consistent with the existing test style and should guard regressions in the activation rules.
src/frontend/src/features/api/Repository.ts (1)
27-74: New mail-domain and service typing matches backend usage.
MailDomainStatusplusOrganization.mail_domain/mail_domain_statusalign with the backend enum and serializer behaviour.SERVICE_TYPE_PROCONNECTand the addedServicefields (type,can_activate,config.help_center_url) are consistent with how the UI dispatches and renders ProConnect vs regular services.These additions look coherent and should make downstream components’ props safer and clearer.
src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx (1)
7-15: Service ordering + dispatcher integration look correct.
servicesOrderedcorrectly promotes ProConnect (type === SERVICE_TYPE_PROCONNECT) to the top and then sorts the rest bydisplay_prioritydescending, with missing priorities naturally falling to the end.- The map now delegates to
ServiceBlockDispatcherwith the fullserviceandorganizationobjects, which plugs into the new ProConnect/regular block split without changing the page’s overall layout.- The loading guard (
isServicesLoading || !organization || !operator) keeps the UI from rendering blocks before essential data is present.All of this is consistent with the new service model and UI components.
Also applies to: 35-49, 107-118
src/backend/core/api/serializers.py (1)
326-371: ProConnect IDP immutability validation looks good; ensure it’s wired to update flows onlyThe
_validate_proconnect_subscription/validatecombo correctly prevents changingmetadata.idp_idon an already-active ProConnect subscription by comparing the old and new values and raising a field-scopedValidationError.A couple of minor points to keep in mind:
- This logic only runs when
self.instanceis set (i.e., updates), which is appropriate; new subscriptions are governed by the model-levelvalidate_proconnect_subscription.- The guard
if not isinstance(new_metadata, dict) or not "idp_id" in new_metadata:intentionally skips cases where metadata changes don’t touchidp_id, which seems consistent with the requirement.No changes strictly required here; just ensure your API endpoints use this serializer for the flows where IDP updates are allowed/forbidden so the constraint is actually enforced.
src/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx
Show resolved
Hide resolved
We it fails it was quite difficult to understand where the error come from.
Let's add the config of the operator to the serializer, we need to be able to grab the idp informations from the frontend.
The frontend will start to put some data in this field, so let's add it to the serializer so the frontend can get its value.
NathanVss
force-pushed
the
feat/pc-service
branch
from
7912cbc to
4c70396
Compare
We need to be able to allow activation on services only if some others are activated.
This is a special service which behave is a quite specific way. So let's implements those checks.
In order to provide the domain mail to the ProConnect card, we need to implement email extraction based on the RPNT data.
We need to get more information from the outcoming webhooks.
Let's add the previous commits attribute into django admin.
We'll need RHF to perform form-like validation on cards.
Let's adjust the types to the recent backend changes.
As of now, the service cards are not always generic. Let's implement a new approach that allow for deep customization and reusability of default components.
Update the translation based on the previous commits.
NathanVss
force-pushed
the
feat/pc-service
branch
from
4c70396 to
6a07e3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (6)
src/backend/core/models.py (2)
773-785: Logic bug:can_activateincorrectly returns True if ANY required service is active instead of ALL.The current implementation uses
.exists(), which returnsTrueas soon as at least one required service has an active subscription. This meanscan_activatewill incorrectly reportTrueeven when some required services are missing.Apply this fix to check that ALL required services are active:
def can_activate(self, organization: Organization): """ Check if the service can be activated for the given organization. """ - if not self.required_services.count(): + required_count = self.required_services.count() + if not required_count: return True - return ( - self.required_services.all() - .filter( - subscriptions__organization=organization, subscriptions__is_active=True - ) - .exists() - ) + active_count = ( + self.required_services.filter( + subscriptions__organization=organization, + subscriptions__is_active=True, + ) + .distinct() + .count() + ) + return active_count == required_count
843-855: PotentialKeyErrorwhen accessingoperator.config["idps"].Line 850 directly accesses
self.operator.config["idps"]without using.get(). If the config dict exists but lacks theidpskey, this will raiseKeyError.Apply this diff:
@property def idp_name(self): """ Get the name of the IDP for the ProConnect subscription. """ if not self.metadata.get("idp_id"): return None - if not self.operator.config["idps"]: + idps = self.operator.config.get("idps") + if not idps: return None - for idp in self.operator.config["idps"]: + for idp in idps: if idp["id"] == self.metadata.get("idp_id"): return idp["name"] return Nonesrc/backend/core/api/serializers.py (1)
459-463: PotentialKeyErroriforganizationis missing from serializer context.Line 462 directly accesses
self.context["organization"]without checking if the key exists. If the serializer is instantiated without this context, aKeyErrorwill be raised.Add validation to ensure clear error messaging:
def to_representation(self, instance): """Convert the representation to the desired format.""" data = super().to_representation(instance) + if "organization" not in self.context: + raise ValueError("OrganizationServiceSerializer requires 'organization' in context") + organization = self.context["organization"] - data["can_activate"] = instance.can_activate(self.context["organization"]) + data["can_activate"] = instance.can_activate(organization) return datasrc/frontend/src/features/i18n/translations.json (1)
29-53: Translations for ProConnect are complete; one minor FR typo remainsThe new EN/FR keys for
services.cannot_activate,services.idp,services.mail_domain, andservices.help_centerlook consistent with the UI usage.Minor nit: in the French block,
organizations.services.idp.modal.helpstill hasSelectionnez un FI pour l'organisation.— you probably wantSélectionnezwith the accent:- "help": "Selectionnez un FI pour l'organisation.", + "help": "Sélectionnez un FI pour l'organisation.",Purely cosmetic, feel free to adjust when convenient.
Also applies to: 98-121
src/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx (1)
68-76: Add a defensive guard to prevent IDP changes on active subscriptionsUI-wise,
interactive={!props.service.subscription?.is_active}disables the attribute button, butsaveIdpChangewill still happily send an IDP update ifonChangeis ever triggered (e.g. via a future refactor or programmatic call), which conflicts with the “cannot change IDP once active” rule.A lightweight guard keeps behavior consistent with backend validation:
- const saveIdpChange = (idp?: OperatorIdp) => { + const saveIdpChange = (idp?: OperatorIdp) => { + if (props.service.subscription?.is_active) { + return; + } const data: Partial<ServiceSubscription> = { metadata: { idp_id: idp?.id }, }; if (!props.service.subscription) { data.is_active = false; } blockProps.onChangeSubscription(data); };This ensures active subscriptions can’t get their IDP mutated accidentally, even if the UI wiring changes later.
Also applies to: 138-144
src/frontend/src/features/api/Repository.ts (1)
11-24: RevisitOperator.config.idpstyping to match nullable backend configBackend
Operator.configis nullable and may not always contain anidpsarray, but the type here assumes a non‑null{ idps: OperatorIdp[] }. This, combined with usages likeoperator?.config.idps?.map(...), can lead to runtime errors whenconfigisnullor missingidps.Consider aligning the type and updating call sites to use optional chaining/defaults:
export type Operator = { id: string; name: string; url: string; scope: string; is_active: boolean; - config: { - idps: OperatorIdp[]; - }; + config: { + idps?: OperatorIdp[]; + } | null; };And then in consumers (e.g. ProConnectServiceBlock):
-const getIdp = (idp_id: string) => { - return operator?.config.idps?.find((idp) => idp.id == idp_id); -}; +const getIdp = (idp_id: string) => { + return operator?.config?.idps?.find((idp) => idp.id === idp_id); +}; ... -operator?.config.idps?.map((idp) => ...) +operator?.config?.idps?.map((idp) => ...) ?? []This keeps the frontend model honest with the backend and prevents crashes when
configis absent.
🧹 Nitpick comments (3)
src/backend/core/api/serializers.py (1)
349-349: Improve readability of membership check.Use
not inoperator for clearer intent.- if not isinstance(new_metadata, dict) or not "idp_id" in new_metadata: + if not isinstance(new_metadata, dict) or "idp_id" not in new_metadata: returnsrc/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx (1)
51-53: Minor cleanups: no-op submit handler and external link relTwo small nits you may want to address:
onSubmitis a memoized no-op solely to satisfy<form onSubmit={handleSubmit(onSubmit)}>. You can simplify withoutuseMemo:- const onSubmit = useMemo(() => { - return () => void 0; - }, []); + const onSubmit = () => void 0;
- The help-center link opens in a new tab but lacks a
relattribute; addingrel="noreferrer noopener"is a safer default whentarget="_blank":- <a target="_blank" href={props.service.config?.help_center_url}> + <a + target="_blank" + rel="noreferrer noopener" + href={props.service.config?.help_center_url} + > {t("organizations.services.help_center.label")} </a>Both are non-functional tweaks but improve clarity and security posture.
Also applies to: 193-205
src/frontend/src/pages/operators/[operator_id]/organizations/index.scss (1)
189-257: SCSS additions look good; fix duplicatemargin-topon warningThe new classes for service attributes/errors/warnings align with the new components and use consistent theming.
One small issue:
.dc__service__warningdefinesmargin-toptwice, with the latter winning:.dc__service__warning { ... margin-top: 4px; margin-top: 16px; }Drop one (probably the 4px) to avoid confusion:
.dc__service__warning { color: var(--c--theme--colors--warning-500); font-size: 12px; font-weight: 400; - margin-top: 4px; - margin-top: 16px; + margin-top: 16px; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
src/backend/core/admin.py(2 hunks)src/backend/core/api/serializers.py(6 hunks)src/backend/core/api/viewsets/service.py(2 hunks)src/backend/core/migrations/0009_service_required_services.py(1 hunks)src/backend/core/models.py(6 hunks)src/backend/core/tests/api/test_operators.py(2 hunks)src/backend/core/tests/api/test_organizations.py(3 hunks)src/backend/core/tests/api/test_services.py(1 hunks)src/backend/core/tests/api/test_services_proconnect.py(1 hunks)src/backend/core/tests/models/test_organization.py(1 hunks)src/backend/core/tests/utils.py(1 hunks)src/backend/core/webhooks.py(2 hunks)src/frontend/package.json(1 hunks)src/frontend/src/features/api/Repository.ts(3 hunks)src/frontend/src/features/i18n/translations.json(2 hunks)src/frontend/src/features/ui/components/service/ServiceAttribute.tsx(1 hunks)src/frontend/src/features/ui/components/service/ServiceBlock.tsx(1 hunks)src/frontend/src/features/ui/components/service/ServiceBlockDispatcher.tsx(1 hunks)src/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx(1 hunks)src/frontend/src/features/ui/components/service/implementations/RegularServiceBlock.tsx(1 hunks)src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx(3 hunks)src/frontend/src/pages/operators/[operator_id]/organizations/index.scss(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (11)
- src/frontend/src/features/ui/components/service/ServiceAttribute.tsx
- src/backend/core/tests/utils.py
- src/frontend/src/features/ui/components/service/ServiceBlock.tsx
- src/backend/core/webhooks.py
- src/backend/core/migrations/0009_service_required_services.py
- src/backend/core/api/viewsets/service.py
- src/frontend/package.json
- src/backend/core/tests/api/test_services_proconnect.py
- src/frontend/src/features/ui/components/service/implementations/RegularServiceBlock.tsx
- src/backend/core/tests/api/test_organizations.py
- src/backend/core/admin.py
🧰 Additional context used
📓 Path-based instructions (5)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Use Django’s ORM for database interactions; avoid raw SQL unless strictly necessary for performance
Use Django’s built-in user model and authentication framework for user management
Log expected and unexpected application actions with appropriate log levels
Respect log level semantics: DEBUG (diagnostics), INFO (normal events), WARN (unexpected but continuing), ERROR (functional failure)
Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Files:
src/backend/core/tests/api/test_services.pysrc/backend/core/tests/api/test_operators.pysrc/backend/core/tests/models/test_organization.pysrc/backend/core/api/serializers.pysrc/backend/core/models.py
src/backend/**/tests/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/tests/**/*.py: Use Django’s testing tools with pytest-django for reliable test suites
Unit tests should focus on a single use-case, keep assertions minimal, and cover all plausible scenarios
Files:
src/backend/core/tests/api/test_services.pysrc/backend/core/tests/api/test_operators.pysrc/backend/core/tests/models/test_organization.py
src/backend/**/serializers.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use DRF serializers for JSON responses
Files:
src/backend/core/api/serializers.py
src/backend/**/{models,forms,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
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Define appropriate database indexes and optimize query patterns via model Meta and QuerySets
Files:
src/backend/core/models.py
🧠 Learnings (2)
📚 Learning: 2025-10-03T23:40:57.889Z
Learnt from: CR
Repo: suitenumerique/st-deploycenter PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-10-03T23:40:57.889Z
Learning: Applies to src/backend/**/serializers.py : Use DRF serializers for JSON responses
Applied to files:
src/backend/core/api/serializers.py
📚 Learning: 2025-10-03T23:40:57.889Z
Learnt from: CR
Repo: suitenumerique/st-deploycenter PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-10-03T23:40:57.889Z
Learning: Applies to src/backend/**/*.py : Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Applied to files:
src/backend/core/api/serializers.py
🧬 Code graph analysis (7)
src/frontend/src/features/ui/components/service/ServiceBlockDispatcher.tsx (4)
src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx (1)
Organization(16-122)src/frontend/src/features/api/Repository.ts (1)
SERVICE_TYPE_PROCONNECT(56-56)src/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx (1)
ProConnectServiceBlock(41-211)src/frontend/src/features/ui/components/service/implementations/RegularServiceBlock.tsx (1)
RegularServiceBlock(7-13)
src/backend/core/tests/models/test_organization.py (2)
src/backend/core/models.py (5)
Organization(329-564)adresse_messagerie_domain(506-510)site_internet_domain(513-529)get_mail_domain_status(537-564)MailDomainStatus(335-342)src/backend/core/factories.py (1)
OrganizationFactory(69-102)
src/frontend/src/features/api/Repository.ts (1)
src/backend/core/models.py (4)
MailDomainStatus(335-342)Organization(329-564)Service(655-785)ServiceSubscription(788-917)
src/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx (5)
src/frontend/src/features/layouts/components/GlobalLayout.tsx (1)
useOperatorContext(57-63)src/frontend/src/features/ui/components/service/ServiceBlock.tsx (2)
useServiceBlock(11-53)ServiceBlock(55-189)src/frontend/src/features/api/Repository.ts (1)
OperatorIdp(11-14)src/frontend/src/features/ui/components/service/ServiceAttribute.tsx (1)
ServiceAttribute(1-30)src/backend/core/models.py (1)
MailDomainStatus(335-342)
src/backend/core/api/serializers.py (1)
src/backend/core/models.py (3)
mail_domain(532-535)get_mail_domain_status(537-564)can_activate(773-785)
src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx (2)
src/frontend/src/features/api/Repository.ts (1)
SERVICE_TYPE_PROCONNECT(56-56)src/frontend/src/features/ui/components/service/ServiceBlockDispatcher.tsx (1)
ServiceBlockDispatcher(9-17)
src/backend/core/models.py (1)
src/frontend/src/features/api/Repository.ts (2)
Organization(33-45)ServiceSubscription(47-54)
🔇 Additional comments (3)
src/frontend/src/features/api/Repository.ts (1)
27-74: Mail-domain and service extensions look consistent with backend modelThe
MailDomainStatusenum,Organization.mail_domain/mail_domain_status,SERVICE_TYPE_PROCONNECT, and the extendedServicefields (type,can_activate,config.help_center_url) line up with the described backend behavior and are typed sensibly for the consuming UI components.src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx (1)
35-49: Service ordering and dispatcher integration look solid
servicesOrdereddefensively handlesservices?.results, promotes ProConnect first, then orders byoperator_config.display_priority(descending with undefineds last), and the render path correctly waits forservices/organization/operatorbefore mapping throughServiceBlockDispatcher. This matches the intended UX without obvious edge‑case issues.Also applies to: 108-116
src/frontend/src/features/ui/components/service/ServiceBlockDispatcher.tsx (1)
1-17: Dispatcher abstraction is clear and type-safeThe
ServiceBlockDispatchercleanly routes betweenProConnectServiceBlockandRegularServiceBlockbased onservice.typeand reuses sharedService/Organizationtypes, which keeps the per-service branching centralized.
src/frontend/src/features/ui/components/service/implementations/ProConnectServiceBlock.tsx
Show resolved
Hide resolved
We do not want to expose all params, we just want to cherry-pick.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/backend/core/api/viewsets/service.py (1)
181-186: HandleDoesNotExistexceptions for invalid foreign key lookups.Lines 181-185 perform direct
.get()calls for Organization, Service, and Operator without exception handling. Invalid IDs will raiseDoesNotExistexceptions, causing 500 errors instead of proper 404 responses.+from django.shortcuts import get_object_or_404 + def partial_update(self, request, *args, **kwargs): """ Partially update or create the subscription for the operator-organization-service triple. Creates the subscription if it doesn't exist (upsert behavior). """ queryset = self.get_queryset() subscription = queryset.first() - organization = models.Organization.objects.get( - id=self.kwargs["organization_id"] + organization = get_object_or_404( + models.Organization, id=self.kwargs["organization_id"] ) - service = models.Service.objects.get(id=self.kwargs["service_id"]) - operator = models.Operator.objects.get(id=self.kwargs["operator_id"]) + service = get_object_or_404(models.Service, id=self.kwargs["service_id"]) + operator = get_object_or_404(models.Operator, id=self.kwargs["operator_id"])
♻️ Duplicate comments (2)
src/backend/core/api/serializers.py (1)
476-480: Handle missingorganizationin serializer context gracefully.Direct access to
self.context["organization"]raisesKeyErrorif the serializer is instantiated without the required context. The viewset at lines 101-104 ofservice.pyprovides this context, but the serializer should be defensive.def to_representation(self, instance): """Convert the representation to the desired format.""" data = super().to_representation(instance) - data["can_activate"] = instance.can_activate(self.context["organization"]) + organization = self.context.get("organization") + if not organization: + raise ValueError("OrganizationServiceSerializer requires 'organization' in context") + data["can_activate"] = instance.can_activate(organization) return datasrc/backend/core/api/viewsets/service.py (1)
98-105: HandleDoesNotExistexception for invalid organization ID.The direct
Organization.objects.get()call raisesOrganization.DoesNotExistfor invalid IDs, resulting in a 500 error instead of a proper 404 response. The permission class only validates thatorganization_idexists in kwargs, not that it's valid in the database.+from django.shortcuts import get_object_or_404 + def get_serializer_context(self): - """Add operator_id and organization to serializer context.""" + """Add operator_id and organization to serializer context.""" context = super().get_serializer_context() context["operator_id"] = self.kwargs["operator_id"] - organization = models.Organization.objects.get( - id=self.kwargs["organization_id"] + organization = get_object_or_404( + models.Organization, id=self.kwargs["organization_id"] ) context["organization"] = organization return context
🧹 Nitpick comments (1)
src/backend/core/api/serializers.py (1)
366-366: Usenot inoperator for better Python style.Replace
not "idp_id" in new_metadatawith the more Pythonic"idp_id" not in new_metadata.- if not isinstance(new_metadata, dict) or not "idp_id" in new_metadata: + if not isinstance(new_metadata, dict) or "idp_id" not in new_metadata:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/backend/core/api/serializers.py(6 hunks)src/backend/core/api/viewsets/service.py(2 hunks)src/backend/core/tests/api/test_operators.py(3 hunks)src/backend/core/tests/api/test_services.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/backend/core/tests/api/test_operators.py
- src/backend/core/tests/api/test_services.py
🧰 Additional context used
📓 Path-based instructions (2)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Use Django’s ORM for database interactions; avoid raw SQL unless strictly necessary for performance
Use Django’s built-in user model and authentication framework for user management
Log expected and unexpected application actions with appropriate log levels
Respect log level semantics: DEBUG (diagnostics), INFO (normal events), WARN (unexpected but continuing), ERROR (functional failure)
Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Files:
src/backend/core/api/viewsets/service.pysrc/backend/core/api/serializers.py
src/backend/**/serializers.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use DRF serializers for JSON responses
Files:
src/backend/core/api/serializers.py
🧠 Learnings (2)
📚 Learning: 2025-10-03T23:40:57.889Z
Learnt from: CR
Repo: suitenumerique/st-deploycenter PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-10-03T23:40:57.889Z
Learning: Applies to src/backend/**/serializers.py : Use DRF serializers for JSON responses
Applied to files:
src/backend/core/api/serializers.py
📚 Learning: 2025-10-03T23:40:57.889Z
Learnt from: CR
Repo: suitenumerique/st-deploycenter PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-10-03T23:40:57.889Z
Learning: Applies to src/backend/**/*.py : Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Applied to files:
src/backend/core/api/serializers.py
🧬 Code graph analysis (1)
src/backend/core/api/serializers.py (1)
src/backend/core/models.py (15)
Meta(62-63)Meta(174-177)Meta(227-231)Meta(318-323)Meta(488-500)Meta(596-604)Meta(640-649)Meta(753-760)Meta(832-838)Meta(979-997)Meta(1062-1069)Operator(188-282)ServiceSubscription(788-917)mail_domain(532-535)get_mail_domain_status(537-564)
🔇 Additional comments (2)
src/backend/core/api/serializers.py (2)
286-307: Good implementation of config whitelisting.The SerializerMethodField with explicit whitelist (
["idps"]) effectively addresses the security concerns raised in previous reviews by preventing exposure of sensitive keys likeexternal_management_api_key.
314-337: LGTM! Consistent security pattern.The whitelist approach for Service config mirrors the OperatorSerializer implementation and appropriately exposes only non-sensitive configuration (
["help_center_url"]).
The optimistic implementation was made for the activation but not when disabling.
Added error handling for the case when an organization is not found in the database, raising a NotFound exception with a clear message.
Added a check in the OrganizationServiceSerializer to ensure that the 'organization' key is present in the context, raising a ValueError if it is missing. This improves error handling and ensures proper usage of the serializer.
Added a step to create a Docker network in both the linting and testing jobs of the CI workflow. This ensures the necessary network setup is in place before running subsequent steps.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/backend/core/api/serializers.py (1)
343-402: Active ProConnect subscriptions can still loseidp_idvia metadata replacement.
_validate_proconnect_subscriptionreturns early when"idp_id"is not inattrs["metadata"]. For an active ProConnect subscription with an existingmetadata["idp_id"], a partial update that sendsmetadatawithoutidp_idreplaces the JSON and effectively clears the IDP while leavingis_active=True, contradicting the invariant described in the docstring.You can close this hole by only short‑circuiting on non‑dict metadata, and always comparing the effective
idp_idfor active ProConnect subscriptions:if "metadata" not in attrs: return new_metadata = attrs["metadata"] - if not isinstance(new_metadata, dict) or not "idp_id" in new_metadata: - return + if not isinstance(new_metadata, dict): + return current_idp_id = instance.metadata.get("idp_id") new_idp_id = new_metadata.get("idp_id") if current_idp_id == new_idp_id: returnAlso, because
validate()guards onself.instance, this check currently only runs on updates. If you also need similar constraints on initial creation viaOrganizationServiceSubscriptionViewSet.partial_update, you’ll need a complementary check there or rely on model‑level validation being invoked explicitly.src/backend/core/api/viewsets/service.py (1)
186-191: UnhandledDoesNotExistexceptions inpartial_updatefor organization/service/operator lookups.Lines 186-190 perform direct
.objects.get()calls that will raise unhandledDoesNotExistexceptions if any ID is invalid, resulting in 500 responses instead of 404s. The codebase already establishes the correct pattern inget_serializer_context(lines 101-108) andget_object(lines 153-158) whereDoesNotExistis caught and converted toNotFound. Apply the same pattern here:- organization = models.Organization.objects.get( - id=self.kwargs["organization_id"] - ) - service = models.Service.objects.get(id=self.kwargs["service_id"]) - operator = models.Operator.objects.get(id=self.kwargs["operator_id"]) + try: + organization = models.Organization.objects.get( + id=self.kwargs["organization_id"] + ) + except models.Organization.DoesNotExist as err: + raise NotFound("Organization not found.") from err + + try: + service = models.Service.objects.get(id=self.kwargs["service_id"]) + except models.Service.DoesNotExist as err: + raise NotFound("Service not found.") from err + + try: + operator = models.Operator.objects.get(id=self.kwargs["operator_id"]) + except models.Operator.DoesNotExist as err: + raise NotFound("Operator not found.") from err
♻️ Duplicate comments (2)
src/frontend/src/features/ui/components/service/ServiceBlock.tsx (1)
134-149: Alignaria-labelwith tooltip and adjust wrapper semanticsThe tooltip already distinguishes between externally managed and non-activatable services, but the
aria-labelstays hardcoded to the “externally managed” message:aria-label={t("organizations.services.externally_managed")}It should reuse the same condition as the tooltip so screen-reader users get the right reason:
- aria-label={t("organizations.services.externally_managed")} + aria-label={t( + isExternallyManaged + ? "organizations.services.externally_managed" + : "organizations.services.cannot_activate" + )}Also, since this wrapper has
role="button"andtabIndex={0}but no click/key handlers, it’s effectively non-interactive; you could either drop therole/tabIndexor addaria-disabledand key handlers if you want it to behave like a (disabled) button.src/backend/core/api/viewsets/service.py (1)
97-110: Graceful 404 when organization ID is invalid.Catching
Organization.DoesNotExistinget_serializer_contextand re‑raisingNotFound("Organization not found.")cleanly converts badorganization_idpath params into a 404 and guarantees the serializer always sees a validorganizationin its context; this addresses the earlier unhandled exception risk.
🧹 Nitpick comments (4)
.github/workflows/tests.yml (1)
20-21: Consider removing network setup from the lint-back job.Linting is a static analysis task that doesn't typically require Docker network setup. The network setup step appears to be unnecessary overhead in this job.
lint-back: runs-on: ubuntu-latest steps: - name: Checkout repository uses: actions/checkout@v4 - - name: Run network setup - run: make create-docker-network - name: Create env files run: make create-env-filesIf linting does require network setup for some reason (e.g., to resolve DNS or access services), then this change can be safely ignored.
src/frontend/src/features/ui/components/service/ServiceBlock.tsx (2)
26-39: TightenonChangeSubscriptiontyping and drop theascastHook logic looks good, but you can simplify the types and avoid the unsafe cast by reusing the mutation’s own parameter type instead of
MutateOptions+as:-import { MutateOptions } from "@tanstack/react-query"; @@ - const onChangeSubscription = ( - data: Partial<ServiceSubscription>, - options?: MutateOptions - ) => { + const onChangeSubscription = ( + data: Partial<ServiceSubscription>, + options?: Parameters<typeof updateOrganizationServiceSubscription>[1] + ) => { @@ - options as Parameters<typeof updateOrganizationServiceSubscription>[1] + options ); };This keeps
onChangeSubscriptionexactly in sync with the mutation signature and removes one import.
172-185: Addrel="noreferrer noopener"on external link opened in new tabThe plain
<a>usestarget="_blank"without arel, which can allow reverse tabnabbing:<a target="_blank" href={service.url}>Consider:
- <a target="_blank" href={service.url}> + <a target="_blank" rel="noreferrer noopener" href={service.url}>This keeps behavior while hardening the link.
src/backend/core/api/serializers.py (1)
285-307: Config whitelisting appropriately limits exposed data.Exposing
configvia computed dicts that only returnidps(operator) andhelp_center_url(service) is a solid improvement over returning the raw JSON and significantly reduces the risk of leaking secrets. If more config keys need selective exposure later, consider extracting the whitelist logic into a small shared helper to keep behaviour consistent across serializers.Based on learnings, this aligns with the guidance to avoid exposing sensitive configuration data.
Also applies to: 313-337
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/tests.yml(1 hunks)src/backend/core/api/serializers.py(6 hunks)src/backend/core/api/viewsets/service.py(2 hunks)src/frontend/src/features/ui/components/service/ServiceBlock.tsx(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/backend/**/*.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
src/backend/**/*.py: Use Django’s ORM for database interactions; avoid raw SQL unless strictly necessary for performance
Use Django’s built-in user model and authentication framework for user management
Log expected and unexpected application actions with appropriate log levels
Respect log level semantics: DEBUG (diagnostics), INFO (normal events), WARN (unexpected but continuing), ERROR (functional failure)
Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Files:
src/backend/core/api/viewsets/service.pysrc/backend/core/api/serializers.py
src/backend/**/serializers.py
📄 CodeRabbit inference engine (.cursor/rules/django-python.mdc)
Use DRF serializers for JSON responses
Files:
src/backend/core/api/serializers.py
🧠 Learnings (2)
📚 Learning: 2025-10-03T23:40:57.889Z
Learnt from: CR
Repo: suitenumerique/st-deploycenter PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-10-03T23:40:57.889Z
Learning: Applies to src/backend/**/serializers.py : Use DRF serializers for JSON responses
Applied to files:
src/backend/core/api/serializers.py
📚 Learning: 2025-10-03T23:40:57.889Z
Learnt from: CR
Repo: suitenumerique/st-deploycenter PR: 0
File: .cursor/rules/django-python.mdc:0-0
Timestamp: 2025-10-03T23:40:57.889Z
Learning: Applies to src/backend/**/*.py : Do not log sensitive information (authorization tokens, passwords, financial/health data, PII)
Applied to files:
src/backend/core/api/serializers.py
🧬 Code graph analysis (3)
src/backend/core/api/viewsets/service.py (3)
src/frontend/src/features/api/Repository.ts (2)
Organization(33-45)ServiceSubscription(47-54)src/backend/core/models.py (2)
Organization(329-564)ServiceSubscription(788-917)src/backend/core/authentication/views.py (1)
get(116-137)
src/frontend/src/features/ui/components/service/ServiceBlock.tsx (4)
src/frontend/src/pages/operators/[operator_id]/organizations/[id].tsx (1)
Organization(16-122)src/frontend/src/features/api/Repository.ts (1)
updateOrganizationServiceSubscription(159-174)src/frontend/src/hooks/useQueries.tsx (1)
useMutationUpdateOrganizationServiceSubscription(87-117)src/frontend/src/features/layouts/components/GlobalLayout.tsx (1)
useOperatorContext(57-63)
src/backend/core/api/serializers.py (1)
src/backend/core/models.py (15)
Meta(62-63)Meta(174-177)Meta(227-231)Meta(318-323)Meta(488-500)Meta(596-604)Meta(640-649)Meta(753-760)Meta(832-838)Meta(979-997)Meta(1062-1069)Operator(188-282)ServiceSubscription(788-917)mail_domain(532-535)get_mail_domain_status(537-564)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test-back
🔇 Additional comments (6)
.github/workflows/tests.yml (2)
9-9: Cosmetic YAML quoting change.The quoted wildcard syntax
"*"is functionally equivalent to the unquoted'*'and is a valid YAML style preference.
32-33: Network setup step appropriately added to test-back job.The "Run network setup" step is correctly placed before environment file creation in the test-back job, which aligns with the backend changes that introduce service dependencies and ProConnect-related logic requiring Docker network configuration during tests.
src/frontend/src/features/ui/components/service/ServiceBlock.tsx (1)
92-131: Switch activation/deactivation flow looks consistentThe optimistic update pattern for both activation and deactivation (setting
checkedimmediately and reverting inonError) is consistent and should feel snappy for users while still recovering correctly on failures.src/backend/core/api/serializers.py (2)
436-442: Organization serialization enrichment with mail-domain fields is consistent.Injecting
mail_domainandmail_domain_statusfrominstance.get_mail_domain_status()into_representationcleanly augments the API without changing the DB schema and matches the frontendOrganizationshape; this looks good.
476-482: Explicit requirement fororganizationin serializer context is a good safety guard.Enforcing
"organization"inself.contextand raising a clearValueErrorif it’s missing makes misuse ofOrganizationServiceSerializerfail fast and aligns withOrganizationServiceViewSet.get_serializer_context()now always providing that object.src/backend/core/api/viewsets/service.py (1)
201-210: Persisting validatedmetadataon subscription creation is correct.Using
serializer.validated_data.get("metadata", {})when creating theServiceSubscriptionensures any provided metadata is saved while preserving the{}default when omitted, and keeps the viewset aligned with the updated serializer contract.
Todo left
Test
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.