diff --git a/.gitignore b/.gitignore index bea9510..12ec773 100644 --- a/.gitignore +++ b/.gitignore @@ -37,6 +37,5 @@ test-results/ # Release artifacts artifacts/ -# agent -plan.md -review.md \ No newline at end of file +# Plan verification tracking +.plans/.verified_plans \ No newline at end of file diff --git a/.plans/issue-124-nav-sidebar-fix.md b/.plans/done/issue-124-nav-sidebar-fix.md similarity index 100% rename from .plans/issue-124-nav-sidebar-fix.md rename to .plans/done/issue-124-nav-sidebar-fix.md diff --git a/.plans/issue-18-members-migration.md b/.plans/done/issue-18-members-migration.md similarity index 100% rename from .plans/issue-18-members-migration.md rename to .plans/done/issue-18-members-migration.md diff --git a/.plans/issue-19-addresses-phones-emails-migration.md b/.plans/done/issue-19-addresses-phones-emails-migration.md similarity index 100% rename from .plans/issue-19-addresses-phones-emails-migration.md rename to .plans/done/issue-19-addresses-phones-emails-migration.md diff --git a/.plans/issue-192-database-portability.md b/.plans/done/issue-192-database-portability.md similarity index 100% rename from .plans/issue-192-database-portability.md rename to .plans/done/issue-192-database-portability.md diff --git a/.plans/issue-20-member-entity-mapper.md b/.plans/done/issue-20-member-entity-mapper.md similarity index 100% rename from .plans/issue-20-member-entity-mapper.md rename to .plans/done/issue-20-member-entity-mapper.md diff --git a/.plans/issue-21-member-service.md b/.plans/done/issue-21-member-service.md similarity index 100% rename from .plans/issue-21-member-service.md rename to .plans/done/issue-21-member-service.md diff --git a/.plans/issue-22-member-controller.md b/.plans/done/issue-22-member-controller.md similarity index 100% rename from .plans/issue-22-member-controller.md rename to .plans/done/issue-22-member-controller.md diff --git a/.plans/issue-23-member-list-vue.md b/.plans/done/issue-23-member-list-vue.md similarity index 100% rename from .plans/issue-23-member-list-vue.md rename to .plans/done/issue-23-member-list-vue.md diff --git a/.plans/issue-24-member-detail-vue.md b/.plans/done/issue-24-member-detail-vue.md similarity index 100% rename from .plans/issue-24-member-detail-vue.md rename to .plans/done/issue-24-member-detail-vue.md diff --git a/.plans/issue-42-feecalculationservice.md b/.plans/done/issue-42-feecalculationservice.md similarity index 100% rename from .plans/issue-42-feecalculationservice.md rename to .plans/done/issue-42-feecalculationservice.md diff --git a/.plans/issue-45-fee-calculation-tests.md b/.plans/done/issue-45-fee-calculation-tests.md similarity index 100% rename from .plans/issue-45-fee-calculation-tests.md rename to .plans/done/issue-45-fee-calculation-tests.md diff --git a/.plans/issue-46-reportservice-pdf.md b/.plans/done/issue-46-reportservice-pdf.md similarity index 100% rename from .plans/issue-46-reportservice-pdf.md rename to .plans/done/issue-46-reportservice-pdf.md diff --git a/.plans/issue-48-encrypted-export.md b/.plans/done/issue-48-encrypted-export.md similarity index 100% rename from .plans/issue-48-encrypted-export.md rename to .plans/done/issue-48-encrypted-export.md diff --git a/.plans/issue-52-filelink-fileexplorer.md b/.plans/done/issue-52-filelink-fileexplorer.md similarity index 100% rename from .plans/issue-52-filelink-fileexplorer.md rename to .plans/done/issue-52-filelink-fileexplorer.md diff --git a/.plans/issue-53-visual-query-builder.md b/.plans/done/issue-53-visual-query-builder.md similarity index 100% rename from .plans/issue-53-visual-query-builder.md rename to .plans/done/issue-53-visual-query-builder.md diff --git a/.plans/issue-58-import-wizard.md b/.plans/done/issue-58-import-wizard.md similarity index 100% rename from .plans/issue-58-import-wizard.md rename to .plans/done/issue-58-import-wizard.md diff --git a/.plans/open/issue-200-n-plus-one-queries-member-list.md b/.plans/open/issue-200-n-plus-one-queries-member-list.md new file mode 100644 index 0000000..6a66632 --- /dev/null +++ b/.plans/open/issue-200-n-plus-one-queries-member-list.md @@ -0,0 +1,57 @@ +# Issue #200: N+1 Query Problem in Member List + +## Problem + +The `MemberService::findAll()` method (and similar methods) loads members, then makes 3 additional queries per member to fetch addresses, phones, and emails: + +```php +public function findAll(?int $limit = null, ?int $offset = null): array { + $members = $this->memberMapper->findAll($limit, $offset); + return array_map(function (Member $member) { + $id = $member->getId(); + $addresses = $this->addressMapper->findByMemberId($id); // +1 query + $phones = $this->phoneMapper->findByMemberId($id); // +1 query + $emails = $this->emailMapper->findByMemberId($id); // +1 query + return $this->buildMemberResponse($member, $addresses, $phones, $emails); + }, $members); +} +``` + +For a page of 20 members, this results in **1 + 20×3 = 61 queries**. + +## Impact + +- Slow page loads, especially with pagination +- Increased database load +- Poor performance on larger member lists + +## Solution + +Create a new mapper method that uses JOINs to fetch all data in a single query: + +```sql +SELECT m.*, a.*, p.*, e.* +FROM mv_members m +LEFT JOIN mv_addresses a ON m.id = a.member_id +LEFT JOIN mv_phones p ON m.id = p.member_id +LEFT JOIN mv_emails e ON m.id = e.member_id +WHERE m.deleted_at IS NULL +ORDER BY m.nachname, m.vorname +``` + +Then parse the result set in PHP to group related sub-entities. + +## Tasks + +- [ ] Create `MemberMapper::findAllWithRelations(?int $limit, ?int $offset): array` that returns raw joined data +- [ ] Create a DTO or use an associative array to represent joined results +- [ ] Update `MemberService::findAll()` to use the new method +- [ ] Apply same pattern to `findFiltered()`, `findByFamily()`, `search()`, `fullTextSearch()` +- [ ] Benchmark before/after query counts + +## Labels + +- enhancement +- backend +- priority:high +- performance \ No newline at end of file diff --git a/.plans/open/issue-201-count-archived-loads-all-members.md b/.plans/open/issue-201-count-archived-loads-all-members.md new file mode 100644 index 0000000..898299a --- /dev/null +++ b/.plans/open/issue-201-count-archived-loads-all-members.md @@ -0,0 +1,51 @@ +# Issue #201: countArchived() Loads All Members into Memory + +## Problem + +The `MemberService::countArchived()` method loads ALL members into memory just to count archived ones: + +```php +public function countArchived(): int { + $allMembers = $this->memberMapper->findAll(null, null, true); + return count(array_filter($allMembers, fn(Member $m) => $m->getDeletedAt() !== null)); +} +``` + +This is extremely inefficient for large member databases. + +## Impact + +- Memory exhaustion with large member counts +- Slow archive page loads +- N+1 query problem (sub-entities loaded unnecessarily) + +## Solution + +Add a COUNT query to the mapper: + +```php +public function countArchived(): int { + $qb = $this->db->getQueryBuilder(); + $qb->select($qb->createFunction('COUNT(*)')) + ->from($this->getTableName()) + ->where($qb->expr()->isNull('deleted_at')); + + // Add: where status = 'geloescht' or deleted_at IS NOT NULL + ... +} +``` + +Alternatively, add a `deleted_at IS NOT NULL` check to the existing `findAll()` method and return only the count. + +## Tasks + +- [ ] Add `MemberMapper::countArchived(): int` using `SELECT COUNT(*) WHERE deleted_at IS NOT NULL` +- [ ] Update `MemberService::countArchived()` to use the new mapper method +- [ ] Verify the count matches the archive list page size + +## Labels + +- bug +- backend +- priority:high +- performance \ No newline at end of file diff --git a/.plans/open/issue-202-find-archived-filtering-in-memory.md b/.plans/open/issue-202-find-archived-filtering-in-memory.md new file mode 100644 index 0000000..37a75a2 --- /dev/null +++ b/.plans/open/issue-202-find-archived-filtering-in-memory.md @@ -0,0 +1,52 @@ +# Issue #202: findArchived() Filters in PHP Instead of SQL + +## Problem + +The `MemberService::findArchived()` method fetches ALL members then filters in PHP: + +```php +public function findArchived(?int $limit = null, ?int $offset = null): array { + $members = $this->memberMapper->findAll($limit, $offset, true); + $archived = array_filter($members, fn(Member $m) => $m->getDeletedAt() !== null); + return array_values(array_map(function (Member $member) { + // ... + }, $archived)); +} +``` + +This means pagination is broken — `limit` and `offset` apply to ALL members, not just archived ones. + +## Impact + +- Incorrect pagination on archive page +- Unnecessary data transfer (all members, not just archived) +- N+1 query problem (sub-entities loaded unnecessarily) + +## Solution + +Add a dedicated mapper method: + +```php +public function findArchived(?int $limit = null, ?int $offset = null): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('*') + ->from($this->getTableName()) + ->where($qb->expr()->isNull('deleted_at')) + ->andWhere($qb->expr()->isNotNull('deleted_at')) // or status check + ->orderBy('deleted_at', 'DESC') + // ... pagination +} +``` + +## Tasks + +- [ ] Add `MemberMapper::findArchived(?int $limit, ?int $offset): array` +- [ ] Update `MemberService::findArchived()` to use the new mapper method +- [ ] Remove the PHP array_filter call +- [ ] Test pagination with archived members + +## Labels + +- bug +- backend +- priority:high \ No newline at end of file diff --git a/.plans/open/issue-203-missing-database-transactions.md b/.plans/open/issue-203-missing-database-transactions.md new file mode 100644 index 0000000..7bf4013 --- /dev/null +++ b/.plans/open/issue-203-missing-database-transactions.md @@ -0,0 +1,63 @@ +# Issue #203: Missing Database Transactions in MemberService + +## Problem + +The `MemberService::create()` method inserts a member and its sub-entities (addresses, phones, emails) in separate queries without a transaction: + +```php +public function create(array $data, array $addresses = [], ...): array { + // ... + $member = $this->memberMapper->insert($member); + + $savedAddresses = $this->saveAddresses($member->getId(), $addresses); + $savedPhones = $this->savePhones($member->getId(), $phones); + $savedEmails = $this->saveEmails($member->getId(), $emails); + // ... +} +``` + +If `savePhones()` fails, the member is created but phones are not attached. + +Similarly, `softDelete()` deletes sub-entities separately without a transaction. + +## Impact + +- Data inconsistency if partial failures occur +- Orphaned sub-entities on failed member creation +- Orphaned member on failed sub-entity creation + +## Solution + +Wrap the operation in a transaction: + +```php +use OCP\DB\Events\SQLFileEmitter; + +public function create(array $data, ...): array { + $this->db->beginTransaction(); + try { + $member = $this->memberMapper->insert($member); + $savedAddresses = $this->saveAddresses($member->getId(), $addresses); + // ... + $this->db->commit(); + } catch (\Exception $e) { + $this->db->rollback(); + throw $e; + } +} +``` + +## Tasks + +- [ ] Add transaction wrapping to `MemberService::create()` +- [ ] Add transaction wrapping to `MemberService::softDelete()` +- [ ] Add transaction wrapping to `MemberService::update()` (sync operations) +- [ ] Verify rollback works correctly on failure +- [ ] Consider adding a `DbTransactionTrait` for reusability + +## Labels + +- bug +- backend +- priority:high +- data-integrity \ No newline at end of file diff --git a/.plans/open/issue-204-duplicate-sync-methods.md b/.plans/open/issue-204-duplicate-sync-methods.md new file mode 100644 index 0000000..5396b4d --- /dev/null +++ b/.plans/open/issue-204-duplicate-sync-methods.md @@ -0,0 +1,77 @@ +# Issue #204: Duplicate Sub-Entity Sync Methods + +## Problem + +`MemberService` contains three nearly identical methods for syncing sub-entities: + +- `syncAddresses()` +- `syncPhones()` +- `syncEmails()` + +They all follow the same pattern: +1. Fetch existing entities +2. Index by ID +3. Update kept IDs, create new ones +4. Delete removed IDs + +This violates DRY (Don't Repeat Yourself) and makes maintenance harder. + +## Current Code + +```php +private function syncAddresses(int $memberId, array $incoming): void { + $existing = $this->addressMapper->findByMemberId($memberId); + $existingById = []; + foreach ($existing as $addr) { + $existingById[$addr->getId()] = $addr; + } + $keptIds = []; + foreach ($incoming as $item) { + $id = isset($item['id']) ? (int)$item['id'] : 0; + if ($id > 0 && isset($existingById[$id])) { + $this->updateAddress($id, $item); + $keptIds[$id] = true; + } else { + $this->addAddress($memberId, $item); + } + } + foreach ($existingById as $id => $addr) { + if (!isset($keptIds[$id])) { + $this->deleteAddress($id); + } + } +} +// syncPhones() and syncEmails() are identical except for Address → Phone/Email +``` + +## Solution + +Create a generic sync utility: + +```php +private function syncSubEntities( + int $memberId, + array $incoming, + callable $findByMemberId, + callable $add, + callable $update, + callable $delete +): void { + // Generic sync logic +} +``` + +Or create a `SubEntitySyncService` for reusability across entities. + +## Tasks + +- [ ] Extract common sync logic into a generic method or service +- [ ] Refactor `syncAddresses()`, `syncPhones()`, `syncEmails()` to use the new pattern +- [ ] Ensure identical behavior is preserved (test thoroughly) +- [ ] Document the generic sync algorithm + +## Labels + +- refactoring +- backend +- priority:medium \ No newline at end of file diff --git a/.plans/open/issue-205-date-calculations-duplicated-frontend.md b/.plans/open/issue-205-date-calculations-duplicated-frontend.md new file mode 100644 index 0000000..99a45fd --- /dev/null +++ b/.plans/open/issue-205-date-calculations-duplicated-frontend.md @@ -0,0 +1,59 @@ +# Issue #205: Date Calculations Duplicated in Frontend + +## Problem + +The `MemberList.vue` component contains duplicate date calculation logic for `alter` (age) and `mitgliedsdauer` (membership duration) that also exists in the backend `MemberService`: + +**Backend** (`lib/Service/MemberService.php`): +```php +private function calculateMitgliedsdauer(Member $member): string { + $eintritt = new DateTime($member->getEintritt()); + $end = $member->getAustritt() + ? new DateTime($member->getAustritt()) + : new DateTime(); + $diff = $eintritt->diff($end); + // ... +} +``` + +**Frontend** (`src/views/MemberList.vue`): +```javascript +function calculateAge(geburtsdatum) { + const birth = new Date(geburtsdatum) + const today = new Date() + let age = today.getFullYear() - birth.getFullYear() + // ... +} + +function calculateMitgliedsdauer(eintritt) { + const start = new Date(eintritt) + const today = new Date() + // ... +} +``` + +## Impact + +- Inconsistent calculations if date logic changes +- Maintenance burden (two places to update) +- Potential for bugs if frontend and backend diverge + +## Solution + +1. **Option A (Recommended)**: Calculate `alter` and `mitgliedsdauer` in the backend and include them in the API response +2. **Option B**: Extract to a shared utility in a format both can use (e.g., precompute in migration, or use a build-time code generator) + +## Tasks + +- [ ] Add `alter` and `mitgliedsdauer` to `Member::jsonSerialize()` +- [ ] Update `MemberService::buildMemberResponse()` to compute these values +- [ ] Remove duplicate calculations from `MemberList.vue` +- [ ] Verify frontend displays match backend calculations +- [ ] Update any other views using these calculations + +## Labels + +- refactoring +- frontend +- backend +- priority:medium \ No newline at end of file diff --git a/.plans/open/issue-206-missing-unit-tests.md b/.plans/open/issue-206-missing-unit-tests.md new file mode 100644 index 0000000..b867c41 --- /dev/null +++ b/.plans/open/issue-206-missing-unit-tests.md @@ -0,0 +1,64 @@ +# Issue #206: Missing Unit Tests for Core Services + +## Problem + +The codebase has PHP unit tests (`tests/` directory) but the core services lack comprehensive test coverage. Key untested areas: + +- `MemberService::create()` - no tests for validation, duplicate detection +- `MemberService::update()` - no tests for field filtering, partial updates +- `MemberService::softDelete()` - no tests for data purge behavior +- `MemberService::findFiltered()` - no tests for filter combinations +- `FeeCalculationService` - mentioned in docs but tests may be incomplete +- `AuditService` - no tests for field masking, diff computation + +## Impact + +- Risk of regressions when modifying services +- Harder to refactor with confidence +- Potential security issues (e.g., audit log masking) undetected + +## Solution + +Add comprehensive unit tests using PHPUnit: + +```php +class MemberServiceTest extends\TestCase { + + /** @var MemberService */ + private $service; + + protected function setUp(): void { + // Mock mappers and services + } + + public function testCreateValidatesRequiredFields(): void { + $this->expectException(ValidationException::class); + $this->service->create([]); + } + + public function testCreateDetectsDuplicates(): void { + $this->expectException(DuplicateMemberException::class); + $this->service->create([...]); + } + + // ... more tests +} +``` + +## Tasks + +- [ ] Set up PHPUnit with mocks for mappers and services +- [ ] Test `MemberService::create()` - required fields, dates, duplicates +- [ ] Test `MemberService::update()` - editable fields filter, partial updates +- [ ] Test `MemberService::softDelete()` - data purge verification +- [ ] Test `MemberService::findFiltered()` - all filter combinations +- [ ] Test `AuditService::maskIfEncrypted()` - field masking +- [ ] Test `FeeCalculationService::calculateAnnualFee()` +- [ ] Achieve >70% code coverage + +## Labels + +- enhancement +- testing +- backend +- priority:high \ No newline at end of file diff --git a/.plans/open/issue-207-authorization-middleware-instanceof-chains.md b/.plans/open/issue-207-authorization-middleware-instanceof-chains.md new file mode 100644 index 0000000..da6736e --- /dev/null +++ b/.plans/open/issue-207-authorization-middleware-instanceof-chains.md @@ -0,0 +1,74 @@ +# Issue #207: Authorization Middleware Uses instanceof Chains + +## Problem + +The `AuthorizationMiddleware::getAdminMethods()` method uses a chain of `instanceof` checks to determine admin-only methods: + +```php +private function getAdminMethods($controller): ?array { + if ($controller instanceof FeeController) { + return self::ADMIN_METHODS_FEE; + } + if ($controller instanceof StufeController) { + return self::ADMIN_METHODS_STUFE; + } + if ($controller instanceof ExportController) { + return self::ADMIN_METHODS_EXPORT; + } + if ($controller instanceof MemberController) { + return self::ADMIN_METHODS_MEMBER; + } + // ... more controllers + return null; +} +``` + +This pattern violates Open/Closed Principle — adding a new controller requires modifying the middleware. + +## Impact + +- Hard to maintain as controller count grows +- Easy to forget to add a new controller to the chain +- Mixing of concerns (controller identity vs. permissions) + +## Solution + +Use PHP attributes to declare permissions on controller methods: + +```php +#[Attribute] +class RequiresAdmin implements \Attribute {} + +#[RequiresAdmin] +public function createRule(): JSONResponse { ... } +``` + +Then scan for attributes in middleware: + +```php +public function beforeController($controller, $methodName): void { + $method = new \ReflectionMethod($controller, $methodName); + if ($method->getAttributes(RequiresAdmin::class)) { + if (!$this->permissionService->isAdmin($userId)) { + throw new \RuntimeException('Authorization: admin required'); + } + } + // ... rest of checks +} +``` + +## Tasks + +- [ ] Create `RequiresRead` and `RequiresWrite` attributes +- [ ] Create `RequiresAdmin` attribute +- [ ] Add attributes to all controller methods +- [ ] Refactor `AuthorizationMiddleware` to use reflection + attributes +- [ ] Remove `ADMIN_METHODS_*` constants and `getAdminMethods()` method +- [ ] Test all permission levels still work correctly + +## Labels + +- refactoring +- backend +- priority:medium +- architecture \ No newline at end of file diff --git a/.plans/open/issue-208-member-controller-method-bloat.md b/.plans/open/issue-208-member-controller-method-bloat.md new file mode 100644 index 0000000..2023a6f --- /dev/null +++ b/.plans/open/issue-208-member-controller-method-bloat.md @@ -0,0 +1,69 @@ +# Issue #208: MemberController Has Too Many Responsibilities + +## Problem + +`MemberController` handles: +- Member CRUD (index, show, create, update, destroy) +- Sub-entity CRUD (addresses, phones, emails) +- Admin-only operations (revealAllergies, archive) +- Search (search, fullTextSearch) + +This violates Single Responsibility Principle. The controller is 500+ lines with 20+ methods. + +## Current Structure + +``` +MemberController +├── search() # Full-text search +├── index() # List with filters +├── show() # Single member +├── create() # Create member +├── update() # Update member +├── destroy() # Delete member +├── revealAllergies() # Admin-only +├── archive() # Admin-only +├── createAddress() # Sub-entity +├── updateAddress() # Sub-entity +├── destroyAddress() # Sub-entity +├── createPhone() # Sub-entity +├── updatePhone() # Sub-entity +├── destroyPhone() # Sub-entity +├── createEmail() # Sub-entity +├── updateEmail() # Sub-entity +├── destroyEmail() # Sub-entity +``` + +## Solution + +Split into focused controllers: + +``` +Controllers/ +├── MemberController # Member CRUD only +├── MemberAddressController # Address CRUD +├── MemberPhoneController # Phone CRUD +├── MemberEmailController # Email CRUD +├── MemberSearchController # Search operations +├── MemberAdminController # Admin-only operations +``` + +Each controller has 3-5 methods, making them easier to understand and test. + +## Tasks + +- [ ] Create `MemberAddressController` for address CRUD +- [ ] Create `MemberPhoneController` for phone CRUD +- [ ] Create `MemberEmailController` for email CRUD +- [ ] Create `MemberSearchController` for search operations +- [ ] Create `MemberAdminController` for admin operations +- [ ] Move sub-entity and admin methods from MemberController +- [ ] Update routes in `AppInfo/Application.php` +- [ ] Update frontend API calls to use new endpoints +- [ ] Remove old methods from MemberController + +## Labels + +- refactoring +- backend +- priority:medium +- architecture \ No newline at end of file diff --git a/.plans/open/issue-209-api-versioning-strategy.md b/.plans/open/issue-209-api-versioning-strategy.md new file mode 100644 index 0000000..6315b3c --- /dev/null +++ b/.plans/open/issue-209-api-versioning-strategy.md @@ -0,0 +1,60 @@ +# Issue #209: No API Versioning Strategy + +## Problem + +All API endpoints use `/api/v1/` prefix but there's no formal versioning strategy. When breaking changes are needed (e.g., restructuring the member response), there's no clean migration path. + +Current endpoints: +``` +/apps/mitgliederverwaltung/api/v1/members +/apps/mitgliederverwaltung/api/v1/families +/apps/mitgliederverwaltung/api/v1/fees +... +``` + +## Impact + +- Breaking frontend changes require immediate backend deployment +- No backward compatibility for mobile apps or third-party integrations +- Hard to deprecate old formats gracefully + +## Solution + +Implement a proper versioning strategy: + +1. **URL Versioning** (current approach): + ``` + /api/v1/members → Current version + /api/v2/members → New version with breaking changes + ``` + +2. **Header Versioning** (alternative): + ``` + GET /api/members + Accept: application/vnd.mitgliederverwaltung.v1+json + + GET /api/members + Accept: application/vnd.mitgliederverwaltung.v2+json + ``` + +3. **Deprecation Path**: + - v1 marked as deprecated in response headers + - v2 available in parallel + - v1 removed after 6 months + +## Tasks + +- [ ] Document API versioning strategy in docs/ +- [ ] Implement v2 endpoints for critical entities (Member first) +- [ ] Add deprecation headers to v1 responses +- [ ] Update frontend to use v2 endpoints +- [ ] Create migration guide for v1 → v2 +- [ ] Set deprecation timeline (e.g., remove v1 in next major release) + +## Labels + +- enhancement +- backend +- api +- priority:low +- architecture \ No newline at end of file diff --git a/.plans/open/issue-210-inconsistent-error-response-format.md b/.plans/open/issue-210-inconsistent-error-response-format.md new file mode 100644 index 0000000..5f41bd3 --- /dev/null +++ b/.plans/open/issue-210-inconsistent-error-response-format.md @@ -0,0 +1,91 @@ +# Issue #210: Inconsistent Error Response Format + +## Problem + +Different controllers return errors in different formats: + +```php +// MemberController +return new JSONResponse(['error' => 'Mitglied nicht gefunden'], Http::STATUS_NOT_FOUND); + +// Some controllers might return +return new JSONResponse(['message' => 'Not found'], Http::STATUS_NOT_FOUND); + +// Validation errors sometimes include field-level details: +return new JSONResponse(['error' => 'Validation failed', 'fields' => [...]], Http::STATUS_BAD_REQUEST); +``` + +This makes frontend error handling difficult. + +## Impact + +- Inconsistent frontend error parsing +- Difficulty implementing global error handling +- Poor developer experience when debugging + +## Solution + +Create a standardized error response format: + +```php +class ApiResponse { + public static function success(mixed $data, int $status = 200): JSONResponse { ... } + + public static function error( + string $message, + int $status, + ?array $details = null, + ?string $errorCode = null + ): JSONResponse { + return new JSONResponse([ + 'success' => false, + 'error' => [ + 'code' => $errorCode ?? self::getDefaultCode($status), + 'message' => $message, + 'details' => $details, + ] + ], $status); + } +} +``` + +Standard response format: +```json +{ + "success": true, + "data": { ... }, + "meta": { + "total": 42, + "limit": 20, + "offset": 0 + } +} +``` + +Error response format: +```json +{ + "success": false, + "error": { + "code": "MEMBER_NOT_FOUND", + "message": "Mitglied nicht gefunden", + "details": { "memberId": 123 } + } +} +``` + +## Tasks + +- [ ] Create `ApiResponse` utility class +- [ ] Define standard error codes (MEMBER_NOT_FOUND, VALIDATION_ERROR, etc.) +- [ ] Update all controllers to use `ApiResponse::error()` +- [ ] Update frontend stores to handle new error format +- [ ] Document the response format in docs/ + +## Labels + +- enhancement +- backend +- frontend +- api +- priority:medium \ No newline at end of file diff --git a/.plans/open/issue-211-authorization-middleware-refactor.md b/.plans/open/issue-211-authorization-middleware-refactor.md new file mode 100644 index 0000000..cbf5882 --- /dev/null +++ b/.plans/open/issue-211-authorization-middleware-refactor.md @@ -0,0 +1,78 @@ +# Issue #211: Authorization Middleware Maintenance Burden + +## Problem + +The `AuthorizationMiddleware` requires manual maintenance for each new controller and method: + +1. When adding a new admin-only controller, it must be added to the `beforeController` chain: + ```php + if ($controller instanceof ImportController + || $controller instanceof PermissionController + || $controller instanceof AuditController + || $controller instanceof DsgvoController + || $controller instanceof BackupController + // NEW: Must add here + ) { ... } + ``` + +2. When adding an admin-only method, it must be added to a constant: + ```php + private const ADMIN_METHODS_MEMBER = ['revealAllergies', 'archive']; + // NEW: Must add here + ``` + +3. When adding a read-only method, it must be added to: + ```php + private const READ_METHODS = [ + 'index', 'show', 'search', 'preview', ... + // NEW: Must add here + ]; + ``` + +## Impact + +- Easy to forget to add a new controller/method to the right list +- Growing maintenance burden as the app expands +- Potential security issues if a write method is accidentally omitted +- Violates Open/Closed Principle + +## Solution + +Use PHP 8 attributes to declare permissions directly on controller methods: + +```php +#[Attribute] +class RequirePermission { + public function __construct(string $level) { ... } +} + +class MemberController { + #[RequirePermission('read')] + public function index(): JSONResponse { ... } + + #[RequirePermission('write')] + public function create(): JSONResponse { ... } + + #[RequirePermission('admin')] + public function revealAllergies(): JSONResponse { ... } +} +``` + +Middleware then reads attributes via reflection. + +## Tasks + +- [ ] Create `RequirePermission` attribute class +- [ ] Add `#[RequirePermission]` attributes to all controller methods +- [ ] Refactor `AuthorizationMiddleware::beforeController()` to use reflection +- [ ] Remove hardcoded controller and method lists +- [ ] Add tests to verify permission enforcement +- [ ] Document the new attribute-based approach + +## Labels + +- refactoring +- backend +- security +- priority:medium +- architecture \ No newline at end of file diff --git a/.plans/open/issue-212-error-handling-inconsistency.md b/.plans/open/issue-212-error-handling-inconsistency.md new file mode 100644 index 0000000..f1a25f2 --- /dev/null +++ b/.plans/open/issue-212-error-handling-inconsistency.md @@ -0,0 +1,75 @@ +# Issue #212: Inconsistent Error Handling Across Controllers + +## Problem + +Different controllers handle errors differently: + +**Logging inconsistency:** +```php +// Some controllers log errors: +$this->logger->error('Failed to create member', ['exception' => $e, ...]); + +// Some controllers don't log at all +``` + +**Exception handling inconsistency:** +```php +// Some controllers catch multiple exception types: +} catch (DoesNotExistException $e) { + return new JSONResponse(['error' => 'Not found'], 404); +} catch (\Exception $e) { + return new JSONResponse(['error' => 'Server error'], 500); +} + +// Some controllers use generic catches: +} catch (Exception $e) { ... } +``` + +**Error message language:** +```php +return new JSONResponse(['error' => 'Mitglied nicht gefunden'], 404); // German +return new JSONResponse(['error' => 'Member not found'], 404); // English +``` + +## Impact + +- Inconsistent user experience +- Difficult to debug production issues +- Potential information leakage (stack traces in logs) +- Mixed language in API responses + +## Solution + +1. Create a base controller with standardized error handling: + ```php + abstract class BaseApiController extends ApiController { + protected function handleNotFound(DoesNotExistException $e): JSONResponse { + $this->logger->warning('Resource not found', ['exception' => $e]); + return $this->error('Resource not found', 404); + } + + protected function handleError(\Exception $e, string $action): JSONResponse { + $this->logger->error("$action failed", ['exception' => $e]); + return $this->error('An error occurred', 500); + } + } + ``` + +2. Define consistent error messages in one place (enum or constants) + +3. Ensure all controllers extend the base class + +## Tasks + +- [ ] Create `BaseApiController` with standardized error methods +- [ ] Define `ErrorMessages` enum with all error strings +- [ ] Update all controllers to extend `BaseApiController` +- [ ] Ensure consistent logging (all errors logged, exceptions include context) +- [ ] Standardize on German or bilingual error messages +- [ ] Add tests for error handling behavior + +## Labels + +- bug +- backend +- priority:medium \ No newline at end of file diff --git a/.plans/open/issue-213-frontend-state-management-issues.md b/.plans/open/issue-213-frontend-state-management-issues.md new file mode 100644 index 0000000..a7bb081 --- /dev/null +++ b/.plans/open/issue-213-frontend-state-management-issues.md @@ -0,0 +1,66 @@ +# Issue #213: Frontend State Management Issues + +## Problem + +Several issues in the Pinia stores and Vue components: + +### 1. Store State Not Persisted + +The `members.js` store state (filters, pagination) is lost on page refresh. Users lose their filter settings. + +### 2. No Optimistic UI Updates + +When creating/updating/deleting members, the UI waits for the server response before updating. For better UX, implement optimistic updates: + +```javascript +async function deleteMember(id) { + // Optimistic: remove from local state immediately + this.members = this.members.filter(m => m.id !== id) + + try { + await axios.delete(url) + } catch (err) { + // Revert on failure + await this.fetchMembers() + throw err + } +} +``` + +### 3. Stale Data After Navigation + +After updating a member in `MemberDetail.vue`, navigating back to `MemberList.vue` may show stale data if the list wasn't refetched. + +### 4. Error State Not Cleared + +`store.clearError()` exists but isn't always called before retry operations, potentially showing old errors alongside new data. + +## Impact + +- Poor UX (page refresh loses context) +- Slower perceived performance (no optimistic updates) +- Confusing states when data is stale +- Confusing error messages + +## Solution + +1. Add `pinia-plugin-persistedstate` to persist store state +2. Implement optimistic updates in CRUD operations +3. Use `watch` to refetch data when navigating back +4. Ensure `clearError()` is called at the start of all fetch operations + +## Tasks + +- [ ] Add `pinia-plugin-persistedstate` dependency +- [ ] Configure persistence for filter and pagination state +- [ ] Implement optimistic updates in `deleteMember()`, `updateMember()`, `createMember()` +- [ ] Add navigation watchers to refetch stale data +- [ ] Verify `clearError()` is called before all fetch operations +- [ ] Test page refresh preserves filter state + +## Labels + +- enhancement +- frontend +- ux +- priority:medium \ No newline at end of file diff --git a/.plans/open/issue-214-no-rate-limiting-implementation.md b/.plans/open/issue-214-no-rate-limiting-implementation.md new file mode 100644 index 0000000..66ff7ac --- /dev/null +++ b/.plans/open/issue-214-no-rate-limiting-implementation.md @@ -0,0 +1,55 @@ +# Issue #214: Rate Limiting Not Fully Implemented + +## Problem + +The `RateLimitMiddleware` exists but may not be properly configured or may have gaps: + +1. **Bulk operations not rate-limited**: The `revealAllergies()` endpoint iterates over ALL members and decrypts each — this should be rate-limited per admin session. + +2. **No per-endpoint limits**: Search endpoints could be abused for enumeration attacks. + +3. **No limit feedback**: When rate limited, users don't know when they can retry. + +## Impact + +- Potential DoS via expensive operations (bulk decryption) +- Enumeration attacks on search endpoints +- Poor UX with no retry-after information + +## Solution + +1. Add specific rate limits to expensive endpoints: + ```php + // In middleware configuration + 'member.allergien.reveal' => ['limit' => 1, 'period' => 3600], // 1 per hour + 'member.search' => ['limit' => 30, 'period' => 60], // 30 per minute + ``` + +2. Return proper rate limit headers: + ```php + $response->headers->set('X-RateLimit-Remaining', (string)$remaining); + $response->headers->set('X-RateLimit-Reset', (string)$resetTime); + ``` + +3. Return 429 with Retry-After header when limited: + ```php + return new JSONResponse(['error' => 'Rate limit exceeded'], 429, [ + 'Retry-After' => $secondsRemaining + ]); + ``` + +## Tasks + +- [ ] Audit all endpoints for rate limit requirements +- [ ] Configure per-endpoint rate limits +- [ ] Add X-RateLimit-* response headers +- [ ] Return 429 with Retry-After when limited +- [ ] Document rate limit configuration +- [ ] Add tests for rate limiting behavior + +## Labels + +- enhancement +- security +- backend +- priority:medium \ No newline at end of file diff --git a/.plans/open/issue-215-column-rendering-duplicated.md b/.plans/open/issue-215-column-rendering-duplicated.md new file mode 100644 index 0000000..765b766 --- /dev/null +++ b/.plans/open/issue-215-column-rendering-duplicated.md @@ -0,0 +1,59 @@ +# Issue #215: Column Rendering Logic Duplicated Across Views + +## Problem + +The `allColumns` definition in `MemberList.vue` contains rendering logic that's duplicated when the same fields appear in other views (e.g., `MemberDetail.vue`, export templates): + +```javascript +const allColumns = [ + { key: 'name', label: 'Name', render: m => m.nachname + ', ' + m.vorname }, + { key: 'stufe', label: 'Stufe', render: m => getStufeNameById(m.stufeId) }, + { key: 'status', label: 'Status', render: m => statusMap[m.status] || m.status }, + // ... +] +``` + +The `statusMap`, `rolleMap`, `geschlechtMap`, `kvTypMap` translations are duplicated in multiple components. + +## Impact + +- Inconsistent rendering if one copy is updated but not others +- Maintenance burden (3+ places to update for a field change) +- Difficulty adding new columns consistently across views + +## Solution + +Create a shared column definitions module: + +```javascript +// src/utils/columnDefinitions.js +export const STATUS_MAP = { aktiv: 'Aktiv', inaktiv: 'Inaktiv', geloescht: 'Gelöscht' } +export const ROLLE_MAP = { mitglied: 'Mitglied', erziehungsberechtigter: 'Erziehungsberechtigter' } +// ... + +export const MEMBER_LIST_COLUMNS = [ + { key: 'name', label: 'Name', render: m => `${m.nachname}, ${m.vorname}` }, + { key: 'status', label: 'Status', render: m => STATUS_MAP[m.status] || m.status }, + // ... +] +``` + +Then import in components: +```javascript +import { MEMBER_LIST_COLUMNS, STATUS_MAP } from '../utils/columnDefinitions.js' +``` + +## Tasks + +- [ ] Create `src/utils/columnDefinitions.js` with all maps and column definitions +- [ ] Update `MemberList.vue` to import from the shared module +- [ ] Update `MemberDetail.vue` to use shared maps +- [ ] Update export templates to use shared definitions +- [ ] Update any other views using these maps +- [ ] Add TypeScript types for column definitions (if using TS) + +## Labels + +- refactoring +- frontend +- priority:low \ No newline at end of file diff --git a/.plans/open/issue-216-missing-integration-tests.md b/.plans/open/issue-216-missing-integration-tests.md new file mode 100644 index 0000000..5fdbc75 --- /dev/null +++ b/.plans/open/issue-216-missing-integration-tests.md @@ -0,0 +1,74 @@ +# Issue #216: Missing Integration Tests + +## Problem + +While unit tests exist for some services, there are no integration tests that verify the full request-response cycle: + +- No tests for API endpoint behavior +- No tests for database operations end-to-end +- No tests for authentication/authorization flow +- No tests for concurrent access patterns +- No tests for data migration scripts + +## Impact + +- Unknown behavior of API contracts +- Risk of breaking changes going undetected +- Difficult to verify authorization works correctly +- Risk of migration failures in production + +## Solution + +Add integration tests using Nextcloud's testing infrastructure: + +```php +class MemberApiIntegrationTest extends \PHPUnit\Framework\TestCase { + + use Helper\EmptyContainerTrait; + + private ?AppFrameworkApp $app = null; + private ?MemberController $controller = null; + + protected function setUp(): void { + $this->app = $this->container->get('app'); + $this->controller = $this->app->getContainer() + ->get(MemberController::class); + } + + public function testCreateMemberRequiresAuthentication(): void { + $this->expectException(UnauthorizedException::class); + $this->controller->create(); + } + + public function testCreateMemberValidatesRequiredFields(): void { + $this->givenAuthenticatedUser('admin'); + $response = $this->controller->create([]); + $this->assertEquals(400, $response->getStatusCode()); + } + + public function testSoftDeletePurgesSensitiveData(): void { + $member = $this->givenMemberWithAllergies(); + $this->controller->destroy($member->id); + + $deleted = $this->memberMapper->findById($member->id); + $this->assertNull($deleted->getAllergienEncrypted()); + } +} +``` + +## Tasks + +- [ ] Set up Nextcloud integration test environment +- [ ] Add integration tests for MemberController CRUD +- [ ] Add integration tests for authorization (admin vs. user) +- [ ] Add integration tests for FamilyController +- [ ] Add integration tests for FeeController +- [ ] Add integration tests for migration scripts +- [ ] Set up CI pipeline for integration tests + +## Labels + +- enhancement +- testing +- backend +- priority:high \ No newline at end of file diff --git a/.plans/open/issue-217-no-request-validation-layer.md b/.plans/open/issue-217-no-request-validation-layer.md new file mode 100644 index 0000000..6065ac0 --- /dev/null +++ b/.plans/open/issue-217-no-request-validation-layer.md @@ -0,0 +1,82 @@ +# Issue #217: No Request Validation Layer + +## Problem + +Input validation is scattered across controllers and services: + +1. **Controller level**: Some controllers validate input exists: + ```php + $data = $this->getRequestData(); + // Assumes $data is always valid + ``` + +2. **Service level**: `MemberService::validateRequiredFields()` checks for required fields: + ```php + private function validateRequiredFields(array $data): void { + $required = ['vorname', 'nachname', 'eintritt']; + // ... + } + ``` + +3. **No type validation**: Phone numbers are validated via `PhoneValidator::validateAndNormalize()`, but other fields have no type checking. + +4. **No schema validation**: There's no declarative validation schema. + +## Impact + +- Inconsistent validation (some fields validated, others not) +- Potential security issues (SQL injection mitigated by ORM, but XSS, etc. not checked) +- Difficult to know what validation exists for each endpoint +- No auto-generated API documentation from validation schemas + +## Solution + +Implement a validation layer using Symfony Validator or similar: + +```php +class CreateMemberRequest { + #[Assert\NotBlank] + #[Assert\Length(min: 1, max: 100)] + public string $vorname; + + #[Assert\NotBlank] + #[Assert\Length(min: 1, max: 100)] + public string $nachname; + + #[Assert\NotBlank] + #[Assert\Date] + public string $eintritt; + + #[Assert\Date] + public ?string $geburtsdatum = null; + + #[Assert\Choice(['maennlich', 'weiblich', 'divers'])] + public ?string $geschlecht = null; +} + +public function create(): JSONResponse { + $data = $this->getRequestData(); + $violations = $this->validator->validate($data, CreateMemberRequest::class); + if (count($violations) > 0) { + return $this->validationError($violations); + } + // ... +} +``` + +## Tasks + +- [ ] Add Symfony Validator dependency +- [ ] Create validation DTOs for each endpoint +- [ ] Add Assert annotations for all fields +- [ ] Implement validation middleware or base controller method +- [ ] Return structured validation errors (field → message) +- [ ] Update services to remove inline validation (keep business logic only) +- [ ] Add tests for validation rules + +## Labels + +- enhancement +- security +- backend +- priority:medium \ No newline at end of file