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.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughReorganized documentation, tightened content-management authorization (Board or explicit flag), implemented ESNcard DELETE (revocation + refund transaction), added event sold-out payment-blocking and Form List fallback, introduced a unified reimbursement modal in the frontend, and moved GitHub Actions Changes
Sequence Diagram(s)sequenceDiagram
actor Board as Board Member
participant UI as Profile UI
participant API as Treasury API
participant DB as Database
participant Lock as Row Lock Manager
Board->>UI: Click "Revoke latest ESNcard"
UI->>UI: Show confirmation dialog
Board->>UI: Confirm revocation
UI->>API: DELETE /esncard/{id}/
activate API
API->>Lock: Acquire row lock on ESNcard
API->>DB: Load ESNcard + emission transaction
DB-->>API: Emission tx (amount > 0)
API->>DB: Validate refund account (open & sufficient)
DB-->>API: ✓ Valid
API->>DB: Create RIMBORSO_ESNCARD transaction (negative amount)
DB-->>API: Refund tx created
API->>DB: Delete ESNcard record
DB-->>API: ✓ Deleted
deactivate API
Lock->>Lock: Release lock
API-->>UI: 200 OK (original/refund tx IDs)
UI->>UI: Show success toast and refresh profile
sequenceDiagram
actor User as Event Subscriber
participant Form as Public Event Form
participant API as Events API
participant DB as Database
participant Cache as Capacity Checker
User->>Form: Submit event form
Form->>API: POST /public_form/
activate API
API->>Cache: Check Main list capacity (locked read)
API->>Cache: Check Waiting list capacity (locked read)
alt Both lists full
API->>DB: Create subscription on Form List, mark payment_required true
API-->>Form: 201 (assigned_label="Form List", capacity_blocked=true)
Form->>User: Confirmation + sold-out messaging
else Space available
API->>DB: Create subscription on Main/Waiting (and possibly create SumUp checkout)
API-->>Form: 201 (assigned_label=Main/Waiting)
Form->>User: Confirmation
end
deactivate API
sequenceDiagram
actor Organizer as Organizer
participant Modal as ReimburseSelectionModal
participant AccountsAPI as Accounts API
participant ReimburseAPI as Reimbursement API
participant DB as Database
Organizer->>Modal: Open reimbursement modal
Modal->>AccountsAPI: GET /accounts/
AccountsAPI->>DB: Fetch accounts
DB-->>AccountsAPI: Accounts list
AccountsAPI-->>Modal: Return accounts
Organizer->>Modal: Select items + account, Confirm
Modal->>ReimburseAPI: POST /reimburse_quota/ (if quota/services)
ReimburseAPI->>DB: Create refund entries, update subscription records
DB-->>ReimburseAPI: Success
Modal->>ReimburseAPI: POST /reimburse_deposits/ (if deposit)
ReimburseAPI->>DB: Create deposit refund entries
DB-->>ReimburseAPI: Success
ReimburseAPI-->>Modal: Per-item responses
Modal->>Organizer: Show results and trigger refresh as needed
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
There was a problem hiding this comment.
Pull request overview
This PR prepares the application for deployment by tightening authorization rules, adding operational finance flows (ESNcard revoke + refund tracking), improving payment/reimbursement UX, and reorganizing/refreshing project documentation and build artifacts.
Changes:
- Added ESNcard revocation (Board-only) that deletes the card and records a dedicated refund transaction (
rimborso_esncard), with tests and UI integration. - Introduced “sold-out” online-payment blocking logic (backend status + process_payment) and updated the payment page to surface the blocked state.
- Replaced quota-only reimburse modal entrypoint with a unified reimbursement selection flow (fee/services/deposit) and updated transaction type display/config.
Reviewed changes
Copilot reviewed 43 out of 45 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| README_SHARED_LISTS_FINAL.md | Removed large final summary document (cleanup before deploy). |
| frontend/src/utils/useMaintenanceNotification.js | Reduced polling frequency for maintenance status checks. |
| frontend/src/utils/displayAttributes.jsx | Added display label for rimborso_esncard. |
| frontend/src/Pages/profiles/Profile.jsx | Added Board-only “Revoke last ESNcard” UI + confirmation dialog; adjusted content-manager toggle messaging. |
| frontend/src/Pages/events/EventPayment.jsx | Updated payment flow to react to payment_blocked and better reuse checkout id. |
| frontend/src/Pages/events/Event.jsx | Switched from quota-only reimbursement modal to unified selection modal. |
| frontend/src/data/transactionConfigs.js | Added config entry for rimborso_esncard. |
| frontend/src/Components/treasury/TransactionModal.jsx | Allowed delete/negative handling for rimborso_esncard. |
| frontend/src/Components/events/ReimburseSelectionModal.jsx | New modal to select refund items (quota/services/deposit) and orchestrate backend calls. |
| frontend/src/Components/events/EventListAccordions.jsx | Replaced multiple reimbursement icons with a single “Reimburse” action opening the unified flow. |
| frontend/build/index.html | Updated built asset reference for deployment build output. |
| backend/users/views.py | Relaxed content-manager assignment constraint to “ESNer only”; adjusted effective content permission rules. |
| backend/users/serializers.py | Updated virtual manage_content permission to Board or explicit flag only. |
| backend/users/models.py | Clarified can_manage_content semantics in field comment. |
| backend/treasury/views.py | Implemented DELETE /esncard/<pk>/ revoke flow + rimborso_esncard export/label updates; fixed reimbursable deposits for external subs. |
| backend/treasury/tests.py | Added test coverage for ESNcard revoke flow + external subscription deposits. |
| backend/treasury/models.py | Added RIMBORSO_ESNCARD transaction type and validation rules. |
| backend/README_Models.md | Updated/translated documentation to English. |
| backend/README_Content.md | Updated/translated documentation to English + aligned permissions wording. |
| backend/README_APIs.md | Updated API docs (ESNcard endpoints + revoke behavior). |
| backend/Project Documentation/TEST_COVERAGE_REPORT.md | Added a new consolidated test coverage report document. |
| backend/Project Documentation/07_MAINTENANCE_MODULE.md | Added maintenance module technical documentation. |
| backend/Project Documentation/06_INTEGRATION_E2E.md | Added cross-module integration/E2E baseline documentation. |
| backend/Project Documentation/05_CONTENT_MODULE.md | Added content module documentation. |
| backend/Project Documentation/04_TREASURY_MODULE.md | Added treasury module documentation (including ESNcard revoke). |
| backend/Project Documentation/03_EVENTS_MODULE.md | Added events module documentation (including sold-out/payment block notes). |
| backend/Project Documentation/02_PROFILES_MODULE.md | Added profiles module documentation. |
| backend/Project Documentation/01_USERS_MODULE.md | Added users module documentation (auth + permission model). |
| backend/Project Documentation/00_OVERVIEW.md | Added system overview documentation (architecture + invariants). |
| backend/events/views.py | Added sold-out payment blocking logic + refactors for list availability helpers + improved email messaging. |
| backend/events/tests.py | Added tests for “form submit allowed even if full” + payment blocked status/process_payment cases. |
| backend/events.md | Removed outdated events note doc. |
| backend/docs/test_specifications/06_INTEGRATION_E2E.md | Removed old test specification document (superseded). |
| backend/docs/test_specifications/05_CONTENT_MODULE.md | Removed old test specification document (superseded). |
| backend/docs/test_specifications/02_PROFILES_MODULE.md | Removed old test specification document (superseded). |
| backend/docs/test_specifications/01_USERS_MODULE.md | Removed old test specification document (superseded). |
| backend/docs/test_specifications/00_OVERVIEW.md | Removed old test specification document (superseded). |
| backend/docs/TEST_COVERAGE_REPORT.md | Removed old test coverage report (superseded). |
| backend/content/views.py | Centralized _can_manage_content and tightened write permissions to Board/explicit flag only. |
| backend/content/tests.py | Updated content permission tests to match new policy; added explicit-role test cases. |
| .github/workflows/deploy-production.yml | Changed version bump strategy (minor vs patch) for production deploy workflow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
| Filename | Overview |
|---|---|
| backend/events/views.py | Adds payment-blocking when main+waiting lists are full; refactors list-space helpers into module-level functions; edge case: missing main list is treated as "full", which could incorrectly block payment for events without a main list |
| backend/treasury/views.py | Adds ESNcard DELETE endpoint that revokes the card and creates a RIMBORSO_ESNCARD refund transaction atomically; includes defensive guards against multiple linked emissions and closed accounts |
| backend/treasury/models.py | Introduces RIMBORSO_ESNCARD TransactionType with clean() validation ensuring no subscription or esncard link; no accompanying migration file included |
| backend/content/views.py | Removes implicit content-management permission for 'Attivi' group; centralizes the check in _can_manage_content(); existing Attivi content managers will need explicit flag migration |
| frontend/src/Components/events/ReimburseSelectionModal.jsx | New unified reimbursement modal combining quota, services, and deposit refund categories into a single flow with per-item status chips and sequential API calls |
| frontend/src/Components/events/EventListAccordions.jsx | Replaces two separate quota/deposit reimburse buttons with unified PriceCheckIcon button calling onOpenReimburseMenu; onOpenReimburseDeposits prop and useMemo dependency are now stale dead code |
| frontend/src/Pages/events/EventPayment.jsx | Adds payment_blocked handling from status API; always re-fetches status on mount (removing old checkoutId guard) to ensure sold-out check runs fresh; improves error message surfacing from process_payment errors |
| frontend/src/Pages/profiles/Profile.jsx | Adds "Revoca Ultima ESNcard" board-only button that calls the new DELETE /esncard/:id/ endpoint with confirmation dialog; updates content-manager toggle to show for all ESNers |
| frontend/src/utils/useMaintenanceNotification.js | Reduces maintenance status polling interval from 30 seconds to 5 minutes to cut unnecessary background requests |
| .github/workflows/deploy-production.yml | Switches version bump strategy from PATCH increment to MINOR increment on production deploy |
Sequence Diagram
sequenceDiagram
participant FE as Frontend
participant EV as events/views
participant TR as treasury/views
participant DB as Database
note over FE,DB: ESNcard Revocation Flow
FE->>TR: DELETE /esncard/:id/
TR->>DB: select_for_update ESNcard + linked Transactions
TR->>DB: check non-ESNCARD refs (abort if any)
TR->>DB: check linked ESNCARD emissions (abort if >1)
TR->>DB: check account balance >= emission amount
TR->>DB: INSERT RIMBORSO_ESNCARD Transaction (amount = -emission)
TR->>DB: DELETE ESNcard
TR-->>FE: 200 {revoked_esncard_number, refund_transaction_id}
note over FE,DB: Sold-Out Payment Block Flow
FE->>EV: GET /subscription/:id/status/
EV->>DB: fetch subscription + event + lists
EV->>DB: subscription_count for main_list / waiting_list
EV-->>FE: {payment_blocked: true, payment_blocked_reason: "sold_out"}
FE->>EV: POST /subscription/:id/process_payment/
EV->>DB: check _is_payment_blocked_by_full_lists
EV-->>FE: 409 {status: "BLOCKED", error: "sold_out"}
Comments Outside Diff (1)
-
backend/content/views.py, line 129-148 (link)'Attivi' group loses implicit content management access
The previous implementation granted write access to content endpoints for all members of the
'Attivi'group. This PR removes that implicit grant, replacing it with an explicitcan_manage_contentflag or Board membership. The same change is applied consistently acrossIsContentManagerOrReadOnly,whatsapp_config, andUserReactSerializer.get_permissions.Existing
'Attivi'users who were relying on this implicit access will silently lose the ability to create/edit/delete content items without being individually granted thecan_manage_contentflag. There is no fallback or migration of the flag for existing users.If any
'Attivi'member is currently acting as a content manager, they will need the explicit flag set before or immediately after this deploy. Worth confirming that affected users have been notified / their flags pre-populated.Prompt To Fix With AI
This is a comment left during a code review. Path: backend/content/views.py Line: 129-148 Comment: **'Attivi' group loses implicit content management access** The previous implementation granted write access to content endpoints for all members of the `'Attivi'` group. This PR removes that implicit grant, replacing it with an explicit `can_manage_content` flag or Board membership. The same change is applied consistently across `IsContentManagerOrReadOnly`, `whatsapp_config`, and `UserReactSerializer.get_permissions`. Existing `'Attivi'` users who were relying on this implicit access will silently lose the ability to create/edit/delete content items without being individually granted the `can_manage_content` flag. There is no fallback or migration of the flag for existing users. If any `'Attivi'` member is currently acting as a content manager, they will need the explicit flag set before or immediately after this deploy. Worth confirming that affected users have been notified / their flags pre-populated. How can I resolve this? If you propose a fix, please make it concise.
Prompt To Fix All With AI
This is a comment left during a code review.
Path: frontend/src/Components/events/EventListAccordions.jsx
Line: 698
Comment:
**Stale `onOpenReimburseDeposits` in `useMemo` dependency array**
`onOpenReimburseDeposits` is still listed as a dependency here but is no longer referenced inside the `useMemo` body — the unified reimburse button now calls `onOpenReimburseMenu` exclusively. This means:
1. Any new function reference created in the parent on each render (which is the case for `handleOpenReimburseDeposits` in `Event.jsx`) will cause the memo to recompute unnecessarily.
2. The prop is also still destructured at line 36 but never called, making it dead interface surface.
Consider removing `onOpenReimburseDeposits` from both the destructured props and the `useMemo` dependency array:
```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: backend/events/views.py
Line: 307-330
Comment:
**Payment blocked for events with no main list**
`_is_payment_blocked_by_full_lists` calls `_list_has_space(main_list)` where `main_list` may be `None` if the event was not set up with a main list. `_list_has_space(None)` returns `False`, so `main_full` becomes `True`. Combined with the explicit `waiting_full = True` when there is no waiting list, the function returns `True` (blocked) for **any** subscription in an event that has neither a main nor a waiting list but has `allow_online_payment=True`.
This could silently block all online payments for events that use only a custom form list without main/waiting lists.
```python
def _is_payment_blocked_by_full_lists(subscription):
...
main_list, waiting_list = _get_main_waiting_lists(subscription.event)
main_full = not _list_has_space(main_list) # True when main_list is None
waiting_full = not _list_has_space(waiting_list) if waiting_list else True
return main_full and waiting_full # True when no lists exist at all
```
Consider treating a missing main list as "no list configured" (return `False` immediately) rather than "full":
```python
main_list, waiting_list = _get_main_waiting_lists(subscription.event)
if not main_list: # No main list configured — block logic doesn't apply
return False
main_full = not _list_has_space(main_list)
waiting_full = not _list_has_space(waiting_list) if waiting_list else True
return main_full and waiting_full
```
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/treasury/models.py
Line: 78
Comment:
**Missing Django migration for new `RIMBORSO_ESNCARD` transaction type**
Adding a value to a `TextChoices` enum causes Django to detect a migration change (it tracks `choices` metadata on `CharField`). No migration file appears in this PR, so running `python manage.py makemigrations --check` in CI will likely fail. The database schema itself is unaffected (choices are not enforced at the DB level), but the migration history will be inconsistent.
Generate and include the migration:
```bash
python manage.py makemigrations treasury --name add_rimborso_esncard_transaction_type
```
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/content/views.py
Line: 129-148
Comment:
**'Attivi' group loses implicit content management access**
The previous implementation granted write access to content endpoints for all members of the `'Attivi'` group. This PR removes that implicit grant, replacing it with an explicit `can_manage_content` flag or Board membership. The same change is applied consistently across `IsContentManagerOrReadOnly`, `whatsapp_config`, and `UserReactSerializer.get_permissions`.
Existing `'Attivi'` users who were relying on this implicit access will silently lose the ability to create/edit/delete content items without being individually granted the `can_manage_content` flag. There is no fallback or migration of the flag for existing users.
If any `'Attivi'` member is currently acting as a content manager, they will need the explicit flag set before or immediately after this deploy. Worth confirming that affected users have been notified / their flags pre-populated.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "Refactor EventPayment component to handl..." | Re-trigger Greptile
| @@ -76,6 +76,7 @@ class Transaction(BaseEntity): | |||
| class TransactionType(models.TextChoices): | |||
| SUBSCRIPTION = "subscription", _("Subscription") | |||
| ESNCARD = "esncard", _("ESNcard") | |||
There was a problem hiding this comment.
RIMBORSO_ESNCARD transaction type
Adding a value to a TextChoices enum causes Django to detect a migration change (it tracks choices metadata on CharField). No migration file appears in this PR, so running python manage.py makemigrations --check in CI will likely fail. The database schema itself is unaffected (choices are not enforced at the DB level), but the migration history will be inconsistent.
Generate and include the migration:
python manage.py makemigrations treasury --name add_rimborso_esncard_transaction_typePrompt To Fix With AI
This is a comment left during a code review.
Path: backend/treasury/models.py
Line: 78
Comment:
**Missing Django migration for new `RIMBORSO_ESNCARD` transaction type**
Adding a value to a `TextChoices` enum causes Django to detect a migration change (it tracks `choices` metadata on `CharField`). No migration file appears in this PR, so running `python manage.py makemigrations --check` in CI will likely fail. The database schema itself is unaffected (choices are not enforced at the DB level), but the migration history will be inconsistent.
Generate and include the migration:
```bash
python manage.py makemigrations treasury --name add_rimborso_esncard_transaction_type
```
How can I resolve this? If you propose a fix, please make it concise.![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
backend/content/tests.py (1)
622-635:⚠️ Potential issue | 🟡 MinorTest method name contradicts its purpose.
The method is named
test_whatsapp_config_patch_allowed_for_attivibut the docstring says "should be forbidden" and expects403. Rename to accurately reflect the test's intent.Proposed fix
- def test_whatsapp_config_patch_allowed_for_attivi(self): - """PATCH whatsapp-config should be forbidden for Attivi without explicit role.""" + def test_whatsapp_config_patch_forbidden_for_attivi(self): + """PATCH whatsapp-config should be forbidden for Attivi without explicit role."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/content/tests.py` around lines 622 - 635, The test method name test_whatsapp_config_patch_allowed_for_attivi contradicts its docstring and assertion expecting 403; rename the test to reflect that PATCH is forbidden for attivi (e.g., test_whatsapp_config_patch_forbidden_for_attivi or test_whatsapp_config_patch_forbidden_for_attivi_without_role) and update any references; keep the body, docstring and assertion unchanged and only change the function name to the chosen descriptive identifier.frontend/src/Components/events/EventListAccordions.jsx (1)
449-477:⚠️ Potential issue | 🟠 MajorThe unified reimburse action dropped the treasury-permission gate.
This button now only checks paid status and the organizer-only flag. Users without
canChangeTransactionscan still open the reimbursement modal, while the bulk reimbursement action in the same component is still gated by that permission.Possible fix
- const canReimburseAny = canReimburseQuota || canReimburseServices || canReimburseDeposit; + const canReimburseAny = + canChangeTransactions && + (canReimburseQuota || canReimburseServices || canReimburseDeposit); <IconButton - title={reimbursementsRestricted ? 'Rimborso riservato a organizzatori/Board' : 'Rimborsa'} + title={ + !canChangeTransactions + ? 'Permessi tesoreria mancanti' + : reimbursementsRestricted + ? 'Rimborso riservato a organizzatori/Board' + : 'Rimborsa' + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/events/EventListAccordions.jsx` around lines 449 - 477, The reimburse IconButton currently only checks canReimburseAny and reimbursementsRestricted, which allows users without treasury permission to open the modal; update the button and click handler to also respect the treasury permission (e.g., canChangeTransactions). Specifically, include a check for canChangeTransactions in the disabled prop (disabled={!canReimburseAny || reimbursementsRestricted || !canChangeTransactions}), add the same check in the onClick guard before calling onOpenReimburseMenu(sub), and apply the disabled styling when !canChangeTransactions (e.g., treat it like reimbursementsRestricted for opacity/pointerEvents) so users without the treasury permission cannot open the reimbursement modal.frontend/src/Pages/profiles/Profile.jsx (1)
1410-1436:⚠️ Potential issue | 🟡 MinorUse an explicit Board-inheritance check here.
can_manage_content !== effective_can_manage_contentonly detects inherited access when the raw flag is false. A Board user withcan_manage_content=truestill gets an enabled “Revoca Content Manager” button and a success toast even though effective access remains granted via Board.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/profiles/Profile.jsx` around lines 1410 - 1436, The check detecting Board-inherited Content Manager access is wrong; replace the boolean-diff check with an explicit Board-inheritance check: compute a flag like isInheritedFromBoard = user?.groups?.includes('Board') && financePerms?.effective_can_manage_content (or include any existing inheritance marker on financePerms) and use that flag in place of financePerms.can_manage_content !== financePerms.effective_can_manage_content so the disabled Tooltip/disabled Button branch triggers for Board members regardless of the raw can_manage_content value; update the conditional around financePerms, the Tooltip and the Button (references: user?.groups?.includes('Board'), profileType, financePerms, toggleContentManagerRole) accordingly.backend/treasury/views.py (1)
453-460:⚠️ Potential issue | 🟠 MajorKeep
RIMBORSO_ESNCARDout of the generic manual-delete list.Once
esncard_detail(DELETE)has revoked the card, deleting the refund transaction only restores the account balance; it cannot restore the ESNcard or the original business event. That leaves treasury state and card history out of sync.💡 Suggested fix
list_deleteable = [ Transaction.TransactionType.RIMBORSO_CAUZIONE, - Transaction.TransactionType.RIMBORSO_ESNCARD, Transaction.TransactionType.REIMBURSEMENT, Transaction.TransactionType.RIMBORSO_QUOTA, Transaction.TransactionType.DEPOSIT,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/treasury/views.py` around lines 453 - 460, The list_deleteable array currently includes Transaction.TransactionType.RIMBORSO_ESNCARD which allows generic manual deletion of ESN card refund transactions; remove Transaction.TransactionType.RIMBORSO_ESNCARD from the list_deleteable definition so ESN card refunds cannot be deleted via the generic manual-delete path (leave other types unchanged) and ensure any tests or handlers that rely on list_deleteable (e.g., code referencing list_deleteable in views handling DELETE) still behave correctly for the remaining types.
🧹 Nitpick comments (5)
.github/workflows/deploy-production.yml (1)
36-37: ResetPATCHwhen bumpingMINORfor SemVer consistency.Line 36 preserves the old patch while Line 37 increments minor, producing tags like
v1.3.7afterv1.2.7. It works, but it’s non-standard and can make release progression harder to reason about.♻️ Proposed adjustment
- PATCH=$((PATCH + 0)) - MINOR=$((MINOR + 1)) + PATCH=0 + MINOR=$((MINOR + 1))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy-production.yml around lines 36 - 37, When bumping the MINOR version you should reset the PATCH counter for SemVer consistency: change the logic around the MINOR and PATCH variables so that PATCH is set to 0 and then MINOR is incremented (i.e., replace the current code that leaves PATCH unchanged and only increments MINOR). Update the lines that reference the PATCH and MINOR variables (PATCH and MINOR) to ensure PATCH=0 and MINOR is incremented in that order.frontend/src/Components/treasury/TransactionModal.jsx (1)
80-80: Stale closure warning:negative_typesdependency missing fromuseEffect.The
useEffecton line 46 referencesnegative_types(line 65) but doesn't include it in the dependency array (line 80). Sincenegative_typesis defined inside the component, it gets recreated on each render. While this works because the array values are constant, ESLint would flag this.♻️ Move `negative_types` outside the component to avoid dependency issues
+// List of transaction types that represent negative amounts (outflows) +const negative_types = ['withdrawal', 'rimborso_esncard', 'rimborso_cauzione', 'rimborso_quota', 'reimbursement']; + export default function TransactionModal({open, onClose, transaction}) { const [isLoading, setLoading] = useState(true); const [accounts, setAccounts] = useState([]); const [popup, setPopup] = useState(null); const [submitting, setSubmitting] = useState(false); const [deleting, setDeleting] = useState(false); const [confirmDialog, setConfirmDialog] = useState({open: false, action: null, message: ''}); - const negative_types = ['withdrawal', 'rimborso_esncard', 'rimborso_cauzione', 'rimborso_quota', 'reimbursement'];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/treasury/TransactionModal.jsx` at line 80, The useEffect in TransactionModal references the in-component array negative_types but doesn't include it in the dependency array, causing a stale-closure/ESLint warning; fix by moving the negative_types declaration out of the TransactionModal component to module scope (e.g., define const negative_types = [...] above the component) so it is stable across renders, then keep the useEffect dependency array as [open, transaction]; update any references inside the component to use that module-scope negative_types.backend/treasury/models.py (1)
114-118: Validation logic forRIMBORSO_ESNCARDis correct but follows a problematic pattern.The validation correctly ensures
RIMBORSO_ESNCARDtransactions have no linkedsubscriptionoresncard, which aligns with the PR objective of standalone refund transactions.However, note that
ValueErroris raised here instead of Django'sValidationError. Whilesave()(line 151) callsclean()to enforce this, the relevant code snippet shows thatTransactionSerializer.create()inbackend/treasury/serializers.py:299usesTransaction.objects.create()directly—which does callsave()but anyValueErrorraised will surface as a 500 error to API clients rather than a 400 validation error.This is a pre-existing pattern in this file, so not blocking, but worth noting for future improvement.
♻️ Optional: Consider using Django's ValidationError for proper HTTP 400 responses
+from django.core.exceptions import ValidationError ... def clean(self): # Validate fields based on transaction type if self.type == self.TransactionType.SUBSCRIPTION and not self.subscription: - raise ValueError("Le transazioni di Iscrizione devono avere un'Iscrizione.") + raise ValidationError("Le transazioni di Iscrizione devono avere un'Iscrizione.")This would require updating all
ValueErrorraises inclean()toValidationErrorfor consistent API error handling.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/treasury/models.py` around lines 114 - 118, The clean() validation for TransactionType.RIMBORSO_ESNCARD currently raises ValueError (in the block checking self.subscription and self.esncard); change these to django.core.exceptions.ValidationError so model-level validation surfaces as HTTP 400 when TransactionSerializer.create() calls Transaction.objects.create() (save() calls clean()). Update all ValueError raises in the Transaction.clean() method to raise ValidationError with the same messages and add the appropriate import (ValidationError) at the top of the file.backend/users/views.py (1)
379-380: Redundant validation check.This
is_esnercheck forcontent_fieldsis redundant because line 370-371 already validatestarget.profile.is_esnerand returns a 400 error if false. The code at line 379 can never trigger since any non-ESNer request would have already been rejected.Consider whether this duplication is intentional for defense-in-depth or can be removed for clarity.
Option: Remove redundant check
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 379 - 380, Remove the redundant validation that checks target.profile.is_esner when content_fields is set: the earlier check that rejects non-ESNer targets already handles this, so delete the conditional "if content_fields and not target.profile.is_esner: return Response(...)" (leave the initial is_esner validation intact) and ensure any logic that depends on content_fields proceeds without this duplicate guard.frontend/src/Pages/events/EventPayment.jsx (1)
150-163: Error handling could fail silently on non-Response errors.The error handling checks
errorResponse?.json(a method) for truthiness, which works whenerrorResponseis aResponseobject. However, iferrorResponse.json()is called on an already-consumed response or if the response isn't JSON, the catch block silently swallows the error.Consider adding more defensive checks:
🛡️ Suggested improvement for error handling
onError: async (errorResponse) => { let apiMessage = ''; - if (errorResponse?.json) { + if (errorResponse && typeof errorResponse.json === 'function') { try { const errorData = await errorResponse.json(); apiMessage = errorData?.message || errorData?.error || ''; - } catch { + } catch (parseError) { + // Response body may have been consumed or isn't JSON apiMessage = ''; } } else if (errorResponse?.message) { apiMessage = errorResponse.message; } setStatus('failed'); setMessage(apiMessage || 'Payment confirmation error.'); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/events/EventPayment.jsx` around lines 150 - 163, The onError handler currently assumes errorResponse.json exists and swallows errors; update the onError callback to first check that errorResponse is an object and typeof errorResponse.json === 'function' before calling json(), fall back to calling errorResponse.text() if json parsing fails or if json isn't available, and for non-Response errors (strings, Error objects) extract message via errorResponse.message || String(errorResponse); ensure the caught parsing error is logged or included in apiMessage and then call setStatus('failed') and setMessage(apiMessage || 'Payment confirmation error.'). Use the identifiers onError, errorResponse, apiMessage, setStatus and setMessage to locate and change the code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@backend/content/views.py`:
- Around line 200-202: The comment above the lines setting user and can_edit
uses an EN DASH (U+2013); replace it with a standard hyphen-minus (U+002D) so
the comment reads "# PATCH - authorized content managers can edit". Locate the
comment immediately preceding the assignments to user and can_edit (and near the
call to _can_manage_content) and update the character only—no other changes to
code or surrounding whitespace.
In `@backend/events/views.py`:
- Around line 2102-2114: When falling back to "Form List" after checking
_get_main_waiting_lists and _list_has_space, the code must surface the sold-out
state so users don't get a pay-now link; detect the case where assigned_label ==
"Form List" and either set online_payment_required = False before calling
_send_form_subscription_email(sub, assigned_label, online_payment_required,
payment_required) or add a new boolean flag (e.g., capacity_blocked) and pass it
into _send_form_subscription_email so the email renderer can suppress
immediate-payment copy/link until capacity reopens; update the
_send_form_subscription_email signature and templates accordingly if you add the
flag.
- Around line 313-328: The capacity check in _is_payment_blocked_by_full_lists
(and the similar logic in attempt_move_from_form_list) is racy because it reads
list capacities without locking; fix by performing the capacity check and any
subsequent move/reservation inside a single DB transaction using row-level
locking (e.g., select_for_update) on the involved List rows returned by
_get_main_waiting_lists (or on a dedicated capacity/seat row), so that
_list_has_space is evaluated under lock and the move/reservation is done
atomically; alternatively implement optimistic locking (version counter) on the
List/event capacity and retry on conflict to ensure only one successful
payment/move when capacity is 1.
- Around line 2205-2219: The current early-return guard computing
should_block_payment (using event.allow_online_payment, has_payable_amount,
_subscription_payment_already_registered(sub), and
_is_payment_blocked_by_full_lists(sub)) short-circuits requests before
_process_sumup_checkout() runs, causing already-completed SumUp checkouts to be
treated as "sold out" and never reconciled; change the logic so that before
returning the 409 BLOCKED response you first call or check
_process_sumup_checkout() (or otherwise verify whether a SumUp checkout was
created/completed for this subscription) and only return BLOCKED if no external
checkout/payment exists and the other conditions hold; ensure the code
references and uses the same sub/transaction identifiers that
_process_sumup_checkout() uses so completed SumUp payments are detected and
recorded instead of blocked.
In `@backend/Project` Documentation/06_INTEGRATION_E2E.md:
- Around line 147-159: Update the Italian section header "Suite integrazione
dedicate" to English; locate the heading under "## 3. Test Asset Mapping" and
replace the text "Suite integrazione dedicate" with an appropriate English
translation such as "Dedicated integration suites" to keep language consistency
in the document.
- Around line 135-145: Replace the Italian typo "visibile" with the English word
"visible" in the sequence step text (the line containing "banner frontend
visibile") so the Sequence reads "banner frontend visible"; search for any other
occurrences of "visibile" in the same document (e.g., within the Sequence block
or nearby test descriptions) and correct them to "visible" to keep terminology
consistent.
- Around line 86-99: The documentation contains an Italian phrase
"orchestrazione chiamate reimburse_quota e/o reimburse_deposits"; replace it
with English for consistency—suggested wording: "orchestrate calls to
reimburse_quota and/or reimburse_deposits" (keep the rest of the list intact and
ensure the exact phrase is updated wherever it appears in the section).
In `@backend/Project` Documentation/07_MAINTENANCE_MODULE.md:
- Line 121: Replace the Italian phrase "action clear resetta notification_id e
triggered_at" with an English equivalent in the documentation; update the
sentence in 07_MAINTENANCE_MODULE.md to something like "action clear resets
notification_id and triggered_at" (or another accurate English phrasing that
matches the intent), ensuring grammar and tense match surrounding lines and any
referenced action names (e.g., "action clear") remain unchanged.
In `@backend/README_Content.md`:
- Around line 48-52: The README incorrectly states that "Write" is limited to
Board members; update the Permissions section to reflect the actual
authorization: list both "Board members" and "Users with
can_manage_content=True" (or equivalent phrasing) as having write access, and
optionally link or reference the implementation in views.py/_can_manage_content
so readers can see the rule source; ensure the permission wording matches the
flag name can_manage_content used in the code.
- Around line 36-46: Update the API documentation lines for POST/PATCH/DELETE
under /backend/content/sections/ and /backend/content/links/ so they mention
that write access is available to users with the can_manage_content flag in
addition to Board members; specifically adjust the descriptive comment text
"(Board only)" next to the endpoints for POST /backend/content/sections/, PATCH
/backend/content/sections/{id}/, DELETE /backend/content/sections/{id}/ and POST
/backend/content/links/, PATCH /backend/content/links/{id}/, DELETE
/backend/content/links/{id}/ to say something like "(Board or users with
can_manage_content)" or equivalent to reflect both access paths.
In `@backend/treasury/views.py`:
- Around line 214-250: The refund Transaction (type RIMBORSO_ESNCARD) is only
saving contextual info in the free-text description; populate the structured
fields before calling refund_tx.save() so downstream reporting can reliably link
the refund to the original emission, subscription and card. Specifically, set
refund_tx.subscription = emission_tx.subscription, refund_tx.esncard = esncard
and set a reference to the original emission (e.g. refund_tx.original_emission =
emission_tx or refund_tx.original_transaction_id = original_transaction_id) on
the Transaction instance created in this block (the refund_tx created before
refund_tx.save()), then save; adjust field names if your Transaction model uses
different attribute names. Ensure these assignments occur before the try/except
that calls refund_tx.save().
In `@frontend/src/Components/events/ReimburseSelectionModal.jsx`:
- Around line 107-117: The services option in ReimburseSelectionModal.jsx is
enabled when servicesStatus === 'paid' even if servicesTotal <= 0, causing €0.00
refunds to be submitted; update the logic around hasServices / the block that
sets disabled and reason (variables: hasServices, servicesTotal, servicesStatus,
disabled, reason) so that any servicesTotal <= 0 forces disabled = true and sets
reason = 'Nessun servizio selezionato' (treat services as non-reimbursable
regardless of status); ensure the condition that currently checks servicesStatus
=== 'paid' also requires servicesTotal > 0 before enabling the option.
- Around line 150-165: The modal reset/effect in useEffect is currently keyed to
refundItems which causes resets during onRefresh; update the dependency array
and gating so the reset and fetch only run when the modal opens or the
subscription record changes: keep the body (setLoading, setPopup,
setSelectedAccount, setNotes, setCompletedItems, setItemResults,
setSelectedItems(makeInitialSelection(refundItems)) and fetchCustom(...)) but
change the dependency array from [open, refundItems] to [open, subscription?.id]
(or the appropriate subscription identifier prop), and ensure the effect
short-circuits when !open so partial-success retries that update refundItems do
not trigger a reset.
---
Outside diff comments:
In `@backend/content/tests.py`:
- Around line 622-635: The test method name
test_whatsapp_config_patch_allowed_for_attivi contradicts its docstring and
assertion expecting 403; rename the test to reflect that PATCH is forbidden for
attivi (e.g., test_whatsapp_config_patch_forbidden_for_attivi or
test_whatsapp_config_patch_forbidden_for_attivi_without_role) and update any
references; keep the body, docstring and assertion unchanged and only change the
function name to the chosen descriptive identifier.
In `@backend/treasury/views.py`:
- Around line 453-460: The list_deleteable array currently includes
Transaction.TransactionType.RIMBORSO_ESNCARD which allows generic manual
deletion of ESN card refund transactions; remove
Transaction.TransactionType.RIMBORSO_ESNCARD from the list_deleteable definition
so ESN card refunds cannot be deleted via the generic manual-delete path (leave
other types unchanged) and ensure any tests or handlers that rely on
list_deleteable (e.g., code referencing list_deleteable in views handling
DELETE) still behave correctly for the remaining types.
In `@frontend/src/Components/events/EventListAccordions.jsx`:
- Around line 449-477: The reimburse IconButton currently only checks
canReimburseAny and reimbursementsRestricted, which allows users without
treasury permission to open the modal; update the button and click handler to
also respect the treasury permission (e.g., canChangeTransactions).
Specifically, include a check for canChangeTransactions in the disabled prop
(disabled={!canReimburseAny || reimbursementsRestricted ||
!canChangeTransactions}), add the same check in the onClick guard before calling
onOpenReimburseMenu(sub), and apply the disabled styling when
!canChangeTransactions (e.g., treat it like reimbursementsRestricted for
opacity/pointerEvents) so users without the treasury permission cannot open the
reimbursement modal.
In `@frontend/src/Pages/profiles/Profile.jsx`:
- Around line 1410-1436: The check detecting Board-inherited Content Manager
access is wrong; replace the boolean-diff check with an explicit
Board-inheritance check: compute a flag like isInheritedFromBoard =
user?.groups?.includes('Board') && financePerms?.effective_can_manage_content
(or include any existing inheritance marker on financePerms) and use that flag
in place of financePerms.can_manage_content !==
financePerms.effective_can_manage_content so the disabled Tooltip/disabled
Button branch triggers for Board members regardless of the raw
can_manage_content value; update the conditional around financePerms, the
Tooltip and the Button (references: user?.groups?.includes('Board'),
profileType, financePerms, toggleContentManagerRole) accordingly.
---
Nitpick comments:
In @.github/workflows/deploy-production.yml:
- Around line 36-37: When bumping the MINOR version you should reset the PATCH
counter for SemVer consistency: change the logic around the MINOR and PATCH
variables so that PATCH is set to 0 and then MINOR is incremented (i.e., replace
the current code that leaves PATCH unchanged and only increments MINOR). Update
the lines that reference the PATCH and MINOR variables (PATCH and MINOR) to
ensure PATCH=0 and MINOR is incremented in that order.
In `@backend/treasury/models.py`:
- Around line 114-118: The clean() validation for
TransactionType.RIMBORSO_ESNCARD currently raises ValueError (in the block
checking self.subscription and self.esncard); change these to
django.core.exceptions.ValidationError so model-level validation surfaces as
HTTP 400 when TransactionSerializer.create() calls Transaction.objects.create()
(save() calls clean()). Update all ValueError raises in the Transaction.clean()
method to raise ValidationError with the same messages and add the appropriate
import (ValidationError) at the top of the file.
In `@backend/users/views.py`:
- Around line 379-380: Remove the redundant validation that checks
target.profile.is_esner when content_fields is set: the earlier check that
rejects non-ESNer targets already handles this, so delete the conditional "if
content_fields and not target.profile.is_esner: return Response(...)" (leave the
initial is_esner validation intact) and ensure any logic that depends on
content_fields proceeds without this duplicate guard.
In `@frontend/src/Components/treasury/TransactionModal.jsx`:
- Line 80: The useEffect in TransactionModal references the in-component array
negative_types but doesn't include it in the dependency array, causing a
stale-closure/ESLint warning; fix by moving the negative_types declaration out
of the TransactionModal component to module scope (e.g., define const
negative_types = [...] above the component) so it is stable across renders, then
keep the useEffect dependency array as [open, transaction]; update any
references inside the component to use that module-scope negative_types.
In `@frontend/src/Pages/events/EventPayment.jsx`:
- Around line 150-163: The onError handler currently assumes errorResponse.json
exists and swallows errors; update the onError callback to first check that
errorResponse is an object and typeof errorResponse.json === 'function' before
calling json(), fall back to calling errorResponse.text() if json parsing fails
or if json isn't available, and for non-Response errors (strings, Error objects)
extract message via errorResponse.message || String(errorResponse); ensure the
caught parsing error is logged or included in apiMessage and then call
setStatus('failed') and setMessage(apiMessage || 'Payment confirmation error.').
Use the identifiers onError, errorResponse, apiMessage, setStatus and setMessage
to locate and change the code.
🪄 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: dd8b7d7d-d6a9-4821-86c6-75fa9c33e9b9
📒 Files selected for processing (45)
.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/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/tests.pybackend/events/views.pybackend/treasury/models.pybackend/treasury/tests.pybackend/treasury/views.pybackend/users/models.pybackend/users/serializers.pybackend/users/views.pyfrontend/build/assets/index-BKm4YlQa.jsfrontend/build/assets/index-BMkv-w2E.jsfrontend/build/index.htmlfrontend/src/Components/events/EventListAccordions.jsxfrontend/src/Components/events/ReimburseSelectionModal.jsxfrontend/src/Components/treasury/TransactionModal.jsxfrontend/src/Pages/events/Event.jsxfrontend/src/Pages/events/EventPayment.jsxfrontend/src/Pages/profiles/Profile.jsxfrontend/src/data/transactionConfigs.jsfrontend/src/utils/displayAttributes.jsxfrontend/src/utils/useMaintenanceNotification.js
💤 Files with no reviewable changes (10)
- backend/events.md
- backend/docs/test_specifications/01_USERS_MODULE.md
- backend/docs/test_specifications/03_EVENTS_MODULE.md
- README_SHARED_LISTS_FINAL.md
- backend/docs/test_specifications/06_INTEGRATION_E2E.md
- backend/docs/test_specifications/05_CONTENT_MODULE.md
- backend/docs/test_specifications/04_TREASURY_MODULE.md
- backend/docs/test_specifications/00_OVERVIEW.md
- backend/docs/test_specifications/02_PROFILES_MODULE.md
- backend/docs/TEST_COVERAGE_REPORT.md
📜 Review details
⏰ 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: Agent
🧰 Additional context used
🪛 GitHub Check: CodeQL
backend/treasury/views.py
[warning] 248-248: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 LanguageTool
backend/Project Documentation/06_INTEGRATION_E2E.md
[grammar] ~88-~88: Ensure spelling is correct
Context: ...with automatic disable for invalid items 3. orchestrazione chiamate reimburse_quota e/o reimburse_deposits ...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~139-~139: Ensure spelling is correct
Context: ...d maintenance/status 3. banner frontend visibile 4. clear notification Test oracles: - correct ...
(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] ~65-~65: Ensure spelling is correct
Context: ...thout linked documents, student IDs, or ESNcards. ### Fetch specified profile * `GET /profile...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[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)
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/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/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)
🪛 markdownlint-cli2 (0.22.0)
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)
backend/Project Documentation/04_TREASURY_MODULE.md
[warning] 4-4: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🪛 Ruff (0.15.7)
backend/content/views.py
[warning] 200-200: Comment contains ambiguous – (EN DASH). Did you mean - (HYPHEN-MINUS)?
(RUF003)
🔇 Additional comments (44)
frontend/build/index.html (1)
15-15: Build artifact update for deployment.This is a standard Vite build output filename change reflecting the new bundle hash. Consider whether committing build artifacts to the repository is intentional or if a CI/CD pipeline should handle builds instead.
backend/treasury/models.py (1)
79-79: New transaction type addition looks correct.The
RIMBORSO_ESNCARDtype is properly added to the enum and follows the established naming convention.frontend/src/utils/displayAttributes.jsx (1)
68-68: Display label addition is correct.The new
rimborso_esncardlabel aligns with the backendRIMBORSO_ESNCARDtransaction type and follows the existing naming pattern.frontend/src/utils/useMaintenanceNotification.js (1)
54-55: Polling interval increase is reasonable.Changing from 30 seconds to 5 minutes significantly reduces server load. For maintenance notifications, the potential delay before users see the notification is acceptable.
Note:
lastNotificationIdis correctly scoped within the effect closure to persist across interval calls while resetting onaccessTokenchanges.backend/users/models.py (1)
18-18: Comment accurately reflects the updated authorization model.The updated comment correctly documents that
can_manage_contentis explicitly granted, with Board members having implicit access. This aligns with the authorization logic inbackend/users/views.pyandbackend/content/views.py.frontend/src/Components/treasury/TransactionModal.jsx (2)
16-16: Deletable types correctly synchronized with backend.The addition of
rimborso_esncardtodeletableTranTypesmatches the backend'slist_deleteableinbackend/treasury/views.py:453-460.
25-25: Negative amount handling is correct.Adding
rimborso_esncardtonegative_typesensures the refund transaction displays and submits with proper sign handling, consistent with other reimbursement types.frontend/src/data/transactionConfigs.js (1)
10-10: Transaction config follows established pattern.The
rimborso_esncardconfiguration usescolor: 'default', consistent with other reimbursement types (rimborso_cauzione,rimborso_quota,rimborso_service).backend/users/serializers.py (2)
73-76: Permission logic correctly updated.The virtual
manage_contentpermission is now granted based on Board membership or explicitcan_manage_contentflag, aligning withbackend/users/views.py:351-352andbackend/content/views.py:35-39.
96-97: Effective permission calculation is consistent.The simplified logic (
obj.can_manage_content or self._is_in(obj, 'Board')) matches the pattern used throughout the codebase for content management authorization.backend/content/views.py (2)
35-41: Clean centralization of authorization logic.The
_can_manage_content(user)helper effectively consolidates the permission check, ensuring consistency acrossIsContentManagerOrReadOnlyandwhatsapp_config. Usinggetattrwith a default handles the case where the attribute might not exist gracefully.
142-142: Authorization logic correctly simplified.The permission check now delegates to the shared helper, which aligns with the updated docstring and removes the previous
Attivigroup andcan_manage_cassereferences as documented in the AI summary.backend/content/tests.py (2)
142-154: Good addition of explicit content-role permission test.This test properly verifies that users with
can_manage_content=Truecan create sections even without Board or Attivi group membership, validating the new authorization model.
637-653: Comprehensive test for explicit content manager role on WhatsApp config.The test correctly validates both the HTTP response and the persisted state, ensuring the new permission model works end-to-end.
backend/README_Models.md (1)
1-33: Documentation successfully localized to English.The rewrite provides clear, structured documentation of the events/subscriptions module with accurate descriptions of list management, payment handling, and row operations.
backend/users/views.py (1)
354-355: Authorization helper correctly updated.The
effective_contenthelper now only grants effective content management permission to users with the explicit flag or Board membership, removing the previousAttivigroup check. This aligns with the serializer'sget_effective_can_manage_contentmethod (lines 96-97 inserializers.py).backend/Project Documentation/TEST_COVERAGE_REPORT.md (1)
1-131: Comprehensive test coverage report added.This documentation provides valuable visibility into the test suite, including:
- Clear test inventory with counts per module
- Functional coverage descriptions per domain
- Residual risk identification
- Maintenance policy for ongoing test hygiene
The AI agent interpretation notes are a thoughtful addition for automated tooling.
backend/README_APIs.md (2)
175-227: ESNcard API documentation accurately reflects implementation.The documented endpoints match the actual implementation:
POST /esncard_emission/withprofile_id,esncard_number,account_idparameters (verified againsttreasury/views.py:110-142)DELETE /esncard/<str:pk>/creatingrimborso_esncardrefund transaction (verified againsttreasury/views.py:184-245)- Error codes (401, 404, 409) correctly document the guarded conditions
6-7: Good terminology standardization.Updating "matricola/matricole" to "student ID (
matricola)" and "cassa" to "cash account" improves clarity for non-Italian speakers while preserving the technical terms in parentheses for reference.backend/Project Documentation/06_INTEGRATION_E2E.md (1)
1-216: Well-structured E2E specification document.The document provides comprehensive coverage of cross-module integration scenarios with clear acceptance criteria, test asset mapping, and risk identification. The traceability map (Section 8) is particularly useful for quick troubleshooting.
backend/Project Documentation/02_PROFILES_MODULE.md (3)
35-39: Static analysis false positive - Italian term is intentional.The term "relazione" is Italian for "relationship/relation" and is appropriate given the project's Italian context (ESN Polimi). The static analysis flagged this as a spelling error, but it's intentional domain terminology.
125-130: ESNcard revocation documentation aligns with new treasury functionality.The documentation correctly references the ESNcard revocation flow that generates a
rimborso_esncardtransaction, which matches the newRIMBORSO_ESNCARDtransaction type and tests added inbackend/treasury/tests.py.
1-172: Comprehensive module documentation.The documentation provides clear coverage of:
- Domain model (Profile, Document entities)
- API contract with authentication requirements
- Lifecycle flows for Erasmus and ESNer registration
- Authorization rules including object-level access
- Integration touchpoints with other modules
This is valuable for onboarding and AI-agent analysis.
backend/events/tests.py (3)
812-838: Well-structured test for form submission with full lists.The test correctly verifies that form submission remains allowed when Main/Waiting lists are full, and the subscription is placed in Form List with a checkout ID generated. Good coverage of the edge case.
1549-1575: Test appropriately validates payment blocking status.The test setup creates a full Main/Waiting list scenario and verifies that
payment_blocked,payment_blocked_reason, andpayment_blocked_messageare correctly returned. The assertions align with the backend implementation.
1643-1672: Comprehensive test for process_payment blocking.This test properly verifies:
- HTTP 409 status code for blocked payments
- Response contains
status="BLOCKED"anderror="sold_out"- Mocks are not called when payment is blocked (confirming early return)
The mock assertions
assert_not_called()are valuable for ensuring no unintended side effects.frontend/src/Pages/events/Event.jsx (5)
20-20: Import updated correctly to new unified modal.The import change from
ReimburseQuotaModaltoReimburseSelectionModalaligns with the PR objective of creating a unified reimbursement UI modal.
175-183: Handler refactoring looks correct.The
handleOpenReimburseDepositsnow properly setssingleSubToReimburse(line 175), and the newhandleOpenReimburseMenuhandler correctly sets the subscription and opens the selection modal. This maintains the same subscription reference for both deposit and selection reimbursement flows.
292-300: Modal props correctly match component interface.Based on the relevant code snippets,
ReimburseSelectionModalexpects{open, onClose, onRefresh, event, subscription}. The props provided here match exactly:
open={reimburseSelectionModalOpen}onClose={handleCloseReimburseSelectionModal}onRefresh={refreshEventData}event={data}subscription={singleSubToReimburse}
519-519: Callback prop renamed consistently.The prop
onOpenReimburseMenumatches the new handler namehandleOpenReimburseMenu, maintaining consistency with the refactored reimbursement flow.
217-224: Close handler properly resets state.The
handleCloseReimburseSelectionModalcorrectly:
- Closes the modal
- Shows success popup when applicable
- Refreshes event data on success
- Clears
singleSubToReimbursestateThe success/message handling works as intended:
ReimburseSelectionModalcallsonClose(true, 'Rimborso completato con successo')when the reimbursement succeeds, andonClose(false)when the user cancels. The popup will display correctly on successful completion.> Likely an incorrect or invalid review comment.backend/Project Documentation/07_MAINTENANCE_MODULE.md (1)
108-113: Operational risks well-documented.The documentation appropriately highlights critical operational concerns:
- Race conditions in multi-process environments
- State mismatch across instances
- Token exposure in query strings (security consideration)
- Worker saturation risks
These are important considerations for deployment and monitoring.
backend/treasury/tests.py (8)
295-308: Board-only authorization test correctly validates access control.The test verifies that non-Board members cannot delete ESNcards (HTTP 401) and that the card remains in the database after the failed attempt.
309-348: Thorough test for ESNcard revocation with refund.This test comprehensively validates the revocation flow:
- Creates emission transaction and verifies initial balance
- Deletes the ESNcard via DELETE endpoint
- Verifies the card is deleted but emission transaction preserved
- Verifies refund transaction created with correct negative amount
- Verifies response includes both transaction IDs
- Verifies account balance restored to original
Excellent coverage of the core revocation functionality.
350-365: Edge case: revocation without linked emission.Good coverage for the scenario where an ESNcard exists without an associated emission transaction (possibly created through other means). The test correctly expects null transaction IDs in the response.
367-401: Proper validation of insufficient balance rejection.The test creates a scenario where:
- ESNcard is emitted (+10.00)
- Withdrawal removes the balance (-10.00)
- Revocation attempted (would require -10.00 refund)
Correctly expects 409 status and verifies no refund transaction is created.
403-429: Closed account rejection test validates account state check.Tests that revocation fails with 409 when the emission account is closed, preventing refund transactions on inactive accounts.
431-462: Multiple linked emissions rejection prevents ambiguous refunds.This is an important safeguard - if somehow multiple emission transactions are linked to the same ESNcard (data corruption or edge case), the system correctly refuses to process the revocation rather than guessing which to refund.
464-489: Legacy/corrupted data handling test.The test simulates a scenario where an ESNcard is incorrectly linked to a non-ESNCARD transaction type (e.g., DEPOSIT). Using
Transaction.objects.filter(pk=tx.pk).update(esncard=card)to bypass model validation is a valid way to simulate corrupted data. The system correctly rejects this with 409.
1230-1260: External subscription support test is valuable.This test ensures the
reimbursable_depositsendpoint handles external subscriptions (whereprofile=None) without crashing. The assertions correctly verify:
profile_idis nullprofile_namefalls back toexternal_nameThis aligns with the AI summary mentioning response fallback behavior.
frontend/src/Pages/events/EventPayment.jsx (4)
6-6: Appropriate fallback message constant.The
SOLD_OUT_MESSAGEprovides a user-friendly message when the backend doesn't provide one. This aligns with the backend'sPAYMENT_SOLD_OUT_MESSAGEconstant.
54-65: Correct handling of payment blocking status.The logic properly:
- Checks
data.payment_blockedfrom the backend response- Sets failed status and displays the blocked message
- Falls back to
SOLD_OUT_MESSAGEif no message provided- Uses
sumup_checkout_idfrom response with fallback to existing stateThis aligns with the backend response structure shown in context snippet 1.
81-84: Cleanup function correctly structured.The cleanup function properly sets
canceled = trueto prevent state updates after unmount or dependency changes. This prevents potential memory leaks and "update on unmounted component" warnings.
86-113: Widget loading effect properly guards against race conditions.The effect correctly:
- Guards with
!checkoutId || !subscriptionIdandstatus !== 'init'- Uses
canceledflag for cleanup- Loads external script and verifies
window.SumUpCardavailabilityThe dependency array
[checkoutId, subscriptionId, status]is appropriate.
- 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.
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/events/views.py (1)
2102-2107:⚠️ Potential issue | 🟠 MajorSold-out submissions still receive a live checkout ID.
The checkout is created before capacity is classified as blocked, and
checkout_idis returned even whencapacity_blocked=True. A client can still attempt widget payment with that checkout despite sold-out state.💡 Suggested direction
- if event.allow_online_payment and total_cost > 0: + # Determine capacity block before creating SumUp checkout + main_list, waiting_list = _get_main_waiting_lists(event) + capacity_blocked = not _list_has_space(main_list) and not _list_has_space(waiting_list) + + if event.allow_online_payment and total_cost > 0 and not capacity_blocked: try: checkout_id, _ = create_sumup_checkout(sub, total_cost, currency="EUR") sub.sumup_checkout_id = checkout_id sub.save(update_fields=['sumup_checkout_id'])And avoid returning
checkout_idwhen blocked.Also applies to: 2118-2130, 2144-2146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/events/views.py` around lines 2102 - 2107, The code creates a SumUp checkout (create_sumup_checkout) and assigns sub.sumup_checkout_id before the submission capacity is classified as blocked, which allows a checkout_id to be returned for sold-out submissions; move the create_sumup_checkout call so it happens only after you determine capacity_blocked is False (the same capacity check used later in the view), and ensure you do not set or return sub.sumup_checkout_id or the checkout_id when capacity_blocked is True; if any existing code paths could create a checkout before the check, either postpone creation or cancel/rollback that checkout and clear sub.sumup_checkout_id so no live checkout is exposed (apply the same change to the other similar blocks around the create_sumup_checkout usage).
♻️ Duplicate comments (1)
backend/events/views.py (1)
2229-2252:⚠️ Potential issue | 🟠 MajorCapacity gating is still TOCTOU under concurrent payment attempts.
The sold-out check and payment finalization are still separate phases. Two requests can pass availability checks around the last spot and diverge later. This is the same root issue previously flagged.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/events/views.py` around lines 2229 - 2252, The capacity gating is vulnerable to TOCTOU because availability checks (via _is_payment_blocked_by_full_lists and _subscription_payment_already_registered) and finalization (_process_sumup_checkout / _ensure_sumup_transactions) are separate; fix by performing an atomic re-check-and-reserve inside a database transaction or using a DB row lock so only one concurrent request can pass: start a transaction, lock the relevant records (the subscription or the related list/event rows) with SELECT ... FOR UPDATE (or your ORM equivalent), re-evaluate _is_payment_blocked_by_full_lists and _subscription_payment_already_registered inside that lock, then either create the payment/registration record or proceed to call _process_sumup_checkout/_ensure_sumup_transactions and return; ensure the lock scope covers all rows those helper functions inspect and that you release the transaction after committing/rolling back.
🧹 Nitpick comments (14)
.github/workflows/deploy-production.yml (1)
35-36: Reset PATCH to0when bumping MINOR.At Line 35-36, a minor bump currently keeps the previous patch (e.g.,
v1.2.9 -> v1.3.9). That’s unusual for SemVer and can confuse consumers that infer release intent from the tag.Proposed fix
- PATCH=$((PATCH + 0)) - MINOR=$((MINOR + 1)) + PATCH=0 + MINOR=$((MINOR + 1))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/deploy-production.yml around lines 35 - 36, The workflow currently increments MINOR while leaving PATCH unchanged (PATCH=$((PATCH + 0))), causing tags like v1.2.9 -> v1.3.9; change the logic so that when bumping MINOR you set PATCH back to 0 and increment MINOR (i.e., update the MINOR/PATCH update block to set PATCH=0 and MINOR=$((MINOR + 1))). Ensure you only alter the section that updates the PATCH and MINOR variables (the PATCH and MINOR assignments) so minor bumps produce vX.Y.0 semantics.frontend/src/Pages/ESNerForm.jsx (1)
244-252: Same note as ErasmusForm:replaceAllwith/gflag is redundant.For consistency with any refactor applied to
ErasmusForm.jsx, consider usingreplace(/\D/g, '')here as well. Both approaches work identically.🔧 Optional: Use conventional replace
if (name === 'person_code') { - value = value.replaceAll(/\D/g, ''); + value = value.replace(/\D/g, ''); if (value.length > 8) value = value.slice(0, 8); } if (name === 'matricola_number') { // Only accept numeric digits, max 6 - value = value.replaceAll(/\D/g, ''); + value = value.replace(/\D/g, ''); if (value.length > 6) value = value.slice(0, 6); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/ESNerForm.jsx` around lines 244 - 252, The regex replacement calls in ESNerForm.jsx use replaceAll(/\D/g, '') which is redundant with the global flag; update both occurrences (the branch handling name === 'person_code' and the branch handling name === 'matricola_number') to use String.prototype.replace with the regex (e.g., replace(/\D/g, '')) to match the change made in ErasmusForm.jsx so behavior stays identical and code is consistent.frontend/src/Pages/profiles/Profile.jsx (2)
1410-1442: Consider extracting the IIFE to a named variable for readability.The logic is correct: Board members with
effective_can_manage_contenthave an implicit/inherited role that cannot be revoked from this UI. However, the inline IIFE pattern adds cognitive complexity.🔧 Optional: Extract conditional rendering
+ {/* Content Manager role toggle (Board → ESNer; Board target users inherit the role) */} + {user?.groups?.includes('Board') && profileType === 'ESNer' && (() => { + const isInheritedFromBoard = profile?.group === 'Board' && financePerms?.effective_can_manage_content; + if (!financePerms) return null; + if (isInheritedFromBoard) { + return ( + <Tooltip title="Il permesso è implicito per il gruppo Board e non può essere revocato da qui" arrow> + <span> + <Button variant="outlined" color="info" startIcon={<EditIcon/>} disabled> + Revoca Content Manager + </Button> + </span> + </Tooltip> + ); + } + return ( + <Tooltip title="Permette di vedere e modificare la pagina Gestione Contenuti" arrow> + <Button + variant={financePerms?.can_manage_content ? 'outlined' : 'contained'} + color="info" + startIcon={<EditIcon/>} + onClick={toggleContentManagerRole} + > + {financePerms?.can_manage_content ? 'Revoca Content Manager' : 'Concedi Content Manager'} + </Button> + </Tooltip> + ); + })()}Alternatively, extract this to a separate component or a
useMemoblock for cleaner JSX.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/profiles/Profile.jsx` around lines 1410 - 1442, Extract the inline IIFE used for rendering the Content Manager toggle into a named constant or small component to improve readability: compute the boolean isInheritedFromBoard (based on profile?.group === 'Board' && financePerms?.effective_can_manage_content) and the rendered JSX for the two cases (disabled Tooltip/Button vs actionable Tooltip/Button) outside the JSX return, referencing financePerms, profileType, user?.groups?.includes('Board'), toggleContentManagerRole and EditIcon; then replace the IIFE invocation with that named variable or component (or a useMemo) so the JSX becomes a simple reference.
1526-1526: Extract nested ternary to improve readability.Static analysis flagged the nested ternary. Consider extracting to a named variable for clarity.
🔧 Proposed fix
Add before the return statement or as a
useMemo:const createButtonText = useMemo(() => { if (!latestESNcard) return "Rilascia"; return latestESNcard.is_valid ? "Card Smarrita" : "Rinnova"; }, [latestESNcard]);Then use:
-createText={latestESNcard ? (latestESNcard.is_valid ? "Card Smarrita" : "Rinnova") : "Rilascia"} +createText={createButtonText}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/profiles/Profile.jsx` at line 1526, Replace the nested ternary used in the createText prop with a named variable to improve readability: compute a createButtonText value (e.g., using a local const or useMemo) that checks latestESNcard and returns "Rilascia" when falsy, otherwise returns latestESNcard.is_valid ? "Card Smarrita" : "Rinnova"; then update the JSX to pass createText={createButtonText} (referencing latestESNcard and the createText prop to locate where to change).frontend/src/Pages/ErasmusForm.jsx (1)
190-196: Functionally equivalent, butreplacewith/gflag is more conventional.Using
replaceAllwith a global regex (/gflag) is redundant sincereplacewith/galready replaces all occurrences. The change works correctly but offers no benefit. Consider keepingreplace(/\D/g, '')for clarity and broader familiarity.🔧 Optional: Revert to conventional replace
if (name === 'person_code') { - value = value.replaceAll(/\D/g, ''); + value = value.replace(/\D/g, ''); if (value.length > 8) value = value.slice(0, 8); } if (name === 'matricola_number') { // Only accept numeric digits, max 6 - value = value.replaceAll(/\D/g, ''); + value = value.replace(/\D/g, ''); if (value.length > 6) value = value.slice(0, 6); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Pages/ErasmusForm.jsx` around lines 190 - 196, Replace the use of replaceAll with a regex in ErasmusForm.jsx to the conventional replace call: locate the branches handling the generic field and the 'matricola_number' branch where value.replaceAll(/\D/g, '') is used and change them to use value.replace(/\D/g, '') so the code uses the standard replace with /g for global regex replacements.frontend/src/Components/events/SubscriptionModal.jsx (1)
204-215: Logic correctly differentiates ESN members from non-ESN members.The condition change is correct: ESN members (
is_esner=True) don't require ESNcard or matricola validation to subscribe to events since they're part of the organization. Only non-ESN members (students) need these checks.One minor asymmetry:
setProfileHasEsncardis not explicitly set for ESNers here, whereas theProfileSearchonChange handler at line 573 always sets it. For ESNers,profileHasEsncardstaysnull, which doesn't trigger blocking (null === falseisfalse), so this works correctly. However, for consistency and clarity, consider explicitly resetting it:🔧 Optional: Explicitly reset `profileHasEsncard` for ESNers
if (profileData.is_esner) { + setProfileHasEsncard(null); setMatricolaStatus({ isMissing: false, isExpired: false }); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/events/SubscriptionModal.jsx` around lines 204 - 215, In the is_esner branch, explicitly reset the ESN card state to avoid leaving profileHasEsncard as null; update the block handling profileData.is_esner to call setProfileHasEsncard(null) (or false if you prefer a boolean) alongside setMatricolaStatus so the component state is consistent—modify the branch that checks profileData.is_esner and the usage of setProfileHasEsncard to set a clear value.backend/treasury/views.py (4)
31-33: Minor import organization: consider moving the constant after all imports.The
MSG_UNAUTHORIZEDconstant is defined between imports (from django.utils import timezonefollows on line 33). While this works, grouping all imports together before constants improves readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/treasury/views.py` around lines 31 - 33, Move the MSG_UNAUTHORIZED constant so all import statements stay grouped together: keep the existing import from django.utils import timezone (and any other imports) together at the top of the file, then define MSG_UNAUTHORIZED = 'Non autorizzato.' immediately after the import block; update any references to MSG_UNAUTHORIZED as needed but do not change its value.
186-188: HTTP 401 is semantically incorrect for authorization failures; 403 is more appropriate.HTTP 401 ("Unauthorized") indicates missing/invalid authentication credentials, whereas 403 ("Forbidden") indicates the server understood the request but the authenticated user lacks permission. This pattern is used throughout the file for consistency, but it may confuse API consumers expecting standard HTTP semantics.
📋 Suggested change
elif request.method == 'DELETE': if not get_action_permissions('esncard_detail_delete', request.user): - return Response({'error': MSG_UNAUTHORIZED}, status=401) + return Response({'error': MSG_UNAUTHORIZED}, status=403)Note: This would be a breaking change if API consumers depend on the current 401 status code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/treasury/views.py` around lines 186 - 188, In the DELETE branch where you call get_action_permissions('esncard_detail_delete', request.user) and return Response({'error': MSG_UNAUTHORIZED}, status=401), change the HTTP status from 401 to 403 to reflect a permission/forbidden response; if a MSG_FORBIDDEN constant exists prefer using that message instead of MSG_UNAUTHORIZED, and update any related tests/docs to reflect the status code change.
670-670: Note: Line 670 correctly uses HTTP 403 for authorization failure.This is the semantically correct status code for "authenticated but not authorized" scenarios. Consider aligning other authorization failures (currently using 401) with this pattern for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/treasury/views.py` at line 670, The review notes that this view correctly returns HTTP 403 for authorization failures (Response({'error': MSG_UNAUTHORIZED}, status=403)); update other places that currently return status=401 for "authenticated but not authorized" scenarios to use status=403 for consistency. Search for other Response(..., status=401) or uses of MSG_UNAUTHORIZED in view functions or methods (e.g., any view that checks permissions/authz) and change the status code to 403 while keeping the same error payload and message identifiers.
272-274: Consider using 404 for PATCH as well for REST consistency.DELETE correctly returns 404 when the ESNcard doesn't exist. However, PATCH returns 400 which is typically used for malformed requests, not missing resources. Both operations target the same resource by
pk, so 404 would be more semantically consistent.📋 Suggested change
except ESNcard.DoesNotExist: - not_found_status = 404 if request.method == 'DELETE' else 400 - return Response({'error': 'La ESNcard non esiste'}, status=not_found_status) + return Response({'error': 'La ESNcard non esiste'}, status=404)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/treasury/views.py` around lines 272 - 274, The current exception handler checks ESNcard.DoesNotExist and sets not_found_status = 404 if request.method == 'DELETE' else 400, causing PATCH to return 400; change the logic so missing ESNcard always returns 404 (regardless of request.method), i.e. set not_found_status = 404 (or remove the conditional) before returning Response({'error': 'La ESNcard non esiste'}, status=not_found_status) to ensure both DELETE and PATCH return HTTP 404 for a non-existent resource.frontend/src/Components/events/ReimburseSelectionModal.jsx (1)
107-107: Add parentheses for clarity.Due to operator precedence,
&&binds tighter than||, so the current expression works correctly. However, explicit parentheses improve readability and prevent future confusion.✨ Suggested clarity improvement
- const hasServices = servicesTotal > 0 || servicesStatus !== null && servicesStatus !== undefined; + const hasServices = servicesTotal > 0 || (servicesStatus !== null && servicesStatus !== undefined);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/src/Components/events/ReimburseSelectionModal.jsx` at line 107, The boolean expression assigned to hasServices is correct but ambiguous due to operator precedence; update the initializer for hasServices (the const hasServices variable) to use explicit parentheses around the && sub-expression, e.g. (servicesTotal > 0 || (servicesStatus !== null && servicesStatus !== undefined)), so it’s clear that servicesStatus checks are grouped together.backend/events/tests.py (2)
814-839: Strengthen this test with an explicitcapacity_blockedassertion.This case is specifically about sold-out handling, so assert the response contract directly (
capacity_blocked=True) to prevent silent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/events/tests.py` around lines 814 - 839, Add an explicit assertion that the response indicates sold-out handling by checking response.data["capacity_blocked"] is True in the test_event_form_submit_online_payment_allows_form_list_when_main_waiting_full test; after the current assertions on response.status_code and response.data["success"], add self.assertTrue(response.data["capacity_blocked"]) (or equivalent assertEqual to True) so the test explicitly verifies the capacity_blocked contract when Main/Waiting are full.
1645-1672: Assert reconciliation probe is still executed before blocking.Please assert
_process_sumup_checkoutis called once in this blocked-path test. That preserves the regression guard for the “already paid remotely” reconciliation behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/events/tests.py` around lines 1645 - 1672, The test test_subscription_process_payment_blocked_when_lists_full currently checks the 409 blocked response but does not assert that the reconciliation probe is still executed; add an assertion that the mocked _process_sumup_checkout (mock_process) was called exactly once to preserve the “already paid remotely” reconciliation guard. After the existing response assertions in test_subscription_process_payment_blocked_when_lists_full, add mock_process.assert_called_once() (or equivalent) to verify _process_sumup_checkout was invoked once before the blocking logic returns.backend/events/views.py (1)
476-521: Refactor_send_form_subscription_emailto reduce cognitive complexity (Sonar gate).The branching in this function is over the configured threshold and currently fails Sonar quality checks.
♻️ Refactor sketch
-def _send_form_subscription_email(subscription, assigned_label, online_payment_required, payment_required, capacity_blocked=False): +def _send_form_subscription_email(subscription, assigned_label, online_payment_required, payment_required, capacity_blocked=False): + # guard clauses... + html_parts = _build_form_subscription_email_body( + subscription=subscription, + assigned_label=assigned_label, + online_payment_required=online_payment_required, + payment_required=payment_required, + capacity_blocked=capacity_blocked, + ) + ... + +def _build_form_subscription_email_body(*, subscription, assigned_label, online_payment_required, payment_required, capacity_blocked): + # isolate branching and return assembled parts + ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@backend/events/views.py` around lines 476 - 521, The _send_form_subscription_email function exceeds cognitive complexity due to nested branching around capacity_blocked, payment_required and online_payment_required; refactor by extracting small focused helpers (e.g., build_capacity_blocked_message(subscription,event), build_payment_required_message(event, subscription, online_payment_required, notify_lists, waiting, main_available), build_no_payment_message()) and replace the big if/elif/else block with a simple dispatcher that appends the returned HTML parts to html_parts; keep existing early returns (created_by_form, subscription_confirmation_email_sent, recipient) and preserve variables used (assigned_label_lower, waiting, main_available, notify_lists, pay_link generation using settings.SCHEME_HOST, subject, html_parts) so behavior is unchanged while reducing nesting and complexity in _send_form_subscription_email.
🤖 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 11-12: The create-github-release job is missing an explicit
permissions block which prevents actions/create-release@v1 from having write
access; add a permissions: contents: write entry to the create-github-release
job definition (the job named create-github-release that calls
actions/create-release@v1) so the GITHUB_TOKEN has repository write permissions
required to create releases.
In `@backend/events/views.py`:
- Around line 313-314: The generator expressions using the ambiguous
single-letter variable `l` in computing `main_list = next((l for l in all_lists
if l.is_main_list), None)` and `waiting_list = next((l for l in all_lists if
l.is_waiting_list), None)` should be renamed to a clearer identifier (e.g.,
`lst` or `list_obj`) to satisfy Ruff E741 and CI; update both generator
expressions to use the new name consistently (for example `next((lst for lst in
all_lists if lst.is_main_list), None)` and similarly for `waiting_list`) so the
semantics remain identical but the variable is no longer a single-letter `l`.
In `@backend/README_Content.md`:
- Line 11: Update the permission wording that currently reads "only Board" to
match endpoint docs by saying "Board members and users with
can_manage_content=True" (or equivalently "users with can_manage_content=True,
including Board members") wherever it appears (specifically the lines containing
the phrase "only Board" referenced in the diff), so the README_Content.md
language is consistent with the permission model described on Line 51 and in the
endpoints.
- Line 35: The fenced code block that begins with ``` and contains the API list
(e.g., the line "GET /backend/content/sections/ - list
sections" and following lines) must declare a language for markdownlint MD040;
update the opening fence from ``` to ```text (or another appropriate language)
so the block becomes fenced with a language specifier.
- Around line 106-108: Update the README sentence describing who can access the
content management page: change "only Board members" to "Board members and users
with the Content Manager role (grantable by Board members via the Profiles
page)". Keep the existing references to the sidebar item "Content Management"
and the direct URL `/content-manager`, and note that the implementation checks
for the `can_manage_content=True` permission when determining access.
---
Outside diff comments:
In `@backend/events/views.py`:
- Around line 2102-2107: The code creates a SumUp checkout
(create_sumup_checkout) and assigns sub.sumup_checkout_id before the submission
capacity is classified as blocked, which allows a checkout_id to be returned for
sold-out submissions; move the create_sumup_checkout call so it happens only
after you determine capacity_blocked is False (the same capacity check used
later in the view), and ensure you do not set or return sub.sumup_checkout_id or
the checkout_id when capacity_blocked is True; if any existing code paths could
create a checkout before the check, either postpone creation or cancel/rollback
that checkout and clear sub.sumup_checkout_id so no live checkout is exposed
(apply the same change to the other similar blocks around the
create_sumup_checkout usage).
---
Duplicate comments:
In `@backend/events/views.py`:
- Around line 2229-2252: The capacity gating is vulnerable to TOCTOU because
availability checks (via _is_payment_blocked_by_full_lists and
_subscription_payment_already_registered) and finalization
(_process_sumup_checkout / _ensure_sumup_transactions) are separate; fix by
performing an atomic re-check-and-reserve inside a database transaction or using
a DB row lock so only one concurrent request can pass: start a transaction, lock
the relevant records (the subscription or the related list/event rows) with
SELECT ... FOR UPDATE (or your ORM equivalent), re-evaluate
_is_payment_blocked_by_full_lists and _subscription_payment_already_registered
inside that lock, then either create the payment/registration record or proceed
to call _process_sumup_checkout/_ensure_sumup_transactions and return; ensure
the lock scope covers all rows those helper functions inspect and that you
release the transaction after committing/rolling back.
---
Nitpick comments:
In @.github/workflows/deploy-production.yml:
- Around line 35-36: The workflow currently increments MINOR while leaving PATCH
unchanged (PATCH=$((PATCH + 0))), causing tags like v1.2.9 -> v1.3.9; change the
logic so that when bumping MINOR you set PATCH back to 0 and increment MINOR
(i.e., update the MINOR/PATCH update block to set PATCH=0 and MINOR=$((MINOR +
1))). Ensure you only alter the section that updates the PATCH and MINOR
variables (the PATCH and MINOR assignments) so minor bumps produce vX.Y.0
semantics.
In `@backend/events/tests.py`:
- Around line 814-839: Add an explicit assertion that the response indicates
sold-out handling by checking response.data["capacity_blocked"] is True in the
test_event_form_submit_online_payment_allows_form_list_when_main_waiting_full
test; after the current assertions on response.status_code and
response.data["success"], add self.assertTrue(response.data["capacity_blocked"])
(or equivalent assertEqual to True) so the test explicitly verifies the
capacity_blocked contract when Main/Waiting are full.
- Around line 1645-1672: The test
test_subscription_process_payment_blocked_when_lists_full currently checks the
409 blocked response but does not assert that the reconciliation probe is still
executed; add an assertion that the mocked _process_sumup_checkout
(mock_process) was called exactly once to preserve the “already paid remotely”
reconciliation guard. After the existing response assertions in
test_subscription_process_payment_blocked_when_lists_full, add
mock_process.assert_called_once() (or equivalent) to verify
_process_sumup_checkout was invoked once before the blocking logic returns.
In `@backend/events/views.py`:
- Around line 476-521: The _send_form_subscription_email function exceeds
cognitive complexity due to nested branching around capacity_blocked,
payment_required and online_payment_required; refactor by extracting small
focused helpers (e.g., build_capacity_blocked_message(subscription,event),
build_payment_required_message(event, subscription, online_payment_required,
notify_lists, waiting, main_available), build_no_payment_message()) and replace
the big if/elif/else block with a simple dispatcher that appends the returned
HTML parts to html_parts; keep existing early returns (created_by_form,
subscription_confirmation_email_sent, recipient) and preserve variables used
(assigned_label_lower, waiting, main_available, notify_lists, pay_link
generation using settings.SCHEME_HOST, subject, html_parts) so behavior is
unchanged while reducing nesting and complexity in
_send_form_subscription_email.
In `@backend/treasury/views.py`:
- Around line 31-33: Move the MSG_UNAUTHORIZED constant so all import statements
stay grouped together: keep the existing import from django.utils import
timezone (and any other imports) together at the top of the file, then define
MSG_UNAUTHORIZED = 'Non autorizzato.' immediately after the import block; update
any references to MSG_UNAUTHORIZED as needed but do not change its value.
- Around line 186-188: In the DELETE branch where you call
get_action_permissions('esncard_detail_delete', request.user) and return
Response({'error': MSG_UNAUTHORIZED}, status=401), change the HTTP status from
401 to 403 to reflect a permission/forbidden response; if a MSG_FORBIDDEN
constant exists prefer using that message instead of MSG_UNAUTHORIZED, and
update any related tests/docs to reflect the status code change.
- Line 670: The review notes that this view correctly returns HTTP 403 for
authorization failures (Response({'error': MSG_UNAUTHORIZED}, status=403));
update other places that currently return status=401 for "authenticated but not
authorized" scenarios to use status=403 for consistency. Search for other
Response(..., status=401) or uses of MSG_UNAUTHORIZED in view functions or
methods (e.g., any view that checks permissions/authz) and change the status
code to 403 while keeping the same error payload and message identifiers.
- Around line 272-274: The current exception handler checks ESNcard.DoesNotExist
and sets not_found_status = 404 if request.method == 'DELETE' else 400, causing
PATCH to return 400; change the logic so missing ESNcard always returns 404
(regardless of request.method), i.e. set not_found_status = 404 (or remove the
conditional) before returning Response({'error': 'La ESNcard non esiste'},
status=not_found_status) to ensure both DELETE and PATCH return HTTP 404 for a
non-existent resource.
In `@frontend/src/Components/events/ReimburseSelectionModal.jsx`:
- Line 107: The boolean expression assigned to hasServices is correct but
ambiguous due to operator precedence; update the initializer for hasServices
(the const hasServices variable) to use explicit parentheses around the &&
sub-expression, e.g. (servicesTotal > 0 || (servicesStatus !== null &&
servicesStatus !== undefined)), so it’s clear that servicesStatus checks are
grouped together.
In `@frontend/src/Components/events/SubscriptionModal.jsx`:
- Around line 204-215: In the is_esner branch, explicitly reset the ESN card
state to avoid leaving profileHasEsncard as null; update the block handling
profileData.is_esner to call setProfileHasEsncard(null) (or false if you prefer
a boolean) alongside setMatricolaStatus so the component state is
consistent—modify the branch that checks profileData.is_esner and the usage of
setProfileHasEsncard to set a clear value.
In `@frontend/src/Pages/ErasmusForm.jsx`:
- Around line 190-196: Replace the use of replaceAll with a regex in
ErasmusForm.jsx to the conventional replace call: locate the branches handling
the generic field and the 'matricola_number' branch where
value.replaceAll(/\D/g, '') is used and change them to use value.replace(/\D/g,
'') so the code uses the standard replace with /g for global regex replacements.
In `@frontend/src/Pages/ESNerForm.jsx`:
- Around line 244-252: The regex replacement calls in ESNerForm.jsx use
replaceAll(/\D/g, '') which is redundant with the global flag; update both
occurrences (the branch handling name === 'person_code' and the branch handling
name === 'matricola_number') to use String.prototype.replace with the regex
(e.g., replace(/\D/g, '')) to match the change made in ErasmusForm.jsx so
behavior stays identical and code is consistent.
In `@frontend/src/Pages/profiles/Profile.jsx`:
- Around line 1410-1442: Extract the inline IIFE used for rendering the Content
Manager toggle into a named constant or small component to improve readability:
compute the boolean isInheritedFromBoard (based on profile?.group === 'Board' &&
financePerms?.effective_can_manage_content) and the rendered JSX for the two
cases (disabled Tooltip/Button vs actionable Tooltip/Button) outside the JSX
return, referencing financePerms, profileType, user?.groups?.includes('Board'),
toggleContentManagerRole and EditIcon; then replace the IIFE invocation with
that named variable or component (or a useMemo) so the JSX becomes a simple
reference.
- Line 1526: Replace the nested ternary used in the createText prop with a named
variable to improve readability: compute a createButtonText value (e.g., using a
local const or useMemo) that checks latestESNcard and returns "Rilascia" when
falsy, otherwise returns latestESNcard.is_valid ? "Card Smarrita" : "Rinnova";
then update the JSX to pass createText={createButtonText} (referencing
latestESNcard and the createText prop to locate where to change).
🪄 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: d3672b17-6b12-4e54-b353-3d5be93867fd
📒 Files selected for processing (28)
.github/workflows/deploy-production.ymlbackend/Project Documentation/06_INTEGRATION_E2E.mdbackend/Project Documentation/07_MAINTENANCE_MODULE.mdbackend/README_Content.mdbackend/backend/db_audit.pybackend/content/views.pybackend/events/models.pybackend/events/tests.pybackend/events/views.pybackend/profiles/tests.pybackend/profiles/views.pybackend/treasury/serializers.pybackend/treasury/tests.pybackend/treasury/views.pybackend/users/views.pyfrontend/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/EventPayment.jsxfrontend/src/Pages/profiles/Profile.jsxfrontend/src/Pages/treasury/AccountsList.jsxfrontend/src/utils/useMaintenanceNotification.js
💤 Files with no reviewable changes (1)
- backend/events/models.py
✅ Files skipped from review due to trivial changes (9)
- frontend/src/Components/treasury/AccountsDash.jsx
- backend/backend/db_audit.py
- frontend/src/Pages/treasury/AccountsList.jsx
- frontend/src/Components/treasury/TransactionModal.jsx
- backend/profiles/views.py
- backend/treasury/serializers.py
- backend/profiles/tests.py
- backend/Project Documentation/06_INTEGRATION_E2E.md
- backend/Project Documentation/07_MAINTENANCE_MODULE.md
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/users/views.py
- frontend/src/utils/useMaintenanceNotification.js
- backend/treasury/tests.py
- frontend/src/Pages/events/EventPayment.jsx
- backend/content/views.py
- frontend/src/Components/events/EventListAccordions.jsx
📜 Review details
🧰 Additional context used
🪛 GitHub Check: CodeQL
backend/events/views.py
[warning] 2241-2241: Information exposure through an exception
Stack trace information flows to this location and may be exposed to an external user.
🪛 GitHub Check: SonarCloud Code Analysis
frontend/src/Components/events/ReimburseSelectionModal.jsx
[warning] 168-168: 'subscription.id' is missing in props validation
frontend/src/Pages/profiles/Profile.jsx
[warning] 1526-1526: Extract this nested ternary operation into an independent statement.
backend/events/views.py
[failure] 476-476: Refactor this function to reduce its Cognitive Complexity from 20 to the 15 allowed.
🪛 markdownlint-cli2 (0.22.0)
backend/README_Content.md
[warning] 35-35: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 Ruff (0.15.7)
backend/events/views.py
[error] 313-313: Ambiguous variable name: l
(E741)
[error] 314-314: Ambiguous variable name: l
(E741)
🔇 Additional comments (28)
frontend/src/Components/events/EditAnswersModal.jsx (1)
289-289: Good cleanup: call signature now matches helper definition.This change is correct and keeps the
renderFieldInputinvocation aligned with its 3-argument signature.frontend/src/Components/MaintenanceBanner.jsx (2)
4-4: Good addition of runtime prop validation import.Importing
prop-typeshere is appropriate and keeps runtime contract checks explicit in this component.
108-111: PropTypes contract is correctly defined and strict.Requiring both
messageandonClosematches the component’s actual expectations and helps catch integration mistakes early in development..github/workflows/deploy-production.yml (1)
53-54: Nice improvement: job-scoped push permissions for deploy jobs.Line 53-54 and Line 91-92 correctly scope
contents: writeto only the jobs that push branches, which is a cleaner least-privilege setup than broad workflow-wide write access.Also applies to: 91-92
frontend/src/Pages/ErasmusForm.jsx (3)
119-131: LGTM! WhatsApp validation logic is now correct.The validation properly clears errors when
sameWAasPhoneis true (fields are hidden and copied from phone) and validates as required when false (separate WhatsApp number is provided). This aligns with the conditional rendering at line 572.
269-275: LGTM! Simplified boolean expression is correct.The condition correctly identifies the starting anchor based on semester type: autumn semesters target February anchors (month === 1), spring semesters target July anchors (month === 6).
279-282: LGTM! Modern array access syntax improves readability.Using
.at(-1)is a clean ES2022 idiom for accessing the last element. The fallback logic ensures a valid date is always returned even iftargetIndexexceeds the array bounds.frontend/src/Pages/profiles/Profile.jsx (5)
92-93: LGTM! State declarations follow existing patterns.The new state variables for ESNcard revocation (
revokeESNcardConfirmOpen,revokingESNcard) are consistent with the existing profile deletion pattern (deleteConfirmOpen,deletingProfile).
190-190: LGTM! Derived variable centralizes ESNcard access.Extracting
latestESNcardas a derived constant improves consistency and avoids repeated optional chaining onprofile?.latest_esncardthroughout the component.
901-930: LGTM! Revocation handlers are well-structured.The implementation correctly:
- Guards against missing ESNcard before opening the dialog
- Validates
latestESNcard.idbefore the DELETE request- Keeps the dialog open on error (allowing retry)
- Refreshes profile data on success
- Properly manages loading state in
onFinallyThe error handling via
defaultErrorHandlerwill appropriately surface backend responses (401, 404, 409).
1499-1520: LGTM! Revoke button UI is well-implemented.The button correctly:
- Gates access to Board members only
- Disables when no ESNcard exists or revocation is in progress
- Provides contextual tooltip explaining the disabled state
- Uses
<span>wrapper to allow Tooltip display on disabled buttons (MUI requirement)
1631-1662: LGTM! Confirmation dialog follows established patterns.The dialog correctly:
- Prevents closing during the revocation operation
- Displays the ESNcard number for user confirmation
- Explains the consequences (refund transaction creation)
- Disables actions during in-flight requests
- Maintains consistent UX with the existing profile deletion dialog
frontend/src/Pages/ESNerForm.jsx (1)
157-169: LGTM! WhatsApp validation logic is consistent with ErasmusForm.The validation correctly handles both cases:
sameWAasPhone === true: clears errors (fields hidden, values copied from phone)sameWAasPhone === false: validates as required (separate WhatsApp number)backend/treasury/views.py (7)
236-253: RIMBORSO_ESNCARD correctly omitssubscriptionandesncardfields per model constraints.The model validation at
backend/treasury/models.py:114-118explicitly forbids settingsubscriptionoresncardonRIMBORSO_ESNCARDtransactions:if self.type == self.TransactionType.RIMBORSO_ESNCARD: if self.subscription: raise ValueError("Le transazioni di Rimborso ESNcard non devono avere un'Iscrizione.") if self.esncard: raise ValueError("Le transazioni di Rimborso ESNcard non devono avere una ESNcard associata.")The implementation correctly relies on the description field (which includes the emission transaction ID) for audit traceability. The previous review comment suggesting structured fields would actually violate the model constraints.
247-251: Stack trace is logged server-side only, not exposed to users.The
logger.exception()call writes the stack trace to server logs for debugging purposes, while the HTTP response contains only a static generic error message. This is the correct pattern for error handling — it aids debugging without leaking implementation details to external users.
64-65: LGTM!Board-only restriction for ESNcard deletion is appropriate given the financial implications of the refund operation.
190-214: Good defensive guards and proper row locking for concurrent access.The implementation correctly:
- Uses
select_for_update()to prevent race conditions on the ESNcard, linked transactions, and account- Rejects revocation when non-ESNcard transactions are linked (line 198-202)
- Prevents ambiguous scenarios with multiple emission transactions (line 210-214)
813-840: LGTM! Good query optimization and defensive null handling.The refactor:
- Uses
select_related/prefetch_relatedto eliminate N+1 queries- Correctly iterates over prefetched
transaction_set.all()without triggering new queries- Handles
profile=Nonecases gracefully with fallback toexternal_nameor"Esterno"
456-463: LGTM!Adding
RIMBORSO_ESNCARDto deletable transaction types is consistent with other refund types and enables error correction when needed.
1046-1048: LGTM!Export functions correctly handle
RIMBORSO_ESNCARDwith appropriate labels that match the existing "Quota Associativa" naming convention for ESNcard transactions.Also applies to: 1077-1078
frontend/src/Components/events/ReimburseSelectionModal.jsx (8)
23-57: LGTM!The helper functions
formatApiError,extractErrorMessage, andmakeInitialSelectionare well-implemented with comprehensive error format handling and defensive coding practices.
59-82: LGTM!State management is appropriate for the component's complexity. The
servicesTotalmemo has good defensive coding withNumber.isFinitechecks and proper handling of edge cases.
153-168: Good fix on the dependency array.The change from
[open, refundItems]to[open, subscription?.id]correctly preserves the retry flow. Note thatrefundItemsis referenced at line 161 but intentionally omitted from dependencies to avoid resets during partial-success refreshes.Consider adding an ESLint disable comment to document this intentional omission if your lint rules flag it:
// eslint-disable-next-line react-hooks/exhaustive-deps }, [open, subscription?.id]);
177-227: LGTM!The
callPostwrapper provides clean error handling that always resolves (enabling sequential processing). ThehandleSubmitvalidation thoroughly covers edge cases including the business rule about services-only refunds.
229-319: LGTM!The submission logic handles the complex partial-success scenario well:
- Sequential API calls with individual success/error tracking
- Completed items get unchecked and marked for visual feedback
- Partial failures display detailed per-item error messages
onRefreshis correctly called only when there are successes
321-342: LGTM!Clean status rendering with correct priority: result state → completed → disabled reason → available.
398-438: LGTM!Form controls are properly wired with appropriate disabled states. The account selector correctly handles closed accounts with visual indication, and the submit button appropriately shows loading state during submission.
356-356: No action needed. Thesize={{xs: 12}}syntax is the correct Grid API for MUI v7.3.1, which this project uses. The Grid component will render correctly.> Likely an incorrect or invalid review comment.
|
Summary by CodeRabbit
New Features
Improvements
Chores
Other