Conversation
…logic and fixing Tests logic
…h ReimburseSelectionModal, update event handling, and modify UI components for improved user experience
- Modified TransactionModal to include 'rimborso_esncard' in deletable transaction types and negative types. - Enhanced Profile component to support revoking the latest ESNcard, including confirmation dialog and API integration for deletion. - Updated transactionConfigs to include configuration for 'rimborso_esncard'. - Added display name for 'rimborso_esncard' in displayAttributes.
…ove error messaging; update maintenance notification polling interval to 5 minutes; replace build assets with new versions.
- Added row-level locking for waiting lists to prevent race conditions during subscription processing. - Enhanced sold-out messages for better user experience in event forms and payment pages. - Updated permission handling for event-related actions to use constants for better maintainability. - Improved error handling in profile and treasury views to use a consistent internal error message. - Refactored various components in the frontend to improve code readability and maintainability, including the use of `PropTypes` for type checking. - Optimized transaction handling in treasury views, including the use of `Set` for better performance in type checks. - Fixed issues with WhatsApp field validation in forms and ensured proper handling of numeric inputs. - Updated various components to use modern JavaScript features like `replaceAll` and `at` for better clarity and performance.
Prepare to deploy
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
| Filename | Overview |
|---|---|
| backend/events/views.py | Major refactor: adds sold-out/capacity-blocking logic for payments with row-level locking, and cleans up helper functions; one style issue (constant defined mid-imports). |
| backend/treasury/views.py | Adds DELETE endpoint for ESNcard revocation with automatic refund transaction; fixes HTTP 401→403 for unauthorized responses; adds N+1 fix for reimbursable deposits query. |
| frontend/src/Components/events/ReimburseSelectionModal.jsx | New unified reimbursement modal replacing separate quota and deposit modals; minor missing dependency in useEffect for refundItems. |
| frontend/src/Components/events/EditAnswersModal.jsx | Case 'c' (choice fields) now falls through to case 's' (plain text) — dropdown select rendering is broken for choice-type fields. |
| frontend/src/Pages/events/EventPayment.jsx | Adds payment_blocked handling (sold-out state); consolidates status fetch and checkout flow effects; improves error message propagation from the API. |
| frontend/src/Pages/profiles/Profile.jsx | Adds ESNcard revocation flow (Board-only) with confirmation dialog and treasury refund; updates content-manager toggle to reflect Board-only implicit permission. |
| .github/workflows/deploy-production.yml | Moves GitHub permissions from global to per-job scope (least-privilege improvement); changes version bump strategy from PATCH to MINOR for this release. |
Sequence Diagram
sequenceDiagram
participant User
participant Frontend as EventPayment.jsx
participant API as Backend API
participant SumUp
User->>Frontend: Navigate to /event/:id/pay
Frontend->>API: GET /subscription/:id/payment_status/
API-->>Frontend: { sumup_checkout_id, payment_blocked, payment_blocked_reason }
alt payment_blocked == true
Frontend-->>User: Show sold-out alert message
else checkout available
Frontend->>Frontend: setCheckoutId(sumup_checkout_id || stateCheckout)
Frontend->>SumUp: Load SumUpCard widget
SumUp-->>User: Render payment widget
User->>SumUp: Enter card & pay
SumUp-->>Frontend: onResponse(type="success", payload)
Frontend->>API: POST /subscription/:id/process_payment/ (with token)
Note over API: select_for_update() lock on subscription + lists
alt capacity still available
API->>SumUp: Process checkout
SumUp-->>API: payment result
API-->>Frontend: { status: "PAID" }
Frontend-->>User: Redirect to success page
else lists became full (race)
API-->>Frontend: { status: "BLOCKED", error: "sold_out" } HTTP 409
Frontend-->>User: Show sold-out message
end
end
Prompt To Fix All With AI
This is a comment left during a code review.
Path: frontend/src/Components/events/ReimburseSelectionModal.jsx
Line: 152-163
Comment:
**`refundItems` missing from `useEffect` dependency array**
`makeInitialSelection(refundItems)` is called inside the effect, but `refundItems` is not listed as a dependency. When the modal is opened for the same subscription but with updated payment statuses (e.g., after a successful payment polling), `refundItems` will be recomputed by `useMemo` yet the effect won't re-run, leaving `selectedItems` initialised from stale data. Add `refundItems` to the deps array:
```suggestion
}, [open, subscription?.id, refundItems]);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: backend/events/views.py
Line: 24-26
Comment:
**Module-level constant placed in the middle of import block**
`PERM_VIEW_EVENT` is defined between two `from … import` lines. PEP 8 and standard project style expect module constants to be grouped after all imports, not interleaved with them. Move the constant (and `PAYMENT_SOLD_OUT_MESSAGE` below) to right after the last import statement.
```suggestion
PERM_VIEW_EVENT = 'events.view_event'
from rest_framework.exceptions import ValidationError as DRFValidationError
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: frontend/src/Components/events/EventListAccordions.jsx
Line: 698
Comment:
**Stale `onOpenReimburseDeposits` reference in `useMemo` deps**
`onOpenReimburseDeposits` is still listed in the `useMemo` dependency array on this line, but the individual row-level "Rimborsa Cauzione" button that called it was removed. The prop is no longer referenced inside the memo callback, making this dependency spurious. If it is still used for a bulk action somewhere in the component this comment does not apply, but if not, removing it keeps the deps honest and avoids unnecessary recomputation.
```suggestion
}, [data, hasDeposit, hasQuota, isBoardMember, isOrganizer, onOpenEditAnswers, onOpenReimburseMenu, showFormColumns, showAdditionalColumns, showProfileColumns]);
```
How can I resolve this? If you propose a fix, please make it concise.
---
This is a comment left during a code review.
Path: frontend/src/Components/events/EditAnswersModal.jsx
Line: 117-120
Comment:
**Choice-type fields lose dropdown rendering**
The `case 'c':` label is still present but its `return` statement was removed, so it now falls through to `case 's'` (the plain text input). Any form field with type `'c'` will render as a free-text `TextField` instead of a `<TextField select>` with `<MenuItem>` options, breaking the dropdown select behavior for choice fields in the edit modal.
If `'c'` and `'s'` are intentionally consolidated into the same plain-text renderer, remove the `case 'c':` label entirely. If the dropdown behavior must be preserved for choice-type fields, restore the select renderer that was deleted.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Merge branch 'main' into staging" | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Prepares the staging branch for deployment by tightening authorization semantics (403 vs 401), adding ESNcard revocation with treasury refund tracking, improving event payment “sold-out” handling, and consolidating documentation.
Changes:
- Treasury: add ESNcard revoke (
DELETE /esncard/<pk>/) with automaticrimborso_esncardtransaction, plus related UI and transaction-type support. - Events: block online payment when Main+Waiting are full (while still accepting form submissions), and introduce a unified reimbursement-selection modal in the event UI.
- Ops/docs: standardize permission responses (403), update/replace documentation set, and adjust production deploy workflow versioning/permissions.
Reviewed changes
Copilot reviewed 56 out of 58 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| README_SHARED_LISTS_FINAL.md | Removed obsolete shared-lists implementation summary doc. |
| frontend/src/utils/useMaintenanceNotification.js | Switch API_HOST access + slow down polling interval. |
| frontend/src/utils/displayAttributes.jsx | Add display label for rimborso_esncard. |
| frontend/src/Pages/treasury/AccountsList.jsx | Use Number.parseFloat for numeric check. |
| frontend/src/Pages/profiles/Profile.jsx | Add ESNcard revoke flow + content-manager toggle tweak. |
| frontend/src/Pages/events/EventPayment.jsx | Handle blocked payment state and improved error parsing. |
| frontend/src/Pages/events/Event.jsx | Replace quota-only reimburse modal with unified selection modal. |
| frontend/src/Pages/ESNerForm.jsx | Fix WhatsApp validation when “same as phone” is enabled. |
| frontend/src/Pages/ErasmusForm.jsx | Fix WhatsApp validation; minor semester/date helper improvements. |
| frontend/src/data/transactionConfigs.js | Add config for rimborso_esncard. |
| frontend/src/Components/treasury/TransactionModal.jsx | Use Set for type checks; include rimborso_esncard in delete/negative rules. |
| frontend/src/Components/treasury/AccountsDash.jsx | Use Number.parseFloat for numeric check. |
| frontend/src/Components/MaintenanceBanner.jsx | Add PropTypes for banner props. |
| frontend/src/Components/events/SubscriptionModal.jsx | Correct matricola status logic for ESNer vs non-ESNer. |
| frontend/src/Components/events/ReimburseSelectionModal.jsx | New modal to select reimburse items (quota/services/deposit). |
| frontend/src/Components/events/EventListAccordions.jsx | Unify reimburse action into single button + export copy improvements. |
| frontend/src/Components/events/EditAnswersModal.jsx | Simplify renderFieldInput call site. |
| frontend/build/index.html | Update built asset reference hash. |
| backend/users/views.py | Minor signature/doc tweaks for finance permissions. |
| backend/users/test_integration.py | Update expected unauthorized status to 403. |
| backend/users/serializers.py | Restrict manage_content virtual permission to Board/explicit flag. |
| backend/users/models.py | Clarify can_manage_content semantics in comment. |
| backend/treasury/views.py | Add ESNcard DELETE revoke flow + standardize unauthorized responses to 403 + query optimization. |
| backend/treasury/tests.py | Add ESNcard revoke tests; update expected 403 statuses; add external-subscriber coverage. |
| backend/treasury/serializers.py | Factor Drive scope + default mimetype constants. |
| backend/treasury/models.py | Add RIMBORSO_ESNCARD transaction type + validation rules. |
| backend/README_Models.md | Rewrite/translate to English and refresh descriptions. |
| backend/README_Content.md | Rewrite/translate + update permission model narrative. |
| backend/README_APIs.md | Update API docs; add ESNcard revoke endpoint description. |
| backend/Project Documentation/TEST_COVERAGE_REPORT.md | New consolidated test coverage report. |
| backend/Project Documentation/07_MAINTENANCE_MODULE.md | New maintenance module spec. |
| backend/Project Documentation/06_INTEGRATION_E2E.md | New cross-module E2E spec baseline. |
| backend/Project Documentation/05_CONTENT_MODULE.md | New content module spec. |
| backend/Project Documentation/04_TREASURY_MODULE.md | New treasury module spec. |
| backend/Project Documentation/03_EVENTS_MODULE.md | New events module spec. |
| backend/Project Documentation/02_PROFILES_MODULE.md | New profiles module spec. |
| backend/Project Documentation/01_USERS_MODULE.md | New users module spec. |
| backend/Project Documentation/00_OVERVIEW.md | New system-wide technical overview. |
| backend/profiles/views.py | Introduce shared internal-error message constant. |
| backend/profiles/tests.py | Minor test cleanups (unused vars). |
| backend/events/views.py | Add sold-out payment blocking + extracted helpers/constants; minor permission const. |
| backend/events/tests.py | Add tests for sold-out form-submit and blocked payment/status. |
| backend/events/models.py | Minor service validation loop adjustment. |
| backend/events.md | Removed outdated legacy notes doc. |
| backend/docs/test_specifications/06_INTEGRATION_E2E.md | Removed old test spec doc (superseded by new consolidated docs). |
| backend/docs/test_specifications/05_CONTENT_MODULE.md | Removed old test spec doc (superseded). |
| backend/docs/test_specifications/01_USERS_MODULE.md | Removed old test spec doc (superseded). |
| backend/docs/test_specifications/00_OVERVIEW.md | Removed old test spec overview (superseded). |
| backend/docs/TEST_COVERAGE_REPORT.md | Removed old coverage report (superseded). |
| backend/content/views.py | Centralize content-manager auth (_can_manage_content) + tighten permissions. |
| backend/content/tests.py | Update tests for new content-manager permission rules. |
| backend/backend/db_audit.py | Minor simplification of pk_set sorting. |
| .github/workflows/deploy-production.yml | Job-scoped permissions + bump MINOR versioning logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/README_APIs.md (1)
391-400:⚠️ Potential issue | 🟡 MinorDuplicate parameters in transaction creation documentation.
The
accountparameters are listed twice in the parameter table, which appears to be a copy-paste error.📝 Proposed fix
| name | type | mandatory | description | |----------|--------|-----------|-------------| | account | string | yes | | | email | string | yes | -| email | string | yes | -| account | string | yes | | event_id | string | no | | amount | float | yes | | reason | string | yes |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/README_APIs.md` around lines 391 - 400, The parameter table in backend/README_APIs.md for the transaction creation API contains duplicated rows for "email" and "account"; remove the duplicate rows so each parameter appears exactly once (keep one "account" and one "email" entry) and verify the table still lists all required parameters (e.g., event_id, amount, reason) correctly; update the table header/body so there are no repeated parameter names and the "mandatory" and "description" cells remain accurate for the unique parameters.
🧹 Nitpick comments (4)
backend/treasury/serializers.py (1)
68-68: Optional: collapse repeated Drive client bootstrap into one helper.Credential/service setup is now repeated in three functions with identical lines. Extracting a small helper would reduce future drift risk.
♻️ Suggested refactor
+def _build_drive_service(): + credentials = service_account.Credentials.from_service_account_file( + settings.GOOGLE_SERVICE_ACCOUNT_FILE, + scopes=[GOOGLE_DRIVE_SCOPE], + ) + return build('drive', 'v3', credentials=credentials) def upload_receipt_to_drive(receipt_file, user, instance_time, prefix): @@ - SERVICE_ACCOUNT_FILE = settings.GOOGLE_SERVICE_ACCOUNT_FILE - credentials = service_account.Credentials.from_service_account_file( - SERVICE_ACCOUNT_FILE, - scopes=[GOOGLE_DRIVE_SCOPE] - ) - service = build('drive', 'v3', credentials=credentials) + service = _build_drive_service()Also applies to: 126-126, 186-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/treasury/serializers.py` at line 68, Several functions in serializers.py repeat the same Google Drive credential/service bootstrap; extract that duplicated logic into a single helper (e.g., build_drive_service or get_drive_service) that returns the authenticated service (and credentials if needed), then call that helper from the three places that currently construct credentials/service (the bootstrap blocks used before upload/download operations). Ensure the new helper preserves existing behavior (scopes, key loading, exception handling) and update the callers to use the returned service instead of duplicating setup code.backend/users/views.py (1)
375-380: Remove the unreachable ESNer re-check.Line 370 already rejects every non-ESNer target for PATCH, so the
content_fieldsbranch here can never run. If the intent is per-field behavior later, move the broader guard down; otherwise this duplicate check should be deleted.Suggested cleanup
finance_fields = {k for k in request.data if k in ['can_manage_casse', 'can_view_casse_import']} - content_fields = {k for k in request.data if k == 'can_manage_content'} if finance_fields and not _in_group(target, 'Aspiranti'): return Response({'error': 'Permessi casse applicabili solo agli Aspiranti.'}, status=400) - if content_fields and not target.profile.is_esner: - return Response({'error': 'Ruolo Content Manager applicabile solo agli ESNer.'}, status=400)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/users/views.py` around lines 375 - 380, The check against target.profile.is_esner inside the content_fields branch is unreachable because a prior PATCH guard already rejects non-ESNer targets; remove the redundant branch (the content_fields conditional that returns the ESNer error) to avoid dead code, or if you intended per-field validation instead, move the earlier broad non-ESNer PATCH guard below the per-field checks (content_fields, finance_fields) so each field can be validated individually; update logic around request.data, content_fields, _in_group, and target.profile.is_esner accordingly.backend/users/serializers.py (1)
73-75: Extract the content-manager rule into one shared helper.The same Board-or-
can_manage_contentpredicate now lives inget_permissions(),get_effective_can_manage_content(),backend/content/views.py, andbackend/users/views.py. Centralizing it would keep the login payload,effective_*flags, and write authorization from drifting again.Also applies to: 96-97
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/users/serializers.py` around lines 73 - 75, The Board-or-can_manage_content predicate is duplicated; extract it into a single helper (e.g., a new function can_manage_content(user_or_obj) or user_has_manage_content(user)) and replace the inline checks in get_permissions(), get_effective_can_manage_content(), and the permission checks inside content/views and users/views with calls to that helper; ensure the helper accepts the same inputs used today (user or object with groups and can_manage_content attribute) and returns a boolean so existing code can simply add 'manage_content' when true.backend/events/views.py (1)
26-28: Minor: Constant defined between imports.
PERM_VIEW_EVENTis defined between import statements. Consider moving it after all imports (around line 50) alongsidePAYMENT_SOLD_OUT_MESSAGEfor better organization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/events/views.py` around lines 26 - 28, PERM_VIEW_EVENT is declared in the middle of import statements; move the PERM_VIEW_EVENT constant declaration so all imports remain grouped together and place the constant with other module-level constants (e.g., next to PAYMENT_SOLD_OUT_MESSAGE) after the import block to keep file organization consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/deploy-production.yml:
- Around line 34-36: The inline comment is incorrect: update the comment that
currently says "Incrementa PATCH - MINOR - MAJOR" to reflect that this block
performs a minor version bump (it sets PATCH=0 and increments MINOR). Edit the
comment near the PATCH and MINOR assignments so it clearly states "Increment
MINOR and reset PATCH" (or similar) to match the behavior of the PATCH and MINOR
variables.
In `@backend/Project` Documentation/02_PROFILES_MODULE.md:
- Around line 43-65: The docs show a conflicting base path: the module base path
is declared as "/backend/" but the email verification route is listed as
"/api/profile/verify-email/<uid>/<token>/", causing confusion; update the
documentation so all endpoints use the same base path (e.g., change
"/api/profile/verify-email/<uid>/<token>/" to
"/backend/profile/verify-email/<uid>/<token>/") or explicitly state the
exception under "3.2 Creation and Verification" (mentioning
"/profile/initiate-creation/" and "/profile/<pk>/manual-verify-email/" as
examples) so readers know the verify-email route uses the "/api/" prefix.
In `@backend/Project` Documentation/03_EVENTS_MODULE.md:
- Around line 52-60: Update the documentation to clarify that the DB constraint
`unique(profile, event)` only enforces uniqueness for non-null `profile` values
and does not prevent duplicate external subscriptions where `profile` is null;
explicitly state that deduplication of external subscribers (the `external_*`
fields) is implemented via application-level validation rather than the database
constraint, and adjust the section listing `profile` (nullable) and `external_*`
fields and the "Constraint: unique(profile, event)" note to reflect this
separation (also mirror the clarification where external deduplication is
mentioned later in the document).
In `@backend/Project` Documentation/04_TREASURY_MODULE.md:
- Around line 113-120: The wording is ambiguous about transaction PATCH/DELETE
rights: update the doc entry for transactions to say that can_manage_casse (and
group-based treasury managers) are only allowed to change transaction status
(the relaxed path implemented in backend/treasury/views.py around the
transaction PATCH/DELETE logic), whereas full transaction edits or deletes still
require the standard edit permission; refer to the view that enforces
status-only changes to mirror the exact restriction in the text and mention the
specific permission name can_manage_casse.
- Around line 163-204: The document mixes Italian headings and phrases into an
English spec; replace all Italian terms with clear English equivalents (e.g.,
change "Depositi" to "Deposits", "Orchestrazione UI unificata" to "Unified UI
Orchestration (single icon)", "rimborso_cauzione" and "rimborso_esncard" usages
to "deposit_refund" / "esncard_refund" or consistent English identifiers, and
translate phrases like "identita attore e ownership richieste" to "actor
identity and ownership requirements" and "avvisi operativi" to "operational
notifications"). Ensure all section titles and inline bullet points use English,
keep original identifiers like transactions, transactions_export,
reimburse_quota, reimburse_deposits unchanged, and update any Italian
notes/comments so the whole file is consistently in English.
In `@backend/Project` Documentation/07_MAINTENANCE_MODULE.md:
- Around line 22-31: The payload schema is missing the message field referenced
elsewhere: update the documented payload schema to include message (string,
nullable or default) alongside notification_id and triggered_at, and update the
invariants to state that message contains the notification text (defaults to
operational text when not provided) and that message is present/used only when
notification_id is non-null; ensure the examples and any references to message
in the doc are consistent with the new schema for notification_id, triggered_at,
and message.
In `@backend/README_Models.md`:
- Around line 4-28: Update the Markdown heading hierarchy so the document flows
from the top-level "# Events" to second-level sections: change the "Main
Features" and "Benefits" headings to use "##" instead of "###", and ensure the
"# Events" header precedes them so there is no jump from a top-level header to a
third-level header; adjust the headings for "Main Features" and "Benefits" in
README_Models.md accordingly.
In `@backend/treasury/models.py`:
- Around line 114-118: The RIMBORSO_ESNCARD branch in Transaction.clean()
currently raises ValueError and only checks relations, allowing non-negative
amounts which would move balances incorrectly; change the validation to raise
django.core.exceptions.ValidationError instead of ValueError and add a check
that self.amount is less than 0 for when self.type ==
self.TransactionType.RIMBORSO_ESNCARD and self.subscription/ self.esncard are
validated (use the existing conditionals in clean()), so invalid ESNcard refunds
produce form-friendly ValidationError messages; while here, consider replacing
other ValueError usages in clean() with ValidationError for consistent Django
validation handling.
In `@frontend/src/Components/events/EventListAccordions.jsx`:
- Around line 440-452: The actions column currently hides service reimbursements
when the event-level data.services array is empty; remove that gate and
determine service refund availability per-row. Change the hasAnyRefundCategory
calculation to not rely on data.services.length, and inside the Cell (function
using row.original / sub) compute canReimburseServices from the subscription's
own data (e.g. sub.selected_services?.length > 0 && sub.status_services ===
'paid') instead of the event-level data.services; ensure canReimburseQuota and
canReimburseDeposit remain unchanged and that the overall canReimburseAny uses
these per-row flags so the ReimburseSelectionModal and refund actions appear
whenever the subscription itself has refundable services.
In `@frontend/src/Components/events/ReimburseSelectionModal.jsx`:
- Around line 219-220: The confirmation text and header currently use
subscription.profile_name only, which leaves recipient blank for external
subscriptions; update both places (the message construction using
accountName/selectedLabels/selectedAmount and the header logic referenced around
lines 355-359) to prefer subscription.profile_name but fallback to
subscription.external_name when profile_name is missing/empty (e.g., const
recipientName = subscription.profile_name || subscription.external_name ||
'N/A'), then use recipientName wherever subscription.profile_name was used.
In `@frontend/src/Components/events/SubscriptionModal.jsx`:
- Around line 208-209: The code uses profileData.latest_esncard when setting
setProfileHasEsncard, but the backend returns esncard_number (not
latest_esncard), so subscriptions may be incorrectly blocked; update the check
in SubscriptionModal.jsx to look for either profileData.latest_esncard OR
profileData.esncard_number (or normalize the field into a single variable when
loading profile data) so setProfileHasEsncard reflects the actual payload shape;
adjust any related logic that reads profileData.latest_esncard to use the
normalized value to avoid API/field mismatches.
In `@frontend/src/Pages/profiles/Profile.jsx`:
- Around line 919-923: The success toast always claims a treasury refund was
registered; update the onSuccess callback used with setPopup to conditionally
choose the message based on res?.refund_transaction_id (or another explicit
field on the response) instead of always falling back to the refund text—i.e.,
inspect refund_transaction_id inside the onSuccess handler and set the popup
message to the “refund registered” text only when refund_transaction_id is
non-null, otherwise use a generic “revoked”/no-refund message; keep the existing
setPopup API and state/id usage intact.
---
Outside diff comments:
In `@backend/README_APIs.md`:
- Around line 391-400: The parameter table in backend/README_APIs.md for the
transaction creation API contains duplicated rows for "email" and "account";
remove the duplicate rows so each parameter appears exactly once (keep one
"account" and one "email" entry) and verify the table still lists all required
parameters (e.g., event_id, amount, reason) correctly; update the table
header/body so there are no repeated parameter names and the "mandatory" and
"description" cells remain accurate for the unique parameters.
---
Nitpick comments:
In `@backend/events/views.py`:
- Around line 26-28: PERM_VIEW_EVENT is declared in the middle of import
statements; move the PERM_VIEW_EVENT constant declaration so all imports remain
grouped together and place the constant with other module-level constants (e.g.,
next to PAYMENT_SOLD_OUT_MESSAGE) after the import block to keep file
organization consistent.
In `@backend/treasury/serializers.py`:
- Line 68: Several functions in serializers.py repeat the same Google Drive
credential/service bootstrap; extract that duplicated logic into a single helper
(e.g., build_drive_service or get_drive_service) that returns the authenticated
service (and credentials if needed), then call that helper from the three places
that currently construct credentials/service (the bootstrap blocks used before
upload/download operations). Ensure the new helper preserves existing behavior
(scopes, key loading, exception handling) and update the callers to use the
returned service instead of duplicating setup code.
In `@backend/users/serializers.py`:
- Around line 73-75: The Board-or-can_manage_content predicate is duplicated;
extract it into a single helper (e.g., a new function
can_manage_content(user_or_obj) or user_has_manage_content(user)) and replace
the inline checks in get_permissions(), get_effective_can_manage_content(), and
the permission checks inside content/views and users/views with calls to that
helper; ensure the helper accepts the same inputs used today (user or object
with groups and can_manage_content attribute) and returns a boolean so existing
code can simply add 'manage_content' when true.
In `@backend/users/views.py`:
- Around line 375-380: The check against target.profile.is_esner inside the
content_fields branch is unreachable because a prior PATCH guard already rejects
non-ESNer targets; remove the redundant branch (the content_fields conditional
that returns the ESNer error) to avoid dead code, or if you intended per-field
validation instead, move the earlier broad non-ESNer PATCH guard below the
per-field checks (content_fields, finance_fields) so each field can be validated
individually; update logic around request.data, content_fields, _in_group, and
target.profile.is_esner accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0fb178bc-929c-448c-b34c-a5267fd14735
📒 Files selected for processing (58)
.github/workflows/deploy-production.ymlREADME_SHARED_LISTS_FINAL.mdbackend/Project Documentation/00_OVERVIEW.mdbackend/Project Documentation/01_USERS_MODULE.mdbackend/Project Documentation/02_PROFILES_MODULE.mdbackend/Project Documentation/03_EVENTS_MODULE.mdbackend/Project Documentation/04_TREASURY_MODULE.mdbackend/Project Documentation/05_CONTENT_MODULE.mdbackend/Project Documentation/06_INTEGRATION_E2E.mdbackend/Project Documentation/07_MAINTENANCE_MODULE.mdbackend/Project Documentation/TEST_COVERAGE_REPORT.mdbackend/README_APIs.mdbackend/README_Content.mdbackend/README_Models.mdbackend/backend/db_audit.pybackend/content/tests.pybackend/content/views.pybackend/docs/TEST_COVERAGE_REPORT.mdbackend/docs/test_specifications/00_OVERVIEW.mdbackend/docs/test_specifications/01_USERS_MODULE.mdbackend/docs/test_specifications/02_PROFILES_MODULE.mdbackend/docs/test_specifications/03_EVENTS_MODULE.mdbackend/docs/test_specifications/04_TREASURY_MODULE.mdbackend/docs/test_specifications/05_CONTENT_MODULE.mdbackend/docs/test_specifications/06_INTEGRATION_E2E.mdbackend/events.mdbackend/events/models.pybackend/events/tests.pybackend/events/views.pybackend/profiles/tests.pybackend/profiles/views.pybackend/treasury/models.pybackend/treasury/serializers.pybackend/treasury/tests.pybackend/treasury/views.pybackend/users/models.pybackend/users/serializers.pybackend/users/test_integration.pybackend/users/views.pyfrontend/build/assets/index-BKm4YlQa.jsfrontend/build/assets/index-BMkv-w2E.jsfrontend/build/index.htmlfrontend/src/Components/MaintenanceBanner.jsxfrontend/src/Components/events/EditAnswersModal.jsxfrontend/src/Components/events/EventListAccordions.jsxfrontend/src/Components/events/ReimburseSelectionModal.jsxfrontend/src/Components/events/SubscriptionModal.jsxfrontend/src/Components/treasury/AccountsDash.jsxfrontend/src/Components/treasury/TransactionModal.jsxfrontend/src/Pages/ESNerForm.jsxfrontend/src/Pages/ErasmusForm.jsxfrontend/src/Pages/events/Event.jsxfrontend/src/Pages/events/EventPayment.jsxfrontend/src/Pages/profiles/Profile.jsxfrontend/src/Pages/treasury/AccountsList.jsxfrontend/src/data/transactionConfigs.jsfrontend/src/utils/displayAttributes.jsxfrontend/src/utils/useMaintenanceNotification.js
💤 Files with no reviewable changes (11)
- backend/docs/test_specifications/05_CONTENT_MODULE.md
- backend/events.md
- backend/docs/TEST_COVERAGE_REPORT.md
- README_SHARED_LISTS_FINAL.md
- backend/events/models.py
- backend/docs/test_specifications/04_TREASURY_MODULE.md
- backend/docs/test_specifications/01_USERS_MODULE.md
- backend/docs/test_specifications/02_PROFILES_MODULE.md
- backend/docs/test_specifications/00_OVERVIEW.md
- backend/docs/test_specifications/06_INTEGRATION_E2E.md
- backend/docs/test_specifications/03_EVENTS_MODULE.md
📜 Review details
🧰 Additional context used
🪛 GitHub Check: CodeQL
backend/events/views.py
[warning] 2267-2267: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
[warning] 2287-2287: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 LanguageTool
backend/Project Documentation/04_TREASURY_MODULE.md
[grammar] ~162-~162: Ensure spelling is correct
Context: ...lignment ### 5.4 Event Reimbursements Depositi: 1. bulk subscription_ids input 2. origina...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~203-~203: Ensure spelling is correct
Context: ...sed in reimbursements - profiles/users: identita attore e ownership richieste - notifica...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
backend/Project Documentation/01_USERS_MODULE.md
[grammar] ~18-~18: Ensure spelling is correct
Context: ... ## 2. Data Model and Identity ### 2.1 Entita User User extends AbstractBaseUser...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[style] ~97-~97: To form a complete sentence, be sure to include a subject.
Context: ...y to Aspiranti. 3. can_manage_content can be assigned only to ESNer profiles. 4. ...
(MISSING_IT_THERE)
backend/Project Documentation/00_OVERVIEW.md
[grammar] ~72-~72: Ensure spelling is correct
Context: ...icit privileges across most modules. 2. Attivi can operate in treasury areas based on ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~73-~73: Ensure spelling is correct
Context: ...areas based on endpoint permissions. 3. Aspiranti require explicit flags for extra capabi...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
backend/Project Documentation/02_PROFILES_MODULE.md
[grammar] ~35-~35: Ensure spelling is correct
Context: ... - latest_document ### 2.2 Document - relazione: FK su Profile - document type: enum (Pa...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~136-~136: Ensure spelling is correct
Context: ...- name, surname, email - document number - esncard number - phone/whatsapp `esncardValidi...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
backend/README_APIs.md
[grammar] ~72-~72: Ensure spelling is correct
Context: ...he profile's documents, student IDs and ESNcards. ### Update specified profile * `PATCH /prof...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
backend/Project Documentation/03_EVENTS_MODULE.md
[grammar] ~184-~184: Ensure spelling is correct
Context: ... can be shared across multiple events. Invarianti: 1. list capacity is a single cross-event po...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~207-~207: Ensure spelling is correct
Context: ...s 2. fields/services schema validation 3. idempotenza process_payment + webhook 4. `move-subs...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.22.0)
backend/README_Models.md
[warning] 4-4: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
backend/README_APIs.md
[warning] 142-142: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
[warning] 175-175: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
[warning] 231-231: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🔇 Additional comments (43)
backend/treasury/serializers.py (2)
16-17: Good extraction of shared Google Drive constants.Centralizing scope and fallback mimetype removes duplicated literals and makes future updates safer.
64-64: Scope constant usage is consistent across all upload helpers.No correctness or integration concerns here.
Also applies to: 101-101, 163-163
.github/workflows/deploy-production.yml (3)
11-12: Good least-privilege scoping for tag creation.Moving
contents: writeto thecreate-release-tagjob is a clean permission boundary for this workflow step.
53-54: Job-level permissions here look correct.
contents: writeis appropriate for both deploy jobs since they push branches/tags and avoids broader workflow-level write scope.Also applies to: 91-92
144-145: Release job permission scope is appropriate.Granting
contents: writeonly oncreate-github-releaseis consistent with least privilege for release creation.frontend/src/Components/events/EditAnswersModal.jsx (1)
289-289: LGTM.This keeps the additional-fields render path aligned with
renderFieldInput’s declared signature and with the form-fields call site above.frontend/src/utils/displayAttributes.jsx (1)
68-68: New transaction label wiring looks correct.
rimborso_esncardis now mapped in display names and aligns with the new transaction type usage in config/UI.frontend/src/utils/useMaintenanceNotification.js (1)
19-20: Polling update is coherent and low-risk.Using
globalThis.API_HOSTplus a 5-minute interval keeps behavior intact while reducing request churn.Also applies to: 54-55
frontend/src/Components/MaintenanceBanner.jsx (1)
4-4: Prop validation addition is good.
messageandonCloserequirements are now explicit and help catch misuse early.Also applies to: 108-111
frontend/build/index.html (1)
15-15: Build artifact reference update looks fine.This is a straightforward bundle hash refresh in the generated HTML.
backend/backend/db_audit.py (1)
220-220: Nice cleanup in m2m PK serialization.Removing the redundant
list(...)conversion keeps behavior the same and simplifies the expression.frontend/src/data/transactionConfigs.js (1)
10-10: Config addition is consistent with the new transaction type.
rimborso_esncardis correctly wired to display labels and chip color config.frontend/src/Components/treasury/TransactionModal.jsx (1)
16-16: Type-handling refactor is solid.Using
Set.has(...)and includingrimborso_esncardin both deletion and negative-amount paths makes the transaction behavior more consistent and explicit.Also applies to: 25-25, 65-66, 118-119, 167-167, 370-370
backend/users/test_integration.py (1)
376-376: 403 is the correct expectation here.The client is already authenticated in this branch, so this is an authorization denial, not an authentication failure.
frontend/src/Pages/ESNerForm.jsx (1)
156-169: Good fix for hidden WhatsApp validation.Clearing those errors when
sameWAasPhoneis enabled keeps validation consistent with the payload built inhandleSubmit, so the hidden fields no longer block a valid submit.backend/Project Documentation/00_OVERVIEW.md (1)
1-187: Documentation is comprehensive and well-structured.This system overview provides a clear technical baseline covering architecture, domain boundaries, authorization model, and API surface. The structure aligns well with the module documentation index and supports both human developers and AI automation.
backend/README_APIs.md (1)
175-227: ESNcard revoke endpoint documentation is thorough.The documentation clearly describes the Board-only revoke operation, the refund transaction type (
rimborso_esncard), and common error scenarios (401, 404, 409). This aligns well with the backend implementation described in the AI summary.backend/README_Content.md (1)
1-138: Documentation accurately reflects the updated permission model.The content management documentation clearly communicates:
- Write permissions extended to Board members and users with
can_manage_content=True- Removal of
action_typefor special actions- Removal of static fallback content behavior
The strikethrough formatting with "Removed"/"Updated" annotations effectively communicates deprecated behavior.
frontend/src/Pages/events/EventPayment.jsx (2)
55-66: Payment blocking and checkout resolution implemented correctly.The sold-out handling properly intercepts the
payment_blockedresponse before attempting payment, displaying an appropriate message. The checkout ID resolution now correctly prefers the server-sidesumup_checkout_idover the stale navigation state, which is the right approach for recovering the session.
151-165: The error handling pattern correctly handles both cases thatfetchCustomproduces. TheonErrorcallback receives either aResponseobject (with.json()method) for failed API responses, or anErrorobject (with.messageproperty) for network errors or missing auth tokens. The fallback logic appropriately handles both types, so the pattern will not fail silently.fetchCustomnever passes already-parsed objects toonError, making the concern about that scenario invalid.> Likely an incorrect or invalid review comment.backend/events/tests.py (3)
812-840: Comprehensive test for form submission when Main/Waiting lists are full.This test correctly validates the sold-out scenario:
- Form submission succeeds and assigns to Form List
payment_required=Trueandcapacity_blocked=Trueflags are set- No
sumup_checkout_idis created when capacity is blockedThis aligns with the documented behavior that form submission remains allowed while payment is blocked.
1645-1675: Good verification of the reconciliation probe behavior.The test correctly verifies that:
- Payment processing returns 409 BLOCKED when lists are full
_ensure_sumup_transactionsis NOT called (no local accounting when blocked)_process_sumup_checkoutIS called once (reconciliation probe for already-paid edge case)Lines 1674-1675: The comment explaining why
mock_processis still called is helpful for understanding the design intent.
1551-1576: Payment status blocking test covers the essential assertions.The test properly validates the
payment_blocked,payment_blocked_reason, andpayment_blocked_messagefields in the status response when capacity is exhausted.backend/Project Documentation/01_USERS_MODULE.md (1)
1-142: Users module documentation is comprehensive and well-organized.The documentation covers JWT authentication lifecycle, user management APIs, authorization rules, and security considerations. The API contract table format is clear and the business rules are explicitly documented.
backend/Project Documentation/TEST_COVERAGE_REPORT.md (2)
17-26: Test inventory provides useful breadth metric.The methodology note correctly distinguishes between test function counts and line coverage percentage. This is a pragmatic approach for documenting test suite breadth.
Consider periodically validating the counts match actual test files to prevent documentation drift.
52-52: Sold-out handling documentation aligns with implementation.Line 52 accurately documents the new behavior: "sold-out handling: form submission stays allowed in Form List, payment blocked only when Main+Waiting are full" - this matches the test assertions and frontend implementation reviewed in this PR.
backend/Project Documentation/06_INTEGRATION_E2E.md (3)
58-77: Scenario C accurately documents the sold-out payment reconciliation flow.The scenario correctly captures:
- Form submission creates subscription in Form List
payment_blocked=truewithsold_outreason when Main/Waiting fullprocess_paymentreturns409 BLOCKEDin sold-out condition- Test oracles include idempotency, no double accounting, and form submission acceptance
This aligns with the backend implementation and frontend handling reviewed in this PR.
100-117: Scenario E documents ESNcard emission/revocation with proper accounting.The scenario covers the full ESNcard lifecycle including:
- Fee calculation and ESNcard transaction creation
- Board-only revocation with
rimborso_esncardtransaction- Emission transaction preserved for audit
- Edge case blocking (insufficient balance, closed account)
This matches the API documentation in
README_APIs.mdfor the new revoke endpoint.
200-212: Traceability map provides useful cross-reference for debugging.The scenario-to-test mapping enables quick identification of relevant test files when troubleshooting specific flows. The note clarifying this doesn't imply exhaustive edge-case coverage sets appropriate expectations.
backend/events/views.py (8)
2249-2289: Solid payment blocking implementation with proper concurrency handling.The implementation correctly:
- Re-fetches subscription with
select_for_update()to serialize concurrent requests (line 2252)- Double-checks if payment was already processed by SumUp before returning BLOCKED (lines 2263-2268)
- Uses 409 Conflict status for the blocking case (appropriate HTTP semantics)
The race condition between checking capacity and processing payment is properly handled by the row-level lock.
51-54: LGTM!Centralizing the sold-out message as a constant ensures consistent messaging across form submission emails, payment status responses, and payment processing.
144-144: LGTM!Renaming to
_field_namecorrectly signals that the parameter is intentionally unused while maintaining API compatibility.
300-340: Well-designed capacity-checking helpers.The implementation correctly:
- Treats
capacity == 0as unlimited (line 303-304)- Uses
select_for_update().order_by('id')for consistent lock ordering to prevent deadlocks (line 312)- Treats missing waiting list as "full" to be conservative (line 339)
- Wraps the check in
transaction.atomic()to ensure consistent readsThe separation of concerns between the three helpers makes the capacity logic easy to understand and test.
379-386: LGTM!The auto-move logic now uses
transaction.atomic()with row-level locking to prevent race conditions when multiple subscriptions compete for limited capacity.
476-527: LGTM!The email function correctly handles the new
capacity_blockedscenario by informing users when all spots are sold out while preserving existing email flows for normal cases.
2100-2148: Capacity-blocking logic correctly implemented.The flow:
- Subscription is always created in Form List (preserving the form submission)
- Capacity check determines
capacity_blockedstatus- No SumUp checkout is created when blocked (saving resources and preventing dead checkouts)
- User is informed via email and API response
This design correctly separates "accepting a subscription" from "accepting payment", allowing manual processing when capacity opens up.
2191-2209: LGTM!The
payment_blockedcomputation correctly checks all necessary conditions before indicating a block. Includingreasonandmessagein the response provides good client-side UX.backend/treasury/views.py (6)
37-38: LGTM!The
MSG_UNAUTHORIZEDconstant centralizes the unauthorized message, improving consistency across the module and making future i18n easier.
64-65: LGTM!Board-only restriction for ESNcard deletion is appropriate given the financial implications of the automatic refund transaction.
186-269: Well-structured ESNcard revocation implementation.The DELETE flow correctly:
- Uses
transaction.atomic()withselect_for_update()to prevent race conditions- Validates that only ESNcard emission transactions are linked (lines 195-202)
- Guards against multiple emission transactions (lines 210-214)
- Validates refund amount, account status, and balance before creating the refund
- Creates the
RIMBORSO_ESNCARDtransaction without subscription/esncard FK (matching model validation)- Deletes the ESNcard only after successful refund transaction save
The error handling with 409 Conflict status for business logic failures is appropriate.
813-840: Good performance optimization.The change from per-subscription queries to a single prefetch with in-memory filtering eliminates N+1 query patterns. The
select_related('profile')andPrefetch('transaction_set', queryset=Transaction.objects.select_related('account'))ensure all needed relationships are loaded efficiently.
456-463: LGTM!Adding
RIMBORSO_ESNCARDto the deleteable transaction types is consistent with other refund types, allowing administrative correction when needed.
1046-1048: LGTM!Export helpers correctly handle
RIMBORSO_ESNCARDwith appropriate Italian labels consistent with the existing ESNcard terminology.Also applies to: 1077-1078
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation