Restructure .plans/ into done/ and open/ subdirectories
- Move completed plan files to .plans/done/ - Move 18 open plan files to .plans/open/ - Update .gitignore to exclude .verified_plans temp file - Verified all 18 open plans still describe unimplemented issues
This commit is contained in:
shahondin1624
+2
-3
@@ -37,6 +37,5 @@ test-results/
|
||||
# Release artifacts
|
||||
artifacts/
|
||||
|
||||
# agent
|
||||
plan.md
|
||||
review.md
|
||||
# Plan verification tracking
|
||||
.plans/.verified_plans
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
@@ -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
|
||||
Reference in New Issue
Block a user