address reviews

Signed-off-by: Swikriti Tripathi <swikriti808@gmail.com>
nextcloud · Oct 18, 2023 · 7aada3a · 7aada3a
1 parent 9e82f20
commit 7aada3a
Show file tree

Hide file tree

Showing 4 changed files with 64 additions and 30 deletions.
diff --git a/lib/Controller/OpenProjectAPIController.php b/lib/Controller/OpenProjectAPIController.php
@@ -412,7 +412,7 @@ public function getAvailableOpenProjectProjects(): DataResponse {
 	 *                                }
 	 *                                }
 	 *                           See POST request for create work package https://www.openproject.org/docs/api/endpoints/work-packages/
-	 * 							 Note that this api will send `200` even with empty body
+	 * 							 Note that this api will send `200` even with empty body and the body content is similar to that of create workpackages
 	 * @return DataResponse
 	 */
 	public function getOpenProjectWorkPackageForm(string $projectId, array $body): DataResponse {
@@ -492,6 +492,12 @@ public function createWorkPackage(array $body): DataResponse {
 		} elseif (!OpenProjectAPIService::validateURL($this->openprojectUrl)) {
 			return new DataResponse('', Http::STATUS_BAD_REQUEST);
 		}
+		// we don't want to check if all the data in the body is set or not because
+		// that calculation will be done by the openproject api itself
+		// we don't want to duplicate the logic
+		if (empty($body)) {
+			return new DataResponse('Body cannot be empty', Http::STATUS_BAD_REQUEST);
+		}
 		try {
 			$result = $this->openprojectAPIService->createWorkPackage($this->userId, $body);
 		} catch (OpenprojectErrorException $e) {

diff --git a/lib/Service/OpenProjectAPIService.php b/lib/Service/OpenProjectAPIService.php
@@ -1323,7 +1323,7 @@ public function getAvailableOpenProjectProjects(string $userId): array {
 	 * @throws OpenprojectErrorException
 	 * @throws OpenprojectResponseException
 	 */
-	private function getProjectStorages(string $userId, string $endpoint): bool {
+	public function getProjectStorages(string $userId, string $endpoint): bool {
 		$result = $this->request($userId, $endpoint);
 		if (isset($result['error'])) {
 			throw new OpenprojectErrorException($result['error'], $result['statusCode']);

diff --git a/tests/lib/Controller/OpenProjectAPIControllerTest.php b/tests/lib/Controller/OpenProjectAPIControllerTest.php
@@ -1541,8 +1541,7 @@ public function testGetOpenProjectWorkPackageFormEmptyBody(): void {
 			'test'
 		);
 		$response = $controller->getOpenProjectWorkPackageForm('6', []);
-		$this->assertSame(HTTP::STATUS_BAD_REQUEST, $response->getStatus());
-		$this->assertSame('Body cannot be empty', $response->getData());
+		$this->assertSame(HTTP::STATUS_OK, $response->getStatus());
 	}
 
 	/**
@@ -1690,10 +1689,10 @@ public function testCreateWorkpackages(): void {
 		];
 		$service = $this->getMockBuilder(OpenProjectAPIService::class)
 			->disableOriginalConstructor()
-			->onlyMethods(['createWorkPackages'])
+			->onlyMethods(['createWorkPackage'])
 			->getMock();
 		$service->expects($this->once())
-			->method('createWorkPackages')
+			->method('createWorkPackage')
 			->with('test', $body)
 			->willReturn($response);
 
@@ -1706,7 +1705,7 @@ public function testCreateWorkpackages(): void {
 			$this->loggerMock,
 			'test'
 		);
-		$result = $controller->createWorkPackages($body);
+		$result = $controller->createWorkPackage($body);
 		$this->assertSame(Http::STATUS_CREATED, $result->getStatus());
 		$this->assertSame(
 			$response, $result->getData());
@@ -1756,7 +1755,7 @@ public function testCreateWorkpackagesException(
 			->disableOriginalConstructor()
 			->getMock();
 		$service
-			->method('createWorkPackages')
+			->method('createWorkPackage')
 			->willThrowException($exception);
 		$controller = new OpenProjectAPIController(
 			'integration_openproject',
@@ -1767,7 +1766,7 @@ public function testCreateWorkpackagesException(
 			$this->loggerMock,
 			'test'
 		);
-		$response = $controller->createWorkPackages($body);
+		$response = $controller->createWorkPackage($body);
 		$this->assertSame($expectedHttpStatusCode, $response->getStatus());
 		$this->assertSame($expectedError, $response->getData());
 	}
@@ -1789,7 +1788,7 @@ public function testCreateWorkpackagesEmptyBody(): void {
 			$this->loggerMock,
 			'test'
 		);
-		$response = $controller->createWorkPackages([]);
+		$response = $controller->createWorkPackage([]);
 		$this->assertSame(HTTP::STATUS_BAD_REQUEST, $response->getStatus());
 		$this->assertSame('Body cannot be empty', $response->getData());
 	}

diff --git a/tests/lib/Service/OpenProjectAPIServiceTest.php b/tests/lib/Service/OpenProjectAPIServiceTest.php
@@ -303,6 +303,12 @@ class OpenProjectAPIServiceTest extends TestCase {
 						],
 						"parent" => [
 							"href" => null
+						],
+						"storages" => [
+							[
+								"href" => "/api/v3/storages/37",
+								"title" => "nc-26"
+							]
 						]
 					]
 				]
@@ -481,6 +487,24 @@ class OpenProjectAPIServiceTest extends TestCase {
 		]
 	];
 
+
+	private $validStoragesResponse = [
+		"_type" => "Storage",
+		"id" => 37,
+		"name" => "nc-26",
+		"_embedded" => [
+			"oauthApplication" => [
+				"_type" => "OAuthApplication",
+				"id" => 56,
+			]
+		],
+		"_links" => [
+			"origin" => [
+				"href" => "https://nc.my-server.org",
+			]
+		]
+	];
+
 	/**
 	 * @return void
 	 * @before
@@ -3090,22 +3114,6 @@ public function testGetWorkPackageFileDeleteLinkNotFoundPact(): void {
 	 */
 	public function testGetAvailableOpenProjectProjectsPact(): void {
 		$expectedResult = [
-			6 => [
-				"_type" => "Project",
-				"id" => 6,
-				"identifier" => "dev-custom-fields",
-				"name" => "[dev] Custom fields",
-				"_links" => [
-					"self" => [
-						"href" => "/api/v3/projects/6",
-						"title" => "[dev] Custom fields"
-					],
-					"parent" => [
-						"href" => "/api/v3/projects/5",
-						"title" => "[dev] Large"
-					]
-				]
-			],
 			5 => [
 				"_type" => "Project",
 				"id" => 5,
@@ -3118,6 +3126,12 @@ public function testGetAvailableOpenProjectProjectsPact(): void {
 					],
 					"parent" => [
 						"href" => null
+					],
+					"storages" => [
+						[
+							"href" => "/api/v3/storages/37",
+							"title" => "nc-26"
+						]
 					]
 				]
 			]
@@ -3128,16 +3142,31 @@ public function testGetAvailableOpenProjectProjectsPact(): void {
 			->setPath($this->getProjectsPath)
 			->setHeaders(["Authorization" => "Bearer 1234567890"]);
 
+		$consumerRequestStorage = new ConsumerRequest();
+		$consumerRequestStorage
+			->setMethod('GET')
+			->setPath('/api/v3/storages/37')
+			->setHeaders(["Authorization" => "Bearer new-Token"]);
+
 		$providerResponse = new ProviderResponse();
 		$providerResponse
 			->setStatus(Http::STATUS_OK)
 			->addHeader('Content-Type', 'application/json')
 			->setBody($this->validOpenProjectGetProjectsResponse);
+		$providerResponseStorage = new ProviderResponse();
+		$providerResponseStorage
+			->setStatus(Http::STATUS_OK)
+			->addHeader('Content-Type', 'application/json')
+			->setBody($this->validStoragesResponse);
 
 		$this->builder
 			->uponReceiving('a GET request to /work_packages/available_projects')
 			->with($consumerRequest)
 			->willRespondWith($providerResponse);
+		$this->builder
+			->uponReceiving('a GET request to storages')
+			->with($consumerRequestStorage)
+			->willRespondWith($providerResponseStorage);
 		$storageMock = $this->getStorageMock();
 		$service = $this->getOpenProjectAPIService($storageMock);
 		$result = $service->getAvailableOpenProjectProjects('testUser');
@@ -3149,7 +3178,7 @@ public function testGetAvailableOpenProjectProjectsPact(): void {
 	 * @param array<mixed> $response
 	 * @return void
 	 */
-	public function testGetAvailableOpenProjectProjectsMalformedResponse($response): void {
+	public function testGetAvailableOpenProjectProjectsMalformedResponse(array $response): void {
 		$service = $this->getServiceMock();
 		$service->method('request')
 			->willReturn($response);
@@ -3318,7 +3347,7 @@ public function testCreateWorkpackagePact(): void {
 			->willRespondWith($providerResponse);
 		$storageMock = $this->getStorageMock();
 		$service = $this->getOpenProjectAPIService($storageMock);
-		$result = $service->createWorkPackages('testUser', $this->validCreateWorkpackageBody);
+		$result = $service->createWorkPackage('testUser', $this->validCreateWorkpackageBody);
 		$this->assertSame($this->createWorkpackageResponse, $result);
 	}
 
@@ -3342,7 +3371,7 @@ public function testCreateWorkpackagesMalformedResponse($response) {
 		$service->method('request')
 			->willReturn($response);
 		$this->expectException(OpenprojectResponseException::class);
-		$service->createWorkPackages('testUser', $this->validCreateWorkpackageBody);
+		$service->createWorkPackage('testUser', $this->validCreateWorkpackageBody);
 	}
 
 	/**
@@ -3353,6 +3382,6 @@ public function testCreateWorkpackagesErrorResponse(): void {
 		$service->method('request')
 			->willReturn(['error' => 'something went wrong', 'statusCode' => 500]);
 		$this->expectException(OpenprojectErrorException::class);
-		$service->createWorkPackages('testUser', $this->validCreateWorkpackageBody);
+		$service->createWorkPackage('testUser', $this->validCreateWorkpackageBody);
 	}
 }