Skip to content
This repository was archived by the owner on May 29, 2026. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions appinfo/routes.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@
['name' => 'settings#create', 'url' => '/api/settings', 'verb' => 'POST'],
['name' => 'settings#load', 'url' => '/api/settings/load', 'verb' => 'POST'],

// Column CRUD endpoints.
['name' => 'column#index', 'url' => '/api/columns', 'verb' => 'GET'],
['name' => 'column#create', 'url' => '/api/columns', 'verb' => 'POST'],
['name' => 'column#update', 'url' => '/api/columns/{id}', 'verb' => 'PATCH'],

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] HTTP verb mismatch: route uses PATCH but spec says PUT

The task acceptance criteria in openspec/changes/spec/tasks.md (Task 1) specifies:

PUT /api/columns/{id} updates a column (title, position)

But the route registers PATCH. While PATCH is semantically more correct for partial updates per the NL API Strategie guidelines, this creates a discrepancy between the spec/tasks and the implementation.

Required fix: Either update the spec to say PATCH (preferred — PATCH is correct for partial updates) or change the verb to PUT. The spec and implementation must match.

['name' => 'column#destroy', 'url' => '/api/columns/{id}', 'verb' => 'DELETE'],

// Prometheus metrics endpoint.
['name' => 'metrics#index', 'url' => '/api/metrics', 'verb' => 'GET'],
// Health check endpoint.
Expand Down
289 changes: 289 additions & 0 deletions lib/Controller/ColumnController.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,289 @@
<?php

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SUGGESTION] SPDX license comment header missing

The Conduction Common Ground convention (CLAUDE.md) requires a machine-readable SPDX comment at the top of every new file:

// SPDX-License-Identifier: EUPL-1.2
// Copyright (C) 2026 Conduction B.V.

The current @license PHPDoc tag is human-readable but not SPDX-compliant. The same applies to ColumnService.php. Existing files in the repo also lack this header, so this is consistent with the codebase today — but worth aligning in new files going forward.


/**
* Planix Column Controller
*
* Controller for managing kanban board columns per project.
*
* @category Controller
* @package OCA\Planix\Controller
*
* @author Conduction Development Team <dev@conductio.nl>
* @copyright 2026 Conduction B.V.
* @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12
*
* @version GIT: <git-id>
*
* @link https://conduction.nl
*/

declare(strict_types=1);

namespace OCA\Planix\Controller;

use OCA\Planix\AppInfo\Application;
use OCA\Planix\Service\ColumnService;
use OCP\AppFramework\Controller;
use OCP\AppFramework\Http;
use OCP\AppFramework\Http\JSONResponse;
use OCP\IRequest;

/**
* Controller for managing kanban board columns.
*/
class ColumnController extends Controller
{
/**
* Constructor for the ColumnController.
*
* @param IRequest $request The request object
* @param ColumnService $columnService The column service
*
* @return void
*/
public function __construct(
IRequest $request,
private ColumnService $columnService,
) {
parent::__construct(appName: Application::APP_ID, request: $request);
}//end __construct()

/**
* List columns for a project, ordered by position.
*
* @NoAdminRequired
*
* @return JSONResponse
*/
public function index(): JSONResponse
{
$projectId = $this->request->getParam('projectId');

if (empty($projectId) === true) {
return new JSONResponse(
['error' => 'The projectId query parameter is required.'],
Http::STATUS_BAD_REQUEST
);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] Double project lookup per request

In index(), create(), and similar methods, the controller calls findProject() explicitly (for the 404 check) and then immediately calls isProjectMember(). But isProjectMember() calls findProject() internally again, resulting in two round-trips to OpenRegister per request just to check project existence and membership.

// Lookup #1 (explicit 404 check)
if ($this->columnService->findProject($projectId) === null) { ... }

// Lookup #2 (inside isProjectMember → findProject)
if ($this->columnService->isProjectMember($projectId) === false) { ... }

Required fix: Refactor isProjectMember() to accept the already-fetched project array as an optional parameter, or combine the 404 + membership check into a single method (e.g., findProjectForMember()) that returns null when not found and false when not a member.

$project = $this->columnService->findProject($projectId);
if ($project === null) {
return new JSONResponse(
['error' => 'Project not found.'],
Http::STATUS_NOT_FOUND
);
}

if ($this->columnService->isProjectMember($projectId, $project) === false) {
return new JSONResponse(
['error' => 'You are not a member of this project.'],
Http::STATUS_FORBIDDEN
);
}

$columns = $this->columnService->listColumns($projectId);

return new JSONResponse($columns);

}//end index()

/**
* Create a new column.
*
* @NoAdminRequired
*
* @return JSONResponse
*/
public function create(): JSONResponse
{
$data = $this->request->getParams();
$projectId = $data['project'] ?? ($data['projectId'] ?? null);

if (empty($projectId) === true) {
return new JSONResponse(
['error' => 'The project field is required.'],
Http::STATUS_BAD_REQUEST
);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[INFO] No server-side allow-list validation on type and color fields

$data['type'] ?? 'active' and $data['color'] ?? null are passed through to storage without validation. Accepting arbitrary strings for type can cause the frontend to render unknown column states silently; accepting arbitrary strings for color bypasses the intent of a constrained palette.

Recommendation: add a minimal allow-list before building $columnData:

$allowedTypes = ['active', 'done', 'backlog'];
$type = in_array($data['type'] ?? '', $allowedTypes, true) ? $data['type'] : 'active';

For color, validate it matches a hex colour pattern: /^#[0-9a-fA-F]{6}$/.

$project = $this->columnService->findProject($projectId);
if ($project === null) {
return new JSONResponse(
['error' => 'Project not found.'],
Http::STATUS_NOT_FOUND
);
}

if ($this->columnService->isProjectMember($projectId, $project) === false) {
return new JSONResponse(
['error' => 'You are not a member of this project.'],
Http::STATUS_FORBIDDEN
);
}

$title = $data['title'] ?? '';
if (empty($title) === true) {
return new JSONResponse(
['error' => 'The title field is required.'],
Http::STATUS_BAD_REQUEST
);
}

$allowedTypes = ['active', 'done'];
$type = $data['type'] ?? 'active';
if (in_array($type, $allowedTypes, true) === false) {
return new JSONResponse(
['error' => 'Invalid type. Allowed values: active, done.'],
Http::STATUS_BAD_REQUEST
);
}

$wipLimit = null;
if (isset($data['wipLimit']) === true) {
$wipLimit = (int) $data['wipLimit'];
}

$columnData = [
'title' => $title,
'project' => $projectId,
'order' => (int) ($data['order'] ?? ($data['position'] ?? 0)),
'wipLimit' => $wipLimit,
'color' => $data['color'] ?? null,
'type' => $type,
];

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[WARNING] Empty PATCH body may cause unintended data losslib/Controller/ColumnController.php line ~202-229

update() builds $updateData from recognised fields only. If the request body contains none of these keys (title, order, wipLimit, color, type), $updateData = [] and updateColumn($id, []) is called.

In ColumnService::updateColumn() this becomes:

$objectService->saveObject(register: 'planix', schema: 'column', object: ['id' => $id]);

If OpenRegister's saveObject uses full-replace semantics (not merge/patch) this will silently erase all column fields. Even with merge semantics, accepting an empty PATCH violates RFC 5789 §2 (a PATCH request MUST contain a patch document).

Fix: add an early return before the service call:

if (empty($updateData) === true) {
    return new JSONResponse(['error' => 'No updatable fields provided.'], Http::STATUS_BAD_REQUEST);
}


$column = $this->columnService->createColumn($columnData, $project);

return new JSONResponse($column, Http::STATUS_CREATED);

}//end create()

/**
* Update an existing column.
*
* @param string $id The column UUID
*
* @NoAdminRequired
*
* @return JSONResponse
*/
public function update(string $id): JSONResponse
{
$column = $this->columnService->findColumn($id);
if ($column === null) {
return new JSONResponse(
['error' => 'Column not found.'],
Http::STATUS_NOT_FOUND
);
}

$projectId = $column['project'] ?? '';
if ($this->columnService->isProjectMember($projectId) === false) {
return new JSONResponse(
['error' => 'You are not a member of this project.'],
Http::STATUS_FORBIDDEN
);
}

$data = $this->request->getParams();
$updateData = [];

if (isset($data['title']) === true) {
$updateData['title'] = $data['title'];
}

if (isset($data['order']) === true || isset($data['position']) === true) {
$updateData['order'] = (int) ($data['order'] ?? $data['position']);
}

if (array_key_exists('wipLimit', $data) === true) {
if ($data['wipLimit'] !== null) {
$updateData['wipLimit'] = (int) $data['wipLimit'];
} else {
$updateData['wipLimit'] = null;
}
}

if (isset($data['color']) === true) {
$updateData['color'] = $data['color'];
}

if (isset($data['type']) === true) {
$allowedTypes = ['active', 'done'];
if (in_array($data['type'], $allowedTypes, true) === false) {
return new JSONResponse(
['error' => 'Invalid type. Allowed values: active, done.'],
Http::STATUS_BAD_REQUEST
);
}

$updateData['type'] = $data['type'];
}

if (empty($updateData) === true) {
return new JSONResponse(
['error' => 'No valid fields provided for update.'],
Http::STATUS_BAD_REQUEST
);
}

try {
$updated = $this->columnService->updateColumn($id, $updateData);
} catch (\RuntimeException $e) {
return new JSONResponse(
['error' => 'Failed to update column.'],
Http::STATUS_INTERNAL_SERVER_ERROR
);
}

return new JSONResponse($updated);

}//end update()

/**
* Delete a column. Tasks in this column are moved to the backlog.
*
* @param string $id The column UUID

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[SUGGESTION] Dead code — the $deleted === false branch is unreachable

deleteColumn() either returns true (success) or throws \RuntimeException (failure). The catch block above already handles the exception, so the if ($deleted === false) check can never be true — the variable will always be true at that point.

Safe to remove lines 278-283:

// These lines are unreachable:
if ($deleted === false) {
    return new JSONResponse(['error' => 'Failed to delete column.'], Http::STATUS_INTERNAL_SERVER_ERROR);
}

*
* @NoAdminRequired
*
* @return JSONResponse
*/
public function destroy(string $id): JSONResponse
{
$column = $this->columnService->findColumn($id);
if ($column === null) {
return new JSONResponse(
['error' => 'Column not found.'],
Http::STATUS_NOT_FOUND
);
}

$projectId = $column['project'] ?? '';
if ($this->columnService->isProjectMember($projectId) === false) {
return new JSONResponse(
['error' => 'You are not a member of this project.'],
Http::STATUS_FORBIDDEN
);
}

try {
$deleted = $this->columnService->deleteColumn($id);
} catch (\RuntimeException $e) {
return new JSONResponse(
['error' => 'Failed to delete column.'],
Http::STATUS_INTERNAL_SERVER_ERROR
);
}

if ($deleted === false) {
return new JSONResponse(
['error' => 'Failed to delete column.'],
Http::STATUS_INTERNAL_SERVER_ERROR
);
}

return new JSONResponse(null, Http::STATUS_NO_CONTENT);

}//end destroy()

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[CRITICAL] DELETE returns 200 instead of 204 No Content

destroy() returns new JSONResponse(['success' => true]) (HTTP 200) on success. According to the NL API strategie / Common Ground API standard, a successful DELETE MUST return HTTP 204 No Content with no response body.

Additionally, the return value of $this->columnService->deleteColumn($id) is never checked. If deletion fails (e.g. OpenRegister returns false), the controller silently returns 200 OK — a false success signal to the caller.

Fix:

$success = $this->columnService->deleteColumn($id);
if ($success === false) {
    return new JSONResponse(
        ['error' => 'Column could not be deleted.'],
        Http::STATUS_INTERNAL_SERVER_ERROR
    );
}
return new JSONResponse(null, Http::STATUS_NO_CONTENT);

}//end class
Loading
Loading