[#40969] show correct error message in files tab

https://community.openproject.org/work_packages/40969 Signed-off-by: Artur Neumann <artur@jankaritech.com>
nextcloud · Mar 9, 2022 · a666389 · a666389
1 parent d713464
commit a666389
Show file tree

Hide file tree

Showing 8 changed files with 345 additions and 63 deletions.
diff --git a/lib/Controller/OpenProjectAPIController.php b/lib/Controller/OpenProjectAPIController.php
@@ -129,20 +129,36 @@ public function getNotifications(?string $since = null): DataResponse {
 	 * @NoAdminRequired
 	 *
 	 * @param ?string $searchQuery
+	 * @param ?int $fileId
 	 *
 	 * @return DataResponse
 	 */
-	public function getSearchedWorkPackages(?string $searchQuery = null): DataResponse {
+	public function getSearchedWorkPackages(?string $searchQuery = null, ?int $fileId = null): DataResponse {
 		if ($this->accessToken === '' || !OpenProjectAPIService::validateOpenProjectURL($this->openprojectUrl)) {
-			return new DataResponse('', Http::STATUS_BAD_REQUEST);
+			return new DataResponse(
+				'invalid open project configuration', Http::STATUS_UNAUTHORIZED
+			);
 		}
 		$result = $this->openprojectAPIService->searchWorkPackage(
-			$this->openprojectUrl, $this->accessToken, $this->refreshToken, $this->clientID, $this->clientSecret, $this->userId, $searchQuery
+			$this->openprojectUrl,
+			$this->accessToken,
+			$this->refreshToken,
+			$this->clientID,
+			$this->clientSecret,
+			$this->userId,
+			$searchQuery,
+			$fileId
 		);
+
 		if (!isset($result['error'])) {
 			$response = new DataResponse($result);
 		} else {
-			$response = new DataResponse($result, Http::STATUS_UNAUTHORIZED);
+			if (isset($result['statusCode'])) {
+				$statusCode = $result['statusCode'];
+			} else {
+				$statusCode = Http::STATUS_BAD_REQUEST;
+			}
+			$response = new DataResponse($result, $statusCode);
 		}
 		return $response;
 	}

diff --git a/lib/Service/OpenProjectAPIService.php b/lib/Service/OpenProjectAPIService.php
@@ -238,22 +238,28 @@ public function getNotifications(string $url, string $accessToken,
 	 * @param string $clientID
 	 * @param string $clientSecret
 	 * @param string $userId
-	 * @param string $query
+	 * @param string|null $query
+	 * @param int|null $fileId
 	 * @param int $offset
 	 * @param int $limit
-	 * @return array<string>
+	 * @return array<mixed>
+	 * @throws \OCP\PreConditionNotMetException
 	 * @throws \Safe\Exceptions\JsonException
 	 */
 	public function searchWorkPackage(string $url, string $accessToken,
 							string $refreshToken, string $clientID, string $clientSecret, string $userId,
-							string $query, int $offset = 0, int $limit = 5): array {
+							string $query = null, int $fileId = null, int $offset = 0, int $limit = 5): array {
 		$resultsById = [];
+		$filters = [];
 
 		// search by description
-		$filters = [
-			["description" => ["operator" => "~", "values" => [$query]]],
-			["status" => ["operator" => "!", "values" => ["14"]]]
-		];
+		if ($fileId !== null) {
+			$filters[] = ['file_link_origin_id' => ['operator' => '=', 'values' => [(string)$fileId]]];
+		}
+		if ($query !== null) {
+			$filters[] = ['description' => ['operator' => '~', 'values' => [$query]]];
+			$filters[] = ['status' => ['operator' => '!', 'values' => ['14']]];
+		}
 		$params = [
 			'filters' => \Safe\json_encode($filters),
 			'sortBy' => '[["updatedAt", "desc"]]',
@@ -263,28 +269,38 @@ public function searchWorkPackage(string $url, string $accessToken,
 			$url, $accessToken, $refreshToken, $clientID, $clientSecret, $userId, 'work_packages', $params
 		);
 
+		if (isset($searchDescResult['error'])) {
+			return $searchDescResult;
+		}
+
 		if (isset($searchDescResult['_embedded'], $searchDescResult['_embedded']['elements'])) {
 			foreach ($searchDescResult['_embedded']['elements'] as $wp) {
 				$resultsById[$wp['id']] = $wp;
 			}
 		}
 		// search by subject
-		$filters = [
-			["subject" => ["operator" => "~", "values" => [$query]]],
-			["status" => ["operator" => "!", "values" => ["14"]]]
-		];
-		$params = [
-			'filters' => \Safe\json_encode($filters),
-			'sortBy' => '[["updatedAt", "desc"]]',
-			// 'limit' => $limit,
-		];
-		$searchSubjectResult = $this->request(
+		if ($query !== null) {
+			$filters = [
+				['subject' => ['operator' => '~', 'values' => [$query]]],
+				['status' => ['operator' => '!', 'values' => ['14']]]
+			];
+			$params = [
+				'filters' => \Safe\json_encode($filters),
+				'sortBy' => '[["updatedAt", "desc"]]',
+				// 'limit' => $limit,
+			];
+			$searchSubjectResult = $this->request(
 			$url, $accessToken, $refreshToken, $clientID, $clientSecret, $userId, 'work_packages', $params
 		);
 
-		if (isset($searchSubjectResult['_embedded'], $searchSubjectResult['_embedded']['elements'])) {
-			foreach ($searchSubjectResult['_embedded']['elements'] as $wp) {
-				$resultsById[$wp['id']] = $wp;
+			if (isset($searchSubjectResult['error'])) {
+				return $searchSubjectResult;
+			}
+
+			if (isset($searchSubjectResult['_embedded'], $searchSubjectResult['_embedded']['elements'])) {
+				foreach ($searchSubjectResult['_embedded']['elements'] as $wp) {
+					$resultsById[$wp['id']] = $wp;
+				}
 			}
 		}
 
@@ -386,14 +402,7 @@ public function request(string $openprojectUrl, string $accessToken, string $ref
 			} else {
 				return ['error' => $this->l10n->t('Bad HTTP method')];
 			}
-			$body = $response->getBody();
-			$respCode = $response->getStatusCode();
-
-			if ($respCode >= 400) {
-				return ['error' => $this->l10n->t('Bad credentials')];
-			} else {
-				return json_decode($body, true);
-			}
+			return json_decode($response->getBody(), true);
 		} catch (ServerException | ClientException $e) {
 			$response = $e->getResponse();
 			$body = (string) $response->getBody();
@@ -425,13 +434,20 @@ public function request(string $openprojectUrl, string $accessToken, string $ref
 			}
 			return [
 				'error' => $e->getMessage(),
-				'statusCode' => $e->getResponse()->getStatusCode(),
+				'statusCode' => $response->getStatusCode(),
 			];
-		} catch (ConnectException | Exception $e) {
+		} catch (ConnectException $e) {
+			$this->logger->warning('OpenProject connection error : '.$e->getMessage(), ['app' => $this->appName]);
 			return [
 				'error' => $e->getMessage(),
 				'statusCode' => 404,
 			];
+		} catch (Exception $e) {
+			$this->logger->critical('OpenProject error : '.$e->getMessage(), ['app' => $this->appName]);
+			return [
+				'error' => $e->getMessage(),
+				'statusCode' => 500,
+			];
 		}
 	}
 

diff --git a/src/components/tab/EmptyContent.vue b/src/components/tab/EmptyContent.vue
@@ -33,6 +33,10 @@ export default {
 			type: String,
 			required: true,
 		},
+		errorMessage: {
+			type: String,
+			default: '',
+		},
 	},
 	data() {
 		return {
@@ -49,8 +53,12 @@ export default {
 		emptyContentMessage() {
 			if (this.state === 'no-token') {
 				return t('integration_openproject', 'No OpenProject account connected')
-			} else if (this.state === 'error') {
+			} else if (this.state === 'connection-error') {
 				return t('integration_openproject', 'Error connecting to OpenProject')
+			} else if (this.state === 'failed-fetching-workpackages') {
+				return t('integration_openproject', 'Could not fetch work packages from OpenProject')
+			} else if (this.state === 'error') {
+				return t('integration_openproject', 'Unexpected Error')
 			} else if (this.state === 'ok') {
 				return t('integration_openproject', 'No workspaces linked yet, search for work package to add!')
 			}

diff --git a/src/views/ProjectsTab.vue b/src/views/ProjectsTab.vue
@@ -77,19 +77,23 @@ export default {
 
 		async fetchWorkpackages(fileId) {
 			const req = {}
-			const url = generateUrl('/apps/integration_openproject/workpackages/' + fileId)
+			const url = generateUrl('/apps/integration_openproject/work_packages?fileId=' + fileId)
 			try {
 				const response = await axios.get(url, req)
 				if (!Array.isArray(response.data)) {
-					this.state = 'error'
+					this.state = 'failed-fetching-workpackages'
 				} else {
 					this.state = 'ok'
 				}
 			} catch (error) {
 				if (error.response && error.response.status === 401) {
 					this.state = 'no-token'
-				} else {
+				} else if (error.response.status === 404) {
+					this.state = 'connection-error'
+				} else if (error.response.status === 500) {
 					this.state = 'error'
+				} else {
+					this.state = 'failed-fetching-workpackages'
 				}
 			}
 		},

diff --git a/tests/jest/components/tab/EmptyContent.spec.js b/tests/jest/components/tab/EmptyContent.spec.js
@@ -3,6 +3,15 @@ import { shallowMount, createLocalVue } from '@vue/test-utils'
 import EmptyContent from '../../../../src/components/tab/EmptyContent'
 const localVue = createLocalVue()
 
+function getWrapper(propsData = {}) {
+	return shallowMount(EmptyContent, {
+		localVue,
+		mocks: {
+			t: (msg) => msg,
+		},
+		propsData,
+	})
+}
 describe('EmptyContent.vue Test', () => {
 	let wrapper
 	const emptyContentMessageSelector = '.title'
@@ -28,14 +37,21 @@ describe('EmptyContent.vue Test', () => {
 				requestUrl: 'http://openproject/oauth/',
 			},
 		})
+		wrapper = getWrapper({ state: cases.state })
 		expect(wrapper.find(connectButtonSelector).exists()).toBe(cases.viewed)
 	})
 	it.each([{
 		state: 'no-token',
 		message: 'No OpenProject account connected',
 	}, {
 		state: 'error',
+		message: 'Unexpected Error',
+	}, {
+		state: 'connection-error',
 		message: 'Error connecting to OpenProject',
+	}, {
+		state: 'failed-fetching-workpackages',
+		message: 'Could not fetch work packages from OpenProject',
 	}, {
 		state: 'ok',
 		message: 'No workspaces linked yet',
@@ -53,6 +69,7 @@ describe('EmptyContent.vue Test', () => {
 				requestUrl: 'http://openproject/oauth/',
 			},
 		})
+		wrapper = getWrapper({ state: cases.state })
 		expect(wrapper.find(emptyContentMessageSelector).exists()).toBeTruthy()
 		expect(wrapper.find(emptyContentMessageSelector).text()).toMatch(cases.message)
 	})

diff --git a/tests/jest/views/ProjectsTab.spec.js b/tests/jest/views/ProjectsTab.spec.js
@@ -45,23 +45,10 @@ describe('ProjectsTab.vue Test', () => {
 	})
 	describe('fetchWorkpackages', () => {
 		it.each([
-			{ responseData: [], AppState: 'ok' },
-			{ responseData: 'string', AppState: 'error' },
-			{ responseData: null, AppState: 'error' },
-			{ responseData: undefined, AppState: 'error' },
-		])('sets states according to response content in case of success', async (cases) => {
-			axios.get.mockImplementationOnce(() =>
-				Promise.resolve({
-					data: cases.responseData,
-				}),
-			)
-			await wrapper.vm.update({ id: 123 })
-			expect(wrapper.vm.state).toBe(cases.AppState)
-		})
-		it.each([
-			{ HTTPStatus: 400, AppState: 'error' },
+			{ HTTPStatus: 400, AppState: 'failed-fetching-workpackages' },
 			{ HTTPStatus: 401, AppState: 'no-token' },
-			{ HTTPStatus: 404, AppState: 'error' },
+			{ HTTPStatus: 402, AppState: 'failed-fetching-workpackages' },
+			{ HTTPStatus: 404, AppState: 'connection-error' },
 			{ HTTPStatus: 500, AppState: 'error' },
 		])('sets states according to HTTP error codes', async (cases) => {
 			const err = new Error()

diff --git a/tests/lib/Controller/OpenProjectAPIControllerTest.php b/tests/lib/Controller/OpenProjectAPIControllerTest.php
@@ -176,43 +176,69 @@ public function testGetOpenProjectAvatarNoType() {
 	}
 
 	/**
+	 * @return array<mixed>
+	 */
+	public function searchWorkPackagesDataProvider() {
+		return [
+			['test', null, [['id' => 1], ['id' => 2], ['id' => 3], ['id' => 4], ['id' => 5]]],
+			['test', 9090, [['id' => 1], ['id' => 2], ['id' => 3], ['id' => 4], ['id' => 5]]],
+			[null, 9090, [['id' => 1], ['id' => 2], ['id' => 3], ['id' => 4], ['id' => 5]]]
+		];
+	}
+
+	/**
+	 * @param string|null $searchQuery
+	 * @param int|null $fileId
+	 * @param array<mixed> $expectedResponse
 	 * @return void
+	 * @dataProvider searchWorkPackagesDataProvider
 	 */
-	public function testGetSearchedWorkPackages():void {
+	public function testGetSearchedWorkPackages($searchQuery, $fileId, array $expectedResponse):void {
 		$this->getUserValueMock();
 		$service = $this->getMockBuilder(OpenProjectAPIService::class)
 			->disableOriginalConstructor()
 			->onlyMethods(['searchWorkPackage'])
 			->getMock();
 		$service->expects($this->once())
 			->method('searchWorkPackage')
-			->willReturn([['id' => 1], ['id' => 2], ['id' => 3], ['id' => 4], ['id' => 5]]);
+			->with(
+				$this->anything(),
+				$this->anything(),
+				$this->anything(),
+				$this->anything(),
+				$this->anything(),
+				$this->anything(),
+				$searchQuery,
+				$fileId
+			)
+			->willReturn($expectedResponse);
 
 		$controller = new OpenProjectAPIController(
 			'integration_openproject', $this->requestMock, $this->configMock, $service, 'test'
 		);
-		$response = $controller->getSearchedWorkPackages('test');
+		$response = $controller->getSearchedWorkPackages($searchQuery, $fileId);
 		$this->assertSame(Http::STATUS_OK, $response->getStatus());
-		$this->assertSame([['id' => 1], ['id' => 2], ['id' => 3], ['id' => 4], ['id' => 5]], $response->getData());
+		$this->assertSame($expectedResponse, $response->getData());
 	}
 
+
 	/**
 	 * @return void
 	 */
-	public function testGetSearchedWorkPackagesErrorResponse(): void {
+	public function testGetSearchedWorkPackagesNoAccessToken(): void {
 		$this->getUserValueMock('');
 		$service = $this->createMock(OpenProjectAPIService::class);
 		$controller = new OpenProjectAPIController(
 			'integration_openproject', $this->requestMock, $this->configMock, $service, 'test'
 		);
 		$response = $controller->getSearchedWorkPackages('test');
-		$this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus());
+		$this->assertSame(Http::STATUS_UNAUTHORIZED, $response->getStatus());
 	}
 
 	/**
 	 * @return void
 	 */
-	public function testGetSearchedWorkPackagesNoAccessToken(): void {
+	public function testGetSearchedWorkPackagesErrorResponse(): void {
 		$this->getUserValueMock();
 		$service = $this->getMockBuilder(OpenProjectAPIService::class)
 			->disableOriginalConstructor()
@@ -225,7 +251,7 @@ public function testGetSearchedWorkPackagesNoAccessToken(): void {
 			'integration_openproject', $this->requestMock, $this->configMock, $service, 'test'
 		);
 		$response = $controller->getSearchedWorkPackages('test');
-		$this->assertSame(Http::STATUS_UNAUTHORIZED, $response->getStatus());
+		$this->assertSame(Http::STATUS_BAD_REQUEST, $response->getStatus());
 		$this->assertSame(['error' => 'something went wrong'], $response->getData());
 	}