From e20bfdadabb84bb460b1139875ed28c4f18fa519 Mon Sep 17 00:00:00 2001
From: Chen Asraf <casraf@pm.me>
Date: Sat, 22 Nov 2025 23:05:02 +0200
Subject: [PATCH] refactor(Roles): update UserRoleController & SetRole command
 to use UserRoleService

---
 lib/Command/SetRole.php               | 32 ++++++++++++---------------
 lib/Controller/UserRoleController.php | 23 +++++++++++++------
 2 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/lib/Command/SetRole.php b/lib/Command/SetRole.php
index a3b1eea..533dc4f 100644
--- a/lib/Command/SetRole.php
+++ b/lib/Command/SetRole.php
@@ -8,8 +8,7 @@ declare(strict_types=1);
 namespace OCA\Forum\Command;
 
 use OCA\Forum\Db\RoleMapper;
-use OCA\Forum\Db\UserRole;
-use OCA\Forum\Db\UserRoleMapper;
+use OCA\Forum\Service\UserRoleService;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Db\MultipleObjectsReturnedException;
 use OCP\IUserManager;
@@ -21,7 +20,7 @@ use Symfony\Component\Console\Output\OutputInterface;
 class SetRole extends Command {
 	public function __construct(
 		private RoleMapper $roleMapper,
-		private UserRoleMapper $userRoleMapper,
+		private UserRoleService $userRoleService,
 		private IUserManager $userManager,
 	) {
 		parent::__construct();
@@ -70,22 +69,19 @@ class SetRole extends Command {
 		}
 
 		// Check if user already has this role
-		$userRoles = $this->userRoleMapper->findByUserId($username);
-		foreach ($userRoles as $userRole) {
-			if ($userRole->getRoleId() === $role->getId()) {
-				$output->writeln("<comment>User '$username' already has the role '{$role->getName()}'.</comment>");
-				return 0;
-			}
+		if ($this->userRoleService->hasRole($username, $role->getId())) {
+			$output->writeln("<comment>User '$username' already has the role '{$role->getName()}'.</comment>");
+			return 0;
 		}
 
-		// Add the role to the user
-		$userRole = new UserRole();
-		$userRole->setUserId($username);
-		$userRole->setRoleId($role->getId());
-		$userRole->setCreatedAt(time());
-		$this->userRoleMapper->insert($userRole);
-
-		$output->writeln("<info>Successfully assigned role '{$role->getName()}' to user '$username'.</info>");
-		return 0;
+		// Add the role to the user using the service
+		try {
+			$this->userRoleService->assignRole($username, $role->getId(), skipIfExists: false);
+			$output->writeln("<info>Successfully assigned role '{$role->getName()}' to user '$username'.</info>");
+			return 0;
+		} catch (\Exception $ex) {
+			$output->writeln("<error>Failed to assign role '{$role->getName()}' to user '$username': {$ex->getMessage()}</error>");
+			return 1;
+		}
 	}
 }
diff --git a/lib/Controller/UserRoleController.php b/lib/Controller/UserRoleController.php
index ac040b9..def3327 100644
--- a/lib/Controller/UserRoleController.php
+++ b/lib/Controller/UserRoleController.php
@@ -9,6 +9,7 @@ namespace OCA\Forum\Controller;
 
 use OCA\Forum\Attribute\RequirePermission;
 use OCA\Forum\Db\UserRoleMapper;
+use OCA\Forum\Service\UserRoleService;
 use OCP\AppFramework\Db\DoesNotExistException;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\Attribute\ApiRoute;
@@ -23,6 +24,7 @@ class UserRoleController extends OCSController {
 		string $appName,
 		IRequest $request,
 		private UserRoleMapper $userRoleMapper,
+		private UserRoleService $userRoleService,
 		private LoggerInterface $logger,
 	) {
 		parent::__construct($appName, $request);
@@ -75,22 +77,29 @@ class UserRoleController extends OCSController {
 	 *
 	 * @param string $userId Nextcloud user ID
 	 * @param int $roleId Role ID
-	 * @return DataResponse<Http::STATUS_CREATED, array<string, mixed>, array{}>
+	 * @return DataResponse<Http::STATUS_CREATED, array<string, mixed>, array{}>|DataResponse<Http::STATUS_CONFLICT, array{error: string}, array{}>
 	 *
 	 * 201: Role assigned to user
+	 * 409: User already has this role
 	 */
 	#[NoAdminRequired]
 	#[RequirePermission('canEditRoles')]
 	#[ApiRoute(verb: 'POST', url: '/api/user-roles')]
 	public function create(string $userId, int $roleId): DataResponse {
 		try {
-			$userRole = new \OCA\Forum\Db\UserRole();
-			$userRole->setUserId($userId);
-			$userRole->setRoleId($roleId);
-			$userRole->setCreatedAt(time());
+			// Check if user already has the role
+			if ($this->userRoleService->hasRole($userId, $roleId)) {
+				return new DataResponse(['error' => 'User already has this role'], Http::STATUS_CONFLICT);
+			}
+
+			// Assign the role using the service
+			$createdUserRole = $this->userRoleService->assignRole($userId, $roleId, skipIfExists: false);
+
+			if ($createdUserRole === null) {
+				// This shouldn't happen since we checked hasRole above, but handle it just in case
+				return new DataResponse(['error' => 'Failed to assign role'], Http::STATUS_INTERNAL_SERVER_ERROR);
+			}
 
-			/** @var \OCA\Forum\Db\UserRole */
-			$createdUserRole = $this->userRoleMapper->insert($userRole);
 			return new DataResponse($createdUserRole->jsonSerialize(), Http::STATUS_CREATED);
 		} catch (\Exception $e) {
 			$this->logger->error('Error assigning role to user: ' . $e->getMessage());